Open
Conversation
Contributor
📝 WalkthroughWalkthroughcomposer.json aliases utopia-php/database to "dev-unique-exception as 2.3.2". Two new exception constants were added: DOCUMENT_UNIQUE_VIOLATION and ROW_UNIQUE_VIOLATION. app/config/errors.php was updated to expose those exceptions as public errors with HTTP 409. Documents Action gained protected getUniqueException() to choose the appropriate constant. Documents Create now catches Utopia\Database\Exception\Unique and maps it via getUniqueException(). Tests added testUniqueUid twice (duplicate definitions) and updated assertions for duplicate-ID conflicts. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
…que-exception # Conflicts: # composer.lock
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
composer.json(1 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). (18)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E Shared Mode Service Test (Dev Keys) (Shared V2)
- GitHub Check: E2E Shared Mode Service Test (Site Screenshots) (Shared V1)
- GitHub Check: Unit Test
- GitHub Check: E2E General Test
✨ Benchmark results
⚡ Benchmark Comparison
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
app/config/errors.php(2 hunks)src/Appwrite/Extend/Exception.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php(3 hunks)tests/e2e/Services/Databases/Legacy/DatabasesBase.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (3)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Action.php (1)
isCollectionsAPI(57-62)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Action.php (1)
isCollectionsAPI(48-51)src/Appwrite/Extend/Exception.php (1)
Exception(7-466)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php (2)
src/Appwrite/Extend/Exception.php (1)
Exception(7-466)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (1)
getUniqueException(133-138)
app/config/errors.php (1)
src/Appwrite/Extend/Exception.php (1)
Exception(7-466)
tests/e2e/Services/Databases/Legacy/DatabasesBase.php (2)
tests/e2e/Client.php (1)
Client(8-342)tests/e2e/Scopes/Scope.php (1)
getHeaders(145-145)
🪛 GitHub Actions: Linter
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php
[error] 1-1: PSR-12: ordered_imports failed. 1 style issue. (PHP CS/PHPCS lint: ordered_imports)
⏰ 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). (20)
- GitHub Check: E2E Shared Mode Service Test (GraphQL, Shared V1)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Shared Mode Service Test (Dev Keys) (Shared V2)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: Unit Test
- GitHub Check: E2E General Test
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: scan
🔇 Additional comments (3)
src/Appwrite/Extend/Exception.php (1)
215-215: LGTM! New unique violation constants follow the existing pattern.The new
DOCUMENT_UNIQUE_VIOLATIONandROW_UNIQUE_VIOLATIONconstants are well-placed and follow the established naming convention for exception types.Also applies to: 225-225
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php (1)
445-446: Exception handling for unique violations is correctly implemented.The catch block for
UniqueExceptionis properly positioned before the more generalDuplicateExceptionand correctly maps to the appropriate error code viagetUniqueException().app/config/errors.php (1)
765-769: LGTM! Clear error mappings for unique constraint violations.The new error mappings for
DOCUMENT_UNIQUE_VIOLATIONandROW_UNIQUE_VIOLATIONare well-structured and appropriate:
- HTTP 409 correctly represents conflict status for unique constraint violations
- Descriptions are clear and actionable for API consumers
- Structure is consistent with existing error definitions
- Proper differentiation from
DOCUMENT_ALREADY_EXISTS/ROW_ALREADY_EXISTS(which handle_uidviolations specifically)Also applies to: 807-811
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php
Outdated
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/Services/Databases/Legacy/DatabasesBase.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/Services/Databases/Legacy/DatabasesBase.php (2)
tests/e2e/Client.php (1)
Client(8-342)tests/e2e/Scopes/Scope.php (1)
getHeaders(145-145)
⏰ 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). (20)
- GitHub Check: E2E Shared Mode Service Test (Databases/TablesDB, Shared V2)
- GitHub Check: E2E Shared Mode Service Test (Projects, Shared V2)
- GitHub Check: E2E Shared Mode Service Test (Databases/Legacy, Shared V1)
- GitHub Check: E2E Shared Mode Service Test (Locale, Shared V1)
- GitHub Check: E2E Shared Mode Service Test (Account, Shared V1)
- GitHub Check: E2E Shared Mode Service Test (Console, Shared V1)
- GitHub Check: E2E Shared Mode Service Test (Avatars, Shared V1)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Shared Mode Service Test (Site Screenshots) (Shared V1)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E Shared Mode Service Test (Dev Keys) (Shared V2)
- GitHub Check: E2E Shared Mode Service Test (Dev Keys) (Shared V1)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E Shared Mode Service Test (Site Screenshots) (Shared V2)
- GitHub Check: Unit Test
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
| ]); | ||
|
|
||
| $this->assertEquals(409, $duplicate['headers']['status-code']); | ||
| $this->assertEquals('Document with the requested ID already exists. Try again with a different ID or use ID.unique() to generate a unique ID.', $duplicate['body']['message']); |
Contributor
There was a problem hiding this comment.
Assert the new unique error type
We need to verify the response surfaces Exception::DOCUMENT_UNIQUE_VIOLATION, otherwise the new UniqueException path is untested.
$this->assertEquals(409, $duplicate['headers']['status-code']);
+ $this->assertEquals(Exception::DOCUMENT_UNIQUE_VIOLATION, $duplicate['body']['type']);
$this->assertEquals('Document with the requested ID already exists. Try again with a different ID or use ID.unique() to generate a unique ID.', $duplicate['body']['message']);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/e2e/Services/Databases/Legacy/DatabasesBase.php around line 4288, the
test only asserts the error message for duplicate documents but does not assert
the error type; add an assertion that the response surfaces the new
Exception::DOCUMENT_UNIQUE_VIOLATION type (e.g. insert an assertion such as
assertEquals(Exception::DOCUMENT_UNIQUE_VIOLATION, $duplicate['body']['type'])
in addition to the existing message assertion) so the UniqueException path is
covered.
Comment on lines
+4324
to
+4326
| $this->assertEquals(409, $document['headers']['status-code']); | ||
| $this->assertEquals('Document with the requested ID already exists. Try again with a different ID or use ID.unique() to generate a unique ID.', $document['body']['message']); | ||
| } |
Contributor
There was a problem hiding this comment.
Also assert the unique violation type here
Same concern: without checking the type, the test won’t catch regressions in the new exception mapping.
$this->assertEquals(409, $document['headers']['status-code']);
+ $this->assertEquals(Exception::DOCUMENT_UNIQUE_VIOLATION, $document['body']['type']);
$this->assertEquals('Document with the requested ID already exists. Try again with a different ID or use ID.unique() to generate a unique ID.', $document['body']['message']);📝 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.
Suggested change
| $this->assertEquals(409, $document['headers']['status-code']); | |
| $this->assertEquals('Document with the requested ID already exists. Try again with a different ID or use ID.unique() to generate a unique ID.', $document['body']['message']); | |
| } | |
| $this->assertEquals(409, $document['headers']['status-code']); | |
| $this->assertEquals(Exception::DOCUMENT_UNIQUE_VIOLATION, $document['body']['type']); | |
| $this->assertEquals( | |
| 'Document with the requested ID already exists. Try again with a different ID or use ID.unique() to generate a unique ID.', | |
| $document['body']['message'] | |
| ); |
🤖 Prompt for AI Agents
In tests/e2e/Services/Databases/Legacy/DatabasesBase.php around lines 4324 to
4326, the test asserts the 409 status and error message but does not assert the
error "type"; add an assertion that verifies document['body']['type'] matches
the expected unique-violation type (e.g. assertEquals('unique_violation',
$document['body']['type'])), or, if the project exposes a constant or mapped
value for that type, use that constant instead to ensure the test catches
regressions in exception-to-type mapping.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added support for UniqueException to differentiate it from DuplicateException.
UniqueException extends DuplicateException for backwards compatibility. It will be thrown for all unique constraint violations except those caused by the _uid constraint.
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