Skip to content

chore(server): bump REST API version - #3159

Open
imbajin wants to merge 23 commits into
apache:masterfrom
hugegraph:cx/bump-server-api-version
Open

chore(server): bump REST API version#3159
imbajin wants to merge 23 commits into
apache:masterfrom
hugegraph:cx/bump-server-api-version

Conversation

@imbajin

@imbajin imbajin commented Aug 16, 2026

Copy link
Copy Markdown
Member

Closeout TODO

  • Publish REST API 0.72 consistently through source properties, the API
    JAR manifest, and the runtime version endpoint.
  • Add GraphSpace-wide default-role mutations for existing and future graphs
    while keeping historical per-graph observer grants removable.
  • Prepare the audit limiter during login so the first authenticated write
    succeeds without a warm-up request.
  • Let global administrators and GraphSpace managers update or delete schema
    templates within their authority.
  • Preserve the creator-owned mutation path without eagerly requiring an
    authenticator in anonymous mode.
  • Keep Kubernetes peer stability in stable StatefulSet/Service DNS and
    remove the earlier Server-side PD dynamic IP/allowlist refresh.
  • Use 120 columns consistently in Checkstyle, EditorConfig, contributor
    guidance, and current-PR formatting without reformatting legacy sources.
  • Keep future TinkerPop 3.7 merge-step behavior outside this 3.5.1 PR.
  • Resolve every current review thread.
  • Finish all current-head checks except explicitly deferred Codecov and the ignored dependency-review job.
  • Obtain the remaining upstream approval.

1.5–1.8 compatibility

  • 1.8/current master — exposes the new API 0.72 GraphSpace and template
    authorization contract used by current Hubble.
  • 1.7 — remains on the legacy API 0.71 contract; compatibility handling
    stays in the Client/Hubble boundary.
  • 1.5 — unchanged; auth/PD compatibility work is outside this Server PR.

Before → After

flowchart LR
    B["Before<br/>master and 1.7 both reported API 0.71<br/>future-graph roles were incomplete<br/>first writes needed prior limiter state<br/>PD address refresh leaked into Server"]
    A["After<br/>master reports API 0.72<br/>GraphSpace-wide default roles<br/>first authenticated write works directly<br/>Kubernetes DNS owns peer stability"]
    B --> A
Loading
Area Before After
API capability Current master was indistinguishable from 1.7 Current master reports API 0.72
GraphSpace roles Observer grants were tied to known graphs The all-graphs target covers existing and future graphs
First write Audit limiting could be initialized after the first mutation Login prepares the limiter before authenticated writes
Template ownership Manager checks could eagerly require auth state Owners keep the direct path; managers use scoped authorization
Kubernetes peers Server contained PD-driven dynamic address refresh Stable StatefulSet/Service DNS remains the single owner
Source style Java wrapping guidance stopped at 100 columns New and current-PR code uses a single 120-column threshold

Current state

  • Server head: 7083242676277b2536614e4804c4f93313707bed
  • Product source matches validated 28641f54; the only later diff is the dependency-review Action upgrade
  • Previous matching-tree checks: 21/21 passed
  • Unresolved review threads: 0
  • GitHub checks: 20 effective checks passed; Codecov is deferred and dependency-review v5 was canceled/ignored after stalling on the oversized summary
  • Merge gate: upstream review required

@dosubot dosubot Bot added size:XS This PR changes 0-9 lines, ignoring generated files. api Changes of API labels Aug 16, 2026
- identify the default-role REST contract as API 0.72
- preserve 1.7 releases at API 0.71 for client compatibility
- document when the manifest version must change
- install reactor outputs before assembling Server images
- isolate and lock Maven caches by source revision
- verify packaged API versions against source in CI
- run Docker CI for every reactor source change
@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Aug 16, 2026
@codecov

codecov Bot commented Aug 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 118 lines in your changes missing coverage. Please review.
✅ Project coverage is 0.34%. Comparing base (431f6e6) to head (7083242).

