Conversation
📝 WalkthroughWalkthroughUpdates the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 hostname support to the
Confidence Score: 4/5Safe to review, but should not be merged to main until The implementation changes (Pool delegation, Mongo hostname fix) are correct. The blocking concern is the
Important Files Changed
Reviews (4): Last reviewed commit: "Mongo getHost" | 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 |
| "utopia-php/cache": "1.*", | ||
| "utopia-php/pools": "1.*", | ||
| "utopia-php/mongo": "1.*" | ||
| "utopia-php/mongo": "dev-get-host as 1.0.3" |
There was a problem hiding this comment.
"utopia-php/mongo": "dev-get-host as 1.0.3" pins the entire library to an unpublished development branch aliased as a stable version. Composer's as alias bypasses the minimum-stability: stable guard and lets the branch be installed as if it were a real 1.0.3 release — but the underlying ref is a mutable branch tip. If that branch is rebased, force-pushed, or deleted, any fresh install will either break or silently pull different code. This should be replaced with a proper tagged release of utopia-php/mongo that includes the getHost() method before this PR is merged to main.
|
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
| "utopia-php/cache": "1.*", | ||
| "utopia-php/pools": "1.*", | ||
| "utopia-php/mongo": "1.*" | ||
| "utopia-php/mongo": "dev-get-host as 1.0.3" |
There was a problem hiding this comment.
Avoid merging a dev-branch dependency to main.
Pinning utopia-php/mongo to dev-get-host as 1.0.3 ties this PR to an unmerged feature branch of the Mongo package. Merging this to main will make installs brittle (branch can disappear/move) and break downstream consumers whose minimum-stability doesn't permit dev aliases transitively. Please get the corresponding utopia-php/mongo change released as a tagged version (e.g., 1.0.4) and update the constraint to a stable range ("1.*" or "^1.0.4") before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@composer.json` at 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.
| public function testDatabaseHostname(): void | ||
| { | ||
| /** @var Database $database */ | ||
| $database = $this->getDatabase(); | ||
|
|
||
| $host = $database->getAdapter()->getHostname(); | ||
|
|
||
| if (!$database->getAdapter()->getSupportForHostname()) { | ||
| $this->expectNotToPerformAssertions(); | ||
| return; | ||
| } | ||
|
|
||
| $this->assertTrue(in_array($host, ['mysql', 'mariadb', 'postgres', 'mongo'])); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm what each SQL adapter's getHostname() actually returns (PDO helper signature)
ast-grep --pattern 'function getHostname(): string { $$$ }'
rg -nP -C2 '\bgetHostname\s*\(' --type=phpRepository: utopia-php/database
Length of output: 2266
🏁 Script executed:
#!/bin/bash
# Find getSupportForHostname() implementations
rg -nP 'getSupportForHostname' --type=php -A2 -B1Repository: utopia-php/database
Length of output: 2163
🏁 Script executed:
#!/bin/bash
# Check the test setup to understand which adapters are tested
rg -nP 'getDatabase|setUp|Provider' tests/e2e/Adapter/Scopes/CollectionTests.php -B2 -A2 | head -100Repository: utopia-php/database
Length of output: 3641
🏁 Script executed:
#!/bin/bash
# Check what values PDO.getHostname() actually returns by examining parseDsn
rg -nP 'parseDsn' src/Database/PDO.php -A20 | head -80Repository: utopia-php/database
Length of output: 1370
🏁 Script executed:
#!/bin/bash
# Check for test environment setup and what hostnames are configured
fd -type f -name "*.php" "tests/" | xargs rg -l "mysql|mariadb|postgres|mongo" | head -10Repository: utopia-php/database
Length of output: 644
🏁 Script executed:
#!/bin/bash
# Look for test configuration files (docker-compose, .env, etc)
find . -maxdepth 2 \( -name "docker-compose*" -o -name ".env*" -o -name "phpunit.xml" \) -exec cat {} \;Repository: utopia-php/database
Length of output: 6764
🏁 Script executed:
#!/bin/bash
# Check how test adapters are instantiated and what DSNs are used
rg -n "MySQLTest|MariaDBTest|PostgresTest|MongoDBTest" tests/e2e/Adapter/ -A5 | head -50Repository: utopia-php/database
Length of output: 3103
🏁 Script executed:
#!/bin/bash
# Look for test setup/bootstrap files that might configure DSNs
find tests -name "*.php" -path "*/Adapter/*Test.php" | head -3 | xargs head -50Repository: utopia-php/database
Length of output: 4025
🏁 Script executed:
#!/bin/bash
# Check what PDO.getHostname() actually returns by examining the DSN parsing
# For MySQL/MariaDB, host=mariadb would be parsed and returned as 'mariadb'
rg -n "host=" tests/e2e/Adapter/ --type=php | grep -E "(mysql|postgres|mongo)"Repository: utopia-php/database
Length of output: 1188
🏁 Script executed:
#!/bin/bash
# Verify what Mongo.getHostname() returns - check the Client class
rg -n "getHost" src/ --type=php -B2 -A5Repository: utopia-php/database
Length of output: 2800
🏁 Script executed:
#!/bin/bash
# Check PostgreSQL and MongoDB test setups
rg -n "getDatabase" tests/e2e/Adapter/PostgresTest.php -A20 | head -40Repository: utopia-php/database
Length of output: 1485
🏁 Script executed:
#!/bin/bash
# Check MongoDB getHost implementation
rg -n "getHost\|class Client" src/ -B2 -A3 --type=php | grep -A5 "getHost"Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Verify all test adapter setups for hostname values
rg -n "dbHost|Host.*=" tests/e2e/Adapter/PostgresTest.php tests/e2e/Adapter/MongoDBTest.phpRepository: utopia-php/database
Length of output: 280
Move getHostname() call after the guard and use assertContains() for clarity.
Two issues in this test:
-
getHostname()is called before thegetSupportForHostname()guard. This invokes the API unnecessarily for adapters that don't support hostname (like SQLite). Move the call after the guard—the codebase already uses this pattern elsewhere (e.g.,Database.php:9344). -
The assertion
in_array($host, ['mysql','mariadb','postgres','mongo'])should useassertContains()for a clearer failure message and better semantics. While the checked values match the test environment hostnames, this is more defensive if an unexpected value is returned.
Suggested fix
public function testDatabaseHostname(): void
{
/** `@var` Database $database */
$database = $this->getDatabase();
- $host = $database->getAdapter()->getHostname();
-
if (!$database->getAdapter()->getSupportForHostname()) {
$this->expectNotToPerformAssertions();
return;
}
- $this->assertTrue(in_array($host, ['mysql', 'mariadb', 'postgres', 'mongo']));
+ $host = $database->getAdapter()->getHostname();
+ $this->assertContains($host, ['mysql', 'mariadb', 'postgres', 'mongo']);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/Adapter/Scopes/CollectionTests.php` around lines 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()).
2 participants
Summary by CodeRabbit