Copilot commented on code in PR #2944: URL: https://github.com/apache/incubator-hugegraph/pull/2944#discussion_r2749522553
########## hugegraph-server/hugegraph-test/src/test/java/org/apache/hugegraph/unit/config/ServerOptionsTest.java: ########## @@ -0,0 +1,76 @@ +/* + * 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)); + } + + @Test + public void testUrlNormalizationEdgeCases() { + 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)); + + 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)); Review Comment: This test verifies that "HTTP://127.0.0.1:8080" is converted to lowercase "http://127.0.0.1:8080";. While this specific test case is acceptable (since it only contains scheme and host), the test doesn't verify that path components would be incorrectly lowercased as well. Consider adding a test case that includes a URL with a path component to ensure the implementation handles it correctly, for example: "http://127.0.0.1:8080/SomePath"; should preserve the case of "/SomePath". ########## hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/config/ServerOptions.java: ########## @@ -270,15 +270,15 @@ public class ServerOptions extends OptionHolder { "to clients. only used when starting the server in k8s.", disallowEmpty(), "http://0.0.0.0:8080"; - ); + ).withUrlNormalization("http://";); Review Comment: The SERVER_URLS_TO_PD configuration is used as a comma-separated list (split at line 204 in GraphManager.java), but the URL normalization is applied to the entire string before splitting. This means: 1. If a user configures "url1,url2,url3", the normalization logic would try to normalize the entire string as a single URL, which is incorrect 2. The normalization should be applied to each individual URL after splitting, not to the concatenated string Consider either: - Moving the normalization logic to happen after the split operation - Documenting that this option should only contain a single URL (not a comma-separated list) - Implementing special handling for comma-separated URL lists in the normalization logic ```suggestion ); ``` ########## hugegraph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/config/HugeConfig.java: ########## @@ -213,4 +221,53 @@ 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; + } + + // Normalize URL config values by adding default scheme if missing. + // Only applies to allowlisted URL options (see defaultSchemeFor()). + 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) { + TypedOption<?, ?> option = OptionSpace.get(key); + if (option instanceof ConfigOption) { + ConfigOption<?> configOption = (ConfigOption<?>) option; + if (configOption.needsUrlNormalization()) { + return configOption.getDefaultScheme(); + } + } + 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 lower; // Return LOWERCASE version + } + return scheme + lower; // Return scheme + LOWERCASE input Review Comment: This implementation lowercases the entire URL, including any path and query parameters that might be present. According to RFC 3986, only the scheme and hostname are case-insensitive; path and query components are case-sensitive. While the typical use cases for these URL configs (restserver.url, gremlinserver.url, etc.) are scheme://host:port patterns without paths, this implementation would break if a user configures a URL with a case-sensitive path component. Consider preserving the original case for the path and query components. A safer approach would be to parse the URL, lowercase only the scheme and hostname, and preserve the case of the path and other components. Alternatively, if these URLs are guaranteed to never contain paths, document this restriction clearly. ```suggestion // Detect scheme in a case-insensitive way, but preserve original case String lower = s.toLowerCase(); if (lower.startsWith("http://";) || lower.startsWith("https://";)) { return s; // Return original value, preserving path/query case } // Prefix default scheme without changing the case of the rest of the URL return scheme + s; ``` ########## hugegraph-server/hugegraph-test/src/test/java/org/apache/hugegraph/unit/config/ServerOptionsTest.java: ########## @@ -0,0 +1,76 @@ +/* + * 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)); + } Review Comment: The test suite doesn't verify that URLs with an existing https:// scheme are preserved correctly. While line 65-68 tests HTTP:// (uppercase), there's no test for a URL that already has https:// to ensure it doesn't get downgraded to http://. Add a test case like: ``` conf.setProperty("server.k8s_url", "https://MyK8sHost:8888";); Assert.assertEquals("https://myk8shost:8888";, config.get(ServerOptions.SERVER_K8S_URL)); ``` This would verify that https:// URLs are preserved (though lowercased), not converted to http://. -- 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]
