Skip to content

Conversation

p123-stack
Copy link
Collaborator

No description provided.

@p123-stack p123-stack marked this pull request as ready for review August 26, 2025 08:53
@transistive
Copy link
Collaborator

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

@p123-stack
Copy link
Collaborator Author

p123-stack commented Sep 29, 2025

Hello @transistive

Thank you! I’ve prepared a detailed overview of all the changes—both architectural and functionality

Authentication Refactor

  • Unified all authentication methods to use BoltConnection.
  • Removed dependency on protocol version classes.
  • Standardized request-response flow with send()->getResponse().

Connection & Message Factory Improvements

  • BoltMessageFactory now initialized with full BoltConnection instead of protocol references.
  • Added consistent state tracking for unconsumed results and logging.
  • Session::close() now calls discardUnconsumedResults() instead of consumeResults().
  • Robust error handling for RESET messages with proper cleanup.
  • New discardUnconsumedResults() helper method for explicit result discarding.
  • Connection metadata now updated with accurate server agent after authentication.

Changes in the BoltUnmanagedTransaction

  • Exception type: Changed from ClientExceptionTransactionException for clearer, domain-specific errors.
  • Error messages: Rephrased to be more concise and context-specific (e.g., "Can't commit a committed transaction.").
  • Commit handling: send()send()->getResponse() to properly wait for the server’s acknowledgment.
  • Rollback checks: Removed terminated case; only committed/rolled back states now block rollback.
  • Run checks: Added guard clauses to prevent running queries on finished transactions.

Session Refactor: Connection Tracking, Cleanup, and Centralized Retry Handling

  • New Session tracks used connections and adds a close() method to discard unconsumed results, preventing leaks.
  • Retry logic moved to TransactionHelper for cleaner, centralized handling.
  • BoltMessageFactory now takes the full connection instead of just the protocol for better flexibility.
  • Improves resource cleanup, maintainability, and robustness.

Introduce TransactionHelper for Centralized Retry and Error Handling

  • Extracted transaction retry logic into TransactionHelper.
  • Unified retry and rollback handling in one place.
  • Centralized transaction retry logic for consistency.

Unify System Update Handling in SummarizedResultFormatter

  • Some Neo4j versions return contains-system-updates, others return system-updates.
  • Using ($stats['contains-system-updates'] ?? $stats['system-updates'] ?? 0) ensures reliable capture.
  • Fallback ?? 0 guarantees numeric value to prevent missing key errors.
  • Improves summary accuracy across Neo4j versions.

Query ID (qid) in CypherList

  • Added qid to CypherList to uniquely identify the source query.
  • Improves traceability, debugging, and result management.

AbstractRunner Handler

  • Code changes: Return precise DriverErrorResponse for session/transaction errors, handle TransactionException separately, and simplify result key extraction.
  • Improves error reporting, type safety, and maintainability.
  • Testkit progress: Backend updated to support refined session and transaction handling.

RetryableNegative and RetryablePositive

  • RetryableNegative: Now returns FrontendErrorResponse instead of BackendErrorResponse for proper client reporting.

  • RetryablePositive: Updated to actually commit the transaction instead of just returning RetryableDoneResponse.

    • Retrieves transaction from repository, validates it, commits it.
    • Returns DriverErrorResponse if commit fails or BackendErrorResponse if missing.
  • Ensures correct retryable behavior and robust error handling.

Decode Transaction Metadata (CypherMap & CypherList)

  • SessionBeginTransaction: Added decodeToValue to convert CypherMap/List into PHP objects before passing metadata.
  • SessionReadTransaction: Same decoding logic applied for correctness and consistency.
  • SessionWriteTransaction: Metadata decoded before execution, ensuring structured data is handled properly.
  • Improves compatibility, prevents type mismatches, and ensures Neo4j interprets metadata correctly.

Enhance DriverErrorResponse

  • Updated to support both Neo4jException and TransactionException.
  • Ensures correct serialization of Neo4j-specific error codes/messages.
  • Differentiates between driver-level and transaction-level errors.

Set 24-Hour Timeout for Server Socket

  • Added stream_set_timeout($streamSocketServer, 60 * 60 * 24);.
  • Ensures socket stays open for long-running operations.
  • Improves stability for persistent connections and long-lived test sessions.

Copy link
Collaborator

@transistive transistive left a 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.


final class TransactionHelper
{
public const ROLLBACK_CLASSIFICATIONS = ['ClientError', 'TransientError', 'DatabaseError'];
Copy link
Collaborator

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?

Copy link
Collaborator Author

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,
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

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'];
Copy link
Collaborator

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

Copy link
Collaborator Author

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(...).

Copy link
Collaborator Author

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:

  1. 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)
  2. Cross-Version Compatibility:
  3. Neo4j 5.x: Supports native elementId() function
  4. Neo4j 4.x: Uses fallback to string representation of integer IDs
  5. This PHP client supports both versions (4.2 through 5.26)
  6. 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);
Copy link
Collaborator

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

Copy link
Collaborator Author

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
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

$config = $this->mergeTsxConfig($config);

return $this->retry($tsxHandler, true, $config);
return $this->retry($tsxHandler, false, $config);
Copy link
Collaborator

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

Copy link
Collaborator Author

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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this removed

* @return array<string, string|Position>
*/
public function toArray(): array
{
Copy link
Collaborator

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

Copy link
Collaborator Author

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,
Copy link
Collaborator

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

Copy link
Collaborator Author

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:

  1. makeFromBoltNode method: Added an else clause to handle v4 protocols by setting $elementId = (string) $node->id when the node is not a v5 structure.
  2. makeFromBoltRelationship method: Applied the same fix for relationships, setting $elementId = (string) $rel->id for v4 protocols.
  3. makeFromBoltUnboundRelationship method: Applied the same fix for unbound relationships for consistency.

$startNodeElementId = null;
$endNodeElementId = null;
if (method_exists($value, 'getStartNodeElementId')) {
$startNodeElementId = $value->getStartNodeElementId();
Copy link
Collaborator

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.

Copy link
Collaborator Author

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);
Copy link
Collaborator

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

Copy link
Collaborator Author

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
@transistive transistive merged commit c3f388f into main Oct 16, 2025
14 checks passed
@transistive transistive mentioned this pull request Oct 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants