feat(client): add support for skipping required checks in HugeClientBuilder#718
feat(client): add support for skipping required checks in HugeClientBuilder#718FrostyHec wants to merge 1 commit intoapache:masterfrom
Conversation
|
Also need handle the ci error |
There was a problem hiding this comment.
Pull request overview
This PR adds an opt-in flag to HugeClientBuilder/HugeClient.builder(...) to bypass required url/graph argument validation, enabling more flexible client initialization flows (e.g., deferring configuration until later).
Changes:
- Added
skipRequiredCheckstoHugeClientBuilder(new constructor overload + conditional validation in ctor/build). - Added a new
HugeClient.builder(..., boolean skipRequiredChecks)overload. - Added a new unit test class covering skip vs non-skip behavior; minor formatting update in
User.UserRole.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| hugegraph-client/src/test/java/org/apache/hugegraph/unit/HugeClientBuilderTest.java | Adds tests for the new skipRequiredChecks behavior (but currently has some issues around determinism/coverage). |
| hugegraph-client/src/main/java/org/apache/hugegraph/structure/auth/User.java | Wraps a long generic type declaration for line-length; introduces trailing whitespace. |
| hugegraph-client/src/main/java/org/apache/hugegraph/driver/HugeClientBuilder.java | Introduces skipRequiredChecks flag and conditionally skips required argument validation. |
| hugegraph-client/src/main/java/org/apache/hugegraph/driver/HugeClient.java | Adds a new public builder overload exposing skipRequiredChecks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Test | ||
| public void testBuilderWithSkipRequiredChecks() { | ||
| // Should not throw IllegalArgumentException when skipRequiredChecks is true and graph is null | ||
| HugeClientBuilder builder = new HugeClientBuilder("http://127.0.0.1:8080", "DEFAULT", null, true); | ||
| try { | ||
| builder.build(); | ||
| } catch (IllegalArgumentException e) { | ||
| Assert.fail("Should not throw IllegalArgumentException when skipRequiredChecks is true, but got: " + e.getMessage()); | ||
| } catch (Exception e) { | ||
| // Expected since there is probably no server running at localhost:8080 | ||
| // The fact we reach here means the bypass of graph/url check was successful. | ||
| } |
There was a problem hiding this comment.
These tests call builder.build(), which constructs HugeClient and triggers server API version checks over HTTP. If no server is running, this can block up to the default 20s timeout per test, making the suite slow/flaky. Consider avoiding the network path (e.g., assert only that constructor/build validation doesn’t throw) or explicitly set very small connect/read timeouts before build() so failures return quickly.
| @Test(expected = IllegalArgumentException.class) | ||
| public void testBuilderWithoutSkipRequiredChecks() { | ||
| // Should throw exception when skipRequiredChecks is false and graph is null | ||
| HugeClientBuilder builder = new HugeClientBuilder("http://127.0.0.1:8080", "DEFAULT", null, false); | ||
| builder.build(); | ||
| } |
There was a problem hiding this comment.
@Test(expected = IllegalArgumentException.class) here is satisfied by the exception thrown in the constructor (because graph is null) before build() is reached, so it doesn’t actually validate the new conditional checks inside build(). To cover the build() path, construct with a non-null graph, then set configGraph(null) (and/or configUrl(null)) before calling build() and asserting the expected behavior with/without skipRequiredChecks.
|
|
||
| import org.apache.hugegraph.driver.HugeClient; | ||
| import org.apache.hugegraph.driver.HugeClientBuilder; | ||
| import org.apache.hugegraph.rest.ClientException; |
There was a problem hiding this comment.
Unused import ClientException should be removed to keep the test file clean and avoid failing builds in environments that treat unused imports as errors (e.g., stricter compiler/lint settings).
| import org.apache.hugegraph.rest.ClientException; |
| public static HugeClientBuilder builder(String url, String graphSpace, String graph) { | ||
| return new HugeClientBuilder(url, graphSpace, graph); | ||
| } | ||
|
|
||
| public static HugeClientBuilder builder(String url, String graphSpace, String graph, | ||
| boolean skipRequiredChecks) { | ||
| return new HugeClientBuilder(url, graphSpace, graph, skipRequiredChecks); | ||
| } |
There was a problem hiding this comment.
PR description indicates this does not affect the public API, but this adds a new public overload HugeClient.builder(..., boolean skipRequiredChecks) (and a new public HugeClientBuilder constructor signature). The PR metadata/checkboxes should be updated to reflect that this is a public API change.
| private Map<String, Map<String, Map<HugePermission, Map<String, | ||
| List<HugeResource>>>>> roles; | ||
|
|
||
| public Map<String, Map<String, Map<HugePermission, Map<String, List<HugeResource>>>>> roles() { | ||
| public Map<String, Map<String, Map<HugePermission, Map<String, | ||
| List<HugeResource>>>>> roles() { |
There was a problem hiding this comment.
These wrapped generic type lines include trailing whitespace before the newline (after the comma). This is easy to miss in reviews and can create noisy diffs or fail whitespace-sensitive tooling; please remove the trailing spaces while keeping the line wrap.
Purpose of the PR
This pull request introduces a new option to bypass required argument checks when building a
HugeClientBuilder, allowing for more flexible client initialization. The main changes include updating constructors and builder methods to support theskipRequiredChecksflag, modifying validation logic, and adding unit tests to verify this new behavior.Main Changes
Enhancements to client builder flexibility:
skipRequiredChecksboolean parameter to theHugeClientBuilderconstructor and the correspondingHugeClient.builderstatic method, allowing users to bypass mandatory checks forurlandgrapharguments when needed.build()method ofHugeClientBuilderto conditionally skip argument checks based on theskipRequiredChecksflag.Testing:
HugeClientBuilderTestwith tests to ensure that the builder behaves correctly with and without theskipRequiredChecksflag, including cases where required arguments are missing.Verifying these changes
Does this PR potentially affect the following parts?
Documentation Status
Doc - TODODoc - DoneDoc - No Need