Copilot commented on code in PR #2944:
URL:
https://github.com/apache/incubator-hugegraph/pull/2944#discussion_r2724261894
##########
hugegraph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/config/HugeConfig.java:
##########
@@ -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;
Review Comment:
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.
##########
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:
Missing test coverage for several edge cases: (1) empty string values -
verify they remain empty after normalization, (2) whitespace-only values -
verify behavior is consistent, (3) URLs with uppercase scheme prefixes like
"HTTP://" or "HTTPS://" - verify they are preserved, (4) fallback to default
value behavior - verify that when a config key is not set, the default value is
used and properly normalized (or not, since defaults already have schemes).
Additionally, consider testing malformed URLs to ensure the normalization
doesn't create invalid URLs.
--
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]
