Add SQL trigger in getDocument#880
Conversation
📝 WalkthroughWalkthroughgetDocument() now routes generated SELECT SQL through the Database::EVENT_DOCUMENT_READ trigger and applies any returned SQL. Database::before accepts null callbacks. Tests register, capture, assert, and then unregister the document-read listener and reset metadata. ChangesDocument-read event trigger
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
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 |
Greptile SummaryThis PR hooks
Confidence Score: 4/5The SQL and Database changes are safe; the test cleanup leaves a metadata transformation registered on the shared adapter, which will prepend a SQL comment to every query in tests that run afterward. The adapter and Database changes are straightforward and correct. The only real defect is in the test teardown: resetMetadata() clears the metadata array but leaves the EVENT_ALL metadata callback still in place, so any test running after testTransformations on the same adapter instance will have the scope comment silently prepended to its queries. tests/e2e/Adapter/Scopes/CollectionTests.php — the cleanup block needs an extra before(EVENT_ALL, 'metadata', null) call to fully undo the metadata transformation. Important Files Changed
Reviews (5): Last reviewed commit: "resetMetadata" | Re-trigger Greptile |
…ests
testTransformations registers a before() callback that rewrites SQL to
"SELECT 1". Now that SQL.php::getDocument triggers EVENT_DOCUMENT_READ,
this callback persists across subsequent tests in the same class
instance, causing PDO HY093 errors when bindValue(':_uid') is called on
the parameter-less rewritten query. Wrap the assertion in try/finally
and unregister via the adapter's before(name, null) form.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Claude pushed fixes from: healing |
… SQL
The transformation rewrote the prepared SQL to "SELECT 1", but SQL.php's
getDocument still calls bindValue(':_uid', $id) (and ':_tenant' for
sharedTables). PDO throws HY093 because the rewritten query has no
placeholders.
Wrap the original query as a derived table with WHERE 1=0 instead. The
inner query keeps the :_uid / :_tenant placeholders so the bindValue
calls match, while the outer WHERE forces zero rows so isEmpty() still
asserts the trigger was applied.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Claude pushed fixes from: healing |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…ent-hooks # Conflicts: # tests/e2e/Adapter/Scopes/CollectionTests.php
| $database->before(Database::EVENT_DOCUMENT_READ, 'test', null); | ||
| $database->resetMetadata(); |
There was a problem hiding this comment.
resetMetadata() empties $this->metadata but does not remove the before(EVENT_ALL, 'metadata', ...) transformation that setMetadata() registered. The closure captures $output = "/* scope: api.users */ " by value, so after this test every subsequent query in the same adapter instance will still have that comment prepended. This silently pollutes every test that runs afterwards on the same database object.
| $database->before(Database::EVENT_DOCUMENT_READ, 'test', null); | |
| $database->resetMetadata(); | |
| $database->before(Database::EVENT_DOCUMENT_READ, 'test', null); | |
| $database->before(Database::EVENT_ALL, 'metadata', null); | |
| $database->resetMetadata(); |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/Adapter/Scopes/CollectionTests.php`:
- Around line 1674-1692: The test registers a listener via
Database::before('test', Database::EVENT_DOCUMENT_READ) and sets metadata with
Database::setMetadata('scope', 'api.users') but does not guarantee they are
cleaned up if an assertion fails; wrap the invocation of
$database->getDocument('docs', 'doc1') and subsequent assertions in a
try/finally block and move the $database->before(..., null) removal call and
$database->resetMetadata() into the finally so the EVENT_DOCUMENT_READ hook (the
'test' listener) and metadata are always unregistered/reset even on failure.
🪄 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: 193f3449-d9f0-423d-a9e1-6c73ab45f2c1
📒 Files selected for processing (2)
src/Database/Database.phptests/e2e/Adapter/Scopes/CollectionTests.php
| $database->setMetadata('scope', 'api.users'); | ||
|
|
||
| $capturedSql = ''; | ||
| $database->before(Database::EVENT_DOCUMENT_READ, 'test', function (string $sql) use (&$capturedSql) { | ||
| $sql .= ' AND 1=0'; | ||
| $capturedSql = $sql; | ||
| return $sql; | ||
| }); | ||
|
|
||
| $result = $database->getDocument('docs', 'doc1'); | ||
|
|
||
| $this->assertTrue($result->isEmpty()); | ||
|
|
||
| if ($database->getAdapter() instanceof SQL) { | ||
| $this->assertStringContainsString('/* scope: api.users */', $capturedSql); | ||
| } | ||
|
|
||
| $database->before(Database::EVENT_DOCUMENT_READ, 'test', null); | ||
| $database->resetMetadata(); |
There was a problem hiding this comment.
Guard listener + metadata cleanup with finally.
If this test fails before cleanup, the Database::EVENT_DOCUMENT_READ hook and metadata remain active and can pollute later tests. Wrap execution/assertions in try/finally and always unregister/reset in finally.
Proposed fix
$database->setMetadata('scope', 'api.users');
$capturedSql = '';
$database->before(Database::EVENT_DOCUMENT_READ, 'test', function (string $sql) use (&$capturedSql) {
$sql .= ' AND 1=0';
$capturedSql = $sql;
return $sql;
});
- $result = $database->getDocument('docs', 'doc1');
-
- $this->assertTrue($result->isEmpty());
-
- if ($database->getAdapter() instanceof SQL) {
- $this->assertStringContainsString('/* scope: api.users */', $capturedSql);
- }
-
- $database->before(Database::EVENT_DOCUMENT_READ, 'test', null);
- $database->resetMetadata();
+ try {
+ $result = $database->getDocument('docs', 'doc1');
+ $this->assertTrue($result->isEmpty());
+
+ if ($database->getAdapter() instanceof SQL) {
+ $this->assertStringContainsString('/* scope: api.users */', $capturedSql);
+ }
+ } finally {
+ $database->before(Database::EVENT_DOCUMENT_READ, 'test', null);
+ $database->resetMetadata();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $database->setMetadata('scope', 'api.users'); | |
| $capturedSql = ''; | |
| $database->before(Database::EVENT_DOCUMENT_READ, 'test', function (string $sql) use (&$capturedSql) { | |
| $sql .= ' AND 1=0'; | |
| $capturedSql = $sql; | |
| return $sql; | |
| }); | |
| $result = $database->getDocument('docs', 'doc1'); | |
| $this->assertTrue($result->isEmpty()); | |
| if ($database->getAdapter() instanceof SQL) { | |
| $this->assertStringContainsString('/* scope: api.users */', $capturedSql); | |
| } | |
| $database->before(Database::EVENT_DOCUMENT_READ, 'test', null); | |
| $database->resetMetadata(); | |
| $database->setMetadata('scope', 'api.users'); | |
| $capturedSql = ''; | |
| $database->before(Database::EVENT_DOCUMENT_READ, 'test', function (string $sql) use (&$capturedSql) { | |
| $sql .= ' AND 1=0'; | |
| $capturedSql = $sql; | |
| return $sql; | |
| }); | |
| try { | |
| $result = $database->getDocument('docs', 'doc1'); | |
| $this->assertTrue($result->isEmpty()); | |
| if ($database->getAdapter() instanceof SQL) { | |
| $this->assertStringContainsString('/* scope: api.users */', $capturedSql); | |
| } | |
| } finally { | |
| $database->before(Database::EVENT_DOCUMENT_READ, 'test', null); | |
| $database->resetMetadata(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/Adapter/Scopes/CollectionTests.php` around lines 1674 - 1692, The
test registers a listener via Database::before('test',
Database::EVENT_DOCUMENT_READ) and sets metadata with
Database::setMetadata('scope', 'api.users') but does not guarantee they are
cleaned up if an assertion fails; wrap the invocation of
$database->getDocument('docs', 'doc1') and subsequent assertions in a
try/finally block and move the $database->before(..., null) removal call and
$database->resetMetadata() into the finally so the EVENT_DOCUMENT_READ hook (the
'test' listener) and metadata are always unregistered/reset even on failure.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests