You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hugegraph.apache.org by "bitflicker64 (via GitHub)" <gi...@apache.org> on 2026/01/23 19:40:14 UTC
[PR] Normalize server URL config values when scheme is missing [incubator-hugegraph]
bitflicker64 opened a new pull request, #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944
<!--
Thank you very much for contributing to Apache HugeGraph, we are happy that you want to help us improve it!
Here are some tips for you:
1. If this is your first time, please read the [contributing guidelines](https://github.com/apache/hugegraph/blob/master/CONTRIBUTING.md)
2. If a PR fix/close an issue, type the message "close xxx" (xxx is the link of related
issue) in the content, GitHub will auto link it (Required)
3. Name the PR title in "Google Commit Format", start with "feat | fix | perf | refactor | doc | chore",
such like: "feat(core): support the PageRank algorithm" or "fix: wrong break in the compute loop" (module is optional)
skip it if you are unsure about which is the best component.
4. One PR address one issue, better not to mix up multiple issues.
5. Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]` (or click it directly after
published)
-->
## Purpose of the PR
- close #2942 <!-- or use "fix #xxx", "xxx" is the ID-link of related issue, e.g: close #1024 -->
- Some server URL configs currently require an explicit scheme (`http://` / `https://`). If a user sets something like `127.0.0.1:8080`, it can fail to parse or behave inconsistently compared to other modules that already default the scheme.
This PR fixes that by normalizing **only the relevant server URL options**: if the scheme is missing, we auto-prefix a sensible default, while leaving explicitly provided schemes untouched.
Also covered the review corner case: `server.k8s_url` should default to **`https://`** (so we don’t accidentally downgrade it to `http://`).
<!--
Please explain more context in this section, clarify why the changes are needed.
e.g:
- If you propose a new API, clarify the use case for a new API.
- If you fix a bug, you can clarify why it is a bug, and should be associated with an issue.
-->
## Main Changes
<!-- Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue. These change logs are helpful for better and faster reviews.)
For example:
- If you introduce a new feature, please show detailed design here or add the link of design documentation.
- If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
- If there is a discussion in the mailing list, please add the link. -->
`HugeConfig.get(...)` now applies URL normalization for a small allowlist of keys:
* `restserver.url` → default `http://`
* `gremlinserver.url` → default `http://`
* `server.urls_to_pd` → default `http://`
* `server.k8s_url` → default `https://`
Normalization only triggers when:
* the config key is in the allowlist, **and**
* the value is a `String`, **and**
* the value has **no scheme**
If the value already starts with `http://` or `https://`, it’s left as-is.
Non-string config values are untouched.
## Verifying these changes
<!-- Please pick the proper options below -->
- [ ] Trivial rework / code cleanup without any test coverage. (No Need)
- [ ] Already covered by existing tests, such as *(please modify tests here)*.
- [x] Need tests and can be verified as follows:
Tests are included and you can verify the behavior like this:
* Added unit tests in `HugeConfigTest` covering:
* missing scheme → default prefix is added
* existing scheme → preserved as-is
* `server.k8s_url` missing scheme → becomes `https://...` (no accidental downgrade)
## Does this PR potentially affect the following parts?
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] Dependencies ([add/update license](https://hugegraph.apache.org/docs/contribution-guidelines/contribute/#321-check-licenses) info & [regenerate_known_dependencies.sh](../install-dist/scripts/dependency/regenerate_known_dependencies.sh)) <!-- Don't forget to add/update the info in "LICENSE" & "NOTICE" files (both in root & dist module) -->
- [x] Modify configurations
- [ ] The public API
- [ ] Other affects (typed here)
- [ ] Nope
## Documentation Status
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `Doc - TODO` <!-- Your PR changes impact docs and you will update later -->
- [ ] `Doc - Done` <!-- Related docs have been already added or updated -->
- [x] `Doc - No Need` <!-- Your PR changes don't impact/need docs -->
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#issuecomment-3822374725
> > Hi, I tried refactoring HugeConfigTest to use ServerOptions directly as suggested. However, this test is in the hugegraph-common module, while ServerOptions is in hugegraph-server. When I add the dependency, it creates a circular dependency and the build fails with this error:
> > ```
> > The projects in the reactor contain a cyclic reference:
> > org.apache.hugegraph:hugegraph-api
> > -> org.apache.hugegraph:hugegraph-core
> > -> org.apache.hugegraph:hugegraph-common
> > -> org.apache.hugegraph:hugegraph-api
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > Because of this, ServerOptions cannot be used in hugegraph-common tests without breaking the module structure.
>
> Fine, then we could keep the original logic to test it
i think we have one more option , move the ServerOptions-related tests to the hugegraph-server module.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#discussion_r2751125571
##########
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:
fixed and added the following tests -
```java
@Test
public void testUrlNormalizationPreservesHostnameCase() {
// Uppercase scheme + mixed-case hostname
PropertiesConfiguration conf = new PropertiesConfiguration();
conf.setProperty("restserver.url", "HTTP://MyServer:8080");
HugeConfig config = new HugeConfig(conf);
Assert.assertEquals("http://MyServer:8080",
config.get(ServerOptions.REST_SERVER_URL));
// HTTPS with mixed-case hostname
conf = new PropertiesConfiguration();
conf.setProperty("restserver.url", "HTTPS://MyHost:8080");
config = new HugeConfig(conf);
Assert.assertEquals("https://MyHost:8080",
config.get(ServerOptions.REST_SERVER_URL));
}
@Test
public void testUrlNormalizationPreservesPathCase() {
PropertiesConfiguration conf = new PropertiesConfiguration();
conf.setProperty("restserver.url", "http://127.0.0.1:8080/SomePath/CaseSensitive");
HugeConfig config = new HugeConfig(conf);
Assert.assertEquals("http://127.0.0.1:8080/SomePath/CaseSensitive",
config.get(ServerOptions.REST_SERVER_URL));
}
@Test
public void testHttpsSchemeIsNotDowngraded() {
PropertiesConfiguration conf = new PropertiesConfiguration();
conf.setProperty("restserver.url", "https://127.0.0.1:8080");
HugeConfig config = new HugeConfig(conf);
Assert.assertEquals("https://127.0.0.1:8080",
config.get(ServerOptions.REST_SERVER_URL));
}
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#issuecomment-3864869398
> > @imbajin hey I'm interested in contributing more to the project and was wondering if I could get an invitation to the Slack workspace?
>
> Thank you for your interest! It’s great to see you’re looking to get more involved with the project.
>
> Regarding the [Slack](https://app.slack.com/client/T4S1WH2J3/C059UU2FJ23) and Discord channels, they are actually public and you are welcome to join anytime without a specific invitation. However, please note that we haven't officially started active operations or management for those channels yet. If you're interested, we’d be happy to grant you the necessary permissions to help lead or moderate the community there!
>
> Currently, our primary communication still happens through GitHub Issues/Discussions and Email.
>
> We are always open to any suggestions or feedback you might have, whether you'd like to share them publicly or privately. Looking forward to your further contributions!
Hi, I tried joining via the Slack link, but it says I don’t have an account. I also couldn’t find the Discord.
Could you please share active invite links?
My email: himanshuverma151006@gmail.com
Thanks
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#discussion_r2750969999
##########
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:
added tests for preservation behaviour
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#issuecomment-3831508354
All review feedback implemented . There's one failing test `KoutApiTest.testGet` (graph traversal) that I can't trace back to my URL changes would really appreciate your help debugging this .also after this ill update https://github.com/apache/incubator-hugegraph-doc
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#issuecomment-3823373557
@imbajin Thanks for the discussion and guidance. I’ve moved the ServerOptions related tests into ServerOptionsTest and confirmed that all tests and CI are passing now.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#issuecomment-3860615037
@imbajin hey I'm interested in contributing more to the project and was wondering if I could get an invitation to the Slack workspace?
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#issuecomment-3823274780
## [Codecov](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2944?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 93.25%. Comparing base ([`99baf2b`](https://app.codecov.io/gh/apache/incubator-hugegraph/commit/99baf2bde2ddc6c0db6a4f811109f9f2c34abdc8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`358d037`](https://app.codecov.io/gh/apache/incubator-hugegraph/commit/358d037cc8e010a50008e541efb685f1926c5509?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #2944 +/- ##
=============================================
+ Coverage 35.63% 93.25% +57.62%
+ Complexity 333 65 -268
=============================================
Files 801 9 -792
Lines 67523 267 -67256
Branches 8778 22 -8756
=============================================
- Hits 24061 249 -23812
+ Misses 40901 8 -40893
+ Partials 2561 10 -2551
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2944?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#discussion_r2751126348
##########
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:
fixed and added tests -
```java
@Test
public void testUrlNormalizationPreservesHostnameCase() {
// Uppercase scheme + mixed-case hostname
PropertiesConfiguration conf = new PropertiesConfiguration();
conf.setProperty("restserver.url", "HTTP://MyServer:8080");
HugeConfig config = new HugeConfig(conf);
Assert.assertEquals("http://MyServer:8080",
config.get(ServerOptions.REST_SERVER_URL));
// HTTPS with mixed-case hostname
conf = new PropertiesConfiguration();
conf.setProperty("restserver.url", "HTTPS://MyHost:8080");
config = new HugeConfig(conf);
Assert.assertEquals("https://MyHost:8080",
config.get(ServerOptions.REST_SERVER_URL));
}
@Test
public void testUrlNormalizationPreservesPathCase() {
PropertiesConfiguration conf = new PropertiesConfiguration();
conf.setProperty("restserver.url", "http://127.0.0.1:8080/SomePath/CaseSensitive");
HugeConfig config = new HugeConfig(conf);
Assert.assertEquals("http://127.0.0.1:8080/SomePath/CaseSensitive",
config.get(ServerOptions.REST_SERVER_URL));
}
@Test
public void testHttpsSchemeIsNotDowngraded() {
PropertiesConfiguration conf = new PropertiesConfiguration();
conf.setProperty("restserver.url", "https://127.0.0.1:8080");
HugeConfig config = new HugeConfig(conf);
Assert.assertEquals("https://127.0.0.1:8080",
config.get(ServerOptions.REST_SERVER_URL));
}
```
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#issuecomment-3869831654
> > Hi, I tried joining via the Slack link, but it says I don’t have an account. I also couldn’t find the Discord. Could you please share active invite links? My email: [himanshuverma151006@gmail.com](mailto:himanshuverma151006@gmail.com) Thanks
>
> Seems I don't have the permission to invite u to the ASF's Slack:)
>
> When I add your account, it said:
>
> > Your administrator has prohibited the use of Slack Connect in this workspace, so only ASF members and guests can join this channel.
>
> Discord: https://discord.gg/uQqtb9U5vu (Indeed Not active)
>
> BTW, our [docker-compose](https://github.com/apache/incubator-hugegraph/blob/master/docker/docker-compose.yml) images have some issues with the latest version, u could submit a new issue & test/fix it
> > Hi, I tried joining via the Slack link, but it says I don’t have an account. I also couldn’t find the Discord. Could you please share active invite links? My email: [himanshuverma151006@gmail.com](mailto:himanshuverma151006@gmail.com) Thanks
>
> Seems I don't have the permission to invite u to the ASF's Slack:)
>
> When I add your account, it said:
>
> > Your administrator has prohibited the use of Slack Connect in this workspace, so only ASF members and guests can join this channel.
>
> Discord: https://discord.gg/uQqtb9U5vu (Indeed Not active)
>
> BTW, our [docker-compose](https://github.com/apache/incubator-hugegraph/blob/master/docker/docker-compose.yml) images have some issues with the latest version, u could submit a new issue & test/fix it
Thanks for checking the slack invite and i've joined the discord . I’ll look into the docker issue and update accordingly.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#discussion_r2751174466
##########
hugegraph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/config/ConfigOption.java:
##########
@@ -20,6 +20,22 @@
import com.google.common.base.Predicate;
public class ConfigOption<T> extends TypedOption<T, T> {
+ private boolean urlNormalize = false;
+ private String defaultScheme = null;
+
+ public ConfigOption<T> withUrlNormalization(String scheme) {
Review Comment:
this method is only used when `ConfigOption` constants are first created (for example, in `ServerOptions`) It runs once when the class is loaded, sets the metadata, and is never changed after that. Because of this, there is no real risk of concurrency issues in the current design. Ill add documentation to make this clearer
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#discussion_r2724261885
##########
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#issuecomment-3822347834
> Hi, I tried refactoring HugeConfigTest to use ServerOptions directly as suggested. However, this test is in the hugegraph-common module, while ServerOptions is in hugegraph-server. When I add the dependency, it creates a circular dependency and the build fails with this error:
>
> ```
> The projects in the reactor contain a cyclic reference:
> org.apache.hugegraph:hugegraph-api
> -> org.apache.hugegraph:hugegraph-core
> -> org.apache.hugegraph:hugegraph-common
> -> org.apache.hugegraph:hugegraph-api
> ```
>
> Because of this, ServerOptions cannot be used in hugegraph-common tests without breaking the module structure.
Fine, then we could keep the original logic to test it
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#discussion_r2732101647
##########
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:
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.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#issuecomment-3798219171
> Apologize for hitting approve by mistake :)
its fine i am still working on this
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#discussion_r2724268236
##########
hugegraph-commons/hugegraph-common/src/test/java/org/apache/hugegraph/unit/config/HugeConfigTest.java:
##########
@@ -32,6 +32,15 @@
import java.util.List;
import java.util.Map;
+import org.apache.commons.collections.IteratorUtils;
Review Comment:
**🧹 Minor: Import Order Inconsistency**
The test file has imports reordered (Apache Commons imports moved before HugeGraph imports). While functionally correct, this breaks the existing import ordering convention:
```diff
-import org.apache.hugegraph.config.ConfigConvOption;
-...
import org.apache.commons.collections.IteratorUtils;
import org.apache.commons.configuration2.AbstractConfiguration;
+...
+import org.apache.hugegraph.config.ConfigConvOption;
```
Consider keeping the original import order to reduce diff noise and maintain consistency.
##########
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.
Review Comment:
**🧹 Minor: Documentation Could Be Clearer**
The comment mentions "URL options are defined as ConfigOption<String>", but this is an implementation detail that might confuse readers. Consider clarifying the intent:
```suggestion
// 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);
}
```
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#discussion_r2750995189
##########
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:
Yes, this is intentional.
The default values already have http://, so they are already correct. There is nothing to fix, so normalization is skipped. User values may not have the scheme (for example: 127.0.0.1:8080), so only those need normalization.
.withUrlNormalization() is just metadata. It tells the system: “this option is a URL, and use this scheme when fixing user input.”
```
So:
Default → already correct → no change
User value → may be incomplete → normalize
```
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#discussion_r2751125571
##########
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:
fixed and added the following tests -
```java
@Test
public void testUrlNormalizationPreservesHostnameCase() {
// Uppercase scheme + mixed-case hostname
PropertiesConfiguration conf = new PropertiesConfiguration();
conf.setProperty("restserver.url", "HTTP://MyServer:8080");
HugeConfig config = new HugeConfig(conf);
Assert.assertEquals("http://MyServer:8080",
config.get(ServerOptions.REST_SERVER_URL));
// HTTPS with mixed-case hostname
conf = new PropertiesConfiguration();
conf.setProperty("restserver.url", "HTTPS://MyHost:8080");
config = new HugeConfig(conf);
Assert.assertEquals("https://MyHost:8080",
config.get(ServerOptions.REST_SERVER_URL));
}
@Test
public void testUrlNormalizationPreservesPathCase() {
PropertiesConfiguration conf = new PropertiesConfiguration();
conf.setProperty("restserver.url", "http://127.0.0.1:8080/SomePath/CaseSensitive");
HugeConfig config = new HugeConfig(conf);
Assert.assertEquals("http://127.0.0.1:8080/SomePath/CaseSensitive",
config.get(ServerOptions.REST_SERVER_URL));
}
@Test
public void testHttpsSchemeIsNotDowngraded() {
PropertiesConfiguration conf = new PropertiesConfiguration();
conf.setProperty("restserver.url", "https://127.0.0.1:8080");
HugeConfig config = new HugeConfig(conf);
Assert.assertEquals("https://127.0.0.1:8080",
config.get(ServerOptions.REST_SERVER_URL));
}
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#discussion_r2749523489
##########
hugegraph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/config/ConfigOption.java:
##########
@@ -20,6 +20,22 @@
import com.google.common.base.Predicate;
public class ConfigOption<T> extends TypedOption<T, T> {
+ private boolean urlNormalize = false;
+ private String defaultScheme = null;
+
+ public ConfigOption<T> withUrlNormalization(String scheme) {
Review Comment:
⚠️ **API Design: Builder pattern requires mutation**
The `withUrlNormalization()` method mutates the `ConfigOption` instance and returns `this`, but it's called during option declaration:
```java
new ConfigOption<>(...).withUrlNormalization("http://");
```
**Issue:** If `ConfigOption` instances are ever shared or reused, this mutation could cause race conditions.
**Suggested improvement:** Make `withUrlNormalization()` immutable or ensure `ConfigOption` is only constructed once during static initialization (add final modifier or documentation).
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#discussion_r2724267506
##########
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://)
Review Comment:
**‼️ Critical: Logic Flow Issue**
The normalization logic modifies the default value before returning it, but this happens **after** the value is already set from `defaultValue()`. This means the normalization will apply to **both** user-provided values AND default values.
However, if the `ConfigOption` already has a proper default like `"http://127.0.0.1:8080"`, the normalization is redundant. More importantly, this design couples the config retrieval logic with URL-specific assumptions.
Consider: Does this break if someone later adds a URL option that isn't in the allowlist? The normalization silently won't apply, creating inconsistent behavior.
##########
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
Review Comment:
**‼️ Critical: Case Sensitivity Bug**
Using `.toLowerCase()` for comparison but returning the original string creates a vulnerability:
```java
String lower = s.toLowerCase();
if (lower.startsWith("http://") || lower.startsWith("https://")) {
return s; // Returns original - might be "HTTP://" or "HtTp://"
}
```
This means `"HTTP://127.0.0.1:8080"` would pass through unchanged. While URLs are case-insensitive for the scheme part per RFC 3986, this creates inconsistent output where some schemes are lowercase and others preserve their original casing.
**Suggestion:**
```suggestion
// Keep original string if scheme already exists (case-insensitive check)
if (s.startsWith("http://") || s.startsWith("https://") ||
s.startsWith("HTTP://") || s.startsWith("HTTPS://")) {
return s;
}
```
Or normalize consistently:
```java
// Check if scheme exists (case-insensitive)
if (lower.startsWith("http://") || lower.startsWith("https://")) {
// Normalize scheme to lowercase per RFC 3986
return lower;
}
```
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#issuecomment-3817131730
> > I noticed that my tests currently use a duplicated `UrlOptions` class instead of the actual `ServerOptions`. Because of this, if config keys change in `ServerOptions`, the tests will not fail, since they rely on hardcoded duplicates.
> > ```java
> > // Test duplicate
> > public static class UrlOptions extends OptionHolder {
> > public static final ConfigOption<String> restUrl =
> > new ConfigOption<>("restserver.url", ...).withUrlNormalization("http://");
> > }
> >
> > // Production (ServerOptions.java)
> > public static final ConfigOption<String> GREMLIN_SERVER_URL =
> > new ConfigOption<>("gremlinserver.url", ...).withUrlNormalization("http://");
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > Would you prefer that I: Refactor tests to use ServerOptions directly or Keep the duplicated UrlOptions (more isolated, but requires manual syncing)? Let me know your preference and I’ll refactor accordingly.
>
> @bitflicker64 Apologize for the delay, prefer PlanA (Refactor tests to keep DRY rule)
Hi, I tried refactoring HugeConfigTest to use ServerOptions directly as suggested. However, this test is in the hugegraph-common module, while ServerOptions is in hugegraph-server. When I add the dependency, it creates a circular dependency and the build fails with this error:
```
The projects in the reactor contain a cyclic reference:
org.apache.hugegraph:hugegraph-api
-> org.apache.hugegraph:hugegraph-core
-> org.apache.hugegraph:hugegraph-common
-> org.apache.hugegraph:hugegraph-api
```
Because of this, ServerOptions cannot be used in hugegraph-common tests without breaking the module structure.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin merged PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#discussion_r2725604000
##########
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://)
Review Comment:
```java
// Introduces a fromDefault flag to separate system defaults from user input,
// avoiding redundant normalization of valid defaults while preserving safety for user values.
```
```java
public <T, R> R get(TypedOption<T, R> option) {
Object value = this.getProperty(option.name());
boolean fromDefault = false;
if (value == null) {
value = option.defaultValue();
fromDefault = true;
}
if (!fromDefault) {
value = normalizeUrlOptionIfNeeded(option.name(), value);
}
return (R) value;
}
```
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#issuecomment-3795322063
Thanks for the feedback. I’ll go through the comments and update the implementation to address the issues you mentioned ill also keep these points in mind to avoid making similar mistakes in future changes
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#issuecomment-3806327341
I noticed that my tests currently use a duplicated `UrlOptions` class instead of the actual `ServerOptions`. Because of this, if config keys change in `ServerOptions`, the tests will not fail, since they rely on hardcoded duplicates.
```java
// Test duplicate
public static class UrlOptions extends OptionHolder {
public static final ConfigOption<String> restUrl =
new ConfigOption<>("restserver.url", ...).withUrlNormalization("http://");
}
// Production (ServerOptions.java)
public static final ConfigOption<String> GREMLIN_SERVER_URL =
new ConfigOption<>("gremlinserver.url", ...).withUrlNormalization("http://");
```
Would you prefer that I:
Refactor tests to use ServerOptions directly or
Keep the duplicated UrlOptions (more isolated, but requires manual syncing)?
Let me know your preference and I’ll refactor accordingly.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#issuecomment-3893225549
> > Hi, I tried joining via the Slack link, but it says I don’t have an account. I also couldn’t find the Discord. Could you please share active invite links? My email: [himanshuverma151006@gmail.com](mailto:himanshuverma151006@gmail.com) Thanks
>
> Seems I don't have the permission to invite u to the ASF's Slack:)
>
> When I add your account, it said:
>
> > Your administrator has prohibited the use of Slack Connect in this workspace, so only ASF members and guests can join this channel.
>
> Discord: https://discord.gg/uQqtb9U5vu (Indeed Not active)
>
> BTW, our [docker-compose](https://github.com/apache/incubator-hugegraph/blob/master/docker/docker-compose.yml) images have some issues with the latest version, u could submit a new issue & test/fix it
I am running this setup on macOS where host networking is not available and the PD container shows three listening ports. While setting up a local development environment using bridge networking, could you clarify whether all PD ports need to be exposed or only the ones required for normal HugeGraph operation? Additionally, is bridge networking the recommended approach on macOS instead of host mode, considering potential port conflicts and platform limitations?
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#issuecomment-3822475835
> i think we have one more option , move the ServerOptions-related tests to the hugegraph-server module.
Hi, I checked the structure, we could add ServerOptionsTest in hugegraph-test (UT), and remove the duplicate UrlOptions class and related tests from HugeConfigTest.java (LGTM)
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
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: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 `/`:
```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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#discussion_r2724267776
##########
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) {
Review Comment:
**‼️ Critical: Hardcoded Config Keys Break Maintainability**
The switch statement hardcodes config key strings:
```java
case "restserver.url":
case "gremlinserver.url":
case "server.urls_to_pd":
case "server.k8s_url":
```
This creates tight coupling between `HugeConfig` (commons module) and specific config keys from various modules. If a key is renamed or a new URL option is added elsewhere, developers must remember to update this switch statement.
**Better approach:** Let `ConfigOption` declare whether it needs URL normalization via metadata:
```java
public class ConfigOption<T> {
private boolean urlNormalize = false;
private String defaultScheme = "http://";
public ConfigOption<T> withUrlNormalization(String scheme) {
this.urlNormalize = true;
this.defaultScheme = scheme;
return this;
}
}
```
Then in `HugeConfig.get()`:
```java
if (option.needsUrlNormalization()) {
value = prefixSchemeIfMissing((String) value, option.getDefaultScheme());
}
```
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#issuecomment-3815918044
> I noticed that my tests currently use a duplicated `UrlOptions` class instead of the actual `ServerOptions`. Because of this, if config keys change in `ServerOptions`, the tests will not fail, since they rely on hardcoded duplicates.
>
> ```java
> // Test duplicate
> public static class UrlOptions extends OptionHolder {
> public static final ConfigOption<String> restUrl =
> new ConfigOption<>("restserver.url", ...).withUrlNormalization("http://");
> }
>
> // Production (ServerOptions.java)
> public static final ConfigOption<String> GREMLIN_SERVER_URL =
> new ConfigOption<>("gremlinserver.url", ...).withUrlNormalization("http://");
> ```
>
> Would you prefer that I: Refactor tests to use ServerOptions directly or Keep the duplicated UrlOptions (more isolated, but requires manual syncing)? Let me know your preference and I’ll refactor accordingly.
@bitflicker64 Apologize for the delay, prefer PlanA (Refactor tests to keep DRY rule)
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#issuecomment-3835137372
> We should also update the config files' default value (like https://github.com/apache/incubator-hugegraph/blob/9babe493919c01f012a56e7b5fb4d8b9faf64cf5/hugegraph-server/hugegraph-dist/src/assembly/static/conf/rest-server.properties#L3C2-L3C2)
>
> Remove the unnecessary prefix of URL scheme & Also need to update it in https://github.com/apache/incubator-hugegraph-doc (website page), thx
>
> <img alt="image" width="356" height="162" src="https://private-user-images.githubusercontent.com/17706099/543256732-c59ca107-9bf9-404b-afbc-190d820845b6.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NzAwMzg1ODcsIm5iZiI6MTc3MDAzODI4NywicGF0aCI6Ii8xNzcwNjA5OS81NDMyNTY3MzItYzU5Y2ExMDctOWJmOS00MDRiLWFmYmMtMTkwZDgyMDg0NWI2LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNjAyMDIlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjYwMjAyVDEzMTgwN1omWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWI1MGEyY2E2ZmI1MGJhMTcyODYwZGUzYjdmOWVjMzBkODI5NGNmNTY5YmNlYTk2N2ZlYTk2NDFiNGUwNmQ1NTQmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.xQzkM3k1zlOnIjqkwWvg2nFDluSxsIK9LkMtDUSXJhA">
Done! I've updated the documentation repo to remove the `http://` prefix from all configuration examples.
Created PR: apache/incubator-hugegraph-doc#449
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#issuecomment-3864028148
> @imbajin hey I'm interested in contributing more to the project and was wondering if I could get an invitation to the Slack workspace?
Thank you for your interest! It’s great to see you’re looking to get more involved with the project.
Regarding the [Slack](https://app.slack.com/client/T4S1WH2J3/C059UU2FJ23) and Discord channels, they are actually public and you are welcome to join anytime without a specific invitation. However, please note that we haven't officially started active operations or management for those channels yet. If you're interested, we’d be happy to grant you the necessary permissions to help lead or moderate the community there!
Currently, our primary communication still happens through GitHub Issues/Discussions and Email.
We are always open to any suggestions or feedback you might have, whether you'd like to share them publicly or privately. Looking forward to your further contributions!
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(server): unify URL configs when scheme is missing [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on PR #2944:
URL: https://github.com/apache/incubator-hugegraph/pull/2944#issuecomment-3869655064
> Hi, I tried joining via the Slack link, but it says I don’t have an account. I also couldn’t find the Discord. Could you please share active invite links? My email: [himanshuverma151006@gmail.com](mailto:himanshuverma151006@gmail.com) Thanks
Seems I don't have the permission to invite u to the ASF's Slack:)
When I add your account, it said:
> Your administrator has prohibited the use of Slack Connect in this workspace, so only ASF members and guests can join this channel.
Discord: https://discord.gg/uQqtb9U5vu (Indeed Not active)
BTW, our [docker-compose](https://github.com/apache/incubator-hugegraph/blob/master/docker/docker-compose.yml) images have some issues with the latest version, u could submit a new issue & test/fix it
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org