imbajin commented on PR #3029:
URL: https://github.com/apache/hugegraph/pull/3029#issuecomment-4469401272
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:
```text
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.
```text
PD controller
-> catch PDException
-> return old RestApiResponse / old JSON
-> new advice is bypassed
```
2. 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`.
```text
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.
3. 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.
```text
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:
```text
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.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
