-
Notifications
You must be signed in to change notification settings - Fork 55
Postgres case insensitive for UID #739
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughPostgreSQL adapter collation definitions updated from utf8_ci to utf8_ci_ai with adjusted locale settings. COLLATE clauses added to indexed columns (_uid, _document) across shared and non-shared table configurations. Test exception handling refactored from expectException to try-catch blocks for duplicate detection scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
5331-5342: Add accent‑insensitive duplicate check for $id (Postgres‑only)To fully cover the PR objective, also assert that 'café' vs 'cafe' (and/or 'CAFÉ') duplicates on $id under Postgres.
Example test to add (scoped to Postgres):
public function testExceptionAccentInsensitiveDuplicateUID(): void { /** @var Database $database */ $database = static::getDatabase(); // Only relevant for the Postgres adapter if (!($database->getAdapter() instanceof \Utopia\Database\Adapter\Postgres)) { $this->expectNotToPerformAssertions(); return; } $doc = new Document([ '$id' => 'café', '$permissions' => [Permission::read(Role::any())], ]); // First insert $doc->removeAttribute('$sequence'); $database->createDocument('documents', $doc); // Same ID without accent should conflict under accent-insensitive collation $doc->setAttribute('$id', 'cafe'); $doc->removeAttribute('$sequence'); try { $database->createDocument('documents', $doc); $this->fail('Failed to throw exception'); } catch (\Throwable $e) { $this->assertInstanceOf(DuplicateException::class, $e); } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Database/Adapter/Postgres.php(5 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
PR: utopia-php/database#721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
tests/e2e/Adapter/Scopes/DocumentTests.php
🧬 Code graph analysis (2)
tests/e2e/Adapter/Scopes/DocumentTests.php (5)
src/Database/Adapter/Postgres.php (2)
createDocument(950-1050)create(135-162)src/Database/Adapter/MariaDB.php (2)
createDocument(822-933)create(28-43)src/Database/Adapter/SQLite.php (2)
createDocument(522-630)create(113-116)src/Database/Database.php (2)
createDocument(4176-4270)create(1241-1260)src/Database/Document.php (3)
Document(12-470)getId(63-66)removeAttribute(287-293)
src/Database/Adapter/Postgres.php (3)
src/Database/Adapter/SQLite.php (1)
getSQLTable(1080-1083)src/Database/Adapter/SQL.php (1)
getSQLTable(1704-1707)src/Database/Adapter.php (1)
filter(1196-1205)
⏰ 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: Setup & Build Docker Image
🔇 Additional comments (7)
tests/e2e/Adapter/Scopes/DocumentTests.php (3)
4737-4765: LGTM: duplicate on unique index asserted correctlyTry/catch pattern and DuplicateException assertion look good and consistent with the suite.
4801-4807: LGTM: update collision handledUpdate path correctly asserts DuplicateException via try/catch.
5309-5319: LGTM: preparing re-insert by clearing $sequenceClearing '$sequence' before each create avoids manual sequence leakage; duplicate $id is asserted properly.
src/Database/Adapter/Postgres.php (4)
247-247: LGTM! Collation correctly applied to _uid indexes.The COLLATE clause is properly added to the unique indexes on the
_uidcolumn for both shared and non-shared table modes, ensuring case-insensitive and accent-insensitive matching for the $id attribute as intended by the PR objectives.Also applies to: 254-254
855-855: LGTM! Appropriate simplification of index attribute formatting.The removal of per-attribute COLLATE logic from
createIndexis correct, as the collation is now explicitly applied only to the built-in indexes on_uidand_documentcolumns increateCollection. This makes the behavior more explicit and aligns with the PR objective to limit the collation changes to the $id attribute rather than all unique indexes.
903-903: LGTM! Improved variable naming for clarity.Renaming the variable from
$nameto$collectionimproves code readability by using a more descriptive identifier.
282-282: Verify whether COLLATE on_documentshould be applied to shared mode permissions index.The asymmetry is confirmed: in non-shared mode (line 282), the
_documentcolumn in the unique permissions index getsCOLLATE utf8_ci_ai, while in shared mode (lines 273–274), the_documentcolumn in the unique index has no COLLATE clause. No inline comments explain this difference.Both modes use
_documentin the unique index structure, so this difference may be intentional or an oversight. Confirm whether shared mode should also applyCOLLATE utf8_ci_aion_documentfor consistency, or document the architectural reason for the asymmetry.
Support Postgres case-insensitivity and accent-insensitive ONLY on $id attribute unique index
Summary by CodeRabbit