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:
📝 WalkthroughWalkthroughAdds a new in-memory database adapter Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
abnegate
reviewed
Apr 20, 2026
Contributor
Greptile SummaryThis PR introduces a full in-memory database adapter backed by PHP arrays, with journal-based nested transactions, inverted permission indexes, unique-index hash tables, and the complete query/ordering/cursor surface. The CI matrix and MySQL healthcheck are also improved.
Confidence Score: 3/5Safe to merge for non-production / test-fixture use cases, but the null-handling divergence in negation operators means the Memory adapter does not faithfully replicate SQL adapter semantics — cross-adapter test results will diverge. Score reflects one P1 finding (NOT_EQUAL null inclusion) that adds to the existing set of previously-flagged null-divergence issues in negation operators. The adapter is large and the pattern recurs across multiple query types, increasing the risk of unexpected test divergence between Memory and the SQL adapters. src/Database/Adapter/Memory.php — the Important Files Changed
Reviews (12): Last reviewed commit: "test(memory): tighten regression tests f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 8
🤖 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/Memory.php`:
- Around line 239-248: The updateAttribute implementation must not return early
when $newKey is provided: modify updateAttribute (and its call to
renameAttribute) so the attribute metadata (type/size/signed/array/required) is
applied to the new key after renaming instead of skipping the rest of the
update; additionally, ensure renameAttribute and deleteAttribute both rewrite
any index definitions and uniqueness metadata that reference the old attribute
name to point to the new name (or remove the attribute from indexes on delete)
so indexed queries and unique checks remain consistent — update the logic that
mutates $this->data[$key]['indexes'] and any stored unique maps inside
renameAttribute, deleteAttribute, and updateAttribute to reflect the new
attribute name and/or remove references.
- Around line 1148-1153: documentToRow() JSON-encodes array attribute values but
rowToDocument() only decodes the special _permissions field, so normal array
fields return as JSON strings from getDocument()/find(); update rowToDocument()
to mirror documentToRow() by detecting and json_decode()-ing attribute values
that are JSON-encoded arrays (skip/keep existing handling of _permissions), or
otherwise reverse the encoding logic so attributes encoded in documentToRow()
are decoded in rowToDocument() to restore proper array types for fields returned
by getDocument()/find().
- Around line 338-356: createIndex currently registers Database::INDEX_UNIQUE
without checking existing rows; update createIndex to, when $type ===
Database::INDEX_UNIQUE, iterate the existing rows stored under
$this->data[$key]['rows'], compute the index key for each row using the same
attribute list (and apply $this->filter/$this->collation logic if used
elsewhere), detect any duplicate index keys and throw a DatabaseException if
duplicates exist; only if no duplicates are found, proceed to set
$this->data[$key]['indexes'][$id] and return true. Ensure you reference/create
the same normalization used on writes so enforcement is consistent.
- Around line 28-30: The Memory adapter stores documents keyed only by
$document->getId(), causing shared-table tenants to clobber each other; change
the documents structure to include tenant isolation (e.g.,
$data[...]['documents'][$tenant][$id] or a composite key like "$tenant|$id") and
update all code that writes, reads, updates, deletes, checks existence, and
iterates documents (the sections handling documents and the methods around the
documents/indexes manipulation) to use the tenant-aware key or nested array;
also ensure sequence handling and index maintenance use the same tenant scoping
so tenants cannot overwrite or read each other's records.
- Around line 598-612: The bulk-delete loop removes documents by matching
$seqSet but only cleans permissions when $permissionIds is provided, causing
stale permission rows; update the method that contains the shown loop to always
remove permission entries for documents that were deleted by sequence: after the
foreach that unsets $this->data[$key]['documents'][$uid], collect the deleted
document ids (from $row['_id'] or $uid), build a set (like $deletedSet) and then
filter $this->permissions[$key] to drop any permission where $p['document']
exists in either $deletedSet or the existing $permSet derived from
$permissionIds, ensuring $this->permissions is pruned regardless of whether
$permissionIds was populated.
- Around line 1577-1578: The Memory adapter currently throws DuplicateException
when $rowSignature === $signature, but that should only happen for duplicate
_uid collisions; change the error to throw UniqueException for user-defined
unique-index collisions. Update the branch around $rowSignature === $signature
to inspect the unique index fields (e.g., the index or key definition used to
compute $signature) and: if the index is the _uid index then throw
DuplicateException, otherwise throw UniqueException; use the existing symbols
$rowSignature, $signature, DuplicateException and UniqueException and adjust the
logic where the signature is compared to perform this distinction.
- Around line 79-113: rollbackTransaction() currently pops and restores the most
recent snapshot, but the contract requires rolling back all active transactions;
change rollbackTransaction() to restore the earliest snapshot (the initial state
captured by startTransaction(), e.g. $this->snapshots[0] or
reset($this->snapshots')) instead of the last one, then clear $this->snapshots
and set $this->inTransaction = 0; ensure you still restore both $this->data and
$this->permissions from that snapshot and return true/false as before.
- Around line 501-540: Batch updates in updateDocuments() bypass unique-index
checks because enforceUniqueIndexes() is never invoked; modify updateDocuments()
to call enforceUniqueIndexes() for each document (or once for the whole batch)
before applying writes to $this->data so duplicate unique values are rejected.
Specifically, use the existing $key, $uid, $attrs and $updates context: validate
the proposed attribute changes against enforceUniqueIndexes($key, $uid, $attrs
or computed new row) and abort/throw if enforcement fails, ensuring no partial
writes occur (rollback or validate-before-write) and preserve existing behavior
for createdAt/updatedAt/permissions handling.
🪄 Autofix (Beta)
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
Run ID: babcac22-6b47-43f5-8a95-53fb25d8ea93
📒 Files selected for processing (3)
.github/workflows/tests.ymlsrc/Database/Adapter/Memory.phptests/e2e/Adapter/MemoryTest.php
- rollbackTransaction now decrements inTransaction and pops a single snapshot, matching the SQL adapter and the nested-transaction contract - updateAttribute applies new metadata after a rename instead of returning early and silently dropping type/size/required changes - renameAttribute and deleteAttribute keep index attribute lists in sync so indexed queries and uniqueness checks remain consistent - createIndex(INDEX_UNIQUE) refuses to register when existing rows already contain duplicate values for the indexed columns - enforceUniqueIndexes skips signatures with NULLs so multiple null values do not collide on a unique index - updateDocuments runs enforceUniqueIndexes per row so batch updates cannot introduce duplicate unique values - deleteDocuments always purges permissions for documents removed by sequence, not only when explicit permissionIds are provided - rowToDocument decodes JSON-encoded array attributes so array fields round-trip from getDocument/find - introduce documentKey() and key documents by tenant|id under sharedTables so tenants no longer clobber each other on the same id Adds regression coverage for each fix.
Contributor
CI Healing Report — Flaky (no fix committed)Failing job: Adapter Tests (MySQL) — run 25084920551 Root cause: Infrastructure flake, not a code regression. All 535 errors are the same: Every MySQL E2E test failed at Action: No code fix is appropriate. Recommend re-running the MySQL leg only. — task: healing |
A second pass over the Memory adapter resolves observable behavior
differences from MariaDB so the adapter can serve as a true drop-in.
- find/count/sum/deleteDocument(s)/updateDocuments throw NotFoundException
when the collection itself is missing (mirrors MariaDB's PDO unknown-table
exception). deleteDocument still returns false when only the document
is absent — that path matches rowCount() == 0 on MariaDB.
- createIndex(INDEX_UNIQUE) raises DuplicateException (not the generic
DatabaseException) when existing rows violate the constraint.
Database::createIndex catches that and silently registers the index as
an "orphan" — matching MariaDB+Database's end-state where the metadata
exists but the physical index does not.
- Query::TYPE_SELECT now applies projection: find/getDocument restrict
user attributes to the requested set while preserving internal columns
($id, $sequence, $createdAt, $updatedAt, $permissions, $tenant).
- deleteDocuments now skips rows that don't belong to the current tenant
in shared-tables mode, preventing cross-tenant destruction when
auto-incrementing sequences collide between tenants.
- getSequences uses each document's own tenant when building the lookup
key (mirroring MariaDB's :_tenant_$i bind), instead of always using the
adapter's current tenant. Documents with sequences already set are
skipped to match the SQL contract.
- find sorts by _id ASC by default to mirror MariaDB's clustered-index
ordering, so paginating without an explicit order is stable.
- count(max:0) and sum(max:0) honour zero (LIMIT 0 returns no rows on
MariaDB) instead of treating any non-positive max as "no limit".
- increaseDocumentAttribute returns true on bound violations (silent
no-op) — MariaDB encodes the bound check in WHERE so the UPDATE just
matches zero rows but the call still returns true.
- rowToDocument no longer JSON-sniffs string values that happen to start
with [ or { (which would inadvertently parse user-stored JSON-shaped
strings on read). Array decoding is delegated to the Database layer's
casting step; getSupportForCasting now returns true so arrays
round-trip through Database::casting like the SQL adapters.
- enforceUniqueIndexes type-normalizes signatures (booleans → ints,
numeric strings → numbers, arrays → JSON-encoded) so a doc value of
`true` matches a stored row value of `1`.
- applyOrdering shuffles up-front when ORDER_RANDOM is requested instead
of returning random_int(-1,1) inside usort — that comparator is
non-transitive and breaks the sort.
Each behavior change ships with a regression test in MemoryTest.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the bespoke MemoryTest with one that extends Base, so the Memory adapter is exercised against the same comprehensive scenarios as MariaDB, MySQL, Postgres and friends. Inherits all 13 scope traits (Collection, Document, Attribute, Index, Operator, Permission, Relationship, Spatial, Schemaless, ObjectAttribute, Vector, General, CustomDocumentType); scope tests gated by getSupportFor* flags self-skip for the unimplemented features (relationships, operators, vectors, spatial, fulltext, schemaless, object attributes). Seven inherited tests are explicitly skipped via markTestSkipped where the surrounding trait does not gate on a flag (relationship-specific permission/order tests, upsert-with-JSON, attribute-name-with-dots, attribute structural type-coercion on resize, varchar truncation on shrink, and a no-op keyword test). Memory-specific regressions (transaction nesting, raw store layout after attribute operations, tenant isolation under shared tables, sequence resolution from per-document tenant) stay as direct tests, but route through a `freshDatabase()` helper so they cannot leak collections into the shared `getDatabase()` instance the inherited scopes use. Adapter parity tweaks unblocked by the broader coverage: - createDocument honours the per-call skipDuplicates flag (mirrors MariaDB INSERT IGNORE); - delete(name) only purges the namespace if the database flag was actually set, so a stray drop of a sibling schema (e.g. testEvents' hellodb) no longer wipes METADATA; - increase/decrease bound checks compare the pre-update column value (matching MariaDB's WHERE-clause semantics) instead of the post-update value; - documentKey lower-cases the id, matching MariaDB's default case-insensitive collation; - getDocument honours Query::select projection, dropping unselected user columns while preserving internals; - getMaxIndexLength returns 1024 (Mongo's value) so callers that derive sizes via `getMaxIndexLength() - N` arithmetic don't go negative; - getSupportNonUtfCharacters reports false (Memory does not actively reject non-UTF byte sequences); - TYPE_CONTAINS / TYPE_CONTAINS_ANY against scalar strings use stripos for MariaDB-compatible case-insensitive substring matching; - TYPE_CONTAINS_ANY and TYPE_CONTAINS_ALL are implemented for arrays. 647 tests now run against Memory, of which 25 are skipped (relationships, operators, vectors, spatial, fulltext, regex, schemaless plus the seven Memory-specific gaps); 2447 assertions all pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
CI run 25086657068 — flaky test, no fix committed What failed Why this is flaky The same test has been hardened against this flake before in other sub-cases — see commits Why it isn't this PR's fault
It does not touch Action |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/Database/Adapter/Memory.php (2)
1880-1881:⚠️ Potential issue | 🟠 MajorUser-defined unique-index collisions should throw
UniqueException, notDuplicateException.This branch handles unique-index enforcement, not duplicate
_uidprimary identity collisions. ThrowingDuplicateExceptionhere breaks adapter parity for uniqueness error semantics.Proposed fix
use Utopia\Database\Exception\Duplicate as DuplicateException; use Utopia\Database\Exception\NotFound as NotFoundException; +use Utopia\Database\Exception\Unique as UniqueException; ... if ($rowSignature === $signature) { - throw new DuplicateException('Document with the requested unique attributes already exists'); + $mapped = \array_map(fn (string $a) => $this->mapAttribute($a), $attributes); + $isUidIndex = \count($mapped) === 1 && $mapped[0] === '_uid'; + if ($isUidIndex) { + throw new DuplicateException('Document already exists'); + } + throw new UniqueException('Document with the requested unique attributes already exists'); }Based on learnings: In
src/Database/Adapter/MariaDB.php, only duplicate_uidviolations should throwDuplicateException; other unique constraint violations should throwUniqueException.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/Memory.php` around lines 1880 - 1881, The code currently throws DuplicateException when a user-defined unique-index collision is detected (the branch where $rowSignature === $signature); change this to throw UniqueException instead so uniqueness violations use UniqueException while keeping DuplicateException reserved for primary _uid duplicate cases (mirror the behavior in MariaDB.php); update the branch handling $rowSignature === $signature to throw UniqueException and ensure any callers/tests expecting DuplicateException for non-_uid conflicts are adjusted accordingly.
91-125:⚠️ Potential issue | 🔴 CriticalRollback semantics violate the adapter contract for nested transactions.
rollbackTransaction()currently restores only the latest snapshot and decrements by one. The adapter contract requires rollback to cancel all active transactions and reset depth to 0.Proposed fix
public function startTransaction(): bool { - $this->snapshots[] = [ - 'data' => $this->deepCopy($this->data), - 'permissions' => $this->deepCopy($this->permissions), - ]; + if ($this->inTransaction === 0) { + $this->snapshots[] = [ + 'data' => $this->deepCopy($this->data), + 'permissions' => $this->deepCopy($this->permissions), + ]; + } $this->inTransaction++; return true; } public function commitTransaction(): bool { if ($this->inTransaction === 0) { return false; } - \array_pop($this->snapshots); $this->inTransaction--; + if ($this->inTransaction === 0) { + $this->snapshots = []; + } return true; } public function rollbackTransaction(): bool { if ($this->inTransaction === 0) { return false; } - $snapshot = \array_pop($this->snapshots); + $snapshot = $this->snapshots[0] ?? null; if ($snapshot !== null) { $this->data = $snapshot['data']; $this->permissions = $snapshot['permissions']; } - $this->inTransaction--; + $this->inTransaction = 0; + $this->snapshots = []; return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/Memory.php` around lines 91 - 125, rollbackTransaction() currently only pops the latest snapshot and decrements inTransaction; change it to cancel all active transactions by restoring the initial pre-transaction snapshot, clearing the snapshots stack, and resetting inTransaction to 0. Specifically, in the Memory adapter adjust rollbackTransaction() to: if inTransaction === 0 return false; take the first snapshot from $this->snapshots (restore its 'data' and 'permissions' into $this->data and $this->permissions), clear $this->snapshots (or set to empty array), set $this->inTransaction = 0, and return true; keep startTransaction() and commitTransaction() behavior unchanged except ensure snapshots management remains consistent with nested transactions created by startTransaction().tests/e2e/Adapter/MemoryTest.php (1)
427-430:⚠️ Potential issue | 🟠 MajorUnique-index collision tests should assert
UniqueExceptioninstead ofDuplicateException.These assertions currently lock in the wrong error contract for user-defined unique constraints.
Proposed fix
use Utopia\Database\Exception\Duplicate as DuplicateException; use Utopia\Database\Exception\NotFound as NotFoundException; +use Utopia\Database\Exception\Unique as UniqueException; ... - $this->expectException(DuplicateException::class); + $this->expectException(UniqueException::class); $database->updateDocuments('handles', new Document(['handle' => 'taken']), [ Query::equal('$id', ['h2']), ]); ... - $this->expectException(DuplicateException::class); + $this->expectException(UniqueException::class); $database->createDocument('flags', new Document([ '$id' => 'second', '$permissions' => [Permission::read(Role::any())], 'active' => true, ]));Based on learnings: In
src/Database/Adapter/MariaDB.php, only duplicate_uidviolations should throwDuplicateException; all other unique violations should throwUniqueException.Also applies to: 659-664
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/Adapter/MemoryTest.php` around lines 427 - 430, The test is asserting the wrong exception for user-defined unique-index collisions; replace expectException(DuplicateException::class) with expectException(UniqueException::class) for the updateDocuments(...) cases that trigger unique-index violations (e.g., the call using updateDocuments('handles', new Document(['handle' => 'taken']), [ Query::equal('$id', ['h2']) ]) and the similar case later around the second occurrence) so the tests match the intended contract where only internal _uid duplicate errors throw DuplicateException while all other unique constraint violations throw UniqueException.
🤖 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/Memory.php`:
- Around line 406-426: The unique-index creation loop in createIndex (handling
Database::INDEX_UNIQUE over $this->data[$key]['documents']) builds signatures
from raw row values but must mirror write-time enforcement by normalizing each
attribute exactly as enforceUniqueIndexes does; update the inner loop to call
the same normalizeIndexValue (using $this->normalizeIndexValue(...) or the
equivalent method) on each $row[$this->mapAttribute($attribute)] value
(preserving null handling), then JSON-hash those normalized values to detect
duplicates and throw DuplicateException as before so index creation and runtime
enforcement behave identically.
- Around line 648-698: The current loop in Memory.php updates each document then
calls enforceUniqueIndexes, which allows earlier writes to persist if a later
document violates uniqueness; change this to a two-phase commit: first iterate
$documents and for each build the merged Document (use rowToDocument(),
documentKey(), and new Document(\array_merge(...))), call
enforceUniqueIndexes($key, $merged, $uid) for validation only and collect the
merged rows and permission changes into temporary arrays without mutating
$this->data or $this->permissions; if all validations pass, perform a second
loop that writes the prepared rows into $this->data[$key]['documents'][$docKey],
encodes arrays and permissions, and sets _createdAt/_updatedAt using $updates,
and finally update $this->permissions (use Database::PERMISSIONS and
$updates->getPermissionsByType) — this ensures atomic-like behavior for the
batch.
---
Duplicate comments:
In `@src/Database/Adapter/Memory.php`:
- Around line 1880-1881: The code currently throws DuplicateException when a
user-defined unique-index collision is detected (the branch where $rowSignature
=== $signature); change this to throw UniqueException instead so uniqueness
violations use UniqueException while keeping DuplicateException reserved for
primary _uid duplicate cases (mirror the behavior in MariaDB.php); update the
branch handling $rowSignature === $signature to throw UniqueException and ensure
any callers/tests expecting DuplicateException for non-_uid conflicts are
adjusted accordingly.
- Around line 91-125: rollbackTransaction() currently only pops the latest
snapshot and decrements inTransaction; change it to cancel all active
transactions by restoring the initial pre-transaction snapshot, clearing the
snapshots stack, and resetting inTransaction to 0. Specifically, in the Memory
adapter adjust rollbackTransaction() to: if inTransaction === 0 return false;
take the first snapshot from $this->snapshots (restore its 'data' and
'permissions' into $this->data and $this->permissions), clear $this->snapshots
(or set to empty array), set $this->inTransaction = 0, and return true; keep
startTransaction() and commitTransaction() behavior unchanged except ensure
snapshots management remains consistent with nested transactions created by
startTransaction().
In `@tests/e2e/Adapter/MemoryTest.php`:
- Around line 427-430: The test is asserting the wrong exception for
user-defined unique-index collisions; replace
expectException(DuplicateException::class) with
expectException(UniqueException::class) for the updateDocuments(...) cases that
trigger unique-index violations (e.g., the call using updateDocuments('handles',
new Document(['handle' => 'taken']), [ Query::equal('$id', ['h2']) ]) and the
similar case later around the second occurrence) so the tests match the intended
contract where only internal _uid duplicate errors throw DuplicateException
while all other unique constraint violations throw UniqueException.
🪄 Autofix (Beta)
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
Run ID: 5072368a-4b6b-4e90-aebe-a5a8c4d7cf37
📒 Files selected for processing (2)
src/Database/Adapter/Memory.phptests/e2e/Adapter/MemoryTest.php
Track per-database collection ownership so delete($name) only wipes the collections registered to that schema. Storage keys are prefixed with the current database name so two databases under the same namespace stay isolated, mirroring MariaDB's CREATE DATABASE behaviour. Honour the metadata-collection NULL-tenant fallback under shared tables (parity with SQL.php's `OR _tenant IS NULL` clause), and persist each document's own tenant rather than the adapter's current tenant so tenantPerDocument writes land on the right partition. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Apply Operator instances against the stored row before writing back, so increment/decrement/multiply/divide/modulo/power, string concat/replace, toggle, array append/prepend/insert/remove/unique/intersect/diff/filter, and date add/sub/setNow operators all behave the same as in MariaDB. Numeric overflow is detected via remaining-headroom comparison rather than naive addition — preserves int-ness near PHP_INT_MAX so the Range validator still accepts the result. Skip the upsert+operator scope tests in MemoryTest because Memory does not implement upserts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implement Query::TYPE_SEARCH / TYPE_NOT_SEARCH / TYPE_REGEX in the in-memory matcher: tokenize both haystack and needle on whitespace and punctuation, return true if any needle token is present (matching MariaDB MATCH AGAINST natural-language semantics) or if a quoted phrase appears verbatim. Wildcard suffix (`term*`) collapses to a prefix scan across tokens. Regex queries delegate to preg_match against a PCRE delimiter-wrapped pattern. Drop the throw on `INDEX_FULLTEXT` from createIndex — Memory has no physical index, so the metadata is enough. Flip the corresponding support flags so the inherited fulltext/regex scope tests run instead of self-skipping. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- getSupportForObject() / getSupportForObjectIndexes() now true - Query, order, and cursor attribute references accept dotted paths (foo.bar.baz) and resolve through nested objects on read - Storage keeps PHP arrays in place; resolveAttributeValue decodes legacy JSON-encoded blobs on demand for backwards compatibility
Flip getSupportForRelationships() to true and implement createRelationship, updateRelationship, and deleteRelationship by registering relationship fields on the in-memory attribute list. Reads now surface registered relationship keys as null when unpopulated, matching MariaDB's `ADD COLUMN ... DEFAULT NULL` semantics so the wrapper can populate, traverse, and remove relationships consistently across drivers. updateDocument now applies a sparse merge against the existing row instead of replacing it wholesale, so the wrapper's pattern of removing unchanged relationship attributes before update no longer clobbers stored FKs. resolveAttributeValue tries the fully-filtered column name before falling back to dot-path traversal so relationship keys whose names contain dots (e.g. `\$symbols_coll.ection3`) match the row's flattened column. Cascade and metadata orchestration continue to live in the Database wrapper; the adapter only mutates the underlying row maps. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/Database/Adapter/Memory.php (3)
2583-2584:⚠️ Potential issue | 🟠 MajorThrow
UniqueExceptionfor user-defined unique collisions.Lines 2583-2584 are enforcing a user-defined unique index, not a duplicate
_uid. ReturningDuplicateExceptionhere diverges from the other adapters and also bakes the wrong contract into the new Memory E2E assertions. Based on learnings, only duplicate_uidviolations should throwDuplicateException; all other unique constraint violations should throwUniqueException.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/Memory.php` around lines 2583 - 2584, The code in Memory.php currently throws DuplicateException when a user-defined unique index collides (the check comparing $rowSignature === $signature); change this to throw UniqueException instead so that only duplicate _uid violations use DuplicateException while all other unique constraint conflicts use UniqueException; locate the collision handling around the $rowSignature === $signature comparison and replace the thrown exception class accordingly (keeping or adapting the error message as needed).
952-1007:⚠️ Potential issue | 🟠 MajorValidate the full batch before mutating any rows.
Lines 953-1006 still validate and write one document at a time. If a later document fails
enforceUniqueIndexes(), earlier updates remain applied, which diverges from the statement-level behavior of the SQL adapters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/Memory.php` around lines 952 - 1007, Current code applies updates per-document and calls enforceUniqueIndexes immediately, causing partial commits if a later document fails; change to a two-phase update in the same method: first iterate $documents to compute per-document $resolvedAttrs via $this->applyOperators, build the merged Document (using $this->rowToDocument + $resolvedAttrs + ['$id'=>$uid]) and call $this->enforceUniqueIndexes for every merged candidate without mutating $this->data (collect these computed rows and any permission/timestamp changes into a staging array), and only after all documents pass validation perform a second loop that writes the staged changes into $this->data (setting filtered attributes, _createdAt/_updatedAt, _permissions and updating $this->permissions). Ensure you reference the same symbols ($documents, $attrs, applyOperators, enforceUniqueIndexes, $this->data, $updates, $this->permissions) so the validation happens fully before any mutation.
678-699:⚠️ Potential issue | 🟠 MajorMirror write-time unique enforcement when backfilling a unique index.
Lines 685-693 hash raw row values from every stored row.
enforceUniqueIndexes()normalizes values and scopes shared-table checks per tenant, socreateIndex()can miss already-conflicting rows (truevs1) and can also reject valid duplicates that only exist in other tenants.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/Memory.php` around lines 678 - 699, The duplicate-detection loop in Memory.php currently hashes raw row values, which can mismatch enforceUniqueIndexes() behavior; update the loop inside createIndex() (the block using $this->data[$key]['documents'], $this->mapAttribute, $seen and throwing DuplicateException) to normalize each attribute value the same way enforceUniqueIndexes() does and to include the tenant scope in the signature/hash (i.e., compute signature elements by calling the same normalization helper used by enforceUniqueIndexes() for each mapped attribute and prepend/append the row's tenant identifier when building the hash) so comparisons treat equivalent values (true vs "1") consistently and only detect duplicates within the same tenant.
🤖 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/Memory.php`:
- Around line 2184-2196: The switch branch currently treats
Query::TYPE_CONTAINS, Query::TYPE_CONTAINS_ANY and Query::TYPE_CONTAINS_ALL
identically and returns true on the first matching candidate; change it so that
TYPE_CONTAINS_ANY and TYPE_CONTAINS (if intended as single-value contains) keep
the early-return-on-first-match behavior but Query::TYPE_CONTAINS_ALL uses an
all-match check: when $haystack is null return false, then for TYPE_CONTAINS_ALL
iterate $values and if any candidate is not contained (using
$this->jsonContains($haystack, $this->wrapScalarObjectValue($candidate))) return
false and only return true after the loop; keep existing early-true logic for
TYPE_CONTAINS_ANY (and TYPE_CONTAINS if appropriate) or split the cases into
separate case blocks to implement the differing behaviors.
- Around line 1258-1263: getMaxIndexLength() currently returns 1024 (1 KB) but
the comment and expected cap are 1024 KB (1,048,576 bytes); update the return
value in Memory::getMaxIndexLength() to 1048576 and adjust the inline comment to
state "1024 KB (1,048,576 bytes)" so callers receive the correct byte limit
matching MongoDB's 1024 KB index entry cap.
- Around line 172-182: The exists() method uses $this->key($collection) which
relies on the adapter's current database instead of the $database argument
passed in; update the code so collection existence checks use the provided
$database: modify the key(...) helper to accept an optional $database parameter
(or add a new keyFor(collection, database) helper) and call that from
exists(string $database, ?string $collection) so the lookup uses $database when
checking $this->databases[$database][...]; keep the original behavior when no
$database param is supplied.
---
Duplicate comments:
In `@src/Database/Adapter/Memory.php`:
- Around line 2583-2584: The code in Memory.php currently throws
DuplicateException when a user-defined unique index collides (the check
comparing $rowSignature === $signature); change this to throw UniqueException
instead so that only duplicate _uid violations use DuplicateException while all
other unique constraint conflicts use UniqueException; locate the collision
handling around the $rowSignature === $signature comparison and replace the
thrown exception class accordingly (keeping or adapting the error message as
needed).
- Around line 952-1007: Current code applies updates per-document and calls
enforceUniqueIndexes immediately, causing partial commits if a later document
fails; change to a two-phase update in the same method: first iterate $documents
to compute per-document $resolvedAttrs via $this->applyOperators, build the
merged Document (using $this->rowToDocument + $resolvedAttrs + ['$id'=>$uid])
and call $this->enforceUniqueIndexes for every merged candidate without mutating
$this->data (collect these computed rows and any permission/timestamp changes
into a staging array), and only after all documents pass validation perform a
second loop that writes the staged changes into $this->data (setting filtered
attributes, _createdAt/_updatedAt, _permissions and updating
$this->permissions). Ensure you reference the same symbols ($documents, $attrs,
applyOperators, enforceUniqueIndexes, $this->data, $updates, $this->permissions)
so the validation happens fully before any mutation.
- Around line 678-699: The duplicate-detection loop in Memory.php currently
hashes raw row values, which can mismatch enforceUniqueIndexes() behavior;
update the loop inside createIndex() (the block using
$this->data[$key]['documents'], $this->mapAttribute, $seen and throwing
DuplicateException) to normalize each attribute value the same way
enforceUniqueIndexes() does and to include the tenant scope in the
signature/hash (i.e., compute signature elements by calling the same
normalization helper used by enforceUniqueIndexes() for each mapped attribute
and prepend/append the row's tenant identifier when building the hash) so
comparisons treat equivalent values (true vs "1") consistently and only detect
duplicates within the same tenant.
🪄 Autofix (Beta)
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
Run ID: 9a5a2141-8ce5-4fde-9be4-0c790df75293
📒 Files selected for processing (2)
src/Database/Adapter/Memory.phptests/e2e/Adapter/MemoryTest.php
Exec-form healthcheck did not shell-expand $$MYSQL_USER and $$MYSQL_PASSWORD, so mysqladmin received literal '$MYSQL_USER' as the username. mysqladmin ping reports 'mysqld is alive' on connection errors via stderr while still exiting non-zero with bad credentials, making the check unstable and CI flakes (Adapter Tests SharedTables/MySQL) appear with all tests failing on Connection refused before MySQL was actually ready. Switch to CMD-SHELL so the env vars expand at run time, ping over TCP on the configured port, and use root/MYSQL_ROOT_PASSWORD which is always set.
- exists($database, $collection) now keys lookup by the $database argument and the filtered collection name, so callers can probe a database other than the adapter's currently-bound one. Storage map is now [database][collectionName] => storageKey. - updateDocument / updateDocuments / deleteDocument / deleteDocuments guard permission cleanup with the current tenant under shared tables, so deleting a row in tenant A no longer wipes another tenant's permissions for a document that happens to share the same $id. - updateDocuments validates every row (including unique-index checks) before any write lands, so a uniqueness violation on row N no longer leaves rows 0..N-1 partially committed. - TYPE_CONTAINS_ALL on object attributes now requires every candidate to be present, instead of returning true on the first match (was behaving like CONTAINS_ANY).
PHP's `==` treats `null == 0` and `null == ""` as true, which made applyOrdering and applyCursor silently equate null with 0/empty strings. Switch both call sites to `===` and explicitly place NULLs first under ASC ordering to mirror SQL collation, so cursor boundaries no longer drop rows whose values happen to coerce to the cursor reference.
The two-phase validator only saw each row's pre-update state, so two documents updated to the same unique value in one call both passed phase 1 (each saw the other's old value, no conflict) and silently violated the constraint at phase 2. Phase 1 now also checks every candidate against the other pending merges in the same batch, so sibling collisions are caught before any write lands.
Closes the algorithmic hot spots flagged in the perf audit. Each change moves a hot path from O(dataset) or O(N) to O(touched) or O(1) per write, with the suite runtime dropping ~30% locally (42s → 29s) and assertion count unchanged at 4623. - Transactions: replace `unserialize(serialize($data))` snapshots with a per-transaction mutation journal. Every mutating entry point records its inverse op; `commit` discards the active journal, `rollback` walks it in reverse. Cost becomes O(touched rows) instead of O(dataset) per startTransaction, and nested transactions no longer multiply the copy. - Permissions: maintain `permissionsByDocument` and `permissionsByPermission` inverted indexes alongside the flat list. `applyPermissions` now intersects the user's roles against the permission map (O(roles × matched-buckets)) instead of walking the whole list per query. Per-document deletion in updateDocument / updateDocuments / deleteDocument / deleteDocuments is O(|that doc's perms|) instead of O(P). - Unique indexes: maintain `uniqueIndexHashes` keyed by index + signature. enforceUniqueIndexes is a hash probe, not a full scan; updateDocuments batch path drops the per-row sibling array_filter and detects sibling collisions while building a single pending-sig map (O(M·K) vs O(M·K·(N+K))). - Find pipeline: fuse tenantFilter / applyQueries / applyPermissions into a single pass that allocates one output array. Ordering and cursoring stay separate (they need the full set materialised), but the no-order path skips sorting and trusts insertion order. - Array attributes round-trip natively now — no more JSON encode-on-write / decode-per-access. resolveAttributeValue and decodeArrayValue collapse to pass-through for already-decoded arrays. - applyOrdering precomputes column mapping outside the usort closure and uses a decorate-sort-undecorate transform so the comparator only sees small key tuples. - Memory overrides Adapter::filter() with a per-instance cache so the preg_replace inside the find inner loop runs once per unique raw input, not once per row × per attribute.
The hashed unique-index map keyed signatures by attribute values only, so two tenants storing the same value in a unique-indexed column collided and the second write threw DuplicateException. MariaDB models this as a composite (attr, _tenant) unique index; mirror that contract by prepending the row's tenant to the signature when sharedTables is on. createIndex's initial scan drops its tenant skip — every tenant's rows now contribute their own per-tenant signature.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/MemoryTest.php`:
- Around line 697-738: The test testUniqueIndexNormalizesBoolAndNumericString
currently inserts two boolean true values only; update it so it actually tests
the boolean vs numeric-string normalization path by creating one document with
'active' => true and another that uses a numeric string (e.g., 'active' => "1")
to trigger the coercion/normalization logic in createDocument for the 'flags'
collection, keeping the expectException(DuplicateException::class) assertion;
locate the test method by its name testUniqueIndexNormalizesBoolAndNumericString
and modify the second createDocument call to use the numeric-string fixture so
the unique-index dedupe path is exercised.
- Around line 452-455: The test currently only expects a DuplicateException from
updateDocuments('handles', new Document(['handle' => 'taken']),
[Query::equal('$id', ['h2'])]) but doesn't assert no partial updates occurred;
change the test to snapshot the original rows (fetch the documents for the
affected ids), then call updateDocuments inside a try block and call
$this->fail() if no exception is thrown, catch DuplicateException, and after
catching reload the same documents and assert their attributes match the
original snapshot (no partial application). Apply the same
try/catch+snapshot/assert pattern to the similar case around lines 501-504.
- Around line 685-694: Test currently creates a document with id 'a' in tenant 7
but then probes tenant 1 for id 'a', so a bug in getSequences() that ignores
probe.'$tenant' will still pass; fix by making the tenant-7 fixture use a
different id (e.g., change the Document passed to createDocument in the
setTenant(7) block to have '$id' => 'a7') so that [$probe] = new Document(['$id'
=> 'a', '$tenant' => 1]) truly requires tenant-aware lookup by getSequences(),
or alternatively switch the probe to a tenant with no matching row before
calling getSequences(); update the code around adapter->setTenant(7),
adapter->createDocument(... new Document(...)) and the $probe definition
accordingly.
🪄 Autofix (Beta)
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
Run ID: 9ce7d62f-48a1-4cd7-9f98-4219973cc292
📒 Files selected for processing (2)
src/Database/Adapter/Memory.phptests/e2e/Adapter/MemoryTest.php
The inherited scope test asserts that a document's $updatedAt after an immediate update differs from the initial $updatedAt. The Memory adapter's create+update sequence runs faster than a millisecond, so both timestamps coincide and the assertion fails. Override the test to wait 1.1ms between the two writes; semantics for slower adapters are unchanged.
The class docblock was written when Memory was a CRUD-only stub and relationships, operators, fulltext, regex, schemas, schemaless, and object attributes all threw DatabaseException. All of those are now implemented and exercised by the inherited adapter scope tests; the docblock claimed otherwise. Only spatial types and vector search still throw, since those genuinely need a real engine.
- testBatchUpdateEnforcesUniqueIndexes / testBatchUpdateRejectsSiblingCollision now wrap the call in try/catch and assert the original attribute values are still intact after the failure, so they fail if a partial batch lands. - testGetSequencesUsesDocumentTenant uses different ids per tenant and probes the adapter while it is scoped to the *other* tenant, so the assertion fails if getSequences ignored the document's $tenant. - testUniqueIndexNormalizesBoolAndNumericString split into testUniqueIndexNormalizesBool (the bool path the original test actually exercised) and testUniqueIndexNormalizesNumericString (writes through the adapter directly with string "3" then int 3 so the numeric-string normalisation path is exercised end-to-end).
Comment on lines
+2631
to
+2638
| case Query::TYPE_NOT_EQUAL: | ||
| foreach ($queryValues as $candidate) { | ||
| if ($this->looseEquals($value, $candidate)) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; |
Contributor
There was a problem hiding this comment.
TYPE_NOT_EQUAL includes null-valued rows, diverging from SQL semantics
When a document's attribute value is null and the query is notEqual('field', 'value'), looseEquals(null, 'value') returns false (neither === nor both numeric), so the loop completes and the method returns true — the null row is included. In all SQL adapters, WHERE col != 'value' excludes NULL rows (null comparisons evaluate to NULL, not true). This means queries like Query::notEqual('status', 'active') will return null-status documents on the Memory adapter but not on MariaDB/Postgres, causing test results to diverge between adapters.
abnegate
approved these changes
Apr 29, 2026
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.
Summary by CodeRabbit
New Features
Tests
Chores