Conversation
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:
📝 WalkthroughWalkthroughCreateCollection now drops and recreates orphaned physical collections on adapter DuplicateException and proceeds to metadata creation; rollback always attempts cleanup; many tenant equality checks changed from strict to loose (!=); Sequence validator treats null as valid, accepts strings/ints and enforces string for UUID7; tests updated for id acceptance. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
participant Adapter
participant MetadataStore
Client->>Database: createCollection(request)
Database->>Adapter: createPhysicalCollection(attrs)
alt Adapter throws DuplicateException
Database->>Adapter: dropPhysicalCollection(existing)
Database->>Adapter: createPhysicalCollection(attrs)
end
Database->>MetadataStore: persistCollectionMetadata(meta)
alt persist fails
Database->>Database: cleanupCollection(physical name)
Database->>Client: return error
else
Database->>Client: return success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Database.php (1)
1815-1815:⚠️ Potential issue | 🟠 MajorRemove dead condition at Line 1815 (currently failing PHPStan).
$createdis set totruein both successful creation paths (Line 1797 and Line 1805), soif ($created)at Line 1815 is always true.Suggested fix
- } catch (\Throwable $e) { - if ($created) { - try { - $this->cleanupCollection($id); - } catch (\Throwable $e) { - Console::error("Failed to rollback collection '{$id}': " . $e->getMessage()); - } - } + } catch (\Throwable $e) { + try { + $this->cleanupCollection($id); + } catch (\Throwable $e) { + Console::error("Failed to rollback collection '{$id}': " . $e->getMessage()); + } throw new DatabaseException("Failed to create collection metadata for '{$id}': " . $e->getMessage(), previous: $e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Database.php` at line 1815, The conditional "if ($created)" is always true because $created is set to true in both creation branches; remove the dead condition and run the contained block unconditionally (or refactor so $created is only set when intended) — locate the $created variable and the conditional around it and delete the "if ($created)" guard and its matching else/endif so the success-handling code (originally inside the if) executes directly; ensure no other logic relies on $created remaining as a guard.
🤖 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 1802-1804: In the duplicate-recovery flow in Database.php, wrap
the call to $this->adapter->deleteCollection($id) in a try/catch that
specifically catches the adapter's NotFoundException (or equivalent) and ignores
it so processing can continue to $this->adapter->createCollection($id,
$attributes, $indexes); ensure you only swallow the not-found error and rethrow
other exceptions so genuine failures still surface; reference the
deleteCollection and createCollection calls in the duplicate-recovery block for
the change.
---
Outside diff comments:
In `@src/Database/Database.php`:
- Line 1815: The conditional "if ($created)" is always true because $created is
set to true in both creation branches; remove the dead condition and run the
contained block unconditionally (or refactor so $created is only set when
intended) — locate the $created variable and the conditional around it and
delete the "if ($created)" guard and its matching else/endif so the
success-handling code (originally inside the if) executes directly; ensure no
other logic relies on $created remaining as a guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46a99d4e-a70e-4a1e-8bc0-50c02b474d55
📒 Files selected for processing (1)
src/Database/Database.php
- Wrap deleteCollection in try/catch for NotFoundException in duplicate recovery flow - Remove dead $created condition (always true, flagged by PHPStan) - Skip Sequence validation for $tenant attribute - Use loose comparison for tenant checks to handle type mismatches Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ison getTenant() casts numeric strings to int, but the no-change comparison uses strict equality. This caused false shouldUpdate=true, triggering authorization checks on documents with only read permissions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ison Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/Database/Validator/Sequence.php (1)
33-36: Consider updatinggetType()to reflect mixed type acceptance.
getType()returnsTYPE_STRING, butisValid()now accepts both strings and integers. If external code relies ongetType()to determine expected input types, this could be misleading. Consider either:
- Updating
getType()to returnTYPE_MIXED(if supported by the Validator base class), or- Adding a docblock clarifying that
getType()returns the primary type but integers are also accepted forVAR_INTEGERsequences.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Validator/Sequence.php` around lines 33 - 36, The getType() method currently returns TYPE_STRING while isValid() accepts both strings and integers; update getType() in the Sequence class to accurately reflect that mixed inputs are allowed by either returning TYPE_MIXED (if the Validator base supports it) or, if TYPE_MIXED isn't available, add a clear docblock above getType() stating that although the primary type is TYPE_STRING the validator also accepts integer inputs for VAR_INTEGER sequences and points callers to the isValid() behavior; reference the getType() and isValid() methods and the TYPE_STRING/TYPE_MIXED symbols when making the change.
🤖 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 1795-1805: The DuplicateException recovery currently
unconditionally calls $this->adapter->deleteCollection($id) then
createCollection(...), which can delete a legitimately-created concurrent
collection; instead implement per-collection serialization or a non-destructive
retry: before calling $this->adapter->deleteCollection($id) re-check the
authoritative metadata insert result (the same metadata store write that
originally threw DuplicateException) or acquire a short-lived mutex keyed by $id
so only one creator proceeds to delete/recreate; if the metadata now exists for
$id, skip the physical delete/create and treat the operation as succeeded,
otherwise proceed with deleteCollection($id) and
$this->adapter->createCollection($id, $attributes, $indexes); ensure you still
catch NotFoundException from deleteCollection and avoid deleting when another
writer has already won.
- Around line 6161-6164: The code uses loose comparison ($value != $oldValue) to
set $shouldUpdate which can miss type-only changes; change the check in the
update-detection block to a strict comparison ($value !== $oldValue) so type
differences (e.g., 1 vs "1") mark $shouldUpdate true, preserving proper UPDATE
authorization and updatedAt behavior; update the condition around
$oldValue/$value comparisons (the block setting $shouldUpdate) and ensure
downstream logic that relies on $shouldUpdate (including the calls to
Adapter::updateDocument() and the authorization branches that switch between
UPDATE and READ) continues to use the same $shouldUpdate semantics.
- Around line 1858-1861: The tenant comparisons use loose inequality which can
conflate different IDs (e.g. '01' vs 1); update the guards in the Database class
to use strict inequality (!==) instead of != in the listed methods:
updateCollection(), getCollection(), getSizeOfCollection(),
getSizeOfCollectionOnDisk(), deleteCollection(), and the immutability check in
upsertDocumentsWithIncrease(); if tenant normalization is required do it once at
the boundary (e.g., when setTenant() is called) so all internal comparisons
remain exact and use the tenant value returned by setTenant()/getTenant() for
the strict checks.
---
Nitpick comments:
In `@src/Database/Validator/Sequence.php`:
- Around line 33-36: The getType() method currently returns TYPE_STRING while
isValid() accepts both strings and integers; update getType() in the Sequence
class to accurately reflect that mixed inputs are allowed by either returning
TYPE_MIXED (if the Validator base supports it) or, if TYPE_MIXED isn't
available, add a clear docblock above getType() stating that although the
primary type is TYPE_STRING the validator also accepts integer inputs for
VAR_INTEGER sequences and points callers to the isValid() behavior; reference
the getType() and isValid() methods and the TYPE_STRING/TYPE_MIXED symbols when
making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f901aa7d-c3ed-4471-8d5c-e688974ab455
📒 Files selected for processing (2)
src/Database/Database.phpsrc/Database/Validator/Sequence.php
…heck Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ence Non-primary VAR_ID attributes like $tenant can be any int/string value and should not be forced through UUID7 or Integer range checks. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/Database/Database.php (4)
6062-6073:⚠️ Potential issue | 🔴 CriticalID-field loose compare can misclassify real updates.
Using
!=for ID-typed fields at Line 6173 can treat distinct values as unchanged (e.g.,nullvs0), which can incorrectly follow the READ-authorization path and skip$updatedAtupdates.Suggested fix
$oldValue = $old->getAttribute($key); $isIdType = isset($idAttributes[$key]); - if ($isIdType ? $value != $oldValue : $value !== $oldValue) { + $normalizeId = static fn (mixed $v): mixed => $v === null ? null : (string) $v; + if ($isIdType ? $normalizeId($value) !== $normalizeId($oldValue) : $value !== $oldValue) { $shouldUpdate = true; break; }In PHP 8.x loose comparison, is null != 0 false, and does that differ from strict comparison (!==)?Also applies to: 6172-6174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Database.php` around lines 6062 - 6073, The loose comparison used for ID-typed fields can misclassify changes (e.g., null vs 0) and cause the READ-authorisation path to be taken and skip updating $updatedAt; locate the comparison logic that checks ID attributes (use references Database::VAR_ID and the $idAttributes array in the same class/method) and replace loose inequality checks (!= / ==) with strict comparisons (!== / ===) so null and 0 are treated as distinct values, ensuring the change-detection branch runs and $updatedAt is updated appropriately.
1815-1819:⚠️ Potential issue | 🔴 CriticalRollback cleanup should not run on metadata duplicate races.
If metadata insert at Line 1813 fails with
DuplicateExceptionbecause another creator already succeeded, Line 1816 cleanup can delete the winner’s physical collection. HandleDuplicateExceptionseparately and return the existing metadata document.Suggested fix
- try { - $createdCollection = $this->silent(fn () => $this->createDocument(self::METADATA, $collection)); - } catch (\Throwable $e) { + try { + $createdCollection = $this->silent(fn () => $this->createDocument(self::METADATA, $collection)); + } catch (DuplicateException) { + return $this->silent(fn () => $this->getCollection($id)); + } catch (\Throwable $e) { try { $this->cleanupCollection($id); } catch (\Throwable $e) { Console::error("Failed to rollback collection '{$id}': " . $e->getMessage()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Database.php` around lines 1815 - 1819, The rollback currently always calls $this->cleanupCollection(...) when a metadata insert fails, which can delete a concurrently-created collection; modify the error handling so that if the insert throws a DuplicateException you do NOT call $this->cleanupCollection but instead fetch and return the existing metadata document; otherwise keep the current catch behavior (log and cleanup). Specifically, catch DuplicateException separately (or detect it in the catch block), retrieve the winner metadata (e.g., load the existing document by $id) and return it, and only call cleanupCollection for non-duplicate failures.
1860-1860:⚠️ Potential issue | 🔴 CriticalTenant isolation checks must use strict comparison.
These guards now use
!=, which allows coercion-based equivalence and can bypass tenant mismatch detection. Use!==for exact tenant identity checks in all listed locations.Suggested fix
- && $collection->getTenant() != $this->adapter->getTenant() + && $collection->getTenant() !== $this->adapter->getTenant() - && $collection->getTenant() != $this->adapter->getTenant() + && $collection->getTenant() !== $this->adapter->getTenant() -if ($this->adapter->getSharedTables() && $collection->getTenant() != $this->adapter->getTenant()) { +if ($this->adapter->getSharedTables() && $collection->getTenant() !== $this->adapter->getTenant()) { -if ($this->adapter->getSharedTables() && $collection->getTenant() != $this->adapter->getTenant()) { +if ($this->adapter->getSharedTables() && $collection->getTenant() !== $this->adapter->getTenant()) { -if ($this->adapter->getSharedTables() && $collection->getTenant() != $this->adapter->getTenant()) { +if ($this->adapter->getSharedTables() && $collection->getTenant() !== $this->adapter->getTenant()) { - if (!$old->isEmpty() && $old->getTenant() != $document->getTenant()) { + if (!$old->isEmpty() && $old->getTenant() !== $document->getTenant()) {In PHP 8.x, how do loose comparisons evaluate "01" == 1, "1e2" == 100, and null == 0?Also applies to: 1896-1896, 1951-1951, 1977-1977, 2011-2011, 7214-7214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Database.php` at line 1860, The tenant isolation checks are using loose comparison (!=) which can be bypassed; update every guard that compares $collection->getTenant() and $this->adapter->getTenant() (and any equivalent tenant comparisons in this Database class) to use strict identity (!==) instead of !=; ensure you replace all instances mentioned in the review (the other tenant-check locations) and run unit/integration tests to verify behavior remains correct.
1799-1805:⚠️ Potential issue | 🔴 CriticalNon-destructive reconciliation is still missing before dropping schema.
At Line 1801, this path can still drop a legitimately created concurrent collection because the earlier metadata check is stale. Re-check metadata (or lock by collection ID) before destructive delete/recreate.
Suggested fix
} catch (DuplicateException $e) { + $existing = $this->silent(fn () => $this->getCollection($id)); + if (!$existing->isEmpty()) { + return $existing; + } + try { $this->adapter->deleteCollection($id); } catch (NotFoundException) { // Already removed by a concurrent reconciler. } $this->adapter->createCollection($id, $attributes, $indexes); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Database.php` around lines 1799 - 1805, Before calling $this->adapter->deleteCollection($id) re-check the current collection metadata or acquire a per-collection lock to avoid destroying a concurrently created legit collection: call the adapter's read method (e.g. $this->adapter->getCollection($id) or getCollectionMetadata($id)) and compare its attributes/indexes against the expected $attributes/$indexes and only proceed with delete/create if they still differ; if your codebase has a lock facility use a lock keyed by $id (e.g. $this->lockManager->acquire($id)) around the read/compare/delete/create sequence, preserving the existing NotFoundException catch for the delete call.
🧹 Nitpick comments (1)
tests/unit/Validator/StructureTest.php (1)
776-788: Add one explicit boundary assertion for non-primaryidstrings.This changed block now asserts both numeric and UUID7-style IDs are valid under
Database::VAR_INTEGER. Consider adding one extra assertion that documents whether an arbitrary string ID should pass or fail, so the intended policy is unambiguous.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/Validator/StructureTest.php` around lines 776 - 788, Add one explicit assertion in the failing block that documents the intended behavior for non-primary string IDs: create another $validator->isValid(new Document([...])) case using a non-numeric arbitrary string for the 'id' field (e.g. 'id' => 'random-string') and assert the intended outcome (fail — assertEquals(false, ...) if arbitrary strings should be invalid under Database::VAR_INTEGER); ensure you reference the same $validator, Document and Database::VAR_INTEGER context so the test clearly documents policy for non-primary id strings.
🤖 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/Validator/Sequence.php`:
- Around line 52-54: The early return in Sequence (when $this->primary is false)
short-circuits format validation and lets arbitrary strings pass; remove that
unconditional "if (!$this->primary) return true" behavior in the Sequence
validator and instead apply the same explicit format checks for non-primary
VAR_IDs (e.g., the UUID7 regex check and integer check) or replace the return
with a constrained branch that enforces allowed formats; update the Sequence
class/method where $this->primary is evaluated so non-primary IDs run through
the UUID7/integer validation logic rather than bypassing it.
---
Duplicate comments:
In `@src/Database/Database.php`:
- Around line 6062-6073: The loose comparison used for ID-typed fields can
misclassify changes (e.g., null vs 0) and cause the READ-authorisation path to
be taken and skip updating $updatedAt; locate the comparison logic that checks
ID attributes (use references Database::VAR_ID and the $idAttributes array in
the same class/method) and replace loose inequality checks (!= / ==) with strict
comparisons (!== / ===) so null and 0 are treated as distinct values, ensuring
the change-detection branch runs and $updatedAt is updated appropriately.
- Around line 1815-1819: The rollback currently always calls
$this->cleanupCollection(...) when a metadata insert fails, which can delete a
concurrently-created collection; modify the error handling so that if the insert
throws a DuplicateException you do NOT call $this->cleanupCollection but instead
fetch and return the existing metadata document; otherwise keep the current
catch behavior (log and cleanup). Specifically, catch DuplicateException
separately (or detect it in the catch block), retrieve the winner metadata
(e.g., load the existing document by $id) and return it, and only call
cleanupCollection for non-duplicate failures.
- Line 1860: The tenant isolation checks are using loose comparison (!=) which
can be bypassed; update every guard that compares $collection->getTenant() and
$this->adapter->getTenant() (and any equivalent tenant comparisons in this
Database class) to use strict identity (!==) instead of !=; ensure you replace
all instances mentioned in the review (the other tenant-check locations) and run
unit/integration tests to verify behavior remains correct.
- Around line 1799-1805: Before calling $this->adapter->deleteCollection($id)
re-check the current collection metadata or acquire a per-collection lock to
avoid destroying a concurrently created legit collection: call the adapter's
read method (e.g. $this->adapter->getCollection($id) or
getCollectionMetadata($id)) and compare its attributes/indexes against the
expected $attributes/$indexes and only proceed with delete/create if they still
differ; if your codebase has a lock facility use a lock keyed by $id (e.g.
$this->lockManager->acquire($id)) around the read/compare/delete/create
sequence, preserving the existing NotFoundException catch for the delete call.
---
Nitpick comments:
In `@tests/unit/Validator/StructureTest.php`:
- Around line 776-788: Add one explicit assertion in the failing block that
documents the intended behavior for non-primary string IDs: create another
$validator->isValid(new Document([...])) case using a non-numeric arbitrary
string for the 'id' field (e.g. 'id' => 'random-string') and assert the intended
outcome (fail — assertEquals(false, ...) if arbitrary strings should be invalid
under Database::VAR_INTEGER); ensure you reference the same $validator, Document
and Database::VAR_INTEGER context so the test clearly documents policy for
non-primary id strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8d73fc6a-2564-4199-a8f2-0d3f68a06a0f
📒 Files selected for processing (3)
src/Database/Database.phpsrc/Database/Validator/Sequence.phptests/unit/Validator/StructureTest.php
…instead The loose comparison in the update-detection block could miss type-only changes, allowing writes to persist with only READ authorization checked instead of UPDATE. Normalize the old document's $tenant value so both sides match under strict comparison. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/Database/Database.php (2)
1860-1860:⚠️ Potential issue | 🔴 CriticalRevert tenant guards to strict inequality to preserve tenant isolation semantics.
Using
!=here can conflate distinct tenant identifiers (type-juggling), including the upsert immutability check.Suggested fix
-&& $collection->getTenant() != $this->adapter->getTenant() +&& $collection->getTenant() !== $this->adapter->getTenant()-if (!$old->isEmpty() && $old->getTenant() != $document->getTenant()) { +if (!$old->isEmpty() && $old->getTenant() !== $document->getTenant()) {In PHP 8.x loose comparison rules, do "01" != 1 and null != 0 both evaluate to false due type juggling?Also applies to: 1896-1896, 1951-1951, 1977-1977, 2011-2011, 7208-7208
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Database.php` at line 1860, The tenant comparisons using loose inequality (use of !=) risk type-juggling errors and must be changed to strict inequality (!==) to preserve tenant isolation semantics; update all occurrences where the code compares $collection->getTenant() to $this->adapter->getTenant() (and analogous checks around upsert immutability) to use !== instead of != so distinct tenant identifiers like "01" vs 1 or null vs 0 are not conflated.
1799-1805:⚠️ Potential issue | 🔴 CriticalConcurrent create still has a destructive race in duplicate-recovery.
If two workers create the same collection concurrently, the loser can still drop/recreate schema, then later hit metadata duplicate and run cleanup, leaving metadata present but physical schema removed.
Suggested direction
- try { - $createdCollection = $this->silent(fn () => $this->createDocument(self::METADATA, $collection)); - } catch (\Throwable $e) { + try { + $createdCollection = $this->silent(fn () => $this->createDocument(self::METADATA, $collection)); + } catch (DuplicateException $e) { + // Another creator won metadata write; treat as success path. + return $this->silent(fn () => $this->getCollection($id)); + } catch (\Throwable $e) { try { $this->cleanupCollection($id); } catch (\Throwable $e) { Console::error("Failed to rollback collection '{$id}': " . $e->getMessage()); } throw new DatabaseException("Failed to create collection metadata for '{$id}': " . $e->getMessage(), previous: $e); }Also applies to: 1815-1820
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Database.php` around lines 1799 - 1805, The duplicate-recovery path in Database.php currently unconditionally deletes then recreates the physical collection via $this->adapter->deleteCollection($id) and $this->adapter->createCollection($id, $attributes, $indexes), which allows a destructive race when two workers concurrently create the same collection; change the recovery to be non-destructive by first checking metadata and using an idempotent/create-if-missing flow: after catching the duplicate metadata error, re-read the metadata record for $id and only attempt to create or delete the physical collection if the metadata indicates that the physical schema is absent or needs migration; avoid unconditional delete+create and instead perform create-only when missing (or use an adapter method like ensureCollectionExists or a transactional/lock-backed update) — apply the same fix to the analogous block referenced around lines 1815-1820.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Database/Database.php`:
- Line 1860: The tenant comparisons using loose inequality (use of !=) risk
type-juggling errors and must be changed to strict inequality (!==) to preserve
tenant isolation semantics; update all occurrences where the code compares
$collection->getTenant() to $this->adapter->getTenant() (and analogous checks
around upsert immutability) to use !== instead of != so distinct tenant
identifiers like "01" vs 1 or null vs 0 are not conflated.
- Around line 1799-1805: The duplicate-recovery path in Database.php currently
unconditionally deletes then recreates the physical collection via
$this->adapter->deleteCollection($id) and $this->adapter->createCollection($id,
$attributes, $indexes), which allows a destructive race when two workers
concurrently create the same collection; change the recovery to be
non-destructive by first checking metadata and using an
idempotent/create-if-missing flow: after catching the duplicate metadata error,
re-read the metadata record for $id and only attempt to create or delete the
physical collection if the metadata indicates that the physical schema is absent
or needs migration; avoid unconditional delete+create and instead perform
create-only when missing (or use an adapter method like ensureCollectionExists
or a transactional/lock-backed update) — apply the same fix to the analogous
block referenced around lines 1815-1820.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a1351c2f-e1c6-4708-9b16-376e49e0f68f
📒 Files selected for processing (1)
src/Database/Database.php
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.
Summary by CodeRabbit