Skip to content

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.

Sign up for GitHub

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

Merged
imbajin merged 11 commits into from
Feb 3, 2026
Merged

refactor(server): unify URL configs when scheme is missing #2944

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
85dc7db
config: normalize scheme for selected URL options
bitflicker64 Jan 23, 2026
8dff246
Add optional URL scheme normalization for config options
bitflicker64 Jan 27, 2026
575a642
Add URL normalization for config options
bitflicker64 Jan 27, 2026
a1b42cd
Add URL normalization for config options
bitflicker64 Jan 27, 2026
20d9cf2
Add URL normalization for config options
bitflicker64 Jan 27, 2026
f3b01e7
Add URL normalization for config options
bitflicker64 Jan 27, 2026
67a0c9d
Add URL normalization for config options
bitflicker64 Jan 27, 2026
358d037
Move ServerOptions URL normalization tests to server test module
bitflicker64 Jan 30, 2026
09c4fc0
Merge branch 'master' into fix-url-normalization
imbajin Jan 31, 2026
d1de457
feat: add URL normalization with warning logs
bitflicker64 Feb 1, 2026
fb5459c
repair
bitflicker64 Feb 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 56 additions & 1 deletion hugegraph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/config/HugeConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,12 @@ private void setLayoutIfNeeded(Configuration conf) {
public <T, R> R get(TypedOption<T, R> option) {
Object value = this.getProperty(option.name());
if (value == null) {
return option.defaultValue();
value = option.defaultValue();
}

// Normalize URL options if needed (add scheme like http://)
value = normalizeUrlOptionIfNeeded(option.name(), value);

return (R) value;
Comment on lines 91 to 104
Copy link

Copilot AI Jan 24, 2026 •
edited by imbajin
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The URL normalization happens at retrieval time in the get() method rather than at storage time.

This means:

  1. the original unnormalized value remains in the underlying configuration storage and will be returned by direct access methods like getProperty() if they exist
  2. if the configuration is saved using save(), the unnormalized values will be written to the file, potentially causing confusion.
    Consider documenting this behavior or normalizing at storage time instead to maintain consistency.

Copilot uses AI. Check for mistakes.
}

Expand Down Expand Up @@ -213,4 +217,55 @@ private static Configuration loadConfigFile(File configFile) {
e, configFile);
}
}

private static Object normalizeUrlOptionIfNeeded(String key, Object value) {
if (value == null) {
return null;
}

String scheme = defaultSchemeFor(key);
if (scheme == null) {
return value;
}

// URL options are defined as ConfigOption<String> and normalized here.
if (value instanceof String) {
return prefixSchemeIfMissing((String) value, scheme);
}

// 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) {
switch (key) {
case "restserver.url":
case "gremlinserver.url":
case "server.urls_to_pd":
return "http://";
case "server.k8s_url":
return "https://";
default:
return null;
}
}

private static String prefixSchemeIfMissing(String raw, String scheme) {
if (raw == null) {
return null;
}
String s = raw.trim();
if (s.isEmpty()) {
return s;
}

// Keep original string if scheme already exists
String lower = s.toLowerCase();
if (lower.startsWith("http://") || lower.startsWith("https://")) {
return s;
}
Copy link
Member

@imbajin imbajin Jan 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Important: Missing Edge Case Handling

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 (scheme://[userinfo@]host[:port]), detecting these requires checking for @ before any /:

Suggested change
}
// Keep original string if scheme already exists
String lower = s.toLowerCase();
if (lower.startsWith("http://") || lower.startsWith("https://")) {
return s;
}
// Don't add scheme if userinfo is present without scheme
// (e.g., "user:pass@host:port" - likely malformed or needs manual fixing)
int slashPos = s.indexOf('/');
int atPos = s.indexOf('@');
if (atPos != -1 && (slashPos == -1 || atPos < slashPos)) {
// Has userinfo component - preserve as-is to avoid masking config errors
return s;
}
return scheme + s;

Copy link
Contributor Author

@bitflicker64 bitflicker64 Jan 27, 2026

Choose a reason for hiding this comment

The 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.


return scheme + s;
}
}
105 changes: 96 additions & 9 deletions ...mmons/hugegraph-common/src/test/java/org/apache/hugegraph/unit/config/HugeConfigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,15 @@
import java.util.List;
import java.util.Map;

