Skip to content

Conversation

@ocean
Copy link
Owner

@ocean ocean commented Jan 12, 2026

Addresses issues from PR #57 where Oban failed due to unsupported parameter types.

Problem:

  • Rustler cannot serialise complex Elixir structs like DateTime, NaiveDateTime, Date, Time, and Decimal
  • This caused 'Unsupported argument type' errors when using Oban with ecto_libsql
  • Oban Lifeline plugin failed with Enumerable protocol errors

Solution:

  • Added encode/3 in DBConnection.Query implementation to convert temporal types and Decimal to ISO8601/string format before passing to Rust NIF
  • Added guard clause for non-list params to prevent crashes
  • decode/3 remains a simple pass-through as Native.ex already handles result normalisation correctly

Changes:

  • lib/ecto_libsql/query.ex: Added parameter encoding with type conversions
  • test/ecto_libsql_query_encoding_test.exs: Comprehensive test suite for parameter encoding and result pass-through

Testing:

  • All 638 existing tests pass
  • New test suite covers DateTime, NaiveDateTime, Date, Time, Decimal encoding
  • Tests verify nil, integer, float, string, binary, boolean pass-through unchanged
  • Tests verify mixed parameter types work correctly

Fixes #57

Summary by CodeRabbit

Release Notes

  • New Features

    • Added support for automatic conversion of DateTime, NaiveDateTime, Date, Time, and Decimal types to SQLite-compatible ISO8601 string formats in database queries.
  • Tests

    • Introduced comprehensive test suite validating parameter encoding behaviour and ensuring query results pass through correctly for various Elixir data types.

✏️ Tip: You can customize this high-level summary in your review settings.

Addresses issues from PR #57 where Oban failed due to unsupported parameter types.

**Problem**:
- Rustler cannot serialise complex Elixir structs like DateTime, NaiveDateTime, Date, Time, and Decimal
- This caused 'Unsupported argument type' errors when using Oban with ecto_libsql
- Oban Lifeline plugin failed with Enumerable protocol errors

**Solution**:
- Added encode/3 in DBConnection.Query implementation to convert temporal types and Decimal to ISO8601/string format before passing to Rust NIF
- Added guard clause for non-list params to prevent crashes
- decode/3 remains a simple pass-through as Native.ex already handles result normalisation correctly

**Changes**:
- lib/ecto_libsql/query.ex: Added parameter encoding with type conversions
- test/ecto_libsql_query_encoding_test.exs: Comprehensive test suite for parameter encoding and result pass-through

**Testing**:
- All 638 existing tests pass
- New test suite covers DateTime, NaiveDateTime, Date, Time, Decimal encoding
- Tests verify nil, integer, float, string, binary, boolean pass-through unchanged
- Tests verify mixed parameter types work correctly

Co-authored-by: nadilas <nadilas@users.noreply.github.com>
Fixes #57
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 12, 2026

Walkthrough

This pull request adds encoding logic to the query module to convert Elixir-native types (DateTime, NaiveDateTime, Date, Time, Decimal) into SQLite-compatible string representations before passing parameters to the NIF layer. New tests validate encoding and decoding behaviour across various type combinations.

Changes

Cohort / File(s) Summary
Query encoding implementation
lib/ecto_libsql/query.ex
Adds encode/3 implementation for DBConnection.Query protocol with pattern matching for list parameters; introduces six private encoder functions for DateTime, NaiveDateTime, Date, Time, and Decimal types, converting them to ISO8601 or string representations; other values pass through unchanged.
Encoding test suite
test/ecto_libsql_query_encoding_test.exs
New test module validating DBConnection.Query.encode/3 for individual and mixed-type parameters (DateTime, NaiveDateTime, Date, Time, Decimal, nil, numerics, strings, binaries, booleans); includes separate test describe block for decode/3 result pass-through behaviour.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A hop through time types we did take,
DateTime to string for SQLite's sake,
NaiveDateTime, Date, and Time so fine,
Now encoded proper, in ISO line,
With Decimal and Decimal tests to prance,
Our parameters leap in SQLite's dance!

🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main changes: adding DateTime/Decimal parameter encoding to resolve Oban compatibility issues, which directly matches the PR objectives.
Linked Issues check ✅ Passed The PR implements encode/3 to convert DateTime, NaiveDateTime, Date, Time, and Decimal to ISO8601/string formats, directly addressing the primary requirement from issue #57 to prevent 'Unsupported argument type' errors.
Out of Scope Changes check ✅ Passed All changes are focused on parameter encoding via encode/3 and corresponding tests; no unrelated modifications are present.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-pr57-issues

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
lib/ecto_libsql/query.ex (1)

50-55: Consider additional type encodings for completeness.

The current implementation covers the main types causing Oban issues. However, consider whether other types might need encoding:

  • UUID: Ecto may pass Ecto.UUID structs
  • Maps/lists: Nested structures might contain temporal types
  • Atoms: Some query params might be atoms (e.g., :null)

For now, these will pass through to the NIF and may fail if unsupported. This is acceptable if the current scope addresses the immediate Oban issues.

Based on learnings, if additional type conversion issues arise, also check the type loaders/dumpers in lib/ecto/adapters/libsql.ex.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d3fd488 and d00bea8.

📒 Files selected for processing (2)
  • lib/ecto_libsql/query.ex
  • test/ecto_libsql_query_encoding_test.exs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ex,exs,rs,md}

📄 CodeRabbit inference engine (CLAUDE.md)

Use British/Australian English for all code, comments, and documentation (except SQL keywords and compatibility requirements)

Files:

  • lib/ecto_libsql/query.ex
  • test/ecto_libsql_query_encoding_test.exs
lib/**/*.ex

📄 CodeRabbit inference engine (CLAUDE.md)

lib/**/*.ex: Use proper transactions with timeout for database operations: Repo.transaction(fn -> ... end, timeout: 15_000) to avoid database locked errors
Ensure connections are closed in try/after blocks to prevent connection leaks
Use immediate transactions for write operations: begin(state, behavior: :immediate) for write-heavy workloads
Verify schema types match database columns and check custom types have loaders/dumpers to fix type conversion errors
Use cast/3 in changesets for automatic type conversion in Elixir

Files:

  • lib/ecto_libsql/query.ex
test/**/*.{exs,ex}

📄 CodeRabbit inference engine (CLAUDE.md)

test/**/*.{exs,ex}: For state threading in tests, use consistent variable naming: state for connection scope, trx_state for transaction scope, cursor for cursor scope, stmt_id for prepared statement ID scope
When an error operation returns updated state in tests that IS needed for subsequent operations, rebind the state variable and add clarifying comments
When an error operation returns state in tests that is NOT needed for subsequent operations, discard it with underscore
For terminal test operations, use underscore variable names and assert error tuples with the pattern assert {:error, %EctoLibSql.Error{}, _conn}

Files:

  • test/ecto_libsql_query_encoding_test.exs
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T07:35:10.435Z
Learning: Applies to lib/ecto/adapters/libsql.ex : Update type loaders and dumpers in `lib/ecto/adapters/libsql.ex` when fixing type conversion issues
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T07:35:10.435Z
Learning: Applies to lib/ecto/adapters/libsql/connection.ex : Update SQL generation in `lib/ecto/adapters/libsql/connection.ex` when adding SQLite function support
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T07:35:10.435Z
Learning: Applies to lib/**/*.ex : Use `cast/3` in changesets for automatic type conversion in Elixir
📚 Learning: 2026-01-12T07:35:10.435Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T07:35:10.435Z
Learning: Applies to lib/ecto/adapters/libsql/connection.ex : Update SQL generation in `lib/ecto/adapters/libsql/connection.ex` when adding SQLite function support

