Skip to content

Commit 4d3e4d3

Browse files
p123-stackp123-stack
committed
Fix transaction state management and bookmark chaining and fixed bookmarks tests
1 parent bf9de59 commit 4d3e4d3

18 files changed

+524
-89
lines changed

‎BOOKMARK_FIXES_SUMMARY.md‎

Lines changed: 214 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,214 @@
1+
# Bookmark Tests Fixes Summary
2+
3+
## Changes Made to Fix Bookmark Tests
4+
5+
### 1. **BoltConnection.php** - Fixed Transaction State Management
6+
7+
#### a) Fixed `isStreaming()` method to handle null protocol
8+
**Location:** Lines 163-174
9+
```php
10+
public function isStreaming(): bool
11+
{
12+
if (!isset($this->boltProtocol)) {
13+
return false;
14+
}
15+
16+
return in_array(
17+
$this->protocol()->serverState,
18+
[ServerState::STREAMING, ServerState::TX_STREAMING],
19+
true
20+
);
21+
}
22+
```
23+
**Why:** Prevents crashes when checking streaming state on a connection that might not be fully initialized.
24+
25+
#### b) Improved `begin()` method
26+
**Location:** Lines 228-247
27+
```php
28+
public function begin(?string $database, ?float $timeout, BookmarkHolder $holder, ?iterable $txMetaData): void
29+
{
30+
$this->logger?->log(LogLevel::DEBUG, 'BEGIN transaction');
31+
32+
// Only consume results if we're actually streaming
33+
if ($this->isStreaming()) {
34+
$this->logger?->log(LogLevel::DEBUG, 'Consuming results before BEGIN');
35+
$this->consumeResults();
36+
}
37+
38+
$extra = $this->buildRunExtra($database, $timeout, $holder, $this->getAccessMode(), $txMetaData, true);
39+
$this->logger?->log(LogLevel::DEBUG, 'BEGIN with extra', $extra);
40+
41+
$message = $this->messageFactory->createBeginMessage($extra);
42+
$response = $message->send()->getResponse();
43+
$this->assertNoFailure($response);
44+
$this->inTransaction = true;
45+
46+
$this->logger?->log(LogLevel::DEBUG, 'BEGIN successful');
47+
}
48+
```
49+
**Key Changes:**
50+
- Only consume results if connection is actually streaming
51+
- Still passes `true` as the 6th parameter to `buildRunExtra()` to ensure access mode ('w' or 'r') is sent with BEGIN
52+
- Sets `$this->inTransaction = true`
53+
54+
#### c) Fixed `rollback()` method
55+
**Location:** Lines 297-307
56+
```php
57+
public function rollback(): void
58+
{
59+
if ($this->isStreaming()) {
60+
$this->consumeResults();
61+
}
62+
63+
$message = $this->messageFactory->createRollbackMessage();
64+
$response = $message->send()->getResponse();
65+
$this->assertNoFailure($response);
66+
$this->inTransaction = false;
67+
}
68+
```
69+
**Why:** Now properly resets the transaction flag after rollback.
70+
71+
#### d) Added `setTransactionFinished()` method
72+
**Location:** Lines 502-505
73+
```php
74+
public function setTransactionFinished(): void
75+
{
76+
$this->inTransaction = false;
77+
}
78+
```
79+
**Why:** Allows commit message to reset transaction state.
80+
81+
### 2. **BoltCommitMessage.php** - Fixed Transaction State After Commit
82+
83+
**Location:** Lines 43-67
84+
```php
85+
public function getResponse(): Response
86+
{
87+
$response = parent::getResponse();
88+
89+
$this->connection->protocol()->serverState = ServerState::READY;
90+
91+
/** @var array{bookmark?: string} $content */
92+
$content = $response->content;
93+
$bookmark = $content['bookmark'] ?? '';
94+
95+
if (trim($bookmark) !== '') {
96+
$this->logger?->log(LogLevel::DEBUG, 'Setting bookmark after commit', ['bookmark' => $bookmark]);
97+
$this->bookmarks->setBookmark(new Bookmark([$bookmark]));
98+
}
99+
100+
$this->connection->protocol()->serverState = ServerState::READY;
101+
$this->connection->setTransactionFinished(); // NEW
102+
103+
return $response;
104+
}
105+
```
106+
**Key Change:**
107+
- Now calls `$this->connection->setTransactionFinished()` after successful commit
108+
- Added debug logging for bookmark updates
109+
110+
### 3. **testkit.sh** - Enabled All Bookmark Tests
111+
112+
**Location:** Lines 139-150
113+
```bash
114+
#TestBookmarksV5
115+
python3 -m unittest tests.stub.bookmarks.test_bookmarks_v5.TestBookmarksV5.test_bookmarks_can_be_set || EXIT_CODE=1
116+
python3 -m unittest tests.stub.bookmarks.test_bookmarks_v5.TestBookmarksV5.test_last_bookmark || EXIT_CODE=1
117+
python3 -m unittest tests.stub.bookmarks.test_bookmarks_v5.TestBookmarksV5.test_send_and_receive_bookmarks_write_tx || EXIT_CODE=1
118+
python3 -m unittest tests.stub.bookmarks.test_bookmarks_v5.TestBookmarksV5.test_sequence_of_writing_and_reading_tx || EXIT_CODE=1
119+
python3 -m unittest tests.stub.bookmarks.test_bookmarks_v5.TestBookmarksV5.test_send_and_receive_multiple_bookmarks_write_tx || EXIT_CODE=1
120+
121+
#TestBookmarksV4
122+
python3 -m unittest tests.stub.bookmarks.test_bookmarks_v4.TestBookmarksV4.test_bookmarks_on_unused_sessions_are_returned || EXIT_CODE=1
123+
python3 -m unittest tests.stub.bookmarks.test_bookmarks_v4.TestBookmarksV4.test_bookmarks_session_run || EXIT_CODE=1
124+
python3 -m unittest tests.stub.bookmarks.test_bookmarks_v4.TestBookmarksV4.test_sequence_of_writing_and_reading_tx || EXIT_CODE=1
125+
python3 -m unittest tests.stub.bookmarks.test_bookmarks_v4.TestBookmarksV4.test_bookmarks_tx_run || EXIT_CODE=1
126+
```
127+
**Change:** Uncommented all bookmark tests (V4 and V5)
128+
129+
### 4. **testkit.sh** - Fixed venv Setup
130+
131+
**Location:** Lines 30-36
132+
```bash
133+
cd testkit || (echo 'cannot cd into testkit' && exit 1)
134+
if [ ! -d venv ]; then
135+
python3 -m venv venv
136+
fi
137+
source venv/bin/activate
138+
pip install --upgrade pip
139+
pip install -r requirements.txt
140+
```
141+
**Change:** Added check to avoid recreating venv if it already exists
142+
143+
## How Bookmarks Now Work
144+
145+
### Transaction Flow with Bookmarks
146+
147+
1. **Session Creation with Initial Bookmarks:**
148+
```
149+
Session created with bookmarks: ["neo4j:bookmark:v1:tx42"]
150+
```
151+
152+
2. **First Transaction Begins:**
153+
```
154+
BEGIN {"bookmarks": ["neo4j:bookmark:v1:tx42"], "mode": "w"}
155+
```
156+
- The `mode: 'w'` is sent because we pass `true` to `buildRunExtra()`
157+
- Initial bookmarks are sent to ensure causal consistency
158+
159+
3. **First Transaction Commits:**
160+
```
161+
COMMIT
162+
SUCCESS {"bookmark": "neo4j:bookmark:v1:tx4242"}
163+
```
164+
- Server returns new bookmark
165+
- `BoltCommitMessage` updates `BookmarkHolder` with new bookmark
166+
- Transaction state is reset via `setTransactionFinished()`
167+
168+
4. **Second Transaction Begins:**
169+
```
170+
BEGIN {"bookmarks": ["neo4j:bookmark:v1:tx4242"]}
171+
```
172+
- Uses the UPDATED bookmark from first transaction
173+
- Ensures second transaction sees changes from first transaction
174+
175+
5. **Second Transaction Commits:**
176+
```
177+
COMMIT
178+
SUCCESS {"bookmark": "neo4j:bookmark:v1:tx424242"}
179+
```
180+
- Session now has the latest bookmark
181+
182+
### Key Points
183+
184+
- **Bookmark Chaining:** Each transaction uses the bookmark from the previous transaction
185+
- **Session-Level Bookmarks:** The `BookmarkHolder` in the session maintains the current bookmark
186+
- **Causality Guarantee:** Bookmarks ensure that reads see writes from previous transactions
187+
- **Mode is Critical:** Sending `mode: 'w'` or `mode: 'r'` with BEGIN is essential for proper bookmark handling
188+
189+
## Running the Tests
190+
191+
To run all bookmark tests:
192+
193+
```bash
194+
cd /home/pratiksha/dev/neo4j-php-client
195+
docker compose up testkit
196+
```
197+
198+
This will run:
199+
- 5 V5 bookmark tests
200+
- 4 V4 bookmark tests
201+
202+
## Expected Results
203+
204+
All 9 bookmark tests should now pass:
205+
- ✅ test_bookmarks_can_be_set
206+
- ✅ test_last_bookmark
207+
- ✅ test_send_and_receive_bookmarks_write_tx
208+
- ✅ test_sequence_of_writing_and_reading_tx
209+
- ✅ test_send_and_receive_multiple_bookmarks_write_tx
210+
- ✅ test_bookmarks_on_unused_sessions_are_returned
211+
- ✅ test_bookmarks_session_run
212+
- ✅ test_sequence_of_writing_and_reading_tx (V4)
213+
- ✅ test_bookmarks_tx_run
214+

‎READY_TO_TEST.md‎

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
# Bookmark Tests - Ready to Test
2+
3+
## Summary of Changes
4+
5+
I've made comprehensive changes to fix the bookmark handling in the Neo4j PHP client. Here's what was done:
6+
7+
### 1. **Fixed `BoltConnection.php`**
8+
- ✅ Fixed `isStreaming()` to handle null protocol safely
9+
- ✅ Improved `begin()` to only consume results when actually streaming
10+
- ✅ Ensured BEGIN sends `mode: 'w'` or `mode: 'r'` (critical for bookmarks)
11+
- ✅ Fixed `rollback()` to reset transaction state
12+
- ✅ Added `setTransactionFinished()` method for commit tracking
13+
- ✅ Added detailed debug logging throughout
14+
15+
### 2. **Fixed `BoltCommitMessage.php`**
16+
- ✅ Now calls `setTransactionFinished()` after successful commit
17+
- ✅ Added logging for bookmark updates
18+
- ✅ Properly updates `BookmarkHolder` with new bookmarks
19+
20+
### 3. **Fixed `Session.php`**
21+
- ✅ Added extensive debug logging to track transaction flow
22+
- ✅ Added error handling for all exception types
23+
24+
### 4. **Fixed `SessionBeginTransaction.php` (testkit backend)**
25+
- ✅ Added comprehensive error handling and logging
26+
- ✅ Catches all `Throwable` types, not just `Neo4jException`
27+
28+
### 5. **Updated `testkit.sh`**
29+
- ✅ Enabled all 9 bookmark tests (5 V5 + 4 V4)
30+
- ✅ Fixed venv setup
31+
32+
## How to Run the Tests
33+
34+
```bash
35+
cd /home/pratiksha/dev/neo4j-php-client
36+
37+
# Make sure all containers are stopped
38+
docker compose down
39+
40+
# Run all bookmark tests
41+
docker compose up testkit
42+
```
43+
44+
## Expected Test Results
45+
46+
All 9 bookmark tests should pass:
47+
48+
### V5 Tests:
49+
1. ✓ `test_bookmarks_can_be_set` - Verifies bookmarks can be set on session creation
50+
2. ✓ `test_last_bookmark` - Verifies bookmark is returned after commit
51+
3. ✓ `test_send_and_receive_bookmarks_write_tx` - Verifies bookmark flow in write transactions
52+
4. ✓ `test_sequence_of_writing_and_reading_tx` - Verifies bookmark chaining across multiple transactions
53+
5. ✓ `test_send_and_receive_multiple_bookmarks_write_tx` - Verifies handling of multiple bookmarks
54+
55+
### V4 Tests:
56+
6. ✓ `test_bookmarks_on_unused_sessions_are_returned` - Verifies bookmarks persist on unused sessions
57+
7. ✓ `test_bookmarks_session_run` - Verifies bookmarks with auto-commit transactions
58+
8. ✓ `test_sequence_of_writing_and_reading_tx` - Verifies bookmark chaining (V4)
59+
9. ✓ `test_bookmarks_tx_run` - Verifies bookmarks in explicit transactions
60+
61+
## Debugging
62+
63+
If tests fail, check logs:
64+
65+
```bash
66+
# Follow backend logs
67+
docker compose logs -f testkit_backend
68+
69+
# Check for errors
70+
docker compose logs testkit_backend | grep -i "error\|exception"
71+
72+
# Check detailed transaction flow
73+
docker compose logs testkit_backend | grep -i "begin\|commit\|bookmark"
74+
```
75+
76+
## What the Fixes Accomplish
77+
78+
### Bookmark Flow:
79+
1. **Session created with initial bookmarks** → `BookmarkHolder` initialized
80+
2. **BEGIN transaction** → Sends bookmarks + mode ('w'/'r') to server
81+
3. **COMMIT transaction** → Server returns new bookmark
82+
4. **Bookmark updated** → `BookmarkHolder` stores new bookmark
83+
5. **Next transaction** → Uses updated bookmark (bookmark chaining)
84+
85+
### Key Features:
86+
- ✅ **Causality chains**: Each transaction uses the bookmark from the previous one
87+
- ✅ **Read-after-write consistency**: Ensures reads see previous writes
88+
- ✅ **Session-level bookmarks**: Bookmarks maintained across transaction lifecycle
89+
- ✅ **Transaction state tracking**: Proper `inTransaction` flag management
90+
- ✅ **Error handling**: Comprehensive error catching and logging
91+
92+
## Files Modified
93+
94+
1. `src/Bolt/BoltConnection.php` - Core connection logic
95+
2. `src/Bolt/Messages/BoltCommitMessage.php` - Commit bookmark handling
96+
3. `src/Bolt/Session.php` - Session transaction management
97+
4. `testkit-backend/src/Handlers/SessionBeginTransaction.php` - Test backend error handling
98+
5. `testkit-backend/testkit.sh` - Test script configuration
99+
100+
## Next Steps
101+
102+
Run the tests:
103+
```bash
104+
docker compose up testkit
105+
```
106+
107+
The tests will:
108+
1. Start the testkit Python runner
109+
2. Connect to the testkit_backend (PHP)
110+
3. Run stub server tests
111+
4. Report results
112+
113+
Expected output:
114+
```
115+
...
116+
Ran 9 tests
117+
118+
OK
119+
```
120+
121+
If any test fails, the detailed logs will help identify the issue.
122+

0 commit comments

Comments
 (0)