-
Notifications
You must be signed in to change notification settings - Fork 2
feat: add upsert method #137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -255,8 +255,12 @@ let anvil: Product = Product::find(&pool, anvil.id).await?; | |
| // Upsert (insert or update on PK conflict) | ||
| let anvil: Product = anvil.save(&pool).await?; | ||
|
|
||
| // Bulk upsert a collection | ||
| let products = vec![anvil]; | ||
| products.upsert(&pool, Product::ID).await?; | ||
|
|
||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 🤖 Prompt for AI Agents |
||
| # Ok(()) | ||
| # } | ||
| ``` | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| use crate::{ | ||
| database::Column, | ||
| dialect::Dialect, | ||
| model::{Model, Persist}, | ||
| }; | ||
|
|
||
| pub trait UniqueBy<M> { | ||
| fn column_names() -> &'static [&'static str]; | ||
| } | ||
|
|
||
| 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); | ||
|
Comment on lines
+11
to
+26
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial | 💤 Low value Tuple arity capped at 4 — document or extend.
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 |
||
|
|
||
| pub trait Upsert<DB: Dialect> { | ||
| type Model: Model; | ||
|
|
||
| fn upsert<'e, A, U>( | ||
| self, | ||
| executor: A, | ||
| unique_by: U, | ||
| ) -> impl Future<Output = Result<(), crate::Error>> | ||
| where | ||
| A: sqlx::Acquire<'e, Database = DB> + Send + 'e, | ||
| U: UniqueBy<Self::Model>; | ||
| } | ||
|
|
||
| impl<M, DB> Upsert<DB> for Vec<M> | ||
| where | ||
| M: Persist<DB> + Send, | ||
| DB: Dialect, | ||
| <DB as sqlx::Database>::Arguments: sqlx::IntoArguments<DB>, | ||
| for<'c> &'c mut <DB as sqlx::Database>::Connection: sqlx::Executor<'c, Database = DB>, | ||
| { | ||
| type Model = M; | ||
|
|
||
| async fn upsert<'e, A, U>(self, executor: A, _unique_by: U) -> Result<(), crate::Error> | ||
| where | ||
| A: sqlx::Acquire<'e, Database = DB> + Send + 'e, | ||
| U: UniqueBy<M>, | ||
| { | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| if self.is_empty() { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let mut conn = executor.acquire().await.map_err(crate::Error::from)?; | ||
|
|
||
| let mut qb = sqlx::QueryBuilder::new("INSERT INTO "); | ||
| qb.push(M::table_name()); | ||
| qb.push(" ("); | ||
| qb.push(M::columns().join(", ")); | ||
| qb.push(") "); | ||
|
|
||
| qb.push_values(self, |separated, model| { | ||
| model.push_bind_values(separated); | ||
| }); | ||
|
|
||
| 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?; | ||
|
Comment on lines
+71
to
+81
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 When all columns are part of the conflict target,
All three produce syntax errors. The semantically correct behavior is Add a guard at line 78 to call if update_cols.is_empty() {
qb.push(DB::do_nothing_sql());
} else {
qb.push(DB::do_update_sql(&update_cols));
}🤖 Prompt for AI Agents |
||
| Ok(()) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,6 +42,7 @@ impl<'a> FactoryCodegen<'a> { | |
| let factory_ident = &self.ident; | ||
| let factory_fields = self.generate_factory_fields(); | ||
| let factory_method_new = self.generate_factory_method_new(); | ||
| let factory_method_make = self.generate_factory_method_make(); | ||
| let factory_method_fields = self.generate_factory_method_fields(); | ||
| let factory_methods_for_relation = self.generate_factory_methods_for_relation(); | ||
| let factory_relation_fields = self.generate_factory_relation_fields(); | ||
|
|
@@ -87,6 +88,8 @@ impl<'a> FactoryCodegen<'a> { | |
| impl<DB: ::fabrique::Dialect> #factory_ident<DB> { | ||
| #factory_method_new | ||
|
|
||
| #factory_method_make | ||
|
|
||
| #(#factory_method_fields)* | ||
|
|
||
| #(#factory_methods_for_relation)* | ||
|
|
@@ -369,6 +372,47 @@ impl<'a> FactoryCodegen<'a> { | |
| } | ||
| } | ||
|
|
||
| /// 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,)* | ||
| } | ||
|
Comment on lines
+407
to
+411
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 |
||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+375
to
+414
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win Deduplicate field-construction logic shared with
Extract a small helper that returns the shared ♻️ 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 As per coding guidelines on essential refactors: code smells such as duplicate code (copy/paste, similar logic) should be removed. 🤖 Prompt for AI Agents |
||
|
|
||
| /// Generates setter methods for each field in the factory struct. | ||
| /// | ||
| /// Each setter method takes a value and stores it in the factory's optional | ||
|
|
@@ -661,6 +705,15 @@ mod tests { | |
| } | ||
| } | ||
|
|
||
| pub fn make(self) -> Anvil { | ||
| Anvil { | ||
| id: self.id.unwrap_or_else(::fabrique::seeded_value::<u32>), | ||
| hammer_id: self.hammer_id.unwrap_or_else(::fabrique::seeded_value::<u32>), | ||
| hardness: self.hardness.unwrap_or_else(::fabrique::seeded_value::<u32>), | ||
| weight: self.weight.unwrap_or_else(::fabrique::seeded_value::<u32>), | ||
| } | ||
| } | ||
|
|
||
| pub fn id(mut self, id: u32) -> Self { | ||
| self.id = Some(id); | ||
| self | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: 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:
Repository: robinstraub/fabrique
Length of output: 156
🏁 Script executed:
Repository: 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:
Repository: 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:
Repository: 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.mdlines 217–218) omit the turbofish and compile successfully. Update to match the consistent pattern elsewhere:🤖 Prompt for AI Agents