Applied to files:

  • lib/ecto_libsql/query.ex
  • test/ecto_libsql_query_encoding_test.exs
📚 Learning: 2026-01-12T07:35:10.435Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T07:35:10.435Z
Learning: Applies to lib/ecto/adapters/libsql.ex : Update type loaders and dumpers in `lib/ecto/adapters/libsql.ex` when fixing type conversion issues

Applied to files:

  • lib/ecto_libsql/query.ex
  • test/ecto_libsql_query_encoding_test.exs
📚 Learning: 2026-01-12T07:35:10.435Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T07:35:10.435Z
Learning: Applies to lib/**/*.ex : Use `cast/3` in changesets for automatic type conversion in Elixir

Applied to files:

  • lib/ecto_libsql/query.ex
  • test/ecto_libsql_query_encoding_test.exs
📚 Learning: 2026-01-12T07:35:34.542Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-12T07:35:34.542Z
Learning: Use JSON functions from `EctoLibSql.JSON` module for working with JSON/JSONB data instead of raw SQL strings

Applied to files:

  • lib/ecto_libsql/query.ex
  • test/ecto_libsql_query_encoding_test.exs
📚 Learning: 2026-01-12T07:35:10.435Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T07:35:10.435Z
Learning: Applies to priv/repo/migrations/*.exs : For SQLite migrations requiring column type changes, use table recreation pattern: create new table with desired schema, copy data with transformation, drop old table, rename new table, recreate indexes (SQLite doesn't support ALTER COLUMN; SQLite < 3.35.0 doesn't support DROP COLUMN)

Applied to files:

  • lib/ecto_libsql/query.ex
📚 Learning: 2026-01-12T07:35:34.542Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-12T07:35:34.542Z
Learning: Use prepared statements with `EctoLibSql.Native.prepare/2` for queries executed more than once in a loop or high-frequency operation

Applied to files:

  • lib/ecto_libsql/query.ex
📚 Learning: 2026-01-12T07:35:34.542Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-12T07:35:34.542Z
Learning: Applies to priv/repo/migrations/*.exs : Use `STRICT` table option in migrations for type safety and to prevent accidental type coercion

Applied to files:

  • lib/ecto_libsql/query.ex
📚 Learning: 2026-01-12T07:35:10.435Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T07:35:10.435Z
Learning: Applies to test/**/*.{exs,ex} : For terminal test operations, use underscore variable names and assert error tuples with the pattern `assert {:error, %EctoLibSql.Error{}, _conn}`

Applied to files:

  • test/ecto_libsql_query_encoding_test.exs
📚 Learning: 2026-01-12T07:35:10.435Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T07:35:10.435Z
Learning: Applies to lib/**/*.ex : Verify schema types match database columns and check custom types have loaders/dumpers to fix type conversion errors

Applied to files:

  • test/ecto_libsql_query_encoding_test.exs
⏰ 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 (4)
test/ecto_libsql_query_encoding_test.exs (2)

169-182: Clarify expected behaviour for write operation columns/rows.

The PR objectives state "columns should be a list (not nil) for write commands", yet this test asserts that columns: nil and rows: nil are preserved for write operations. This appears to contradict the stated goal.

If normalisation happens in Native.ex (as the decode/3 docstring suggests), this test is correct for decode/3's pass-through behaviour. However, please verify the end-to-end behaviour aligns with Ecto/Oban expectations.


128-141: No action required. The test expectation is correct. Time.to_iso8601(~T[10:30:45]) outputs "10:30:45" because the Time struct has zero microsecond precision, and the ISO8601 format omits fractional seconds when precision is 0.

lib/ecto_libsql/query.ex (2)

41-48: LGTM!

The implementation correctly handles both list and non-list params. The guard clause ordering ensures list params are encoded while non-list params pass through safely, preventing crashes as mentioned in the PR objectives.


57-59: LGTM!

The pass-through implementation for decode/3 is appropriate given that Native.ex handles result normalisation. The comment clearly documents this design decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants