Fix backtick stripping in Cypher map literal keys#3322
Fix backtick stripping in Cypher map literal keys#3322robfrank merged 1 commit intoArcadeData:mainfrom
Conversation
Summary of ChangesHello @ExtReMLapin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue in the Cypher parser where map literal keys enclosed in backticks were not being properly unquoted. This fix ensures that keys like Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue with backticked keys in Cypher map literals by introducing and applying the stripBackticks utility method. The change to make stripBackticks static is a good design choice for reusability, and the new tests provide solid coverage for the fixed scenarios. However, the fix appears to be partial, as similar logic is needed in other parts of the parser to ensure consistent handling of backticked keys across all map-like constructs. I've added a specific comment detailing the missed locations.
engine/src/main/java/com/arcadedb/query/opencypher/parser/CypherExpressionBuilder.java
Show resolved
Hide resolved
🧪 CI InsightsHere's what we observed from your CI run for 42b9260. 🟢 All jobs passed!But CI Insights is watching 👀 |
|
Claude wrote all of this. Fixes #3321 |
5e4d1d3 to
1c64267
Compare
Fixed an issue where backticks in map literal keys and property names
were being included instead of being properly stripped. Applied the fix
consistently across all map-related and property access parsing contexts.
Changes:
- Made CypherASTBuilder.stripBackticks() static and package-private
to allow access from CypherExpressionBuilder
- Updated CypherExpressionBuilder.parseMapLiteralExpression() to strip
backticks from map keys (RETURN clause map literals)
- Updated CypherExpressionBuilder.parseMapProperties() to strip
backticks from map keys (pattern properties)
- Updated CypherASTBuilder.visitMap() to strip backticks from map keys
(CREATE/MERGE clause map literals)
- Updated CypherExpressionBuilder.parseMapProjection() to strip
backticks from both explicit keys and property names (map projections)
- Updated CypherExpressionBuilder.parseExpression2WithPostfix() to strip
backticks from property names in property access expressions
- Updated CypherExpressionBuilder.parseExpressionText() to strip
backticks from property names in text-based parsing
- Added comprehensive test cases in CypherMapBackticksTest to verify:
- Single backticked key in RETURN (e.g., `@rid`)
- Multiple backticked keys in RETURN
- Escaped backticks within keys (e.g., `key``with``backticks`)
- Backticked keys in CREATE clause
- Backticked keys in map projections
- Property access with backticked names (e.g., n.`@id`)
Example queries that now work correctly:
- RETURN: collect({`@rid`: ID(c), text: c.text})
- CREATE: CREATE (n {`@special`: 'value'})
- Map projection: n{.name, `@id`: n.id}
- Property access: RETURN n.`@special`
Before: keys/properties were stored/accessed as "`@rid`" (with backticks)
After: keys/properties are stored/accessed as "@Rid" (without backticks)
https://claude.ai/code/session_01ATpxvdUh9HNtuBW7x9NmLT
1c64267 to
42b9260
Compare
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3322 +/- ##
==========================================
- Coverage 56.61% 56.58% -0.03%
==========================================
Files 1368 1368
Lines 100618 100618
Branches 20486 20486
==========================================
- Hits 56961 56934 -27
- Misses 34538 34545 +7
- Partials 9119 9139 +20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…3322) Fixed an issue where backticks in map literal keys and property names were being included instead of being properly stripped. Applied the fix consistently across all map-related and property access parsing contexts. Changes: - Made CypherASTBuilder.stripBackticks() static and package-private to allow access from CypherExpressionBuilder - Updated CypherExpressionBuilder.parseMapLiteralExpression() to strip backticks from map keys (RETURN clause map literals) - Updated CypherExpressionBuilder.parseMapProperties() to strip backticks from map keys (pattern properties) - Updated CypherASTBuilder.visitMap() to strip backticks from map keys (CREATE/MERGE clause map literals) - Updated CypherExpressionBuilder.parseMapProjection() to strip backticks from both explicit keys and property names (map projections) - Updated CypherExpressionBuilder.parseExpression2WithPostfix() to strip backticks from property names in property access expressions - Updated CypherExpressionBuilder.parseExpressionText() to strip backticks from property names in text-based parsing - Added comprehensive test cases in CypherMapBackticksTest to verify: - Single backticked key in RETURN (e.g., `@rid`) - Multiple backticked keys in RETURN - Escaped backticks within keys (e.g., `key``with``backticks`) - Backticked keys in CREATE clause - Backticked keys in map projections - Property access with backticked names (e.g., n.`@id`) Before: keys/properties were stored/accessed as "`@rid`" (with backticks) After: keys/properties are stored/accessed as "@Rid" (without backticks) (cherry picked from commit ada9a89)
What does this PR do?
This PR fixes an issue where backticks in map literal keys were not being properly stripped during Cypher query parsing. The
stripBackticksmethod inCypherASTBuilderis now made static and called when parsing map literals inCypherExpressionBuilder, ensuring that backticked keys (e.g.,`@rid`) are correctly converted to their unquoted form (e.g.,@rid).Motivation
When using map literals in Cypher queries with backticked keys (commonly needed for special characters like
@), the backticks were being retained in the resulting map keys. This caused queries like:to produce maps with keys like
`@rid`instead of@rid, breaking downstream code that expected the unquoted key names.Related issues
This fix addresses the handling of backticked identifiers in map literals, which is part of the Cypher specification for escaping reserved words and special characters.
Additional Notes
stripBackticksmethod is nowstaticto allow reuse across different builder classesCypherExpressionBuildernow call this method:parseMapLiteralExpressionandparseMapProperties→` ``)Checklist
mvn clean packagecommandhttps://claude.ai/code/session_01ATpxvdUh9HNtuBW7x9NmLT