Do not add the test worker suffix twice to a SQLite URI with query params - #351
Open
jamesridgway wants to merge 1 commit into
Open
jamesridgway wants to merge 1 commit into
jamesridgway wants to merge 1 commit 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.
![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 implementation correctly preserves query parameters while preventing duplicate suffixes and includes focused regression coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Prevents duplicate SQLite test-worker suffixes when database URIs include query parameters.
Changes:
- Separates URI query parameters before checking and appending the suffix.
- Adds regression tests and changelog documentation.
[!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 |
|---|---|
lib/active_record/tenanted/database_adapters/sqlite.rb |
Makes URI suffixing idempotent with query parameters. |
test/unit/database_adapters_sqlite_test.rb |
Tests paths and URIs with and without existing suffixes. |
CHANGELOG.md |
Documents the SQLite URI 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.
The Issue
SQLite#test_workerizeadds the test worker suffix to the database name. Rails can call it a second time for the same database configuration (see rails/rails#55769), so the method checks for an existing suffix.The check covers a file path and a URI without query params. It does not cover a URI with query params, where the suffix goes before the
?. A second call changesinto
The
primary_uri_dbtest scenario has a database of this form.The Change
Split the query params from the path first, then do the same check for all three forms.
The new tests in
database_adapters_sqlite_test.rbcover a file path, a URI, and a URI with query params. The test "URI with query params that has the suffix" fails without the change.