#4210 fix: [cypher] remove dead quote-stripping in remaining executor steps

[robfrank]

Summary

Follow-up to #4237 (the original fix for issue #4210). The contributor's diff in the issue pointed out the same dead quote-stripping pattern in four other executor steps. This PR removes them so they cannot crash with Range [1, 0) out of bounds for length 1 when a property value is a single-quote character (') or any string that starts and ends with a quote.

Changes

• ExpandPathStep.matchesTargetProperties: drop quote-stripping; values from the parsed AST are already clean.
• MatchNodeStep.matchesProperties: drop quote-stripping; add length > 1 guard on the legacy $name parameter reference.
• MatchRelationshipStep.matchesTargetProperties and matchesEdgeProperties: same as above.
• FilterPropertiesStep: keeps the unquoting (its regex matches raw WHERE source text where quotes really are present) but adds a length >= 2 guard so a one-character ' literal no longer crashes.

Tests

Extended Issue4210MergeUnwindSingleQuoteTest:

• matchNodeWithSingleQuotePropertyViaUnwindDoesNotCrash exercises MatchNodeStep.matchesProperties via UNWIND $batch AS item MATCH (n:Tag {name: item.name}) with ', 'wrapped', and plain values.
• matchRelationshipEndpointWithSingleQuotePropertyViaUnwindDoesNotCrash exercises MatchRelationshipStep.matchesTargetProperties via an UNWIND-driven endpoint match.

Test plan

• mvn test -pl engine -Dtest=Issue4210MergeUnwindSingleQuoteTest (4/4 pass)
• mvn test -pl engine -Dtest="*Cypher*" (5939 pass, 0 failures, 91 skipped)
• mvn compile -pl engine (build success)

Closes #4210.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics -6 complexity

Metric	Results
Complexity	-6

View in Codacy

🟢 Coverage 57.14% diff coverage · -7.49% coverage variation

Metric	Results
Coverage variation	✅ -7.49% coverage variation
Diff coverage	✅ 57.14% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (d0e03eb)	127078	93666	73.71%
Head commit (63763c1)	158741 (+31663)	105122 (+11456)	66.22% (-7.49%)

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 (#4240)	7	4	57.14%

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.}

[gemini-code-assist]

Code Review

This pull request removes redundant quote-stripping logic from several OpenCypher executor steps, including ExpandPathStep, MatchNodeStep, and MatchRelationshipStep, to ensure that property values containing quotes are handled correctly. It also refines the quote-stripping implementation in FilterPropertiesStep and adds regression tests for UNWIND operations. Feedback suggests that ExpandPathStep is now inconsistent with other executor steps because it lacks numeric type-safe comparisons and parameter resolution logic.

[gemini-code-assist]

While removing the dead quote-stripping logic is correct, this method remains inconsistent with MatchNodeStep.matchesProperties and MatchRelationshipStep.matchesTargetProperties. It lacks numeric type-safe comparison (e.g., Integer vs Long), which will cause matches to fail for mixed numeric types. Additionally, it is missing parameter resolution (ParameterReference) and legacy "$name" support, which are present in other executor steps. Consider bringing this implementation in line with the other steps to ensure consistent behavior across different match types.

      Object expected = entry.getValue();
      if (expected instanceof String strValue && strValue.startsWith("$") && strValue.length() > 1) {
        final String paramName = strValue.substring(1);
        if (context.getInputParameters() != null) {
          final Object paramValue = context.getInputParameters().get(paramName);
          if (paramValue != null)
            expected = paramValue;
        }
      }
      if (actual == null || !actual.equals(expected)) {
        // Numeric type-safe comparison (Integer vs Long)
        if (actual instanceof Number && expected instanceof Number) {
          if (((Number) actual).longValue() != ((Number) expected).longValue())
            return false;
        } else
          return false;
      }

[robfrank]

Pushing back on the scope. The pre-fix matchesTargetProperties here had neither numeric coercion, nor ParameterReference resolution, nor the legacy "$name" branch - removing the dead quote-stripping does not regress anything that worked before. Adding those features to align with MatchNodeStep / MatchRelationshipStep is genuine new code, not a fix for #4210, and would need its own tests against VLP endpoints. I'd rather keep this PR scoped to the bug and open a separate change for the alignment if a real query surfaces the gap.

[claude]

