Return new value atomically #604
Conversation
WalkthroughThe changes refactor the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
participant Adapter
Client->>Database: increaseDocumentAttribute(collection, id, attribute, value, max)
Database->>Database: Begin transaction
Database->>Database: Fetch document with forUpdate lock
Database->>Database: Authorize operation
Database->>Adapter: increaseDocumentAttribute(collection, id, attribute, value, updatedAt, min, max)
Adapter-->>Database: Success or throws PDOException
Database->>Database: Purge cache, trigger event
Database->>Database: Commit transaction
Database-->>Client: Return updated Document
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (11)
✨ Finishing Touches
🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
1024-1024:⚠️ Potential issueInconsistent return type expectations in error test cases.
These test methods still expect boolean returns (
trueorfalse) from the increment/decrement methods, which is inconsistent with the refactored methods that now return the new attribute values. These should be updated to expect the appropriate exceptions or new values instead of booleans.Lines:
- 1024:
testIncreaseLimitMaxexpectstruebut should likely expect an exception- 1036:
testDecreaseLimitMinexpectsfalsebut should likely expect an exception- 1049:
testIncreaseTextAttributeexpectsfalsebut should likely expect an exception- $this->assertEquals(true, $database->increaseDocumentAttribute('increase_decrease', $document->getId(), 'increase', 10.5, 102.4)); + // This should likely expect an exception or updated value, not boolean true - $this->assertEquals(false, $database->decreaseDocumentAttribute('increase_decrease', $document->getId(), 'decrease', 10, 99)); + // This should likely expect an exception, not boolean false - $this->assertEquals(false, $database->increaseDocumentAttribute('increase_decrease', $document->getId(), 'increase_text')); + // This should likely expect an exception, not boolean falseAlso applies to: 1036-1036, 1049-1049
♻️ Duplicate comments (1)
src/Database/Database.php (1)
5222-5226: Duplicate whitelist blockSame observations as in
increaseDocumentAttribute()– typo and duplication.
Extract to a shared helper/constant to avoid divergence.
🧹 Nitpick comments (2)
src/Database/Database.php (2)
5093-5099: Doc-block return type is outdated
increaseDocumentAttribute()now returnsint|floatbut the PHP-Doc still advertisesint.
Update the annotation to keep IDEs, static analysers and code search tools in sync.- * @return int The new value of the attribute after the increase + * @return int|float The new value of the attribute after the increase
5118-5134: Hard-coded whitelist & mis-spelling
- Typo –
$whiteList→$whitelist(project consistently uses camelCase).- Instead of a literal array, leverage the existing constants – it avoids drift when new numeric types are added.
-$whiteList = [ - self::VAR_INTEGER, - self::VAR_FLOAT -]; +// Only numeric scalar types are allowed +$whitelist = [self::VAR_INTEGER, self::VAR_FLOAT];Consider extracting the list into a private constant to remove duplication with
decreaseDocumentAttribute.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Database/Adapter.php(1 hunks)src/Database/Adapter/MariaDB.php(2 hunks)src/Database/Database.php(5 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/Database/Adapter.php (5)
src/Database/Adapter/Postgres.php (1)
increaseDocumentAttribute(1280-1312)src/Database/Adapter/MariaDB.php (1)
increaseDocumentAttribute(1385-1429)src/Database/Adapter/Pool.php (1)
increaseDocumentAttribute(463-466)src/Database/Database.php (1)
increaseDocumentAttribute(5107-5185)src/Database/Mirror.php (1)
increaseDocumentAttribute(951-954)
src/Database/Adapter/MariaDB.php (6)
src/Database/Adapter.php (2)
increaseDocumentAttribute(1156-1164)execute(1202-1202)src/Database/Adapter/Postgres.php (3)
increaseDocumentAttribute(1280-1312)execute(87-107)processException(1997-2030)src/Database/Database.php (1)
increaseDocumentAttribute(5107-5185)src/Database/Adapter/SQL.php (2)
execute(1753-1756)processException(1744-1747)src/Database/Adapter/SQLite.php (1)
processException(1225-1238)src/Database/Adapter/MySQL.php (1)
processException(86-99)
src/Database/Database.php (5)
src/Database/Adapter.php (4)
increaseDocumentAttribute(1156-1164)withTransaction(372-404)getDocument(670-670)trigger(429-439)src/Database/Adapter/Postgres.php (1)
increaseDocumentAttribute(1280-1312)src/Database/Adapter/MariaDB.php (1)
increaseDocumentAttribute(1385-1429)src/Database/Document.php (5)
getAttribute(212-219)getId(59-62)isEmpty(386-389)getUpdate(107-110)getUpdatedAt(160-163)src/Database/Validator/Authorization.php (4)
Authorization(7-225)skip(160-170)isValid(55-77)getDescription(41-44)
🪛 GitHub Actions: CodeQL
src/Database/Database.php
[error] 5184-5184: Cannot call method getAttribute() on bool|Utopia\Database\Document.
[error] 5280-5280: Cannot call method getAttribute() on bool|Utopia\Database\Document.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup & Build Docker Image
🔇 Additional comments (9)
tests/e2e/Adapter/Scopes/DocumentTests.php (4)
995-995: LGTM: Test assertion correctly updated for new return type.The assertion now verifies that
increaseDocumentAttributereturns the new attribute value (101) instead of a boolean, which aligns with the method's refactored behavior.
1001-1001: LGTM: Test assertion correctly updated for new return type.The assertion now verifies that
decreaseDocumentAttributereturns the new attribute value (99) instead of a boolean, which is consistent with the refactored method behavior.
1005-1005: LGTM: Test assertion correctly updated for float operations.The assertion correctly expects the new float value (105.5) to be returned by
increaseDocumentAttribute, demonstrating that the method works correctly with both integer and float attributes.
1009-1009: LGTM: Test assertion correctly updated for float operations.The assertion correctly expects the new float value (104.4) to be returned by
decreaseDocumentAttribute, maintaining consistency with the new return type behavior.src/Database/Adapter.php (1)
1156-1164: LGTM! Improved method signature readability.The reformatting from single-line to multi-line method signature enhances code readability and follows consistent formatting standards.
src/Database/Adapter/MariaDB.php (2)
1385-1393: LGTM! Method signature formatting improved.The multi-line method signature aligns with the abstract method declaration and enhances readability.
1422-1427: Excellent improvement to error handling.The change from direct execution with generic
DatabaseExceptionto a proper try-catch block that leveragesprocessException()is a significant improvement. This enables conversion of specific PDO error codes into domain-specific exceptions (TimeoutException, DuplicateException, etc.), providing better error handling and debugging capabilities.src/Database/Database.php (2)
5180-5185: Potentialbooldereference after earlierfalsereturnThis is the crash site highlighted by CodeQL: if the inner callback returned
false,$documentis a boolean.
Fixing the previous comment resolves this, but keep an assert here to satisfy static analysers:assert($document instanceof Document);🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 5184-5184: Cannot call method getAttribute() on bool|Utopia\Database\Document.
5194-5197: Sync the return annotation fordecreaseDocumentAttribute()The PHP-Doc correctly lists
int|float, but the method header comment a few lines earlier still says “@return int|float”.
Nothing critical, just confirm both annotations match.
Summary by CodeRabbit