Chore update db #104
Chore update db #104
Conversation
|
Caution Review failedThe pull request is closed. Walkthroughcomposer.json relaxes the utopia-php/database constraint from "2." to "". Tests under tests/Abuse/Bench were moved to namespace Utopia\Tests\Bench and several classes renamed (DatabaseBench→Database, RedisBench→Redis, RedisClusterBench→RedisCluster). tests/Abuse/Bench/Base.php adds imports and replaces some calls with global-qualified functions. tests/Abuse/Bench/Database.php switches from a MariaDB-specific adapter to the SQL adapter, aliases Utopia\Database\Database as UtopiaDatabase, and initializes PDO using named parameters and SQL::getPDOAttributes(); setUp sequence adjusted. docker-compose.yml removes the adminer service and changes redis-cluster images to bitnamilegacy/redis-cluster:7.4. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
composer.json(1 hunks)tests/Abuse/Bench/Base.php(1 hunks)tests/Abuse/Bench/Database.php(3 hunks)tests/Abuse/Bench/Redis.php(1 hunks)tests/Abuse/Bench/RedisCluster.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/Abuse/Bench/RedisCluster.php (3)
src/Abuse/Abuse.php (1)
Abuse(5-56)src/Abuse/Adapters/TimeLimit.php (1)
TimeLimit(8-105)tests/Abuse/Bench/Base.php (1)
Base(12-32)
tests/Abuse/Bench/Redis.php (1)
tests/Abuse/Bench/Base.php (1)
Base(12-32)
tests/Abuse/Bench/Base.php (2)
src/Abuse/Abuse.php (1)
Abuse(5-56)src/Abuse/Adapters/TimeLimit.php (1)
TimeLimit(8-105)
tests/Abuse/Bench/Database.php (2)
tests/Abuse/Bench/Base.php (1)
Base(12-32)tests/Abuse/Base.php (1)
Base(9-99)
🪛 GitHub Actions: Linter
tests/Abuse/Bench/Database.php
[error] 1-1: Pint: no_unused_imports - Unused import detected. See PSR-12 in the lint results.
🔇 Additional comments (11)
tests/Abuse/Bench/Base.php (2)
9-10: LGTM: Necessary imports for namespace reorganization.The new imports for
AbuseandTimeLimitalign with the namespace refactoring fromAbuse\BenchtoUtopia\Tests\Bench.
25-28: LGTM: Explicit global function qualification.Using fully-qualified global function calls (
\rand,\rtrim) is a defensive coding practice that improves clarity and avoids potential namespace conflicts.tests/Abuse/Bench/RedisCluster.php (3)
3-3: LGTM: Namespace standardization.The namespace change to
Utopia\Tests\Benchaligns with the broader namespace reorganization across all benchmark files.
7-7: LGTM: Clean adapter aliasing.Using
RedisClusterAdapteras an alias improves readability and avoids confusion with theRedisClusterclass name.
9-9: LGTM: Simplified class naming.Renaming from
RedisClusterBenchtoRedisClusterremoves redundant "Bench" suffix while the namespace already indicates this is a benchmark file.tests/Abuse/Bench/Redis.php (2)
3-3: LGTM: Namespace standardization.The namespace change to
Utopia\Tests\Benchis consistent with the broader refactoring across all benchmark files.
9-9: LGTM: Simplified class naming.Renaming from
RedisBenchtoRedisremoves the redundant "Bench" suffix.tests/Abuse/Bench/Database.php (3)
32-37: LGTM: Modern PDO constructor syntax.Using named parameters improves readability and makes the PDO instantiation more explicit.
39-39: LGTM: Consistent alias usage.Using
UtopiaDatabaseconsistently with the import alias clarifies the distinction between the benchmark class and the library class.
12-13: Verify SQL adapter compatibility with MariaDB for this benchmark.The adapters are external dependencies in the Utopia\Database package and cannot be verified through codebase inspection. Ensure that
SQL::getPDOAttributes()returns appropriate PDO configuration for your MariaDB setup. The benchmark instantiates theMySQLadapter, so thegetPDOAttributes()call must provide compatible options. Test the benchmark locally to confirm the migration works as expected.composer.json (1)
25-25: Wildcard dependency constraint introduces breaking change risk.Changing from
"2.*"to"*"removes major version pinning forutopia-php/database, allowing any future major version to be installed. This confirmed to be a concern: version 3.0.0 was just released today (2025-10-20), and changing the constraint to a wildcard would allow this major version in routine dependency updates.Before proceeding, verify that:
- Code changes in this PR ensure compatibility with utopia-php/database 3.0.0 and its breaking changes, or
- Pin the constraint to
"2.*"or"^2.0"if 3.0.0 support is not intended.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docker-compose.yml(2 hunks)tests/Abuse/Bench/Database.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Abuse/Bench/Database.php (2)
tests/Abuse/Bench/Base.php (1)
Base(12-32)tests/Abuse/Base.php (1)
Base(9-99)
🔇 Additional comments (6)
tests/Abuse/Bench/Database.php (5)
3-3: LGTM! Namespace refactoring aligns with test suite restructuring.The namespace change from
Abuse\BenchtoUtopia\Tests\Benchis consistent with the broader test suite refactoring mentioned in the AI summary.
11-12: LGTM! Necessary imports for adapter changes.The
SQLadapter import supports the newSQL::getPDOAttributes()usage, and theUtopiaDatabasealiasing appropriately avoids naming conflicts with the test class.
15-15: LGTM! Class rename and final modifier are appropriate.The class rename from
DatabaseBenchtoDatabasealigns with the naming convention changes across benchmark tests, and thefinalmodifier is a good practice for classes not intended for inheritance.
44-44: LGTM! Minor formatting improvement.The updated formatting with the negation operator inside the parentheses improves code readability.
31-36: No issues found—PDO instantiation is correct.The SQL::getPDOAttributes() method from Utopia Database provides appropriate PDO attributes for MySQL/MariaDB connections, including proper timeout, fetch mode, error handling, and prepared statement configuration. The code correctly uses the MySQL adapter with the mysql:// DSN, and the PHP 8.0+ named parameters follow best practices.
docker-compose.yml (1)
53-53: Confirm strategy for legacy Redis cluster image—frozen, no security updates.Bitnami moved redis-cluster 7.4 to a legacy archive as part of repo reorganization and launch of paid Bitnami Secure Images; legacy images are frozen with no new versions, security fixes, or support. This is a known organizational change, not an error.
However, this creates a security risk: your deployment runs on an unmaintained image that will never receive patches. Before proceeding:
- Confirm this is intentional – ensure the team explicitly accepts the frozen image limitation
- Plan mitigation – either:
- Pin and mirror the exact bitnami/redis-cluster:7.4 image into your private registry, or
- Migrate to an actively maintained Redis cluster alternative
- Document the decision – add a comment in
docker-compose.ymlexplaining the legacy image choice and accepted riskAlso applies to: lines 67, 75, 83
| ); | ||
|
|
||
| $db = new Database(new MySQL($pdo), new Cache(new NoCache())); | ||
| $db = new UtopiaDatabase(new MySQL($pdo), new Cache(new NoCache())); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify MySQL adapter compatibility with existing database setup.
The database instantiation now explicitly uses the MySQL adapter. Please confirm that switching from the MariaDB adapter (if previously used) to the MySQL adapter doesn't affect functionality, especially given that the docker-compose.yml uses a MySQL 8 image.
Run the benchmark tests to ensure the adapter change doesn't introduce regressions:
🏁 Script executed:
#!/bin/bash
# Run the database benchmark tests
echo "=== Running Database benchmark tests ==="
vendor/bin/phpbench run tests/Abuse/Bench/Database.php --report=defaultLength of output: 106
🏁 Script executed:
#!/bin/bash
# Check docker-compose.yml for database service configuration
echo "=== Docker Compose Database Configuration ==="
find . -name "docker-compose.yml" -o -name "docker-compose.yaml" | head -5 | xargs cat
echo -e "\n=== Looking for adapter-related code ==="
# Search for MySQL adapter usage patterns
rg -n "new MySQL|new MariaDB" --type=php -C2 | head -30
echo -e "\n=== Check for database adapter configuration ==="
rg -n "adapter|Adapter" tests/Abuse/Bench/Database.php -C3
echo -e "\n=== Examine the full database benchmark file ==="
cat tests/Abuse/Bench/Database.phpLength of output: 5047
🏁 Script executed:
#!/bin/bash
# Search for adapter implementations to compare getPDOAttributes()
echo "=== Searching for MariaDB adapter implementation ==="
rg -n "class MariaDB|getPDOAttributes" --type=php -B2 -A5 | grep -A 10 "class MariaDB"
echo -e "\n=== Searching for SQL adapter implementation ==="
rg -n "class SQL|getPDOAttributes" --type=php -B2 -A5 | grep -A 10 "class SQL"
echo -e "\n=== Check if MariaDB is imported in Bench/Database.php ==="
rg -n "MariaDB" tests/Abuse/Bench/Database.php
echo -e "\n=== Look for all uses of getPDOAttributes in tests ==="
rg -n "getPDOAttributes" --type=php -B1 -A1Length of output: 407
🏁 Script executed:
#!/bin/bash
# Search for adapter implementations to compare getPDOAttributes()
echo "=== Searching for MariaDB adapter implementation ==="
rg -n "class MariaDB|getPDOAttributes" --type=php -B2 -A5 | head -50
echo -e "\n=== Searching for SQL adapter implementation ==="
rg -n "class SQL" --type=php -A10 | grep -A 10 "getPDOAttributes"
echo -e "\n=== Check if MariaDB is imported in Bench/Database.php ==="
rg -n "MariaDB" tests/Abuse/Bench/Database.php
echo -e "\n=== Look for all uses of getPDOAttributes in codebase ==="
rg -n "getPDOAttributes" --type=php -B2 -A2 | head -60Length of output: 1059
🏁 Script executed:
#!/bin/bash
# Check composer.json for Utopia Database version
echo "=== Composer dependency info ==="
cat composer.json | grep -A 5 -B 5 "utopia/database" || grep -i database composer.json
echo -e "\n=== Check DatabaseTest.php for comparison ==="
cat tests/Abuse/DatabaseTest.php | head -50
echo -e "\n=== Search for any recent changes or patterns in test setup ==="
rg -n "SQL::getPDOAttributes|MariaDB::getPDOAttributes" --type=php -B3 -A3Length of output: 2049
🏁 Script executed:
#!/bin/bash
# Search for all database initialization patterns
echo "=== All database initialization patterns in tests ==="
rg -n "new PDO.*mysql:|new MySQL|new MariaDB" --type=php -B1 -A1 | head -80
echo -e "\n=== Check if there are other benchmark or test files ==="
find tests -name "*.php" -type f | xargs grep -l "UtopiaDatabase\|new Database" | head -10
echo -e "\n=== Look for any base setup or factory ==="
cat tests/Abuse/Bench/Base.php 2>/dev/null || echo "Base.php not found in expected location"Length of output: 1573
Update PDO attributes in DatabaseTest.php to match Bench/Database.php for consistency.
The change in Bench/Database.php uses SQL::getPDOAttributes() instead of MariaDB::getPdoAttributes() when connecting to MySQL 8. However, tests/Abuse/DatabaseTest.php (line 34) still uses MariaDB::getPdoAttributes() with the MySQL adapter, creating an inconsistency. Both test files should use the same PDO attributes source.
Update tests/Abuse/DatabaseTest.php line 34 to use SQL::getPDOAttributes() and import the SQL adapter class:
use Utopia\Database\Adapter\SQL;Then change:
$pdo = new PDO("mysql:host={$dbHost};port={$dbPort};charset=utf8mb4", $dbUser, $dbPass, MariaDB::getPdoAttributes());To:
$pdo = new PDO("mysql:host={$dbHost};port={$dbPort};charset=utf8mb4", $dbUser, $dbPass, SQL::getPDOAttributes());🤖 Prompt for AI Agents
In tests/Abuse/DatabaseTest.php around line 34, update the MySQL PDO attributes
to match Bench/Database.php by importing the SQL adapter and switching the PDO
attributes call from MariaDB::getPdoAttributes() to SQL::getPDOAttributes(); add
the line to import Utopia\Database\Adapter\SQL at the top of the file and
replace the fourth argument passed to the PDO constructor accordingly so the
MySQL adapter uses SQL::getPDOAttributes().
Summary by CodeRabbit
Chores
Tests