fix: correct Bolt protocol version encoding, handshake negotiation and skip limit by robfrank · Pull Request #3413 · ArcadeData/arcadedb (original) (raw)

Pull Request Review: Fix Bolt Protocol Version Encoding and Handshake Negotiation

Summary

This PR fixes a critical bug in the Bolt protocol implementation where v4.4 was incorrectly encoded as 0x00000104 (v4.1) instead of 0x00000404, and adds proper support for the range field used by modern Neo4j 5.x drivers. The PR also includes an important fix for OpenCypher's ORDER BY + SKIP + LIMIT optimization.

✅ Strengths

1. Excellent Bug Fix

The core fix correctly addresses the version encoding issue:

• Before: 0x00000104 incorrectly represented v4.1
• After: 0x00000404 correctly represents v4.4
• The encoding format [unused(8)][range(8)][minor(8)][major(8)] is now properly documented

2. Comprehensive Test Coverage

The new BoltVersionNegotiationTest.java is exemplary:

• 15+ test cases covering edge cases (exact matches, range negotiation, padding, fallback scenarios)
• Tests real-world scenarios (Neo4j 5.x and 4.x driver handshakes)
• Clear, well-documented test methods
• Follows the project's preferred assertion style: assertThat().isEqualTo()

3. Proper Range Field Support

The negotiation logic now correctly implements Bolt's range field:

if (clientMajor == serverMajor && serverMinor <= clientMinor && serverMinor >= clientMinor - clientRange)

This ensures compatibility with modern Neo4j 5.x drivers.

4. Code Quality

• Clean separation of concerns (helper methods for version extraction)
• Good inline comments explaining the negotiation logic
• Consistent with project coding standards
• Methods appropriately scoped as package-private static for testing

5. OpenCypher Fix is Critical

The Top-K optimization fix in CypherExecutionPlan.java solves a correctness issue:

• Before: OrderByStep received only LIMIT value, causing incorrect results when SKIP > 0
• After: OrderByStep receives SKIP + LIMIT to retain enough rows before skipping
• New test orderBySkipLimitWithManyNodes() validates the fix

⚠️ Issues & Concerns

1. Critical: Maven Surefire Configuration Change

Location: pom.xml:212-214

Issue: This excludes all integration tests from the default mvn test run. According to CLAUDE.md:

Run all the connected tests could be affected by the change to ensure nothing is broken

Why this is problematic:

• The existing skipITs property already controls integration tests
• This hardcoded exclusion bypasses that mechanism
• Developers may unknowingly skip integration tests
• CI pipelines may need adjustment

Recommendation: Remove this change unless there's a compelling reason (if so, document it in the commit message).

2. Potential Performance Impact

Location: CypherExecutionPlan.java (multiple locations)

The Top-K optimization now uses topKVal = limitVal + skipVal, which means:

• More rows retained in memory during sorting
• For ORDER BY ... SKIP 1000000 LIMIT 10, the OrderByStep will keep 1,000,010 rows instead of 10

Recommendation: Consider adding a reasonable upper bound or documenting this trade-off. For very large SKIP values, this could cause memory pressure.

3. Missing Edge Case: Integer Overflow

Location: CypherExecutionPlan.java:615

final Integer topKVal = limitVal != null ? limitVal + (skipVal != null ? skipVal : 0) : null;

Issue: If limitVal + skipVal exceeds Integer.MAX_VALUE, this will silently overflow.

Recommendation:

final Integer topKVal = limitVal != null ? Math.addExact(limitVal, (skipVal != null ? skipVal : 0)) : null;

Or add explicit bounds checking with a meaningful error message.

4. Neo4j Driver Version Centralization

Location: bolt/pom.xml, e2e/pom.xml, pom.xml

Good: Moved neo4j-driver.version to the parent POM for centralization.

Concern: The PR description doesn't mention this change. Was this intentional or accidental? Also, the version was bumped from 5.27.0 to 5.28.10 without explanation.

Recommendation: Mention in the PR description if the version bump is related to the fix (e.g., testing with newer drivers).

5. Minor: Test Isolation Concern

Location: BoltProtocolIT.java:597-605

The transactionIsolation() test was rewritten to use a single session. While simpler, the original test verified isolation between sessions, which is a stronger guarantee.

Original intent: Session 1 creates uncommitted data → Session 2 shouldn't see it
New behavior: Single session rollback → same session shouldn't see rolled-back data

Recommendation: Consider keeping both tests (rename the new one to transactionRollback) to maintain the cross-session isolation test coverage.

🔍 Security & Safety

✅ No Security Concerns

• Input validation: Version negotiation safely handles malformed client proposals (zero padding, out-of-range values)
• No SQL injection or unsafe deserialization risks introduced
• No new dependencies added

📊 Test Coverage Assessment

Bolt Protocol Changes

• ✅ Unit tests for version encoding helpers
• ✅ Comprehensive negotiation scenarios
• ✅ Integration test validates real Neo4j driver compatibility
• ✅ Edge cases covered (padding, range matching, fallback)

OpenCypher Changes

• ✅ New test orderBySkipLimitWithManyNodes() validates the fix
• ⚠️ Could benefit from testing larger SKIP values to validate performance characteristics

📝 Documentation & Code Style

✅ Follows Project Guidelines

• Adheres to existing code style
• Single-statement if blocks without braces (per CLAUDE.md)
• Uses final keyword appropriately
• No fully qualified names (imports used correctly)
• Preferred assertion style: assertThat().isEqualTo()

⚠️ Minor Documentation Gap

The helper methods (getMajorVersion, getMinorVersion, getVersionRange) would benefit from Javadoc comments explaining the bit layout, especially since this is a protocol-specific encoding.

🎯 Recommendations

Must Fix

1. Remove or justify the <excludes>/*IT.java</excludes> change in pom.xml

Should Fix

1. Add overflow protection for topKVal = limitVal + skipVal
2. Document the Neo4j driver version bump rationale
3. Add Javadoc to version helper methods

Nice to Have

1. Add a test for very large SKIP values to validate memory behavior
2. Consider keeping the original cross-session isolation test
3. Add a comment about the memory trade-off in the Top-K optimization

✅ Final Verdict

Approval Status: Approve with minor changes requested

This is a high-quality PR that fixes a real bug affecting Neo4j 5.x driver compatibility. The test coverage is excellent, and the code follows project conventions. The OpenCypher fix is also important for correctness.

The main concern is the Maven Surefire configuration change, which should be removed or explained. The integer overflow risk should also be addressed.

Great work on the comprehensive testing and clear problem diagnosis! 🎉