fix: correct Bolt protocol version encoding, handshake negotiation and skip limit#3413
fix: correct Bolt protocol version encoding, handshake negotiation and skip limit#3413
Conversation
The v4.4 version constant was incorrectly encoded as 0x00000104 (v4.1) instead of 0x00000404. The version negotiation also didn't handle the range field used by modern Neo4j 5.x drivers, causing handshake failures.
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 critical issues in the Bolt protocol implementation, primarily focusing on accurate version encoding and robust handshake negotiation. By correcting the Bolt v4.4 version constant and enhancing the negotiation logic to support version ranges, it significantly improves compatibility with modern Neo4j drivers. These changes ensure reliable communication and prevent connection failures, streamlining interactions with the database. 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
|
🧪 CI InsightsHere's what we observed from your CI run for b81c364. 🟢 All jobs passed!But CI Insights is watching 👀 |
Code Review - PR #3413: Fix Bolt Protocol Version Encoding and Handshake NegotiationSummaryThis PR addresses a critical bug in Bolt protocol version negotiation where v4.4 was incorrectly encoded as Positive Aspects ✅1. Critical Bug Fix
2. Range Support Implementation
3. Improved Code Quality
4. Dependency Management
Issues and Concerns
|
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an issue with Bolt protocol version encoding and handshake negotiation. The v4.4 constant is now correctly encoded, and the negotiation logic properly handles version ranges from modern Neo4j drivers. The dependency for neo4j-driver has also been centralized and updated. My review includes one suggestion to improve code readability by encapsulating the version parsing logic into helper methods.
| final int clientMajor = clientVersion & 0xFF; | ||
| final int clientMinor = (clientVersion >> 8) & 0xFF; | ||
| final int clientRange = (clientVersion >> 16) & 0xFF; | ||
|
|
||
| for (final int supportedVersion : SUPPORTED_VERSIONS) { | ||
| // Check major version match (upper 16 bits for BOLT 4.x) | ||
| if (clientVersion == supportedVersion || | ||
| (clientVersion >> 8) == (supportedVersion >> 8)) { | ||
| final int serverMajor = supportedVersion & 0xFF; | ||
| final int serverMinor = (supportedVersion >> 8) & 0xFF; |
There was a problem hiding this comment.
To improve readability and encapsulate the logic for parsing Bolt protocol versions, consider extracting the bitwise operations into private helper methods. This makes the version negotiation logic easier to understand and maintain.
For example, you could add the following static helper methods to the BoltNetworkExecutor class:
private static int getMajorVersion(int version) {
return version & 0xFF;
}
private static int getMinorVersion(int version) {
return (version >> 8) & 0xFF;
}
private static int getVersionRange(int version) {
return (version >> 16) & 0xFF;
}Then, you can use these methods to simplify the code in the loop.
| final int clientMajor = clientVersion & 0xFF; | |
| final int clientMinor = (clientVersion >> 8) & 0xFF; | |
| final int clientRange = (clientVersion >> 16) & 0xFF; | |
| for (final int supportedVersion : SUPPORTED_VERSIONS) { | |
| // Check major version match (upper 16 bits for BOLT 4.x) | |
| if (clientVersion == supportedVersion || | |
| (clientVersion >> 8) == (supportedVersion >> 8)) { | |
| final int serverMajor = supportedVersion & 0xFF; | |
| final int serverMinor = (supportedVersion >> 8) & 0xFF; | |
| final int clientMajor = getMajorVersion(clientVersion); | |
| final int clientMinor = getMinorVersion(clientVersion); | |
| final int clientRange = getVersionRange(clientVersion); | |
| for (final int supportedVersion : SUPPORTED_VERSIONS) { | |
| final int serverMajor = getMajorVersion(supportedVersion); | |
| final int serverMinor = getMinorVersion(supportedVersion); |
Pull Request ReviewThis PR fixes a critical bug in the Bolt protocol version negotiation. The changes are well-implemented and thoroughly tested. Strengths1. Correct Bug Fix
2. Excellent Test CoverageThe new BoltVersionNegotiationTest.java is exemplary:
3. Proper Range Support ImplementationThe negotiation logic now correctly handles the range field, allowing clients to specify version ranges (e.g., "I support v4.2 through v4.4") 4. Good Code Hygiene
Security ConsiderationsNo security concerns. The changes:
Performance ConsiderationsExcellent performance characteristics:
Testing RecommendationsPlease verify integration tests pass, especially BoltProtocolIT which tests end-to-end handshakes with real Neo4j drivers. Final VerdictRecommendation: APPROVE with minor testing verification This is a high-quality fix that:
Before Merging:
Great work on this fix! The bug was subtle (incorrect hex encoding) but would have caused real-world connection failures. The comprehensive test suite ensures this won't regress. |
📜 License Compliance Check✅ License check passed. See artifacts for full report. License Summary (first 50 lines) |
Code Review: Bolt Protocol Version Encoding and Handshake NegotiationThank you for this fix! This PR addresses a critical bug in the Bolt protocol version negotiation. Here's my detailed review: ✅ Strengths
|
Pull Request ReviewThis PR fixes two critical issues in the Bolt protocol implementation and addresses a performance optimization bug in Cypher query execution. Overall, this is a well-structured, high-quality fix with excellent test coverage. ✅ Strengths1. Correct Bug Fixes
2. Excellent Test Coverage
3. Code Quality
4. Dependency Management
🔍 Code AnalysisBolt Version Negotiation LogicThe negotiation algorithm in if (clientMajor == serverMajor && serverMinor <= clientMinor && serverMinor >= clientMinor - clientRange)This logic ensures the server version falls within the client's supported range: Correctness verified by test case: Cypher Top-K Optimization FixThe fix in final Integer topKVal = limitVal != null ? limitVal + (skipVal != null ? skipVal : 0) : null;This ensures that after skipping N rows, there are still M rows remaining for the limit. 🐛 Potential Issues1. Minor: Code Duplication in CypherExecutionPlanThe top-K calculation logic is duplicated in 5 locations within
Recommendation: Extract this to a helper method to reduce duplication and improve maintainability: private Integer calculateTopK(final Expression skipExpr, final Expression limitExpr,
final Result context) {
final Integer limitVal = limitExpr != null ?
new ExpressionEvaluator(functionFactory).evaluateSkipLimit(limitExpr,
new ResultInternal(), context) : null;
final Integer skipVal = skipExpr != null ?
new ExpressionEvaluator(functionFactory).evaluateSkipLimit(skipExpr,
new ResultInternal(), context) : null;
return limitVal != null ? limitVal + (skipVal != null ? skipVal : 0) : null;
}However, this is a minor issue and can be addressed in a future refactoring PR. 2. Minor: Test Method VisibilityThe helper methods in 🔒 Security ConsiderationsNo security issues identified. The changes are protocol-level fixes that don't introduce:
⚡ Performance ConsiderationsPositive ImpactThe Cypher fix improves correctness without degrading performance. The top-K optimization still applies correctly - it just requests the right number of rows now. No Regression Concerns
📋 Test Coverage AssessmentExcellent coverage:
Suggested additional testing (can be future work):
📝 Documentation & CommentsWell-documented:
Minor suggestion: Consider adding a brief comment in // Top-K must account for SKIP so enough rows survive after skipping.
// Example: SKIP 5 LIMIT 5 needs to fetch at least 10 rows from the sorter.✅ Adherence to CLAUDE.md Guidelines
🎯 Final RecommendationAPPROVE with minor suggestions for future improvement. This PR:
The minor code duplication in Great work! 🎉 |
Pull Request Review: Fix Bolt Protocol Version Encoding and Handshake NegotiationSummaryThis 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. ✅ Strengths1. Excellent Bug FixThe core fix correctly addresses the version encoding issue:
2. Comprehensive Test CoverageThe new
3. Proper Range Field SupportThe 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
5. OpenCypher Fix is CriticalThe Top-K optimization fix in
|
Code Review: Bolt Protocol Version Encoding and Handshake FixThis PR fixes a critical bug in the Bolt protocol implementation and addresses a related OpenCypher query execution issue. Overall, the changes are well-implemented and ready to merge with some minor observations below. ✅ Strengths
🔍 Code Quality ObservationsBoltNetworkExecutor.java (bolt/src/main/java/com/arcadedb/bolt/BoltNetworkExecutor.java)Minor Suggestion: Consider adding explicit bounds checking in the helper methods: static int getMajorVersion(final int version) {
return version & 0xFF;
}
static int getMinorVersion(final int version) {
return (version >> 8) & 0xFF;
}
static int getVersionRange(final int version) {
return (version >> 16) & 0xFF;
}These are correct, but adding a comment about the valid range (0-255 for each component) would improve maintainability. Logging: The existing debug logging at line 205 is good, but consider adding a debug log when a version is successfully negotiated: if (protocolVersion != 0) {
LogManager.instance().log(this, Level.FINE,
"Negotiated Bolt protocol version %d.%d", getMajorVersion(protocolVersion), getMinorVersion(protocolVersion));
}CypherExecutionPlan.java (engine/src/main/java/com/arcadedb/query/opencypher/executor/CypherExecutionPlan.java)Performance Consideration: The Top-K optimization is correctly implemented, but be aware that for queries with large SKIP values and small LIMIT values, the OrderByStep will still materialize Code Duplication: The same
Consider extracting to a helper method: private Integer calculateTopKValue(final Expression skip, final Expression limit,
final CommandContext context, final FunctionFactory functionFactory) {
final Integer skipVal = skip != null ?
new ExpressionEvaluator(functionFactory).evaluateSkipLimit(skip, new ResultInternal(), context) : null;
final Integer limitVal = limit != null ?
new ExpressionEvaluator(functionFactory).evaluateSkipLimit(limit, new ResultInternal(), context) : null;
return limitVal != null ? limitVal + (skipVal != null ? skipVal : 0) : null;
}However, this is not a blocker for merging—the current implementation is clear and correct. 🔒 Security & Performance
🧪 TestingExcellent test coverage:
Recommendation: Verify that existing Bolt integration tests pass, particularly those using the Neo4j driver. 📋 POM ChangesIssue Found (pom.xml:212-214): <excludes>
<exclude>**/*IT.java</exclude>
</excludes>This excludes ALL integration tests from
This change appears unintentional and would break the testing workflow. The Action Required: Please remove this change or clarify if it's intentional. ✅ Adherence to CLAUDE.md Guidelines
🎯 RecommendationApprove with one fix required:
Once the POM change is addressed, this PR is ready to merge. 📝 Minor Enhancements (Optional)
Great work on the comprehensive testing and correct implementation! 🚀 |
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 preferencesFootnotes
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3413 +/- ##
============================================
+ Coverage 36.12% 41.81% +5.68%
============================================
Files 1254 1255 +1
Lines 84215 84541 +326
Branches 17260 17361 +101
============================================
+ Hits 30423 35351 +4928
+ Misses 48273 43312 -4961
- Partials 5519 5878 +359 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The v4.4 version constant was incorrectly encoded as 0x00000104 (v4.1)
instead of 0x00000404. The version negotiation also didn't handle the
range field used by modern Neo4j 5.x drivers, causing handshake failures.