Copilot commented on code in PR #3241: URL: https://github.com/apache/hugegraph/pull/3241#discussion_r4111173641
########## hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/cypher/CypherErrorMapper.java: ########## @@ -0,0 +1,67 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hugegraph.api.cypher; + +/** + * Classify cypher execution failures into stable error codes and attach + * actionable hints for the most common, cryptic translator messages. + */ +public final class CypherErrorMapper { + + public static final String SYNTAX_ERROR = "HugeGraph.Cypher.SyntaxError"; + public static final String UNSUPPORTED_FEATURE = + "HugeGraph.Cypher.UnsupportedFeature"; + public static final String MISSING_PARAMETER = + "HugeGraph.Cypher.MissingParameter"; + public static final String EXECUTION_ERROR = "HugeGraph.Cypher.ExecutionError"; + + private CypherErrorMapper() { + } + + public static CypherModel.CypherError map(Throwable e) { + String message = e.getMessage() != null ? e.getMessage() : e.toString(); + // strip noisy exception class prefixes, e.g. "...driver.exception.ResponseException: " + message = message.replaceFirst( + "^([a-zA-Z0-9_]+\\.)+[A-Za-z0-9_]+(Exception|Error):\\s*", ""); + String lower = message.toLowerCase(); + + 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"); Review Comment: The hint tells clients to provide `?parameters=...`, but this API currently exposes only `cypher` as the GET query parameter or POST body, and `CypherClient.createRequest()` never forwards a parameter map. Following this hint cannot bind a `$city` value, so the new MissingParameter metadata is misleading; either add end-to-end parameter plumbing or change this mapping/hint to describe the actual supported behavior. ########## hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/cypher/CypherErrorMapper.java: ########## @@ -0,0 +1,67 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hugegraph.api.cypher; + +/** + * Classify cypher execution failures into stable error codes and attach + * actionable hints for the most common, cryptic translator messages. + */ +public final class CypherErrorMapper { + + public static final String SYNTAX_ERROR = "HugeGraph.Cypher.SyntaxError"; + public static final String UNSUPPORTED_FEATURE = + "HugeGraph.Cypher.UnsupportedFeature"; + public static final String MISSING_PARAMETER = + "HugeGraph.Cypher.MissingParameter"; + public static final String EXECUTION_ERROR = "HugeGraph.Cypher.ExecutionError"; + + private CypherErrorMapper() { + } + + public static CypherModel.CypherError map(Throwable e) { + String message = e.getMessage() != null ? e.getMessage() : e.toString(); + // strip noisy exception class prefixes, e.g. "...driver.exception.ResponseException: " + message = message.replaceFirst( + "^([a-zA-Z0-9_]+\\.)+[A-Za-z0-9_]+(Exception|Error):\\s*", ""); + String lower = message.toLowerCase(); + + 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"); Review Comment: The PR description says this feature complements #238 for bound-query support, but the linked `apache/hugegraph#238` is a closed issue about avoiding redundant index labels. Please correct the reference or link the intended bound-query change, since it currently makes the `MISSING_PARAMETER` contract and rebase context misleading. ########## hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/cypher/CypherModel.java: ########## @@ -37,6 +37,9 @@ public class CypherModel { @Schema(description = "The query result") public Result result = new Result(); + @Schema(description = "Structured errors, empty on success") + public List<CypherError> errors = Collections.emptyList(); Review Comment: This PR changes the public `/cypher` response contract and introduces stable error-code/hint semantics, but no in-repository documentation describes the new `errors` schema or the code-to-hint mapping. The repository guidance requires user-visible API behavior changes to ship with matching docs in the same PR, so the paired website PR or a follow-up alone does not document this contract for repository users. ########## hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/api/CypherApiTest.java: ########## @@ -72,6 +72,24 @@ public void testRelationQuery() { this.testCypherQueryAndContains(cypher, "friend"); } + @Test + public void testSyntaxErrorHasStructuredErrorWithHint() { + Response r = client().post(PATH, + "MATCH (n:person) RETURN not_defined_var"); + String content = assertResponseStatus(200, r); + assertContains("\"errors\"", content); + assertContains("SyntaxError", content); + assertContains("Declare the variable", content); Review Comment: These assertions only look for the substring `SyntaxError`, so they do not verify the stable machine-readable code promised by this change; a regression to an unrelated field or message containing that text would still pass. Assert the complete `HugeGraph.Cypher.SyntaxError` code and the `hint` field instead of partial words. This issue also appears on line 89 of the same file. -- 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]
