refactor(infra): standardize SQL query construction across repositories

- Add push_update_field! macro to reduce UPDATE query boilerplate
- Rename sort_clause() to order_clause() for consistency
- Convert bags.rs update method from string concatenation to QueryBuilder
- Apply macro to roasters.rs, roasts.rs, and bags.rs update methods

This reduces ~100 lines of repetitive code and ensures consistent
patterns for building dynamic UPDATE queries across all repositories.

src/application/routes/bags.rs

@@ -217,10 +217,10 @@ pub(crate) async fn update_bag(
         finished_at: body_update.finished_at.or(update_params.finished_at),
     };
 
-    if let Some(true) = update.closed {
+    if let Some(true) = update.closed
-        if update.finished_at.is_none() {
+        && update.finished_at.is_none()
-            update.finished_at = Some(chrono::Utc::now().date_naive());
+    {
-        }
+        update.finished_at = Some(chrono::Utc::now().date_naive());
     }
 
     let bag = state
@@ -231,28 +231,28 @@ pub(crate) async fn update_bag(
 
     if let Some(true) = update.closed {
         // Fetch roast and roaster for timeline event
-        if let Ok(roast) = state.roast_repo.get(bag.roast_id).await {
+        if let Ok(roast) = state.roast_repo.get(bag.roast_id).await
-            if let Ok(roaster) = state.roaster_repo.get(roast.roaster_id).await {
+            && let Ok(roaster) = state.roaster_repo.get(roast.roaster_id).await
-                let event = NewTimelineEvent {
+        {
-                    entity_type: "bag".to_string(),
+            let event = NewTimelineEvent {
-                    entity_id: bag.id.into_inner(),
+                entity_type: "bag".to_string(),
-                    action: "finished".to_string(),
+                entity_id: bag.id.into_inner(),
-                    occurred_at: chrono::Utc::now(),
+                action: "finished".to_string(),
-                    title: format!("{}", roast.name),
+                occurred_at: chrono::Utc::now(),
-                    details: vec![
+                title: roast.name.to_string(),
-                        TimelineEventDetail {
+                details: vec![
-                            label: "Roaster".to_string(),
+                    TimelineEventDetail {
-                            value: roaster.name,
+                        label: "Roaster".to_string(),
-                        },
+                        value: roaster.name,
-                        TimelineEventDetail {
+                    },
-                            label: "Amount".to_string(),
+                    TimelineEventDetail {
-                            value: format!("{}g", bag.amount),
+                        label: "Amount".to_string(),
-                        },
+                        value: format!("{}g", bag.amount),
-                    ],
+                    },
-                    tasting_notes: vec![],
+                ],
-                };
+                tasting_notes: vec![],
-                let _ = state.timeline_repo.insert(event).await;
+            };
-            }
+            let _ = state.timeline_repo.insert(event).await;
         }
     }
 

src/infrastructure/repositories/bags.rs

@@ -1,7 +1,8 @@
 use async_trait::async_trait;
 use chrono::{DateTime, NaiveDate, Utc};
-use sqlx::query_as;
+use sqlx::{QueryBuilder, query_as};
 
+use super::macros::push_update_field;
 use crate::domain::RepositoryError;
 use crate::domain::bags::{Bag, BagSortKey, BagWithRoast, NewBag, UpdateBag};
 use crate::domain::ids::{BagId, RoastId};
@@ -29,6 +30,23 @@ impl SqlBagRepository {
         Self { pool }
     }
 
+    fn order_clause(request: &ListRequest<BagSortKey>) -> String {
+        let sort_column = match request.sort_key {
+            BagSortKey::RoastDate => "b.roast_date",
+            BagSortKey::CreatedAt => "b.created_at",
+            BagSortKey::Roaster => "rr.name",
+            BagSortKey::Roast => "r.name",
+            BagSortKey::FinishedAt => "b.finished_at",
+        };
+
+        let direction = match request.sort_direction {
+            SortDirection::Asc => "ASC",
+            SortDirection::Desc => "DESC",
+        };
+
+        format!("{} {}", sort_column, direction)
+    }
+
     fn to_domain(record: BagRecord) -> Bag {
         Bag {
             id: BagId::new(record.id),
@@ -106,21 +124,7 @@ impl BagRepository for SqlBagRepository {
         &self,
         request: &ListRequest<BagSortKey>,
     ) -> Result<Page<BagWithRoast>, RepositoryError> {
-        let sort_column = match request.sort_key {
+        let order_clause = Self::order_clause(request);
-            BagSortKey::RoastDate => "b.roast_date",
-            BagSortKey::CreatedAt => "b.created_at",
-            BagSortKey::Roaster => "rr.name",
-            BagSortKey::Roast => "r.name",
-            BagSortKey::FinishedAt => "b.finished_at",
-        };
-
-        let direction = match request.sort_direction {
-            SortDirection::Asc => "ASC",
-            SortDirection::Desc => "DESC",
-        };
-
-        let order_clause = format!("{} {}", sort_column, direction);
-
         let count_query = "SELECT COUNT(*) FROM bags";
 
         crate::infrastructure::repositories::pagination::paginate(
@@ -153,47 +157,20 @@ impl BagRepository for SqlBagRepository {
     }
 
     async fn update(&self, id: BagId, changes: UpdateBag) -> Result<Bag, RepositoryError> {
-        let mut query = "UPDATE bags SET updated_at = CURRENT_TIMESTAMP".to_string();
+        let mut builder = QueryBuilder::new("UPDATE bags SET updated_at = CURRENT_TIMESTAMP");
-        let mut has_changes = false;
+        let mut sep = true; // Already have updated_at
 
-        if changes.remaining.is_some() {
+        push_update_field!(builder, sep, "remaining", changes.remaining);
-            query.push_str(", remaining = ?");
+        push_update_field!(builder, sep, "closed", changes.closed);
-            has_changes = true;
+        push_update_field!(builder, sep, "finished_at", changes.finished_at);
-        }
+        let _ = sep; // Suppress unused_assignments warning from macro
 
-        if changes.closed.is_some() {
+        builder.push(" WHERE id = ");
-            query.push_str(", closed = ?");
+        builder.push_bind(id.into_inner());
-            has_changes = true;
+        builder.push(" RETURNING id, roast_id, roast_date, amount, remaining, closed, finished_at, created_at, updated_at");
-        }
 
-        if changes.finished_at.is_some() {
+        let record = builder
-            query.push_str(", finished_at = ?");
+            .build_query_as::<BagRecord>()
-            has_changes = true;
-        }
-
-        if !has_changes {
-            return self.get(id).await;
-        }
-
-        query.push_str(" WHERE id = ? RETURNING id, roast_id, roast_date, amount, remaining, closed, finished_at, created_at, updated_at");
-
-        let mut q = query_as::<_, BagRecord>(&query);
-
-        if let Some(remaining) = changes.remaining {
-            q = q.bind(remaining);
-        }
-
-        if let Some(closed) = changes.closed {
-            q = q.bind(closed);
-        }
-
-        if let Some(finished_at) = changes.finished_at {
-            q = q.bind(finished_at);
-        }
-
-        q = q.bind(id.into_inner());
-
-        let record = q
             .fetch_optional(&self.pool)
             .await
             .map_err(|err| RepositoryError::unexpected(err.to_string()))?
@@ -239,23 +216,8 @@ impl BagRepository for SqlBagRepository {
         &self,
         request: &ListRequest<BagSortKey>,
     ) -> Result<Page<BagWithRoast>, RepositoryError> {
-        let sort_column = match request.sort_key {
+        let order_clause = Self::order_clause(request);
-            BagSortKey::RoastDate => "b.roast_date",
-            BagSortKey::CreatedAt => "b.created_at",
-            BagSortKey::Roaster => "rr.name",
-            BagSortKey::Roast => "r.name",
-            BagSortKey::FinishedAt => "b.finished_at",
-        };
-
-        let direction = match request.sort_direction {
-            SortDirection::Asc => "ASC",
-            SortDirection::Desc => "DESC",
-        };
-
-        let order_clause = format!("{} {}", sort_column, direction);
-
         let base_query = format!("{} WHERE b.closed = TRUE", BASE_SELECT);
-
         let count_query = "SELECT COUNT(*) FROM bags WHERE closed = TRUE";
 
         crate::infrastructure::repositories::pagination::paginate(

src/infrastructure/repositories/macros.rs

@@ -0,0 +1,26 @@
+/// Helper macro for building dynamic UPDATE queries with QueryBuilder.
+///
+/// Handles the common pattern of conditionally adding SET clauses with proper
+/// comma separation.
+///
+/// # Example
+/// ```ignore
+/// let mut builder = QueryBuilder::new("UPDATE users SET ");
+/// let mut sep = false;
+/// push_update_field!(builder, sep, "name", changes.name);
+/// push_update_field!(builder, sep, "email", changes.email);
+/// ```
+macro_rules! push_update_field {
+    ($builder:expr, $separator:expr, $field:literal, $value:expr) => {
+        if let Some(value) = $value {
+            if $separator {
+                $builder.push(", ");
+            }
+            $separator = true;
+            $builder.push(concat!($field, " = "));
+            $builder.push_bind(value);
+        }
+    };
+}
+
+pub(super) use push_update_field;

src/infrastructure/repositories/mod.rs

@@ -1,4 +1,5 @@
 pub mod bags;
+mod macros;
 pub mod pagination;
 pub mod roasters;
 pub mod roasts;

src/infrastructure/repositories/roasters.rs

@@ -2,6 +2,7 @@ use async_trait::async_trait;
 use chrono::{DateTime, Utc};
 use sqlx::{QueryBuilder, query, query_as};
 
+use super::macros::push_update_field;
 use crate::domain::RepositoryError;
 use crate::domain::ids::RoasterId;
 use crate::domain::listing::{ListRequest, Page, SortDirection};
@@ -20,7 +21,7 @@ impl SqlRoasterRepository {
         Self { pool }
     }
 
-    fn sort_clause(request: &ListRequest<RoasterSortKey>) -> String {
+    fn order_clause(request: &ListRequest<RoasterSortKey>) -> String {
         let dir_sql = match request.sort_direction() {
             SortDirection::Asc => "ASC",
             SortDirection::Desc => "DESC",
@@ -184,7 +185,7 @@ impl RoasterRepository for SqlRoasterRepository {
         &self,
         request: &ListRequest<RoasterSortKey>,
     ) -> Result<Page<Roaster>, RepositoryError> {
-        let order_clause = Self::sort_clause(request);
+        let order_clause = Self::order_clause(request);
         let base_query =
             "SELECT id, name, slug, country, city, homepage, notes, created_at FROM roasters";
         let count_query = "SELECT COUNT(*) FROM roasters";
@@ -206,50 +207,15 @@ impl RoasterRepository for SqlRoasterRepository {
         changes: UpdateRoaster,
     ) -> Result<Roaster, RepositoryError> {
         let mut builder = QueryBuilder::new("UPDATE roasters SET ");
-        let mut wrote_field = false;
+        let mut sep = false;
 
-        if let Some(name) = changes.name {
+        push_update_field!(builder, sep, "name", changes.name);
-            if wrote_field {
+        push_update_field!(builder, sep, "country", changes.country);
-                builder.push(", ");
+        push_update_field!(builder, sep, "city", changes.city);
-            }
+        push_update_field!(builder, sep, "homepage", changes.homepage);
-            wrote_field = true;
+        push_update_field!(builder, sep, "notes", changes.notes);
-            builder.push("name = ");
-            builder.push_bind(name);
-        }
-        if let Some(country) = changes.country {
-            if wrote_field {
-                builder.push(", ");
-            }
-            wrote_field = true;
-            builder.push("country = ");
-            builder.push_bind(country);
-        }
-        if let Some(city) = changes.city {
-            if wrote_field {
-                builder.push(", ");
-            }
-            wrote_field = true;
-            builder.push("city = ");
-            builder.push_bind(city);
-        }
-        if let Some(homepage) = changes.homepage {
-            if wrote_field {
-                builder.push(", ");
-            }
-            wrote_field = true;
-            builder.push("homepage = ");
-            builder.push_bind(homepage);
-        }
-        if let Some(notes) = changes.notes {
-            if wrote_field {
-                builder.push(", ");
-            }
-            wrote_field = true;
-            builder.push("notes = ");
-            builder.push_bind(notes);
-        }
 
-        if !wrote_field {
+        if !sep {
             return Err(RepositoryError::unexpected(
                 "No fields provided for update".to_string(),
             ));

src/infrastructure/repositories/roasts.rs

@@ -3,6 +3,7 @@ use chrono::{DateTime, Utc};
 use serde_json::{from_str, to_string};
 use sqlx::{Error as SqlxError, QueryBuilder, query, query_as, query_scalar};
 
+use super::macros::push_update_field;
 use crate::domain::RepositoryError;
 use crate::domain::ids::{RoastId, RoasterId};
 use crate::domain::listing::{ListRequest, Page, SortDirection};
@@ -261,96 +262,52 @@ impl RoastRepository for SqlRoastRepository {
             .await
             .map_err(|err| RepositoryError::unexpected(err.to_string()))?;
 
-        let UpdateRoast {
-            roaster_id,
-            name,
-            origin,
-            region,
-            producer,
-            tasting_notes,
-            process,
-        } = changes;
-
         let mut builder = QueryBuilder::new("UPDATE roasts SET ");
-        let mut wrote_field = false;
+        let mut sep = false;
 
-        if let Some(roaster_id) = roaster_id {
+        // Handle roaster_id specially due to type conversion
-            if wrote_field {
+        if let Some(roaster_id) = changes.roaster_id {
-                builder.push(", ");
+            sep = true;
-            }
-            wrote_field = true;
             builder.push("roaster_id = ");
             builder.push_bind(i64::from(roaster_id));
         }
-        if let Some(name) = name {
+
-            if wrote_field {
+        push_update_field!(builder, sep, "name", changes.name);
-                builder.push(", ");
+        push_update_field!(builder, sep, "origin", changes.origin);
-            }
+        push_update_field!(builder, sep, "region", changes.region);
-            wrote_field = true;
+        push_update_field!(builder, sep, "producer", changes.producer);
-            builder.push("name = ");
+        push_update_field!(builder, sep, "process", changes.process);
-            builder.push_bind(name);
+
-        }
+        // Handle tasting_notes specially due to JSON encoding
-        if let Some(origin) = origin {
+        if let Some(tasting_notes) = changes.tasting_notes {
-            if wrote_field {
-                builder.push(", ");
-            }
-            wrote_field = true;
-            builder.push("origin = ");
-            builder.push_bind(origin);
-        }
-        if let Some(region) = region {
-            if wrote_field {
-                builder.push(", ");
-            }
-            wrote_field = true;
-            builder.push("region = ");
-            builder.push_bind(region);
-        }
-        if let Some(producer) = producer {
-            if wrote_field {
-                builder.push(", ");
-            }
-            wrote_field = true;
-            builder.push("producer = ");
-            builder.push_bind(producer);
-        }
-        if let Some(process) = process {
-            if wrote_field {
-                builder.push(", ");
-            }
-            wrote_field = true;
-            builder.push("process = ");
-            builder.push_bind(process);
-        }
-        if let Some(tasting_notes) = tasting_notes {
             let notes_json = Self::encode_notes(&tasting_notes)?;
-            if wrote_field {
+            if sep {
                 builder.push(", ");
             }
-            wrote_field = true;
+            sep = true;
             builder.push("tasting_notes = ");
             builder.push_bind(notes_json);
         }
 
-        if wrote_field {
+        if !sep {
-            builder.push(" WHERE id = ");
-            builder.push_bind(i64::from(id));
-
-            let result = builder
-                .build()
-                .execute(&mut *tx)
-                .await
-                .map_err(|err| RepositoryError::unexpected(err.to_string()))?;
-
-            if result.rows_affected() == 0 {
-                return Err(RepositoryError::NotFound);
-            }
-        } else {
             return Err(RepositoryError::unexpected(
                 "No fields provided for update".to_string(),
             ));
         }
 
+        builder.push(" WHERE id = ");
+        builder.push_bind(i64::from(id));
+
+        let result = builder
+            .build()
+            .execute(&mut *tx)
+            .await
+            .map_err(|err| RepositoryError::unexpected(err.to_string()))?;
+
+        if result.rows_affected() == 0 {
+            return Err(RepositoryError::NotFound);
+        }
+
         tx.commit()
             .await
             .map_err(|err| RepositoryError::unexpected(err.to_string()))?;

src/presentation/cli/mod.rs

@@ -1,5 +1,5 @@
-mod macros;
 pub mod bags;
+mod macros;
 pub mod roasters;
 pub mod roasts;
 pub mod tokens;

src/presentation/cli/roasters.rs

@@ -73,4 +73,10 @@ pub async fn update_roaster(client: &BrewlogClient, command: UpdateRoasterComman
     print_json(&roaster)
 }
 
-define_delete_command!(DeleteRoasterCommand, delete_roaster, RoasterId, roasters, "roaster");
+define_delete_command!(
+    DeleteRoasterCommand,
+    delete_roaster,
+    RoasterId,
+    roasters,
+    "roaster"
+);