refactor(server): unify URL configs when scheme is missing#2944
refactor(server): unify URL configs when scheme is missing#2944imbajin merged 11 commits intoapache:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #2942 by normalizing server URL configuration values when the scheme (http:// or https://) is missing. The normalization allows users to configure URLs without explicitly specifying the scheme, with sensible defaults automatically applied.
Changes:
- Modified
HugeConfig.get()to apply URL normalization for specific config keys - Added URL normalization logic that defaults to
http://for most server URLs andhttps://for Kubernetes URLs - Added comprehensive unit tests to verify normalization behavior
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| hugegraph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/config/HugeConfig.java | Implements URL normalization logic in the config retrieval method with allowlist-based scheme defaulting |
| hugegraph-commons/hugegraph-common/src/test/java/org/apache/hugegraph/unit/config/HugeConfigTest.java | Adds test cases for URL normalization and reorganizes imports in alphabetical order |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...-commons/hugegraph-common/src/test/java/org/apache/hugegraph/unit/config/HugeConfigTest.java
Outdated
Show resolved
Hide resolved
| 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; |
Copilot
AI
Jan 24, 2026
•
There was a problem hiding this comment.
The URL normalization happens at retrieval time in the get() method rather than at storage time.
This means:
- the original unnormalized value remains in the underlying configuration storage and will be returned by direct access methods like getProperty() if they exist
- 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/main/java/org/apache/hugegraph/config/HugeConfig.java
Outdated
Show resolved
Hide resolved
hugegraph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/config/HugeConfig.java
Outdated
Show resolved
Hide resolved
hugegraph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/config/HugeConfig.java
Show resolved
Hide resolved
| String lower = s.toLowerCase(); | ||
| if (lower.startsWith("http://") || lower.startsWith("https://")) { | ||
| return s; | ||
| } |
There was a problem hiding this comment.
The current implementation doesn't handle URLs with credentials or userinfo:
// 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 /:
| } | |
| // 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; |
There was a problem hiding this 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.
...-commons/hugegraph-common/src/test/java/org/apache/hugegraph/unit/config/HugeConfigTest.java
Outdated
Show resolved
Hide resolved
...-commons/hugegraph-common/src/test/java/org/apache/hugegraph/unit/config/HugeConfigTest.java
Outdated
Show resolved
Hide resolved
hugegraph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/config/HugeConfig.java
Outdated
Show resolved
Hide resolved
|
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 |
imbajin
left a comment
There was a problem hiding this comment.
Apologize for hitting approve by mistake :)
its fine i am still working on this |
|
I noticed that my tests currently use a duplicated // 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: |
@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: Because of this, ServerOptions cannot be used in hugegraph-common tests without breaking the module structure. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2944 +/- ##
============================================
- Coverage 35.63% 1.87% -33.76%
+ Complexity 333 86 -247
============================================
Files 801 788 -13
Lines 67523 65289 -2234
Branches 8778 8354 -424
============================================
- Hits 24061 1224 -22837
- Misses 40901 63971 +23070
+ Partials 2561 94 -2467 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
hugegraph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/config/HugeConfig.java
Outdated
Show resolved
Hide resolved
...-server/hugegraph-test/src/test/java/org/apache/hugegraph/unit/config/ServerOptionsTest.java
Show resolved
Hide resolved
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/config/ServerOptions.java
Show resolved
Hide resolved
...-server/hugegraph-test/src/test/java/org/apache/hugegraph/unit/config/ServerOptionsTest.java
Show resolved
Hide resolved
hugegraph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/config/HugeConfig.java
Outdated
Show resolved
Hide resolved
hugegraph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/config/HugeConfig.java
Show resolved
Hide resolved
hugegraph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/config/HugeConfig.java
Show resolved
Hide resolved
| fromDefault = true; | ||
| } | ||
|
|
||
| if (!fromDefault) { |
There was a problem hiding this comment.
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 tohttp://127.0.0.1:8080
Question: Is this intentional? If defaults already have schemes, why do we need .withUrlNormalization() on the options?
There was a problem hiding this 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
...-server/hugegraph-test/src/test/java/org/apache/hugegraph/unit/config/ServerOptionsTest.java
Show resolved
Hide resolved
hugegraph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/config/HugeConfig.java
Outdated
Show resolved
Hide resolved
hugegraph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/config/ConfigOption.java
Show resolved
Hide resolved
- Add URL normalization support for config options - Automatically prefix missing schemes (http://, https://) - Log warnings when auto-correcting user-provided values - Add comprehensive test coverage for normalization logic - Update config files to demonstrate the feature Changes: - ConfigOption: Add withUrlNormalization() builder method - ServerOptions: Apply normalization to REST, Gremlin, K8s URLs - HugeConfig: Implement lazy cache and normalization logic - Add ServerOptionsTest with 5 test cases - Simplify URLs in main and Docker config
|
All review feedback implemented . There's one failing test |
Done! I've updated the documentation repo to remove the |
|
@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 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. |
Seems I don't have the permission to invite u to the ASF's Slack:) When I add your account, it said:
Discord: https://discord.gg/uQqtb9U5vu (Indeed Not active) BTW, our docker-compose 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. |
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? |

Purpose of the PR
http:///https://). If a user sets something like127.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_urlshould default tohttps://(so we don’t accidentally downgrade it tohttp://).Main Changes
HugeConfig.get(...)now applies URL normalization for a small allowlist of keys:restserver.url→ defaulthttp://gremlinserver.url→ defaulthttp://server.urls_to_pd→ defaulthttp://server.k8s_url→ defaulthttps://Normalization only triggers when:
String, andIf the value already starts with
http://orhttps://, it’s left as-is.Non-string config values are untouched.
Verifying these changes
Tests are included and you can verify the behavior like this:
Added unit tests in
HugeConfigTestcovering:server.k8s_urlmissing scheme → becomeshttps://...(no accidental downgrade)Does this PR potentially affect the following parts?
Documentation Status
Doc - TODODoc - DoneDoc - No Need