Repository navigation
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3241 +/- ##
============================================
+ Coverage 41.40% 41.45% +0.05%
- Complexity 7337 7349 +12
============================================
Files 802 803 +1
Lines 69792 69817 +25
Branches 9312 9319 +7
============================================
+ Hits 28897 28944 +47
+ Misses 37604 37577 -27
- Partials 3291 3296 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
One or more issues must be addressed before approval.
Review effort: Lite
Findings: 1
Open (4)
What changed in this PR
Adds structured Cypher error metadata with stable codes and hints, exposing it through the API and covering representative failures.
Changes:
- Adds
errors[].code,message, andhintto responses. - Maps common Cypher failures to structured classifications.
- Adds API tests for syntax and schema errors.
| File | Description |
|---|---|
| hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/api/CypherApiTest.java | Updated as part of this pull request. |
| hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/cypher/CypherModel.java | Updated as part of this pull request. |
| hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/cypher/CypherErrorMapper.java | Updated as part of this pull request. |
| hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/cypher/CypherClient.java | Updated as part of this pull request. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (lower.contains("not defined") && lower.contains("$")) { | ||
| return new CypherModel.CypherError(MISSING_PARAMETER, message, | ||
| "Provide the value via the 'parameters' query argument, " + | ||
| "e.g. ?parameters=%7B%22city%22%3A%20%22Beijing%22%7D"); |
| return new CypherModel.CypherError(MISSING_PARAMETER, message, | ||
| "Provide the value via the 'parameters' query argument, " + | ||
| "e.g. ?parameters=%7B%22city%22%3A%20%22Beijing%22%7D"); |
| @Schema(description = "Structured errors, empty on success") | ||
| public List<CypherError> errors = Collections.emptyList(); |
| assertContains("\"errors\"", content); | ||
| assertContains("SyntaxError", content); | ||
| assertContains("Declare the variable", content); |
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The new top-level errors field is written on every /cypher response, including successful ones, and hugegraph-client cannot parse a response that carries it, so HugeClient.cypher() and Hubble's Cypher queries fail after this change. The mapper also labels real parse errors as ExecutionError, so the new SyntaxError code does not identify syntax errors. Evidence: I ran RestResult.readObject(Response.class) from hugegraph-client 1.8.0 on the response shape before and after this PR; the old shape parses, the new one throws UnrecognizedPropertyException: Unrecognized field "errors". I ran opencypher-gremlin translation 1.0.4 with HugeGraph's default translator definition to capture the messages the mapper sees. CI at ed8344d is green; the in-repo tests read the body as a raw string and do not exercise the Java client.
| public Result result = new Result(); | ||
|
|
||
| @Schema(description = "Structured errors, empty on success") | ||
| public List<CypherError> errors = Collections.emptyList(); |
There was a problem hiding this comment.
Critical: This field breaks every existing Java client call to /cypher, successful queries included. errors defaults to Collections.emptyList() and Jersey's JacksonFeature serializes it, so every response now ends with "errors":[]. hugegraph-toolchain's CypherAPI.post parses the body with RestResult.readObject(org.apache.hugegraph.structure.gremlin.Response.class). That class declares only requestId, status and result and has no ignoreUnknown, and RestResult uses a plain ObjectMapper, where FAIL_ON_UNKNOWN_PROPERTIES is on by default.
Evidence: with hugegraph-client-1.8.0.jar and hugegraph-common-1.7.0.jar, new RestResult(200, body, null).readObject(Response.class) parses the current response shape. With ,"errors":[] appended it throws SerializeException caused by UnrecognizedPropertyException: Unrecognized field "errors" (class org.apache.hugegraph.structure.gremlin.Response), not marked as ignorable. CypherManager.execute() and Hubble's QueryService.executeCypher() both go through that path. CI does not catch this because CypherApiTest checks the body as a string.
Requested change: keep the top-level shape unchanged. The existing status.attributes map (Map<String, Object>, already accepted by the client as Map<String, ?>) can carry the code and hint on failures, for example attributes = {"code": ..., "hint": ...}. If a top-level errors field is still wanted, land @JsonIgnoreProperties(ignoreUnknown = true) on the client's Response in hugegraph-toolchain and release it first, and do not emit the field on success in the meantime.
| "Create the schema element via the schema API before " + | ||
| "running the query"); | ||
| } | ||
| if (lower.contains("not defined") || lower.contains("undefined")) { |
There was a problem hiding this comment.
Important: SyntaxError is assigned only when the message contains "not defined" or "undefined", so real parse errors fall through to ExecutionError with an empty hint on line 65. The PR's goal is to let clients branch on the failure mode, and these codes are described as stable, so a client that checks for HugeGraph.Cypher.SyntaxError will miss most syntax errors.
Evidence: I ran CypherAst.parse(q, {}) from translation 1.0.4 with gremlin+cfog_server_extensions+inline_parameters:
MATCH (n:person RETURN ngivesSyntaxException: Invalid input 'R': expected ..., mapped toExecutionError.MATCH (n:person) RETRUN ngivesInvalid input 'R': expected 'u/U', mapped toExecutionError.MATCH (n) RETURN n; MATCH (m) RETURN mgivesExpected exactly one statement per query but got: 2, mapped toExecutionError.RETURN date()givesSyntaxException: Unknown function 'date', mapped toExecutionError.MATCH (n:person) RETURN not_defined_vargives "Variablenot_defined_varnot defined", the only one mapped toSyntaxError.
The PR description's own live check shows the same result: a syntax error returned HugeGraph.Cypher.ExecutionError with "hint":"". The branch also catches any other HugeGraph message with "undefined", such as Undefined index label: 'x', and tells the user to declare a variable.
Requested change: match the parser messages explicitly (invalid input, expected exactly one statement, unknown function, variable ... not defined) for SyntaxError, narrow the variable hint to the Variable ... not defined message, and add a test that sends a real parse error such as MATCH (n:person RETURN n and asserts "code":"HugeGraph.Cypher.SyntaxError".
| "^([a-zA-Z0-9_]+\\.)+[A-Za-z0-9_]+(Exception|Error):\\s*", ""); | ||
| String lower = message.toLowerCase(); | ||
|
|
||
| if (lower.contains("not defined") && lower.contains("$")) { |
There was a problem hiding this comment.
Minor: This branch cannot fire with the server's default configuration, so MissingParameter is a published code that is never returned. CypherClient.createRequest() sends no bindings, and CypherOpProcessor uses inline_parameters. With an empty parameter map, translation 1.0.4 turns MATCH (n:person) WHERE n.name = $name RETURN n into a traversal that compares against ' cypher.null' and raises no error, so the query returns no rows. Variable-not-defined messages never contain $.
Requested change: drop MISSING_PARAMETER and this branch until the API accepts parameters, together with the ?parameters= hint already raised on line 46.
|
Pushed ca0ef91: Success responses keep the old shape now — The SyntaxError routing was defeated by our own prefix-stripping: the class prefix was stripped before classification, so Agreed on the test point; the strict-parse test enforces the exact wire contract (no unknown top-level fields on success) without depending on client internals. build-server (memory/hbase/rocksdb) is green on the fix commit. |
|
Correction to my comment above: ca0ef91 is not on this PR. I pushed it to a different branch on my fork (u5-error-metadata), while this PR's head is still ed8344d, so the changes described there (legacy-shaped success responses, the SyntaxError classification fix and the strict-parse test) are not in the diff here yet. I'm holding off on pushing them for now and will update this PR once the related docs PR has settled. Until then please review this PR as it stands at ed8344d, and treat the earlier comment as describing a pending change, not the current state. Sorry for the confusion. |
…lures Signed-off-by: Nguyen Quang Minh <135627235+n24q02m@users.noreply.github.com>
…rse failures - CypherModel.errors: @JsonInclude(NON_EMPTY) so successful responses keep the exact pre-PR shape (older hugegraph-client RestResult fails on unknown top-level fields) - CypherErrorMapper: detect translator/parser Syntax/Parse exceptions by class name and full message (incl. ANTLR markers) BEFORE the class-prefix strip, so real parse failures map to SyntaxError even when wrapped or carrying generic messages - CypherApiTest: regression test parses a success response with a strict FAIL_ON_UNKNOWN_PROPERTIES mapper reproducing the client 1.8.0 failure mode
ed8344d to
aabdde7
Compare
…parse errors The Cypher endpoint has no parameter plumbing, so the ?parameters= hint sent users to an argument that does nothing. Remove the MISSING_PARAMETER code and fall through to the generic classification. Add a test with a genuine parse error (unclosed node pattern) asserting the HugeGraph.Cypher.SyntaxError code.
…xError The openCypher front-end reports a malformed statement as "Invalid input 'R': expected ...", which none of the existing parse-failure markers matched, so the response was labelled ExecutionError with no hint.
|
Thanks for the detailed evidence. I rebased onto current master and pushed
|
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The NON_EMPTY change fixes successful responses, but every failed /cypher query still returns a top-level errors array, which hugegraph-client cannot deserialize, so Java client and Hubble users get a Jackson error in place of the Cypher error they see today. Separately, the unsupported branch labels HugeGraph optimizer and backend limits as translator gaps. Evidence: exact head 16cd32e81331a4f57348b063ce2d2cac73984476 read with git show 16cd32e8:<path>; client behaviour reproduced with hugegraph-client-1.8.0.jar, hugegraph-common-1.7.0.jar and Jackson 2.15.2 by reading the failure body with and without the new field; client and Hubble call paths read on hugegraph-toolchain master bcd5ce21 (CypherAPI.post, CypherManager.execute, Response, QueryService.executeCypher). HugeGraph-Server CI at this head is action_required (waiting for maintainer approval), so no CI result covers it. Still open from earlier comments and not repeated here: the not defined/undefined branch also catches Undefined index label and gives the variable hint, and Unknown function / Expected exactly one statement still fall through to ExecutionError.
| res.status.code = 400; | ||
| res.status.message = message; | ||
| if (error != null) { | ||
| res.errors = Collections.singletonList(error); |
There was a problem hiding this comment.
Important: Failed queries still break the Java client and Hubble, because failOf always adds a top-level errors array. The NON_EMPTY change fixed only the success path.
hugegraph-client's CypherAPI.post parses every /cypher body with RestResult.readObject(Response.class). Response declares only requestId, status and result, has no @JsonIgnoreProperties, and RestResult uses a default ObjectMapper, so the unknown field throws before any caller sees status. On toolchain master, CypherManager.execute() never reaches its status.code() != 200 branch, which is what turns the server's status.message into a ServerException. Hubble's QueryService.executeCypher() then gets a SerializeException (Failed to deserialize: ...) through its ClientException branch, where it used to get the Cypher error through the ServerException branch.
Evidence: with hugegraph-client-1.8.0.jar, hugegraph-common-1.7.0.jar and Jackson 2.15.2, a default ObjectMapper reads today's failure body ({"requestId":"r1","status":{"message":"Variable x not defined","code":400,"attributes":{}},"result":{"data":[],"meta":{}}}) into Response with status.code=400. The same body with this PR's errors entry appended throws UnrecognizedPropertyException: Unrecognized field "errors" (class org.apache.hugegraph.structure.gremlin.Response), not marked as ignorable. testSuccessResponseKeepsLegacyShape covers only the success body, and the three failure tests read the body as a string, so nothing in the build catches this.
Requested change: to answer your open question, please move the code and hint into status.attributes (for example {"code": ..., "hint": ...}) and drop the top-level errors field. CypherModel.Status.attributes already exists and the client maps it as Map<String, ?>, so no client release is needed. Then add a failure-path test that parses a failed response with FAIL_ON_UNKNOWN_PROPERTIES enabled, the way the success test does.
| "Declare the variable in a MATCH, UNWIND or WITH clause " + | ||
| "before referencing it"); | ||
| } | ||
| if (lower.contains("not supported") || lower.contains("unsupported")) { |
There was a problem hiding this comment.
Minor: This branch matches not supported or unsupported anywhere in the message, so HugeGraph's own query limits come back as UnsupportedFeature with a hint that blames the translator.
Evidence: at this head, TraversalUtil.newUnsupportedPredicate() throws BackendException("Unsupported predicate: '%s'"), and LocalContainsStep raises CONTAINS query with relation '%s' is not supported. Both run in the HugeGraph traversal optimizer while a translated Cypher traversal executes, after the translator has accepted the query, so pointing the user at the Cypher compatibility guide sends them to the wrong place.
Requested change: limit UNSUPPORTED_FEATURE to failures raised by the translator (check the exception class or the specific translator messages), and let HugeGraph backend and optimizer messages fall through to EXECUTION_ERROR. A test that sends a query using an unsupported predicate would cover it.
HG-19 follow-up to #3241, within #3242. Keep the response envelope compatible with existing Java clients and classify actual Cypher translator failures correctly. Semantic parser errors previously fell through to execution or unsupported-feature errors, while backend unsupported predicates could be mistaken for translator limitations. This PR retains Nguyen Quang Minh's structured-error work and authorship from #3241. Its response metadata lives in status.attributes.errors, preserving the existing top-level fields, status and message. ExecutionError may also cover unrecognized translation failures and does not imply traversal execution started; original messages are preserved and classification does not cover every translator failure.
Purpose of the PR
Machine-readable error metadata for the Cypher API: stable error codes plus per-error hint strings, so clients can branch on failure modes instead of parsing message text.
Complements #238 (bound-query support, currently in hugegraph/hugegraph): that change adds validated parameter bindings; this change makes Cypher failures carry a structured
errorsarray. The two are independent — this branch applies cleanly on master (ed8344d19, single commit,Signed-off-bypresent).Main Changes
CypherErrorMapper(+67): maps Cypher op-processor failure classes to stable error codes and per-class hint strings.CypherModel(+27):errors[].code / message / hintpayload fields.CypherClient(+3 −2): surfaces mapped errors to the client.CypherApiTest(+18): structured-error assertions — syntax failure returns"errors"withSyntaxErrorand a "Declare the variable" hint; undefined schema element returnsExecutionErrorwith a "Create the schema element" hint.Verifying these changes
mvn -DskipTests test-compileon this exact branch: clean.CypherApiTest10/10 incl. error-code assertions, plus live REST verification 2026-09-26 — syntax error →errors:[{"code":"HugeGraph.Cypher.ExecutionError","message":"Invalid input …","hint":""}], valid query → 200 witherrors:[].Does this PR potentially affect the following parts?
/cypher)Documentation Status
Doc - TODO: the error-code → hint table will be documented in the paired website PR (docs: add Cypher language guide, compatibility matrix, and Neo4j migration guide (EN/CN) hugegraph-doc#499 or a follow-up) once the code set is agreed here.Doc - DoneDoc - No NeedNotes
CypherClient.java(this PR: 3 lines) among the files Avoid create redundant index label on property which is primary key #238 reworks.n24q02m/hugegraph@cypher-error-metadata.Conceptual overview
Concept illustration of proposed Cypher failure metadata with code, message, and hint; empty errors are omitted from successful responses.