imbajin commented on code in PR #2944:
URL:
https://github.com/apache/incubator-hugegraph/pull/2944#discussion_r2749523185
##########
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:
‼️ **Critical Logic Error: Returns lowercase for existing schemes**
This normalization converts **all** existing URLs to lowercase, including
those explicitly configured with uppercase (e.g., `HTTP://127.0.0.1:8080` →
`http://127.0.0.1:8080`). While HTTP schemes are case-insensitive per RFC 3986,
this creates **inconsistent behavior**:
- Missing scheme → adds default + lowercases input
- Existing scheme → lowercases entire URL
**Problem:** If a user explicitly sets `HTTPS://MyHost:8080`, the hostname
gets lowercased to `myhost`, which could break case-sensitive server
configurations.
**Suggested fix:** Only lowercase the scheme portion, preserve original
casing for the rest:
```suggestion
private static String prefixSchemeIfMissing(String raw, String scheme) {
if (raw == null) {
return null;
}
String s = raw.trim();
if (s.isEmpty()) {
return s;
}
// Extract scheme if present
int schemeIdx = s.indexOf("://");
if (schemeIdx > 0) {
String existingScheme = s.substring(0, schemeIdx).toLowerCase();
String rest = s.substring(schemeIdx);
return existingScheme + rest;
}
return scheme + s; // Preserve original case when adding scheme
}
```
##########
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;
+ }
Review Comment:
‼️ **Critical Design Issue: Silently modifies user-provided values**
The current implementation normalizes values **even when explicitly set by
users** in config files. This violates the principle of least surprise:
```properties
# User's config file
restserver.url = 127.0.0.1:8080
```
Gets silently transformed to `http://127.0.0.1:8080` without any indication.
Users might expect errors for malformed URLs rather than silent corrections.
**Suggested approach:**
1. Log a warning when auto-prefixing scheme: `LOG.warn("Config '{}' missing
scheme, defaulting to '{}'", key, normalizedValue);`
2. Or validate and reject invalid URLs at startup with clear error messages
##########
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) {
Review Comment:
⚠️ **Performance: Redundant OptionSpace lookup on every config read**
`defaultSchemeFor()` calls `OptionSpace.get(key)` on **every** config value
retrieval, even for non-URL options. This adds overhead to frequently accessed
config reads.
**Suggested optimization:** Cache the URL normalization metadata during
`ConfigOption` registration or HugeConfig initialization:
```suggestion
// In HugeConfig constructor or static initializer
private static final Map<String, String> URL_NORMALIZATIONS = new
HashMap<>();
static {
// Populate from OptionSpace during initialization
OptionSpace.getAll().forEach((key, option) -> {
if (option instanceof ConfigOption) {
ConfigOption<?> co = (ConfigOption<?>) option;
if (co.needsUrlNormalization()) {
URL_NORMALIZATIONS.put(key, co.getDefaultScheme());
}
}
});
}
private static String defaultSchemeFor(String key) {
return URL_NORMALIZATIONS.get(key);
}
```
##########
hugegraph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/config/HugeConfig.java:
##########
@@ -87,9 +87,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) {
Review Comment:
⚠️ **Code Quality: Inconsistent behavior between default and explicit
values**
Line 94-96 shows normalization **only applies to explicitly set values**
(`fromDefault = false`), but **not** to default values from
`option.defaultValue()`.
This creates inconsistent behavior:
- Default value `http://127.0.0.1:8080` → used as-is
- User sets `127.0.0.1:8080` → normalized to `http://127.0.0.1:8080`
**Question:** Is this intentional? If defaults already have schemes, why do
we need `.withUrlNormalization()` on the options?
##########
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));
+
+ conf = new PropertiesConfiguration();
+ conf.setProperty("restserver.url", "[::1]:8080");
+ config = new HugeConfig(conf);
+ Assert.assertEquals("http://[::1]:8080";,
+ config.get(ServerOptions.REST_SERVER_URL));
Review Comment:
⚠️ **Missing Test Coverage: IPv6 URLs with schemes**
The test at line 70-74 covers IPv6 addresses **without** schemes
(`[::1]:8080`), but doesn't test the preservation behavior when schemes are
present:
**Add test case:**
```java
@Test
public void testIpv6WithExistingScheme() {
PropertiesConfiguration conf = new PropertiesConfiguration();
conf.setProperty("restserver.url", "http://[::1]:8080";);
HugeConfig config = new HugeConfig(conf);
Assert.assertEquals("http://[::1]:8080";,
config.get(ServerOptions.REST_SERVER_URL));
}
```
##########
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.
Review Comment:
🧹 **Minor: Unnecessary comment duplicates code logic**
The comment on line 235 restates what the code already expresses clearly
through method name `prefixSchemeIfMissing()` and the allowlist check.
**Suggested simplification:**
```suggestion
// Normalize allowlisted URL options
if (value instanceof String) {
return prefixSchemeIfMissing((String) value, scheme);
}
```
--
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]