Files with missing lines Patch % Lines
.../org/apache/hugegraph/auth/HugeGraphAuthProxy.java 0.00% 61 Missing ⚠️
.../org/apache/hugegraph/api/space/GraphSpaceAPI.java 0.00% 20 Missing ⚠️
...a/org/apache/hugegraph/auth/HugeAuthenticator.java 0.00% 10 Missing ⚠️
...java/org/apache/hugegraph/api/auth/ManagerAPI.java 0.00% 9 Missing ⚠️
...g/apache/hugegraph/auth/StandardAuthManagerV2.java 0.00% 8 Missing ⚠️
.../apache/hugegraph/api/space/SchemaTemplateAPI.java 0.00% 7 Missing ⚠️
...n/java/org/apache/hugegraph/core/GraphManager.java 0.00% 3 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (431f6e6) and HEAD (7083242). Click for more details.

HEAD has 4 uploads less than BASE
Flag BASE (431f6e6) HEAD (7083242)
5 1
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #3159       +/-   ##
============================================
- Coverage     41.06%   0.34%   -40.72%     
+ Complexity      519      74      -445     
============================================
  Files           771     749       -22     
  Lines         65962   63520     -2442     
  Branches       8766    8345      -421     
============================================
- Hits          27088     220    -26868     
- Misses        36008   63277    +27269     
+ Partials       2866      23     -2843     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- normalize configured and runtime peer addresses
- enforce DNS-aware IP authorization for raft traffic
- refresh peer allowlists during membership changes
- cover service updates and raft authorization integration
- persist observer access against the all-graphs target
- apply read-only access to existing and future graphs
- migrate and remove legacy graph-scoped observer grants
- document the GraphSpace-wide contract in API version 0.72
@dosubot dosubot Bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Aug 17, 2026
@imbajin

imbajin commented Aug 17, 2026

Copy link
Copy Markdown
Member Author

Why this branch contains PD Raft changes

The PD change in 3d9d9544 is independent of the REST API 0.72 bump. It was identified while preparing the real 3-PD distributed matrix, where PD peers use stable hostnames but container/Pod IPs may change after restart or membership updates.

Before:

  • IpAuthHandler resolved peer hostnames only at startup or an explicit peer-list update;
  • the same hostname resolving to a new Pod IP could leave the Raft allowlist stale and reject a valid peer;
  • peer changes could update Raft membership without a reliably synchronized allowlist;
  • a fully unresolved/empty result followed the old allow-all path;
  • resolver, singleton, RPC-server and failed-start cleanup were incomplete.

After:

  • configured/runtime peer addresses are normalized and bounded before use;
  • DNS resolution has timeout, concurrency and stale-entry limits, with background refresh;
  • successful membership changes refresh the active allowlist;
  • startup/shutdown failures clean the resolver, handler and RPC resources;
  • unit, service-update and Raft integration tests cover these boundaries.

Most of the apparent size is defensive behavior and tests: this commit is +1375/-226, including about 700 added test lines and relocation/expansion of the old IpAuthHandlerTest.

Scope note: this is not required to distinguish API 0.71 from 0.72, nor by the Hubble permission capability itself. It should be reviewed as a standalone PD reliability/security change (and preferably split from the API-version PR). Passing tests do not replace the pending final 3+3+3 runtime validation.

Scope freeze

The Hubble dependency is limited to the API 0.72 contract and the GraphSpace-wide default-role behavior. The PD Raft DNS/IP work is independent and must not grow further except for a blocker reproduced by the required 3+3+3 runtime. It should remain separately reviewable/splittable from the Hubble compatibility change.

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Summary: No actionable code defect was independently confirmed at this head. Approval remains blocked by the current codecov/project failure. Evidence: Java 11 targeted tests passed: PD auth/raft 20/20, PDService raft update 6/6, GraphSpace API 13/13; current checks show codecov/project FAILURE.

