Conversation
PRAGMA statements can return rows (e.g. PRAGMA wal_checkpoint(FULL) returns busy/log/checkpointed columns), but should_use_query() did not recognise them, causing them to be routed through execute() which fails with "Execute returned rows" from libsql. Two fixes: - native/ecto_libsql/src/utils.rs: Add PRAGMA recognition to should_use_query() so all PRAGMA statements use the query() code path instead of execute(). This is correct behaviour since PRAGMA can always return rows. - test/ecto_integration_test.exs: Add PRAGMA wal_checkpoint(FULL) after CREATE UNIQUE INDEX in the composite unique index test setup. On Linux, WAL mode can cause DDL changes to be invisible to concurrent connections until the WAL is checkpointed, causing the ON CONFLICT clause to fail with "does not match any PRIMARY KEY or UNIQUE constraint". Fixes CI failure introduced by rustler 0.37.1 -> 0.37.3 bump.
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds case-insensitive PRAGMA detection to SQL routing so PRAGMA statements are treated as row-returning queries; updates unit tests to cover PRAGMA variants and whitespace; moves DDL setup to a global test setup and adds WAL checkpointing and teardown in integration tests; includes local CI/allowlist and doc edits. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
native/ecto_libsql/src/utils.rs (1)
362-364: Update theshould_use_querydoc comment to reflect all routing conditions.The summary currently says only "SELECT or has RETURNING clause", but the function already handled EXPLAIN and WITH before this PR, and now also handles PRAGMA. The description will continue to mislead future readers.
📝 Proposed doc comment update
-/// Returns true if should use query() (SELECT or has RETURNING clause). +/// Returns true if the statement should use `query()` rather than `execute()`. +/// +/// Routes through `query()` when the statement may return rows: +/// - `SELECT` — always returns rows +/// - `WITH` — CTE; typically precedes a `SELECT` +/// - `EXPLAIN` — always returns rows +/// - `PRAGMA` — may return rows (e.g. `PRAGMA wal_checkpoint(FULL)`) +/// - Any statement containing a `RETURNING` clause🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/ecto_libsql/src/utils.rs` around lines 362 - 364, Update the doc comment for the function should_use_query to enumerate all routing conditions it checks (SELECT, statements with RETURNING, EXPLAIN, WITH, and PRAGMA) instead of only "SELECT or has RETURNING clause"; locate the comment immediately above the should_use_query function and expand the summary to mention EXPLAIN, WITH, and PRAGMA so the doc accurately reflects the implemented logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@native/ecto_libsql/src/utils.rs`:
- Around line 362-364: Update the doc comment for the function should_use_query
to enumerate all routing conditions it checks (SELECT, statements with
RETURNING, EXPLAIN, WITH, and PRAGMA) instead of only "SELECT or has RETURNING
clause"; locate the comment immediately above the should_use_query function and
expand the summary to mention EXPLAIN, WITH, and PRAGMA so the doc accurately
reflects the implemented logic.
The test_pragma_statements test incorrectly asserted that PRAGMA statements should NOT use the query() path. Since PRAGMA can return rows, the correct behaviour (implemented in the previous commit) is to always route PRAGMA through query(). Update the test assertions accordingly and add extra cases to cover wal_checkpoint and lowercase pragma.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
native/ecto_libsql/src/tests/utils_tests.rs (1)
263-271: All assertions are correct and align with theutils.rsimplementation.The five new cases cover the primary happy-path scenarios (mixed statement variants, lowercase
pragma, and thewal_checkpoint(FULL)case that motivated this PR).Two small coverage gaps exist compared to the test patterns already established for
EXPLAINandSELECT:
- No word-boundary negative case —
test_not_select_if_part_of_wordandtest_not_cte_if_part_of_wordboth guard against prefix collisions (e.g."SELECTED","WITHDRAW"). A PRAGMA equivalent is absent.- No leading-whitespace positive case —
test_explain_with_whitespaceverifies thatskip_whitespace_and_commentsworks beforeEXPLAIN. The same is untested forPRAGMA.🔍 Suggested additional assertions
fn test_pragma_statements() { // PRAGMA statements always use query() since they can return rows. // E.g. PRAGMA wal_checkpoint(FULL) returns busy/log/checkpointed columns. assert!(should_use_query("PRAGMA table_info(users)")); assert!(should_use_query("PRAGMA foreign_keys = ON")); assert!(should_use_query("PRAGMA wal_checkpoint(FULL)")); assert!(should_use_query("pragma journal_mode")); assert!(should_use_query("PRAGMA foreign_keys")); + + // Leading whitespace should be skipped correctly. + assert!(should_use_query(" PRAGMA journal_mode")); + assert!(should_use_query("\tPRAGMA table_info(users)")); + assert!(should_use_query("\nPRAGMA wal_checkpoint(FULL)")); + + // "PRAGMAtic" and similar must NOT match — keyword boundary check. + assert!(!should_use_query("PRAGMAtic settings")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/ecto_libsql/src/tests/utils_tests.rs` around lines 263 - 271, Add two small tests to cover the missing PRAGMA edge cases: (1) a negative word-boundary case that ensures should_use_query does NOT match when "PRAGMA" is part of a larger word (mirror tests like test_not_select_if_part_of_word / test_not_cte_if_part_of_word), and (2) a leading-whitespace positive case that verifies skip_whitespace_and_comments allows matching PRAGMA with leading spaces/comments (mirror test_explain_with_whitespace). Add these assertions alongside test_pragma_statements using the same helper should_use_query so the behavior for should_use_query("...") is validated for both the non-word-boundary and the whitespace-prefixed PRAGMA variants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@native/ecto_libsql/src/tests/utils_tests.rs`:
- Around line 263-271: Add two small tests to cover the missing PRAGMA edge
cases: (1) a negative word-boundary case that ensures should_use_query does NOT
match when "PRAGMA" is part of a larger word (mirror tests like
test_not_select_if_part_of_word / test_not_cte_if_part_of_word), and (2) a
leading-whitespace positive case that verifies skip_whitespace_and_comments
allows matching PRAGMA with leading spaces/comments (mirror
test_explain_with_whitespace). Add these assertions alongside
test_pragma_statements using the same helper should_use_query so the behavior
for should_use_query("...") is validated for both the non-word-boundary and the
whitespace-prefixed PRAGMA variants.
…ity races Creating and dropping the locations table and composite unique index inside each test's setup callback causes a race: each Ecto.Adapters.SQL.query! call checks out a different connection from the pool, and on Linux the connection used for the UPSERT in the test body may not yet see the index written by a different pool connection during setup. The fix is to create the locations table and its unique index once in setup_all alongside the users and posts tables. The per-test setup now simply deletes rows from the table, which is a plain DML statement visible to all connections immediately. This eliminates any DDL-visibility race across pool connections.
…tion Mirror the existing test_not_select_if_part_of_word / test_not_cte_if_part_of_word and test_explain_with_whitespace patterns for the PRAGMA branch of should_use_query: - test_not_pragma_if_part_of_word: asserts that "PRAGMATIC" and similar strings do NOT match, confirming the word-boundary guard is enforced. - test_pragma_with_whitespace: asserts that leading spaces, tabs, and newlines before PRAGMA are correctly skipped by skip_whitespace_and_comments.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
native/ecto_libsql/src/tests/utils_tests.rs (1)
280-286: Consider adding a mixed-case PRAGMA test for parity with other keywords.The SELECT tests include mixed-case variants like
SeLeCtandsElEcT(lines 128–129), but PRAGMA only tests uppercase and fully lowercase. A quick mixed-case assertion would confirm the per-byte case-insensitive logic end-to-end.🧪 Optional: add mixed-case coverage
#[test] fn test_pragma_with_whitespace() { // Leading whitespace before PRAGMA must be skipped correctly. assert!(should_use_query(" PRAGMA foreign_keys")); assert!(should_use_query("\tPRAGMA journal_mode")); assert!(should_use_query("\n PRAGMA wal_checkpoint(FULL)")); } + + #[test] + fn test_pragma_case_insensitive() { + assert!(should_use_query("PrAgMa journal_mode")); + assert!(should_use_query("pRaGmA foreign_keys")); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/ecto_libsql/src/tests/utils_tests.rs` around lines 280 - 286, Add a mixed-case PRAGMA assertion to the existing test_pragma_with_whitespace to mirror the SELECT mixed-case coverage: update the test that calls should_use_query and add at least one assertion using mixed-case letters (e.g., " PrAgMa journal_mode" or "\tpRaGmA wal_checkpoint(FULL)") so the should_use_query function's case-insensitive handling for PRAGMA is exercised end-to-end.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@native/ecto_libsql/src/tests/utils_tests.rs`:
- Around line 280-286: Add a mixed-case PRAGMA assertion to the existing
test_pragma_with_whitespace to mirror the SELECT mixed-case coverage: update the
test that calls should_use_query and add at least one assertion using mixed-case
letters (e.g., " PrAgMa journal_mode" or "\tpRaGmA wal_checkpoint(FULL)") so
the should_use_query function's case-insensitive handling for PRAGMA is
exercised end-to-end.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
native/ecto_libsql/src/tests/utils_tests.rs (1)
263-290: LGTM — new PRAGMA tests are thorough and correctly exercise the routing logic.The three new test functions cover the primary cases (
test_pragma_statements), word-boundary safety (test_not_pragma_if_part_of_word), and leading-whitespace/mixed-case paths (test_pragma_with_whitespace). Each assertion maps cleanly to a reachable branch inshould_use_queryinsideutils.rs.One very minor gap: there is no test for a bare
"PRAGMA"string (exactly six characters, nothing after). Thestart + 6 >= lenguard inutils.rscovers it, but adding a single assertion would document the intent explicitly.✅ Optional assertion to add to
test_pragma_with_whitespaceor a new function+ #[test] + fn test_pragma_alone() { + // Bare PRAGMA keyword (no trailing characters) must also route through query(). + assert!(should_use_query("PRAGMA")); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/ecto_libsql/src/tests/utils_tests.rs` around lines 263 - 290, Add an explicit assertion for the bare "PRAGMA" token to document the intent of the start+6 >= len guard in should_use_query: add assert!(should_use_query("PRAGMA")); (either appended to test_pragma_with_whitespace or in a new small test) so the case of an exact six-character PRAGMA with nothing following is covered and exercised against the should_use_query logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Around line 352-355: The fenced code block containing TURSO_DB_URI and
TURSO_AUTH_TOKEN lacks a language specifier; update the block that surrounds the
two lines (the block showing TURSO_DB_URI="libsql://your-database.turso.io" and
TURSO_AUTH_TOKEN="your-token-here") to include a language tag such as dotenv
(e.g., change the opening ``` to ```dotenv) to enable syntax highlighting and
silence MD040.
- Line 15: Fix the malformed Markdown inline code in CLAUDE.md by removing the
stray backtick at the end of the code span: change the inline text containing
`mix format --check-formatted && cargo fmt --check`` to use a single trailing
backtick so the inline code span is correctly delimited (look for the exact
string shown and update it).
---
Nitpick comments:
In `@native/ecto_libsql/src/tests/utils_tests.rs`:
- Around line 263-290: Add an explicit assertion for the bare "PRAGMA" token to
document the intent of the start+6 >= len guard in should_use_query: add
assert!(should_use_query("PRAGMA")); (either appended to
test_pragma_with_whitespace or in a new small test) so the case of an exact
six-character PRAGMA with nothing following is covered and exercised against the
should_use_query logic.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.claude/settings.local.jsonCLAUDE.mdUSAGE.mdnative/ecto_libsql/src/tests/utils_tests.rs
Amp-Thread-ID: https://ampcode.com/threads/T-019c977d-c248-73b7-a08b-ff94d46ec68a Co-authored-by: Amp <amp@ampcode.com>
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
1 participant
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.
PRAGMA statements can return rows (e.g. PRAGMA wal_checkpoint(FULL) returns busy/log/checkpointed columns), but should_use_query() did not recognise them, causing them to be routed through execute() which fails with "Execute returned rows" from libsql.
Two fixes:
Fixes CI failure introduced by rustler 0.37.1 -> 0.37.3 bump.
Summary by CodeRabbit
Bug Fixes
Tests
Documentation
Chores