fix: skip orphan reconciliation for shared tables in createCollection#841
Merged
fix: skip orphan reconciliation for shared tables in createCollection#841
Conversation
In shared-tables mode, physical tables are reused across tenants. When a second tenant calls createCollection and the physical table already exists, the DuplicateException should not trigger the drop-and-recreate orphan reconciliation logic — that would destroy all other tenants' data. For _metadata, re-throw the DuplicateException so Database::create() properly signals that the database already exists. For other collections, skip the physical table recreation and proceed to create the per-tenant metadata document. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
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:
📝 WalkthroughWalkthroughUpdated Database collection creation and rollback logic to respect adapter shared-tables mode; expanded adapter exception mappings for Mongo, Postgres, and SQLite to convert DB-specific errors into DuplicateException/NotFoundException; added e2e tests for shared-tables multi-tenant collection creation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
participant Adapter
participant PhysicalDB
Client->>Database: createCollection(collectionId, tenant)
Database->>Adapter: createCollection(collectionId, tenant-scoped)
Adapter->>PhysicalDB: create table/collection
alt success
PhysicalDB-->>Adapter: created
Adapter-->>Database: success
Database-->>Client: metadata create
else DuplicateException from Adapter
Adapter-->>Database: DuplicateException
Database->>Adapter: getSharedTables()
alt shared-tables = true
Database->>Adapter: exists(database, collectionId)?
alt exists
Database-->>Client: treat as non-orphan DuplicateException (do not drop physical table)
else not exists
Database-->>Client: rethrow DuplicateException
end
else shared-tables = false
Database->>Adapter: cleanupCollection(collectionId) (best-effort drop physical)
Database->>Adapter: createCollection(... ) to re-create
Database-->>Client: metadata create or error
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Greptile SummaryThis PR fixes a critical data-corruption bug in Key changes and concerns:
Confidence Score: 3/5
Important Files Changed
Last reviewed commit: "fix: skip orphan rec..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
… add rollback observability - Remove _metadata DuplicateException re-throw in shared-tables mode so Database::create() works for every tenant, not just the first - Add Console::warning when metadata document creation fails in shared-tables mode to make partial state visible in logs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Database/Database.php`:
- Around line 1796-1800: The current suppression of DuplicateException when
$this->adapter->getSharedTables() is true can create divergent tenant metadata
($attributes, $indexes) because physical table may not actually match; update
the Database method handling table creation (the block that catches
DuplicateException) to, upon catching DuplicateException in shared-tables mode,
fetch and compare the existing physical table schema (columns and indexes)
against the requested $attributes and $indexes and only suppress the exception
if they match exactly; otherwise throw/fail-fast so metadata is not written for
a mismatched table. Apply the same validation for the other DuplicateException
catch path (the 1822–1829 flow) so both code paths validate existing schema
before persisting tenant metadata.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 044d4be0-e432-42ed-835d-aa26927a440d
📒 Files selected for processing (1)
src/Database/Database.php
- Verify physical table exists before writing tenant metadata when skipping DuplicateException in shared-tables mode - Add test for multi-tenant createCollection in shared-tables v1 mode (per-tenant metadata docs) - Add test for multi-tenant Database::create() idempotency Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/e2e/Adapter/Scopes/CollectionTests.php (1)
1333-1392: Protect shared DB state reset withtry/finallyand restore tenant as well.Line 1333+ and Line 1399+ capture mutable state, but cleanup runs only on the happy path and currently omits tenant restoration. A failing assertion can leak state into later tests.
Suggested cleanup pattern (apply to both new tests)
$database = $this->getDatabase(); $sharedTables = $database->getSharedTables(); $namespace = $database->getNamespace(); $schema = $database->getDatabase(); + $tenant = $database->getAdapter()->getTenant(); if (!$database->getAdapter()->getSupportForSchemas()) { $this->expectNotToPerformAssertions(); return; } - $dbName = 'stMultiTenant'; - if ($database->exists($dbName)) { - $database->setDatabase($dbName)->delete(); - } - - // ... test body ... - - $database - ->setSharedTables($sharedTables) - ->setNamespace($namespace) - ->setDatabase($schema); + try { + $dbName = 'stMultiTenant'; + if ($database->exists($dbName)) { + $database->setDatabase($dbName)->delete(); + } + + // ... test body ... + } finally { + $database + ->setSharedTables($sharedTables) + ->setNamespace($namespace) + ->setDatabase($schema) + ->setTenant($tenant); + }Also applies to: 1399-1430
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/Adapter/Scopes/CollectionTests.php` around lines 1333 - 1392, This test mutates database state (sharedTables, namespace, database/schema and tenant) but only restores on the happy path; wrap the section that sets db state and creates collections in a try/finally so restoration always runs and also restore the original tenant; capture original values (e.g., $sharedTables, $namespace, $schema and $originalTenant = $database->getTenant() or equivalent) before modifying, perform the test logic (setDatabase(...)->setNamespace('')->setSharedTables(true)->setTenant($tenant1)->create() and subsequent createCollection/getCollection calls) in the try block, and in finally call setSharedTables($sharedTables)->setNamespace($namespace)->setDatabase($schema)->setTenant($originalTenant) so state is reset even on assertion failures (apply same pattern to the other test block referenced at 1399-1430).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/Adapter/Scopes/CollectionTests.php`:
- Around line 1357-1387: After switching back to tenant1, add assertions that
verify tenant1's collection metadata and contents were not lost by tenant2's
createCollection; specifically, after calling $database->setTenant($tenant1) and
$database->getCollection('multiTenantCol') (col1Again), assert
$col1Again->isEmpty() is false and assert that
\count($col1Again->getAttribute('attributes')) is still 1 (and optionally verify
any expected rows/records remain). This ensures createCollection for tenant2 did
not drop/recreate the physical table for tenant1.
---
Nitpick comments:
In `@tests/e2e/Adapter/Scopes/CollectionTests.php`:
- Around line 1333-1392: This test mutates database state (sharedTables,
namespace, database/schema and tenant) but only restores on the happy path; wrap
the section that sets db state and creates collections in a try/finally so
restoration always runs and also restore the original tenant; capture original
values (e.g., $sharedTables, $namespace, $schema and $originalTenant =
$database->getTenant() or equivalent) before modifying, perform the test logic
(setDatabase(...)->setNamespace('')->setSharedTables(true)->setTenant($tenant1)->create()
and subsequent createCollection/getCollection calls) in the try block, and in
finally call
setSharedTables($sharedTables)->setNamespace($namespace)->setDatabase($schema)->setTenant($originalTenant)
so state is reset even on assertion failures (apply same pattern to the other
test block referenced at 1399-1430).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f25922b2-d635-4d36-b512-1feb66ea98a6
📒 Files selected for processing (2)
src/Database/Database.phptests/e2e/Adapter/Scopes/CollectionTests.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Database/Database.php
- Use getIdAttributeType() to pick string vs int tenant values for Mongo vs SQL adapters - Run multi-tenant tests within existing shared-tables database when available (SharedTables/* test classes) - Add cleanup to prevent test pollution Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Mongo: extend createCollection pre-check to all collections in shared-tables mode, not just _metadata - Postgres: add 42P01 (Undefined table) → NotFoundException in processException - Postgres: wrap deleteCollection in try-catch with processException to match MariaDB behavior Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/e2e/Adapter/Scopes/CollectionTests.php (1)
1331-1410: Wrap mutable test setup/cleanup intry/finallyto avoid state leakage.Both tests mutate database name, namespace, shared-tables mode, and tenant. If an assertion fails before the tail cleanup, later tests can run with polluted state.
Refactor pattern
- // setup ... - // test body ... - $database - ->setSharedTables($sharedTables) - ->setNamespace($namespace) - ->setDatabase($schema) - ->setTenant($originalTenant); + try { + // setup ... + // test body ... + } finally { + $database + ->setSharedTables($sharedTables) + ->setNamespace($namespace) + ->setDatabase($schema) + ->setTenant($originalTenant); + }Also applies to: 1414-1459
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/Adapter/Scopes/CollectionTests.php` around lines 1331 - 1410, The test mutates $database state (namespace, database/schema, sharedTables, tenant) and performs create/delete operations without guaranteeing cleanup on failure; wrap the mutable setup, test actions (setting $database->setDatabase(...)->setNamespace(...)->setSharedTables(...)->setTenant(...), createCollection calls, and conditional delete/deleteCollection logic) in a try/finally so the finally block restores $database->setSharedTables($sharedTables)->setNamespace($namespace)->setDatabase($schema)->setTenant($originalTenant) and removes created test data (delete or deleteCollection) regardless of assertions; apply the same try/finally pattern to the second similar test block that spans the other region noted (the block that also uses $createdDb, $tenant1/$tenant2, createCollection/getCollection/delete/deleteCollection).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/Adapter/Scopes/CollectionTests.php`:
- Around line 1420-1449: The local variable $createdDb in
testSharedTablesMultiTenantCreate is assigned but never used; remove the unused
variable declaration and any related assignments (references to $createdDb) so
the test only performs tenant setup and create/delete operations without storing
to $createdDb, keeping the rest of the method (database->setTenant, create,
exists, delete, etc.) intact.
---
Nitpick comments:
In `@tests/e2e/Adapter/Scopes/CollectionTests.php`:
- Around line 1331-1410: The test mutates $database state (namespace,
database/schema, sharedTables, tenant) and performs create/delete operations
without guaranteeing cleanup on failure; wrap the mutable setup, test actions
(setting
$database->setDatabase(...)->setNamespace(...)->setSharedTables(...)->setTenant(...),
createCollection calls, and conditional delete/deleteCollection logic) in a
try/finally so the finally block restores
$database->setSharedTables($sharedTables)->setNamespace($namespace)->setDatabase($schema)->setTenant($originalTenant)
and removes created test data (delete or deleteCollection) regardless of
assertions; apply the same try/finally pattern to the second similar test block
that spans the other region noted (the block that also uses $createdDb,
$tenant1/$tenant2, createCollection/getCollection/delete/deleteCollection).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e367953-0924-4858-ab60-4bb736512f05
📒 Files selected for processing (3)
src/Database/Adapter/Mongo.phpsrc/Database/Adapter/Postgres.phptests/e2e/Adapter/Scopes/CollectionTests.php
…xception Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…Exception
The Mongo client throws Exception('Collection Exists') with code 0
rather than using MongoDB's native error code 48. This was only caught
by the pre-check, leaving the in-transaction path unhandled.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/e2e/Adapter/Scopes/CollectionTests.php (1)
1425-1448:testSharedTablesMultiTenantCreate()never checks the_metadatastate it is meant to protect.In the shared-tables branch this only asserts “no exception”, and in the schema-backed branch it only checks
exists($dbName). Both still pass if the secondcreate()recreates_metadataor loses tenant metadata. Add a sentinel collection or metadata entry under tenant 1 and verify it still exists after tenant 2 callscreate().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/Adapter/Scopes/CollectionTests.php` around lines 1425 - 1448, In testSharedTablesMultiTenantCreate() add a sentinel metadata entry/collection under tenant1 before calling the second database->create() and then assert that the sentinel still exists after tenant2 calls create(); specifically, in the shared-tables branch (where you call setTenant($tenant1)->create() then setTenant($tenant2)->create()) insert a tenant-scoped entry into the _metadata store after the first create and verify it remains after the second create, and in the schema-backed branch (after setDatabase($dbName)->setNamespace('')->setSharedTables(true)->setTenant($tenant1)->create()) likewise write a sentinel into _metadata for tenant1 and assert it still exists after setTenant($tenant2)->create(); use the existing methods setTenant(), create(), exists(), and the _metadata collection name to locate where to add the insert and subsequent assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Database/Adapter/Mongo.php`:
- Around line 3523-3525: The new path converting a Mongo error with code===0 and
"Collection Exists" into a DuplicateException causes Mongo::createCollection()'s
local catch to return true and bypass Database::createCollection()'s
shared-table vs orphan-reconciliation logic; only treat this as a no-op for the
shared-table/metadata case and rethrow otherwise. Update the creation/handling
of the DuplicateException in src/Database/Adapter/Mongo.php so that the
code===0/"Collection Exists" case is converted to a DuplicateException only when
the call context indicates a shared-table/metadata operation (the same context
Database::createCollection() expects); for non-shared/in-transaction paths do
not swallow the original exception—either rethrow the original exception or
throw a DuplicateException that will not be caught by the local catch in
Mongo::createCollection() so Database::createCollection() can perform the
drop-and-recreate flow. Ensure you reference the same error-identification
(stripos($e->getMessage(), 'Collection Exists')) and preserve existing behavior
for the code===48 branch and for shared-table metadata paths.
In `@tests/e2e/Adapter/Scopes/CollectionTests.php`:
- Around line 1329-1410: The test testSharedTablesMultiTenantCreateCollection
mutates Database state (setDatabase, setNamespace, setSharedTables, setTenant)
and performs create/delete operations and must restore original state in a
finally block to avoid leaking state on failures; wrap the body that changes
$database (including the branch that sets $createdDb, calls create(),
createCollection(), deleteCollection(), delete()) in try { ... } finally { ... }
and move the final restore/reset of $sharedTables, $namespace, $schema, and
$originalTenant plus any cleanup (delete or deleteCollection calls) into the
finally; apply the same try/finally pattern to the other similar test block
referenced (lines 1412-1461) so both restore state reliably.
---
Nitpick comments:
In `@tests/e2e/Adapter/Scopes/CollectionTests.php`:
- Around line 1425-1448: In testSharedTablesMultiTenantCreate() add a sentinel
metadata entry/collection under tenant1 before calling the second
database->create() and then assert that the sentinel still exists after tenant2
calls create(); specifically, in the shared-tables branch (where you call
setTenant($tenant1)->create() then setTenant($tenant2)->create()) insert a
tenant-scoped entry into the _metadata store after the first create and verify
it remains after the second create, and in the schema-backed branch (after
setDatabase($dbName)->setNamespace('')->setSharedTables(true)->setTenant($tenant1)->create())
likewise write a sentinel into _metadata for tenant1 and assert it still exists
after setTenant($tenant2)->create(); use the existing methods setTenant(),
create(), exists(), and the _metadata collection name to locate where to add the
insert and subsequent assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 053acfc5-85c7-4f3a-b995-a3c50d9e6a45
📒 Files selected for processing (3)
src/Database/Adapter/Mongo.phpsrc/Database/Adapter/SQLite.phptests/e2e/Adapter/Scopes/CollectionTests.php
- DuplicateException with table missing: do orphan recovery (drop + recreate) in all modes, not just per-tenant - Track whether we created the physical table; only rollback on metadata failure if we own the table, regardless of shared-tables mode - Removes mode-specific branching in the rollback path Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds getSchemaIndexes() to match the existing getSchemaAttributes() pattern. When an index exists in physical schema but not metadata (orphan), createIndex now validates the physical definition matches. Mismatched orphans are dropped and recreated. - Adapter: abstract getSchemaIndexes() + getSupportForSchemaIndexes() - MariaDB: queries INFORMATION_SCHEMA.STATISTICS - All others: default stubs (empty array / false) - Database::createIndex: validates orphan index shape before reuse Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…adata paths The code-0 "Collection Exists" from the Mongo client was being converted to DuplicateException in processException, which the local catch in createCollection silently returned true for — bypassing Database::createCollection()'s orphan reconciliation in non-shared mode. Move the handling into createCollection itself where context is available: return true for shared-tables/metadata, throw DuplicateException otherwise so the caller can reconcile. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Match the attribute validator's behavior: when isMigrating is true in shared-tables mode, skip the schema index check so indexes are always created fresh during migration rather than being matched against or dropping existing physical indexes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
2 participants
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.
In shared-tables mode, physical tables are reused across tenants. When a second tenant calls createCollection and the physical table already exists, the DuplicateException should not trigger the drop-and-recreate orphan reconciliation logic — that would destroy all other tenants' data.
For _metadata, re-throw the DuplicateException so Database::create() properly signals that the database already exists. For other collections, skip the physical table recreation and proceed to create the per-tenant metadata document.
Summary by CodeRabbit
Bug Fixes
Tests