import org.apache.commons.collections.IteratorUtils;
import org.apache.commons.configuration2.AbstractConfiguration;
import org.apache.commons.configuration2.Configuration;
import org.apache.commons.configuration2.MapConfiguration;
import org.apache.commons.configuration2.PropertiesConfiguration;
import org.apache.commons.configuration2.convert.DisabledListDelimiterHandler;
import org.apache.commons.configuration2.ex.ConfigurationException;
import org.apache.commons.configuration2.io.FileHandler;
import org.apache.commons.io.FileUtils;
import org.apache.hugegraph.config.ConfigConvOption;
import org.apache.hugegraph.config.ConfigException;
import org.apache.hugegraph.config.ConfigListConvOption;
Expand All @@ -42,15 +51,6 @@
import org.apache.hugegraph.config.OptionSpace;
import org.apache.hugegraph.testutil.Assert;
import org.apache.hugegraph.unit.BaseUnitTest;
import org.apache.commons.collections.IteratorUtils;
import org.apache.commons.configuration2.AbstractConfiguration;
import org.apache.commons.configuration2.Configuration;
import org.apache.commons.configuration2.MapConfiguration;
import org.apache.commons.configuration2.PropertiesConfiguration;
import org.apache.commons.configuration2.convert.DisabledListDelimiterHandler;
import org.apache.commons.configuration2.ex.ConfigurationException;
import org.apache.commons.configuration2.io.FileHandler;
import org.apache.commons.io.FileUtils;
import org.junit.BeforeClass;
import org.junit.Test;

Expand Down Expand Up @@ -429,6 +429,45 @@ public void testFromMapConfigurationWithList() {
Assert.assertTrue(values.contains("b"));
}

@Test
public void testUrlOptionNormalizeAddsDefaultScheme() {
Map<String, Object> map = new HashMap<>();
map.put("restserver.url", "127.0.0.1:8080");
map.put("gremlinserver.url", "127.0.0.1:8182");
map.put("server.urls_to_pd", "0.0.0.0:8080");
map.put("server.k8s_url", "127.0.0.1:8888");

HugeConfig config = new HugeConfig(map);

Assert.assertEquals("http://127.0.0.1:8080",
config.get(UrlOptions.restUrl));
Assert.assertEquals("http://127.0.0.1:8182",
config.get(UrlOptions.gremlinUrl));
Assert.assertEquals("http://0.0.0.0:8080",
config.get(UrlOptions.urlsToPd));

// critical corner case: must NOT downgrade to http
Assert.assertEquals("https://127.0.0.1:8888",
config.get(UrlOptions.k8sUrl));
}

@Test
public void testUrlOptionNormalizeKeepsExistingScheme() {
Map<String, Object> map = new HashMap<>();
map.put("restserver.url", "https://127.0.0.1:8080");
map.put("gremlinserver.url", "http://127.0.0.1:8182");
map.put("server.k8s_url", "http://127.0.0.1:8888");

HugeConfig config = new HugeConfig(map);

Assert.assertEquals("https://127.0.0.1:8080",
config.get(UrlOptions.restUrl));
Assert.assertEquals("http://127.0.0.1:8182",
config.get(UrlOptions.gremlinUrl));
Assert.assertEquals("http://127.0.0.1:8888",
config.get(UrlOptions.k8sUrl));
}

public static class TestOptions extends OptionHolder {

private static volatile TestOptions instance;
Expand Down Expand Up @@ -586,6 +625,54 @@ public static class TestSubOptions extends TestOptions {
);
}

/**
* Added: URL option holder to test HugeConfig URL normalization logic.
*/
public static class UrlOptions extends OptionHolder {

private static volatile UrlOptions instance;

public static synchronized UrlOptions instance() {
if (instance == null) {
instance = new UrlOptions();
instance.registerOptions();
}
return instance;
}

public static final ConfigOption<String> restUrl =
new ConfigOption<>(
"restserver.url",
"rest url",
disallowEmpty(),
"http://127.0.0.1:8080"
);

public static final ConfigOption<String> gremlinUrl =
new ConfigOption<>(
"gremlinserver.url",
"gremlin url",
disallowEmpty(),
"http://127.0.0.1:8182"
);

public static final ConfigOption<String> urlsToPd =
new ConfigOption<>(
"server.urls_to_pd",
"urls to pd",
disallowEmpty(),
"http://0.0.0.0:8080"
);

public static final ConfigOption<String> k8sUrl =
new ConfigOption<>(
"server.k8s_url",
"k8s url",
disallowEmpty(),
"https://127.0.0.1:8888"
);
}

public static class TestOptionsWithTypeError extends OptionHolder {

private static volatile TestOptionsWithTypeError instance;
Expand Down
Loading