-
Notifications
You must be signed in to change notification settings - Fork 577
refactor(server): unify URL configs when scheme is missing #2944
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
85dc7db
8dff246
575a642
a1b42cd
20d9cf2
f3b01e7
67a0c9d
358d037
09c4fc0
d1de457
fb5459c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -43,6 +43,9 @@ public class HugeConfig extends PropertiesConfiguration { | |||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| private static final Logger LOG = Log.logger(HugeConfig.class); | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| // Cache for URL normalization metadata (populated lazily per key) | ||||||||||||||||||||||||||||||||||||
| private static final Map<String, String> URL_NORMALIZATIONS = new HashMap<>(); | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| private String configPath; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| public HugeConfig(Configuration config) { | ||||||||||||||||||||||||||||||||||||
|
|
@@ -87,9 +90,17 @@ private void setLayoutIfNeeded(Configuration conf) { | |||||||||||||||||||||||||||||||||||
| @SuppressWarnings("unchecked") | ||||||||||||||||||||||||||||||||||||
| public <T, R> R get(TypedOption<T, R> option) { | ||||||||||||||||||||||||||||||||||||
| Object value = this.getProperty(option.name()); | ||||||||||||||||||||||||||||||||||||
| boolean fromDefault = false; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if (value == null) { | ||||||||||||||||||||||||||||||||||||
| return option.defaultValue(); | ||||||||||||||||||||||||||||||||||||
| value = option.defaultValue(); | ||||||||||||||||||||||||||||||||||||
| fromDefault = true; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if (!fromDefault) { | ||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line 94-96 shows normalization only applies to explicitly set values ( This creates inconsistent behavior:
Question: Is this intentional? If defaults already have schemes, why do we need
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is intentional. |
||||||||||||||||||||||||||||||||||||
| value = normalizeUrlOptionIfNeeded(option.name(), value); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| return (R) value; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
|
@@ -213,4 +224,86 @@ private static Configuration loadConfigFile(File configFile) { | |||||||||||||||||||||||||||||||||||
| e, configFile); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| private static Object normalizeUrlOptionIfNeeded(String key, Object value) { | ||||||||||||||||||||||||||||||||||||
| if (value == null) { | ||||||||||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
bitflicker64 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| String scheme = defaultSchemeFor(key); | ||||||||||||||||||||||||||||||||||||
| if (scheme == null) { | ||||||||||||||||||||||||||||||||||||
| return value; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| // Normalize URL options if configured with .withUrlNormalization() | ||||||||||||||||||||||||||||||||||||
| if (value instanceof String) { | ||||||||||||||||||||||||||||||||||||
| String original = (String) value; | ||||||||||||||||||||||||||||||||||||
| String normalized = prefixSchemeIfMissing(original, scheme); | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if (!original.equals(normalized)) { | ||||||||||||||||||||||||||||||||||||
| LOG.warn("Config '{}' is missing scheme, auto-corrected to '{}'", | ||||||||||||||||||||||||||||||||||||
| key, normalized); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| return normalized; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| // If it ever hits here, it means config storage returned a non-string type; | ||||||||||||||||||||||||||||||||||||
| // leave it unchanged (safer than forcing toString()). | ||||||||||||||||||||||||||||||||||||
| return value; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| private static String defaultSchemeFor(String key) { | ||||||||||||||||||||||||||||||||||||
bitflicker64 marked this conversation as resolved.
Show resolved
Hide resolved
bitflicker64 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||
| // Check if we already cached this key's scheme | ||||||||||||||||||||||||||||||||||||
| if (URL_NORMALIZATIONS.containsKey(key)) { | ||||||||||||||||||||||||||||||||||||
| return URL_NORMALIZATIONS.get(key); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| // We don't know yet - look it up NOW from OptionSpace | ||||||||||||||||||||||||||||||||||||
| synchronized (URL_NORMALIZATIONS) { | ||||||||||||||||||||||||||||||||||||
| // Double-check after acquiring lock | ||||||||||||||||||||||||||||||||||||
| if (URL_NORMALIZATIONS.containsKey(key)) { | ||||||||||||||||||||||||||||||||||||
| return URL_NORMALIZATIONS.get(key); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| // Look up the option from OptionSpace | ||||||||||||||||||||||||||||||||||||
| TypedOption<?, ?> option = OptionSpace.get(key); | ||||||||||||||||||||||||||||||||||||
| String scheme = null; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if (option instanceof ConfigOption) { | ||||||||||||||||||||||||||||||||||||
| ConfigOption<?> configOption = (ConfigOption<?>) option; | ||||||||||||||||||||||||||||||||||||
| if (configOption.needsUrlNormalization()) { | ||||||||||||||||||||||||||||||||||||
| scheme = configOption.getDefaultScheme(); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| // Cache it for next time (even if null) | ||||||||||||||||||||||||||||||||||||
| URL_NORMALIZATIONS.put(key, scheme); | ||||||||||||||||||||||||||||||||||||
| return scheme; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| private static String prefixSchemeIfMissing(String raw, String scheme) { | ||||||||||||||||||||||||||||||||||||
| if (raw == null) { | ||||||||||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| String s = raw.trim(); | ||||||||||||||||||||||||||||||||||||
| if (s.isEmpty()) { | ||||||||||||||||||||||||||||||||||||
| return s; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| int scIdx = s.indexOf("://"); | ||||||||||||||||||||||||||||||||||||
| if (scIdx > 0) { | ||||||||||||||||||||||||||||||||||||
| // Normalize existing scheme to lowercase while preserving the rest | ||||||||||||||||||||||||||||||||||||
| String existingScheme = s.substring(0, scIdx).toLowerCase(); | ||||||||||||||||||||||||||||||||||||
| String rest = s.substring(scIdx + 3); // skip the "://" delimiter | ||||||||||||||||||||||||||||||||||||
| return existingScheme + "://" + rest; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current implementation doesn't handle URLs with credentials or userinfo: // These would incorrectly get prefixed:
"user:password@127.0.0.1:8080" → "http://user:password@127.0.0.1:8080"
"admin@localhost:8080" → "http://admin@localhost:8080"While valid according to RFC 3986 (
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The prefixSchemeIfMissing() method currently just normalizes URLs and doesn’t try to validate things like userinfo (user@host). That seems reasonable since handling credentials in URLs is discouraged and @ can also appear in valid paths. Adding special checks here would blur the line between normalization and validation.It’s probably better to handle stricter validation at the client/usage layer if needed.I’m happy to add a separate check or open an additional PR if you think stricter handling belongs here. Please let me know your preference. |
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| String defaultScheme = scheme == null ? "" : scheme; | ||||||||||||||||||||||||||||||||||||
| if (!defaultScheme.isEmpty() && !defaultScheme.endsWith("://")) { | ||||||||||||||||||||||||||||||||||||
| defaultScheme = defaultScheme + "://"; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| return defaultScheme + s; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,122 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with this | ||
| * work for additional information regarding copyright ownership. The ASF | ||
| * licenses this file to You under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||
| * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the | ||
| * License for the specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package org.apache.hugegraph.unit.config; | ||
|
|
||
| import org.apache.commons.configuration2.PropertiesConfiguration; | ||
| import org.apache.hugegraph.config.HugeConfig; | ||
| import org.apache.hugegraph.config.OptionSpace; | ||
| import org.apache.hugegraph.config.ServerOptions; | ||
| import org.apache.hugegraph.testutil.Assert; | ||
| import org.junit.BeforeClass; | ||
| import org.junit.Test; | ||
|
|
||
| public class ServerOptionsTest { | ||
|
|
||
| @BeforeClass | ||
| public static void init() { | ||
| OptionSpace.register("server", | ||
| ServerOptions.class.getName()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testUrlOptionNormalizeAddsDefaultScheme() { | ||
| PropertiesConfiguration conf = new PropertiesConfiguration(); | ||
| conf.setProperty("restserver.url", "127.0.0.1:8080"); | ||
| conf.setProperty("gremlinserver.url", "127.0.0.1:8182"); | ||
| conf.setProperty("server.urls_to_pd", "0.0.0.0:8080"); | ||
| conf.setProperty("server.k8s_url", "127.0.0.1:8888"); | ||
|
|
||
| HugeConfig config = new HugeConfig(conf); | ||
|
|
||
| Assert.assertEquals("http://127.0.0.1:8080", | ||
| config.get(ServerOptions.REST_SERVER_URL)); | ||
| Assert.assertEquals("http://127.0.0.1:8182", | ||
| config.get(ServerOptions.GREMLIN_SERVER_URL)); | ||
| Assert.assertEquals("http://0.0.0.0:8080", | ||
| config.get(ServerOptions.SERVER_URLS_TO_PD)); | ||
| Assert.assertEquals("https://127.0.0.1:8888", | ||
| config.get(ServerOptions.SERVER_K8S_URL)); | ||
| } | ||
bitflicker64 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| @Test | ||
| public void testUrlNormalizationEdgeCases() { | ||
| // Whitespace trimming | ||
| PropertiesConfiguration conf = new PropertiesConfiguration(); | ||
| conf.setProperty("restserver.url", " 127.0.0.1:8080 "); | ||
| HugeConfig config = new HugeConfig(conf); | ||
| Assert.assertEquals("http://127.0.0.1:8080", | ||
| config.get(ServerOptions.REST_SERVER_URL)); | ||
|
|
||
| // Case normalization | ||
| conf = new PropertiesConfiguration(); | ||
| conf.setProperty("restserver.url", "HTTP://127.0.0.1:8080"); | ||
| config = new HugeConfig(conf); | ||
| Assert.assertEquals("http://127.0.0.1:8080", | ||
| config.get(ServerOptions.REST_SERVER_URL)); | ||
bitflicker64 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // IPv6 without scheme | ||
| conf = new PropertiesConfiguration(); | ||
| conf.setProperty("restserver.url", "[::1]:8080"); | ||
| config = new HugeConfig(conf); | ||
| Assert.assertEquals("http://[::1]:8080", | ||
| config.get(ServerOptions.REST_SERVER_URL)); | ||
bitflicker64 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // IPv6 with existing scheme | ||
| conf = new PropertiesConfiguration(); | ||
| conf.setProperty("restserver.url", "http://[::1]:8080"); | ||
| config = new HugeConfig(conf); | ||
| Assert.assertEquals("http://[::1]:8080", | ||
| config.get(ServerOptions.REST_SERVER_URL)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testUrlNormalizationPreservesHostnameCase() { | ||
| // Uppercase scheme + mixed-case hostname | ||
| PropertiesConfiguration conf = new PropertiesConfiguration(); | ||
| conf.setProperty("restserver.url", "HTTP://MyServer:8080"); | ||
| HugeConfig config = new HugeConfig(conf); | ||
| // Should lowercase ONLY the scheme, preserve "MyServer" | ||
| Assert.assertEquals("http://MyServer:8080", | ||
| config.get(ServerOptions.REST_SERVER_URL)); | ||
|
|
||
| // Use server.k8s_url for HTTPS test (it defaults to https://) | ||
| conf = new PropertiesConfiguration(); | ||
| conf.setProperty("server.k8s_url", "HTTPS://MyHost:8888"); | ||
| config = new HugeConfig(conf); | ||
| Assert.assertEquals("https://MyHost:8888", | ||
| config.get(ServerOptions.SERVER_K8S_URL)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testUrlNormalizationPreservesPathCase() { | ||
| PropertiesConfiguration conf = new PropertiesConfiguration(); | ||
| conf.setProperty("restserver.url", "http://127.0.0.1:8080/SomePath/CaseSensitive"); | ||
| HugeConfig config = new HugeConfig(conf); | ||
| Assert.assertEquals("http://127.0.0.1:8080/SomePath/CaseSensitive", | ||
| config.get(ServerOptions.REST_SERVER_URL)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testHttpsSchemeIsNotDowngraded() { | ||
| PropertiesConfiguration conf = new PropertiesConfiguration(); | ||
| conf.setProperty("restserver.url", "https://127.0.0.1:8080"); | ||
| HugeConfig config = new HugeConfig(conf); | ||
| Assert.assertEquals("https://127.0.0.1:8080", | ||
| config.get(ServerOptions.REST_SERVER_URL)); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.