Clean, well-scoped follow-up to #4237. The root cause is clear: AST-delivered property values are already unquoted, so re-stripping quotes corrupts any string that starts and ends with a quote character. The PR removes the dead stripping from four more executor steps, adds a defensive length > 1 guard on the legacy dollar-sign parameter-reference path, and adds a length >= 2 guard in FilterPropertiesStep where quote-stripping on raw WHERE source text is legitimately needed. Overall the fix is correct and the approach is consistent.

[claude]

Code Quality and Correctness

• ExpandPathStep, MatchNodeStep, MatchRelationshipStep: Removal of the quote-stripping blocks is correct. The invariant (AST delivers clean values) was established in #4210 fix: [cypher] MERGE+UNWIND crash on single-quote property value #4237; applying it consistently here is correct.
• MatchNodeStep / MatchRelationshipStep - bare-dollar guard: Adding strValue.length() > 1 before treating a string as a parameter reference is a good defensive fix. A bare dollar-sign is not a valid parameter name; the old code would silently look up an empty string in the input parameters map.
• FilterPropertiesStep: Handled correctly. Because the regex matches raw WHERE source text, the quotes really are present and must be stripped. The consolidated if-block is cleaner than the original if/else-if pair, and the length >= 2 guard fixes the single-character crash. Minor nit: aligning the OR operator under AND in the multi-line condition would improve readability.

Test Coverage

The two new tests are well-written and cover the main crash paths. Three gaps worth noting (non-blockers):

1. ExpandPathStep.matchesTargetProperties has no dedicated regression test. A variable-length expand pattern with UNWIND-supplied quote-containing values would close this.
2. FilterPropertiesStep has no test for the new length >= 2 guard - a WHERE clause with a single-character single-quote literal. Without it, a future refactor could silently re-introduce the crash.
3. The MatchRelationshipStep test validates matchesTargetProperties but not matchesEdgeProperties (also changed in this PR). A test filtering on edge properties with a quote-containing value would close that gap.

Performance: Positive - removing quote-stripping eliminates a String allocation per property comparison on the hot matching path.

Summary: The core fix is correct, consistent with #4237, and well-tested for the main crash paths. The only suggestions are to improve the indentation alignment in the FilterPropertiesStep multi-line condition and to add the three additional regression tests listed above.

[robfrank]

Addressed in 63763c1:

• Test gap 1 (ExpandPathStep) - added variableLengthPathEndpointWithSingleQuotePropertyDoesNotCrash, a [:LINK*1..3] VLP with a "'" endpoint property. Pre-fix this would have stripped to empty string and matched zero rows; post-fix it matches and round-trips the value.
• Test gap 3 (matchesEdgeProperties) - added matchRelationshipWithQuoteLiteralEdgePropertyDoesNotCrash filtering an edge by {note: "'"} inline.
• Alignment nit - indented the || sub-expression under its && in FilterPropertiesStep.

Skipping test gap 2 (FilterPropertiesStep length>=2 guard): the regex's value alternative is '[^']*'|"[^"]*"|\w+|\d+(?:\.\d+)?, which cannot match a bare ' (no closing quote available). matcher.find() returns false and the guarded code is never reached for that input. The length >= 2 guard is purely defensive against a future regex change; there is no current user-reachable input that exercises it. Happy to add a unit test that calls the parser directly if you'd like the guard pinned, but the integration-level test would be vacuous.

On the Gemini suggestion to broaden ExpandPathStep.matchesTargetProperties (numeric coercion + ParameterReference + $name) - pushed back in the inline thread. Those are pre-existing gaps, not regressions from this PR, and adding untested feature code paths during a bug fix violates scope.

[ExtReMLapin]

based

[codecov]

Codecov Report

❌ Patch coverage is 14.28571% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.66%. Comparing base (d0e03eb) to head (63763c1).

Files with missing lines	Patch %	Lines
...pencypher/executor/steps/FilterPropertiesStep.java	0.00%	3 Missing ⚠️
...encypher/executor/steps/MatchRelationshipStep.java	0.00%	0 Missing and 2 partials ⚠️
...query/opencypher/executor/steps/MatchNodeStep.java	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4240   +/-   ##
=======================================
  Coverage   64.66%   64.66%           
=======================================
  Files        1644     1644           
  Lines      127078   127063   -15     
  Branches    27216    27211    -5     
=======================================
- Hits        82173    82166    -7     
+ Misses      33440    33432    -8     
  Partials    11465    11465           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[robfrank]

based

do you think is it ready, now?

[ExtReMLapin]

Well I think it is, if all tests passes I guess