Repository navigation
Conversation
…lures Signed-off-by: Nguyen Quang Minh <135627235+n24q02m@users.noreply.github.com>
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. |
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.