Skip to content

[Feature] Unify REST API response format for server, pd and store modules #2975#3029

Open
BrandaoFelipe wants to merge 6 commits into
apache:masterfrom
BrandaoFelipe:restApiResponseUnified
Open

[Feature] Unify REST API response format for server, pd and store modules #2975#3029
BrandaoFelipe wants to merge 6 commits into
apache:masterfrom
BrandaoFelipe:restApiResponseUnified

Conversation

@BrandaoFelipe
Copy link
Copy Markdown

Purpose of the PR

The purpose of this PR is to standardize and unify the REST API global error response format across the core modules (pd, server and store/hg-store-node) using the centralized ApiResponse envelope wrapper.

Currently, unhandled exceptions or module-specific errors could return inconsistent formats or expose raw stack traces to the client. This tracking change enforces a clean, predictable JSON error contract across different frameworks used in the ecosystem (Jersey and Spring Boot), enhancing API security and client-side error handling.

Main Changes

This PR introduces centralized exception mapping logic across two distinct sub-framework layers within the project:

1. hugegraph-server module (Jersey / JAX-RS)

  • HugeExceptionMapper: Intercepts all HugeException occurrences. It safely resolves the underlying root cause using a recursive rootCause() lookup and maps internal core errors directly to appropriate HTTP Status Codes (e.g., IllegalArgumentException -> 400 Bad Request, NoSuchElementException -> 404 Not Found) wrapped cleanly inside ApiResponse.
  • GenericExceptionMapper: Implemented as a top-level global catch-all provider for Exception.class to prevent infrastructure leakages, mapping unexpected exceptions to a secure HTTP 500 "An unexpected error occurred" message payload.

2. hugegraph-store module (hg-store-node sub-module / Spring Boot)

  • StoreExceptionHandler: Created using Spring's @RestControllerAdvice to intercept HgStoreException. It dynamically parses the exception's internal error code to differentiate client errors from infrastructure faults based on explicit ranges:
    • Codes [1200, 1300) (e.g., RocksDB storage failures) -> 500 Internal Server Error (logged as SEVERE/ERROR with full stack trace).
    • Codes [1000, 1200) (e.g., Unsupported data formats) -> 400 Bad Request (logged cleanly as a WARNING message).
  • GenericExceptionMapper: Added as a fallback interceptor for standard Exception.class instances inside the store node rest controllers to ensure uniform 500 error envelopes using the Log4j wrapper.

3. hugegraph-pd module (hg-pd-service sub-module / Spring Boot)

  • PdExceptionHandler: Implemented via @RestControllerAdvice to handle metadata and coordination cluster exceptions (PDException). Centralizes cluster control plane failure logging (Raft state, partition routing) and converts them into the unified ApiResponse schema.
  • Catch-All Filter: Includes standard Exception.class mapping to gracefully mask unhandled runtime infrastructure glitches with a generic "An unexpected error occurred in the cluster control plane" feedback text.

Verifying these changes

  • Need tests and can be verified as follows:
    • Executed mvn clean compile -DskipTests from the root directory to confirm proper multi-module compile-time dependency alignment (validating ApiResponse cross-package visibility inside hg-store-node).
    • Validated error response payloads against the unified schema structure ensuring it adheres to the contract:
      {
        "status": 400,
        "message": "Specific validation/error message string",
        "data": null,
        "statusText": "Bad Request"
      }

Does this PR potentially affect the following parts?

  • Dependencies
  • Modify configurations
  • The public API
  • Other affects (typed here)
  • Nope

Documentation Status

  • Doc - TODO
  • Doc - Done
  • Doc - No Need

@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. api Changes of API feature New feature labels May 16, 2026
@imbajin
Copy link
Copy Markdown
Member

imbajin commented May 17, 2026

I think the current implementation does not actually establish one unified runtime path yet. It adds new mappers/advice, but several existing paths either bypass them or now compete with them.

