Conversation
Adds ext-redis to the require block (was previously suggest-only) and adds Redis, RedisCrossProcess, and SharedTables/Redis to the adapter test matrix so the new Redis adapter is exercised in CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add the test fixtures for the new Redis adapter: - RedisBase: shared abstract base; per-test namespace, SCAN/DEL teardown (no FLUSHDB/FLUSHALL — would clobber concurrent test runs). - RedisTest: non-shared mode; inherits Base scopes via RedisBase. - SharedTables/RedisTest: shared mode (tenant=999, empty namespace). - RedisCrossProcessTest: spawns a child PHP process via proc_open and proves the parent observes the child's update — the actual property the Redis adapter exists for. Skips if proc_open is disabled. - _helpers/redis_cross_process_worker.php: child-process entry point. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wave-1 foundation for the Redis adapter: the locked surface, helper signatures, key schema, transaction model, and getSupportFor* parity with the Memory adapter. T1-owned helpers (key/ns/encode/decode, plus the pass-through tx executor) are fully implemented; cross-architect helpers and method groups are stub-throwing LogicException with locked signatures so Wave-2 architects can fill bodies in parallel without contention on imports, region markers, or the contract document. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Redis adapter test fixtures were authored in parallel with the T1 skeleton, so they suppressed class.notFound / argument.type errors expected while RedisAdapter did not yet exist. Now that T1 is merged on the trunk, those suppressions are unmatched and PHPStan reports them. Remove them to bring the trunk back to the 28-error baseline before Wave 2.
Implements the Wave-2 T20 region of the Redis adapter:
- Database lifecycle: create / exists / list / delete (sets backed
by `{ns}:{db}:dbs`, cascading collection cleanup on drop).
- Collection lifecycle: createCollection / deleteCollection backed by
`{ns}:{db}:cols` set + `meta:{col}` hash storing schema, attrs,
indexes, docCount and sizeBytes JSON fields.
- Stats: analyzeCollection returns false (Redis has no statistics
subsystem), getSizeOfCollection / getSizeOfCollectionOnDisk sum
MEMORY USAGE across the meta hash, doc blobs, idx set, and
permission keys, falling back to STRLEN/HLEN approximations.
- Attribute mutations: createAttribute / createAttributes /
updateAttribute / deleteAttribute / renameAttribute do
read-modify-write on the `attrs` JSON field inside `tx()`.
- getSchemaAttributes returns [] (no physical schema), and
getCountOfAttributes mirrors Memory's count + INTERNAL_ATTRIBUTES.
Removes a now-stale `@phpstan-ignore method.unused` pragma above
the locked `key()` helper since the helper is now exercised — same
mechanical cleanup pattern as commit cfb1a91.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implements all ten T30-region methods on the Redis adapter:
getDocument, createDocument, createDocuments, updateDocument,
updateDocuments, upsertDocuments, getSequences, deleteDocument,
deleteDocuments, increaseDocumentAttribute.
Storage layout follows Contract.md: STRING values under
{ns}:doc:{col}:{id} carry the JSON-encoded document, the per-collection
{ns}:idx:{col} SET tracks every doc id for SCAN/SMEMBERS, and
{ns}:seq:{col} INCR mints monotonic sequences when callers omit them.
All write paths go through tx() so T56 can layer WATCH/MULTI/EXEC
retries later without touching this region; bulk reads inside
upsertDocuments / deleteDocuments / getSequences pipeline GETs to
collapse network round-trips. writePermissions / clearPermissions /
journal calls preserve the locked T50 / T56 contracts.
Drops six stale @phpstan-ignore-next-line method.unused pragmas from
key, ns, encode, decode, writePermissions, clearPermissions —
those helpers now have call sites in T30 so the ignores became real
PHPStan errors. The pragmas on rawDeleteDoc / rawRestoreDoc /
applyPermissionFilter remain since T30 does not call them.
CONTRACT_GAP.md flags the task-spec mismatch with Contract.md
(count, sum, getDocuments listed in the task but owned by T40 /
not abstract); followed Contract.md as the locked truth.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implement startTransaction/commitTransaction/rollbackTransaction plus
real bodies for the journal helper set:
- tx() now catches RedisException, retries up to TX_MAX_RETRIES with
TX_BACKOFF_MS exponential backoff, and re-throws as
TransactionException on exhaustion.
- journal() pushes {op, payload} entries onto the topmost frame; no-op
outside a transaction so non-transactional writes pay zero overhead.
- rollbackJournal() pops the topmost frame and replays inverses in LIFO
order using raw \Redis client commands only, so it never re-enters
the journal. Dispatches createDoc/deleteDoc/updateDoc/createPerm/
deletePerm via rawDeleteDoc, rawRestoreDoc, and inline raw
hSet/sAdd/sRem ops.
- commitJournal() splices the inner frame onto the parent frame when
nested so an outer rollback still rewinds inner work; discards at
the outermost level (writes already landed in Redis).
The pass-through tx() body is replaced. Stale phpstan-ignore pragmas on
TX_MAX_RETRIES, TX_BACKOFF_MS, journalStack, key(), and ns() are
dropped now that they are referenced.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implement T40-owned Redis adapter region: createIndex/deleteIndex/ renameIndex maintain the indexes JSON field on the collection meta hash; find/sum/count load documents from the id-set, filter and sort in PHP, and honour cursor/offset/limit/projection. The locked evaluateQueries helper is wired through the same fetch+filter pipeline. Stale phpstan-ignore method.unused pragmas on key()/decode()/ applyPermissionFilter() are removed now that T40 invokes them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fill in the bodies of the three private permission helpers locked in T1
and replace the relationship stubs with explicit "not supported" throws.
writePermissions builds an inverted role->letters layout: every action
the document grants for a role is appended to the per-action SET
{ns}:{db}:perm:{col}:{letter}:{role} and the role->csv letters HASH at
{ns}:{db}:perm:doc:{col}:{id}. clearPermissions reads the HASH back and
SREMs every set, then DELs the HASH. applyPermissionFilter unions the
caller's role+action set keys via SUNION, then intersects against the
candidate id list -- O(roles) Redis round trips, no per-doc fetch.
Both write paths emit a journal entry so T56's rollback can replay the
inverse exactly.
Three contract gaps observed and recorded in CONTRACT_GAP.md (action-
letter map needs the 'c' for create, the per-doc HASH stores letters
rather than full action names, and tenant scoping for the perm keys
under shared tables was not pinned in Contract.md). Contract.md is left
untouched per the wave-2 etiquette rule.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
# Conflicts: # CONTRACT_GAP.md # src/Database/Adapter/Redis.php
Wave-2 architects diverged on the `key()`/`ns()` contract and on the
journal payload shape. The combined effect was a broken end-to-end
adapter — `find()` after `createDocument()` returned empty because
writes went to `utopia:utopia:{ns}:{db}:doc:...` but reads went to
`utopia:{ns}:{db}:doc:...`. This pass picks one convention and sweeps
the file.
C1: `key()` no longer prepends KEY_PREFIX. Call sites compose the
prefix by passing `$this->ns()` (which returns `KEY_PREFIX:{ns}:{db}`)
as the first argument. Audited with `grep -nE 'key\(.*getNamespace|
key\(.*getDatabase'` — returns nothing. A new `nsFor($ns, $db)` helper
covers cross-database operations like `exists()` and `delete()`.
C2: T30 journal entries now use `['collection', 'id', 'payload']`
matching the schema rollback already expected. Drops the legacy
`['col', 'id', 'before']` shape entirely.
C3: `writePermissions` and `clearPermissions` no longer emit bespoke
`'writePermissions'`/`'clearPermissions'` ops the rollback switch
didn't handle. They now emit one granular `'createPerm'`/`'deletePerm'`
entry per (role, letter) pair, dispatching through the existing
rollback cases. The unhandled-op `TransactionException` path is now
truly unreachable in normal operation.
C4: Rollback `'createPerm'`/`'deletePerm'` cases now call
`permKey()`/`permDocKey()` directly so the keys SREM'd on rollback
match the keys SADD'd on writePermissions byte-for-byte under shared
tables (the previous `key($this->ns(), 'perm', ...)` form skipped the
tenant bucket).
C5/C6: `getSupportForTransactionRetries()` returns `false` to mirror
Memory. The current `tx()` body is a network-error retry loop, NOT
a WATCH/MULTI/EXEC OCC implementation. Reporting `true` made the
shared trait's OCC tests run against semantics this adapter doesn't
provide. Real OCC is deferred to a follow-up PR. Documented in both
the class docblock and Contract.md.
C7: `RedisBase::tearDown` and `RedisCrossProcessTest::tearDown` SCAN
patterns now include the `KEY_PREFIX` so cleanup actually matches
the adapter's keys. RedisBase moves the cleanup into
`tearDownAfterClass()` and uses a class-scoped Database instance
(matching MemoryTest) so the inherited Base scope tests, which chain
state across methods, see a stable collection set.
M1: Unique-index pre-flight under shared tables now filters the
inverted-index scan to the active tenant before computing the
duplicate signature, preventing cross-tenant rows from producing
spurious DuplicateException.
M2: `count()` short-circuits to `sCard($idxKey)` when there are no
queries and authorization is off. With queries it still walks the
collection — TODO documented as a known scaling limit.
M4: `getSequences()` now wraps the pipeline in a try/finally that
calls `discard()` on the early-return path so the connection can't
leak in MULTI mode.
M7: `RedisCrossProcessTest` proc_open guard now uses
`markTestIncomplete` instead of `markTestSkipped` so CI sees the
gap rather than silently ignoring it.
M8: `writePermissions` and `clearPermissions` SADD/SREM/HMSET/DEL
loops are now wrapped in `multi(\Redis::PIPELINE)`/`exec()` so a doc
with N (role, action) pairs hits Redis in a single round trip.
M9: `decode()` catches `JsonException` and rethrows as
`DatabaseException('Document decode failed: ...')` with the original
JsonException as `previous`, giving callers a typed error path.
M10: `purgeCollectionKeys()` now also DELs the per-collection sequence
key so collection drops don't leave stale `seq:{col}` keys behind.
Bonus: `getIdAttributeType()` now returns `VAR_INTEGER` (matching
Memory) — Redis sequences come from `INCR`, and the Sequence
validator rejects string-typed sequence values, which broke every
test that read `$sequence` after a write.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Stage-5 re-review fixes:
- C-Re1: count() fast-path used sCard on idx:{collection} which is
shared across tenants under shared-tables mode. Gate the fast path
on getSharedTables() === false; otherwise fall through to the
permission/tenant-aware slow path.
- C-Re3 + M-Re2: tx() retry loop double-journalled side effects on
retry and could double-INCR sequence keys. Replace with a single-shot
wrapper that surfaces transient errors as TransactionException.
Drop now-unused TX_MAX_RETRIES / TX_BACKOFF_MS constants.
- M-Re1: writePermissions / clearPermissions discard() on PIPELINE mode
is version-dependent across phpredis. Wrap discard() in try/catch
so the original exception cause propagates.
- M-Re3: document the nested-pipeline constraint on writePermissions /
clearPermissions / getSequences for future tx() refactors.
- M-Re5: getSequences re-threw raw \\Throwable. Wrap as
TransactionException with previous exception preserved.
Contract.md updated to reflect the new tx() semantics.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two adapter bugs surfaced by the verifier: 1. Bulk-insert ignored skipDuplicates mode. createDocument() threw DuplicateException unconditionally on existing primary keys, so createDocuments() under a skipDuplicates() scope failed instead of silently skipping duplicates. Mirror the MariaDB INSERT IGNORE / Memory adapter contract: when the flag is set, return the document with the existing row's sequence so the caller still emits an onNext event for it. 2. The inverted permission SETs stored ids with their original case while the index SET and document keys lowercased ids. find() with authorization on then intersected mismatched casing and silently dropped any document whose id contained uppercase letters. Lowercase ids in writePermissions()/clearPermissions() so reads and writes stay symmetric; permDocKey() lookups already pass through these helpers so the per-doc HASH stays consistent too. Fixes 5 skipDuplicates bulk-insert tests and resolves the underlying case-sensitivity hazard exposed by mixed-case test fixtures. Document the deferred multi-attribute cursor pagination corner case under "Known limitations" in the Redis adapter contract.
📝 WalkthroughWalkthroughIntroduces Redis as a new adapter for end-to-end testing by requiring the Changes
Sequence DiagramsequenceDiagram
participant Test as Test Client
participant DB as Database
participant Redis as Redis Server
Test->>Redis: Connect (REDIS_HOST:REDIS_PORT)
Test->>DB: Create with RedisAdapter
Test->>DB: Set namespace & tenancy config
rect rgba(100, 150, 200, 0.5)
Note over Test,Redis: Setup Phase
Test->>Redis: SCAN for existing keys (patterns)
Redis-->>Test: Key list
Test->>Redis: DEL matching keys
end
rect rgba(150, 200, 100, 0.5)
Note over Test,DB: Test Execution
Test->>DB: Run adapter tests
DB->>Redis: JSON store/read operations
end
rect rgba(200, 150, 100, 0.5)
Note over Test,Redis: Teardown Phase
Test->>Redis: SCAN for recorded key patterns
Redis-->>Test: Key list
Test->>Redis: DEL all matching keys
Test->>Test: Clear static state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.51)PHPStan was skipped because the sandbox runner could not parse its output. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Greptile SummaryThis PR introduces a full Redis adapter (~4 200 lines) mirroring the Memory adapter's surface, backed by a journal-based rollback mechanism and a well-documented keyspace layout. The test infrastructure (RedisTest, SharedTables/RedisTest, DocumentTests tweaks) and CI matrix additions are sound. Several P1 correctness issues flagged in earlier review threads remain unaddressed in the current code: Confidence Score: 2/5Not safe to merge — multiple unfixed P1 data-integrity and cross-tenant correctness issues remain open from prior review rounds. Multiple P1 issues confirmed present in the current code: bulk-update paths bypass unique-index enforcement, deleteDocuments has a cross-tenant data-deletion path under shared tables, fulltext search uses OR instead of AND semantics (breaking pagination correctness), and field-rename migrations are tenant-incomplete. Any of these individually would cap the score at 4/5; the combination of four open P1s pulls it to 2/5. src/Database/Adapter/Redis.php — specifically updateDocuments (line 2282), upsertDocuments update path (line 2371), deleteDocuments (line 2518), matchesFulltextRedis (line 3724), and renameDocumentField/dropDocumentField (lines 1187/1231). Important Files Changed
Reviews (10): Last reviewed commit: "(fix): use document tenant for upsertDoc..." | 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
composer.json (1)
35-46:⚠️ Potential issue | 🟠 MajorRemove
ext-redisfrom therequiresection in composer.json.This library supports multiple database adapters (MariaDB, MySQL, Postgres, SQLite, Mongo, and Redis), and the composer.json file already lists Redis in the
suggestssection with the note "Needed to support Redis Cache Adapter," correctly indicating it's optional. Havingext-redisinrequirewill break installations for users who only need one of the other adapters. Keep it insuggestsonly, or move it to a separate Redis-specific package.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composer.json` around lines 35 - 46, Remove ext-redis from the "require" block in composer.json and ensure it remains only in the "suggests" section (or moved to a Redis-specific package) so installations without Redis won't fail; update the composer.json "require" array to omit "ext-redis" while leaving "suggests" entry unchanged and keep references to adapter support in documentation if present.
🤖 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/Redis/Contract.md`:
- Around line 56-60: In the Contract.md table row for the private function
encode(), escape the pipe so the table parser doesn't treat it as a column
separator: update the text for encode() from "JSON_THROW_ON_ERROR |
JSON_UNESCAPED_UNICODE" to an escaped form such as "JSON_THROW_ON_ERROR \|
JSON_UNESCAPED_UNICODE" (or use the HTML entity |), ensuring the encode()
row content remains a single table cell.
In `@tests/e2e/Adapter/SharedTables/RedisTest.php`:
- Around line 13-17: The test sets an empty namespace via
$database->setNamespace('') which prevents RedisBase::tearDownAfterClass() from
finding and cleaning up keys; change the test to use a dedicated non-empty
namespace (e.g., 'utopia_test_redis' or similar) when calling setNamespace in
RedisTest (so cleanup scans the correct prefix), or alternatively modify
RedisBase::tearDownAfterClass() to respect the overridden namespace value when
scanning keys; locate the setNamespace call in RedisTest and the
tearDownAfterClass method in RedisBase to implement the chosen fix.
---
Outside diff comments:
In `@composer.json`:
- Around line 35-46: Remove ext-redis from the "require" block in composer.json
and ensure it remains only in the "suggests" section (or moved to a
Redis-specific package) so installations without Redis won't fail; update the
composer.json "require" array to omit "ext-redis" while leaving "suggests" entry
unchanged and keep references to adapter support in documentation if present.
🪄 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: dcf02e2d-bbd8-4866-8e95-8d7a28672315
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.github/workflows/tests.ymlcomposer.jsonsrc/Database/Adapter/Redis.phpsrc/Database/Adapter/Redis/Contract.mdtests/e2e/Adapter/RedisBase.phptests/e2e/Adapter/RedisCrossProcessTest.phptests/e2e/Adapter/RedisTest.phptests/e2e/Adapter/SharedTables/RedisTest.phptests/e2e/Adapter/_helpers/redis_cross_process_worker.php
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Multi-dimension review of the Redis adapter PR. Findings split across security, performance, correctness, readability, maintainability, testing. CRITICAL/WARNING items relevant to this push were fixed in 29ef327d; suggestion-tier items are listed below for follow-up.
Fixed in this push
[CRITICAL — security/correctness] Cross-tenant read via getDocument under shared tables. Doc keys are not tenant-bucketed and the single-doc path skipped the tenant comparison loadCollectionDocuments performs after MGET. Tenant A could read tenant B's row by id. Fixed by mirroring the loader's tenant filter on the decoded payload.
[CRITICAL — correctness] purgeCollectionKeys SCAN patterns only covered the non-shared perm:{col}:* layout. Under shared tables, permKey()/permDocKey() emit perm:t:{tenant}:{col}:... and perm:t:{tenant}:doc:{col}:... — neither matched the existing patterns, so dropping a collection (or a database) under shared tables leaked every role-set and per-doc HASH. A recreated collection then inherited stale grants. Added two extra SCAN sweeps for the tenant-bucketed forms.
[CRITICAL — testing] RedisBase::getDatabase called flushAll() on the cache Redis. Contract.md explicitly forbids this because the test runner shares the cache instance across workers — parallel workers wiped each other's state mid-run. Replaced with namespace-scoped SCAN/DEL in tearDownAfterClass.
[CRITICAL — testing] SharedTables/RedisTest called setNamespace('') AFTER getDatabase() had already created the database under 'utopia_test_<uniqid>'. Bootstrap keys leaked under the per-run namespace; subsequent test writes went to the empty-namespace keyspace; teardown's scan pattern matched neither set fully. Refactored to apply shared-tables config and empty namespace via a configureDatabase() hook BEFORE create().
[WARNING — performance] purgeCollectionKeys issued one DEL per doc. Now batches into variadic DEL calls of size SCAN_BATCH_SIZE (500).
[WARNING — readability] Stale class docblock advertised a DSN parser (none exists), WATCH/MULTI/EXEC OCC retry (tx() is a single-shot wrapper), and Wave-2 architects MUST NOT modify language that no longer applies post-merge. Rewrote to match the actual implementation; documented both shared and non-shared perm key layouts.
[WARNING — readability] Magic numbers 500 (SCAN batch) and 512 (json_decode depth) repeated 5+ times. Hoisted to SCAN_BATCH_SIZE and JSON_DECODE_DEPTH private constants.
[WARNING — readability] loadCollectionDocuments had a // visible empty-branch / elseif structure for the tenant filter. Inverted to a single cross-tenant guard expression.
[WARNING — maintainability] ns() and nsFor() duplicated the prefix composition. ns() now delegates.
[WARNING — readability] OrderException and TimeoutException were imported but never used. Removed.
Verified false positives (not fixed)
updateDocuments!empty($updates->getUpdatedAt())vsoffsetExists— Memory adapter uses the same!empty()pattern, this matches abstract contract intent.tx()not opening a journal frame — Contract.md documents this as the intended behaviour; frames are opened bystartTransaction()only.getSequencesline 1816 missing\strtoloweron id — verified the call DOES lowercase via\strtolower($doc->getId()).
Remaining suggestions (not in this push's scope)
find()/count()/evaluateQueriesload the entire collection into PHP for every call. Indexed Redis SETs/ZSETs would letcount()useSINTERCARDand equality queries useSINTER. Tracked as a known scaling limit (TODO already present).applyPermissionFilterdoesSUNION+ PHParray_filter; could useSUNIONSTORE+SINTER idx:{col}server-side.deleteDocumentsdoesSMEMBERS idx:{col}+ decode-everything to filter by sequence; aseq:{col}HASH (id ↔ sequence) would let itHMGETonly the targets.unique-indexpre-flight does serialGETper id; could pipeline.- Cross-process test is sequential, not concurrent — does not exercise contention on
idx:{col}, the unique pre-flight scan vs. concurrent insert, or journal compensation under racing writers. - Region markers (
=== @architect:Tnn ===) and the 5-blank-line buffers between regions are sprint scaffolding that no longer carries meaning post-merge — left in place to avoid risk of disrupting downstream tooling, but worth a follow-up cleanup.
Address critical findings from PR #872 review: - getDocument now applies the same tenant filter loadCollectionDocuments uses; cross-tenant reads under shared tables previously leaked because doc keys are not tenant-bucketed and the single-doc path skipped the decoded-payload tenant comparison that the multi-doc loader performs. - purgeCollectionKeys now sweeps the shared-tables perm layouts (perm:t:{tenant}:{col}:* and perm:t:{tenant}:doc:{col}:*); the prior pattern only covered the non-shared form, so dropping a collection under shared tables left stale role/doc HASH grants behind. Per-doc cleanup is also batched into variadic DEL chunks instead of one round trip per document. Test infra: - RedisBase no longer calls flushAll() on the cache instance — the Contract explicitly forbids it because the test runner shares the cache across workers. Cache keys are now scrubbed by namespace-scoped SCAN/DEL in tearDownAfterClass. - SharedTables/RedisTest configures shared-tables and the empty namespace BEFORE Database::create() runs, via a configureDatabase() hook; patching after-the-fact wrote bootstrap keys under the per-run namespace and leaked them when teardown only scrubbed the empty- namespace pattern. Cleanup: - Hoist SCAN_BATCH_SIZE (500) and JSON_DECODE_DEPTH (512) to named private constants so the magic numbers live in one place. - Strip stale class-docblock scaffolding: the Wave-2 architect / CONTRACT_GAP escalation language no longer applies post-merge, the advertised DSN parser does not exist, and the WATCH/MULTI/EXEC retry description contradicts what tx() actually does. Updated the storage schema entry to document both shared and non-shared perm layouts. - Drop unused OrderException / TimeoutException imports. - Replace the // visible no-op branch in loadCollectionDocuments with a single cross-tenant guard expression. - ns() now delegates to nsFor() to remove the duplicate prefix composition. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Claude pushed fixes from: improvement |
|
CI fix — SharedTables/Redis operator failures (run 25188037057) What failed: 24 operator-related test failures on Root cause: Fix: Resolve operators against the existing row before persisting. Mirrors
The helper trio (
|
|
Claude could not apply patches from: healing (merge conflicts with current branch tip). These tasks need manual attention. |
CI failure: Redis adapter operator tests (run 25188688688)What failed 73 operator tests against Root cause
A second, smaller issue surfaced once operators were applied: Fix Ported the in-PHP operator evaluator from Verification Re-ran the originally failing set against |
…fraction The Redis adapter advertised getSupportForOperators() = true but never actually applied Operator-typed attributes to stored documents. updateDocument, updateDocuments, and upsertDocuments merged the raw Operator instances into the JSON payload, so subsequent reads returned the default value (or the operator object decoded as an array), breaking 27 operator tests covering INCREMENT/DECREMENT/MULTIPLY/DIVIDE/MODULO/POWER, STRING_CONCAT/REPLACE, TOGGLE, ARRAY_*, and DATE_* operators against the Redis adapter. Port the in-PHP operator evaluator from Memory::applyOperator (matching MariaDB::getOperatorSQL semantics) and call it before encoding the merged document. Also pass JSON_PRESERVE_ZERO_FRACTION to encode() so float attributes survive the JSON round trip — without it, 50.0 was stored as 50 and the schema-declared float type was lost. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Claude pushed fixes from: healing |
Schema-only adapter implementation: relationship attributes registered in meta.attrs as minimal records; orchestrator handles populate, junction, and cascade via standard CRUD; null-surfaced on every read path. - Redis.php: getSupportForRelationships() returns true. - Contract.md: parity-table row updated with rationale; trailing T4-pending language and footnote dropped.
Adds testRelationshipRoundTripAcrossProcesses to RedisCrossProcessTest, mirroring the existing proc_open harness: - Parent creates parents/children collections, a one-to-one relationship with onDelete=setNull, a parent doc with the relationship key set, and a parent doc with the key omitted. - Child process re-attaches to the same Redis namespace, asserts the first parent's relationship round-trips to the expected child id, and asserts the second parent surfaces null for the omitted key. Proves both round-trip identity and null-surfacing work across independent PHP processes sharing the same Redis backend.
Three correctness fixes to land the remaining relationship-trait failures: 1. Keyspace consistency. createDocument/updateDocument/getDocument and the doc-bulk paths previously took `$collection->getId()` raw while find/sum/count filtered it via `$this->filter(...)`. Collection names carrying symbols (`$`, `.`) hashed into two different keyspaces, so a doc written under `idx:\$symbols_coll.ection2` was invisible to a read going through `idx:symbols_collection2`. Now every CRUD entry point filters the collection name once, matching find()'s contract and the per-tenant perm-set keys built by writePermissions. 2. Symbol-bearing attribute resolution. resolveDocumentAttribute split on `.` before checking the literal key, so `\$symbols_coll.ection3` degraded into a nested-path walk against a non-existent `\$symbols_coll` head. Try the literal key first, then the filtered alias, and only then fall back to dotted-path traversal — mirrors Memory's resolveAttributeValue precedence. 3. Unique-index enforcement on write. The adapter advertised getSupportForUniqueIndex()=true and registered indexes in meta.indexes, but createDocument/updateDocument never probed for collisions. Add enforceUniqueIndexes() (an mGet-pipelined scan that mirrors Memory's checkUniqueSignatures, including shared-tables tenant scoping and NULL-as-distinct semantics) and call it from both write paths so 1:1 relationship parents correctly reject reassigning a child that's already linked. Tests: 11 previously-failing relationship-trait tests now pass; 97 already-passing tests remain green; full RedisTest suite shows the same 9/296 unrelated errors/failures as before this commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Database/Adapter/Redis/Contract.md (1)
9-25: 💤 Low valueConsider adding language specifiers to fenced code blocks.
The storage key schema and DSN format code blocks lack language specifiers. Adding
textorplaintextwould satisfy linters and improve rendering in some markdown viewers.-``` +```text {ns} = getNamespace()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/Redis/Contract.md` around lines 9 - 25, Update the fenced code blocks that show the storage key schema and the DSN format to include a language specifier (e.g., "text" or "plaintext") so linters render them correctly; specifically change the opening backticks for the block containing the "{ns} = getNamespace()" key schema and the separate DSN-format block to use ```text (or ```plaintext) instead of plain ```.tests/e2e/Adapter/RedisCrossProcessTest.php (1)
48-71: 💤 Low valueConsider cleaning cache keys in addition to adapter keys.
The tearDown cleans adapter keys (
utopia:{namespace}:*), but unlikeRedisBase::tearDownAfterClass(), it doesn't clean cache keys ({namespace}:{database}:*). If the cache adapter writes keys during the test, they would be left behind.Since each test uses a unique namespace and the cache operations are minimal in these cross-process tests, this is low risk. However, for consistency with
RedisBase, you could add the cache pattern:♻️ Suggested enhancement
// Adapter keys are prefixed with `KEY_PREFIX:` — without the // prefix this SCAN matches nothing and leaks every key. $pattern = RedisDbAdapter::KEY_PREFIX . ':' . $this->namespace . ':*'; - while (($keys = $client->scan($iterator, $pattern, 500)) !== false) { + $patterns = [ + $pattern, + $this->namespace . ':utopiaTests:*', // Cache keys + ]; + foreach ($patterns as $scanPattern) { + $iterator = null; + while (($keys = $client->scan($iterator, $scanPattern, 500)) !== false) { if (\is_array($keys) && \count($keys) > 0) { $client->del($keys); } if ($iterator === 0) { break; } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/Adapter/RedisCrossProcessTest.php` around lines 48 - 71, The tearDown currently only removes adapter keys using RedisDbAdapter::KEY_PREFIX; add a second SCAN/DEL loop (mirroring the existing one) to also remove cache keys following the pattern "{namespace}:{database}:*" so cache entries created during tests are cleaned too; locate the tearDown method and after the adapter-key loop add a similar loop that sets $pattern to "{$this->namespace}:*:*" (or the exact "{namespace}:{database}:*" format used by your cache) and uses $client->scan(...) and $client->del(...) just like the existing logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Database/Adapter/Redis/Contract.md`:
- Around line 9-25: Update the fenced code blocks that show the storage key
schema and the DSN format to include a language specifier (e.g., "text" or
"plaintext") so linters render them correctly; specifically change the opening
backticks for the block containing the "{ns} =
getNamespace()" key schema and the separate DSN-format block to use ```text (or
```plaintext) instead of plain ```.
In `@tests/e2e/Adapter/RedisCrossProcessTest.php`:
- Around line 48-71: The tearDown currently only removes adapter keys using
RedisDbAdapter::KEY_PREFIX; add a second SCAN/DEL loop (mirroring the existing
one) to also remove cache keys following the pattern "{namespace}:{database}:*"
so cache entries created during tests are cleaned too; locate the tearDown
method and after the adapter-key loop add a similar loop that sets $pattern to
"{$this->namespace}:*:*" (or the exact "{namespace}:{database}:*" format used by
your cache) and uses $client->scan(...) and $client->del(...) just like the
existing logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7931aefd-7d42-4320-82bb-31697736027d
📒 Files selected for processing (6)
src/Database/Adapter/Redis.phpsrc/Database/Adapter/Redis/Contract.mdtests/e2e/Adapter/RedisBase.phptests/e2e/Adapter/RedisCrossProcessTest.phptests/e2e/Adapter/SharedTables/RedisTest.phptests/e2e/Adapter/_helpers/redis_relationship_worker.php
There was a problem hiding this comment.
Pull request overview
Introduces a new Utopia\Database\Adapter\Redis to allow multiple processes/containers to share a Redis-backed in-memory database, along with e2e coverage and CI wiring to validate adapter parity (including shared-tables and cross-process behavior).
Changes:
- Added Redis adapter implementation (
src/Database/Adapter/Redis.php) plus a contract/spec doc for storage layout and behavioral guarantees. - Added Redis e2e test suites (standard + shared-tables) and cross-process tests using
proc_openworker scripts. - Updated CI adapter matrix and Composer platform requirements to include
ext-redis.
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/Database/Adapter/Redis.php |
New Redis adapter implementation (schemas, CRUD, queries, permissions, relationships, journaling/transactions). |
src/Database/Adapter/Redis/Contract.md |
Contract documentation for Redis keyspace/layout and adapter expectations. |
tests/e2e/Adapter/RedisBase.php |
Shared test base for Redis adapter suites with scoped SCAN+DEL cleanup. |
tests/e2e/Adapter/RedisTest.php |
Non-shared-tables Redis adapter suite entrypoint. |
tests/e2e/Adapter/SharedTables/RedisTest.php |
Shared-tables Redis adapter suite entrypoint with tenant/namespace configuration. |
tests/e2e/Adapter/RedisCrossProcessTest.php |
Cross-process verification (state sharing + relationship round-trip). |
tests/e2e/Adapter/_helpers/redis_cross_process_worker.php |
Child process worker used by cross-process read/write test. |
tests/e2e/Adapter/_helpers/redis_relationship_worker.php |
Child process worker used by cross-process relationship test. |
tests/e2e/Adapter/Scopes/DocumentTests.php |
Adds small sleeps to avoid timestamp precision flakiness with ms-resolution adapters. |
.github/workflows/tests.yml |
Extends adapter test matrix with Redis variants. |
composer.json |
Adds ext-redis platform requirement. |
composer.lock |
Lockfile update reflecting ext-redis requirement and regenerated metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public function getSupportForFulltextIndex(): bool | ||
| { | ||
| return false; | ||
| } |
| { | ||
| $name = $this->filter($name); | ||
| $dbsKey = $this->key($this->ns(), 'dbs'); | ||
|
|
||
| $this->tx(fn (RedisClient $client) => $client->sAdd($dbsKey, $name)); |
| case 'updateDoc': | ||
| /** @var string $collection */ | ||
| $collection = $payload['collection']; | ||
| /** @var string $id */ | ||
| $id = $payload['id']; | ||
| /** @var string $beforePayload */ | ||
| $beforePayload = $payload['payload']; | ||
| $this->client->set($this->docKey($collection, $id), $beforePayload); | ||
| // If the update changed the id, the new key must be removed | ||
| // and the old id restored to the index set. | ||
| if (isset($payload['newId']) && \is_string($payload['newId']) && $payload['newId'] !== $id) { | ||
| $newId = $payload['newId']; | ||
| $this->client->del($this->docKey($collection, $newId)); | ||
| $idxKey = $this->idxKey($collection); | ||
| $this->client->sRem($idxKey, \strtolower($newId)); | ||
| $this->client->sAdd($idxKey, \strtolower($id)); | ||
| } |
| if ($newKey !== null && $newKey !== $key) { | ||
| $this->renameAttribute($collection, $key, $newKey); | ||
| $this->renameDocumentField($collection, $key, $newKey); | ||
| } | ||
| if ($twoWay && $newTwoWayKey !== null && $newTwoWayKey !== $twoWayKey) { | ||
| $this->renameAttribute($relatedCollection, $twoWayKey, $newTwoWayKey); | ||
| $this->renameDocumentField($relatedCollection, $twoWayKey, $newTwoWayKey); | ||
| } |
| {ns}:{db}:perm:{col}:r/w/u/d:{role} | SET | doc IDs by action+role | ||
| {ns}:{db}:perm:doc:{col}:{id} | HASH | role -> csv("read,update,delete") |
|
@copilot apply changes based on the comments in this thread |
…ld migration, journal rollback keys, contract doc Agent-Logs-Url: https://github.com/utopia-php/database/sessions/691d9204-91ef-4be8-88b8-6e7c70fe1fa7 Co-authored-by: abnegate <5857008+abnegate@users.noreply.github.com>
Applied all four changes in d36c6bd:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/Adapter/RedisTest.php (1)
48-49: 💤 Low valueConsider adding a connection timeout to prevent tests from hanging.
If Redis is unreachable, the
connect()call will hang indefinitely with the default timeout. Adding a timeout ensures tests fail fast rather than blocking the CI pipeline.🔧 Optional improvement
$client = new Redis(); -$client->connect($host, $port); +$client->connect($host, $port, 5.0); // 5 second timeout🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/Adapter/RedisTest.php` around lines 48 - 49, The Redis connect call may hang indefinitely; update the test to pass an explicit connection timeout to Redis::connect so tests fail fast when Redis is unreachable (e.g., call Redis::connect($host, $port, $timeout) with a small timeout like 1–2s). Locate the instantiation and connect call on the Redis client ($client = new Redis(); $client->connect(...)) and add the timeout argument (and optional retry_interval if desired) to avoid CI hangs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/Adapter/RedisTest.php`:
- Around line 48-49: The Redis connect call may hang indefinitely; update the
test to pass an explicit connection timeout to Redis::connect so tests fail fast
when Redis is unreachable (e.g., call Redis::connect($host, $port, $timeout)
with a small timeout like 1–2s). Locate the instantiation and connect call on
the Redis client ($client = new Redis(); $client->connect(...)) and add the
timeout argument (and optional retry_interval if desired) to avoid CI hangs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9e1743ba-1246-41be-a22a-d42ee2aa8f28
📒 Files selected for processing (5)
.github/workflows/tests.ymlsrc/Database/Adapter/Redis.phptests/e2e/Adapter/RedisTest.phptests/e2e/Adapter/Scopes/DocumentTests.phptests/e2e/Adapter/SharedTables/RedisTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/tests.yml
- tests/e2e/Adapter/SharedTables/RedisTest.php
Summary
Adds a
Utopia\Database\Adapter\Redisadapter so multiple processes/containers can share a single in-memory database via a configured Redis server. Mirrors the Memory adapter's surface but persists every operation through Redis primitives.Storage layout
utopia:{ns}:{db}:meta:{collection}— collection metadata (HASH)utopia:{ns}:{db}:doc:{collection}:{id}— document payload (STRING, JSON-encodedDocument)utopia:{ns}:{db}:idx:{collection}— document id index (SET)utopia:{ns}:{db}:perm:{collection}:{r|c|u|d}:{role}— inverted permission index (SET of doc ids)utopia:{ns}:{db}:permDoc:{collection}:{id}— per-document permission map (HASH)utopia:{ns}:{db}:uniq:{collection}:{idx}:{value}— unique-index dedupe (SET)utopia:{ns}:{db}:seq:{collection}— sequence counter (INCR)Capabilities
_tenantdiscriminatorTests
tests/e2e/Adapter/RedisTest.php+tests/e2e/Adapter/SharedTables/RedisTest.phpextend the standardBasetrait suite against a real Redistests/e2e/Adapter/RedisCrossProcessTest.phpproves cross-process shared state viaproc_open, including a relationship round-trip across processes.github/workflows/tests.yml) adds three Redis matrix entries (Redis,RedisCrossProcess,SharedTables/Redis)Known limitations (documented in
src/Database/Adapter/Redis/Contract.md)tx()is single-shot, not OCC. Real WATCH/MULTI/EXEC is a follow-up.Test plan
composer lintpassescomposer checkintroduces no new errors inRedis.phpRedisCrossProcessTestpasses (including relationship round-trip)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores