Feat mongo transactions #20
Conversation
…ansactions # Conflicts: # composer.lock # src/Client.php
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client
participant MongoDB
User->>Client: startSession()
Client->>MongoDB: send startSession command
MongoDB-->>Client: return sessionId
Client-->>User: sessionId
User->>Client: commitTransaction(lsid, txnNumber, autocommit)
Client->>MongoDB: send commitTransaction command
MongoDB-->>Client: commit result
Client->>MongoDB: send endSessions command
MongoDB-->>Client: endSessions result
User->>Client: abortTransaction(lsid, txnNumber, autocommit)
Client->>MongoDB: send abortTransaction command
MongoDB-->>Client: abort result
Client->>MongoDB: send endSessions command
MongoDB-->>Client: endSessions result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/Client.php (2)
807-832: Well-implemented session ID handling with good flexibility.The method correctly handles multiple input formats and properly transforms them for the MongoDB command. Consider adding validation for empty arrays.
Consider adding a check for empty input:
public function endSessions(array $lsids, array $options = []) { + if (empty($lsids)) { + return null; + } + // Extract session IDs from the format ['id' => sessionId] and format as objects $sessionIds = array_map(function ($lsid) {
740-832: Consider adding unit tests for the new transaction functionality.The new session and transaction methods should have comprehensive test coverage to ensure reliability.
Would you like me to help generate unit tests for these new methods or open an issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
src/Client.php(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests (8.3)
🔇 Additional comments (2)
src/Client.php (2)
40-43: Constants follow established patterns correctly.The new command constants are properly defined and align with MongoDB's wire protocol command names.
755-779: Improve error handling for transaction lifecycle management.The session is ended regardless of whether the commit actually succeeds. If an exception is thrown by the query method, the session will remain open.
Apply this diff to handle errors properly:
public function commitTransaction(array $lsid, int $txnNumber, bool $autocommit = false) { $txnNumber = new \MongoDB\BSON\Int64($txnNumber); - $result = $this->query([ - self::COMMAND_COMMIT_TRANSACTION => 1, - 'lsid' => $lsid, - 'txnNumber' => $txnNumber, - 'autocommit' => $autocommit - ], 'admin'); - - // End the session after successful commit - $this->endSessions([$lsid]); - - return $result; + try { + $result = $this->query([ + self::COMMAND_COMMIT_TRANSACTION => 1, + 'lsid' => $lsid, + 'txnNumber' => $txnNumber, + 'autocommit' => $autocommit + ], 'admin'); + + // End the session after successful commit + $this->endSessions([$lsid]); + + return $result; + } catch (Exception $e) { + // Don't end session on failure - let caller decide + throw $e; + } }Likely an incorrect or invalid review comment.
| /** | ||
| * Start a new logical session. Returns the session id object.. | ||
| * | ||
| * @return object | ||
| * @throws Exception | ||
| */ | ||
| public function startSession(): object | ||
| { | ||
| $result = $this->query([ | ||
| self::COMMAND_START_SESSION => 1 | ||
| ], 'admin'); | ||
|
|
||
| return $result->id->id; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix inconsistent documentation and add defensive property checks.
The method documentation states it returns "the session id object" but actually returns the id value. Additionally, the nested property access could fail without proper checks.
Apply this diff to fix the issues:
/**
- * Start a new logical session. Returns the session id object..
+ * Start a new logical session. Returns the session ID.
*
- * @return object
+ * @return mixed The session ID
* @throws Exception
*/
- public function startSession(): object
+ public function startSession(): mixed
{
$result = $this->query([
self::COMMAND_START_SESSION => 1
], 'admin');
- return $result->id->id;
+ if (!property_exists($result, 'id') || !property_exists($result->id, 'id')) {
+ throw new Exception('Invalid session response structure');
+ }
+
+ return $result->id->id;
}🤖 Prompt for AI Agents
In src/Client.php around lines 740 to 753, the method startSession's docblock
incorrectly states it returns an object while it actually returns the session id
value. Also, the code accesses nested properties without checking if they exist,
which can cause errors. Update the docblock to indicate the method returns a
string (or appropriate type for the session id), and add defensive checks to
ensure $result, $result->id, and $result->id->id exist before returning the id
value, throwing an exception or handling the error if any property is missing.
| /** | ||
| * Abort (rollback) a transaction. | ||
| * | ||
| * @param array $lsid | ||
| * @param int $txnNumber | ||
| * @param bool $autocommit | ||
| * @return mixed | ||
| * @throws Exception | ||
| */ | ||
| public function abortTransaction(array $lsid, int $txnNumber, bool $autocommit = false) | ||
| { | ||
| $txnNumber = new \MongoDB\BSON\Int64($txnNumber); | ||
|
|
||
| $result = $this->query([ | ||
| self::COMMAND_ABORT_TRANSACTION => 1, | ||
| 'lsid' => $lsid, | ||
| 'txnNumber' => $txnNumber, | ||
| 'autocommit' => $autocommit | ||
| ], 'admin'); | ||
|
|
||
| // End the session after successful rollback | ||
| $this->endSessions([$lsid]); | ||
|
|
||
| return $result; | ||
| } |
There was a problem hiding this comment.
Apply consistent error handling as with commitTransaction.
This method has the same session lifecycle issue - the session is ended even if the abort fails.
Apply similar error handling:
public function abortTransaction(array $lsid, int $txnNumber, bool $autocommit = false)
{
$txnNumber = new \MongoDB\BSON\Int64($txnNumber);
- $result = $this->query([
- self::COMMAND_ABORT_TRANSACTION => 1,
- 'lsid' => $lsid,
- 'txnNumber' => $txnNumber,
- 'autocommit' => $autocommit
- ], 'admin');
-
- // End the session after successful rollback
- $this->endSessions([$lsid]);
-
- return $result;
+ try {
+ $result = $this->query([
+ self::COMMAND_ABORT_TRANSACTION => 1,
+ 'lsid' => $lsid,
+ 'txnNumber' => $txnNumber,
+ 'autocommit' => $autocommit
+ ], 'admin');
+
+ // End the session after successful rollback
+ $this->endSessions([$lsid]);
+
+ return $result;
+ } catch (Exception $e) {
+ // Don't end session on failure - let caller decide
+ throw $e;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Abort (rollback) a transaction. | |
| * | |
| * @param array $lsid | |
| * @param int $txnNumber | |
| * @param bool $autocommit | |
| * @return mixed | |
| * @throws Exception | |
| */ | |
| public function abortTransaction(array $lsid, int $txnNumber, bool $autocommit = false) | |
| { | |
| $txnNumber = new \MongoDB\BSON\Int64($txnNumber); | |
| $result = $this->query([ | |
| self::COMMAND_ABORT_TRANSACTION => 1, | |
| 'lsid' => $lsid, | |
| 'txnNumber' => $txnNumber, | |
| 'autocommit' => $autocommit | |
| ], 'admin'); | |
| // End the session after successful rollback | |
| $this->endSessions([$lsid]); | |
| return $result; | |
| } | |
| /** | |
| * Abort (rollback) a transaction. | |
| * | |
| * @param array $lsid | |
| * @param int $txnNumber | |
| * @param bool $autocommit | |
| * @return mixed | |
| * @throws Exception | |
| */ | |
| public function abortTransaction(array $lsid, int $txnNumber, bool $autocommit = false) | |
| { | |
| $txnNumber = new \MongoDB\BSON\Int64($txnNumber); | |
| try { | |
| $result = $this->query([ | |
| self::COMMAND_ABORT_TRANSACTION => 1, | |
| 'lsid' => $lsid, | |
| 'txnNumber' => $txnNumber, | |
| 'autocommit' => $autocommit | |
| ], 'admin'); | |
| // End the session after successful rollback | |
| $this->endSessions([$lsid]); | |
| return $result; | |
| } catch (Exception $e) { | |
| // Don't end session on failure - let caller decide | |
| throw $e; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/Client.php around lines 781 to 805, the abortTransaction method ends the
session immediately after the query without checking if the abort succeeded,
which can cause session lifecycle issues. Modify the method to first perform the
abort query and check for success, then only call endSessions if the abort was
successful, similar to the error handling pattern used in commitTransaction.
This ensures the session is not ended prematurely on failure.
Summary by CodeRabbit
New Features
Enhancements