Fix: public methods

[lohanidamodar]

Fix: public methods

• Fix ordering in database adapter
• Fix batch logging
• Fix return types
• Keep it in sync with the current version of the library

Summary by CodeRabbit

• Breaking Changes

  • Batch logging now returns a boolean success indicator instead of an array of created log records.

• Improvements

  • Query ordering for user/resource audit retrievals refined.
  • Adapter configuration/schema helper methods made more accessible to improve extensibility.

• Tests

  • Audit tests updated to assert boolean success for batch operations rather than expecting returned record arrays.

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

[coderabbitai]

Walkthrough

The pull request changes the audit batch API to return a boolean success flag instead of an array of created Log objects. The abstract Adapter::createBatch signature and implementations (ClickHouse, Database) now return bool. Audit::logBatch likewise returns bool. Database adapter now maps input logs to Document instances and calls createDocuments without returning per-log results; several Query::orderAsc/Desc calls were changed to omit the 'time' field. The SQL adapter raises visibility of five helper methods from protected to public. Tests were updated to assert truthiness of batch calls instead of examining returned arrays.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• src/Audit/Adapter/Database.php: verify removal of Log construction in createBatch, ensure callers no longer rely on returned Log array, and confirm behavior after removing explicit 'time' from Query::orderAsc()/orderDesc().
• src/Audit/Adapter/ClickHouse.php: confirm empty-input early return now true and that removed Log construction doesn't break consumers.
• src/Audit/Adapter/SQL.php: check rationale and impact of changing five methods from protected to public (API surface change).
• src/Audit/Adapter/Adapter.php and src/Audit/Audit.php: confirm public API change (createBatch/logBatch signatures) is intentional and reconciled with callers and tests.
• tests/*: ensure tests fully cover changed return type expectations and any downstream code paths that previously used returned Log objects.

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Fix: public methods' is vague and does not clearly summarize the main changes. While the PR addresses multiple fixes (ordering, batch logging, return types), the title only mentions 'public methods' which is overly broad and doesn't convey the specific nature of the changes.	Consider a more descriptive title that captures the primary changes, such as 'Fix: Update createBatch return type to bool and make SQL methods public' or break down into specific areas addressed.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 87.50% which is sufficient. The required threshold is 80.00%.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ 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 feat-db-3.x

---

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: 1

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bac717c and 5254d12.

⛔ Files ignored due to path filters (1)

• composer.lock is excluded by !**/*.lock

📒 Files selected for processing (5)

• src/Audit/Adapter.php (1 hunks)
• src/Audit/Adapter/ClickHouse.php (2 hunks)
• src/Audit/Adapter/Database.php (5 hunks)
• src/Audit/Adapter/SQL.php (5 hunks)
• src/Audit/Audit.php (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (3)

src/Audit/Adapter/ClickHouse.php (2)

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

• createBatch (87-103)

src/Audit/Adapter.php (1)

• createBatch (62-62)

src/Audit/Adapter/Database.php (4)

src/Audit/Adapter.php (1)

• createBatch (62-62)

src/Audit/Adapter/ClickHouse.php (1)

• createBatch (503-583)

src/Audit/Audit.php (1)

• log (61-72)

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

• getCollectionName (24-27)

src/Audit/Adapter.php (2)

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

• createBatch (87-103)

src/Audit/Adapter/ClickHouse.php (1)

• createBatch (503-583)

🪛 GitHub Actions: CodeQL

src/Audit/Adapter/Database.php

[error] 87-87: PHPStan: @return tag indicates 'array<Utopia\Audit\Log>' but function return type is bool. This is incompatible (return type mismatch in PHPDoc).

🔇 Additional comments (9)

src/Audit/Adapter.php (1)

58-62: LGTM! Breaking API change to simplify batch logging.

The method signature change from returning array<Log> to bool simplifies the batch creation API by returning a success indicator instead of created log entries. This aligns with the PR objectives to fix batch logging.

src/Audit/Adapter/ClickHouse.php (1)

503-582: LGTM! Batch creation simplified to return boolean status.

The implementation correctly aligns with the updated interface, returning true after successful batch insertion instead of constructing and returning Log objects. The early return for empty logs is appropriate.

src/Audit/Audit.php (1)

78-84: LGTM! Public API updated consistently with adapter changes.

The logBatch method signature correctly updated to return bool, maintaining simple delegation to the adapter's createBatch method.

src/Audit/Adapter/Database.php (5)

87-103: LGTM! Batch creation simplified with consistent boolean return.

The refactored implementation correctly transforms logs into Document instances via array_map and creates them in a single batch operation, returning true to indicate success.

---

210-210: Verify that removing the 'time' field parameter maintains correct ordering.

Same concern as Line 154 - ensure Query::orderAsc() without the field parameter maintains time-based ordering.

---

271-271: Verify that removing the 'time' field parameter maintains correct ordering.

Same concern as Line 154 - ensure Query::orderAsc() without the field parameter maintains time-based ordering.

---

335-335: Verify that removing the 'time' field parameter maintains correct ordering.

Same concern as Line 154 - ensure Query::orderAsc() without the field parameter maintains time-based ordering.

---

154-154: The Utopia Database Query builder's orderAsc() and orderDesc() methods implicitly default to the time field when used without parameters. This is verified by the test suite, which confirms that ordering produces correct chronological results (newest first with orderDesc, oldest first with orderAsc). The deliberate removal of the explicit 'time' parameter in commit 4f77e21 was a fix, not a regression, as the framework handles field selection automatically in this context.

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

24-24: LGTM! Visibility broadened to expose metadata accessors.

Five utility methods (getCollectionName, getAttributes, getAttributeDocuments, getIndexes, getIndexDocuments) changed from protected to public, allowing external access to audit log metadata. This aligns with the PR title "Fix: public methods" and enables consistent access across adapters.

Also applies to: 43-43, 127-127, 142-142, 173-173

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5254d12 and 1c7ff3a.

📒 Files selected for processing (3)

• src/Audit/Adapter/Database.php (5 hunks)
• tests/Audit/Adapter/ClickHouseTest.php (1 hunks)
• tests/Audit/AuditBase.php (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• src/Audit/Adapter/Database.php

🧰 Additional context used

🪛 GitHub Actions: Tests

tests/Audit/AuditBase.php

[error] 218-218: Test failure: Utopia\Tests\Audit\Adapter\ClickHouseTest::testLogByBatch - Expected value to be of type 'array', but got 'boolean'. Command: docker compose exec tests vendor/bin/phpunit --configuration phpunit.xml tests

---

[error] 218-218: Test failure: Utopia\Tests\Audit\Adapter\DatabaseTest::testLogByBatch - Expected value to be of type 'array', but got 'boolean'. Command: docker compose exec tests vendor/bin/phpunit --configuration phpunit.xml tests

🔇 Additional comments (1)

tests/Audit/Adapter/ClickHouseTest.php (1)

312-313: LGTM - correctly updated for boolean return type.

The assertion properly validates that logBatch returns a truthy boolean value, consistent with the API changes in this PR.