imbajin commented on code in PR #2944:
URL:
https://github.com/apache/incubator-hugegraph/pull/2944#discussion_r2724267970
##########
hugegraph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/config/HugeConfig.java:
##########
@@ -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;
+ }
Review Comment:
**⚠️ Important: Missing Edge Case Handling**
The current implementation doesn't handle URLs with credentials or userinfo:
```java
// These would incorrectly get prefixed:
"user:[email protected]:8080" → "http://user:[email protected]: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 `/`:
```suggestion
// 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;
```
##########
hugegraph-commons/hugegraph-common/src/test/java/org/apache/hugegraph/unit/config/HugeConfigTest.java:
##########
@@ -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));
+ }
Review Comment:
**⚠️ Important: Test Coverage Gap**
Tests verify the happy path but miss important edge cases:
1. **Empty/whitespace-only strings** - Returns empty after trim, but should
this be valid?
2. **Malformed URLs** - `"://127.0.0.1:8080"` or `"http:127.0.0.1"`
3. **Mixed case schemes** - `"HtTp://"` or `"HTTPS://"`
4. **Trailing/leading whitespace** - `" http://127.0.0.1:8080 "`
5. **IPv6 addresses** - `"[::1]:8080"` should become `"http://[::1]:8080"`
Add test cases:
```java
@Test
public void testUrlNormalizationEdgeCases() {
Map<String, Object> map = new HashMap<>();
// Whitespace handling
map.put("restserver.url", " 127.0.0.1:8080 ");
HugeConfig config = new HugeConfig(map);
Assert.assertEquals("http://127.0.0.1:8080";,
config.get(UrlOptions.restUrl));
// Mixed case scheme preservation
map.put("restserver.url", "HTTP://127.0.0.1:8080");
config = new HugeConfig(map);
Assert.assertEquals("HTTP://127.0.0.1:8080",
config.get(UrlOptions.restUrl));
// IPv6
map.put("restserver.url", "[::1]:8080");
config = new HugeConfig(map);
Assert.assertEquals("http://[::1]:8080";, config.get(UrlOptions.restUrl));
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
