0.71.x #662
0.71.x #662
Conversation
WalkthroughEnables array index support in MySQL adapter, introduces selective filter skipping via an updated skipFilters API with per-filter control, adjusts error types for invalid increase/decrease values, refines min/max propagation, and updates an e2e test to assert specific Limit exceptions. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant DB as Database
participant Adapter
Client->>DB: skipFilters(callback, filters?)
activate DB
DB->>DB: Save current filter state
DB->>DB: Disable all or specified filters
DB->>DB: Execute callback()
DB-->>Client: callback result
DB->>DB: Restore previous filter state
deactivate DB
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
1045-1069: Tighten assertions by catching the exact type and verifying the error messageCatching Exception and then asserting type works, but a typed catch is stricter and easier to read. Also assert a stable part of the message to ensure the correct limit is surfaced.
Apply this diff:
try { $database->decreaseDocumentAttribute( 'increase_decrease', $document->getId(), 'decrease', 10, 99 ); $this->fail('Failed to throw exception'); - } catch (Exception $e) { - $this->assertInstanceOf(LimitException::class, $e); + } catch (LimitException $e) { + $this->assertStringContainsString('Attribute value exceeds minimum limit', $e->getMessage()); + $this->assertStringContainsString('99', $e->getMessage()); } try { $database->decreaseDocumentAttribute( 'increase_decrease', $document->getId(), 'decrease', 1000, 0 ); $this->fail('Failed to throw exception'); - } catch (Exception $e) { - $this->assertInstanceOf(LimitException::class, $e); + } catch (LimitException $e) { + $this->assertStringContainsString('Attribute value exceeds minimum limit', $e->getMessage()); + $this->assertStringContainsString('0', $e->getMessage()); }src/Database/Database.php (3)
347-351: Normalize disabledFilters typing and simplify the checkCurrently, disabledFilters is declared as nullable but never set to null, while decodeAttribute guards for null before checking the map. Align the type with usage and simplify the check.
Apply these diffs:
- /** - * @var array<string, bool>|null - */ - protected ?array $disabledFilters = []; + /** + * @var array<string, bool> + */ + protected array $disabledFilters = [];- if (!\is_null($this->disabledFilters) && isset($this->disabledFilters[$filter])) { - return $value; - } + if (isset($this->disabledFilters[$filter])) { + return $value; + }Also applies to: 6594-6597
817-821: Clarify skipFilters docblock to reflect selective filter skippingThe method now supports per-filter disabling, but the description still implies global skipping only. Consider clarifying that:
- Passing null disables all filters (previous behavior).
- Passing a list disables only those named filters.
- The per-filter control applies to decoding (encode path still applies all filters).
Example docblock text:
/** * Skip filters * * Execute a callback with filters disabled. * - If $filters is null, all filters are disabled (global). * - If $filters is a list of filter names, only those filters are disabled (during decoding). * * @template T * @param callable(): T $callback * @param array<string>|null $filters List of filters to disable; null disables all filters * @return T */
5257-5258: Exception type change: update docs and consumers to use InvalidArgumentExceptionSame note as increase: add @throws \InvalidArgumentException to the docblock and verify downstream handlers.
I can help search for affected call sites (see the script in the previous comment).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Database/Adapter/MySQL.php(1 hunks)src/Database/Database.php(7 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/e2e/Adapter/Scopes/DocumentTests.php (2)
src/Database/Database.php (2)
Database(35-6825)decreaseDocumentAttribute(5249-5332)src/Database/Exception.php (1)
Exception(7-21)
src/Database/Database.php (3)
src/Database/Adapter.php (1)
filter(1112-1121)src/Database/Document.php (1)
getAttribute(212-219)src/Database/Query.php (1)
getAttribute(120-123)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup & Build Docker Image
🔇 Additional comments (3)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
14-14: LGTM: Specific exception alias improves clarityImporting Limit as LimitException makes the intent explicit and keeps assertions readable.
src/Database/Database.php (2)
5204-5206: LGTM: Strict null-check on max and early guardThe stricter null-check and early LimitException provide clearer semantics and reduce adapter churn. Passing max to the adapter as a guard is a good atomicity hook.
5303-5305: LGTM: Strict null-check on min and early guardThe stricter null-check and early LimitException for decreases mirrors the increase path and looks correct.
Summary by CodeRabbit