-
Notifications
You must be signed in to change notification settings - Fork 41
Feat/tesktkit session run #268
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Hello @pratikshazalte69, This PR is pretty big! Good work. Can you provide me a short overview of all the changes? Architecturally as well as functionally. Thank you |
Hello @transistive Thank you! I’ve prepared a detailed overview of all the changes—both architectural and functionalityAuthentication Refactor
Connection & Message Factory Improvements
Changes in the BoltUnmanagedTransaction
Session Refactor: Connection Tracking, Cleanup, and Centralized Retry Handling
Introduce TransactionHelper for Centralized Retry and Error Handling
Unify System Update Handling in SummarizedResultFormatter
Query ID (qid) in CypherList
AbstractRunner Handler
RetryableNegative and RetryablePositive
Decode Transaction Metadata (CypherMap & CypherList)
Enhance DriverErrorResponse
Set 24-Hour Timeout for Server Socket
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @pratikshazalte69 ,
We are almost there. It is a big PR so we'll have to go back and forth a bit. I've put my concerns into comments.
src/Common/TransactionHelper.php
Outdated
|
||
final class TransactionHelper | ||
{ | ||
public const ROLLBACK_CLASSIFICATIONS = ['ClientError', 'TransientError', 'DatabaseError']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pratikshazalte69 , I thought we removed this class? I think we reintroduced it after merging. Can you double check this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're correct. This class should be removed. Only TransactionHelper remains, and all other authentication flows have been updated to use BoltConnection instead of the old V3/V4/V5 classes. I’ll remove this class to clean it up.
private readonly int $endNodeId, | ||
string $type, | ||
CypherMap $properties, | ||
?string $elementId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the startNodeElementId and endNodeElementId supposed to be gone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the startNodeElementId and endNodeElementId properties are intentionally not present in the core Relationship class - only traditional integer IDs (startNodeId/endNodeId) are currently supported. These element ID properties are only implemented in the testkit backend for testing compatibility, with a mapping workaround used to provide them when needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should verify this with official drivers. I'm pretty sure Neo4j version 5 provides elementID, but version 4 doesn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right! We've now properly implemented startNodeElementId and endNodeElementId in the core Relationship class with full backward compatibility support.
In Relationship.php :
- Added startNodeElementId and endNodeElementId as optional nullable properties
- Added getter methods for both properties
- Included them in the toArray() method
In BoltOGMTranslator.php ):
- Implemented smart fallback logic: if the Bolt protocol provides element IDs (Neo4j 5.x), we use them; otherwise, we fall back to the string-converted integer IDs for backward compatibility with Neo4j 4.x
This implementation now properly handles both Neo4j version
|
||
private function decodeToValue(array $param) | ||
{ | ||
$value = $param['data']['value']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Careful, I see a lot of duplicated code here for the decodeToValue
. You may want to move this to a seperate class or function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing that out! I already refactored the code to remove duplication:
decodeToValue is now a public static method in AbstractRunner.
All other handlers, including SessionReadTransaction, now call it via AbstractRunner::decodeToValue(...).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you are absolutely right .
These properties should be present in the core Relationship class
Evidence supporting this decision:
- Testkit Requirements: The official Neo4j testkit explicitly tests for these properties (lines 203-204 in datatypes tests and lines 115-117 in basic_query tests)
- Cross-Version Compatibility:
- Neo4j 5.x: Supports native elementId() function
- Neo4j 4.x: Uses fallback to string representation of integer IDs
- This PHP client supports both versions (4.2 through 5.26)
- Official Driver Compliance: All official Neo4j drivers are expected to provide these properties for compatibility testing
Changes Made:
Updated src/Types/Relationship.php:
- Added optional startNodeElementId and endNodeElementId constructor parameters
- Added getter methods for both properties
- Updated toArray() method to include the new properties
- Updated type annotations
Updated src/Formatter/Specialised/BoltOGMTranslator.php:
- Enhanced makeFromBoltRelationship() to handle element IDs
- For Neo4j 5+: Uses actual element IDs if available
- For Neo4j 4.x: Falls back to string representation of integer IDs
- Maintains backward compatibility
'endNodeElementId' => $endNodeElementId, | ||
]; | ||
|
||
error_log('DEBUG PATH: Stored mapping for key: '.$relationshipKey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep using the PSR logging instead of using error_logs for debug entries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood! Since we don’t need debug logging at the moment, I’ll remove these error_log calls entirely.
CYPHER; | ||
} | ||
|
||
public function testFormatBoltStatsWithFalseSystemUpdates(): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this get removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! I’ve restored the removed tests and updated them to align with the recent architectural changes, so they are now fixed and passing.
src/Bolt/Session.php
Outdated
$config = $this->mergeTsxConfig($config); | ||
|
||
return $this->retry($tsxHandler, true, $config); | ||
return $this->retry($tsxHandler, false, $config); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has to remain true. It is a read transaction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it !!!. I have changed it true.
* | ||
* @psalm-mutation-free | ||
*/ | ||
public function getServerVersion(): string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this removed
src/Databags/Notification.php
Outdated
* @return array<string, string|Position> | ||
*/ | ||
public function toArray(): array | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep the toArray implementation only, we don't need a toSerializedArray. You probably just have to call toArray
on the Position instance to comply with psalm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've simplified the implementation by removing toSerializedArray() and keeping only the toArray() method.
$rel->endNodeId, | ||
$rel->type, | ||
new CypherMap($map), | ||
$elementId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still has to be resolved. V5 will have start and endnode element ids, whereas v4 won't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've successfully resolved the issue mentioned in the comment. The problem was that in Bolt protocol v4, the element_id property doesn't exist on Node and Relationship structures, but it was introduced in v5. The comment indicated that for v4 protocols, the element_id should fallback to the string representation of the node/relationship ID.
Changes Made:
- makeFromBoltNode method: Added an else clause to handle v4 protocols by setting $elementId = (string) $node->id when the node is not a v5 structure.
- makeFromBoltRelationship method: Applied the same fix for relationships, setting $elementId = (string) $rel->id for v4 protocols.
- makeFromBoltUnboundRelationship method: Applied the same fix for unbound relationships for consistency.
$startNodeElementId = null; | ||
$endNodeElementId = null; | ||
if (method_exists($value, 'getStartNodeElementId')) { | ||
$startNodeElementId = $value->getStartNodeElementId(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous situation did appear to properly test against start and endnode element ids provided by the driver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the testkit backend now properly tests element IDs from the driver. It first checks if getStartNodeElementId() and getEndNodeElementId() methods exist on the relationship , directly retrieving element IDs provided by the driver. This ensures we're properly validating the driver's element ID implementation for both Neo4j 4 and 5 compatibility.
|
||
public function testAuthenticateBoltFailureV5(): void | ||
{ | ||
$this->expectException(Neo4jException::class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests still need to be restored
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have restored the tests and fixed them too
…op ?? null) for element_id, startNodeElementId, and endNodeElementId properties.Restored the tests and fixed them
…ection for better developer experience
No description provided.