Skip to content

Add SQL trigger in getDocument #880

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
abnegate merged 8 commits into from
May 19, 2026
+19 −4
Merged

Add SQL trigger in getDocument #880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
abcea3a
Add trigger getDocument
fogelito May 18, 2026
0414b92
Merge branch 'main' of github.com:utopia-php/database into skip-silen…
fogelito May 18, 2026
4c291dd
(fix): CI — unregister test transformation to avoid polluting later t…
github-actions[bot] May 18, 2026
34c3482
(fix): CI — testTransformations: preserve placeholders in transformed…
github-actions[bot] May 18, 2026
6ce88f4
update tests
fogelito May 18, 2026
e0637e4
Merge remote-tracking branch 'origin/skip-silent-hooks' into skip-sil…
fogelito May 18, 2026
82a602a
update tests
fogelito May 18, 2026
0d95a66
resetMetadata
fogelito May 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/Database/Adapter/SQL.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,8 @@ public function getDocument(Document $collection, string $id, array $queries = [
$sql .= " {$forUpdate}";
}

$sql = $this->trigger(Database::EVENT_DOCUMENT_READ, $sql);

$stmt = $this->getPDO()->prepare($sql);

$stmt->bindValue(':_uid', $id);
Expand Down
4 changes: 2 additions & 2 deletions src/Database/Database.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -732,10 +732,10 @@ public function on(string $event, string $name, ?callable $callback): static
*
* @param string $event
* @param string $name
* @param callable $callback
* @param ?callable $callback
* @return $this
*/
public function before(string $event, string $name, callable $callback): static
public function before(string $event, string $name, ?callable $callback): static
{
$this->adapter->before($event, $name, $callback);

Expand Down
17 changes: 15 additions & 2 deletions tests/e2e/Adapter/Scopes/CollectionTests.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace Tests\E2E\Adapter\Scopes;

use Exception;
use Utopia\Database\Adapter\SQL;
use Utopia\Database\Database;
use Utopia\Database\Document;
use Utopia\Database\Exception as DatabaseException;
Expand Down Expand Up @@ -1670,13 +1671,25 @@ public function testTransformations(): void
'name' => 'value1',
]));

$database->before(Database::EVENT_DOCUMENT_READ, 'test', function (string $query) {
return "SELECT 1";
$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();
Comment on lines +1691 to +1692
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

Suggested change
$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();

Comment on lines +1674 to +1692
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Suggested change
$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.

}

public function testSetGlobalCollection(): void
Expand Down
Loading