Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdded a public getColumnType(...) API to Adapter and SQL. Large rework of Database.php introducing centralized metadata persistence, withRetries, rollback helpers, pre-fetched schema state usage, and extensive recovery/compensation logic for collections, attributes, indexes, and relationships. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
participant Adapter
participant MetadataStore
rect rgba(0,128,0,0.5)
Client->>Database: createCollection / createAttribute / createRelationship
end
rect rgba(0,0,255,0.5)
Database->>Adapter: perform physical schema operation
Adapter-->>Database: success / partial-failure / error
end
rect rgba(255,165,0,0.5)
Database->>MetadataStore: updateMetadata(...) withRetries
MetadataStore-->>Database: success / failure
end
alt Adapter succeeded but metadata failed
Database->>Adapter: attempt rollback/compensation (drop/rename/recreate)
Database->>MetadataStore: retry or reverse metadata changes
else Metadata and Adapter consistent
Database-->>Client: success
else Critical failure
Database-->>Client: error (with best-effort rollback)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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: 4
🤖 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/SQL.php`:
- Around line 1832-1835: The override getColumnType(string $type, int $size,
bool $signed = true, bool $array = false, bool $required = false): string
delegates to getSQLType(...) which can throw DatabaseException for unknown $type
values; update this method's docblock to include `@throws` DatabaseException and
likewise add `@throws` DatabaseException to the parent method's docblock in
Adapter.php (the base API) so callers are aware the exception may propagate
instead of returning an empty string; reference getColumnType, getSQLType, and
DatabaseException when updating the docblocks.
In `@src/Database/Database.php`:
- Around line 3221-3227: The PSR-12 argument spacing in the rollbackOperation
closure call to $this->adapter->createAttribute is invalid; run vendor/bin/pint
to auto-fix and/or manually reformat the multi-argument call so it conforms to
PSR-12 (no extra spaces around parentheses, single space after commas, and
consistent indentation). Locate the rollbackOperation: fn () =>
$this->adapter->createAttribute(...) closure and adjust the argument list for
createAttribute (collection->getId(), $id, $attribute['type'],
$attribute['size'], $attribute['signed'] ?? true, $attribute['array'] ?? false,
$attribute['required'] ?? false) to follow PSR-12 spacing rules or simply run
Pint to apply the correct formatting.
- Around line 4569-4612: The NotFoundException catch path leaves $deleted unset
which can make callers see a null return; update the catch (NotFoundException)
block in the delete-flow (around deleteIndex, $deleted, $shouldRollback and
updateMetadata) to explicitly set $deleted = false (and keep $shouldRollback =
false) so the method returns a deterministic boolean when the schema index is
already missing; adjust the catch block near deleteIndex and indexDeleted
handling accordingly.
- Around line 2275-2362: The batch create path using
$this->adapter->createAttributes can fail with a DuplicateException when any
single column already exists, leaving some requested columns uncreated while
metadata is updated; after catching DuplicateException in the block that calls
createAttributes (referencing $attributesToCreate,
$this->adapter->createAttributes and the surrounding try/catch), implement a
fallback that inspects each attribute in $attributesToCreate (use the adapter to
check/filter existing physical columns — e.g., via getColumnType/filter or an
attributeExists helper) and retry creation per-attribute (call
adapter->createAttributes or adapter->createAttribute for each remaining
attribute) so only truly-duplicate columns are skipped and all missing columns
are created, rethrowing or failing if any non-duplicate create fails.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Database/Adapter.phpsrc/Database/Adapter/SQL.phpsrc/Database/Database.php
Fix PSR-12 method_argument_space violations by putting each argument on its own line in multiline function calls. Fix flaky testSingleDocumentDateOperations by adding a sleep before updating to ensure auto-generated $updatedAt differs from creation timestamp. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/Database/Database.php (2)
4574-4618:⚠️ Potential issue | 🟡 MinorReturn value can be null when index is already absent.
$deletedis never initialized if deleteIndex throws NotFoundException, so this method can return null. Set a deterministic value in that path.✅ Suggested fix
- $shouldRollback = false; + $shouldRollback = false; + $deleted = false; try { $deleted = $this->adapter->deleteIndex($collection->getId(), $id); if (!$deleted) { throw new DatabaseException('Failed to delete index'); } $shouldRollback = true; } catch (NotFoundException) { - // Ignore — index already absent from schema + // Index already absent from schema; treat as deleted + $deleted = true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Database.php` around lines 4574 - 4618, The NotFoundException catch path leaves $deleted uninitialized making the method return null; ensure $deleted is set to a deterministic boolean when deleteIndex throws. In the try/catch around $this->adapter->deleteIndex(...) / catch (NotFoundException) set $deleted = false (or another explicit value) and set $shouldRollback appropriately (keep $shouldRollback false on NotFound), so subsequent logic and return value are always defined; update references to $deleted and $shouldRollback in the surrounding code (including the rollback lambda passed to updateMetadata) to rely on these explicit values.
2237-2364:⚠️ Potential issue | 🟠 MajorBatch createAttributes can still skip non-duplicate columns.
If createAttributes throws a DuplicateException for one existing column, the rest are never created but metadata is still updated. Consider a per-attribute fallback or schema pre-filter to ensure missing columns are created.🛠️ Suggested fallback to per-attribute creation
if (!empty($attributesToCreate)) { try { $created = $this->adapter->createAttributes($collection->getId(), $attributesToCreate); if (!$created) { throw new DatabaseException('Failed to create attributes'); } } catch (DuplicateException) { - // No attributes were in metadata (orphan detection above confirmed this). - // A DuplicateException from the adapter means the column(s) exist only - // in physical schema — suppress and proceed to metadata update. + // Fallback: create individually so non-duplicates still land in schema + foreach ($attributesToCreate as $attr) { + try { + $this->adapter->createAttribute( + $collection->getId(), + $attr['$id'], + $attr['type'], + $attr['size'], + $attr['signed'] ?? true, + $attr['array'] ?? false, + $attr['required'] ?? false + ); + $created = true; + } catch (DuplicateException) { + // Column already exists — skip + } + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Database.php` around lines 2237 - 2364, Batch createAttributes can throw DuplicateException on one column and abort creation of remaining columns while metadata is updated; in the catch block for adapter->createAttributes you should fall back to per-attribute creation to ensure missing physical columns are created: when catching DuplicateException for the bulk create (around adapter->createAttributes and the try/catch), iterate the attributes in $attributesToCreate and call adapter->createAttributes (or a new adapter->createAttribute method / adapter->createAttributes with single-item arrays) per attribute, catching DuplicateException for each and skipping already-existing columns, and ensure adapter->deleteAttribute logic (used earlier) remains in place so type-mismatched orphans are handled before this fallback; update $created or track successes so metadata update only proceeds if all non-duplicate columns were created or appropriately skipped.
🤖 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 3581-3621: The catch for DuplicateException currently assumes the
junction collection fully exists; instead, when DuplicateException is caught for
createCollection($junctionCollection, ...), verify the physical junction schema
actually exists (e.g. via $this->getCollection($junctionCollection) or an
equivalent existence-check) and if it's missing, attempt to create the physical
schema again (call createCollection for $junctionCollection with the same
documents/indices, not just silently skip) or call the adapter-level method to
create the physical collection; update the catch block around DuplicateException
(referencing createCollection, DuplicateException, $junctionCollection, and
silent) to perform this existence-check and recovery so metadata-only duplicates
don't leave the M2M junction absent.
In `@tests/e2e/Adapter/Scopes/DocumentTests.php`:
- Around line 5835-5836: The current \usleep(100000) is too short for
second-granularity timestamp adapters and makes the test flaky; replace the
single usleep call by either a deterministic sleep(1) or, preferably, poll the
resource until $updatedAt differs from $createdAt with a bounded timeout (e.g.,
loop checking the refreshed/returned $updatedAt every 100–250ms up to 3
seconds), failing the test if the timeout is reached; update the test code that
uses $createdAt/$updatedAt and the \usleep invocation to use this
polling/timeout approach so second-precision adapters pass reliably.
---
Duplicate comments:
In `@src/Database/Database.php`:
- Around line 4574-4618: The NotFoundException catch path leaves $deleted
uninitialized making the method return null; ensure $deleted is set to a
deterministic boolean when deleteIndex throws. In the try/catch around
$this->adapter->deleteIndex(...) / catch (NotFoundException) set $deleted =
false (or another explicit value) and set $shouldRollback appropriately (keep
$shouldRollback false on NotFound), so subsequent logic and return value are
always defined; update references to $deleted and $shouldRollback in the
surrounding code (including the rollback lambda passed to updateMetadata) to
rely on these explicit values.
- Around line 2237-2364: Batch createAttributes can throw DuplicateException on
one column and abort creation of remaining columns while metadata is updated; in
the catch block for adapter->createAttributes you should fall back to
per-attribute creation to ensure missing physical columns are created: when
catching DuplicateException for the bulk create (around
adapter->createAttributes and the try/catch), iterate the attributes in
$attributesToCreate and call adapter->createAttributes (or a new
adapter->createAttribute method / adapter->createAttributes with single-item
arrays) per attribute, catching DuplicateException for each and skipping
already-existing columns, and ensure adapter->deleteAttribute logic (used
earlier) remains in place so type-mismatched orphans are handled before this
fallback; update $created or track successes so metadata update only proceeds if
all non-duplicate columns were created or appropriately skipped.
- Add @throws docblock to getColumnType in Adapter and SQL - Add per-attribute fallback when batch createAttributes throws DuplicateException, ensuring non-duplicate columns are still created - Initialize $deleted in deleteIndex and set it to true when NotFoundException is caught, ensuring deterministic return value - Ensure junction physical schema exists when metadata already present during M2M relationship creation - Use sleep(1) instead of usleep(100000) in test for adapters with second-precision timestamps Co-Authored-By: Claude Opus 4.6 <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
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
Bug Fixes
New Features