chore: drop sqlx-crud from mostro-core#155
Conversation
Remove the unmaintained sqlx-crud dependency and SqlxCrud derives now that Order and Dispute use db::Crud. Centralize Order INSERT columns so insert/update SQL cannot drift from the bind list. Co-authored-by: Cursor <cursoragent@cursor.com>
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughRemoves the ChangesRemove sqlx-crud and replace with custom SQL binding
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
I did a pass on this PR and I do not currently see a blocker. This looks like the right follow-up to #154. What I specifically checked:
From what I can see, the direction is cleaner now: the persistence story is more explicit, the dependency surface is smaller, and the risk of the old macro path drifting away from the in-tree path is gone. I also like the move toward a single So from my side this looks like a solid cleanup PR and a sensible completion step for the |
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed current head. The sqlx-crud removal looks clean and the new Crud path is well-scoped. One small nit: order_insert_bind_count() in src/db/order.rs currently returns ORDER_INSERT_COLUMNS.len(), so the new guard test compares the constant to itself and doesn't really catch bind-list drift. If you want that test to protect against regressions, consider asserting the generated bind count or removing the helper.
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Strict review
Dropping sqlx-crud and the SqlxCrud derives is correct and well done — Order, Dispute, and the four Restored* helpers only need FromRow. I verified locally with the sqlx feature: build is green, 88 tests pass, clippy and fmt are clean, and the generated SQL is byte-for-byte equivalent to the previous hand-written builders. Cargo.lock is gitignored (library crate), so no lockfile churn is expected.
My concerns are entirely about the db/order.rs binding refactor that ships alongside the dependency removal. It does not deliver the safety the PR description claims, and it trades compile-time guarantees for a runtime panic path.
1. The "guard test" asserts nothing
The description sells "a guard test so column names and bind loops cannot diverge", but insert_column_list_matches_bind_count is tautological: order_insert_bind_count() returns ORDER_INSERT_COLUMNS.len(), so the headline assertion is len == len. The third assertion is also a tautology (order_update_columns() is literally &ORDER_INSERT_COLUMNS[1..]). The only real guard against drift is the roundtrip integration test, not this unit test. See inline comment.
2. Stringly-typed dispatch replaces compile errors with runtime panics
The old code was verbose but fully compile-checked. The new match column { "id" => ..., other => panic!(...) } means a column added to ORDER_INSERT_COLUMNS without a matching arm is no longer a compile error — it is a panic on the first INSERT/UPDATE in production. See inline comment.
3. "Single source of truth" is only partially true
There are now three full enumerations of the 46 columns that must stay in sync: ORDER_INSERT_COLUMNS, the 46 match arms in bind_order_column, and the Order struct / its FromRow. The refactor only removes the desync between the INSERT column list and the INSERT bind order (they now share one iterator) — a real but narrow win. The name→value mapping is still manual and can still drift; it now fails at runtime instead of at compile time. The drift surface was moved, not eliminated, at the cost of static checking.
Recommendation
- Ship the
sqlx-crudremoval as-is. - Reconsider the
db/order.rsrefactor. Either keep the directpush_bindsequence (compile-time safe) plus theORDER_INSERT_COLUMNSconstant for names and rely on the roundtrip test, or — if a single exhaustive source is the goal — drivebind_order_columnoff an exhaustiveenum OrderColumninstead of&str, so the compiler forces every new column to be handled. That would actually make divergence impossible. - At minimum, fix or delete
insert_column_list_matches_bind_count; as written it gives false confidence.
| "cashu_mint_url" => bind_column_value(sep, &order.cashu_mint_url, style), | ||
| "cashu_escrow_token" => bind_column_value(sep, &order.cashu_escrow_token, style), | ||
| "cashu_escrow_locked_at" => bind_column_value(sep, order.cashu_escrow_locked_at, style), | ||
| other => panic!("unmapped order column: {other}"), |
There was a problem hiding this comment.
Compile-time safety regression. This panic! arm only exists because dispatch is now keyed on a &str column name. If a column is added to ORDER_INSERT_COLUMNS but its match arm is forgotten, this is no longer a compile error — it panics on the first INSERT/UPDATE at runtime. The previous direct push_bind sequence had this checked by the compiler. If you want a single exhaustive source, key this on an exhaustive enum OrderColumn instead of &str so the compiler forces every new column to be handled.
| /// Mirrors the INSERT bind loop in [`push_order_insert_binds`]. | ||
| fn order_insert_bind_count() -> usize { | ||
| ORDER_INSERT_COLUMNS.len() | ||
| } | ||
|
|
||
| #[test] | ||
| fn insert_column_list_matches_bind_count() { | ||
| assert_eq!(ORDER_INSERT_COLUMNS.len(), order_insert_bind_count()); | ||
| assert_eq!(ORDER_INSERT_COLUMNS.first(), Some(&"id")); | ||
| assert_eq!(order_update_columns().len(), ORDER_INSERT_COLUMNS.len() - 1); | ||
| } |
There was a problem hiding this comment.
This guard test does not guard anything. order_insert_bind_count() returns ORDER_INSERT_COLUMNS.len(), so line 223 asserts len == len — a tautology. Line 225 is also tautological: order_update_columns() is defined as &ORDER_INSERT_COLUMNS[1..], so its length is always len - 1. Only the first() == Some(&"id") check carries weak signal. The PR description cites this as the mechanism that keeps column names and bind loops from diverging, but the actual divergence guard is the roundtrip test (which would hit the panic! arm). Please fix it to test something real (e.g. bind a sample order and assert no unmapped-column panic) or delete it — as-is it gives false confidence.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/db/order.rs`:
- Around line 61-107: The `Order` struct is missing a field that exists in the
`ORDERS_DDL` schema named `dispute_initiator_pubkey`, and the corresponding
binding call is missing from the INSERT statement. Add the
`dispute_initiator_pubkey` field to the `Order` struct with the appropriate
type, include it in the `ORDER_INSERT_COLUMNS` constant between
`cancel_initiator_pubkey` and `buyer_pubkey`, and add a
`.push_bind(&order.dispute_initiator_pubkey)` call in the binding function
between the push_bind calls for `cancel_initiator_pubkey` and `buyer_pubkey` to
match the schema column order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/db/order.rs`:
- Around line 61-107: The `Order` struct is missing a field that exists in the
`ORDERS_DDL` schema named `dispute_initiator_pubkey`, and the corresponding
binding call is missing from the INSERT statement. Add the
`dispute_initiator_pubkey` field to the `Order` struct with the appropriate
type, include it in the `ORDER_INSERT_COLUMNS` constant between
`cancel_initiator_pubkey` and `buyer_pubkey`, and add a
`.push_bind(&order.dispute_initiator_pubkey)` call in the binding function
between the push_bind calls for `cancel_initiator_pubkey` and `buyer_pubkey` to
match the schema column order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53484cd4-7c54-4b26-b56e-83fd24840049
📒 Files selected for processing (1)
src/db/order.rs
🛑 Comments failed to post (1)
src/db/order.rs (1)
61-107: 🗄️ Data Integrity & Integration | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Check if dispute_initiator_pubkey exists in Order struct and ORDER_INSERT_COLUMNS # Check Order struct definition echo "=== Order struct fields ===" rg -n 'pub struct Order' -A 100 src/order.rs | rg 'dispute_initiator_pubkey' # Check ORDER_INSERT_COLUMNS constant echo -e "\n=== ORDER_INSERT_COLUMNS ===" sed -n '11,59p' src/db/order.rs | rg -C2 'dispute_initiator_pubkey|ORDER_INSERT_COLUMNS' # Verify column count echo -e "\n=== Column count analysis ===" echo "Schema columns (from test_support.rs):" rg 'CREATE TABLE orders' -A 50 src/db/test_support.rs | rg '^\s+\w+' | wc -l echo "Bind calls in push_order_insert_binds:" sed -n '61,107p' src/db/order.rs | rg 'push_bind' | wc -lRepository: MostroP2P/mostro-core
Length of output: 395
🏁 Script executed:
#!/bin/bash echo "=== Full ORDER_INSERT_COLUMNS constant ===" sed -n '11,59p' src/db/order.rs echo -e "\n=== Full Order struct definition ===" rg -n 'pub struct Order' -A 200 src/order.rs | head -150 echo -e "\n=== ORDERS_DDL schema definition (full) ===" rg 'CREATE TABLE orders' -A 60 src/db/test_support.rsRepository: MostroP2P/mostro-core
Length of output: 10399
Fix database schema or struct definition:
dispute_initiator_pubkeycolumn mismatch.The
ORDERS_DDLschema definesdispute_initiator_pubkeyas a column (betweencancel_initiator_pubkeyandbuyer_pubkey), but theOrderstruct has no corresponding field, andORDER_INSERT_COLUMNSdoes not include it. This causes a schema-struct mismatch:
- INSERT statements provide 46 columns but the schema expects 47.
- SELECT with
FromRowwill fail when trying to deserialize the extra column.Either remove
dispute_initiator_pubkeyfrom theORDERS_DDLschema, or add the field to theOrderstruct and include it inORDER_INSERT_COLUMNSand both binding functions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/order.rs` around lines 61 - 107, The `Order` struct is missing a field that exists in the `ORDERS_DDL` schema named `dispute_initiator_pubkey`, and the corresponding binding call is missing from the INSERT statement. Add the `dispute_initiator_pubkey` field to the `Order` struct with the appropriate type, include it in the `ORDER_INSERT_COLUMNS` constant between `cancel_initiator_pubkey` and `buyer_pubkey`, and add a `.push_bind(&order.dispute_initiator_pubkey)` call in the binding function between the push_bind calls for `cancel_initiator_pubkey` and `buyer_pubkey` to match the schema column order.
|
I re-checked the current head after the follow-up changes, and the concerns raised in the strict review now look addressed to me. In particular:
The current version is more explicit again: That is less clever, but also more robust, and I think it is the right trade here. So from my side, the review comments about the reverted last commit now look satisfied. |
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Strict review — chore: drop sqlx-crud from mostro-core
Verdict: APPROVE ✅
This is a clean, well-scoped, behavior-preserving change that removes an unmaintained dependency. It builds, lints, and tests green. The notes below are non-blocking (maintainability / documentation accuracy) and can be addressed in this PR or a follow-up.
What I verified locally (PR branch)
| Check | Result |
|---|---|
cargo fmt --all -- --check |
✅ clean |
cargo clippy --features sqlx --all-targets -- -D warnings |
✅ no warnings |
cargo test --features sqlx |
✅ 87 tests + 4 doctests, 0 failures |
cargo build (default features) |
✅ |
cargo build --no-default-features |
❌ fails — pre-existing on main (wasm_bindgen unguarded), NOT a regression from this PR |
Residual sqlx_crud / SqlxCrud / external_id references |
✅ none, except intentional doc comments in src/db/mod.rs |
sqlx-crud in Cargo.lock |
✅ removed |
Correctness
- Removing the
SqlxCrudderive fromOrderandDisputeis safe: persistence now goes exclusively throughmostro_core::db::Crud(landed in #154), and thedb::order/db::disputeround-trip tests exercisecreate/update/by_idend to end. - The four
Restored*helpers inmessage.rsonly ever neededFromRow(they are read-onlySELECTprojections), so droppingSqlxCrudthere has no functional impact. - The
ORDER_INSERT_COLUMNSrefactor is behavior-preserving. I diffed the three parallel lists:ORDER_INSERT_COLUMNS(46) — identical sequence to thepush_order_insert_bindsbind order (46). ✅push_order_update_set(45) — exactlyINSERTcolumns minusid(theWHEREkey). ✅
Non-blocking observations
-
(MEDIUM) The PR description overstates the refactor. It says the change drives "INSERT/UPDATE SQL from a single
ORDER_INSERT_COLUMNSlist," but:update()/push_order_update_setdoes not use the constant — it still hardcodes every column name inline.- The constant drives only the INSERT column names, not the bind list (
push_order_insert_binds).
So there are still three lists that must be kept in sync (the const, the insert binds, the update set), not one. The net effect is closer to "documented + reusable INSERT column list" than "single source of truth." Suggest softening the description.
-
(MEDIUM) The "guard test" claim is weaker than stated. The doc comment says "Drift is caught by the roundtrip integration tests." In practice
create_by_id_roundtripmostly assertsfetched == created(self-consistency) rather than against known expected values, so a same-typed column/bind transposition (e.g. twoi64columns swapped) would not necessarily be detected, and nothing assertsORDER_INSERT_COLUMNS.len()equals the bind count at compile time or in a test. A cheap, high-value guard would be a unit test asserting the column count, and/or asserting distinct sentinel values per field insample_order. Note this risk is not newly introduced by this PR — it is roughly equal to the prior inline-list approach. -
(LOW) Cross-repo invariant in a comment.
ORDER_INSERT_COLUMNSdocuments that it must stay in sync withmostrodmigrations. That's a fragile cross-repo coupling to track via a comment, but acceptable given the planned follow-upmostroPR.
Conclusion
The primary goal — eliminating the unmaintained sqlx-crud dependency — is achieved cleanly with no behavioral change and full green CI locally. Approving. Please consider tightening the description (point 1) and adding a small length-assertion guard test (point 2).
2 participants
Summary
sqlx-cruddependency andSqlxCrudderives fromOrder,Dispute, and the fourRestored*helpers (they only needFromRow).Order/Disputeis exclusively viamostro_core::db::Crud(landed in feat: add local Crud trait for Order and Dispute #154).db/order.rsto drive INSERT/UPDATE SQL from a singleORDER_INSERT_COLUMNSlist, with a guard test so column names and bind loops cannot diverge.Follow-up (separate
mostroPR): implementCrudforBond, switch daemon imports tomostro_core::db::Crud, and removesqlx-crudfrom mostrod.Summary by CodeRabbit