fix(engine) #5602: close the five OpenCypher follow-ups left by #5484 - #5612
Conversation
1. The argument-count guard could not reach the seven functions that go through SQLFunctionBridge - distance among them, where the #5484 bug actually was - and asserted it had compared zero of the 129 registered names, because no executor declared getMinArgs()/getMaxArgs(). Every executor now declares its contract and enforces it from that declaration (Function.checkArity), so there is one number per function instead of a hand-written bound beside a hand-written if; the bridge passes the wrapped SQL function's through. count and sum are pinned as deliberately narrower in Cypher than in SQL, and the pin is asserted to still bite. A wrong argument count is now a client error (400) from the runtime guards too, matching the parse-time gate. 2. Case folding no longer depends on the server's default locale. #5484 fixed function names; the same pattern survived in procedure names, variable names, IS :: type names, the EXPLAIN/PROFILE prefix scan, temporal units, vector metrics and the graph functions' direction argument. A test reads the sources so a new one cannot slip in - the two forms differ only under a locale CI never runs in. 3. charLength() and isNormalized() work, charAt() is gone. All three parsed and then failed at execution with "Unknown function". charLength is an alias of char_length, isNormalized is the boolean counterpart of normalize() sharing its form parsing, and charAt - which names no Neo4j function either - is unregistered. An unknown-function error now echoes the spelling written. 4. Parse-time argument validation walked RETURN and WITH only, so the same bad call was rejected before the query ran or not depending on its clause. One CypherExpressionWalker replaces the three partial per-clause recursions and the checks now reach WHERE, UNWIND, SET, CREATE, MERGE, DELETE, FOREACH, ORDER BY, SKIP/LIMIT and inline pattern properties. No check is new. 5. An arithmetic error is the caller's, not the server's. Integer overflow and division by zero (including duration(...)/0, which escaped as a raw JDK ArithmeticException) raise ArithmeticErrorException: HTTP 400 and Bolt Neo.ClientError.Statement.ArithmeticError, as Neo4j classifies them. It extends CommandExecutionException, so code written against #5164/#5494 is unaffected, and float arithmetic keeps IEEE 754 semantics.
|
Tick the box to add this pull request to the merge queue (same as
|
…-followups # Conflicts: # docs/release-26.8.1.md
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| CodeStyle | 1 minor |
🟢 Metrics 74 complexity
Metric Results Complexity 74
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
…pin at 500 Issue5221CypherDivByZeroHttpStatusIT pinned `1 / 0` to HTTP 500, on the reasoning that a zero divisor is data-dependent in the general case (n.a / n.b) and so a runtime error rather than a client error. That argument does not survive contact with the rest of the engine: abs(n.name) is data-dependent in exactly the same way and has answered 400 since #5484. What decides the status is whose mistake it is, and Neo4j classifies the whole arithmetic category - division by zero and 64-bit overflow alike - as Neo.ClientError.Statement.ArithmeticError. What #5221 was actually about is unchanged and still asserted: the label stays "Cannot execute command" rather than the misleading "Error on transaction commit", and the detail still names the real cause. ArithmeticErrorException extends CommandExecutionException, so the #5219 classification an embedded caller sees is intact.
Code Review: #5602 - close the five OpenCypher follow-ups left by #5484Reviewed the full diff (~130 files). This is a high-quality, unusually well-documented change: the Javadoc and the release note both explain why each decision was made, the wire-contract change (HTTP 500 -> 400) is called out explicitly rather than smuggled in, and the test coverage ( A few observations, none blocking: Design
Nit
Performance (informational, not a regression)
Correctness spot-checks (all look right)
Note: I could not run the Maven build in this environment (build commands needed approval), so I relied on static inspection plus the green results reported in the PR description. Static checks found no import/usage mismatches beyond the one unused import above. |
…of the Cypher helper Code-review point: the query-language-neutral com.arcadedb.function.Function interface reached into com.arcadedb.function.cypher.CypherFunctionHelper for the message its own checkArity raises - an inversion, since nothing about counting arguments is Cypher-specific. The three helpers move to com.arcadedb.function.FunctionArity, in the same package as Function, so the base interface needs no cross-package import at all. The five call sites follow; nothing is left behind in CypherFunctionHelper, so there is still one home for the wording. Also drops two imports left unused by the earlier commits (IsEmptyFunction, ArithmeticExpression); the one in Function is kept because its checkArity javadoc links CommandSemanticException.
Review: #5602 - OpenCypher follow-upsReviewed the full diff (~130 files). This is a high-quality, unusually well-documented PR: the Javadoc and release notes explain not just what changed but why, and the test coverage (arity registry sweep, Turkish-locale run, source-scan for new default-locale folding, end-to-end HTTP IT, Bolt classification) is genuinely strong. The core design idea - each executor declares its arity once and both the runtime guard ( A few notes, mostly for awareness rather than blockers. 1. Widening parse-time validation is a behavior change worth flagging as potentially breakingItem 4 moves argument/type validation from 2. Two co-existing argument-count mechanisms with different error classes
3. checkArity null-array folding - verified safe, worth a noteThe Javadoc folds the old hand-written Things I specifically verified as correct
Note: I could not run Nice work - the |
…rity() Code-review point: two argument-count mechanisms with different error classes co-existed. checkArity() raises CommandSemanticException (HTTP 400), while the older validateArgs() raised its own IllegalArgumentException with its own wording - and validateArgs() is what the CALL path invokes (CallStep.executeFunction). CallStep wraps what it catches, and the HTTP layer unwraps exactly one cause level, so `CALL fn(<wrong count>)` surfaced as 500 while the identical mistake in an expression gave 400. validateArgs() now delegates to checkArity(), so there is one check, one message and one status however the function was reached. Procedure declares a separate validateArgs() of its own; procedures are a different abstraction with their own registry, their own CALL handling and around eighty implementations, so that one is deliberately left alone and the reason is recorded on the method. Also documents that checkArity()'s null-array tolerance only rejects the null for a function requiring at least one argument - a getMinArgs()==0 executor is handed it unchanged and must still tolerate it - and flags the widened parse-time validation as potentially breaking in the release note.
Review: OpenCypher follow-ups (5602)Reviewed the full diff. This is a high-quality, unusually well-documented PR: the arity contract is declared once per function and held to the parser's view by a build-time test guard, the locale fix ships with a source-scanning regression test, the three partial per-clause recursions collapse into one A few points, mostly minor: 1. The 'embedded catch blocks unaffected' guarantee does not hold for the arity change. 2. 3. 4. WITH clauses can be walked twice. Test coverage. Strong. The Note: I could not compile/run the suite in this review environment (Maven blocked), so the above is a static review; the author reports the relevant modules green. Overall: LGTM with the above as optional follow-ups. Point 1 is the only one I would suggest addressing before merge, and even that only as a documentation clarification. |
…rces its own arity Three code-review points. normalize() and isNormalized() disagreed on their input domain. The pair shares its normal-form parsing, so the form names could not diverge, but normalize() called toString() on whatever arrived: normalize(123) quietly answered '123' where its new counterpart raised a type error, and where Neo4j - which declares both as f(input :: STRING) - raises one. It now rejects a non-STRING the same way, which is the treatment size() and head() got in #5477 and #5476. SQLFunctionBridge.execute() passed its arguments straight through without the runtime arity guard every other executor runs, so the seven bridged functions were the one exception to "each executor's guard reads its own declaration". It now calls checkArity() from the wrapped SQL function's bounds, which can only reject a call that function could not have served anyway. The release note now also states that the arity exception *type* changed, not only the HTTP status: CommandSemanticException extends CommandParsingException, a different branch from the CommandExecutionException the old runtime guards threw, so embedded code catching the latter for a bad argument count has to catch the former. That is the opposite of the arithmetic change, which stayed inside CommandExecutionException on purpose, and the note explains why the two categories legitimately differ.
Code review: #5602 OpenCypher follow-upsI read through the arity infrastructure ( What is strong here
One concrete concern worth verifyingThe
try {
function.validateArgs(args); // now throws CommandSemanticException (a CommandParsingException)
return function.execute(args, context);
} catch (final Exception e) {
...
throw new CommandExecutionException("Error executing function: " + function.getName(), e);
}A wrong-arity Contrast the arithmetic branch, which walks the whole chain and is immune to the same double-wrap. The arity change is the mirror image of the arithmetic change but doesn't get the same chain-walk treatment. Practical exposure is probably low - for functions known to
A targeted test - wrong-arity Minor
Note: I reviewed the merged tree statically and did not rebuild locally, so I'm relying on the PR's stated green runs for compilation/tests. Overall a high-quality PR; the one item I'd want closed before merge is confirming the |
…cution error Code-review point, and a real seam. CallStep wrapped everything it caught in a CommandExecutionException naming what was being called, which threw the classification away: `CALL f(<wrong count>)` became CommandExecutionException carrying the real CommandSemanticException as its cause. The HTTP layer unwraps one level, so a plain request still answered 400 - but on the auto-commit path the chain is TransactionException -> CommandExecutionException -> CommandSemanticException, one level too deep, and the same mistake answered 500. Fixed at the root rather than in the wire layer: a client error (CommandParsingException and its subclasses) is rethrown untouched, so every consumer benefits at once - HTTP, Bolt and embedded callers - instead of each learning to dig further down the chain. Both call sites shared identical catch arms, which collapse into one. Issue5602ArithmeticErrorHttpStatusIT covers it end to end: a wrong-arity CALL on a write statement, asserted at 400. Also makes the five zero-minimum temporal constructors treat a null argument array as empty. They dereferenced it before this branch too, so it is not a regression, but checkArity()'s documented contract now formally lets a null through for a getMinArgs()==0 function, so the contract and the code agree.
Review: #5602 OpenCypher follow-upsReviewed the full diff against What works well
One concrete gap: top-level
|
Code-review point. walkWith() walks a WITH's SKIP and LIMIT, but the top-level
ones hang off the statement rather than off a clause entry and were reached by
neither walk. So `RETURN n LIMIT abs('x')` was not rejected while
`WITH n LIMIT abs('x')` was - the clause-dependent asymmetry this widening
exists to remove, in the last place it survived.
The regression test asserts it through EXPLAIN, which parses and plans without
executing. The plain form proves nothing: SKIP/LIMIT are evaluated even when the
match yields no row, so the function's own runtime guard catches them either way,
and a test written that way passes with the fix reverted. Verified by reverting
it - under EXPLAIN the two top-level forms stop throwing while the WITH and WHERE
forms still do.
PR Review: Cypher follow-ups (#5602)Reviewed the full diff (139 files vs. base). This is a careful, well-documented PR. The Javadoc-as-rationale style makes the intent of each change auditable, and the "declare arity once, guard reads the declaration" refactor is the right fix for the class of drift that produced the original Correctness - looks solid
Minor / latent
TestsCoverage matches the change surface: full-coverage arity registry sweep (replacing the vacuous NoteI could not run the build in this environment (Maven invocation blocked), so the review is static; the green results in the description are consistent with what I read. Overall: high quality and well-reasoned. The only actionable item is the |
Code-review point, latent but a real trap. "No limit" has two spellings - the Cypher registry writes -1, Function.getMaxArgs() defaults to Integer.MAX_VALUE - and FunctionArity.describe() already accepted both while checkArity() compared raw against getMaxArgs(). A Function written to the registry's convention would therefore have rejected every call, since any count exceeds -1, while the message went on describing it as taking "at least N". Both now resolve the maximum through FunctionArity.effectiveMax(), so the check and the message cannot disagree about what unbounded means. No implementation returns -1 today; the test pins both spellings so none can start. Also notes on CypherExpressionWalker that its default arm treats an unrecognised type as a leaf, so an expression type added later without a case here hides whatever it nests from every check that runs through the walker.
Code ReviewReviewed the five follow-ups end to end (arity contract + registry guard, locale folding, missing functions, walker reach, arithmetic classification). This is high-quality work: the design is coherent, the Javadoc explains the why rather than the what, and the test coverage (build-time registry sweep, Turkish-locale run, source scan for new folding sites, 400 end-to-end IT including the auto-commit re-wrap path, Bolt classification incl. wrapped-cause and conflict-wins) is unusually good. The single-declaration-per-function approach (executor declares arity, A few observations, all minor: 1. 2. 3. Wire-contract change is correctly called out. Runtime arity guards now throw Nits:
Nothing here is blocking. Could not run the full Maven build in this environment (sandbox), so I am trusting the reported green suites; the changes I read are internally consistent and should compile. Reviewed with Claude Code. |
… and pattern comprehensions are walked Two code-review points. OPTIONAL CALL returned null on any exception, so a wrong argument count or a bad argument type inside it answered an empty result instead of the 400 the same call gives outside OPTIONAL - hiding the mistake behind something that looks like a legitimately empty match. OPTIONAL suppresses cardinality, not malformed calls, which is also how Neo4j reads it, so a client error now propagates from both the function and the procedure arm. That makes the rethrow helper added a commit ago redundant: the dedicated catch arm precedes the generic one, so the wrap collapses back to a plain CommandExecutionException. CypherExpressionWalker treated PatternComprehensionExpression as a leaf, so a function call in its WHERE or its projection escaped parse-time validation - the exact hazard the class Javadoc warns about. It had the parts but no accessors; they are added and the walker descends into them.
Review: #5602 - close the five OpenCypher follow-upsI reviewed the new core ( No blocking issues found. A few observations: Strengths
Minor observations (non-blocking)
VerificationRelied on the PR's stated green runs plus the registry test's build-time full-coverage assertion, which is the real guard against a narrower-than-real declaration slipping in. The Nice work. |
Code-review nit, raised twice. Deciding how to report a failure means looking past the outermost throwable - it arrives wrapped differently depending on whether the request came in directly, through the auto-commit TransactionException wrapper, or carrying the JDK exception it came from - and each wire layer had grown its own depth-capped loop to do it: two added by this branch plus the pre-existing isRetryableConflict. CauseChain.find/contains in com.arcadedb.exception replaces all three. Engine is the natural home: bolt depends on server only in provided scope, so neither can host something the other needs. The depth cap now lives in one place, with the reason it exists (initCause can build a self-referential chain) written down once instead of three times. Also records in the release note that the arithmetic classification is HTTP and Bolt only - the other wire protocols still report it through their generic execution-error handling.
Review: #5602 OpenCypher follow-upsI read through the whole diff (the 5 items, the new engine classes, the parser/validator rework, the Bolt/HTTP wiring, and the tests). Overall this is a high-quality, unusually well-documented change: the "one declaration per function, one message per mistake" direction is the right one, and the test story (build-time registry sweep, Turkish-locale runs, a source-scan guard, an end-to-end HTTP IT, and the Bolt classification cases) is genuinely strong. The Javadoc explaining why each decision was made is excellent. A few observations, mostly minor. Correctness / design (looks solid)
Minor: wire-contract / breaking-change surfaceIntentional and documented in
Minor: micro-nit on the HTTP handlerIn Minor: redundant double check on the CALL path
Suggestion: guard the
|
…walked or declared a leaf Code-review suggestion, and the best one of the round. CypherExpressionWalker's default arm is where a regression hides without a sound: a composite expression type added later without a case falls into it and everything it nests escapes every check routed through the walker. The class Javadoc says "add a case here", but a comment is not a guard - PatternComprehensionExpression sat in that arm until the previous commit, so a function call in its WHERE or projection was never validated. The test enumerates every Expression / BooleanExpression implementation from the AST sources and requires each to be either handled by the walker or named in an explicit leaf list. It also asserts the leaf list holds no stale entry, so a type that grows children later cannot be excused by a whitelist nobody revisited. Verified it bites: deleting the PatternComprehensionExpression case fails it by name. Same move as the source scan that guards the locale fix. Also resolves the arithmetic error once per catch block in the HTTP handler rather than once in the condition and again to report it.
|
Review of #5602 - close the five OpenCypher follow-ups left by #5484 I read through the core mechanisms (Function.checkArity/FunctionArity, CauseChain, SQLFunctionBridge, CypherExpressionWalker, FunctionValidator, ArithmeticExpression, the Bolt/HTTP classification paths and the new tests) rather than skimming the 129 mechanical per-function edits. This is a genuinely high-quality change: each of the five items is scoped, the rationale is captured in the code where the next maintainer will actually see it, and every claim in the description is backed by a test. What works well
Minor points (non-blocking)
Adherence to project conventions Consistent with CLAUDE.md: functions reuse the shared helpers rather than re-implementing checks, final is applied on new params/locals, tests use the assertThat(...) style, no new dependencies, and the changes are TDD-backed with regression tests across engine/server/bolt plus an end-to-end HTTP IT. No System.out debug left behind. Nice work - the follow-ups are closed cleanly and the guards that failed to guard in #5484 now actually compare something. Reviewed by Claude (automated PR review). |
Not up to standards ⛔🟢 Coverage
|
| Metric | Results |
|---|---|
| Coverage variation | ✅ -6.83% coverage variation |
| Diff coverage | ✅ 96.57% diff coverage |
Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (0f7c013) 142188 108027 75.97% Head commit (02922bc) 181556 (+39368) 125528 (+17501) 69.14% (-6.83%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>
Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#5612) 641 619 96.57% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5612 +/- ##
==========================================
+ Coverage 67.03% 67.18% +0.15%
==========================================
Files 1720 1771 +51
Lines 142188 149564 +7376
Branches 30536 31717 +1181
==========================================
+ Hits 95313 100486 +5173
- Misses 34199 35950 +1751
- Partials 12676 13128 +452 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Review: #5602 OpenCypher follow-upsI read through the full diff (arity contract, arithmetic classification, locale folding, the new A few observations, mostly for downstream awareness rather than blockers: 1. Exception-type change is the most impactful thing here (documented, but worth emphasizing)Arity errors move from 2.
|
…#5612) * fix(engine) #5602: close the five OpenCypher follow-ups left by #5484 1. The argument-count guard could not reach the seven functions that go through SQLFunctionBridge - distance among them, where the #5484 bug actually was - and asserted it had compared zero of the 129 registered names, because no executor declared getMinArgs()/getMaxArgs(). Every executor now declares its contract and enforces it from that declaration (Function.checkArity), so there is one number per function instead of a hand-written bound beside a hand-written if; the bridge passes the wrapped SQL function's through. count and sum are pinned as deliberately narrower in Cypher than in SQL, and the pin is asserted to still bite. A wrong argument count is now a client error (400) from the runtime guards too, matching the parse-time gate. 2. Case folding no longer depends on the server's default locale. #5484 fixed function names; the same pattern survived in procedure names, variable names, IS :: type names, the EXPLAIN/PROFILE prefix scan, temporal units, vector metrics and the graph functions' direction argument. A test reads the sources so a new one cannot slip in - the two forms differ only under a locale CI never runs in. 3. charLength() and isNormalized() work, charAt() is gone. All three parsed and then failed at execution with "Unknown function". charLength is an alias of char_length, isNormalized is the boolean counterpart of normalize() sharing its form parsing, and charAt - which names no Neo4j function either - is unregistered. An unknown-function error now echoes the spelling written. 4. Parse-time argument validation walked RETURN and WITH only, so the same bad call was rejected before the query ran or not depending on its clause. One CypherExpressionWalker replaces the three partial per-clause recursions and the checks now reach WHERE, UNWIND, SET, CREATE, MERGE, DELETE, FOREACH, ORDER BY, SKIP/LIMIT and inline pattern properties. No check is new. 5. An arithmetic error is the caller's, not the server's. Integer overflow and division by zero (including duration(...)/0, which escaped as a raw JDK ArithmeticException) raise ArithmeticErrorException: HTTP 400 and Bolt Neo.ClientError.Statement.ArithmeticError, as Neo4j classifies them. It extends CommandExecutionException, so code written against #5164/#5494 is unaffected, and float arithmetic keeps IEEE 754 semantics. * fix(server) #5602: division by zero reports 400, superseding the #5221 pin at 500 Issue5221CypherDivByZeroHttpStatusIT pinned `1 / 0` to HTTP 500, on the reasoning that a zero divisor is data-dependent in the general case (n.a / n.b) and so a runtime error rather than a client error. That argument does not survive contact with the rest of the engine: abs(n.name) is data-dependent in exactly the same way and has answered 400 since #5484. What decides the status is whose mistake it is, and Neo4j classifies the whole arithmetic category - division by zero and 64-bit overflow alike - as Neo.ClientError.Statement.ArithmeticError. What #5221 was actually about is unchanged and still asserted: the label stays "Cannot execute command" rather than the misleading "Error on transaction commit", and the detail still names the real cause. ArithmeticErrorException extends CommandExecutionException, so the #5219 classification an embedded caller sees is intact. * refactor(engine) #5602: the arity wording moves beside Function, out of the Cypher helper Code-review point: the query-language-neutral com.arcadedb.function.Function interface reached into com.arcadedb.function.cypher.CypherFunctionHelper for the message its own checkArity raises - an inversion, since nothing about counting arguments is Cypher-specific. The three helpers move to com.arcadedb.function.FunctionArity, in the same package as Function, so the base interface needs no cross-package import at all. The five call sites follow; nothing is left behind in CypherFunctionHelper, so there is still one home for the wording. Also drops two imports left unused by the earlier commits (IsEmptyFunction, ArithmeticExpression); the one in Function is kept because its checkArity javadoc links CommandSemanticException. * fix(engine) #5602: validateArgs() runs the same arity check as checkArity() Code-review point: two argument-count mechanisms with different error classes co-existed. checkArity() raises CommandSemanticException (HTTP 400), while the older validateArgs() raised its own IllegalArgumentException with its own wording - and validateArgs() is what the CALL path invokes (CallStep.executeFunction). CallStep wraps what it catches, and the HTTP layer unwraps exactly one cause level, so `CALL fn(<wrong count>)` surfaced as 500 while the identical mistake in an expression gave 400. validateArgs() now delegates to checkArity(), so there is one check, one message and one status however the function was reached. Procedure declares a separate validateArgs() of its own; procedures are a different abstraction with their own registry, their own CALL handling and around eighty implementations, so that one is deliberately left alone and the reason is recorded on the method. Also documents that checkArity()'s null-array tolerance only rejects the null for a function requiring at least one argument - a getMinArgs()==0 executor is handed it unchanged and must still tolerate it - and flags the widened parse-time validation as potentially breaking in the release note. * fix(engine) #5602: normalize() is STRING-only and the SQL bridge enforces its own arity Three code-review points. normalize() and isNormalized() disagreed on their input domain. The pair shares its normal-form parsing, so the form names could not diverge, but normalize() called toString() on whatever arrived: normalize(123) quietly answered '123' where its new counterpart raised a type error, and where Neo4j - which declares both as f(input :: STRING) - raises one. It now rejects a non-STRING the same way, which is the treatment size() and head() got in #5477 and #5476. SQLFunctionBridge.execute() passed its arguments straight through without the runtime arity guard every other executor runs, so the seven bridged functions were the one exception to "each executor's guard reads its own declaration". It now calls checkArity() from the wrapped SQL function's bounds, which can only reject a call that function could not have served anyway. The release note now also states that the arity exception *type* changed, not only the HTTP status: CommandSemanticException extends CommandParsingException, a different branch from the CommandExecutionException the old runtime guards threw, so embedded code catching the latter for a bad argument count has to catch the former. That is the opposite of the arithmetic change, which stayed inside CommandExecutionException on purpose, and the note explains why the two categories legitimately differ. * fix(engine) #5602: CALL no longer flattens a client error into an execution error Code-review point, and a real seam. CallStep wrapped everything it caught in a CommandExecutionException naming what was being called, which threw the classification away: `CALL f(<wrong count>)` became CommandExecutionException carrying the real CommandSemanticException as its cause. The HTTP layer unwraps one level, so a plain request still answered 400 - but on the auto-commit path the chain is TransactionException -> CommandExecutionException -> CommandSemanticException, one level too deep, and the same mistake answered 500. Fixed at the root rather than in the wire layer: a client error (CommandParsingException and its subclasses) is rethrown untouched, so every consumer benefits at once - HTTP, Bolt and embedded callers - instead of each learning to dig further down the chain. Both call sites shared identical catch arms, which collapse into one. Issue5602ArithmeticErrorHttpStatusIT covers it end to end: a wrong-arity CALL on a write statement, asserted at 400. Also makes the five zero-minimum temporal constructors treat a null argument array as empty. They dereferenced it before this branch too, so it is not a regression, but checkArity()'s documented contract now formally lets a null through for a getMinArgs()==0 function, so the contract and the code agree. * fix(engine) #5602: the top-level SKIP/LIMIT are walked at parse time too Code-review point. walkWith() walks a WITH's SKIP and LIMIT, but the top-level ones hang off the statement rather than off a clause entry and were reached by neither walk. So `RETURN n LIMIT abs('x')` was not rejected while `WITH n LIMIT abs('x')` was - the clause-dependent asymmetry this widening exists to remove, in the last place it survived. The regression test asserts it through EXPLAIN, which parses and plans without executing. The plain form proves nothing: SKIP/LIMIT are evaluated even when the match yields no row, so the function's own runtime guard catches them either way, and a test written that way passes with the fix reverted. Verified by reverting it - under EXPLAIN the two top-level forms stop throwing while the WITH and WHERE forms still do. * fix(engine) #5602: checkArity() honours the -1 spelling of "unbounded" Code-review point, latent but a real trap. "No limit" has two spellings - the Cypher registry writes -1, Function.getMaxArgs() defaults to Integer.MAX_VALUE - and FunctionArity.describe() already accepted both while checkArity() compared raw against getMaxArgs(). A Function written to the registry's convention would therefore have rejected every call, since any count exceeds -1, while the message went on describing it as taking "at least N". Both now resolve the maximum through FunctionArity.effectiveMax(), so the check and the message cannot disagree about what unbounded means. No implementation returns -1 today; the test pins both spellings so none can start. Also notes on CypherExpressionWalker that its default arm treats an unrecognised type as a leaf, so an expression type added later without a case here hides whatever it nests from every check that runs through the walker. * fix(engine) #5602: OPTIONAL CALL no longer swallows a malformed call, and pattern comprehensions are walked Two code-review points. OPTIONAL CALL returned null on any exception, so a wrong argument count or a bad argument type inside it answered an empty result instead of the 400 the same call gives outside OPTIONAL - hiding the mistake behind something that looks like a legitimately empty match. OPTIONAL suppresses cardinality, not malformed calls, which is also how Neo4j reads it, so a client error now propagates from both the function and the procedure arm. That makes the rethrow helper added a commit ago redundant: the dedicated catch arm precedes the generic one, so the wrap collapses back to a plain CommandExecutionException. CypherExpressionWalker treated PatternComprehensionExpression as a leaf, so a function call in its WHERE or its projection escaped parse-time validation - the exact hazard the class Javadoc warns about. It had the parts but no accessors; they are added and the walker descends into them. * refactor(engine) #5602: one cause-chain walk instead of three copies Code-review nit, raised twice. Deciding how to report a failure means looking past the outermost throwable - it arrives wrapped differently depending on whether the request came in directly, through the auto-commit TransactionException wrapper, or carrying the JDK exception it came from - and each wire layer had grown its own depth-capped loop to do it: two added by this branch plus the pre-existing isRetryableConflict. CauseChain.find/contains in com.arcadedb.exception replaces all three. Engine is the natural home: bolt depends on server only in provided scope, so neither can host something the other needs. The depth cap now lives in one place, with the reason it exists (initCause can build a self-referential chain) written down once instead of three times. Also records in the release note that the arithmetic classification is HTTP and Bolt only - the other wire protocols still report it through their generic execution-error handling. * test(engine) #5602: the build enforces that every expression type is walked or declared a leaf Code-review suggestion, and the best one of the round. CypherExpressionWalker's default arm is where a regression hides without a sound: a composite expression type added later without a case falls into it and everything it nests escapes every check routed through the walker. The class Javadoc says "add a case here", but a comment is not a guard - PatternComprehensionExpression sat in that arm until the previous commit, so a function call in its WHERE or projection was never validated. The test enumerates every Expression / BooleanExpression implementation from the AST sources and requires each to be either handled by the walker or named in an explicit leaf list. It also asserts the leaf list holds no stale entry, so a type that grows children later cannot be excused by a whitelist nobody revisited. Verified it bites: deleting the PatternComprehensionExpression case fails it by name. Same move as the source scan that guards the locale fix. Also resolves the arithmetic error once per catch block in the HTTP handler rather than once in the condition and again to report it. (cherry picked from commit f9d2576)
1 participant
Closes #5602.
The five follow-ups recorded while fixing #5484, done together because items 1 and 3 touch the same registry.
1. The arity guard now compares something
FunctionValidator'sminArgs/maxArgsbecame a hard parse-time gate in #5484, which is how a declaration narrower than the real signature started rejecting valid queries. The drift guard added alongside it asserted it had compared zero of the 129 registered names: no executor overrodegetMinArgs()/getMaxArgs(), and the seven that reach a SQL function throughSQLFunctionBridge(count,distance,stdev,stdev_pop,stdev_samp,stdevp,sum) had nothing to delegate to.distance— the entry that was actually wrong — was in that group.The issue proposed giving
SQLFunctionan argument-count contract. It already inherits one fromFunction; what was missing is that nothing used it, and adding bounds next to each executor's hand-writtenif (args.length != N)would just be a second hand-written copy free to drift from the first. So instead each executor declares its arity once and its runtime guard reads that declaration (Function.checkArity). The bridge passes the wrapped SQL function's bounds through. What the guard compares is therefore the parser's view against the code's, not two copies of the same number.All 129 names are checked at build time. Two are pinned as deliberately narrower in Cypher than in SQL —
count(whose star form is a separate parser construct) andsum(SQL's is variadic per row) — and each pin is asserted to still bite, so it cannot quietly become a hole.2. Locale-dependent case folding
#5484 fixed function names, where a Turkish default made
"ISNAN".toLowerCase()the dotless"ısnan". The issue listed seven remaining sites; there were more of the same defect — theEXPLAIN/PROFILEprefix scan,IS ::type names, temporal unit names, vector metric names, the graph functions'directionargument. All fold withLocale.ROOT, plus a test that reads the sources: the two forms behave identically under every locale CI runs in, so nothing else would notice a new one.3. Registered-but-unimplemented functions
charLength→ alias of the already-implementedchar_length.isNormalized(input[, normalForm])→ implemented as the boolean counterpart ofnormalize(), sharing its form parsing so the two cannot accept different form names.charAt→ unregistered; it names no function in Neo4j either, so it is now rejected up front with the ordinary unknown-function error instead of parsing and then failing at execution.KNOWN_WITHOUT_EXECUTORis empty and stays pinned. An unknown-function error also echoes the spelling written rather than the folded one.4. Parse-time validation beyond RETURN/WITH
MATCH (n:Nothing) WHERE abs('x') > 0 RETURN nran to completion — and, matching no row, looked like a success — while the identical call in aRETURNwas rejected before the query started. OneCypherExpressionWalkerreplaces the three partial per-clause recursions, so the checks reachWHERE,UNWIND,SET,CREATE,MERGE,DELETE,FOREACH,ORDER BY,SKIP/LIMITand inline pattern properties. No check is new — only its reach. A call that does execute still fails with the same message from the function's own guard.5. Arithmetic errors
abs(-9223372036854775808),9223372036854775807 + 1and1 / 0have no representable answer, decided by the caller's values and not by anything wrong with the server; all answered HTTP 500. Per the design call on the issue, the whole category moves together:ArithmeticErrorExceptionfor 64-bit overflow and for division/modulo by zero — includingduration(...) / 0, which escaped as a rawjava.lang.ArithmeticExceptionand reached the wire as an unrecognised throwable. HTTP answers 400, Bolt answersNeo.ClientError.Statement.ArithmeticError.CommandExecutionException, the class 64-bit integer arithmetic overflow silently wraps around instead of failing #5164 and abs(Long.MIN_VALUE) silently returns a negative value instead of throwing an overflow error #5494 settled on, so embedded catch blocks are unaffected.1.0 / 0.0is stillInfinity,0.0 / 0.0stillNaN.Tests
CypherFollowUpsIssue5602Test— 20 tests across items 2-5, including a Turkish-locale run and a source scan for new default-locale folding.CypherFunctionArityRegistryTest— theisZero()pin replaced by full-coverage assertions;isNormalized,normalizeandcoalesceadded to the per-arity sweep.Issue5602ArithmeticErrorHttpStatusIT— 400 end to end, including the write path where the auto-commit wrapper re-wraps the failure.BoltErrorClassificationTest— three cases for the new status code, including wrapped-as-cause and conflict-wins-over.TextStatelessFunctionsTest— arity assertions updated to the client-error class.Green: engine
query.**+function.**11273/0, server 777/0, bolt 280/0, the new IT 4/0.🤖 Generated with Claude Code
https://claude.ai/code/session_01YBYJ4njo4cYiU9VqTTAr8o