Conversation
📝 WalkthroughWalkthroughAdds public Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 adds The previously reported issues (hardcoded Confidence Score: 4/5Safe to merge after confirming the composer.lock regressions are intentional or corrected. No new P0/P1 logic bugs found in the changed source files. The previously flagged P1 on composer.lock downgrades is still present and unresolved, capping the score at 4.
Important Files Changed
Reviews (9): Last reviewed commit: "Update tests" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
CI failure on run 24836281128 is a flaky, timing-dependent test unrelated to this branch. Failed test: Root cause: The test creates Why it is not this branch's fault:
No code changes made — exiting without commit per healing protocol for flaky failures. A retry of the CI run should be green. The underlying flakiness in |
|
CI Healing — Flaky Infra, No Fix Required The failing job was Why this is flake, not a code bug:
Action: No code change. Re-running the workflow should clear it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Database/Adapter/Mongo.php (1)
81-84: Consider consistency with SQL adapter's error handling pattern, but not required.The Mongo adapter's
getHostname()differs from the SQL adapter: SQL wraps its call in try/catch for defensive error handling, while Mongo returns the result directly. However, the concern aboutClient::getHost()potentially throwing is unfounded—it's a simple property accessor that returns a string and cannot fail. The SQL adapter's defensive pattern exists becausePDO::getHostname()can actually fail; Mongo'sClient::getHost()simply returns an initialized string property.Adding error handling for consistency is a reasonable stylistic choice, but it is not required for correctness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/Mongo.php` around lines 81 - 84, The Mongo adapter's getHostname() currently returns $this->client->getHost() directly while the SQL adapter wraps its hostname retrieval in a try/catch; to keep handling consistent, update Mongo::getHostname() (method getHostname in class using Client::getHost) to optionally wrap the call in a try/catch that returns the host on success and logs/returns an empty string or fallback on exception, or leave as-is if you prefer the simpler direct return since Client::getHost() is a safe accessor—choose one approach and apply it to getHostname() for consistency with the SQL adapter's defensive pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composer.json`:
- Line 44: The dependency "utopia-php/mongo" is pinned to a dev-alias
("dev-get-host as 1.0.3") which should not be merged to main; wait for a proper
release of the utopia-php/mongo change (e.g., tag 1.0.4) and update the composer
constraint in composer.json (replace "utopia-php/mongo": "dev-get-host as
1.0.3") with a stable range such as "utopia-php/mongo": "^1.0.4" or "1.*" so
installations are stable and compatible with consumers' minimum-stability.
In `@tests/e2e/Adapter/Scopes/CollectionTests.php`:
- Around line 76-89: In testDatabaseHostname(), avoid calling getHostname()
before checking support: first call
$database->getAdapter()->getSupportForHostname() and bail with
$this->expectNotToPerformAssertions() if false, then call
$database->getAdapter()->getHostname(); replace the current
$this->assertTrue(in_array($host, [...])) with $this->assertContains($host,
['mysql','mariadb','postgres','mongo']) to get clearer failure output and better
semantics (referencing testDatabaseHostname, getAdapter(),
getSupportForHostname(), getHostname(), assertContains(), and
expectNotToPerformAssertions()).
---
Nitpick comments:
In `@src/Database/Adapter/Mongo.php`:
- Around line 81-84: The Mongo adapter's getHostname() currently returns
$this->client->getHost() directly while the SQL adapter wraps its hostname
retrieval in a try/catch; to keep handling consistent, update
Mongo::getHostname() (method getHostname in class using Client::getHost) to
optionally wrap the call in a try/catch that returns the host on success and
logs/returns an empty string or fallback on exception, or leave as-is if you
prefer the simpler direct return since Client::getHost() is a safe
accessor—choose one approach and apply it to getHostname() for consistency with
the SQL adapter's defensive pattern.
🪄 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: cdb82fcc-73c1-46a3-8491-88c8cb05ca9e
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
composer.jsonsrc/Database/Adapter/Mongo.phpsrc/Database/Adapter/Pool.phptests/e2e/Adapter/Scopes/CollectionTests.php
CI Heal — Adapter Tests (MySQL) flakeRun: 24953428998 / job 73067558612 What failedAll 535 tests in
Why this is a flake, not a code issue
Likely root cause (out of scope for this PR)The MySQL healthcheck in test: ["CMD", "mysqladmin", "ping", "-h", "localhost", "-u $$MYSQL_USER", "-p $$MYSQL_PASSWORD"]uses ActionNo commits — re-running — [healing task] |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/unit/CacheKeyTest.php (2)
157-170: Minor: clarify the inline comments around the global-collection assertions.The comments at lines 157-159 ("Check that tenant 999 exists") and 165-167 ("Check that tenant 999 was removed") read as if the tenant is being mutated on the adapter, when in reality the difference is driven by whether the collection is in the global set registered via
setGlobalCollections(['users']). Re-wording to e.g. "non-global collection retains the tenant segment" / "global collection drops the tenant segment" would make the intent of each block clearer to future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/CacheKeyTest.php` around lines 157 - 170, Update the inline test comments to accurately describe why the cache keys differ: replace "Check that tenant 999 exists" with something like "non-global collection retains the tenant segment" and replace "Check that tenant 999 was removed" with "global collection drops the tenant segment" (these relate to getCacheKeys(Database::METADATA, ...) behavior when collections are registered via setGlobalCollections(['users'])). Ensure the comments reference the global vs non-global collection behavior rather than implying the adapter mutates tenant membership.
134-188: Optional: collapse duplication between the two hostname tests via a data provider.
testParseHostnameandtestSimpleHostnameshare nearly identical adapter-mock setup. A@dataProvider(or a small private helper that builds the mocked adapter given a hostname) would shrink the duplication and make it easier to extend coverage to other hostname shapes (e.g.tcp://host:27017, IPv6, hostnames with credentials, empty string). Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/CacheKeyTest.php` around lines 134 - 188, The two tests testParseHostname and testSimpleHostname duplicate the same Adapter mock setup; extract this into a reusable data provider or helper to reduce duplication. Add either a `@dataProvider` method (e.g., hostnameProvider) that yields hostnames and change tests to accept the hostname parameter and reuse the existing mock setup, or implement a private helper method (e.g., createAdapterMock(string $hostname): Adapter) that builds and returns the configured Adapter mock used by Database in both testParseHostname and testSimpleHostname; update both tests to call the provider/helper and keep assertions identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Database/Database.php`:
- Around line 9346-9347: The cache key generation leaves DSN-style hostnames
unnormalized because $parts from parse_url($hostname) is computed but not
applied; update the hostname normalization in the cache key logic (around the
parse_url call) to assign $hostname to the parsed host when present (use
$parts['host'] if set, otherwise keep the original $hostname) so DSN URLs like
"scheme://host?..." yield just the host; ensure this change satisfies the
expected format used by testParseHostname().
---
Nitpick comments:
In `@tests/unit/CacheKeyTest.php`:
- Around line 157-170: Update the inline test comments to accurately describe
why the cache keys differ: replace "Check that tenant 999 exists" with something
like "non-global collection retains the tenant segment" and replace "Check that
tenant 999 was removed" with "global collection drops the tenant segment" (these
relate to getCacheKeys(Database::METADATA, ...) behavior when collections are
registered via setGlobalCollections(['users'])). Ensure the comments reference
the global vs non-global collection behavior rather than implying the adapter
mutates tenant membership.
- Around line 134-188: The two tests testParseHostname and testSimpleHostname
duplicate the same Adapter mock setup; extract this into a reusable data
provider or helper to reduce duplication. Add either a `@dataProvider` method
(e.g., hostnameProvider) that yields hostnames and change tests to accept the
hostname parameter and reuse the existing mock setup, or implement a private
helper method (e.g., createAdapterMock(string $hostname): Adapter) that builds
and returns the configured Adapter mock used by Database in both
testParseHostname and testSimpleHostname; update both tests to call the
provider/helper and keep assertions identical.
🪄 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: acfef3c1-011b-46ec-adec-012e4441e457
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
src/Database/Database.phptests/e2e/Adapter/Scopes/CollectionTests.phptests/unit/CacheKeyTest.php
✅ Files skipped from review due to trivial changes (1)
- tests/e2e/Adapter/Scopes/CollectionTests.php
|
CI healing — fixed unit test, MySQL adapter failures look infrastructural Unit Test (real bug, fixed): $parts = parse_url($hostname);
//$hostname = $parts['host'] ?? $hostname;Cache keys ended up containing the full DSN ( Adapter Tests (MySQL): All 535 errors were |
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Claude pushed fixes from: healing |
CI failure analysis (run 24961752121)Failure: Root cause: Flaky timing race, unrelated to this branch. The test creates a document with This branch ( A prior fix exists on the unmerged Action: No commit. Re-running CI should clear it; the proper fix belongs in a separate PR that targets the test itself. |
# Conflicts: # src/Database/Database.php
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/CacheKeyTest.php (1)
147-152: Misleading docblock: no DSN parsing is exercised here.The "Check DSN is parsed correctly" comment doesn't match what the test actually verifies.
getHostname()is mocked to return a literal string, so this assertion only validates thatDatabase::getCacheKeys()embeds the hostname into the cache key — DSN/parse_url()parsing lives in the adapter (Pool/Mongo) and isn't covered by this unit test. Consider rewording to avoid confusion.📝 Proposed wording
- /** - * Check DSN is parsed correctly - */ + /** + * Hostname is included in the cache key when the adapter supports it + */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/CacheKeyTest.php` around lines 147 - 152, The test's docblock "Check DSN is parsed correctly" is misleading because getHostname() is mocked and the test only verifies that Database::getCacheKeys() embeds the hostname into the cache key; update the comment to accurately describe the behavior under test (e.g., "Check hostname is embedded into cache key when getHostname() is provided/mocked") and, if helpful, mention that DSN/parse_url() logic is covered by the adapter-level tests (Pool/Mongo) rather than this unit test; locate the comment above the call to $db->getCacheKeys('users') and replace it accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/CacheKeyTest.php`:
- Around line 147-152: The test's docblock "Check DSN is parsed correctly" is
misleading because getHostname() is mocked and the test only verifies that
Database::getCacheKeys() embeds the hostname into the cache key; update the
comment to accurately describe the behavior under test (e.g., "Check hostname is
embedded into cache key when getHostname() is provided/mocked") and, if helpful,
mention that DSN/parse_url() logic is covered by the adapter-level tests
(Pool/Mongo) rather than this unit test; locate the comment above the call to
$db->getCacheKeys('users') and replace it accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6c4f9572-b6e6-4ae2-9dff-e8d6ab2bb8d0
📒 Files selected for processing (2)
src/Database/Database.phptests/unit/CacheKeyTest.php
✅ Files skipped from review due to trivial changes (1)
- src/Database/Database.php
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests