Conversation
WalkthroughThe SQLite binding API changed from bind/3 to bind/2, removing the connection argument. Repo and tests were updated accordingly. SQLite.bind/2 now wraps Driver.bind with try/rescue and logs ArgumentError, returning {:error, :arguments_wrong_length}. Dependencies were bumped (exqlite ~> 0.33, excoveralls ~> 0.18.5). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller as Repo
participant SQL as Feeb.DB.SQLite
participant Driver as Exqlite.Driver
participant Log as Logger
Caller->>SQL: prepare(conn, sql)
SQL-->>Caller: {:ok, stmt}
Caller->>SQL: bind(stmt, bindings)
rect rgba(200,230,255,0.3)
SQL->>Driver: bind(stmt, bindings)
alt ArgumentError raised
SQL->>Log: log error (ArgumentError)
SQL-->>Caller: {:error, :arguments_wrong_length}
else Success
SQL-->>Caller: :ok
end
end
opt On successful bind
Caller->>SQL: all(conn, stmt)
SQL-->>Caller: {:ok, rows}
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
renatomassaro
force-pushed
the
bump-dependencies
branch
from
2a49deb to
c40f57a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
lib/feeb/db/sqlite.ex (1)
44-52: bind/2: good API alignment; refine exception logging and add a typespec.Catching
ArgumentErrorto normalize into{:error, :arguments_wrong_length}is sensible. Two tweaks recommended:
- Log the exception with stacktrace for better debuggability.
- Add a typespec for
bind/2to document the contract.Apply:
def bind(stmt, bindings) when is_list(bindings) do + @spec bind(stmt(), list()) :: :ok | {:error, :arguments_wrong_length} try do Driver.bind(stmt, bindings) rescue e in ArgumentError -> - Logger.error(e) + Logger.error(Exception.format(:error, e, __STACKTRACE__)) {:error, :arguments_wrong_length} end endOptional: include metadata like the number of args (not their values) to avoid logging PII, e.g.,
"args_count=#{length(bindings)}".Also note you kept
bind/3only for the[]case; any remaining 3-arity, non-empty calls will now fail at compile time — which is fine, just ensure none remain (see scan in mix.exs comment).Would you like me to also add a unit test that verifies we log (captured via
@tag :capture_log) and return{:error, :arguments_wrong_length}when passing a wrong number of parameters?lib/feeb/db/repo.ex (2)
235-241: Switch to SQLite.bind/2 is correct; consider a clearer error for argument-length mismatches.The
withclause now correctly expects:okfromSQLite.bind/2. If bind fails with{:error, :arguments_wrong_length}, it flows to the genericelseand logs an opaque error. Consider handling that case explicitly to aid debugging:with {:ok, {stmt, stmt_sql}} <- prepare_query(state, query_id, sql), true = stmt_sql == sql, :ok <- SQLite.bind(stmt, bindings_values), {:ok, rows} <- SQLite.all(state.conn, stmt) do … else + {:error, :arguments_wrong_length} = err -> + Logger.error("Wrong number of parameters for #{inspect(query_id)} (got #{length(bindings_values)})") + {:reply, err, state} {:error, _} = err -> Logger.error("error: #{inspect(err)}") {:reply, err, state} end
257-266: Unreachable code afterraise "Remove or document usage"in prepared_raw path.This callback raises unconditionally; the code below will never run and can be deleted to avoid confusion. If you intend to keep it for future work, replace
raisewith a feature flag orLogger.warning/1plus an explicit error reply.def handle_call({:prepared_raw, raw_sql, bindings_values, opts}, _from, state) do - raise "Remove or document usage" - - with {:ok, stmt} <- SQLite.prepare(state.conn, raw_sql), - :ok <- SQLite.bind(stmt, bindings_values), - {:ok, rows} <- SQLite.all(state.conn, stmt) do - schema = Keyword.fetch!(opts, :schema) - result = format_custom(:all, schema, rows) - {:reply, result, state} - else - {:error, _} = err -> - Logger.error("error: #{inspect(err)}") - # TODO: Rollback? - {:reply, err, state} - end + {:reply, {:error, :prepared_raw_disabled}, state} endtest/db/sqlite_test.exs (2)
27-32: Good negative test; also update the describe title from bind/3 to bind/2.The assertions for wrong-length args are correct and
@tag :capture_logprevents noisy logs. The section header still says"bind/3"though; update for clarity.Apply:
-describe "bind/3" do +describe "bind/2" doOptional: add one case for "too few args" on a 2-parameter statement to cover both directions.
102-102: Assert the bind/2 result to surface failures earlier.Non-assertive call can hide a bind failure; assert it for consistency with other tests.
- SQLite.bind(stmt, ["Jessie", 1]) + assert :ok = SQLite.bind(stmt, ["Jessie", 1])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
mix.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
lib/feeb/db/repo.ex(2 hunks)lib/feeb/db/sqlite.ex(2 hunks)mix.exs(1 hunks)test/db/sqlite_test.exs(2 hunks)
🔇 Additional comments (5)
mix.exs (1)
57-61: All exqlite bind/3 calls removed and mix.lock updatedI’ve confirmed that there are no remaining genuine 3-arity calls to
SQLite.bind/3(all test uses only call the 2-arity API with a list of parameters), andmix.lockpins exqlite at version 0.33.0. This change is safe to merge.• No
SQLite.bind/3call sites found upon refined search
•mix.lockshows"exqlite": "0.33.0"as expectedlib/feeb/db/sqlite.ex (1)
7-8: Requiring Logger is appropriate given new error handling.No issues. This enables structured logging for the new error path in bind/2.
test/db/sqlite_test.exs (3)
22-25: LGTM: bind/2 happy paths covered for both parameterized and zero-parameter statements.This validates the new arity on both “SELECT … WHERE id = ?” and “BEGIN”.
38-38: LGTM: one/2 with a bound parameter.Covers the common success case after
bind/2.
44-44: LGTM: one/2 nil path with bind/2.This exercises the not-found case with the new binding API.
1 participant
Summary by CodeRabbit