Migrations, named parameters, security tests #48
Conversation
- Add Transaction Isolation tests to verify connections cannot access each other's transactions - Add Statement Isolation tests to verify prepared statements are scoped to connections - Add Cursor Isolation tests to verify cursors are scoped to connections - Add Savepoint Isolation tests to verify savepoints belong to owning transactions - Add Concurrent Access Safety tests for thread-safe operations - Add Resource Cleanup tests to verify cleanup on disconnect - Add Pool Isolation tests for multiple connections to same database - Add Cross-Connection Data Isolation tests for separate database files All 12 tests pass, verifying proper ownership tracking and security boundaries.
- Add support for 'strict: true' option in create_table() - STRICT tables enforce type checking (INT, INTEGER, BLOB, TEXT, REAL only) - Can be combined with RANDOM ROWID option - Generates 'STRICT' keyword at end of CREATE TABLE statement - Add tests for SQL generation (execution requires libSQL 3.37+) This implements part of el-z8u (STRICT Tables feature).
Implement full support for executing queries with named parameters (map-based arguments) instead of positional parameters (list-based). Supports all three SQLite syntaxes: - :name (colon prefix) - @name (at-sign prefix) - $name (dollar prefix) Changes: - Add normalize_arguments/3 helper to convert map params to positional list - Add map_to_positional_args/3 to introspect statement and reorder params - Add remove_param_prefix/1 helper to clean parameter names - Update handle_execute (both non-transactional and transactional paths) to normalize args - Add comprehensive test suite with 18 tests covering: - All three parameter syntaxes - Basic CRUD operations (INSERT, SELECT, UPDATE, DELETE) - Transactions (commit and rollback) - Prepared statements with parameter introspection - Edge cases (NULL values, extra params, missing params) - Backward compatibility with positional parameters Tests are thorough and include transaction isolation, error handling, and various parameter combinations. All tests clean up their database files after running. Issue: el-nqb
WalkthroughAdds named-parameter execution with statement introspection and ETS caching, libSQL table options (STRICT, RANDOM ROWID) and generated columns, extensive tests and teardown cleanup, config/ignore tweaks, and new public normalization helpers integrated into execution flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant Elixir as Elixir\n(lib/ecto_libsql/native.ex)
participant Introspect as Introspection\n(prep/cache)
participant NIF as native NIF\n(SQLite)
Note over Elixir,Introspect: Named-parameter normalization + ETS caching
Client->>Elixir: query(conn_id, sql, args)
Elixir->>Elixir: normalise_arguments(conn_id, sql, args)
alt args is a map (named params)
Elixir->>Introspect: prepare(conn_id, sql) to obtain param names
Introspect->>NIF: prepare_statement(sql)
NIF-->>Introspect: stmt_handle + param metadata
Introspect->>Elixir: ordered param names
Elixir->>Elixir: map_to_positional_args(map, ordered_names) and cache names
Elixir->>NIF: close_statement(stmt_handle)
else args already positional
Elixir-->>Elixir: use args unchanged
end
Elixir->>NIF: execute_with_positional_args(conn_id, sql, positional_args)
NIF-->>Elixir: result or error
Elixir-->>Client: result or descriptive error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
AGENTS.md (1)
1454-1547: Remove duplicated “STRICT Tables (Type Enforcement)” sectionThe “STRICT Tables (Type Enforcement)” section appears twice with effectively the same heading and content (first around the migrations section, then again immediately after RANDOM ROWID). This duplication makes the guide longer to scan and triggers markdown duplicate‑heading warnings.
I’d suggest keeping a single STRICT section (probably the first one near the main migrations discussion) and deleting or trimming the second to a short cross‑reference.
Example diff to drop the duplicate block
-#### STRICT Tables (Type Enforcement) - -STRICT tables enforce strict type checking - columns must be one of the allowed SQLite types. This prevents accidental type mismatches and data corruption: -... -# (lines 1606–1698 removed – second copy of STRICT docs) -... -```Also applies to: 1606-1698
lib/ecto/adapters/libsql/connection.ex (1)
384-396: Generated column support is good; consider a bit more validationThe new
generatedhandling incolumn_options/2cleanly maps:
generated: "expr"→GENERATED ALWAYS AS (expr)(virtual), andgenerated: "expr", stored: true→GENERATED ALWAYS AS (expr) STORED,and is reused consistently for CREATE TABLE and ALTER TABLE paths. That’s a solid extension of the existing DDL helpers. (sqlite.org)
Two small robustness improvements you might consider:
- Guard against
generatedanddefaultbeing set on the same column, since SQLite explicitly disallows defaults on generated columns and will error at runtime anyway. (sqlite.org)- Likewise, reject
generated: ...onprimary_key: true, as generated columns cannot participate in a PRIMARY KEY.Raising an
ArgumentErrorduring migration compilation for these combos would give users a clearer, earlier failure than a lower‑level SQLite error.Sketch of a defensive check
- defp column_options(opts, composite_pk) do + defp column_options(opts, composite_pk) do default = column_default(Keyword.get(opts, :default)) null = if Keyword.get(opts, :null) == false, do: " NOT NULL", else: "" @@ - generated = - case Keyword.get(opts, :generated) do + generated_opt = Keyword.get(opts, :generated) + + if generated_opt && Keyword.get(opts, :default) do + raise ArgumentError, + "generated columns cannot have a default value in SQLite" + end + + if generated_opt && Keyword.get(opts, :primary_key) && !composite_pk do + raise ArgumentError, + "generated columns cannot be used as PRIMARY KEY in SQLite" + end + + generated = + case generated_opt do nil -> ""test/security_test.exs (4)
1-3: Consider addingasync: falseexplicitly.The module uses
use ExUnit.Casewithout specifyingasync: trueorasync: false. While the default isasync: falseforExUnit.Case, explicitly setting it improves readability and prevents accidental changes. Given these tests involve database connections and file-based SQLite databases,async: falseis the safer choice.defmodule EctoLibSql.SecurityTest do - use ExUnit.Case + use ExUnit.Case, async: false
163-173: Loop variable shadowing may cause confusion.The loop rebinds
stateon each iteration, but the returnedstateon line 173 comes from the outer scope (line 152), not the loop. While functionally correct (the inserts don't need to propagate state changes), this pattern can confuse readers. Consider using_statein the loop to signal intent:for i <- 1..10 do - {:ok, _, _, state} = + {:ok, _, _, _state} = EctoLibSql.handle_execute( "INSERT INTO test_data (value) VALUES (?)", ["value_#{i}"], [], state ) end
258-263: Cleanup pattern matching could be more defensive.The rollback calls don't pattern match on the result, which could hide errors during cleanup. Consider using pattern matching to ensure cleanup succeeds:
# Cleanup - EctoLibSql.handle_rollback([], state_a) - EctoLibSql.handle_rollback([], state_b) + _ = EctoLibSql.handle_rollback([], state_a) + _ = EctoLibSql.handle_rollback([], state_b) EctoLibSql.disconnect([], state_a) EctoLibSql.disconnect([], state_b)Alternatively, wrap in a try/after block to ensure disconnect always runs.
400-409: Unused variable and incomplete verification.The
stmt_idvariable is prepared but never used. The test also doesn't verify that resources are actually inaccessible after disconnect. Consider either:
- Using the prepared statement before disconnect, then verifying it fails after
- Removing the unused
stmt_idif the intent is just to verify disconnect doesn't crash- {:ok, stmt_id} = EctoLibSql.Native.prepare(state, "SELECT * FROM cleanup_test") + {:ok, _stmt_id} = EctoLibSql.Native.prepare(state, "SELECT * FROM cleanup_test") # Close connection :ok = EctoLibSql.disconnect([], state) - # Resources should not be accessible (they belong to a closed connection) - # This is more of a manual verification - in production would need monitoring - # For now, just verify that closing doesn't crash + # Verify that closing doesn't crash - resource cleanup is handled internally assert truetest/named_parameters_execution_test.exs (1)
460-480: Case-sensitivity test could be more explicit.The test comment says "Parameter names should be case-sensitive" but doesn't actually test case sensitivity by using mixed-case parameter names. Consider adding a test that explicitly verifies case matters:
test "Named parameters are case-sensitive", %{state: state} do # Insert with lowercase parameter names {:ok, _, _, state} = EctoLibSql.handle_execute( "INSERT INTO users (id, name, email, age) VALUES (:id, :name, :email, :age)", %{id: 1, name: "Test", email: "[email protected]", age: 25}, [], state ) # Query with uppercase key should fail (assuming case-sensitivity) result = EctoLibSql.handle_execute( "SELECT * FROM users WHERE id = :id", %{ID: 1}, # Uppercase key [], state ) assert match?({:error, _, _}, result) end
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.beads/issues.jsonl.claude/settings.local.jsonAGENTS.mdCHANGELOG.mdlib/ecto/adapters/libsql/connection.exlib/ecto_libsql/native.extest/ecto_migration_test.exstest/named_parameters_execution_test.exstest/security_test.exs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ex
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ex: Use prepared statements with automatic statement caching for repeated queries in EctoLibSql to improve performance (~10-15x faster than unprepared queries)
Use batch operations (batch/2, batch_transactional/2, or execute_batch_sql/2) instead of individual INSERT/UPDATE/DELETE statements for bulk data operations
Always pattern match on result tuples from EctoLibSql operations to handle {:ok, ...} and {:error, ...} cases, avoiding bare pattern matches that could crash the process
Use cursors and DBConnection.stream for memory-efficient processing of large result sets instead of loading all rows into memory
Enable foreign key constraints in SQLite using EctoLibSql.Pragma.enable_foreign_keys/1 for data integrity in production databases
Use transaction behaviours (deferred, immediate, exclusive, read_only) with EctoLibSql.Native.begin/2 to optimize for specific workload patterns and prevent writer starvation
Use Ecto changesets with proper validation and error handling instead of raw SQL for INSERT/UPDATE operations to leverage Ecto's safety features
Use savepoints (EctoLibSql.Native.create_savepoint/2) within transactions for granular error recovery on partial batch failures
Use Ecto's insert_all/3 for bulk inserts of pre-mapped data to leverage batching and achieve better performance than individual inserts
Configure WAL (Write-Ahead Logging) mode using EctoLibSql.Pragma.set_journal_mode/2 for better concurrency and write performance in local databases
Use vector similarity search with EctoLibSql.Native.vector_distance_cos/2 for semantic search features in AI/ML applications
Encrypt local databases and replicas using encryption_key parameter (minimum 32 characters) for protecting sensitive data at rest
Use statement introspection functions (stmt_parameter_count, stmt_column_count, stmt_column_name) to validate prepared statement structure before execution
Files:
lib/ecto_libsql/native.exlib/ecto/adapters/libsql/connection.ex
AGENTS.md
📄 CodeRabbit inference engine (AGENT.md)
Document agent architecture and design patterns in AGENTS.md
Files:
AGENTS.md
🧠 Learnings (22)
📓 Common learnings
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to priv/repo/migrations/*.exs : Use RANDOM ROWID in table definitions (with options: [random_rowid: true]) for security-sensitive tables to prevent ID enumeration attacks
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: When adding SQLite function support, update `lib/ecto/adapters/libsql/connection.ex` with the expression handler and add corresponding tests
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: When adding SQLite function support, update `lib/ecto/adapters/libsql/connection.ex` with the expression handler and add corresponding tests
Applied to files:
CHANGELOG.mdtest/security_test.exstest/named_parameters_execution_test.exstest/ecto_migration_test.exs.beads/issues.jsonllib/ecto_libsql/native.exlib/ecto/adapters/libsql/connection.exAGENTS.md
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use Ecto changesets with proper validation and error handling instead of raw SQL for INSERT/UPDATE operations to leverage Ecto's safety features
Applied to files:
CHANGELOG.mdtest/security_test.exstest/named_parameters_execution_test.exstest/ecto_migration_test.exslib/ecto/adapters/libsql/connection.exAGENTS.md
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use prepared statements with automatic statement caching for repeated queries in EctoLibSql to improve performance (~10-15x faster than unprepared queries)
Applied to files:
CHANGELOG.mdtest/security_test.exstest/named_parameters_execution_test.exslib/ecto_libsql/native.exAGENTS.md
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Enable foreign key constraints in SQLite using EctoLibSql.Pragma.enable_foreign_keys/1 for data integrity in production databases
Applied to files:
CHANGELOG.mdtest/ecto_migration_test.exsAGENTS.md
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use transaction behaviours (deferred, immediate, exclusive, read_only) with EctoLibSql.Native.begin/2 to optimize for specific workload patterns and prevent writer starvation
Applied to files:
CHANGELOG.mdtest/security_test.exstest/named_parameters_execution_test.exslib/ecto_libsql/native.exAGENTS.md
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use savepoints (EctoLibSql.Native.create_savepoint/2) within transactions for granular error recovery on partial batch failures
Applied to files:
CHANGELOG.mdtest/security_test.exstest/named_parameters_execution_test.exs.beads/issues.jsonlAGENTS.md
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : NEVER use `.unwrap()` in production Rust code - use `safe_lock` helpers instead. Tests MAY use `.unwrap()` for simplicity
Applied to files:
test/security_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust async code, drop locks before calling `TOKIO_RUNTIME.block_on()` to prevent deadlocks
Applied to files:
test/security_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust, use `safe_lock_arc(&client, "function_name client")` for locking Arc<Mutex<T>> instead of `.lock().unwrap()`
Applied to files:
test/security_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Always pattern match on result tuples from EctoLibSql operations to handle {:ok, ...} and {:error, ...} cases, avoiding bare pattern matches that could crash the process
Applied to files:
test/security_test.exstest/named_parameters_execution_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust, use `safe_lock(®ISTRY, "function_name context")` for locking registries instead of `.lock().unwrap()`
Applied to files:
test/security_test.exs
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Use proper transactions with timeout in Elixir: `Repo.transaction(fn -> ... end, timeout: 15_000)` to handle database locked errors
Applied to files:
test/security_test.exslib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to priv/repo/migrations/*.exs : Use RANDOM ROWID in table definitions (with options: [random_rowid: true]) for security-sensitive tables to prevent ID enumeration attacks
Applied to files:
test/ecto_migration_test.exslib/ecto/adapters/libsql/connection.exAGENTS.md
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to priv/repo/migrations/*.exs : Create appropriate indexes (simple, composite, covering) on frequently queried columns to optimize query performance
Applied to files:
test/ecto_migration_test.exslib/ecto/adapters/libsql/connection.exAGENTS.md
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Test coverage must include: happy path, error cases, edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Applied to files:
.beads/issues.jsonl
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: For type conversion issues, update loaders/dumpers in `lib/ecto/adapters/libsql.ex` following the pattern `loaders/dumpers` functions
Applied to files:
lib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Elixir modules must follow the structure: EctoLibSql (DBConnection protocol), EctoLibSql.Native (NIF wrappers), EctoLibSql.State (connection state), Ecto.Adapters.LibSql (main adapter), Ecto.Adapters.LibSql.Connection (SQL generation)
Applied to files:
lib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use batch operations (batch/2, batch_transactional/2, or execute_batch_sql/2) instead of individual INSERT/UPDATE/DELETE statements for bulk data operations
Applied to files:
lib/ecto_libsql/native.ex
📚 Learning: 2025-12-05T22:12:22.789Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENT.md:0-0
Timestamp: 2025-12-05T22:12:22.789Z
Learning: Maintain agent implementation documentation standards
Applied to files:
AGENTS.md
📚 Learning: 2025-12-05T22:12:22.789Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENT.md:0-0
Timestamp: 2025-12-05T22:12:22.789Z
Learning: Applies to AGENTS.md : Document agent architecture and design patterns in AGENTS.md
Applied to files:
AGENTS.md
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use statement introspection functions (stmt_parameter_count, stmt_column_count, stmt_column_name) to validate prepared statement structure before execution
Applied to files:
AGENTS.md
🧬 Code graph analysis (4)
test/security_test.exs (1)
lib/ecto_libsql.ex (8)
connect(77-108)handle_execute(148-160)handle_begin(169-174)handle_commit(177-179)handle_commit(188-198)disconnect(131-133)handle_declare(323-341)handle_fetch(272-299)
test/named_parameters_execution_test.exs (2)
lib/ecto_libsql.ex (1)
handle_execute(148-160)lib/ecto_libsql/native.ex (5)
begin(509-519)commit(534-538)rollback(551-555)prepare(632-640)close_stmt(716-718)
test/ecto_migration_test.exs (2)
lib/ecto/adapters/libsql/connection.ex (10)
execute_ddl(200-203)execute_ddl(205-208)execute_ddl(210-233)execute_ddl(235-243)execute_ddl(245-253)execute_ddl(255-258)execute_ddl(260-263)execute_ddl(265-268)execute_ddl(270-276)query(690-692)lib/ecto_libsql/native.ex (1)
query(373-422)
lib/ecto_libsql/native.ex (3)
native/ecto_libsql/src/statement.rs (3)
prepare_statement(30-71)statement_parameter_count(313-338)statement_parameter_name(361-391)native/ecto_libsql/src/query.rs (1)
query_args(33-116)native/ecto_libsql/src/transaction.rs (2)
query_with_trx_args(322-401)execute_with_transaction(283-307)
🪛 GitHub Actions: CI
test/named_parameters_execution_test.exs
[error] 419-428: mix format --check-formatted failed. The following lines are not formatted as per formatting rules: lines 419-428 in test/named_parameters_execution_test.exs. Run 'mix format' to fix formatting.
🪛 LanguageTool
AGENTS.md
[style] ~1492-~1492: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...oint values only - NULL - NULL values only (rarely used) Usage Examples: ```...
(ADVERB_REPETITION_PREMIUM)
[style] ~1644-~1644: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...oint values only - NULL - NULL values only (rarely used) Usage Examples: ```...
(ADVERB_REPETITION_PREMIUM)
🪛 markdownlint-cli2 (0.18.1)
AGENTS.md
1606-1606: Multiple headings with the same content
(MD024, no-duplicate-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (rust)
🔇 Additional comments (10)
.claude/settings.local.json (1)
46-47: Newgit checkoutpermission meaningfully increases agent write powerAllowing
Bash(git checkout:*)lets agents mutate the working tree (switch branches, reset paths, discard local changes). That’s probably fine for this repo, but it’s a bigger hammer than the existing read‑only git commands.Please just double‑check this broader capability is intentional in your threat model for agent commands.
CHANGELOG.md (1)
12-61: Named-parameters changelog entry is clear and well-scopedThe new “Named Parameters Execution Support” section accurately documents the three SQLite syntaxes, map-based binding, execution paths, and test coverage, and the examples match the public APIs used elsewhere in the docs.
Looks good as a release note for this feature set.
.beads/issues.jsonl (1)
1-32: Backlog metadata updates look consistent with implemented featuresThe enriched issue entries for extensions, RANDOM ROWID, ALTER COLUMN, cross-connection security tests, max_write_replication_index, named parameters, and STRICT tables are structurally valid JSONL and line up with the code and docs added in this PR. The embedded acceptance criteria will be useful for future traceability.
No changes needed here.
lib/ecto/adapters/libsql/connection.ex (1)
443-459: Suffix list approach for RANDOM ROWID and STRICT looks correctThe refactor to accumulate table suffixes in a
suffixeslist and thenEnum.join/1is a nice improvement over a single hard-coded suffix: it naturally supports combinations likeRANDOM ROWIDandSTRICTin either order and keeps the RANDOM ROWID validation logic unchanged.As long as your migration tests cover:
options: [random_rowid: true],options: [strict: true], andoptions: [random_rowid: true, strict: true],this should be safe and future‑proof if additional suffix options are added later.
test/ecto_migration_test.exs (2)
761-806: LGTM! Good coverage of libSQL-specific table options.The tests appropriately validate both SQL generation and actual table creation for
RANDOM ROWID. The STRICT test sensibly only checks SQL generation since older libSQL versions may not support it. Based on learnings, usingRANDOM ROWIDfor security-sensitive tables like sessions helps prevent ID enumeration attacks.
808-844: Good SQL generation tests for generated columns.The tests properly verify the DDL syntax for both virtual and stored generated columns. Consider adding execution tests in a follow-up to verify generated columns work correctly at runtime (insert rows, verify computed values).
test/named_parameters_execution_test.exs (1)
312-376: Good transaction coverage with named parameters.The tests properly verify that named parameters work within transactions and that commit/rollback semantics are preserved. The use of
initial_statevsstatecorrectly handles the state transitions during transaction lifecycle.lib/ecto_libsql/native.ex (3)
303-311: Clean implementation of prefix removal.The function correctly handles all three named parameter prefixes (
:,@,$) documented in the SQL syntax. TheString.slice(name, 1..-1//1)syntax is clear and idiomatic.
286-301: Good normalisation logic with sensible defaults.The function correctly handles the three expected cases: lists (positional), maps (named), and pass-through for anything else. The catch-all ensures forward compatibility but will let invalid types through to the execution layer where they'll produce an appropriate error.
378-381: Good integration of named parameter normalisation.The normalisation is consistently applied in both
query/3andexecute_with_trx/3paths, ensuring named parameters work identically in and out of transactions. The existing result handling is preserved.Also applies to: 430-431, 445-445, 474-474
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
lib/ecto/adapters/libsql/connection.ex (1)
374-409: Generated-column validation and table suffixes look solid; double-check GENERATED clause orderingThe new checks in
column_options/2that reject:generatedcombined with:defaultor:primary_keycorrectly enforce SQLite’s constraints on generated columns, and thegeneratedfragment itself is composed cleanly with optionalSTORED. The revisedtable_options/2logic that accumulates suffixes intosuffixes(allowingRANDOM ROWIDandSTRICTto co-exist) is also straightforward and reads well.One thing worth verifying against SQLite/libSQL’s exact DDL grammar is the ordering of
GENERATED ALWAYS AS (...)relative toNOT NULLand other column constraints, since you currently emitpk/null/defaultbeforegenerated. If SQLite expects the generated clause before constraints for computed columns, you may need to reorder those fragments to avoid DDL parse errors on real migrations.Also applies to: 418-475
test/named_parameters_execution_test.exs (2)
16-43: Consider explicitly disconnecting in setup teardownThe
on_exit/1block removes the temp DB and its WAL/SHM files, which is great, but it never callsEctoLibSql.disconnect/2. Explicitly disconnecting the connection here would keep test resource handling consistent with other modules (likePragmaTest) and avoid leaving open NIF resources until the VM shuts the test process down.
404-503: Edge-case coverage for named parameters is strongThe tests for missing parameters, extra ignored keys,
NULLvalues, and case sensitivity give good confidence around the new normalisation layer and error paths. Once you adjust the underlying implementation to fully support both atom and string keys (see comments onnormalize_arguments/3), these cases will provide a solid regression suite; consider adding one or two tests that pass string-key maps as well, since AGENTS.md documents that shape.test/ecto_migration_test.exs (1)
762-873: Good coverage of libSQL table options and generated columns; consider an execution-level checkThe new tests around
RANDOM ROWID,STRICT, and virtual/stored generated columns line up well with the DDL changes inConnection.execute_ddl/1and give nice regression coverage on the emitted SQL. At the moment the generated-column examples only assert on the SQL string without actually running the DDL; once you’re confident in the syntax ordering, adding a migration/execution test (e.g. creating the table and inserting/selecting across a generated column) would catch any subtle syntax incompatibilities with libSQL/SQLite that pure string assertions can’t.AGENTS.md (1)
1456-1548: STRICT table documentation is clear; tweak stmt_parameter_name notes for consistencyThe STRICT tables section gives a clear, practical overview of how to enable
strict: true, what types are allowed, and what the resulting SQL looks like; the combined example withrandom_rowid: truematches the new DDL behaviour in the adapter.One small consistency issue: in the earlier
stmt_parameter_name/3docs you still describe stripping the:,@, or$prefix and returning"id","name", etc, whereas the new tests now expect the raw parameter token (e.g.":id"). It’d be good to update that paragraph and the example values to match whichever behaviour you’ve standardised on in the NIF/Elixir wrapper so consumers don’t get surprised by the returned name format.test/security_test.exs (2)
4-9: Consider explicit error handling in cleanup helper.The
File.rm/1calls ignore all errors, including permission issues or filesystem errors. While this is often acceptable for test cleanup (we want cleanup to proceed even if files don't exist), it may silently mask real issues.🔎 Proposed refactor with explicit error handling
defp cleanup_db(db_path) do - File.rm(db_path) - File.rm(db_path <> "-wal") - File.rm(db_path <> "-shm") + Enum.each([db_path, db_path <> "-wal", db_path <> "-shm"], fn path -> + case File.rm(path) do + :ok -> :ok + {:error, :enoent} -> :ok + {:error, reason} -> IO.warn("Failed to cleanup #{path}: #{inspect(reason)}") + end + end) end
1-2: Consider usingasync: truefor independent test isolation.The suite sets
async: falseat line 2, which serializes all tests. While some tests check concurrent behaviour and isolation, most describe blocks use unique database files per test (viaSystem.unique_integer()), suggesting tests could run in parallel without interference.Consider:
- Using
async: trueto speed up test execution- If specific tests require serialization, use
@tag :serialon those tests only- Ensure proper cleanup in all tests to prevent cross-test pollution
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
AGENTS.mdlib/ecto/adapters/libsql/connection.exlib/ecto_libsql/native.extest/ecto_migration_test.exstest/error_handling_test.exstest/named_parameters_execution_test.exstest/pragma_test.exstest/security_test.exs
🧰 Additional context used
📓 Path-based instructions (2)
AGENTS.md
📄 CodeRabbit inference engine (AGENT.md)
Document agent architecture and design patterns in AGENTS.md
Files:
AGENTS.md
**/*.ex
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ex: Use prepared statements with automatic statement caching for repeated queries in EctoLibSql to improve performance (~10-15x faster than unprepared queries)
Use batch operations (batch/2, batch_transactional/2, or execute_batch_sql/2) instead of individual INSERT/UPDATE/DELETE statements for bulk data operations
Always pattern match on result tuples from EctoLibSql operations to handle {:ok, ...} and {:error, ...} cases, avoiding bare pattern matches that could crash the process
Use cursors and DBConnection.stream for memory-efficient processing of large result sets instead of loading all rows into memory
Enable foreign key constraints in SQLite using EctoLibSql.Pragma.enable_foreign_keys/1 for data integrity in production databases
Use transaction behaviours (deferred, immediate, exclusive, read_only) with EctoLibSql.Native.begin/2 to optimize for specific workload patterns and prevent writer starvation
Use Ecto changesets with proper validation and error handling instead of raw SQL for INSERT/UPDATE operations to leverage Ecto's safety features
Use savepoints (EctoLibSql.Native.create_savepoint/2) within transactions for granular error recovery on partial batch failures
Use Ecto's insert_all/3 for bulk inserts of pre-mapped data to leverage batching and achieve better performance than individual inserts
Configure WAL (Write-Ahead Logging) mode using EctoLibSql.Pragma.set_journal_mode/2 for better concurrency and write performance in local databases
Use vector similarity search with EctoLibSql.Native.vector_distance_cos/2 for semantic search features in AI/ML applications
Encrypt local databases and replicas using encryption_key parameter (minimum 32 characters) for protecting sensitive data at rest
Use statement introspection functions (stmt_parameter_count, stmt_column_count, stmt_column_name) to validate prepared statement structure before execution
Files:
lib/ecto/adapters/libsql/connection.exlib/ecto_libsql/native.ex
🧠 Learnings (21)
📚 Learning: 2025-12-05T22:12:22.789Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENT.md:0-0
Timestamp: 2025-12-05T22:12:22.789Z
Learning: Maintain agent implementation documentation standards
Applied to files:
AGENTS.md
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use prepared statements with automatic statement caching for repeated queries in EctoLibSql to improve performance (~10-15x faster than unprepared queries)
Applied to files:
AGENTS.mdtest/named_parameters_execution_test.exslib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use transaction behaviours (deferred, immediate, exclusive, read_only) with EctoLibSql.Native.begin/2 to optimize for specific workload patterns and prevent writer starvation
Applied to files:
AGENTS.mdtest/named_parameters_execution_test.exstest/security_test.exslib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: When adding SQLite function support, update `lib/ecto/adapters/libsql/connection.ex` with the expression handler and add corresponding tests
Applied to files:
AGENTS.mdtest/named_parameters_execution_test.exstest/ecto_migration_test.exslib/ecto/adapters/libsql/connection.extest/pragma_test.exstest/error_handling_test.exslib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Elixir modules must follow the structure: EctoLibSql (DBConnection protocol), EctoLibSql.Native (NIF wrappers), EctoLibSql.State (connection state), Ecto.Adapters.LibSql (main adapter), Ecto.Adapters.LibSql.Connection (SQL generation)
Applied to files:
AGENTS.mdtest/named_parameters_execution_test.exslib/ecto/adapters/libsql/connection.extest/pragma_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use statement introspection functions (stmt_parameter_count, stmt_column_count, stmt_column_name) to validate prepared statement structure before execution
Applied to files:
AGENTS.mdlib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use Ecto changesets with proper validation and error handling instead of raw SQL for INSERT/UPDATE operations to leverage Ecto's safety features
Applied to files:
AGENTS.mdtest/named_parameters_execution_test.exstest/ecto_migration_test.exslib/ecto/adapters/libsql/connection.extest/error_handling_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to priv/repo/migrations/*.exs : Use RANDOM ROWID in table definitions (with options: [random_rowid: true]) for security-sensitive tables to prevent ID enumeration attacks
Applied to files:
AGENTS.mdtest/ecto_migration_test.exslib/ecto/adapters/libsql/connection.ex
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Enable foreign key constraints in SQLite using EctoLibSql.Pragma.enable_foreign_keys/1 for data integrity in production databases
Applied to files:
AGENTS.mdtest/ecto_migration_test.exslib/ecto/adapters/libsql/connection.ex
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use savepoints (EctoLibSql.Native.create_savepoint/2) within transactions for granular error recovery on partial batch failures
Applied to files:
AGENTS.mdtest/named_parameters_execution_test.exstest/error_handling_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to priv/repo/migrations/*.exs : Create appropriate indexes (simple, composite, covering) on frequently queried columns to optimize query performance
Applied to files:
AGENTS.mdtest/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Always pattern match on result tuples from EctoLibSql operations to handle {:ok, ...} and {:error, ...} cases, avoiding bare pattern matches that could crash the process
Applied to files:
test/named_parameters_execution_test.exstest/error_handling_test.exslib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: For type conversion issues, update loaders/dumpers in `lib/ecto/adapters/libsql.ex` following the pattern `loaders/dumpers` functions
Applied to files:
lib/ecto/adapters/libsql/connection.ex
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Configure WAL (Write-Ahead Logging) mode using EctoLibSql.Pragma.set_journal_mode/2 for better concurrency and write performance in local databases
Applied to files:
test/pragma_test.exstest/error_handling_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust async code, drop locks before calling `TOKIO_RUNTIME.block_on()` to prevent deadlocks
Applied to files:
test/pragma_test.exstest/error_handling_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : NEVER use `.unwrap()` in production Rust code - use `safe_lock` helpers instead. Tests MAY use `.unwrap()` for simplicity
Applied to files:
test/pragma_test.exstest/error_handling_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Use proper transactions with timeout in Elixir: `Repo.transaction(fn -> ... end, timeout: 15_000)` to handle database locked errors
Applied to files:
test/error_handling_test.exstest/security_test.exslib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust, use `safe_lock(®ISTRY, "function_name context")` for locking registries instead of `.lock().unwrap()`
Applied to files:
test/error_handling_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Run Elixir tests with `mix test`, Rust tests with `cd native/ecto_libsql && cargo test`, and use `--trace` flag for debugging specific tests
Applied to files:
test/error_handling_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust, use `safe_lock_arc(&client, "function_name client")` for locking Arc<Mutex<T>> instead of `.lock().unwrap()`
Applied to files:
test/security_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use batch operations (batch/2, batch_transactional/2, or execute_batch_sql/2) instead of individual INSERT/UPDATE/DELETE statements for bulk data operations
Applied to files:
lib/ecto_libsql/native.ex
🧬 Code graph analysis (3)
test/named_parameters_execution_test.exs (2)
lib/ecto_libsql.ex (1)
handle_execute(148-160)lib/ecto_libsql/native.ex (5)
begin(566-576)commit(591-595)rollback(608-612)prepare(689-697)close_stmt(773-775)
test/ecto_migration_test.exs (1)
lib/ecto/adapters/libsql/connection.ex (9)
execute_ddl(200-203)execute_ddl(205-208)execute_ddl(210-233)execute_ddl(235-243)execute_ddl(245-253)execute_ddl(255-258)execute_ddl(260-263)execute_ddl(265-268)execute_ddl(270-276)
test/security_test.exs (2)
lib/ecto_libsql.ex (9)
connect(77-108)handle_execute(148-160)handle_begin(169-174)handle_commit(177-179)handle_commit(188-198)disconnect(131-133)handle_declare(323-341)handle_fetch(272-299)handle_rollback(207-215)lib/ecto_libsql/native.ex (5)
prepare(689-697)query_stmt(741-760)close_stmt(773-775)create_savepoint(1498-1500)rollback_to_savepoint_by_name(1570-1572)
🪛 LanguageTool
AGENTS.md
[style] ~1494-~1494: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...oint values only - NULL - NULL values only (rarely used) Usage Examples: ```...
(ADVERB_REPETITION_PREMIUM)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / ubuntu-latest
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / macos-latest
- GitHub Check: Analyze (rust)
🔇 Additional comments (6)
test/pragma_test.exs (1)
258-280: Nice WAL/SHM cleanup for the second connectionExtending teardown to remove
-waland-shmfortest_db2keeps the test sandbox tidy and avoids leaking ancillary SQLite files between runs. Looks good.test/error_handling_test.exs (1)
185-226: Consistent cleanup of WAL/SHM artefactsAdding
File.rm(test_db <> "-wal")andFile.rm(test_db <> "-shm")in the concurrent test’s teardown aligns this test with others and avoids stray SQLite files, even if/when the tag is removed and the test is enabled.test/named_parameters_execution_test.exs (1)
380-401: The documentation in AGENTS.md already correctly specifies thatstmt_parameter_name/3returns names WITH their prefixes (:name,@name,$name), which aligns with the test expectations. The inline comments inlib/ecto_libsql/native.ex(showing unprefixed names like"id") are the outdated documentation that should be corrected to match the actual implementation and public API documentation.Likely an incorrect or invalid review comment.
AGENTS.md (1)
488-597: Update documentation to match actual parameter handling behaviourThe "Named Parameters" section shows examples that don't align with the current implementation:
Examples use string-keyed maps (
%{"email" => ...}) buthandle_execute/4expects atom-keyed maps (%{email: ...}) after the normalisation layer strips parameter prefixes and converts keys to atoms. String-keyed maps will silently receivenilvalues.Examples pass maps to prepared statement functions like
query_stmt(state, stmt_id, %{"email" => ...})but these functions don't support map arguments. They expect lists and pass arguments directly to NIFs that don't include the normalisation layer. The documentation should clarify that prepared statements currently accept lists only.Update the examples to either:
- Use atom-keyed maps for
handle_execute/4and lists forquery_stmt/3andexecute_stmt/4, or- Extend prepared statement helpers to also support map-based named parameters with the same normalisation logic, and confirm that both atom- and string-keyed maps are supported as advertised.
test/security_test.exs (2)
328-359: No issues identified. The test correctly validates that concurrent cursor fetches are safe due to mutex-protected access in the Rust implementation, and the manual disconnect call does not interfere with ExUnit'son_exitcleanup callback.
456-460: Remove this comment—the test assertion is correct.The cursor properly returns
{[], [], 0}after disconnect because cursors are not automatically cleaned up when a connection closes. Unlike prepared statements (which validate the connection still exists),fetch_cursoronly verifies cursor ownership, not connection validity. The tuple{[], [], 0}correctly indicates the cursor is exhausted, not that it was cleaned up. Cursors must be explicitly closed via a separate operation.
- Fix struct update pattern in security_test.exs (use map update after pattern match assertion) - Replace unreachable :deallocated clause with :halt in cursor test - Fix unused variable warnings (_state and _i) - Improve test cleanup to properly stop repo before file removal - Add -journal file cleanup to prevent stale files
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
test/security_test.exs (5)
44-59: Overly lenient assertion weakens test validation.This case statement still accepts both error and success outcomes, which defeats the purpose of the isolation test. The test should definitively expect one outcome.
🔎 Recommended fix
case EctoLibSql.handle_execute( "SELECT 1", [], [], state_b_fake ) do {:error, _reason, _state} -> # Expected - transaction belongs to connection A assert true {:ok, _, _, _} -> - # If execution succeeds, the system should prevent the transaction - # from being used across connections anyway. The key is no crash. - # SQLite will likely error on the transaction ID being invalid - assert true + flunk("Connection B should not be able to use connection A's transaction") end
147-149: Manual disconnect of setup-provided state breaks subsequent tests.Line 147 disconnects
state_awhich is provided by the setup block (line 120 shows it comes from setup context). This will cause issues if other tests in this describe block attempt to use the shared connection. Only disconnect resources created within the test (state_b).🔎 Recommended fix
# Cleanup EctoLibSql.Native.close_stmt(stmt_id_a) - EctoLibSql.disconnect([], state_a) EctoLibSql.disconnect([], state_b) cleanup_db(db_path_b)
166-168: Manual disconnect of setup-provided state breaks subsequent tests.Line 167 disconnects
statewhich comes from the setup block. Remove the manual disconnect and let the test framework handle cleanup.🔎 Recommended fix
{:ok, _} -> flunk("Should not be able to use a closed statement") end - - EctoLibSql.disconnect([], state) end
242-244: Manual disconnect of setup-provided state breaks subsequent tests.Line 242 disconnects
state_awhich is provided by the setup block. Only disconnect resources created within the test (state_b).🔎 Recommended fix
- EctoLibSql.disconnect([], state_a) EctoLibSql.disconnect([], state_b) cleanup_db(db_path_b)
412-414: Manual disconnect of setup-provided state breaks subsequent tests.Line 412 disconnects
state_awhich is provided by the setup block. Only disconnect resources created within the test (state_b).🔎 Recommended fix
# Cleanup EctoLibSql.handle_commit([], state_a) EctoLibSql.handle_commit([], state_b) - EctoLibSql.disconnect([], state_a) EctoLibSql.disconnect([], state_b) cleanup_db(db_path_b)
🧹 Nitpick comments (5)
test/security_test.exs (3)
185-193: State variable not threaded through loop iterations.The
forcomprehension captures the result in_state(discarded), but continues using the originalstatevariable. While this may work for SQLite due to autocommit semantics, it's inconsistent with the pattern used elsewhere in the codebase where state is explicitly updated.🔎 Suggested improvement
- for i <- 1..10 do - {:ok, _, _, _state} = - EctoLibSql.handle_execute( - "INSERT INTO test_data (value) VALUES (?)", - ["value_#{i}"], - [], - state - ) - end + Enum.each(1..10, fn i -> + {:ok, _, _, _} = + EctoLibSql.handle_execute( + "INSERT INTO test_data (value) VALUES (?)", + ["value_#{i}"], + [], + state + ) + end)Using
Enum.each/2makes it explicit that the result isn't being accumulated.
249-249: Empty map pattern in test context is unusual.The test signature uses
%{}which doesn't extract any values from the setup context. If this test doesn't require setup context, consider removing the pattern match entirely, or verify the test belongs in a describe block without shared setup.- test "savepoint belongs to owning transaction", %{} do + test "savepoint belongs to owning transaction" do
312-320: State variable not threaded through loop iterations.Same issue as line 185-193 — consider using
Enum.each/2to make the intent explicit.test/ecto_migration_test.exs (2)
818-834: Consider executing generated column SQL to validate runtime behaviour.The test only verifies SQL generation. While the
!String.contains?(sql, "STORED")assertion is correct, usingrefute sql =~ "STORED"would be more idiomatic in ExUnit.🔎 Suggested improvement
# Verify GENERATED clause appears in SQL (but not STORED) assert sql =~ "GENERATED ALWAYS AS" assert sql =~ "first_name || ' ' || last_name" - assert !String.contains?(sql, "STORED") + refute sql =~ "STORED"
799-814: Consider documenting minimum libSQL version requirement for STRICT support.SQLite 3.37.0+ (and thus libSQL builds based on that version) supports STRICT tables. The test verifies SQL generation only—adding a comment specifying the minimum required libSQL version or Ecto version would help future maintainers understand the compatibility requirement. This is optional since the project already uses libSQL 0.9.29 which fully supports STRICT.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
test/ecto_migration_test.exstest/fuzz_test.exstest/security_test.exs
✅ Files skipped from review due to trivial changes (1)
- test/fuzz_test.exs
🧰 Additional context used
🧠 Learnings (18)
📓 Common learnings
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use statement introspection functions (stmt_parameter_count, stmt_column_count, stmt_column_name) to validate prepared statement structure before execution
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use prepared statements with automatic statement caching for repeated queries in EctoLibSql to improve performance (~10-15x faster than unprepared queries)
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: When adding SQLite function support, update `lib/ecto/adapters/libsql/connection.ex` with the expression handler and add corresponding tests
Applied to files:
test/security_test.exstest/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use Ecto changesets with proper validation and error handling instead of raw SQL for INSERT/UPDATE operations to leverage Ecto's safety features
Applied to files:
test/security_test.exstest/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : NEVER use `.unwrap()` in production Rust code - use `safe_lock` helpers instead. Tests MAY use `.unwrap()` for simplicity
Applied to files:
test/security_test.exstest/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use savepoints (EctoLibSql.Native.create_savepoint/2) within transactions for granular error recovery on partial batch failures
Applied to files:
test/security_test.exstest/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust, use `safe_lock_arc(&client, "function_name client")` for locking Arc<Mutex<T>> instead of `.lock().unwrap()`
Applied to files:
test/security_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use transaction behaviours (deferred, immediate, exclusive, read_only) with EctoLibSql.Native.begin/2 to optimize for specific workload patterns and prevent writer starvation
Applied to files:
test/security_test.exs
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Use proper transactions with timeout in Elixir: `Repo.transaction(fn -> ... end, timeout: 15_000)` to handle database locked errors
Applied to files:
test/security_test.exstest/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust async code, drop locks before calling `TOKIO_RUNTIME.block_on()` to prevent deadlocks
Applied to files:
test/security_test.exstest/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Always pattern match on result tuples from EctoLibSql operations to handle {:ok, ...} and {:error, ...} cases, avoiding bare pattern matches that could crash the process
Applied to files:
test/security_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use prepared statements with automatic statement caching for repeated queries in EctoLibSql to improve performance (~10-15x faster than unprepared queries)
Applied to files:
test/security_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust, use `safe_lock(®ISTRY, "function_name context")` for locking registries instead of `.lock().unwrap()`
Applied to files:
test/security_test.exstest/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to priv/repo/migrations/*.exs : Create appropriate indexes (simple, composite, covering) on frequently queried columns to optimize query performance
Applied to files:
test/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to priv/repo/migrations/*.exs : Use RANDOM ROWID in table definitions (with options: [random_rowid: true]) for security-sensitive tables to prevent ID enumeration attacks
Applied to files:
test/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: For type conversion issues, update loaders/dumpers in `lib/ecto/adapters/libsql.ex` following the pattern `loaders/dumpers` functions
Applied to files:
test/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Run Elixir tests with `mix test`, Rust tests with `cd native/ecto_libsql && cargo test`, and use `--trace` flag for debugging specific tests
Applied to files:
test/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Enable foreign key constraints in SQLite using EctoLibSql.Pragma.enable_foreign_keys/1 for data integrity in production databases
Applied to files:
test/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Configure WAL (Write-Ahead Logging) mode using EctoLibSql.Pragma.set_journal_mode/2 for better concurrency and write performance in local databases
Applied to files:
test/ecto_migration_test.exs
🧬 Code graph analysis (2)
test/security_test.exs (2)
lib/ecto_libsql.ex (9)
connect(77-108)handle_execute(148-160)handle_begin(169-174)handle_commit(177-179)handle_commit(188-198)disconnect(131-133)handle_declare(323-341)handle_fetch(272-299)handle_rollback(207-215)lib/ecto_libsql/native.ex (5)
prepare(689-697)query_stmt(741-760)close_stmt(773-775)create_savepoint(1498-1500)rollback_to_savepoint_by_name(1570-1572)
test/ecto_migration_test.exs (1)
lib/ecto/adapters/libsql/connection.ex (10)
execute_ddl(200-203)execute_ddl(205-208)execute_ddl(210-233)execute_ddl(235-243)execute_ddl(245-253)execute_ddl(255-258)execute_ddl(260-263)execute_ddl(265-268)execute_ddl(270-276)query(703-705)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / macos-latest
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / ubuntu-latest
- GitHub Check: Analyze (rust)
🔇 Additional comments (8)
test/security_test.exs (4)
4-9: LGTM!Clean helper function for removing SQLite database files and associated artifacts (WAL, SHM).
494-564: Good test coverage for pool-like isolation.This test validates that separate connections to the same database file maintain independent transaction contexts. The approach of committing conn1 first before conn2's insert correctly handles SQLite's write serialisation.
567-627: Good isolation test for separate database files.Validates that connections to different database files are completely isolated at the schema level.
456-461: Test expectation does not match implementation behaviour.The test expects
{[], [], 0}after disconnect, but the Rustfetch_cursorimplementation (cursor.rs:297–301) will return(columns, [], 0)— it includes the column names even when no rows remain. Cursors are not removed from the registry when a connection closes, so the cursor ownership check succeeds and returns the stored columns rather than an empty array.The inconsistency with statement behaviour (which returns
{:error, _}after disconnect) remains valid: statements are designed to fail after connection loss, whilst cursors return the column metadata with zero rows. However, the test assertion should be updated to match the actual implementation:{columns, [], 0}wherecolumnsis not empty but contains the result column names.test/ecto_migration_test.exs (4)
14-37: Good improvements to test setup/teardown.The enhanced teardown properly:
- Checks process liveness before stopping
- Adds a delay to ensure file handles are released
- Cleans up journal file in addition to WAL/SHM
This aligns with the broader cleanup enhancements seen in other test files.
771-797: Good test coverage for RANDOM ROWID libSQL extension.The test correctly verifies both SQL generation and actual execution, then confirms the table schema in
sqlite_master. Based on learnings, RANDOM ROWID is recommended for security-sensitive tables to prevent ID enumeration attacks.
836-852: LGTM!Good test for stored generated columns. The assertions correctly verify the presence of
GENERATED ALWAYS AS,STORED, and the expression.
854-881: Good constraint validation tests.These tests properly verify that the adapter rejects invalid column configurations at DDL generation time:
- Generated columns with DEFAULT values
- Generated columns as primary keys
Both are SQLite constraints that are correctly enforced early.
Addresses CodeRabbit review comment: query_stmt and execute_stmt now properly normalise map arguments to positional lists using stmt introspection. - Add normalise_arguments_for_stmt/3 for prepared statement parameter conversion - Update execute_stmt/4 to normalise args before execution - Update query_stmt/3 to normalise args before query - Add comprehensive tests for named parameters with prepared statements - Update documentation to reflect both positional and named parameter support - Use British English spelling (normalise vs normalize) per project convention
Change the test to assert that using connection A's transaction on connection B fails with an error, rather than accepting both success and error outcomes. This ensures the security test actually validates that cross-connection transaction usage is rejected.
…test The test was disconnecting state_a which is the shared connection from setup, potentially causing issues. Now only per-test resources (state_b) are managed in the test, leaving the shared state to setup/teardown.
Security tests: - Remove manual disconnects of shared setup-provided state - Make cross-connection isolation test strict (assert error, not accept both) - Tests now properly manage only per-test resources Credo fixes: - Replace String.to_atom/1 with String.to_existing_atom/1 to avoid creating atoms at runtime (security best practice) - Add get_map_value_flexible/2 helper to support both atom and string keys in parameter maps - Parameter names now stored as strings internally, with flexible lookup
Fixes compiler warning that was causing CI to fail with --warnings-as-errors.
Use Enum.reduce instead of for comprehension to properly thread the state variable through each INSERT operation, consistent with patterns used elsewhere in the codebase.
- Remove unnecessary %{} pattern match from savepoint test that has no
shared setup context
- Fix state threading in concurrent access setup using Enum.reduce
Replace assert !String.contains?(sql, "STORED") with the more idiomatic ExUnit refute sql =~ "STORED" pattern.
Apply consistent anonymous `_` pattern for unused variables across test
files to satisfy Credo's consistency checks:
- security_test.exs: Simplify patterns like `{:error, _reason, _state}`
to `{:error, _, _}`, remove unused `db_path` from test context params
- fuzz_test.exs: Replace named unused vars (_state, _reason, _query, _e,
_count, _result, _final_state) with anonymous `_`, fix number format
(10000 → 10_000)
- named_parameters_execution_test.exs: Standardise `_state` → `_`
All 487 tests pass, Credo reports no consistency issues.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
test/security_test.exs (1)
4-9: Tightened security tests look good; consider also cleaning-journalfiles.The new isolation tests (transactions, statements, cursors, savepoints, pools, cross‑DB) now assert failures explicitly and avoid double‑disconnecting shared state, which is a solid improvement for security validation. The only minor thing:
cleanup_db/1currently removes the main file,-wal, and-shmbut not-journal; adding that would align with other tests and avoid stray journal files on non‑WAL setups.Also applies to: 567-627
lib/ecto_libsql/native.ex (2)
286-414: Named-parameter normalisation and caching are well-structured; watch unboundedpersistent_termgrowth.The
normalise_arguments/3/normalise_arguments_for_stmt/3pipeline plus parameter‑name caching gives a clean story for converting map args (including mixed:name,@name,$namesyntaxes) into positional lists, and the flexible atom/string key lookup avoids runtime atom creation. The only thing to keep in mind long‑term is that@param_cache_keystores a global map in:persistent_termkeyed by raw SQL; in workloads that generate many distinct statements, this could grow without bound. If you later see very dynamic SQL usage, consider capping this cache or switching to an ETS‑backed structure with eviction, but it’s fine as‑is for typical Ecto workloads.Also applies to: 420-459
366-380: Consider propagating parameter-introspection failures instead of treating them as zero-parameter statements.In
introspect_and_cache_params/3, any non‑integer result fromstatement_parameter_count/2falls back to0, which then yields an emptyparam_nameslist and an empty positional args list. Ifstatement_parameter_count/2ever returns{:error, reason}(e.g. invalidstmt_id), the calling code will silently proceed with no parameters rather than surfacing the error.Given you already propagate preparation failures as
{:error, reason}andquery/3/execute_with_trx/3turn{:error, reason}fromnormalise_arguments/3into a clearEctoLibSql.Error, it would be more consistent to treat{:error, reason}fromstatement_parameter_count/2similarly instead of coercing it to0. That keeps mis‑bound or invalid statements from being hidden behind confusing SQL errors.Also applies to: 410-414, 472-483, 539-548
test/named_parameters_execution_test.exs (1)
16-43: Comprehensive named-parameter tests; optionally align DB cleanup with other suites.The coverage here is excellent: all three named syntaxes, positional fallback, transactions, prepared statements, and edge cases (missing/extra params, NULL, case sensitivity) map cleanly onto the new normalisation behaviour in
EctoLibSql.Native. One small consistency tweak you might consider is extending theon_exitcleanup to removedb_name <> "-journal"as well, mirroring other tests, so SQLite journal files don’t linger on non‑WAL configurations.Also applies to: 380-577
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.beads/issues.jsonl.claude/settings.local.jsonlib/ecto_libsql/native.extest/ecto_migration_test.exstest/fuzz_test.exstest/named_parameters_execution_test.exstest/security_test.exs
🚧 Files skipped from review as they are similar to previous changes (1)
- .claude/settings.local.json
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ex
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ex: Use prepared statements with automatic statement caching for repeated queries in EctoLibSql to improve performance (~10-15x faster than unprepared queries)
Use batch operations (batch/2, batch_transactional/2, or execute_batch_sql/2) instead of individual INSERT/UPDATE/DELETE statements for bulk data operations
Always pattern match on result tuples from EctoLibSql operations to handle {:ok, ...} and {:error, ...} cases, avoiding bare pattern matches that could crash the process
Use cursors and DBConnection.stream for memory-efficient processing of large result sets instead of loading all rows into memory
Enable foreign key constraints in SQLite using EctoLibSql.Pragma.enable_foreign_keys/1 for data integrity in production databases
Use transaction behaviours (deferred, immediate, exclusive, read_only) with EctoLibSql.Native.begin/2 to optimize for specific workload patterns and prevent writer starvation
Use Ecto changesets with proper validation and error handling instead of raw SQL for INSERT/UPDATE operations to leverage Ecto's safety features
Use savepoints (EctoLibSql.Native.create_savepoint/2) within transactions for granular error recovery on partial batch failures
Use Ecto's insert_all/3 for bulk inserts of pre-mapped data to leverage batching and achieve better performance than individual inserts
Configure WAL (Write-Ahead Logging) mode using EctoLibSql.Pragma.set_journal_mode/2 for better concurrency and write performance in local databases
Use vector similarity search with EctoLibSql.Native.vector_distance_cos/2 for semantic search features in AI/ML applications
Encrypt local databases and replicas using encryption_key parameter (minimum 32 characters) for protecting sensitive data at rest
Use statement introspection functions (stmt_parameter_count, stmt_column_count, stmt_column_name) to validate prepared statement structure before execution
Files:
lib/ecto_libsql/native.ex
🧠 Learnings (23)
📓 Common learnings
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use prepared statements with automatic statement caching for repeated queries in EctoLibSql to improve performance (~10-15x faster than unprepared queries)
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: When adding SQLite function support, update `lib/ecto/adapters/libsql/connection.ex` with the expression handler and add corresponding tests
Applied to files:
test/fuzz_test.exslib/ecto_libsql/native.extest/named_parameters_execution_test.exs.beads/issues.jsonltest/ecto_migration_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Elixir modules must follow the structure: EctoLibSql (DBConnection protocol), EctoLibSql.Native (NIF wrappers), EctoLibSql.State (connection state), Ecto.Adapters.LibSql (main adapter), Ecto.Adapters.LibSql.Connection (SQL generation)
Applied to files:
test/fuzz_test.exslib/ecto_libsql/native.extest/named_parameters_execution_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Configure WAL (Write-Ahead Logging) mode using EctoLibSql.Pragma.set_journal_mode/2 for better concurrency and write performance in local databases
Applied to files:
test/fuzz_test.exstest/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Always pattern match on result tuples from EctoLibSql operations to handle {:ok, ...} and {:error, ...} cases, avoiding bare pattern matches that could crash the process
Applied to files:
test/fuzz_test.exstest/named_parameters_execution_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust, always use `ok_or_else()` with proper error messages for handling Option types instead of `.unwrap()`
Applied to files:
test/fuzz_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : NEVER use `.unwrap()` in production Rust code - use `safe_lock` helpers instead. Tests MAY use `.unwrap()` for simplicity
Applied to files:
test/fuzz_test.exstest/ecto_migration_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Use proper transactions with timeout in Elixir: `Repo.transaction(fn -> ... end, timeout: 15_000)` to handle database locked errors
Applied to files:
test/fuzz_test.exslib/ecto_libsql/native.extest/ecto_migration_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use savepoints (EctoLibSql.Native.create_savepoint/2) within transactions for granular error recovery on partial batch failures
Applied to files:
test/fuzz_test.exstest/named_parameters_execution_test.exs.beads/issues.jsonltest/ecto_migration_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use transaction behaviours (deferred, immediate, exclusive, read_only) with EctoLibSql.Native.begin/2 to optimize for specific workload patterns and prevent writer starvation
Applied to files:
test/fuzz_test.exslib/ecto_libsql/native.extest/security_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust async code, drop locks before calling `TOKIO_RUNTIME.block_on()` to prevent deadlocks
Applied to files:
test/fuzz_test.exstest/ecto_migration_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust, use `safe_lock_arc(&client, "function_name client")` for locking Arc<Mutex<T>> instead of `.lock().unwrap()`
Applied to files:
test/fuzz_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use Ecto changesets with proper validation and error handling instead of raw SQL for INSERT/UPDATE operations to leverage Ecto's safety features
Applied to files:
test/fuzz_test.exslib/ecto_libsql/native.extest/named_parameters_execution_test.exstest/ecto_migration_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use prepared statements with automatic statement caching for repeated queries in EctoLibSql to improve performance (~10-15x faster than unprepared queries)
Applied to files:
test/fuzz_test.exslib/ecto_libsql/native.extest/named_parameters_execution_test.exs
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: For type conversion issues, update loaders/dumpers in `lib/ecto/adapters/libsql.ex` following the pattern `loaders/dumpers` functions
Applied to files:
lib/ecto_libsql/native.ex.beads/issues.jsonltest/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use statement introspection functions (stmt_parameter_count, stmt_column_count, stmt_column_name) to validate prepared statement structure before execution
Applied to files:
lib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use batch operations (batch/2, batch_transactional/2, or execute_batch_sql/2) instead of individual INSERT/UPDATE/DELETE statements for bulk data operations
Applied to files:
lib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Test coverage must include: happy path, error cases, edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Applied to files:
.beads/issues.jsonl
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to priv/repo/migrations/*.exs : Use RANDOM ROWID in table definitions (with options: [random_rowid: true]) for security-sensitive tables to prevent ID enumeration attacks
Applied to files:
test/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to priv/repo/migrations/*.exs : Create appropriate indexes (simple, composite, covering) on frequently queried columns to optimize query performance
Applied to files:
test/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Run Elixir tests with `mix test`, Rust tests with `cd native/ecto_libsql && cargo test`, and use `--trace` flag for debugging specific tests
Applied to files:
test/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Enable foreign key constraints in SQLite using EctoLibSql.Pragma.enable_foreign_keys/1 for data integrity in production databases
Applied to files:
test/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust, use `safe_lock(®ISTRY, "function_name context")` for locking registries instead of `.lock().unwrap()`
Applied to files:
test/ecto_migration_test.exstest/security_test.exs
🧬 Code graph analysis (3)
test/fuzz_test.exs (1)
lib/ecto_libsql.ex (2)
handle_rollback(207-215)handle_execute(148-160)
lib/ecto_libsql/native.ex (3)
native/ecto_libsql/src/statement.rs (5)
prepare_statement(30-71)statement_parameter_count(313-338)statement_parameter_name(361-391)execute_prepared(164-221)query_prepared(86-145)native/ecto_libsql/src/query.rs (1)
query_args(33-116)native/ecto_libsql/src/transaction.rs (1)
query_with_trx_args(322-401)
test/named_parameters_execution_test.exs (2)
lib/ecto_libsql.ex (1)
handle_execute(148-160)lib/ecto_libsql/native.ex (5)
begin(629-639)prepare(752-760)close_stmt(862-864)execute_stmt(782-802)query_stmt(823-849)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / macos-latest
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / ubuntu-latest
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
.beads/issues.jsonl (1)
1-33: Issue metadata changes look consistent with the implemented features.The updated JSONL records for security tests, named parameters, STRICT tables, RANDOM ROWID, and binary round‑trip fixes align with the new tests and adapter changes in this PR. No code changes here, so nothing to block on.
test/ecto_migration_test.exs (1)
14-32: Repo teardown and new migration-feature tests look solid.Stopping
TestRepoinon_exit, adding a short sleep, and cleaning-journalalongside-shm/-walshould help avoid file‑handle races between tests. The new migration tests forrandom_rowid: true,strict: true, and generated columns (virtual vs stored, plus invalid default/PK combinations) validate the SQL emitted byConnection.execute_ddl/1without forcing execution on older libSQL versions, which is a pragmatic balance. These changes align well with the migration and security guidance captured in your AGENTS notes.Also applies to: 771-882
…tests
- Change rescue blocks to return 3-tuples {:error, :exception, state} matching
handle_execute/4's return type spec
- Update case patterns to match 3-tuple error forms {:error, _, _}
- Add {:disconnect, _, _} pattern matching for completeness
- Wrap binary blob data in {:blob, data} tuples so NIF treats them as BLOB
rather than TEXT (fixes null byte truncation issue where <<0>> became "")
The parameter name cache (SQL -> param_names mapping) was stored in persistent_term which has no size limit and can grow unboundedly with dynamic SQL workloads. Changes: - Replace persistent_term with ETS table for parameter name caching - Add maximum cache size of 1000 entries - Implement LRU eviction: when full, evict oldest 500 entries - Add thread-safe table creation with race condition handling - Add clear_param_cache/0 for testing and memory reclamation - Add param_cache_size/0 for monitoring cache usage - Update access times asynchronously to avoid blocking reads
…ling back
Previously, if statement_parameter_count/2 returned an error or unexpected
value, the code would silently fall back to treating it as 0 parameters.
This hid actual errors behind confusing SQL errors at runtime.
Changes:
- introspect_and_cache_params/3: Propagate {:error, reason} from
statement_parameter_count/2 instead of coercing to 0
- Clean up prepared statement before returning error
- Handle {:error, _reason} from statement_parameter_name/3 explicitly
- normalise_arguments_for_stmt/3: Use consistent count >= 0 pattern
and handle {:error, _reason} from statement_parameter_name/3
This makes error handling consistent with prepare failures which already
propagate as {:error, reason} and are converted to EctoLibSql.Error.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/ecto_libsql/native.ex (2)
426-436: Eviction logic is sound but could be more efficient.The
evict_oldest_entries/0function correctly sorts by access time and removes the oldest entries. However, converting the entire table to a list (line 429), sorting it, and iterating could be expensive for large caches. Consider using:ets.select/2with a limit or maintaining a separate ordered structure.Given the cache size limit (1000 entries), this is acceptable for now but worth noting for future optimisation.
330-365: Consider documenting cache memory usage.The ETS cache has a maximum size of 1000 entries (line 334) and evicts 500 entries when full (line 335). Each entry stores: statement SQL (potentially large), parameter names list, and access time. For applications with many dynamic queries, this could consume significant memory.
Consider adding a note in the module documentation about the cache's memory footprint and the ability to clear it using
clear_param_cache/0if needed.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/ecto_libsql/native.extest/named_parameters_execution_test.exstest/security_test.exs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ex
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ex: Use prepared statements with automatic statement caching for repeated queries in EctoLibSql to improve performance (~10-15x faster than unprepared queries)
Use batch operations (batch/2, batch_transactional/2, or execute_batch_sql/2) instead of individual INSERT/UPDATE/DELETE statements for bulk data operations
Always pattern match on result tuples from EctoLibSql operations to handle {:ok, ...} and {:error, ...} cases, avoiding bare pattern matches that could crash the process
Use cursors and DBConnection.stream for memory-efficient processing of large result sets instead of loading all rows into memory
Enable foreign key constraints in SQLite using EctoLibSql.Pragma.enable_foreign_keys/1 for data integrity in production databases
Use transaction behaviours (deferred, immediate, exclusive, read_only) with EctoLibSql.Native.begin/2 to optimize for specific workload patterns and prevent writer starvation
Use Ecto changesets with proper validation and error handling instead of raw SQL for INSERT/UPDATE operations to leverage Ecto's safety features
Use savepoints (EctoLibSql.Native.create_savepoint/2) within transactions for granular error recovery on partial batch failures
Use Ecto's insert_all/3 for bulk inserts of pre-mapped data to leverage batching and achieve better performance than individual inserts
Configure WAL (Write-Ahead Logging) mode using EctoLibSql.Pragma.set_journal_mode/2 for better concurrency and write performance in local databases
Use vector similarity search with EctoLibSql.Native.vector_distance_cos/2 for semantic search features in AI/ML applications
Encrypt local databases and replicas using encryption_key parameter (minimum 32 characters) for protecting sensitive data at rest
Use statement introspection functions (stmt_parameter_count, stmt_column_count, stmt_column_name) to validate prepared statement structure before execution
Files:
lib/ecto_libsql/native.ex
🧠 Learnings (16)
📓 Common learnings
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use prepared statements with automatic statement caching for repeated queries in EctoLibSql to improve performance (~10-15x faster than unprepared queries)
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use statement introspection functions (stmt_parameter_count, stmt_column_count, stmt_column_name) to validate prepared statement structure before execution
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: When adding SQLite function support, update `lib/ecto/adapters/libsql/connection.ex` with the expression handler and add corresponding tests
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: When adding SQLite function support, update `lib/ecto/adapters/libsql/connection.ex` with the expression handler and add corresponding tests
Applied to files:
test/security_test.exstest/named_parameters_execution_test.exslib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use Ecto changesets with proper validation and error handling instead of raw SQL for INSERT/UPDATE operations to leverage Ecto's safety features
Applied to files:
test/security_test.exstest/named_parameters_execution_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : NEVER use `.unwrap()` in production Rust code - use `safe_lock` helpers instead. Tests MAY use `.unwrap()` for simplicity
Applied to files:
test/security_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use savepoints (EctoLibSql.Native.create_savepoint/2) within transactions for granular error recovery on partial batch failures
Applied to files:
test/security_test.exstest/named_parameters_execution_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use transaction behaviours (deferred, immediate, exclusive, read_only) with EctoLibSql.Native.begin/2 to optimize for specific workload patterns and prevent writer starvation
Applied to files:
test/security_test.exslib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust, use `safe_lock_arc(&client, "function_name client")` for locking Arc<Mutex<T>> instead of `.lock().unwrap()`
Applied to files:
test/security_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust async code, drop locks before calling `TOKIO_RUNTIME.block_on()` to prevent deadlocks
Applied to files:
test/security_test.exs
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Use proper transactions with timeout in Elixir: `Repo.transaction(fn -> ... end, timeout: 15_000)` to handle database locked errors
Applied to files:
test/security_test.exslib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust, use `safe_lock(®ISTRY, "function_name context")` for locking registries instead of `.lock().unwrap()`
Applied to files:
test/security_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Always pattern match on result tuples from EctoLibSql operations to handle {:ok, ...} and {:error, ...} cases, avoiding bare pattern matches that could crash the process
Applied to files:
test/security_test.exstest/named_parameters_execution_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use prepared statements with automatic statement caching for repeated queries in EctoLibSql to improve performance (~10-15x faster than unprepared queries)
Applied to files:
test/named_parameters_execution_test.exslib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: For type conversion issues, update loaders/dumpers in `lib/ecto/adapters/libsql.ex` following the pattern `loaders/dumpers` functions
Applied to files:
lib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Elixir modules must follow the structure: EctoLibSql (DBConnection protocol), EctoLibSql.Native (NIF wrappers), EctoLibSql.State (connection state), Ecto.Adapters.LibSql (main adapter), Ecto.Adapters.LibSql.Connection (SQL generation)
Applied to files:
lib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use statement introspection functions (stmt_parameter_count, stmt_column_count, stmt_column_name) to validate prepared statement structure before execution
Applied to files:
lib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use batch operations (batch/2, batch_transactional/2, or execute_batch_sql/2) instead of individual INSERT/UPDATE/DELETE statements for bulk data operations
Applied to files:
lib/ecto_libsql/native.ex
🧬 Code graph analysis (1)
test/named_parameters_execution_test.exs (2)
lib/ecto_libsql.ex (1)
handle_execute(148-160)lib/ecto_libsql/native.ex (7)
begin(735-745)commit(760-764)rollback(777-781)prepare(858-866)close_stmt(968-970)execute_stmt(888-908)query_stmt(929-955)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / macos-latest
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / ubuntu-latest
- GitHub Check: Analyze (rust)
🔇 Additional comments (21)
test/named_parameters_execution_test.exs (5)
16-44: LGTM! Setup and cleanup are sound.The setup correctly creates an isolated temporary database with a unique name and properly cleans up all database artefacts (including WAL and journal files) in the
on_exitcallback. Usingasync: falseis appropriate for database tests that require isolation.
154-183: LGTM! Statement reuse is properly tested.This test validates that the same SQL statement with named parameters can be reused multiple times with different parameter values, which is essential for verifying that statement caching and parameter normalisation work correctly across multiple executions.
382-402: Excellent use of statement introspection.This test properly validates the prepared statement parameter introspection functionality, checking that parameter names (including the prefix) are correctly retrieved. This aligns with the coding guidelines emphasising the use of statement introspection functions to validate prepared statement structure before execution.
Based on learnings, statement introspection is a recommended practice.
480-492: Error handling correctly validates missing parameters.The test properly verifies that missing required parameters result in an error tuple
{:error, _, _}, which aligns with the coding guideline to always pattern match on result tuples from EctoLibSql operations to handle{:ok, ...}and{:error, ...}cases.Based on learnings, proper error handling with pattern matching is essential.
544-577: Case sensitivity test demonstrates important behaviour.This test correctly validates that named parameter matching is case-sensitive (
:Name!=:name), which is important for developers to understand. The test shows that mismatched cases result in no rows being found, not a runtime error.test/security_test.exs (6)
4-10: LGTM! Cleanup helper is well-designed.The
cleanup_db/1helper properly removes the database file and all associated WAL/SHM/journal files. UsingFile.rm/1(which silently succeeds if the file doesn't exist) is appropriate here for cleanup operations.
45-63: LGTM! Cross-connection transaction isolation is properly validated.The test now correctly asserts that using connection A's transaction on connection B must fail with an error tuple (line 55), addressing the previous overly lenient assertion. Both connections are properly disconnected and cleaned up at the end.
117-147: LGTM! Statement isolation test properly manages resources.The test correctly validates that connection B cannot access connection A's prepared statement. Only per-test resources (
state_b,db_path_b) are cleaned up, while the setup-providedstate_ais managed by theon_exitcallback. This addresses the previous manual disconnect issue.
149-161: LGTM! Closed statement test is correct.The test properly validates that a closed statement cannot be used and returns an error. The setup-provided
stateis correctly not manually disconnected, addressing the previous issue.
285-292: LGTM! Proper pattern matching on cleanup operations.Lines 286-289 correctly use pattern matching to ensure cleanup operations succeed (
{:ok, _, _}and:ok), which provides better visibility if cleanup fails during test teardown.
420-465: LGTM! Comprehensive resource cleanup validation.This test thoroughly validates that resources (cursors, prepared statements) are properly cleaned up after disconnect, checking both that they work before disconnect (lines 445-450) and that they're inaccessible after disconnect (lines 456-462). The assertions correctly verify both error returns and empty results.
lib/ecto_libsql/native.ex (10)
286-302: LGTM! Clear entry point for parameter normalisation.The
normalise_arguments/3function provides a clean interface that handles both lists (positional) and maps (named parameters), with appropriate fallback for other types. The function signature correctly returns a list on success or{:error, reason}on failure, enabling proper error propagation.
314-328: Clever handling of both atom and string keys.The
get_map_value_flexible/2function handles both atom-keyed and string-keyed maps without creating atoms at runtime (usesString.to_existing_atom/1with rescue), which is a good practice for preventing atom table exhaustion. This addresses the concern raised in past review comments about string-keyed vs atom-keyed maps.
367-390: Proper handling of ETS table creation race condition.The
ensure_param_cache_table/0function correctly handles the race condition where multiple processes might try to create the table simultaneously by rescuingArgumentError(line 382-384). Using:read_concurrencyand:write_concurrencyoptions is appropriate for this cache use case.
456-514: Excellent error propagation in introspection.The
introspect_and_cache_params/3function properly propagates errors from bothprepare_statement(line 510-512) andstatement_parameter_count(line 504-507), addressing the concern from past review comments about falling back to arbitrary ordering. The function correctly cleans up the prepared statement before returning errors (line 506).This aligns with the coding guideline to use statement introspection functions for validation.
Based on learnings, proper error propagation is essential.
516-565: LGTM! Prepared statement normalisation avoids double preparation.The
normalise_arguments_for_stmt/3function efficiently uses the existingstmt_idfor introspection, avoiding re-preparation of the statement. This is more efficient than the approach used bynormalise_arguments/3which must prepare temporarily for introspection.
573-590: Proper error handling in query execution path.The updated
query/3function correctly callsnormalise_arguments/3and handles both success and error cases (lines 580-589), returning a properEctoLibSql.Errorstruct with a descriptive message on failure. This ensures parameter introspection errors are surfaced clearly to callers.
643-655: Consistent error handling in transaction execution path.The
execute_with_trx/3function mirrors the error handling pattern fromquery/3, properly normalising arguments and propagating errors. This consistency across execution paths is good for maintainability.
659-713: LGTM! Proper handling of RETURNING clause detection.The
do_execute_with_trx/7function correctly detects the RETURNING clause using word-boundary regex (\bRETURNING\b, line 666) to avoid false positives, and routes to the appropriate execution method. The normalisation of empty columns/rows tonil(lines 677-684) ensures consistency with the non-transactional path.
888-908: Proper integration of normalisation in prepared statement execution.The
execute_stmt/4function correctly usesnormalise_arguments_for_stmt/3(line 895) to handle map-based named parameters for prepared statements, addressing the concern from past review comments about prepared statements not accepting maps. The error message is descriptive (line 897).This aligns with the documentation updates showing that prepared statements now accept maps with atom keys.
929-955: Prepared statement query path correctly normalised.The
query_stmt/3function mirrors theexecute_stmt/4approach, properly normalising arguments before calling the NIF. This completes the integration of named parameter support across all execution paths (direct query, transactional, and prepared statements).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/ecto_libsql/native.ex (2)
387-428: LGTM: Well-documented cache management API.The public cache management functions (
clear_param_cache/0andparam_cache_size/0) provide clear control over the parameter name cache. The documentation appropriately explains memory considerations and use cases.Consider documenting that the ETS cache is shared across all processes/connections in the node (not per-connection). This is important for understanding cache behavior in pooled connection scenarios.
561-579: LGTM: Prepared statement argument normalisation.The
normalise_arguments_for_stmt/3function correctly normalises arguments for prepared statements using thestmt_idfor introspection. Since the statement is already prepared, parameter introspection viastatement_parameter_countandstatement_parameter_nameNIFs is fast.The function doesn't use the ETS cache (unlike
normalise_arguments/3) because:
stmt_idis a UUID that changes per preparation- The SQL statement string isn't available at this call site
- Introspection on an already-prepared statement is fast enough
If profiling reveals that repeated introspection on the same prepared statement is a bottleneck, consider adding a stmt_id-based cache with reference counting. However, this is likely premature optimization given the low overhead of the NIFs.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.beads/issues.jsonl.claude/settings.local.jsonlib/ecto_libsql/native.ex
🚧 Files skipped from review as they are similar to previous changes (1)
- .claude/settings.local.json
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ex
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ex: Use prepared statements with automatic statement caching for repeated queries in EctoLibSql to improve performance (~10-15x faster than unprepared queries)
Use batch operations (batch/2, batch_transactional/2, or execute_batch_sql/2) instead of individual INSERT/UPDATE/DELETE statements for bulk data operations
Always pattern match on result tuples from EctoLibSql operations to handle {:ok, ...} and {:error, ...} cases, avoiding bare pattern matches that could crash the process
Use cursors and DBConnection.stream for memory-efficient processing of large result sets instead of loading all rows into memory
Enable foreign key constraints in SQLite using EctoLibSql.Pragma.enable_foreign_keys/1 for data integrity in production databases
Use transaction behaviours (deferred, immediate, exclusive, read_only) with EctoLibSql.Native.begin/2 to optimize for specific workload patterns and prevent writer starvation
Use Ecto changesets with proper validation and error handling instead of raw SQL for INSERT/UPDATE operations to leverage Ecto's safety features
Use savepoints (EctoLibSql.Native.create_savepoint/2) within transactions for granular error recovery on partial batch failures
Use Ecto's insert_all/3 for bulk inserts of pre-mapped data to leverage batching and achieve better performance than individual inserts
Configure WAL (Write-Ahead Logging) mode using EctoLibSql.Pragma.set_journal_mode/2 for better concurrency and write performance in local databases
Use vector similarity search with EctoLibSql.Native.vector_distance_cos/2 for semantic search features in AI/ML applications
Encrypt local databases and replicas using encryption_key parameter (minimum 32 characters) for protecting sensitive data at rest
Use statement introspection functions (stmt_parameter_count, stmt_column_count, stmt_column_name) to validate prepared statement structure before execution
Files:
lib/ecto_libsql/native.ex
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use prepared statements with automatic statement caching for repeated queries in EctoLibSql to improve performance (~10-15x faster than unprepared queries)
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: When adding SQLite function support, update `lib/ecto/adapters/libsql/connection.ex` with the expression handler and add corresponding tests
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use statement introspection functions (stmt_parameter_count, stmt_column_count, stmt_column_name) to validate prepared statement structure before execution
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use batch operations (batch/2, batch_transactional/2, or execute_batch_sql/2) instead of individual INSERT/UPDATE/DELETE statements for bulk data operations
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use Ecto changesets with proper validation and error handling instead of raw SQL for INSERT/UPDATE operations to leverage Ecto's safety features
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use savepoints (EctoLibSql.Native.create_savepoint/2) within transactions for granular error recovery on partial batch failures
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use prepared statements with automatic statement caching for repeated queries in EctoLibSql to improve performance (~10-15x faster than unprepared queries)
Applied to files:
lib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: When adding SQLite function support, update `lib/ecto/adapters/libsql/connection.ex` with the expression handler and add corresponding tests
Applied to files:
lib/ecto_libsql/native.ex.beads/issues.jsonl
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: For type conversion issues, update loaders/dumpers in `lib/ecto/adapters/libsql.ex` following the pattern `loaders/dumpers` functions
Applied to files:
lib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use statement introspection functions (stmt_parameter_count, stmt_column_count, stmt_column_name) to validate prepared statement structure before execution
Applied to files:
lib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use transaction behaviours (deferred, immediate, exclusive, read_only) with EctoLibSql.Native.begin/2 to optimize for specific workload patterns and prevent writer starvation
Applied to files:
lib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use batch operations (batch/2, batch_transactional/2, or execute_batch_sql/2) instead of individual INSERT/UPDATE/DELETE statements for bulk data operations
Applied to files:
lib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Use proper transactions with timeout in Elixir: `Repo.transaction(fn -> ... end, timeout: 15_000)` to handle database locked errors
Applied to files:
lib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use savepoints (EctoLibSql.Native.create_savepoint/2) within transactions for granular error recovery on partial batch failures
Applied to files:
.beads/issues.jsonl
🧬 Code graph analysis (1)
lib/ecto_libsql/native.ex (3)
native/ecto_libsql/src/statement.rs (4)
statement_parameter_name(361-391)statement_parameter_count(313-338)prepare_statement(30-71)query_prepared(86-145)native/ecto_libsql/src/transaction.rs (2)
take(73-94)query_with_trx_args(322-401)native/ecto_libsql/src/query.rs (1)
query_args(33-116)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / macos-latest
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / ubuntu-latest
- GitHub Check: Analyze (rust)
🔇 Additional comments (8)
lib/ecto_libsql/native.ex (8)
304-312: LGTM: Safe parameter prefix handling.The
remove_param_prefix/1function correctly strips SQLite parameter prefixes (:,@,$). The use ofString.slice/3is clear and handles all cases appropriately.
362-370: LGTM: Safe flexible key lookup.The
get_map_value_flexible/2function safely handles both atom and string keys without creating new atoms at runtime. UsingString.to_existing_atom/1in a rescue block is the correct approach—it tries the atom key first (more common) and falls back to the string key if the atom doesn't exist.This avoids atom table exhaustion while maintaining backward compatibility with both key types.
500-559: LGTM: Parameter introspection with proper error handling.The
map_to_positional_args/3andintrospect_and_cache_params/3functions implement a solid caching strategy:
- Check cache first to avoid repeated preparation overhead
- On cache miss, prepare the statement solely for introspection
- Close the statement immediately after extracting parameter metadata
- Cache the parameter order for future queries
Trade-off analysis:
- First execution with a new SQL statement: 2 preparations (one for introspection, one for actual execution)
- Subsequent executions: 0 preparations (cache hit)
This trade-off is reasonable because:
- The cache prevents repeated introspection for the same SQL statement
- Immediate closure avoids statement handle lifecycle complexity
- Error handling correctly closes statements before propagating failures (lines 550-552)
The implementation properly propagates errors from both
statement_parameter_count(line 549) andprepare_statement(line 555), maintaining clear error boundaries.
587-604: LGTM: Query path integration with clear error messages.The
query/3function correctly integrates argument normalisation with proper error handling. The error message "Failed to prepare statement for parameter introspection: #{reason}" (line 598) provides clear context about where the failure occurred, which aids debugging.The normalised arguments are correctly passed to
do_query/7(line 602), maintaining the separation of concerns between normalisation and execution.
652-727: LGTM: Transaction execution path integration.The
execute_with_trx/3function follows the same pattern asquery/3:
- Normalise arguments with clear error propagation (lines 659-664)
- Delegate to
do_execute_with_trx/6with normalised argumentsThe
do_execute_with_trx/6helper correctly routes normalised arguments to eitherquery_with_trx_args(for SELECT or statements with RETURNING) orexecute_with_transaction(for INSERT/UPDATE/DELETE without RETURNING), maintaining consistency across transactional and non-transactional execution paths.
883-922: LGTM: Prepared statement execution with normalisation.The
execute_stmt/4function correctly integratesnormalise_arguments_for_stmt/3:
- Documentation clearly states both positional and named parameter support (lines 890, 898-900)
- Error handling provides descriptive context: "Failed to normalise parameters" (line 911)
- Normalised arguments are passed to
execute_prepared(line 914)The documentation recommends atom keys for named parameters (
%{name: "Alice"}), which is the optimal choice sinceget_map_value_flexible/2tries atom keys first before falling back to string keys.
924-969: LGTM: Prepared statement query with normalisation.The
query_stmt/3function mirrors theexecute_stmt/4pattern:
- Clear documentation with examples for both positional and named parameters (lines 931, 935-941)
- Proper error handling with descriptive messages (line 951)
- Normalised arguments correctly passed to
query_prepared(line 954)The implementation completes the named parameter support across all execution paths (query, execute, transactional, and prepared statements), maintaining consistency throughout the API.
488-498: LRU eviction acceptable for current cache size.The
evict_oldest_entries/0implementation sorts the entire cache (O(n log n)) to find the oldest entries. For the configured maximum of 1,000 entries, this results in approximately 10,000 comparisons per eviction, which is acceptable given eviction only occurs once per 500 new unique statements (whencache_size >= @param_cache_max_size).If the cache size were increased significantly in future (e.g., to 10,000+ entries), the O(n log n) sort could become a bottleneck. Consider alternative approaches for future optimisation if needed:
- Using
:ets.select/2with a match spec to avoid full materialisation- Maintaining a separate ordered structure to track access times
- Implementing an approximate LRU algorithm (e.g., CLOCK or mark-and-sweep)
The current implementation is reasonable for the stated cache size, and no immediate action is required.
Likely an incorrect or invalid review comment.
| @doc false | ||
| # Returns list on success, {:error, reason} on failure. | ||
| def normalise_arguments(conn_id, statement, args) do | ||
| # If args is already a list, return as-is (positional parameters). | ||
| case args do | ||
| list when is_list(list) -> | ||
| list | ||
|
|
||
| map when is_map(map) -> | ||
| # Convert named parameters map to positional list. | ||
| # Returns list on success, {:error, reason} on preparation failure. | ||
| map_to_positional_args(conn_id, statement, map) | ||
|
|
||
| _ -> | ||
| args | ||
| end | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider validating argument types more strictly.
The normalise_arguments/3 function returns non-list/non-map arguments unchanged (lines 299-300). If a caller accidentally passes invalid input (e.g., an integer, atom, or tuple), the error will surface later at the NIF boundary with a less descriptive message like "Unsupported argument type" rather than a clear "arguments must be a list or map" error at this boundary.
Consider adding validation:
🔎 Suggested validation
map when is_map(map) ->
# Convert named parameters map to positional list.
# Returns list on success, {:error, reason} on preparation failure.
map_to_positional_args(conn_id, statement, map)
_ ->
- args
+ {:error, "Arguments must be a list or map, got: #{inspect(args)}"}
end
endAlternatively, if passing through non-standard types is intentional (for future extensibility), document this behaviour and ensure the Rust NIFs provide clear error messages for invalid types.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @doc false | |
| # Returns list on success, {:error, reason} on failure. | |
| def normalise_arguments(conn_id, statement, args) do | |
| # If args is already a list, return as-is (positional parameters). | |
| case args do | |
| list when is_list(list) -> | |
| list | |
| map when is_map(map) -> | |
| # Convert named parameters map to positional list. | |
| # Returns list on success, {:error, reason} on preparation failure. | |
| map_to_positional_args(conn_id, statement, map) | |
| _ -> | |
| args | |
| end | |
| end | |
| @doc false | |
| # Returns list on success, {:error, reason} on failure. | |
| def normalise_arguments(conn_id, statement, args) do | |
| # If args is already a list, return as-is (positional parameters). | |
| case args do | |
| list when is_list(list) -> | |
| list | |
| map when is_map(map) -> | |
| # Convert named parameters map to positional list. | |
| # Returns list on success, {:error, reason} on preparation failure. | |
| map_to_positional_args(conn_id, statement, map) | |
| _ -> | |
| {:error, "Arguments must be a list or map, got: #{inspect(args)}"} | |
| end | |
| end |
🤖 Prompt for AI Agents
In lib/ecto_libsql/native.ex around lines 286 to 302, normalise_arguments/3
currently falls through and returns non-list/non-map values unchanged which
defers a confusing error to the NIF; change the fallback to validate and return
a descriptive error instead of passing-through: if args is a list return it, if
a map call map_to_positional_args/3, otherwise return {:error,
:invalid_arguments_type} (or {:error, "arguments must be a list or map"}) and
update the @doc to state the accepted types.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.