Add a contract test for database adapters - #352
Open
jamesridgway wants to merge 2 commits into
Open
jamesridgway wants to merge 2 commits into
jamesridgway wants to merge 2 commits into
Conversation
…rams SQLite#test_workerize checks for an existing suffix, because Rails can call it a second time for the same database configuration (see rails/rails#55769). The check covered a file path and a URI without query params. It did not cover a URI with query params, where the suffix goes before the "?". A second call changed file:storage/foo/main.sqlite3_1?vfs=unix-dotfile into file:storage/foo/main.sqlite3_1_1?vfs=unix-dotfile Split the query params from the path first, then do the same check for all three forms.
The gem calls a fixed set of methods on a database adapter, but only the SQLite adapter exists, so nothing states what another adapter must do. Add a test that describes that contract: create_database, drop_database, database_exist?, database_ready?, acquire_ready_lock, tenant_databases, validate_tenant_name, and test_workerize. The test runs against the adapter of each database scenario, so a new adapter is covered when its scenarios are added.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Copilot review overview
🟢 Approved
The focused implementation and tests consistently establish the adapter contract and cover the SQLite regression.
Review effort: Balanced
Findings: None
What changed in this PR
Adds a reusable database-adapter contract test to support future MySQL and PostgreSQL adapters, alongside the SQLite fix from #351.
Changes:
- Tests the eight adapter operations across every database scenario.
- Prevents duplicate SQLite worker suffixes on URIs with query parameters.
- Adds regression tests and a changelog entry.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
test/unit/database_adapter_contract_test.rb |
Adds the cross-adapter behavioral contract. |
test/unit/database_adapters_sqlite_test.rb |
Covers SQLite workerization and URI edge cases. |
lib/active_record/tenanted/database_adapters/sqlite.rb |
Makes URI worker suffixing idempotent. |
CHANGELOG.md |
Documents the SQLite fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A few people have tried to add MySQL and PostgreSQL support to this gem: #246, #261 and #283. Thank you to the authors for that work. Those branches are now out of date with
main.I want to learn from those attempts. Then I want to make a focused effort, over a short period, to add and improve MySQL and PostgreSQL support, so that the work does not go out of date again. I will do this in small PRs. This is the first one.
Purpose
This is the first step of the work to support databases other than SQLite. The targets are MySQL and PostgreSQL.
The Issue
The gem calls 8 methods on a database adapter. SQLite is the only adapter, so the SQLite code is the only definition of what these methods must do.
Three upstream branches tried to add an adapter: #246, #261 and #283. Each author had to read the contract from the SQLite code, and the results are different. For example, in all three branches
database_ready?is true while a new tenant database is still in migration. In SQLite it is false until the migration is complete.Why now
A new adapter needs a definition that it can run against. This test is that definition, so it must exist before the first new adapter.
We add it while SQLite is the only adapter. This shows that the test describes the behaviour that the gem has today. There is no change to
lib.The Change
Add
test/unit/database_adapter_contract_test.rb. It describes the contract that the gem uses today:create_database,drop_database,database_exist?database_ready?andacquire_ready_locktenant_databases, also with a test worker idvalidate_tenant_nametest_workerizeThe test runs against the adapter of each database scenario. A new adapter adds its scenarios, and this test then runs against that adapter with no change to the test.
The test found one bug in the SQLite adapter. The fix is in #351, and this branch is on top of that branch. This PR also shows that commit until #351 is merged.
What comes next
create_databasethatdb:migratecan call. Each change to the contract adds a test to this file first.