- classify mutation steps from Gremlin bytecode
- require write access for add and property steps
- require delete access for drop steps
- cover read write delete and nested traversals
- inspect realized traversal steps after script evaluation
- cover vertex edge property and drop mutations
- recurse through nested child traversals
- keep read-only traversals executable
- keep auth wrappers when strategies are copied to script traversals
- align strategy list behavior with its iterator
- cover the copied-strategy contract
- preserve structured mutation checks after script evaluation
- keep membership roles out of data action matching
- preserve explicit read write and delete permissions
- verify members can read without gaining mutations
- retain direct GraphSpace administrator handling
- initialize audit limiter only after successful authentication
- keep failed password and token attempts out of limiter state
- invalidate limiter entries by username when deleting users
- remove PD dynamic DNS and IP refresh from this PR
- cover password token and cleanup paths with unit tests
@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XXL This PR changes 1000+ lines, ignoring generated files. labels Aug 17, 2026
- run metadata callbacks with an internal admin context
- restore the previous context on success or failure
- prevent admin propagation into callback child threads
- cover task override and context restoration boundaries
@dosubot dosubot Bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Aug 17, 2026
- allow space managers to inspect users in their own space
- reject users without current-space grants and global admins
- cover cross-space and multi-space permission boundaries
- recognize custom global admins for user updates
- allow custom global admins to delete ordinary users
- preserve builtin admin behavior and deletion safeguards
- cover builtin and custom admin mutation paths
- align template ownership with global admin semantics
- preserve creator and GraphSpace manager access
- cover all four template management roles
- align Checkstyle and EditorConfig at 120 columns
- update contributor and module style guidance
- compact only current PR code without legacy reformatting
- defer authenticator lookup until manager access is needed
- keep anonymous creators on the owner mutation path
- cover lazy owner and manager authorization paths
bitflicker64 added a commit to hugegraph/hugegraph that referenced this pull request Aug 18, 2026
Bring in server REST API 0.72 changes (PR apache#3159) for local Hubble auth testing.

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Summary: The new Gremlin mutation pre-check misses TinkerPop merge steps, so execute-only users can reach mutation paths without a required WRITE check. Evidence: traversalPermissions() only recognizes AddVertexStartStep, AddVertexStep, AddEdgeStartStep, AddEdgeStep, AddPropertyStep, and DropStep at HugeGraphAuthProxy.java:2533-2541; TinkerPop 3.7.6 also has MergeVertexStep and MergeEdgeStep, whose on-match path can call Vertex.property directly. Please classify both merge steps as WRITE (including nested children) and add execute-only regression tests for mergeV/mergeE create and onMatch cases.

- recognize mergeV and mergeE as write operations
- preserve compatibility with the current TinkerPop baseline
- reject execute-only create and onMatch traversals
- verify recursive child traversal permissions
- match merge steps by exact TinkerPop class names
- retain superclass traversal for provider implementations
- reject unrelated steps sharing merge simple names
- preserve recursive child permission coverage
- add pinned-3.5 test fixtures for TinkerPop merge steps
- route vertex and edge merge shapes through strategy checks
- retain external same-name and child traversal regressions
@dosubot dosubot Bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Aug 18, 2026
- move compatibility fixtures into test output only
- add test output to the unit-test classpath
- construct exact-package fixtures reflectively
- keep main artifacts free of TinkerPop shadow classes
@dosubot dosubot Bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:XXL This PR changes 1000+ lines, ignoring generated files. labels Aug 18, 2026
- select fixture sources from the pinned TinkerPop version
- keep 3.5.1 compatibility classes in test output only
- stop selecting fixtures automatically after a version change
- preserve merge permission regression coverage
@dosubot dosubot Bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Aug 18, 2026
- prefer the official traversal and isStart constructor
- use Merge.onMatch through reflection when available
- keep an explicit 3.5.1 fixture child fallback
- preserve merge authorization coverage
- restore the TinkerPop 3.5.1 authorization scope\n- remove future-version merge detection and fixtures\n- keep the Hubble permission closeout focused on reproduced behavior
@dosubot dosubot Bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:XXL This PR changes 1000+ lines, ignoring generated files. labels Aug 18, 2026
- move dependency review action from v3 to the Node 24 v5 release
- use the supported oversized-summary handling
- keep existing severity and license policy unchanged

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Summary: The current head has authorization correctness and isolation regressions in GraphSpace role checks, Gremlin mutation enforcement, and callback context cleanup. Evidence: exact-head static review plus six independent read-only lanes; latest checks pass for builds/Docker/CodeQL, while dependency-review is cancelled and Codecov failures are non-blocking.

if (hasGraph) {
result = authManager.isDefaultRole(name, graph, user,
defaultRole);
result = authManager.isDefaultRole(name, graph, user, defaultRole);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

‼️ Critical: A space-wide OBSERVER is persisted through createSpaceDefaultRole() under the ALL_GRAPHS marker, but this graph-specific branch checks only isDefaultRole(name, graph, user, defaultRole). A user with the new space-wide role is therefore reported as false when the same check includes a concrete graph parameter; the parallel ManagerAPI path has the same omission. Please treat the ALL_GRAPHS role as covering the requested graph, and add a regression test for both endpoints.

return rolePerm.contains(grant);
RolePermission grantedRole = RolePermission.fromJson(grant);
RolePerm rolePerm = RolePerm.fromJson(role);
if (resourceObject != null &&

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Important: This new authorization shortcut returns true when the operator is a SPACE manager for the current graph space and the target grant merely contains an entry for that space. UserAPI.role() then serializes the complete rolePermission(user) object, so a space-a manager can read the space-b entries of a multi-space grant. Please return a graph-space-scoped projection (or reject mixed grants) before allowing this path, and add a cross-space response-isolation test.

Traversal.Admin<?, ?> traversal,
Set<HugePermission> permissions) {
for (Step<?, ?> step : traversal.getSteps()) {
if (step instanceof AddVertexStartStep ||

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

‼️ Critical: The classifier only recognizes concrete Add*, AddPropertyStep, and DropStep instances. A Groovy lambda such as g.V().sideEffect { it.get().property('k', 'v') } contains none of these steps, while vertices() returns raw graph elements whose property() mutates the backend directly. An EXECUTE-only user can therefore bypass the new WRITE check. Please reject or sandbox lambda mutations, or route element mutations through an authorized proxy; add an execute-only strategy-level regression that calls apply() rather than only reflecting over the classifier.

grpcThread = Thread.currentThread().getName().contains("grpc");
if (grpcThread) {
HugeGraphAuthProxy.setAdmin();
if (Thread.currentThread().getName().contains("grpc")) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

‼️ Critical: The previous gRPC listener wrapper cleared authentication state in finally; this path now scopes only AuthContext through runAsAdmin(). HugeGraphAuthProxy.CONTEXTS is an InheritableThreadLocal, and REQUEST_GRAPH_SPACE is not cleared here, so a reused or inherited listener thread can retain a prior request identity/graph space after the callback or an exception. Please clear or save/restore all auth thread-locals at this listener boundary in finally, and add a thread-reuse/exception regression test.

@@ -2327,7 +2377,9 @@ public TraversalStrategiesProxy(TraversalStrategies strategies) {

@Override
public List<TraversalStrategy<?>> toList() {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Important: TinkerPop's TraversalStrategies.toList() contract requires an immutable list, and both the interface default and DefaultTraversalStrategies wrap their result with Collections.unmodifiableList. This override returns a mutable ArrayList, allowing callers to add/remove strategy proxies and changing the public API contract. Please return Collections.unmodifiableList(proxies) and add an immutability regression test.

- 'hugegraph-server/hugegraph-dist/src/assembly/static/bin/util.sh'
- '.mvn/**'
- 'pom.xml'
- 'hugegraph-commons/**'

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Important: Removing the old **/Dockerfile* filter leaves the repository's docker/hbase/** tree outside every pull_request.paths entry, although it still contains a Dockerfile, entrypoint, and HBase configuration. Changes to that image will no longer trigger Docker Build CI and can merge without any image validation. Please retain docker/hbase/** in the trigger or add a dedicated HBase build job with checks appropriate to that image.

if (hasGraph) {
authManager.deleteDefaultRole(name, user, defaultRole, graph);
} else {
authManager.deleteDefaultRole(name, user, defaultRole);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Important: Space-wide OBSERVER deletion removes the ALL_GRAPHS role first and then performs independent per-graph deletes. If any later metadata delete fails, the request returns an error while the global role is already gone and some legacy graph-level grants remain active, leaving authorization state partially migrated. Please make this migration transactional or explicitly idempotent/resumable with compensation, and cover an injected mid-loop failure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api Changes of API size:XL This PR changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant