Conversation
|
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:
📝 WalkthroughWalkthroughAdded stable callable signature computation and store filter signatures on registration and construction; getCacheKeys now includes enabled filter signatures (with sorted selects and relationships flag) in the per-document payload which is MD5-hashed for the document key; behavior unchanged when no documentId. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Database/Database.php`:
- Around line 9215-9230: The current flattening of $configParts into a
colon-separated string can produce ambiguous collisions (e.g. ['a,b'] vs
['a','b']); replace the ad-hoc separator serialization with an unambiguous
encoder (e.g. json_encode or serialize) when building the config portion added
to $hashParts so each element ($this->filter, $this->resolveRelationships,
sorted \array_keys($this->disabledFilters)) is encoded in a non-ambiguous,
stable form before you run \md5 on \implode('|', $hashParts) to create
$documentHashKey; ensure you keep the existing sort of $this->disabledFilters
and use the same encoder for all entries so $documentHashKey uniquely represents
the config state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 208233e4-443d-4d4c-8897-508e1f40e94f
📒 Files selected for processing (1)
src/Database/Database.php
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Database/Database.php (1)
9211-9230:⚠️ Potential issue | 🟠 MajorSerialize the selects payload structurally too.
$hashParts[] = \implode($selects);is still collision-prone:['ab', 'c']and['a', 'bc']both hash asabc. That means two different projections can still share adocumentHashKeyand return the wrong cached document. Please encode the full payload as structured data instead of concatenated strings.🛠️ Proposed fix
- $hashParts = []; + $hashPayload = []; if (!empty($selects)) { - $hashParts[] = \implode($selects); + $hashPayload['selects'] = \array_values($selects); } // Include non-default config state to prevent cache poisoning when // documents are fetched with filters/relationships disabled if (!$this->filter || !$this->resolveRelationships || !empty($this->disabledFilters)) { - $configParts = [$this->filter ? '1' : '0', $this->resolveRelationships ? '1' : '0']; + $disabled = []; if (!empty($this->disabledFilters)) { - $disabled = \array_keys($this->disabledFilters); + $disabled = \array_keys($this->disabledFilters); \sort($disabled); - $configParts[] = \json_encode($disabled); } - $hashParts[] = \json_encode($configParts); + $hashPayload['config'] = [ + 'filter' => $this->filter, + 'resolveRelationships' => $this->resolveRelationships, + 'disabledFilters' => $disabled, + ]; } - if (!empty($hashParts)) { - $documentHashKey = $documentKey . ':' . \md5(\implode('|', $hashParts)); + if (!empty($hashPayload)) { + $documentHashKey = $documentKey . ':' . \md5(\json_encode($hashPayload)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Database.php` around lines 9211 - 9230, The selects list is currently concatenated with implode which can cause collisions (e.g., ['ab','c'] vs ['a','bc']); in the block that builds $hashParts (the code that currently does $hashParts[] = \implode($selects);), replace that concatenation with a structural, deterministic encoding such as sorting $selects (if order is not significant) and then adding $hashParts[] = \json_encode($selects) (or sort + json_encode to ensure stable ordering) so the documentHashKey (used when building $documentHashKey) uniquely represents the selects payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Database/Database.php`:
- Around line 9211-9230: The selects list is currently concatenated with implode
which can cause collisions (e.g., ['ab','c'] vs ['a','bc']); in the block that
builds $hashParts (the code that currently does $hashParts[] =
\implode($selects);), replace that concatenation with a structural,
deterministic encoding such as sorting $selects (if order is not significant)
and then adding $hashParts[] = \json_encode($selects) (or sort + json_encode to
ensure stable ordering) so the documentHashKey (used when building
$documentHashKey) uniquely represents the selects payload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 925d14cc-1f5c-4fec-bcac-8b1ccda42c4a
📒 Files selected for processing (1)
src/Database/Database.php
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Database/Database.php`:
- Around line 9209-9229: The current key-building logic concatenates fragments
into $hashParts and implodes them which can cause select collisions and leaves
the empty-check for $hashParts pointless; instead build a single structured
payload (include $selects, the resolved relationships flag
$this->resolveRelationships, and the computed $enabled filter list derived from
self::$filters and $this->instanceFilters minus $this->disabledFilters)
JSON-encode that payload and use md5(json_payload) when forming $documentHashKey
from $documentKey, eliminating the fragment implode approach using $hashParts
and the now-dead empty check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dca13bfd-3a36-4def-87da-ca1676181ea4
📒 Files selected for processing (1)
src/Database/Database.php
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Database/Database.php`:
- Around line 9212-9225: The cache payload currently only includes bare filter
names ($enabled) which ignores provenance (instance vs static) used by
decodeAttribute() and can cause collisions when $this->instanceFilters overrides
self::$filters; change the payload construction so "filters" is a mapping of
filter name to a provenance/signature (e.g. mark whether it comes from
$this->instanceFilters or self::$filters and include a stable identifier for
that implementation such as the callable's class::method, function name, or a
hash of the closure/object) instead of a plain list; update the code that builds
$enabled and the json_encode payload to produce that name => signature map so
cache keys reflect the effective filter implementation used by
decodeAttribute().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bc6578ae-d9c7-4204-bdc8-8fd27dc7491a
📒 Files selected for processing (1)
src/Database/Database.php
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Database/Database.php`:
- Around line 474-478: The internal filter arrays now include a 'signature' key
but the PHPDoc shapes were not updated, causing PHPStan failures; update the
docblocks for self::$filters, $this->instanceFilters, the __construct()
parameter $filters, and the getInstanceFilters() return to reflect the new shape
{encode: callable, decode: callable, signature: string} (and likewise update any
other docblocks referencing the old shape such as those near getCacheKeys() and
the alternate filter declarations around the other filter initialization block),
ensuring all annotations use the exact key names and types so references like
$callbacks['signature'] are type-checked correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a0d4a608-dfe8-439f-97d1-3090ca90ce82
📒 Files selected for processing (1)
src/Database/Database.php
| foreach ($filters as $name => $callbacks) { | ||
| $filters[$name]['signature'] = self::computeCallableSignature($callbacks['encode']) | ||
| . ':' . self::computeCallableSignature($callbacks['decode']); | ||
| } | ||
| $this->instanceFilters = $filters; |
There was a problem hiding this comment.
Update the internal filter array-shapes to include signature.
These assignments change both filter stores from {encode, decode} to {encode, decode, signature}, but the class/property/param/return annotations still declare the old shape. That is why PHPStan is already failing on Lines 9228 and 9235 when getCacheKeys() reads $callbacks['signature']. Please extend the shapes for self::$filters, $this->instanceFilters, __construct()'s $filters, and getInstanceFilters().
Also applies to: 8612-8618
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/Database.php` around lines 474 - 478, The internal filter arrays
now include a 'signature' key but the PHPDoc shapes were not updated, causing
PHPStan failures; update the docblocks for self::$filters,
$this->instanceFilters, the __construct() parameter $filters, and the
getInstanceFilters() return to reflect the new shape {encode: callable, decode:
callable, signature: string} (and likewise update any other docblocks
referencing the old shape such as those near getCacheKeys() and the alternate
filter declarations around the other filter initialization block), ensuring all
annotations use the exact key names and types so references like
$callbacks['signature'] are type-checked correctly.
1 participant
Summary by CodeRabbit