Throw Auth in count and Sum methods - Remove order queries too

[fogelito]

Summary by CodeRabbit

• Bug Fixes

  • Enforced document-level security authorization checks during data retrieval operations to prevent unauthorized access when security settings are disabled.

• Removed Features

  • Removed vector-based similarity ordering from query results.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The pull request removes vector-based similarity ordering logic from the SQL adapter's find/count/sum operations and introduces documentSecurity-aware authorization checks to the Database class's find/count/sum methods. Additionally, a test is updated to verify authorization exceptions are thrown for unauthorized access instead of returning empty results.

Changes

Cohort / File(s)	Change Summary
SQL Adapter Vector Handling src/Database/Adapter/SQL.php	Removed collection attribute retrieval and vector-type query extraction from find/count/sum operations; eliminated vector distance-based ORDER BY clause generation and associated SQL bindings
Database Authorization Checks src/Database/Database.php	Added documentSecurity-aware authorization checks to find, count, and sum methods; throws AuthorizationException when user lacks read permissions and documentSecurity is disabled
Permission Test Updates tests/e2e/Adapter/Scopes/PermissionTests.php	Changed test assertion from verifying empty count result to expecting and catching AuthorizationException for unauthorized access

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• Verify that all vector-related code paths have been completely removed from find/count/sum in the SQL adapter without introducing side effects
• Confirm documentSecurity authorization logic is consistently applied across all three Database methods and follows the existing permission check pattern
• Cross-check test expectations align with the new authorization behavior

Possibly related PRs

• Remove spatial attributes injection #719 — Modifies src/Database/Adapter/SQL.php attribute handling logic in the same code areas
• Elevate find sum & count to SQL class #681 — Alters the same SQL adapter methods (find, count, sum) that are modified in this PR
• 1.x #717 — Updates ORDER BY and ordering logic in the SQL adapter alongside this PR's ordering removals

Suggested reviewers

• abnegate
• ArnabChatterjee20k

Poem

🐰 Vectors have hopped right away,
Document security gates hold sway,
Only the rightful may say
"Let me in!" to the array. ✨🔐

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check	✅ Passed	The title mentions 'Throw Auth in count and Sum methods' and 'Remove order queries', which aligns with the actual changes: adding authorization checks to count/sum methods and removing vector-based ordering logic.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch count-with-find

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (4)

tests/e2e/Adapter/Scopes/PermissionTests.php (1)

463-468: Tighten exception expectations for clarity and consistency

Functionally this is fine (it fails if no exception or a non-AuthorizationException is thrown), but it can be simplified and made consistent with nearby tests that use expectException.

You could either use expectException:

-        try {
-            $database->count($collection->getId());
-            $this->fail('Failed to throw exception');
-        } catch (\Throwable $th) {
-            $this->assertInstanceOf(AuthorizationException::class, $th);
-        }
+        $this->expectException(AuthorizationException::class);
+        $database->count($collection->getId());

Or, if you prefer the manual pattern, at least narrow the catch to AuthorizationException instead of \Throwable.

src/Database/Database.php (1)

7965-7970: Auth guard for count/sum now aligned with find/bulk ops

The new documentSecurity-aware check in count() and sum() mirrors the pattern used in find(), updateDocuments(), and deleteDocuments(): if the caller lacks collection-level READ and documentSecurity is disabled (and the collection isn’t _metadata), an AuthorizationException is thrown instead of silently returning an empty aggregate. This looks correct and improves consistency across read and aggregate operations.

To keep this logic from drifting over time, consider extracting this pattern into a small helper (e.g. ensureCollectionAuthorized(string $permission): bool returning $skipAuth), and reuse it in find(), count(), and sum() rather than duplicating the block in multiple methods. This would also let you consistently use getPermissionsByType() everywhere instead of mixing it with getRead().

Also applies to: 8033-8039

src/Database/Adapter/SQL.php (2)

3197-3204: Top‑level vector queries are now ignored in count(); confirm intended semantics

Filtering $queries into $otherQueries by excluding Query::VECTOR_TYPES means any top‑level vector query passed to count() has no effect on the WHERE clause. That’s probably fine (vector queries mainly drive ordering), but it is a silent no‑op.

If the intent is to disallow vector usage in count(), consider either:

• Raising a DatabaseException when a vector query is present, or
• At least documenting that vector queries are ignored for count() to avoid surprises.

Given this is behaviour/API‑level, not correctness‑breaking, leaving it as is is acceptable if it matches your expectations.

---

3279-3284: sum() mirrors count() vector filtering; consider DRY helper and explicit policy

sum() uses the same pattern of excluding Query::VECTOR_TYPES into $otherQueries, so vector queries are likewise ignored when building the aggregation WHERE.

Two small follow‑ups you might consider:

• Extract a tiny helper like filterNonVectorQueries(array $queries): array to avoid duplicating this logic between count() and sum().
• Align and document the policy for vector queries in aggregations (ignored vs. rejected), so callers have clear expectations.

Not blocking, just a clarity/maintainability improvement.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 14250d3 and b5e3d1d.

📒 Files selected for processing (3)

• src/Database/Adapter/SQL.php (2 hunks)
• src/Database/Database.php (2 hunks)
• tests/e2e/Adapter/Scopes/PermissionTests.php (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-10-03T02:04:17.803Z

Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.

Applied to files:

• tests/e2e/Adapter/Scopes/PermissionTests.php

🧬 Code graph analysis (2)

tests/e2e/Adapter/Scopes/PermissionTests.php (4)

src/Database/Adapter/SQL.php (1)

• count (3180-3248)

src/Database/Database.php (1)

• count (7941-7994)

src/Database/Adapter.php (1)

• count (849-849)

src/Database/Adapter/Mongo.php (1)

• count (2095-2182)

src/Database/Adapter/SQL.php (2)

src/Database/Query.php (2)

• getMethod (165-168)
• Query (8-1181)

src/Database/Operator.php (1)

• getMethod (139-142)

⏰ 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). (14)

• GitHub Check: Adapter Tests (SharedTables/MySQL)
• GitHub Check: Adapter Tests (SharedTables/SQLite)
• GitHub Check: Adapter Tests (SharedTables/MariaDB)
• GitHub Check: Adapter Tests (SharedTables/MongoDB)
• GitHub Check: Adapter Tests (Schemaless/MongoDB)
• GitHub Check: Adapter Tests (SharedTables/Postgres)
• GitHub Check: Adapter Tests (Mirror)
• GitHub Check: Adapter Tests (Pool)
• GitHub Check: Adapter Tests (MySQL)
• GitHub Check: Adapter Tests (SQLite)
• GitHub Check: Adapter Tests (MariaDB)
• GitHub Check: Adapter Tests (MongoDB)
• GitHub Check: Adapter Tests (Postgres)
• GitHub Check: Unit Test