feat: add upsert method#137
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughImplements bulk upsert: adds UniqueBy and Upsert traits, extends Persist with push_bind_values for batched bind construction, generates bind helpers and factory.make(), exposes upsert via crate re-exports, adds tests, docs, and product migrations with composite UNIQUE constraints. ChangesBulk Upsert for fabrique Models
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #137 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 30 31 +1
Lines 3418 3491 +73
=========================================
+ Hits 3418 3491 +73 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@docs/src/concepts/factories.md`:
- Line 65: Update the example to remove the unnecessary turbofish type parameter
on Product::factory — Rust will infer the DB type from context, so replace the
explicit Product::factory::<sqlx::Sqlite>() call with a plain Product::factory()
and then chain the same builder methods (e.g., .name(...) and .make()) to match
other examples like the .make() usages in the cookbook.
In `@docs/src/concepts/models.md`:
- Line 263: Replace the call that destroys a random UUID with one that destroys
the actual record created earlier: use the created variable's id (anvil.id) when
calling Product::destroy instead of Uuid::new_v4(), so the example demonstrates
creating, operating on, and then cleaning up the same resource (reference
symbols: Product::destroy, anvil, anvil.id, Uuid::new_v4()).
In `@fabrique-core/src/upsert.rs`:
- Around line 11-25: The impl_unique_by! macro currently implements UniqueBy for
2–4 tuples (C0..C3) causing confusing errors for 5+ column composite keys;
extend support by adding additional macro invocations for higher arities (e.g.,
impl_unique_by!(C0, C1, C2, C3, C4); ... up to C7 for 8-tuples or further as
desired) so UniqueBy and its column_names() cover 5–8 element tuples, and also
add/update rustdoc on the UniqueBy trait to state the maximum supported tuple
arity (mention Column, UniqueBy, impl_unique_by!, and column_names for clarity).
- Around line 70-80: The code builds an empty update clause when all columns are
unique; guard against this by checking the computed update_cols (from
M::columns() filtered against U::column_names()) and, if it is empty, push
DB::do_nothing_sql() to qb instead of DB::do_update_sql(&update_cols); otherwise
keep the existing qb.push(DB::do_update_sql(&update_cols)). Ensure this change
is made where qb is assembled before qb.build().execute(&mut *conn).await so the
query uses DO NOTHING for empty update sets.
- Around line 27-53: The Upsert::upsert trait method must explicitly promise a
Send future and require U to be Send: change the trait method signature to
return "impl Future<Output = Result<(), crate::Error>> + Send + 'e" and add "U:
UniqueBy<Self::Model> + Send" to the where-clause; mirror those same bounds in
the Vec<M> impl (change the impl method's return type to "impl Future<Output =
Result<(), crate::Error>> + Send + 'e" and add "U: UniqueBy<M> + Send" in its
where-clause) so the trait contract and the concrete async impl are consistent
(refer to Upsert::upsert and the impl<M, DB> Upsert<DB> for Vec<M> upsert
method).
In `@fabrique-derive/src/codegen/factory.rs`:
- Around line 375-414: The duplicate logic that computes has_custom_faker,
fake_import, and the per-column column_fields in generate_factory_method_make is
identical to generate_factory_method_create; extract a private helper (e.g.,
build_fake_import_and_column_fields or prepare_field_defaults) that takes &self
and returns a tuple of TokenStreams (fake_import, column_fields) or a small
struct, implement the shared loop that inspects self.analysis.column_fields and
builds the per-field tokens and fake import there, and then call that helper
from both generate_factory_method_make and generate_factory_method_create to
replace the duplicated code paths (refer to generate_factory_method_make,
generate_factory_method_create, and the new helper name when updating callers).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 236092c4-1d23-451c-b177-ce7ea05c3a82
📒 Files selected for processing (13)
docs/src/concepts/factories.mddocs/src/concepts/models.mddocs/src/cookbook/bulk-update-and-upsert.mdfabrique-core/src/lib.rsfabrique-core/src/model.rsfabrique-core/src/upsert.rsfabrique-derive/src/codegen/columns.rsfabrique-derive/src/codegen/factory.rsfabrique-derive/src/codegen/persist.rsfabrique/src/lib.rsfabrique/src/prelude.rsfabrique/src/upsert.rsfabrique/tests/upsert.rs
| # price_cents: i32, | ||
| # } | ||
| # fn main() { | ||
| let product: Product = Product::factory::<sqlx::Sqlite>() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for other factory() calls without turbofish to see the pattern
rg -n 'factory\(\)' --type=md docs/ -A1 -B1Repository: robinstraub/fabrique
Length of output: 19004
🏁 Script executed:
sed -n '60,75p' docs/src/concepts/factories.mdRepository: robinstraub/fabrique
Length of output: 439
🏁 Script executed:
rg -n 'factory.*\).*\.make\(\)' --type=md docs/ -B2 -A2Repository: robinstraub/fabrique
Length of output: 629
🏁 Script executed:
fd -e rs 'factory' src/ | head -20Repository: robinstraub/fabrique
Length of output: 156
🏁 Script executed:
ls -la && find . -name "*.rs" -path "*/factory*" | head -10Repository: robinstraub/fabrique
Length of output: 1842
🏁 Script executed:
cat -n fabrique/src/factory.rs | head -100Repository: robinstraub/fabrique
Length of output: 3543
🏁 Script executed:
rg -n 'fn make\(' fabrique/src/factory.rs -A 5Repository: robinstraub/fabrique
Length of output: 46
🏁 Script executed:
rg -n 'fn make\(' fabrique-core/src/factory.rs -A 5Repository: robinstraub/fabrique
Length of output: 46
🏁 Script executed:
rg -n 'make' fabrique/src/factory.rs | head -20Repository: robinstraub/fabrique
Length of output: 46
🏁 Script executed:
wc -l fabrique/src/factory.rs && tail -100 fabrique/src/factory.rsRepository: robinstraub/fabrique
Length of output: 2680
🏁 Script executed:
cat -n fabrique-core/src/factory.rs | head -150Repository: robinstraub/fabrique
Length of output: 3086
🏁 Script executed:
cat -n fabrique-derive/src/codegen/factory.rs | head -200Repository: robinstraub/fabrique
Length of output: 8993
🏁 Script executed:
rg -n 'fn generate_factory_method_make' fabrique-derive/src/codegen/factory.rs -A 30Repository: robinstraub/fabrique
Length of output: 1146
🏁 Script executed:
rg -n 'fn generate_factory_method_make' fabrique-derive/src/codegen/factory.rs -A 50 | head -70Repository: robinstraub/fabrique
Length of output: 1955
Remove the unnecessary turbofish type parameter.
The example uses Product::factory::<sqlx::Sqlite>() but this explicit type specification is redundant. Rust can infer the database type from context. Other .make() examples in the docs (e.g., docs/src/cookbook/bulk-update-and-upsert.md lines 217–218) omit the turbofish and compile successfully. Update to match the consistent pattern elsewhere:
let product: Product = Product::factory()
.name("Anvil 3000".to_string())
.make();🤖 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 `@docs/src/concepts/factories.md` at line 65, Update the example to remove the
unnecessary turbofish type parameter on Product::factory — Rust will infer the
DB type from context, so replace the explicit Product::factory::<sqlx::Sqlite>()
call with a plain Product::factory() and then chain the same builder methods
(e.g., .name(...) and .make()) to match other examples like the .make() usages
in the cookbook.
|
|
||
| // Delete by primary key, no instance needed | ||
| Product::destroy(&pool, anvil.id).await?; | ||
| Product::destroy(&pool, Uuid::new_v4()).await?; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are similar destroy patterns in other docs
rg -n -C3 '::destroy\(' --type=md docs/Repository: robinstraub/fabrique
Length of output: 2577
🏁 Script executed:
# Get the full context around line 263 in models.md
sed -n '240,280p' docs/src/concepts/models.mdRepository: robinstraub/fabrique
Length of output: 920
Update destroy example to delete the created record for proper lifecycle demonstration.
The example creates an anvil record and demonstrates multiple operations on it, but then deletes a random Uuid::new_v4() instead of the anvil.id. This breaks the expected pattern of creating, using, and cleaning up the same resource. While the comment "no instance needed" suggests this was intentional to show the API capability, the same point can be demonstrated by using anvil.id—which actually destroys the record that was created—rather than a non-existent UUID. This provides a complete and realistic lifecycle example.
🤖 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 `@docs/src/concepts/models.md` at line 263, Replace the call that destroys a
random UUID with one that destroys the actual record created earlier: use the
created variable's id (anvil.id) when calling Product::destroy instead of
Uuid::new_v4(), so the example demonstrates creating, operating on, and then
cleaning up the same resource (reference symbols: Product::destroy, anvil,
anvil.id, Uuid::new_v4()).
| macro_rules! impl_unique_by { | ||
| ($($C:ident),+) => { | ||
| impl<M, $($C),+> UniqueBy<M> for ($($C,)+) | ||
| where | ||
| $($C: Column<M>,)+ | ||
| { | ||
| fn column_names() -> &'static [&'static str] { | ||
| &[$($C::NAME),+] | ||
| } | ||
| } | ||
| }; | ||
| } | ||
| impl_unique_by!(C0, C1); | ||
| impl_unique_by!(C0, C1, C2); | ||
| impl_unique_by!(C0, C1, C2, C3); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Tuple arity capped at 4 — document or extend.
impl_unique_by! only covers 2-, 3-, and 4-tuples. A user attempting a 5+-column composite conflict target (rare but valid for natural keys) will hit a confusing trait-bound error rather than a clear message. Single-column targets are covered by the per-column codegen, so that gap is intentional.
Either extend the macro to a higher arity (e.g., up to 8 or 12, mirroring std's tuple-trait conventions) or note the limit in the trait's rustdoc and in the cookbook section.
🤖 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 `@fabrique-core/src/upsert.rs` around lines 11 - 25, The impl_unique_by! macro
currently implements UniqueBy for 2–4 tuples (C0..C3) causing confusing errors
for 5+ column composite keys; extend support by adding additional macro
invocations for higher arities (e.g., impl_unique_by!(C0, C1, C2, C3, C4); ...
up to C7 for 8-tuples or further as desired) so UniqueBy and its column_names()
cover 5–8 element tuples, and also add/update rustdoc on the UniqueBy trait to
state the maximum supported tuple arity (mention Column, UniqueBy,
impl_unique_by!, and column_names for clarity).
| let unique_cols = U::column_names(); | ||
| qb.push(DB::on_conflict_sql(unique_cols)); | ||
|
|
||
| let update_cols: Vec<&str> = M::columns() | ||
| .iter() | ||
| .copied() | ||
| .filter(|c| !unique_cols.contains(c)) | ||
| .collect(); | ||
| qb.push(DB::do_update_sql(&update_cols)); | ||
|
|
||
| qb.build().execute(&mut *conn).await?; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find Dialect trait definition and impls for on_conflict_sql / do_update_sql
# to verify behavior when an empty slice is passed.
fd -e rs | xargs rg -nP -C5 '\bdo_update_sql\s*\('
echo "---"
fd -e rs | xargs rg -nP -C5 '\bon_conflict_sql\s*\('
echo "---"
# Look for any explicit empty-slice guard
fd -e rs | xargs rg -nP -C2 'is_empty\(\)|len\(\)\s*==\s*0' | rg -i 'do_update|conflict'Repository: robinstraub/fabrique
Length of output: 13107
🏁 Script executed:
sed -n '40,50p' fabrique-core/src/dialect.rsRepository: robinstraub/fabrique
Length of output: 438
🏁 Script executed:
sed -n '68,78p' fabrique-core/src/dialect.rsRepository: robinstraub/fabrique
Length of output: 408
🏁 Script executed:
sed -n '96,106p' fabrique-core/src/dialect.rsRepository: robinstraub/fabrique
Length of output: 408
🏁 Script executed:
# Also check the complete upsert function to see context
sed -n '60,85p' fabrique-core/src/upsert.rsRepository: robinstraub/fabrique
Length of output: 743
Guard against empty update_cols slice.
When all columns are part of the conflict target, update_cols becomes empty. The current dialect implementations generate incomplete SQL for this case:
- Postgres/Sqlite:
" DO UPDATE SET "(incomplete SET clause) - MySql:
" ON DUPLICATE KEY UPDATE "(incomplete UPDATE clause)
All three produce syntax errors. The semantically correct behavior is DO NOTHING.
Add a guard at line 78 to call DB::do_nothing_sql() when update_cols.is_empty():
if update_cols.is_empty() {
qb.push(DB::do_nothing_sql());
} else {
qb.push(DB::do_update_sql(&update_cols));
}🤖 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 `@fabrique-core/src/upsert.rs` around lines 70 - 80, The code builds an empty
update clause when all columns are unique; guard against this by checking the
computed update_cols (from M::columns() filtered against U::column_names()) and,
if it is empty, push DB::do_nothing_sql() to qb instead of
DB::do_update_sql(&update_cols); otherwise keep the existing
qb.push(DB::do_update_sql(&update_cols)). Ensure this change is made where qb is
assembled before qb.build().execute(&mut *conn).await so the query uses DO
NOTHING for empty update sets.
| /// Generates the `make()` method that builds a model instance | ||
| /// without persisting it. | ||
| fn generate_factory_method_make(&self) -> TokenStream { | ||
| let struct_ident = &self.analysis.ident; | ||
|
|
||
| let has_custom_faker = self | ||
| .analysis | ||
| .column_fields | ||
| .iter() | ||
| .any(|f| f.faker.is_some()); | ||
|
|
||
| let fake_import = if has_custom_faker { | ||
| quote! { use ::fabrique::fake::Fake; } | ||
| } else { | ||
| quote! {} | ||
| }; | ||
|
|
||
| let column_fields = self.analysis.column_fields.iter().map(|field| { | ||
| let name = &field.ident; | ||
| let ty = &field.ty; | ||
|
|
||
| match &field.faker { | ||
| Some(faker_expr) => quote! { | ||
| #name: self.#name.unwrap_or_else(|| #faker_expr.fake()) | ||
| }, | ||
| None => quote! { | ||
| #name: self.#name.unwrap_or_else(::fabrique::seeded_value::<#ty>) | ||
| }, | ||
| } | ||
| }); | ||
|
|
||
| quote! { | ||
| pub fn make(self) -> #struct_ident { | ||
| #fake_import | ||
| #struct_ident { | ||
| #(#column_fields,)* | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Deduplicate field-construction logic shared with generate_factory_method_create.
has_custom_faker, fake_import, and the per-column column_fields mapping at lines 380-404 are a verbatim copy of lines 286-314 in generate_factory_method_create. Any future change to the defaulting strategy (new field attributes, alternate seed source, etc.) will need to be applied in both places, and these two paths will silently drift on the first miss.
Extract a small helper that returns the shared (fake_import, column_fields) token streams and call it from both methods.
♻️ Sketch of a shared helper
+ /// Shared field-construction tokens used by both `create()` and
+ /// `make()`. Returns `(fake_import, column_field_initializers)`.
+ fn generate_field_initializers(
+ &self,
+ ) -> (TokenStream, Vec<TokenStream>) {
+ let has_custom_faker = self
+ .analysis
+ .column_fields
+ .iter()
+ .any(|f| f.faker.is_some());
+
+ let fake_import = if has_custom_faker {
+ quote! { use ::fabrique::fake::Fake; }
+ } else {
+ quote! {}
+ };
+
+ let column_fields = self
+ .analysis
+ .column_fields
+ .iter()
+ .map(|field| {
+ let name = &field.ident;
+ let ty = &field.ty;
+ match &field.faker {
+ Some(faker_expr) => quote! {
+ `#name`: self.#name.unwrap_or_else(|| `#faker_expr.fake`())
+ },
+ None => quote! {
+ `#name`: self.#name.unwrap_or_else(::fabrique::seeded_value::<#ty>)
+ },
+ }
+ })
+ .collect();
+
+ (fake_import, column_fields)
+ }
+
fn generate_factory_method_make(&self) -> TokenStream {
let struct_ident = &self.analysis.ident;
-
- let has_custom_faker = self
- .analysis
- .column_fields
- .iter()
- .any(|f| f.faker.is_some());
-
- let fake_import = if has_custom_faker {
- quote! { use ::fabrique::fake::Fake; }
- } else {
- quote! {}
- };
-
- let column_fields = self.analysis.column_fields.iter().map(|field| { /* ... */ });
+ let (fake_import, column_fields) = self.generate_field_initializers();
quote! {
pub fn make(self) -> `#struct_ident` {
`#fake_import`
`#struct_ident` {
#(`#column_fields`,)*
}
}
}
}Then mirror the same call inside generate_factory_method_create in place of lines 286-314.
As per coding guidelines on essential refactors: code smells such as duplicate code (copy/paste, similar logic) should be removed.
🤖 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 `@fabrique-derive/src/codegen/factory.rs` around lines 375 - 414, The duplicate
logic that computes has_custom_faker, fake_import, and the per-column
column_fields in generate_factory_method_make is identical to
generate_factory_method_create; extract a private helper (e.g.,
build_fake_import_and_column_fields or prepare_field_defaults) that takes &self
and returns a tuple of TokenStreams (fake_import, column_fields) or a small
struct, implement the shared loop that inspects self.analysis.column_fields and
builds the per-field tokens and fake import there, and then call that helper
from both generate_factory_method_make and generate_factory_method_create to
replace the duplicated code paths (refer to generate_factory_method_make,
generate_factory_method_create, and the new helper name when updating callers).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
fabrique-derive/src/codegen/factory.rs (1)
380-404: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winExtract shared faker/default field token generation to avoid drift.
This block is duplicated from
generate_factory_method_createand will drift on the next change to defaulting/faker behavior. Please centralize it in a private helper and reuse in both methods.🤖 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 `@fabrique-derive/src/codegen/factory.rs` around lines 380 - 404, The duplication builds the same faker/default tokens (has_custom_faker, fake_import, and column_fields iteration) used in generate_factory_method_create; extract that logic into a private helper method (e.g., a method on the same impl like shared_faker_tokens or build_faker_tokens) that takes &self (or &self.analysis) and returns the fake_import token and the column_fields token stream/iterator so both generate_factory_method_create and the other caller reuse it; replace the duplicated block with a call to that helper and use its returned tokens, preserving existing behavior for field.faker, ::fabrique::seeded_value, and the Fake trait usage.
🤖 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 `@fabrique-derive/src/codegen/factory.rs`:
- Around line 407-411: The make() implementation currently only constructs
`#struct_ident` from `#column_fields` and ignores any stored relation values (fields
named like *_relation), causing for_<relation>() setters to be no-ops; update
make() to consume each relation field (e.g., fields matching /_relation$/) and
apply them to the resulting struct by converting the relation into its foreign
key columns (or using the relation's primary key) so FK fields are set from the
provided relation instead of seeded defaults; locate the make() method and add
logic that checks each *_relation field, extracts the needed key(s) (or calls
the relation's make/into_key helper), and override the corresponding FK column
entries in the built `#struct_ident` accordingly.
In `@fabrique/tests/upsert.rs`:
- Around line 101-105: The test currently asserts that
products.clone().upsert(&pool, (Product::ID, Product::NAME)).await returns an
error, but that expectation is dialect-dependent; update the test to branch on
the DB dialect (using the test helper or pool/dialect accessor available in your
test harness) and only assert is_err() for dialects that use ON CONFLICT (e.g.,
Postgres/SQLite), while for MySQL assert the call succeeds or matches MySQL's
expected behavior. Locate the upsert call in the test (products.upsert,
Product::ID/Product::NAME, pool) and add a small conditional: detect MySQL and
assert the opposite (or skip) otherwise assert is_err().
---
Duplicate comments:
In `@fabrique-derive/src/codegen/factory.rs`:
- Around line 380-404: The duplication builds the same faker/default tokens
(has_custom_faker, fake_import, and column_fields iteration) used in
generate_factory_method_create; extract that logic into a private helper method
(e.g., a method on the same impl like shared_faker_tokens or build_faker_tokens)
that takes &self (or &self.analysis) and returns the fake_import token and the
column_fields token stream/iterator so both generate_factory_method_create and
the other caller reuse it; replace the duplicated block with a call to that
helper and use its returned tokens, preserving existing behavior for
field.faker, ::fabrique::seeded_value, and the Fake trait usage.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ff214748-9bcd-4d0f-94e8-d755c635d135
📒 Files selected for processing (3)
fabrique-core/src/upsert.rsfabrique-derive/src/codegen/factory.rsfabrique/tests/upsert.rs
| pub fn make(self) -> #struct_ident { | ||
| #fake_import | ||
| #struct_ident { | ||
| #(#column_fields,)* | ||
| } |
There was a problem hiding this comment.
make() ignores for_<relation>() inputs, causing inconsistent factory behavior.
make() only materializes column_fields and never consumes *_relation; so relation setters become no-ops on this path, and FK fields may be seeded instead of using caller-provided relations. This is a functional gap versus the fluent API surface.
Suggested direction
pub fn make(mut self) -> `#struct_ident` {
+ // Resolve belongs_to relation fields into FK columns for in-memory build.
+ // - PrimaryKey variant: assign FK directly.
+ // - Factory variant: return an error or panic with a clear message since no DB executor exists.
+ // (Preferably adjust make() signature to Result<..., fabrique::Error> if you want non-panicking behavior.)
+
`#fake_import`
`#struct_ident` {
#(`#column_fields`,)*
}
}🤖 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 `@fabrique-derive/src/codegen/factory.rs` around lines 407 - 411, The make()
implementation currently only constructs `#struct_ident` from `#column_fields` and
ignores any stored relation values (fields named like *_relation), causing
for_<relation>() setters to be no-ops; update make() to consume each relation
field (e.g., fields matching /_relation$/) and apply them to the resulting
struct by converting the relation into its foreign key columns (or using the
relation's primary key) so FK fields are set from the provided relation instead
of seeded defaults; locate the make() method and add logic that checks each
*_relation field, extracts the needed key(s) (or calls the relation's
make/into_key helper), and override the corresponding FK column entries in the
built `#struct_ident` accordingly.
| let result = products | ||
| .clone() | ||
| .upsert(&pool, (Product::ID, Product::NAME)) | ||
| .await; | ||
| assert!(result.is_err()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify dialect-specific upsert SQL behavior and feature flags before applying cfg gating.
# 1) Inspect Dialect impls for conflict SQL shape.
fd -e rs | xargs rg -nP -C3 'impl\s+Dialect\s+for\s+(Postgres|Sqlite|MySql)|fn\s+on_conflict_sql|fn\s+do_update_sql'
# 2) Confirm actual feature names used by this workspace.
rg -nP -C2 '^\[features\]|postgres|sqlite|mysql' -- */Cargo.toml Cargo.tomlRepository: robinstraub/fabrique
Length of output: 4885
🏁 Script executed:
# Read the test file around lines 101-105
head -120 fabrique/tests/upsert.rs | tail -30Repository: robinstraub/fabrique
Length of output: 891
🏁 Script executed:
# Check if the test file uses any dialect-conditional compilation
rg -n '#\[cfg\(' fabrique/tests/upsert.rsRepository: robinstraub/fabrique
Length of output: 99
🏁 Script executed:
# Look at the full upsert.rs test context, focusing on the Product struct and test setup
head -110 fabrique/tests/upsert.rsRepository: robinstraub/fabrique
Length of output: 2879
Make the invalid-composite assertion dialect-aware.
Lines 101–105 test that upserting with (Product::ID, Product::NAME) must error, but this behavior is dialect-dependent. MySQL's on_conflict_sql() returns an empty string (using different INSERT ... ON DUPLICATE KEY UPDATE semantics), while Postgres and SQLite use ON CONFLICT clauses. The test will fail incorrectly on MySQL when that feature is enabled.
Suggested change
let result = products
.clone()
.upsert(&pool, (Product::ID, Product::NAME))
.await;
- assert!(result.is_err());
+ #[cfg(any(feature = "postgres", feature = "sqlite"))]
+ assert!(result.is_err());
+ #[cfg(feature = "mysql")]
+ assert!(result.is_ok());📝 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.
| let result = products | |
| .clone() | |
| .upsert(&pool, (Product::ID, Product::NAME)) | |
| .await; | |
| assert!(result.is_err()); | |
| let result = products | |
| .clone() | |
| .upsert(&pool, (Product::ID, Product::NAME)) | |
| .await; | |
| #[cfg(any(feature = "postgres", feature = "sqlite"))] | |
| assert!(result.is_err()); | |
| #[cfg(feature = "mysql")] | |
| assert!(result.is_ok()); |
🤖 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 `@fabrique/tests/upsert.rs` around lines 101 - 105, The test currently asserts
that products.clone().upsert(&pool, (Product::ID, Product::NAME)).await returns
an error, but that expectation is dialect-dependent; update the test to branch
on the DB dialect (using the test helper or pool/dialect accessor available in
your test harness) and only assert is_err() for dialects that use ON CONFLICT
(e.g., Postgres/SQLite), while for MySQL assert the call succeeds or matches
MySQL's expected behavior. Locate the upsert call in the test (products.upsert,
Product::ID/Product::NAME, pool) and add a small conditional: detect MySQL and
assert the opposite (or skip) otherwise assert is_err().
1 participant
resolves #136
Summary by CodeRabbit
New Features
Documentation
Tests
Database Migrations