Uh oh!
There was an error while loading. Please reload this page.
Unique Exception - #733
Conversation
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesUnique constraint exception mapping
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Database/Adapter/Postgres.php (1)
1928-1928: Refresh PHPDoc to reflectUniqueException.Now that
processException()returnsUniqueExceptionfor duplicate rows, the@throws DuplicateExceptionannotations on methods such ascreateDocument()andupdateDocument()are stale. Please swap them to@throws UniqueException(and keep Duplicate where still valid) so downstream callers see the correct type.- * @throws DuplicateException+ * @throws UniqueExceptionApply the same adjustment to other affected docblocks in this class.
src/Database/Adapter/MariaDB.php (1)
1799-1799: Synchronize PHPDoc with new exception type.Since duplicate-key violations now surface as
UniqueException, please update the affected@throws DuplicateExceptionannotations (e.g., oncreateDocument()/updateDocument()) to mentionUniqueException, keeping any remaining Duplicate mentions only where they’re still thrown.- * @throws DuplicateException+ * @throws UniqueExceptionUpdate the parallel spots in this adapter as well.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Database/Adapter/MariaDB.php(2 hunks)src/Database/Adapter/Postgres.php(2 hunks)src/Database/Exception/Unique.php(1 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(5 hunks)tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php(3 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 (5)
tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php (2)
src/Database/Exception.php (1)
Exception(7-21)src/Database/Exception/Unique.php (1)
Unique(7-9)
src/Database/Exception/Unique.php (1)
src/Database/Exception.php (1)
Exception(7-21)
src/Database/Adapter/MariaDB.php (2)
src/Database/Exception.php (1)
Exception(7-21)src/Database/Exception/Unique.php (1)
Unique(7-9)
src/Database/Adapter/Postgres.php (2)
src/Database/Exception.php (1)
Exception(7-21)src/Database/Exception/Unique.php (1)
Unique(7-9)
tests/e2e/Adapter/Scopes/DocumentTests.php (2)
src/Database/Exception.php (1)
Exception(7-21)src/Database/Exception/Unique.php (1)
Unique(7-9)
⏰ 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)
src/Database/Exception/Unique.php (1)
7-9: Looks good.Dedicated
Uniqueexception aligns with the new behavior.tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php (1)
13-807: Tests updated appropriately.Catching
UniqueExceptionkeeps the one-to-one E2Es aligned with the adapter change.tests/e2e/Adapter/Scopes/DocumentTests.php (5)
17-17: Import alignment looks good.Adding the
UniqueExceptionalias keeps the test expectations consistent with the new exception class.
4733-4733: Updated expectation matches new behavior.Switching to
UniqueExceptionreflects the adapter change for unique constraint violations.
4798-4798: Good call on aligning the update test.Expecting
UniqueExceptionhere mirrors the new adapter semantics during conflict updates.
5303-5303: Consistent duplicate insert expectation.Updating this assertion to
UniqueExceptionkeeps the duplicate-create scenario in sync with the new exception contract.
5319-5319: Case-insensitive duplicate now consistent.Using
UniqueExceptionhere matches the revised behavior for duplicate IDs regardless of casing.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Adapter/SQLite.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Adapter/SQLite.php (2)
src/Database/Exception.php (1)
Exception(7-21)src/Database/Exception/Unique.php (1)
Unique(7-9)
⏰ 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). (10)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MySQL)
🔇 Additional comments (1)
src/Database/Adapter/SQLite.php (1)
15-15: LGTM!The import is correctly added and the alias
UniqueExceptionis clear and appropriate.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Database/Adapter/SQLite.php (2)
520-520: Verify PHPDoc comment has been updated.A previous review noted that the PHPDoc comment at line 520 should be updated from
@throws Duplicateto@throws UniqueException. Please ensure this documentation reflects the new exception type. The same applies to line 642.
11-11: Remove the unusedDuplicateimport.The
Duplicateexception is no longer used after routing errors throughprocessException. This was flagged in a previous review but remains unaddressed.Apply this diff:
-use Utopia\Database\Exception\Duplicate;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Adapter/SQLite.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Adapter/SQLite.php (6)
src/Database/Exception.php (1)
Exception(7-21)src/Database/Exception/Unique.php (1)
Unique(7-9)src/Database/Adapter/MariaDB.php (1)
processException(1775-1831)src/Database/Adapter/Postgres.php (1)
processException(1909-1942)src/Database/Adapter/MySQL.php (1)
processException(147-164)src/Database/Adapter/SQL.php (1)
processException(1943-1946)
⏰ 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). (11)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Unit Test
🔇 Additional comments (2)
src/Database/Adapter/SQLite.php (2)
622-622: LGTM! Exception handling now properly centralized.Both
createDocumentandupdateDocumentnow correctly route PDOExceptions throughprocessException, providing consistent error handling across the adapter. This addresses the concerns from the previous review.Also applies to: 841-841
1247-1250: LGTM! Duplicate row handling correctly updated for SQLite.The logic correctly identifies SQLite unique constraint violations using SQLSTATE '23000' and driver-specific error code 19 (SQLITE_CONSTRAINT), then throws
UniqueExceptioninstead of the previousDuplicateException. This aligns with the changes made to the MariaDB and Postgres adapters.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Database/Adapter/MariaDB.php(4 hunks)src/Database/Exception/Unique.php(1 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(5 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 (3)
tests/e2e/Adapter/Scopes/DocumentTests.php (2)
src/Database/Exception.php (1)
Exception(7-21)src/Database/Exception/Unique.php (1)
Unique(5-7)
src/Database/Exception/Unique.php (1)
src/Database/Exception.php (1)
Exception(7-21)
src/Database/Adapter/MariaDB.php (2)
src/Database/Exception.php (1)
Exception(7-21)src/Database/Exception/Unique.php (1)
Unique(5-7)
🪛 GitHub Actions: Linter
src/Database/Adapter/MariaDB.php
[error] 1-1: PSR-12: single_space_around_construct
⏰ 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). (10)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MySQL)
🔇 Additional comments (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
4738-4767: Double-check expectation for UniqueException on unique indexesLine 4756 currently asserts the duplicate insert on a unique index bubbles up as
DuplicateExceptionbut notUniqueException. Given the PR headline (“surface Unique vs Duplicate exceptions based on constraint context”), can you confirm that this scenario is really supposed to stay on the genericDuplicateExceptionpath? If the adapters now classify unique-constraint violations asUniqueException, thisassertNotInstanceOfwill start failing. Please double-check the intended contract here and adjust either the adapters or the test accordingly.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Adapter/Postgres.php (1)
928-934: Remove debug code and fix Postgres-specific regex pattern.This code has critical issues that prevent it from working correctly:
- Debug code: Lines 928 and 931 contain
var_dump()calls that will pollute production logs and responses.- Wrong regex pattern: The regex
/for key '(?:[^.]+\.)?([^']+)'/is designed for MariaDB error messages ("Duplicate entry '...' for key 'key_name'"), but Postgres uses a different format:"duplicate key value violates unique constraint \"constraint_name\"".As a result, the regex will never match Postgres errors, and the new
UniqueExceptionwill never be thrown despite the code being added.Apply this diff to fix both issues:
- if ($e->getCode() === '23505' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 7) {- var_dump($e->getMessage());- if (preg_match("/for key '(?:[^.]+\.)?([^']+)'/", $e->getMessage(), $m)) {- if ($m[1] === '_uid' || $m[1] === 'PRIMARY') {- var_dump($m);- return new UniqueException('Document already exists', $e->getCode(), $e);- }- }-+ if ($e->getCode() === '23505' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 7) {+ // Postgres format: "duplicate key value violates unique constraint \"constraint_name\""+ if (preg_match('/unique constraint "([^"]+)"/', $e->getMessage(), $m)) {+ // Extract the base constraint name (remove namespace/tenant prefix if present)+ $constraintName = $m[1];+ if (preg_match('/_uid$/', $constraintName)) {+ return new UniqueException('Document already exists', $e->getCode(), $e);+ }+ }+ return new DuplicateException('Document already exists', $e->getCode(), $e); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Database/Adapter/MariaDB.php(4 hunks)src/Database/Adapter/Postgres.php(2 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(5 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 (3)
src/Database/Adapter/Postgres.php (2)
src/Database/Database.php (1)
Database(37-8144)src/Database/Exception/Unique.php (1)
Unique(5-7)
src/Database/Adapter/MariaDB.php (2)
src/Database/Exception.php (1)
Exception(7-21)src/Database/Exception/Unique.php (1)
Unique(5-7)
tests/e2e/Adapter/Scopes/DocumentTests.php (2)
src/Database/Exception.php (1)
Exception(7-21)src/Database/Exception/Unique.php (1)
Unique(5-7)
⏰ 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). (11)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Unit Test
🔇 Additional comments (7)
src/Database/Adapter/Postgres.php (1)
16-16: LGTM!The import alias for
UniqueExceptionis correctly added and follows the existing import pattern.tests/e2e/Adapter/Scopes/DocumentTests.php (4)
17-17: Import for UniqueException looks goodAlias is correct and aligns with new exception type.
5333-5335: Good fix: clearing $sequence before reusing the DocumentThis avoids accidental internal ID collisions when creating another document from a previously fetched instance.
5339-5345: Verify assumption: case-insensitive duplicate should not map to UniqueThe test enforces Duplicate and not Unique. Ensure this path is consistently handled at the application layer (case-insensitive $id comparison) across all adapters, regardless of DB collation. Otherwise, some adapters may throw Unique or allow both IDs.
Do we have adapter-level guarantees normalizing $id uniqueness case-insensitively before hitting the DB?
4738-4767: Adapt exception assertion to be adapter-agnostic
Assert DuplicateException for all adapters; only assert UniqueException on SQL adapters. For example:- } catch (Throwable $e) {- $this->assertInstanceOf(UniqueException::class, $e);- $this->assertInstanceOf(DuplicateException::class, $e);- }+ } catch (Throwable $e) {+ $this->assertInstanceOf(DuplicateException::class, $e);+ if ($database->getAdapter() instanceof SQL) {+ $this->assertInstanceOf(UniqueException::class, $e);+ }+ }Confirm that all CI adapters now throw UniqueException for unique-constraint violations; otherwise this change is necessary.
src/Database/Adapter/MariaDB.php (2)
15-15: LGTM!The import statement for
UniqueExceptionis correctly added and properly namespaced.
820-821: LGTM! Docblocks now correctly document both exception types.The docblocks for both
createDocument()andupdateDocument()now properly advertise that they can throw bothUniqueException(for unique constraint violations on non-document-ID fields) andDuplicateException(for document ID conflicts on_uidorPRIMARYkeys). This accurately reflects the implementation's behavior.Also applies to: 947-948
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/Database/Adapter/MariaDB.php (1)
1801-1808: Handle PRIMARY key duplicates as DuplicateException.MySQL reports PK conflicts as “for key 'PRIMARY'”. Treat these as document duplicates too.
- if (preg_match("/for key '(?:[^.]+\.)?([^']+)'/", $e->getMessage(), $matches)) {- if ($matches[1] === '_uid') {+ if (preg_match("/for key '(?:[^.]+\.)?([^']+)'/", $e->getMessage(), $matches)) {+ if ($matches[1] === '_uid' || $matches[1] === 'PRIMARY') { return new DuplicateException('Document already exists', $e->getCode(), $e); } } return new UniqueException('Document already exists', $e->getCode(), $e);tests/e2e/Adapter/Scopes/DocumentTests.php (2)
4803-4810: Reapply SQL-scoped Unique assertion (still outstanding)Line 4808 repeats the unconditional
UniqueExceptionassertion we previously flagged; it will break non-SQL adapters. Please make theUniqueExceptioncheck conditional on$database->getAdapter() instanceof SQL, keepingDuplicateExceptionas the baseline. (Same fix as earlier feedback.)
4738-4767: Ensure adapter-agnostic duplicate assertionsLine 4745 currently requires
UniqueExceptionfor every adapter. Non-SQL adapters only raiseDuplicateException, so this assertion will fail those runs. AssertDuplicateExceptionunconditionally and gate theUniqueExceptioncheck behind aninstanceof SQLguard.} catch (Throwable $e) { - $this->assertInstanceOf(UniqueException::class, $e);- $this->assertInstanceOf(DuplicateException::class, $e);+ $this->assertInstanceOf(DuplicateException::class, $e);+ if ($database->getAdapter() instanceof SQL) {+ $this->assertInstanceOf(UniqueException::class, $e);+ } }
🧹 Nitpick comments (4)
src/Database/Adapter/Postgres.php (1)
1928-1938: Also treat primary key (_id) duplicates as DuplicateException.For parity with MariaDB/SQLite and intent (“document duplicate” vs other unique), map PK conflicts to DuplicateException too.
Apply this within the same block:
if ($e->getCode() === '23505' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 7) { if (preg_match('/Key \(([^)]+)\)=\(.+\) already exists/', $e->getMessage(), $matches)) { $columns = array_map('trim', explode(',', $matches[1])); sort($columns); $target = $this->sharedTables ? ['_tenant', '_uid'] : ['_uid']; if ($columns == $target) { return new DuplicateException('Document already exists', $e->getCode(), $e); } + // Primary key duplicate (_id) also indicates a document-duplicate+ if ($columns == ['_id']) {+ return new DuplicateException('Document already exists', $e->getCode(), $e);+ } } return new UniqueException('Document already exists', $e->getCode(), $e); }Also consider updating createDocument/updateDocument docblocks to include both UniqueException and DuplicateException to reflect this behavior.
src/Database/Adapter/SQLite.php (2)
520-521: Docblock should also include DuplicateException.processException can return DuplicateException for _uid/PK conflicts.
* @throws Exception * @throws PDOException - * @throws UniqueException+ * @throws UniqueException+ * @throws DuplicateException
639-640: Docblock should also include DuplicateException.Mirror createDocument; both paths can bubble DuplicateException.
* @throws Exception * @throws PDOException - * @throws UniqueException+ * @throws UniqueException+ * @throws DuplicateExceptiontests/e2e/Adapter/Scopes/DocumentTests.php (1)
5345-5345: Drop leftover debug output
var_dump($e);on Line 5345 leaks into the test output and should be removed.- var_dump($e);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Database/Adapter/MariaDB.php(4 hunks)src/Database/Adapter/Postgres.php(2 hunks)src/Database/Adapter/SQLite.php(6 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(5 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 (4)
src/Database/Adapter/Postgres.php (2)
src/Database/Exception.php (1)
Exception(7-21)src/Database/Exception/Unique.php (1)
Unique(5-7)
src/Database/Adapter/MariaDB.php (3)
src/Database/Database.php (1)
Database(37-8144)src/Database/Exception.php (1)
Exception(7-21)src/Database/Exception/Unique.php (1)
Unique(5-7)
tests/e2e/Adapter/Scopes/DocumentTests.php (3)
src/Database/Exception.php (1)
Exception(7-21)src/Database/Exception/Unique.php (1)
Unique(5-7)src/Database/Document.php (2)
Document(12-470)removeAttribute(287-293)
src/Database/Adapter/SQLite.php (6)
src/Database/Exception.php (1)
Exception(7-21)src/Database/Exception/Unique.php (1)
Unique(5-7)src/Database/Adapter/MariaDB.php (1)
processException(1777-1839)src/Database/Adapter/Postgres.php (1)
processException(1909-1951)src/Database/Adapter/SQL.php (1)
processException(1943-1946)src/Database/Adapter/MySQL.php (1)
processException(147-164)
⏰ 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). (11)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Unit Test
🔇 Additional comments (7)
src/Database/Adapter/Postgres.php (1)
16-16: Import looks good.Consistent with other adapters.
src/Database/Adapter/SQLite.php (3)
15-15: Import looks good.Aligns with new exception usage.
622-622: Good: delegate to processException.Consistent, centralized mapping.
841-842: Good: delegate to processException.Keeps behavior consistent.
src/Database/Adapter/MariaDB.php (3)
15-15: Import looks good.Matches new exception strategy.
820-823: Docblock update looks good.Both UniqueException and DuplicateException are advertised.
947-950: Docblock update looks good.Accurately reflects thrown exceptions.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Database/Adapter/SQLite.php (1)
1244-1261: Remove debug output; tighten duplicate classification to exact keys.
- var_dump leaks to output; remove it.
- Current in_array('_uid', $columns) can misclassify other unique constraints. Compare sorted sets to ['_uid'] (or ['_tenant','_uid'] when shared) or ['_id'] only.
Apply this diff:
// Duplicate row if ($e->getCode() === '23000' && ($e->errorInfo[1] ?? null) === 19) { $msg = $e->errorInfo[2] ?? $e->getMessage(); - // Match all table.column pairs (handles commas & spaces)- if (preg_match_all('/\b([^.]+)\.([^\s,]+)/', $msg, $matches, PREG_SET_ORDER)) {- $columns = array_map(fn ($m) => $m[2], $matches);- sort($columns);-- var_dump($columns);-- if ($columns === ['_tenant', '_uid'] || in_array('_uid', $columns)) {- return new DuplicateException('Document already exists', $e->getCode(), $e);- }- }+ // Extract columns from: "UNIQUE constraint failed: tbl._tenant, tbl._uid" or "tbl._id"+ if (preg_match('/UNIQUE constraint failed:\s+(.+)/', $msg, $m)) {+ $parts = array_map('trim', explode(',', $m[1]));+ $cols = array_map(function (string $p): string {+ $dot = strrpos($p, '.');+ $col = $dot !== false ? substr($p, $dot + 1) : $p;+ return rtrim($col, ', ');+ }, $parts);+ sort($cols);+ $target = $this->sharedTables ? ['_tenant', '_uid'] : ['_uid'];+ if ($cols === $target || $cols === ['_id']) {+ return new DuplicateException('Document already exists', $e->getCode(), $e);+ }+ } return new UniqueException('Document already exists', $e->getCode(), $e); }
🧹 Nitpick comments (1)
src/Database/Adapter/SQLite.php (1)
520-521: Docblocks: also declare DuplicateException.These methods can throw both UniqueException and DuplicateException via processException. Add DuplicateException to the @throws list.
* @throws Exception * @throws PDOException - * @throws UniqueException+ * @throws UniqueException+ * @throws DuplicateExceptionAlso applies to: 639-640
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Database/Adapter/Postgres.php(2 hunks)src/Database/Adapter/SQLite.php(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Database/Adapter/Postgres.php (2)
src/Database/Exception.php (1)
Exception(7-21)src/Database/Exception/Unique.php (1)
Unique(5-7)
src/Database/Adapter/SQLite.php (5)
src/Database/Exception/Unique.php (1)
Unique(5-7)src/Database/Adapter/Postgres.php (1)
processException(1909-1953)src/Database/Adapter/MariaDB.php (1)
processException(1777-1839)src/Database/Adapter/MySQL.php (1)
processException(147-164)src/Database/Adapter/SQL.php (1)
processException(1943-1946)
⏰ 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). (10)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (MySQL)
🔇 Additional comments (4)
src/Database/Adapter/Postgres.php (1)
16-16: LGTM: added UniqueException import.src/Database/Adapter/SQLite.php (3)
15-15: LGTM: added UniqueException import.
622-623: LGTM: centralizes exception mapping.Routing through processException is correct and consistent.
841-842: LGTM: centralizes exception mapping.Routing through processException is correct and consistent.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Adapter/MariaDB.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Adapter/MariaDB.php (3)
src/Database/Database.php (1)
Database(37-8144)src/Database/Exception.php (1)
Exception(7-21)src/Database/Exception/Unique.php (1)
Unique(5-7)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Greptile SummaryThe PR introduces a distinct
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (4): Last reviewed commit: "(fix): classify unique violations by vio..." | Re-trigger Greptile |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
a163eb3 to
e10f682CompareChiragAgg5k
commented
Aug 12, 2026
Rebased this PR onto current What changed:
Local checks: Note: a local targeted PHPUnit run requires service containers ( |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Database/Adapter/Postgres.php`:
- Around line 2163-2166: Update the unique-violation classification in the
Postgres adapter to parse the PostgreSQL “DETAIL: Key (...)” column list and
compare those columns with the document ID key, rather than searching the full
exception message for “_uid”. Return UniqueException for violations of other
unique indexes and preserve DuplicateException only when the extracted key
columns identify the document ID.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 954c30f7-6805-42a0-a063-7634d821a09f
📒 Files selected for processing (7)
src/Database/Adapter/MariaDB.phpsrc/Database/Adapter/Memory.phpsrc/Database/Adapter/Mongo.phpsrc/Database/Adapter/Postgres.phpsrc/Database/Adapter/Redis.phpsrc/Database/Adapter/SQLite.phptests/e2e/Adapter/Scopes/DocumentTests.php
💤 Files with no reviewable changes (5)
- src/Database/Adapter/Redis.php
- src/Database/Adapter/Memory.php
- src/Database/Adapter/SQLite.php
- tests/e2e/Adapter/Scopes/DocumentTests.php
- src/Database/Adapter/Mongo.php
Uh oh!
There was an error while loading. Please reload this page.
Searching the whole driver message for `_uid` misreads any conflict whose index name or conflicting value happens to contain that substring, so a plain unique-index violation was reported as a document-ID duplicate. Each adapter now parses the key the driver actually names — MariaDB/MySQL the key name, Postgres the DETAIL column list, SQLite the failed column list, Mongo the index name — and falls back to DuplicateException when the message cannot be parsed, preserving pre-existing behaviour rather than silently downgrading a document-ID duplicate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
abnegate
commented
Aug 13, 2026
Pushed 23eaafb addressing the open review threads. What changed Every adapter was classifying the conflict by searching the whole driver message for
When the message cannot be parsed, all four fall back to
Verification
@greptileai review |
Summary by CodeRabbit
New Features
Bug Fixes
Tests