Current shape:

Expected:
  server / pd / store
        -> one clear response contract

Current PR:
  server -> old ExceptionFilter + new mapper providers coexist
  pd     -> many controller-level catch blocks still return old RestApiResponse
  store  -> HgStoreException business code is replaced by generic HTTP 400/500 in the body

The main design issues are:

  1. PD exceptions are often caught before @RestControllerAdvice can see them. Many PD REST APIs already catch PDException and directly return RestApiResponse or the existing toJSON(e) format, so a new PDExceptionMapper only covers exceptions that escape the controller.
PD controller
  -> catch PDException
  -> return old RestApiResponse / old JSON
  -> new advice is bypassed
  1. Server now has two competing Jersey exception-mapping paths. The server module already has ExceptionFilter with mappers for HugeException, IllegalArgumentException, NotFoundException, NoSuchElementException, WebApplicationException, HugeGremlinException, and unknown exceptions. This PR adds another provider package under org.apache.hugegraph.api, which is scanned by ApplicationConfig.
ApplicationConfig packages("org.apache.hugegraph.api")
        -> existing ExceptionFilter.*Mapper
        -> new exceptionshandler.*Mapper

That makes the effective behavior unclear. If the new HugeExceptionMapper wins, it also maps most HugeException cases to HTTP 400 by default, which can incorrectly classify server-side failures as client errors.

  1. Store loses the original HgStoreException business code. HgStoreException has useful domain codes such as 1001, 1201, 1202, 1208, 1401, etc. But the new StoreExceptionHandler returns status.value() as the response code, so clients only see generic 400 or 500 instead of the actionable store error code.
Before:
  HgStoreException code = 1202

After this PR:
  response.code = 500

I suggest first defining the response contract and then updating the existing runtime paths instead of adding parallel ones:

PD:
  handle existing catch-and-return paths, or migrate/normalize RestApiResponse

Server:
  modify the existing ExceptionFilter path instead of adding competing mappers

Store:
  keep HgStoreException business code while using HTTP status for transport semantics

Tests:
  add REST-level tests that prove actual endpoints return the unified format

One unrelated item: hugegraph-pd/hg-pd-service/pom.xml adds <classifier>exec</classifier>, which changes the PD packaging output and does not seem related to REST response unification. Please split it out or explain why it is required here.

@imbajin
Copy link
Copy Markdown
Member

imbajin commented May 17, 2026

A larger concern is that this PR changes the public REST API response contract. The server / pd / store REST APIs may already be consumed by external services, so a large response-format change can have significant downstream impact.

Current response shapes include fields such as:

server errors:
  exception / message / cause / trace

pd responses:
  message / data / status
  or status / error

store errors:
  module-specific business error codes

The new envelope introduces a different shape:

ApiResponse:
  code / message / data / status

That can break clients that parse existing fields like exception, error, status, or store-specific business codes. Even if the new format is cleaner, this kind of API contract change should be designed and discussed carefully before implementation.

I think we should first decide the compatibility and migration strategy, for example:

Questions to settle before implementation:
  - Is this intended to be a breaking public API response change?
  - If not, how do old clients keep receiving the old format?
  - Should the unified format be opt-in through a version/header/parameter?
  - Is there a deprecation period with compatibility fields?
  - Should normal responses also be wrapped, or only error responses?
  - Is `code` the HTTP status or the business error code?
  - What should be documented in REST docs / OpenAPI / release notes?

If this is intended to be a breaking change, it should probably go through community discussion and come with docs, migration notes, OpenAPI updates, and REST-level compatibility tests. If it is not intended to be breaking, then the implementation needs an explicit compatibility layer rather than replacing existing response bodies directly.

In short:

First:
  define API contract + compatibility/migration plan

Then:
  implement the unified response format against that contract

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

Labels

api Changes of API feature New feature size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Unify REST API response format for server,pd and store modules

2 participants