Feat txn #10023
Feat txn #10023
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds transactional support for databases and grids: two new metadata collections (transactions, transactionLogs), transaction TTL constants, new transaction-related error codes and Exception constants, two new scopes (transactions.read, transactions.write), and validators/models (Operation, Transactions query validator, Transaction model/list). Marks SDK Response readonly and registers transaction models. Extends many document and grid endpoints to accept an optional transactionId for staging operations, and adds HTTP endpoints to create, list, get, update, delete transactions and to add operations. Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (4)
app/controllers/api/databases.php (4)
4598-4623: Same counter / limit hole forupdateDocumentstaging
updateDocumentwrites a log entry (action =update) but likewise forgets to bumptransactions.operations, leaving the transaction inconsistent and limits unenforced.
5256-5263: Bulk-update staging also forgets to bumpoperationsThe
bulkUpdatepath logs the action but never adjuststransactions.operations, compounding the inconsistency noted above.
5355-5376: Bulk-upsert staging: same missing counter updateIdentical issue – please ensure the transaction document is patched alongside the logs.
5482-5488: Delete-within-transaction not countedThe
deleteDocumentstaging block writes the log but omits the counter update.
🧹 Nitpick comments (7)
app/init/constants.php (1)
55-57: Constants fine, but add unit in the name or php-doc for clarityAll three TTL constants are seconds, while many other timeout constants are suffixed with
_MILLISECONDS.
Consider either renaming (…_SECONDS) or adding a short php-doc to avoid confusion when the values are consumed next to the millisecond ones.src/Appwrite/Utopia/Database/Validator/Operation.php (2)
57-64: Type validation runs before presence validation fordata.Because
dataisn’t guaranteed to exist (see above), theis_array($value['data'])call will still emit a notice even if you add it to$requiredbut leave this block here.
Move thedata-presence check to the loop above or guard witharray_key_exists.
65-69: Inefficientin_arraycall in a hot path.
$this->actionsis static – convert it to aconstarray + useisset($map[$action])for O(1) lookup if performance on large payloads matters.src/Appwrite/Utopia/Response/Model/Transaction.php (2)
31-36:statusfield lacks server-side enforcement of allowed values.Add the
allowedkey to the rule so malformed responses can be caught early.- ->addRule('status', [ - 'type' => self::TYPE_STRING, - 'description' => 'Current status of the transaction. One of: pending, committing, committed, rolled_back, failed.', - 'default' => 'pending', - 'example' => 'pending', - ]) + ->addRule('status', [ + 'type' => self::TYPE_STRING, + 'allowed' => ['pending', 'committing', 'committed', 'rolled_back', 'failed'], + 'description' => 'Current status of the transaction.', + 'default' => 'pending', + 'example' => 'pending', + ])
12-18: Model is missing theoperationscounter present in the collection.Exposing it would let clients display progress and enforce limits without extra calls.
app/config/collections/projects.php (1)
2515-2567: Consider adding an automatic TTL index onexpiresAt.A regular KEY index won’t clean up rows automatically.
If you rely on MySQL you’ll need a scheduled job; in Mongo-like stores you can switch to a TTL index to avoid manual sweeps.tests/e2e/Services/Databases/DatabasesBase.php (1)
4110-4110: Fix grammatical issue in comment- '$updatedAt' => '2022-08-01 13:09:23.050' // system will update it not api + '$updatedAt' => '2022-08-01 13:09:23.050' // system will update it, not API
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/config/collections/projects.php(1 hunks)app/config/errors.php(1 hunks)app/config/roles.php(1 hunks)app/config/scopes.php(1 hunks)app/controllers/api/databases.php(31 hunks)app/init/constants.php(1 hunks)src/Appwrite/Extend/Exception.php(1 hunks)src/Appwrite/SDK/Response.php(1 hunks)src/Appwrite/Utopia/Database/Validator/Operation.php(1 hunks)src/Appwrite/Utopia/Database/Validator/Queries/Transactions.php(1 hunks)src/Appwrite/Utopia/Response.php(4 hunks)src/Appwrite/Utopia/Response/Model/Transaction.php(1 hunks)tests/e2e/Services/Databases/DatabasesBase.php(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (16)
src/Appwrite/SDK/Response.php (1)
5-5:readonlyrequires PHP 8.2 – confirm your minimum runtimeDeclaring the whole class as
readonlyis great for immutability, but this syntax is supported only from PHP 8.2 upward.
If the project still targets 8.1 or earlier it will fatal-error at load‐time.
Please double-check thecomposer.json/ CI matrix and bump the platform requirement if necessary.app/config/scopes.php (1)
43-48: Scope names added correctly – remember to wire them into roles & docsNo functional issues spotted with the new
transactions.*scopes.
Just ensure they are:
- Added to the relevant roles in
app/config/roles.php.- Reflected in the public API docs / SDK generation matrix.
app/config/errors.php (1)
813-848: Comprehensive error set – keep codes uniqueThe new transaction error definitions look consistent and follow existing naming & HTTP-code conventions. ✔️
Double-check that no other constant shares the numeric 410/409/400 slots for the same domain to avoid ambiguity in clients.src/Appwrite/Utopia/Database/Validator/Operation.php (1)
80-83:getType()advertises an object whileisArray()returns true.Down-stream consumers might treat TYPE_OBJECT as a map of named properties, not a numerically-indexed array. Verify this aligns with Utopia’s expectations, otherwise return
TYPE_ARRAY.src/Appwrite/Extend/Exception.php (1)
232-240: Constants addition LGTM.Names follow existing style and are unique; no further issues spotted.
app/config/collections/projects.php (1)
2570-2649: Schema and validator drift (actionenum).
transactionLogs.actioncomment listsincrement | decrement, but these are not accepted byOperationvalidator (see earlier comment).
Align both sides to prevent runtime rejects.src/Appwrite/Utopia/Response.php (5)
108-108: Transaction model import added – looks goodThe new
Transactionmodel is correctly imported and used later in the constructor.
169-170: Constants registered for transactions
MODEL_TRANSACTIONandMODEL_TRANSACTION_LISTare consistent with the existing naming scheme and ensure the new entities can be referenced everywhere.
420-420: List registration for transactions
BaseList('Transaction List', … 'transactions', …)wires the collection path and the item model properly. No issues spotted.
519-519: Entity model registration
->setModel(new Transaction())finalises the response-layer support. All good.
375-375: Let’s inspect the surrounding routes to confirm the intended URL segment for Auth Providers:#!/bin/bash sed -n '350,430p' src/Appwrite/Utopia/Response.phpapp/config/roles.php (2)
7-9: Verify that newly added scopes are definedScopes such as
assistant.read,avatars.read,graphql,home, andrules.readhave been added for member roles. Make sure they are present inapp/config/scopes.php; otherwise authentication will reject these tokens.Also applies to: 12-19, 22-30
80-82: Transactions scopes – double-check consistency
transactions.read/transactions.writeare now assigned to admins. Confirm that:
- They are defined in
scopes.php.- All new API routes are protected by the corresponding scope middleware.
If either point is missing, admins might receive 401/403 responses.
tests/e2e/Services/Databases/DatabasesBase.php (3)
5582-5627: Well-structured transaction creation testsThe test properly covers both success and failure scenarios for transaction creation, including TTL validation boundaries.
5778-5860: Comprehensive transaction commit testThe test properly validates the transaction commit flow, including attribute creation wait time and document verification after commit.
3894-3989: Potential data integrity issue in testThe test creates documents with
releaseYearandactorsfields, but these attributes are never defined in the collection schema. Only thetitleattribute is created. This could lead to unexpected behavior or test failures if the system enforces strict schema validation.#!/bin/bash # Check if there are other tests in the codebase that create documents with undefined attributes rg -A 10 -B 5 "releaseYear|actors" tests/e2e/Services/Databases/ | grep -E "(createAttribute|documents.*data)" | head -20
src/Appwrite/Utopia/Database/Validator/Queries/Transactions.php
Outdated
Show resolved
Hide resolved
# Conflicts: # app/config/errors.php # app/config/roles.php # app/controllers/api/databases.php # composer.json # composer.lock # src/Appwrite/Extend/Exception.php # src/Appwrite/Utopia/Response.php # tests/e2e/Services/Databases/Legacy/DatabasesBase.php
✨ Benchmark results
⚡ Benchmark Comparison
|
# Conflicts: # composer.json # composer.lock # tests/e2e/Services/Databases/Legacy/DatabasesBase.php
Co-authored-by: jakeb994 <jakeb994@gmail.com>
…-per-transaction-6dd5 Enforce operations cap per transaction
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
# Conflicts: # composer.lock
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist