fix: preserve field names for single-element Cypher RETURN over Bolt …#3286
fix: preserve field names for single-element Cypher RETURN over Bolt …#3286
Conversation
…protocol When FinalProjectionStep unwraps single-element results (e.g., RETURN n), it stores the vertex directly as an element but loses the field name "n". This caused Bolt protocol to send vertex properties as field names instead of the RETURN clause field name. Changes: - FinalProjectionStep: Store projection name in metadata when unwrapping - BoltNetworkExecutor: Use metadata to get correct field names for elements - BoltNetworkExecutor: Extract element values correctly for Bolt RECORD This fixes 10+ failing Bolt integration tests where nodes/edges were returned as NULL because field names didn't match. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @robfrank, 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 addresses a critical issue in how single-element Cypher 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. 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
|
🧪 CI InsightsHere's what we observed from your CI run for ebce707. 🟢 All jobs passed!But CI Insights is watching 👀 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses an issue where field names were lost for single-element Cypher RETURN statements over the Bolt protocol. The approach of storing the projection name in the result metadata in FinalProjectionStep and then consuming it in BoltNetworkExecutor is a solid solution. The code changes are clear, and the addition of a new test case in CypherResultFormatTest is great for ensuring this fix is covered. I've made a few suggestions to replace the hardcoded metadata key "_projectionName" with the newly introduced FinalProjectionStep.PROJECTION_NAME_METADATA constant to improve maintainability.
| // This happens for queries like "MATCH (n) RETURN n" where the vertex is | ||
| // returned directly but we need to preserve the field name "n" for Bolt protocol | ||
| if (firstResult.isElement()) { | ||
| final Object projectionName = firstResult.getMetadata("_projectionName"); |
There was a problem hiding this comment.
To improve maintainability and avoid magic strings, it's recommended to use the FinalProjectionStep.PROJECTION_NAME_METADATA constant. This ensures that if the metadata key is ever updated, the change is reflected everywhere it's used. You can use the fully qualified name as suggested or add an import for FinalProjectionStep.
| final Object projectionName = firstResult.getMetadata("_projectionName"); | |
| final Object projectionName = firstResult.getMetadata(com.arcadedb.query.opencypher.executor.steps.FinalProjectionStep.PROJECTION_NAME_METADATA); |
|
|
||
| // Check if this is an unwrapped element result | ||
| // (single vertex/edge returned directly from RETURN clause) | ||
| if (result.isElement() && result.getMetadata("_projectionName") != null) { |
There was a problem hiding this comment.
Similar to the previous comment, using the FinalProjectionStep.PROJECTION_NAME_METADATA constant here instead of a hardcoded string will make the code more robust and easier to maintain.
| if (result.isElement() && result.getMetadata("_projectionName") != null) { | |
| if (result.isElement() && result.getMetadata(com.arcadedb.query.opencypher.executor.steps.FinalProjectionStep.PROJECTION_NAME_METADATA) != null) { |
| assertThat(results).hasSize(2); | ||
| for (final Result r : results) { | ||
| assertThat(r.isElement()).isTrue(); | ||
| assertThat(r.getMetadata("_projectionName")).isEqualTo("n"); |
There was a problem hiding this comment.
For consistency and to prevent potential issues if the key changes, it's best to use the FinalProjectionStep.PROJECTION_NAME_METADATA constant in tests as well. You can add an import for com.arcadedb.query.opencypher.executor.steps.FinalProjectionStep to shorten the line.
| assertThat(r.getMetadata("_projectionName")).isEqualTo("n"); | |
| assertThat(r.getMetadata(com.arcadedb.query.opencypher.executor.steps.FinalProjectionStep.PROJECTION_NAME_METADATA)).isEqualTo("n"); |
Code Review - PR #3286Thank you for this fix! This addresses an important issue with field name preservation in Bolt protocol for single-element Cypher RETURN statements. Overall, the implementation is solid and well-documented. Here's my detailed feedback: ✅ Strengths
🔍 Issues & Suggestions1. Magic String Duplication (Minor Issue)The metadata key Recommendation: Import and use the constant to avoid potential bugs from typos: // In BoltNetworkExecutor.java imports:
import com.arcadedb.query.opencypher.executor.steps.FinalProjectionStep;
// Line 806:
final Object projectionName = firstResult.getMetadata(FinalProjectionStep.PROJECTION_NAME_METADATA);
// Line 831:
if (result.isElement() && result.getMetadata(FinalProjectionStep.PROJECTION_NAME_METADATA) != null) {This follows DRY principles and makes refactoring safer. The constant is public, so this is the intended usage pattern. 2. Edge Case Consideration (Question)What happens when a query returns multiple elements with the same projection name? For example: MATCH (n) RETURN n LIMIT 100Looking at the code, 3. Test Coverage Gap (Suggestion)The test
This would provide regression protection at the protocol level where the original bug manifested. 4. Potential Null Handling (Minor)In values.add(BoltStructureMapper.toPackStreamValue(result.getElement().orElse(null)));The 📊 Code Quality Assessment
🎯 RecommendationAPPROVE with minor suggestions. The core fix is correct and addresses the stated problem. The magic string duplication is the only real issue, and it's minor. The suggested improvements (constant usage, additional integration test) would make this even more robust but aren't blockers. 📝 Checklist from CLAUDE.md✅ Problem clearly stated Great work on the fix! The 10+ failing Bolt integration tests should now pass. 🚀 Review performed by Claude Code (Sonnet 4.5) following ArcadeDB project guidelines from CLAUDE.md |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Use column aliases in OpenCypher RETURN clause (AS name, AS height) and access results by column name instead of numeric index. This aligns with the Bolt protocol test patterns and the current OpenCypher result format. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Code Review - PR #3286SummaryThis PR fixes a critical issue where single-element Cypher RETURN queries (e.g., Positive Aspects ✓
Issues & Recommendations1. Magic String - Use Constant Reference (Medium Priority)// BoltNetworkExecutor.java:806, 831
final Object projectionName = firstResult.getMetadata("_projectionName");Issue: Hard-coded string Risk: Typos won't be caught at compile time, violates DRY principle. Fix: import com.arcadedb.query.opencypher.executor.steps.FinalProjectionStep;
// Then use:
final Object projectionName = firstResult.getMetadata(FinalProjectionStep.PROJECTION_NAME_METADATA);
if (result.isElement() && result.getMetadata(FinalProjectionStep.PROJECTION_NAME_METADATA) != null)2. Missing Integration Test (High Priority)According to the PR description, this "fixes 10+ failing Bolt integration tests." Issue: While Recommendation: Add a test in
Example skeleton: @Test
void singleElementReturnPreservesFieldName() {
// Execute: MATCH (n:Person) RETURN n
// Assert: result field name is "n", not vertex property names
// Assert: value is a BoltNode with Person label
}3. Edge Case: Multi-Element Returns (Low Priority)Question: Does The code only handles Recommendation: Add a test case for multi-element returns to ensure no regression. 4. Null Safety (Low Priority)// BoltNetworkExecutor.java:833
values.add(BoltStructureMapper.toPackStreamValue(result.getElement().orElse(null)));Question: Can If not, consider using 5. Code Style - Curly Braces (Minor)Per CLAUDE.md guidelines:
// BoltNetworkExecutor.java:807-809 - could be simplified
if (projectionName instanceof String name) {
return List.of(name);
}This is fine as-is, but the existing codebase may prefer: if (projectionName instanceof String name)
return List.of(name);Check consistency with the rest of Performance Considerations ✓
No performance concerns identified. Security Considerations ✓
Test Coverage Summary
Compliance with CLAUDE.md
Final RecommendationApprove with minor changes:
The core solution is sound and well-architected. Once the constant reference is fixed and integration test is added, this will be production-ready. Generated by Claude Code review agent |
Code Review: Fix Bolt Protocol Field Names for Cypher RETURNSummaryThis PR addresses a critical issue where the Bolt protocol was losing field names when returning single elements (vertices/edges) from Cypher queries like Positive Aspects ✅
Issues & Concerns 🔍1. Test Coverage - Missing Bolt Integration Test Verification (HIGH PRIORITY)The PR description claims it "fixes 10+ failing Bolt integration tests," but I don't see evidence of:
Recommendation:
@Test
void returnNodePreservesFieldName() {
try (Driver driver = getDriver()) {
try (Session session = driver.session(SessionConfig.forDatabase(getDatabaseName()))) {
session.run("CREATE (p:Person {name: 'Alice'})");
final Result result = session.run("MATCH (n:Person) RETURN n");
final Record record = result.next();
assertThat(record.keys()).containsExactly("n");
assertThat(record.get("n").asNode()).isNotNull();
}
}
}2. Potential Edge Case - Multiple Single-Element Projections (MEDIUM)What happens with queries like Recommendation: Add a test case to @Test
void returnMultipleSingleNodesIsProjection() {
// MATCH (a), (b) RETURN a, b should return projection with both nodes
final ResultSet result = database.query("opencypher",
"MATCH (a:Person), (b:Person) WHERE a.name = 'Alice' AND b.name = 'Bob' RETURN a, b");
final Result r = result.next();
assertThat(r.isProjection()).isTrue();
assertThat(r.getPropertyNames()).containsExactlyInAnyOrder("a", "b");
}3. PostgreSQL Wire Protocol Test Changes (LOW)The changes in
These changes are good practices, but they seem to be working around a different issue rather than being part of this fix. The query was changed from Question: Was this PostgreSQL test actually affected by the Bolt fix, or was it failing for a different reason? If the latter, it might warrant a separate commit/PR. 4. Import Organization (LOW - Style)BoltNetworkExecutor.java lines 20-47 show explicit import expansion. While this is valid Java, it differs from the wildcard import style used elsewhere in the codebase. This appears to be an IDE auto-formatting artifact. Recommendation: Check if this matches project import conventions. The CLAUDE.md doesn't specify import style, but consistency is preferable. 5. Memory Overhead (LOW - Performance)Adding metadata to every single-element result adds a small memory overhead ( Analysis: Given that:
This is acceptable, but worth noting for future optimization if needed. Security Considerations 🔒No security issues identified. The metadata key is a compile-time constant, not user-controlled, preventing injection risks. Performance Considerations ⚡
Code Quality Suggestions 📝Minor: Metadata Constant VisibilityThe constant
Minor: Null SafetyBoltNetworkExecutor:854 calls Testing Checklist (per CLAUDE.md) ✓Based on project guidelines, please verify:
Recommendations SummaryBefore Merging:
Nice to Have: VerdictThe core fix is solid and well-implemented. The metadata approach is clean and the changes are focused. However, the PR would benefit from stronger integration test verification before merging, particularly demonstrating that the claimed "10+ failing Bolt tests" now pass. Recommendation: Approve with minor revisions - primarily adding test verification and addressing the test coverage gaps noted above. Great work on isolating and fixing this tricky protocol issue! 🎯 |
(cherry picked from commit 20bdabf)
1 participant
…protocol
When FinalProjectionStep unwraps single-element results (e.g., RETURN n), it stores the vertex directly as an element but loses the field name "n". This caused Bolt protocol to send vertex properties as field names instead of the RETURN clause field name.
Changes:
This fixes 10+ failing Bolt integration tests where nodes/edges were returned as NULL because field names didn't match.