Merge pull request 'transactions + unique-conflict' (#5) from backend-rust into main

Reviewed-on: nvrl/Holy-Flavor-Rater#5

.gitea/workflows/ci.yml

@@ -68,6 +68,17 @@ jobs:
             backend-rs/target
           key: cargo-${{ hashFiles('backend-rs/Cargo.lock') }}
 
+      - name: Fetch common-passwords list
+        run: |
+          mkdir -p assets
+          curl -fsSL -o assets/common-passwords.txt.gz \
+            "https://raw.githubusercontent.com/django/django/${DJANGO_PASSWORDS_TAG}/django/contrib/auth/common-passwords.txt.gz"
+          echo "${DJANGO_PASSWORDS_SHA256}  assets/common-passwords.txt.gz" | sha256sum -c -
+        working-directory: backend-rs
+        env:
+          DJANGO_PASSWORDS_TAG: "5.2"
+          DJANGO_PASSWORDS_SHA256: "3c1baed62596de36860824eb3f436d5932d37ca8b06e59df78f5a44ec175afe4"
+
       - name: Format Check
         run: cargo fmt --check
         working-directory: backend-rs

backend-rs/src/db.rs

@@ -36,5 +36,48 @@ pub async fn connect(database_url: &str) -> anyhow::Result<DatabaseConnection> {
         }
     }
 
+    ensure_schema(&conn).await?;
+
     Ok(conn)
 }
+
+/// Idempotent, additive schema patches the Rust backend owns now that Django is
+/// retired. SQLite has no `ADD COLUMN IF NOT EXISTS`, so we probe `PRAGMA
+/// table_info` first and only ALTER when the column is missing.
+async fn ensure_schema(conn: &DatabaseConnection) -> anyhow::Result<()> {
+    add_column_if_missing(
+        conn,
+        "api_flavor",
+        "aliases",
+        "ALTER TABLE api_flavor ADD COLUMN aliases TEXT",
+    )
+    .await?;
+    Ok(())
+}
+
+async fn add_column_if_missing(
+    conn: &DatabaseConnection,
+    table: &str,
+    column: &str,
+    alter_sql: &str,
+) -> anyhow::Result<()> {
+    let rows = conn
+        .query_all(Statement::from_string(
+            DatabaseBackend::Sqlite,
+            format!("PRAGMA table_info({table})"),
+        ))
+        .await?;
+    let exists = rows
+        .iter()
+        .any(|r| r.try_get::<String>("", "name").as_deref() == Ok(column));
+    if exists {
+        return Ok(());
+    }
+    conn.execute(Statement::from_string(
+        DatabaseBackend::Sqlite,
+        alter_sql.to_owned(),
+    ))
+    .await?;
+    tracing::info!("schema: added {table}.{column}");
+    Ok(())
+}

backend-rs/src/entities/flavor.rs

@@ -22,6 +22,13 @@ pub struct Model {
     pub is_available: bool,
     pub is_legacy: bool,
     pub external_id: Option<i64>,
+    /// Alternate names this flavor is known by — populated when another flavor
+    /// (typically a legacy stand-in) is merged into this one. JSON list of
+    /// strings, stored as TEXT. Lets `seed_legacy` avoid re-creating a merged-away
+    /// legacy flavor and lets search match its old name. Added by the guarded
+    /// startup migration in `db.rs`; may be NULL on rows predating it.
+    #[sea_orm(column_type = "Json", nullable)]
+    pub aliases: Option<Json>,
     /// Django stores naive UTC text ("YYYY-MM-DD HH:MM:SS.ffffff"); we treat it
     /// as UTC when formatting to DRF's ISO-8601 `...Z` output.
     pub created_at: ChronoDateTime,

backend-rs/src/error.rs

@@ -89,3 +89,10 @@ impl IntoResponse for ApiError {
 }
 
 pub type ApiResult<T> = Result<T, ApiError>;
+
+/// True if a `DbErr` is a SQLite UNIQUE-constraint violation. SeaORM surfaces the
+/// underlying sqlx message verbatim; matching on it lets handlers turn a racing
+/// duplicate insert into a clean 400 instead of a generic 500.
+pub fn is_unique_violation(e: &sea_orm::DbErr) -> bool {
+    e.to_string().contains("UNIQUE constraint failed")
+}

backend-rs/src/jobs/seed_legacy.rs

@@ -131,6 +131,18 @@ impl BackgroundJob for SeedLegacy {
                     }
                 }
 
+                // Don't recreate a legacy flavor that has since been merged into a
+                // (Shopify) flavor — the survivor carries this name as an alias.
+                if existing.is_none() {
+                    if let Some(owner) = alias_owner(db, name, cat.id).await? {
+                        log.push(format!(
+                            "Skipping '{name}' (superseded by '{}')",
+                            owner.name
+                        ));
+                        continue;
+                    }
+                }
+
                 // Resolve image first (so we can set local paths on insert).
                 let mut local_paths: Option<Vec<String>> = None;
                 if let Some(url) = image_url.filter(|s| !s.is_empty()) {
@@ -208,6 +220,37 @@ fn pystr(v: Option<&Value>) -> String {
     }
 }
 
+/// A flavor in `category_id` that lists `name` (case-insensitive) among its
+/// merge aliases, if any. Used to suppress recreating a merged-away legacy row.
+async fn alias_owner(
+    db: &DatabaseConnection,
+    name: &str,
+    category_id: i32,
+) -> anyhow::Result<Option<flavor::Model>> {
+    let nl = name.to_lowercase();
+    let candidates = Flavor::find()
+        .filter(flavor::Column::CategoryId.eq(category_id))
+        .filter(flavor::Column::Aliases.is_not_null())
+        .all(db)
+        .await?;
+    Ok(candidates.into_iter().find(|f| {
+        json_str_list(&f.aliases)
+            .iter()
+            .any(|a| a.to_lowercase() == nl)
+    }))
+}
+
+/// Django JSONField (list) stored as TEXT → Vec<String>.
+fn json_str_list(v: &Option<Value>) -> Vec<String> {
+    match v {
+        Some(Value::Array(arr)) => arr
+            .iter()
+            .filter_map(|x| x.as_str().map(String::from))
+            .collect(),
+        _ => Vec::new(),
+    }
+}
+
 async fn goc_category(
     db: &DatabaseConnection,
     name: &str,

backend-rs/src/jobs/sync_flavors.rs

@@ -491,6 +491,10 @@ async fn upsert_flavor(
         .filter(flavor::Column::ExternalId.eq(external_id))
         .one(db)
         .await?;
+    // A row already linked by external_id is the authoritative one for this
+    // product; its name may have been curated (admin rename) or chosen by a merge
+    // that kept the legacy row. Don't let a later sync overwrite that name.
+    let matched_by_external = existing.is_some();
     if existing.is_none() {
         existing = Flavor::find()
             .filter(flavor::Column::Name.eq(name))
@@ -527,7 +531,7 @@ async fn upsert_flavor(
                 if f_is_legacy {
                     am.is_legacy = Set(false);
                 }
-                if f_name != name {
+                if f_name != name && !matched_by_external {
                     let dup = Flavor::find()
                         .filter(flavor::Column::Name.eq(name))
                         .filter(flavor::Column::CategoryId.eq(category_id))

backend-rs/src/mentions.rs

@@ -5,7 +5,7 @@
 use sea_orm::sea_query::{Expr, Func};
 use sea_orm::{
     ActiveValue::{NotSet, Set},
-    ColumnTrait, Condition, DatabaseConnection, EntityTrait, QueryFilter,
+    ColumnTrait, Condition, ConnectionTrait, EntityTrait, QueryFilter,
 };
 
 use crate::entities::prelude::*;
@@ -47,8 +47,8 @@ fn eq_or_null(col: notification::Column, value: Option<i32>) -> Condition {
     }
 }
 
-pub async fn parse_mentions(
+pub async fn parse_mentions<C: ConnectionTrait>(
-    db: &DatabaseConnection,
+    db: &C,
     text: &str,
     actor_id: i32,
     rating_id: Option<i32>,

backend-rs/src/merge.rs

@@ -87,14 +87,46 @@ pub async fn merge_flavors(
         None
     };
 
+    // Record the removed flavor's name (and any aliases it already carried) onto
+    // the survivor, so `seed_legacy` won't recreate the merged-away row and search
+    // still resolves the old name. Deduped case-insensitively; never alias the
+    // kept flavor's own name.
+    let original = json_str_list(&keep.aliases);
+    let mut aliases = original.clone();
+    let mut seen: std::collections::HashSet<String> =
+        aliases.iter().map(|a| a.to_lowercase()).collect();
+    seen.insert(keep.name.to_lowercase());
+    for cand in std::iter::once(remove.name.clone()).chain(json_str_list(&remove.aliases)) {
+        if seen.insert(cand.to_lowercase()) {
+            aliases.push(cand);
+        }
+    }
+    let aliases_changed = aliases.len() != original.len();
+
     Flavor::delete_by_id(remove.id).exec(&txn).await?;
 
-    if let Some(eid) = inherited {
+    if inherited.is_some() || aliases_changed {
         let mut am: flavor::ActiveModel = keep.clone().into();
+        if let Some(eid) = inherited {
             am.external_id = Set(Some(eid));
+        }
+        if aliases_changed {
+            am.aliases = Set(Some(serde_json::json!(aliases)));
+        }
         am.update(&txn).await?;
     }
 
     txn.commit().await?;
     Ok(())
 }
+
+/// Django JSONField (list) stored as TEXT → Vec<String>, tolerating null/non-array.
+fn json_str_list(v: &Option<serde_json::Value>) -> Vec<String> {
+    match v {
+        Some(serde_json::Value::Array(arr)) => arr
+            .iter()
+            .filter_map(|x| x.as_str().map(String::from))
+            .collect(),
+        _ => Vec::new(),
+    }
+}

backend-rs/src/routes/flavors.rs

@@ -80,7 +80,8 @@ async fn list(
             let like = format!("%{w}%");
             cond = cond
                 .add(flavor::Column::Name.like(&like))
-                .add(flavor::Column::Description.like(&like));
+                .add(flavor::Column::Description.like(&like))
+                .add(flavor::Column::Aliases.like(&like));
             has = true;
         }
         if has {
@@ -99,7 +100,7 @@ async fn list(
         .all(&state.db)
         .await?;
 
-    let results = build_flavors(&state, &ctx, models, viewer, &[]).await?;
+    let results = build_flavors(&state, &ctx, models, viewer, &[], false).await?;
     Ok(Json(Paginated::build(&ctx, results, count, page, size)))
 }
 
@@ -114,7 +115,7 @@ async fn retrieve(
         .one(&state.db)
         .await?
         .ok_or(ApiError::NotFound)?;
-    let mut dtos = build_flavors(&state, &ctx, vec![model], viewer, &[]).await?;
+    let mut dtos = build_flavors(&state, &ctx, vec![model], viewer, &[], true).await?;
     dtos.pop().map(Json).ok_or(ApiError::NotFound)
 }
 
@@ -143,7 +144,7 @@ async fn top(
         .all(&state.db)
         .await?;
     Ok(Json(
-        build_flavors(&state, &ctx, models, viewer, &[]).await?,
+        build_flavors(&state, &ctx, models, viewer, &[], false).await?,
     ))
 }
 
@@ -159,7 +160,7 @@ async fn newest(
         .all(&state.db)
         .await?;
     Ok(Json(
-        build_flavors(&state, &ctx, models, viewer, &[]).await?,
+        build_flavors(&state, &ctx, models, viewer, &[], false).await?,
     ))
 }
 
@@ -194,7 +195,7 @@ async fn followed_top(
         .all(&state.db)
         .await?;
     Ok(Json(
-        build_flavors(&state, &ctx, models, Some(uid), &[]).await?,
+        build_flavors(&state, &ctx, models, Some(uid), &[], false).await?,
     ))
 }
 
@@ -229,7 +230,8 @@ async fn search(
             let like = format!("%{w}%");
             cond = cond
                 .add(flavor::Column::Name.like(&like))
-                .add(flavor::Column::Description.like(&like));
+                .add(flavor::Column::Description.like(&like))
+                .add(flavor::Column::Aliases.like(&like));
         }
         q = q.filter(cond);
     }

backend-rs/src/routes/ratings.rs

@@ -196,7 +196,13 @@ async fn create(
     }
 
     let now = crate::datetime::now_micros();
-    let model = rating::ActiveModel {
+    // Insert + mention fan-out in one transaction so a parse_mentions failure
+    // can't leave a rating with no notifications. The DB's UNIQUE(user_id,
+    // flavor_id) index is the real guard against a racing double-rate (the
+    // pre-check above is best-effort and not atomic): map that violation to the
+    // same clean validation error instead of a 500.
+    let txn = state.db.begin().await?;
+    let model = match (rating::ActiveModel {
         id: NotSet,
         user_id: Set(uid),
         flavor_id: Set(flavor_id),
@@ -204,12 +210,22 @@ async fn create(
         comment: Set(comment.clone()),
         created_at: Set(now),
     }
-    .insert(&state.db)
+    .insert(&txn)
-    .await?;
+    .await)
+    {
+        Ok(m) => m,
+        Err(e) if crate::error::is_unique_violation(&e) => {
+            return Err(ApiError::Validation(json!([
+                "You have already rated this flavor."
+            ])));
+        }
+        Err(e) => return Err(e.into()),
+    };
 
     if let Some(text) = comment.as_deref().filter(|s| !s.is_empty()) {
-        parse_mentions(&state.db, text, uid, Some(model.id), None).await?;
+        parse_mentions(&txn, text, uid, Some(model.id), None).await?;
     }
+    txn.commit().await?;
 
     let mut dtos = build_ratings(&state, &ctx, vec![model]).await?;
     let out = dtos.pop().ok_or(ApiError::Internal)?;
@@ -430,6 +446,9 @@ async fn reply(
     }
 
     let now = crate::datetime::now_micros();
+    // Reply + owner-notification + mention fan-out are one unit: a failure in any
+    // step must not persist a partial reply (orphaned reply, missing notification).
+    let txn = state.db.begin().await?;
     let reply_model = reply::ActiveModel {
         id: NotSet,
         user_id: Set(uid),
@@ -437,7 +456,7 @@ async fn reply(
         text: Set(text.to_string()),
         created_at: Set(now),
     }
-    .insert(&state.db)
+    .insert(&txn)
     .await?;
 
     if rating.user_id != uid {
@@ -453,11 +472,12 @@ async fn reply(
             notification_type: Set("reply".to_string()),
             profile_comment_id: Set(None),
         }
-        .insert(&state.db)
+        .insert(&txn)
         .await?;
     }
 
-    parse_mentions(&state.db, text, uid, Some(rating.id), Some(reply_model.id)).await?;
+    parse_mentions(&txn, text, uid, Some(rating.id), Some(reply_model.id)).await?;
+    txn.commit().await?;
 
     let mut dtos = build_replies(&state, vec![reply_model]).await?;
     let out = dtos.pop().ok_or(ApiError::Internal)?;

backend-rs/src/routes/users.rs

@@ -1363,7 +1363,8 @@ async fn dashboard(
     });
 
     let missing_count = all_flavors.len() as i64;
-    let missing_flavors = build_flavors(&state, &ctx, all_flavors, Some(uid), &followed).await?;
+    let missing_flavors =
+        build_flavors(&state, &ctx, all_flavors, Some(uid), &followed, false).await?;
     let user = build_user(&state, &ctx, me, Some(uid)).await?;
 
     Ok(Json(DashboardOut {

backend-rs/src/service.rs

@@ -189,12 +189,51 @@ pub async fn build_ratings(
     Ok(out)
 }
 
+/// Per-flavor rating rollup accumulated in a single pass over the loaded ratings,
+/// replacing the previous O(flavors × ratings) re-scan.
+struct RatingAgg {
+    sum: i64,
+    n: i64,
+    followed_sum: i64,
+    followed_n: i64,
+    dist: BTreeMap<String, i64>,
+    /// The viewer's own score for this flavor, if they rated it.
+    viewer_score: Option<i32>,
+}
+
+impl Default for RatingAgg {
+    fn default() -> Self {
+        RatingAgg {
+            sum: 0,
+            n: 0,
+            followed_sum: 0,
+            followed_n: 0,
+            dist: empty_distribution(),
+            viewer_score: None,
+        }
+    }
+}
+
+impl RatingAgg {
+    fn average(&self) -> Option<f64> {
+        (self.n > 0).then(|| self.sum as f64 / self.n as f64)
+    }
+    fn followed_average(&self) -> Option<f64> {
+        (self.followed_n > 0).then(|| self.followed_sum as f64 / self.followed_n as f64)
+    }
+}
+
+/// Build `FlavorOut`s. `include_replies` controls whether each nested rating
+/// carries its replies: the flavor *detail* view needs them, but card/list
+/// callers (list/top/newest/followed_top) don't — skipping the reply query +
+/// reply-author lookups avoids shipping data the list UI never reads.
 pub async fn build_flavors(
     state: &AppState,
     ctx: &RequestCtx,
     flavors: Vec<flavor::Model>,
     viewer_id: Option<i32>,
     followed_ids: &[i32],
+    include_replies: bool,
 ) -> ApiResult<Vec<FlavorOut>> {
     let media = &state.config.media_url;
     if flavors.is_empty() {
@@ -218,10 +257,11 @@ pub async fn build_flavors(
         .order_by_desc(rating::Column::CreatedAt)
         .all(&state.db)
         .await?;
-    let rating_ids: Vec<i32> = ratings.iter().map(|r| r.id).collect();
 
-    // Replies (order: created_at asc)
+    // Replies (order: created_at asc) — only loaded for the detail view.
-    let replies = if rating_ids.is_empty() {
+    let replies = if include_replies {
+        let rating_ids: Vec<i32> = ratings.iter().map(|r| r.id).collect();
+        if rating_ids.is_empty() {
             Vec::new()
         } else {
             Reply::find()
@@ -229,6 +269,9 @@ pub async fn build_flavors(
                 .order_by_asc(reply::Column::CreatedAt)
                 .all(&state.db)
                 .await?
+        }
+    } else {
+        Vec::new()
     };
 
     // Users referenced by ratings + replies
@@ -267,9 +310,13 @@ pub async fn build_flavors(
 
     // Flavor lookup for per-rating flavor fields
     let flavor_map: HashMap<i32, &flavor::Model> = flavors.iter().map(|f| (f.id, f)).collect();
+    let followed_set: std::collections::HashSet<i32> = followed_ids.iter().copied().collect();
 
-    // Ratings grouped by flavor id
+    // Single pass over ratings: build the nested RatingOut grouped by flavor AND
+    // accumulate average / followed-average / distribution / the viewer's own
+    // score per flavor.
     let mut ratings_by_flavor: HashMap<i32, Vec<RatingOut>> = HashMap::new();
+    let mut agg_by_flavor: HashMap<i32, RatingAgg> = HashMap::new();
     for r in &ratings {
         let user = user_map.get(&r.user_id);
         let username = user.map(|u| u.username.clone()).unwrap_or_default();
@@ -311,47 +358,20 @@ pub async fn build_flavors(
                 created_at: crate::datetime::drf_iso(&r.created_at),
                 replies: replies_by_rating.remove(&r.id).unwrap_or_default(),
             });
-    }
 
-    // Average + distribution per flavor, from loaded ratings
+        let a = agg_by_flavor.entry(r.flavor_id).or_default();
-    let followed_set: std::collections::HashSet<i32> = followed_ids.iter().copied().collect();
-    let mut avg_by_flavor: HashMap<i32, Option<f64>> = HashMap::new();
-    let mut followed_avg_by_flavor: HashMap<i32, Option<f64>> = HashMap::new();
-    let mut dist_by_flavor: HashMap<i32, BTreeMap<String, i64>> = HashMap::new();
-    for fid in &flavor_ids {
-        let mut dist = empty_distribution();
-        let mut sum = 0i64;
-        let mut n = 0i64;
-        let mut fsum = 0i64;
-        let mut fn_ = 0i64;
-        for r in ratings.iter().filter(|r| r.flavor_id == *fid) {
         if (1..=10).contains(&r.score) {
-                *dist.get_mut(&r.score.to_string()).unwrap() += 1;
+            *a.dist.get_mut(&r.score.to_string()).unwrap() += 1;
         }
-            sum += r.score as i64;
+        a.sum += r.score as i64;
-            n += 1;
+        a.n += 1;
         if followed_set.contains(&r.user_id) {
-                fsum += r.score as i64;
+            a.followed_sum += r.score as i64;
-                fn_ += 1;
+            a.followed_n += 1;
         }
+        if Some(r.user_id) == viewer_id {
+            a.viewer_score = Some(r.score);
         }
-        avg_by_flavor.insert(
-            *fid,
-            if n == 0 {
-                None
-            } else {
-                Some(sum as f64 / n as f64)
-            },
-        );
-        followed_avg_by_flavor.insert(
-            *fid,
-            if fn_ == 0 {
-                None
-            } else {
-                Some(fsum as f64 / fn_ as f64)
-            },
-        );
-        dist_by_flavor.insert(*fid, dist);
     }
 
     let mut out = Vec::with_capacity(flavors.len());
@@ -359,12 +379,8 @@ pub async fn build_flavors(
         let (cat_name, cat_slug) = cat_map.get(&f.category_id).cloned().unwrap_or_default();
         let local_paths = json_string_list(&f.local_image_paths);
         let image_urls = json_string_list(&f.image_urls);
-        let user_rating = viewer_id.and_then(|vid| {
+        let agg = agg_by_flavor.remove(&f.id);
-            ratings
+        let user_rating = viewer_id.and(agg.as_ref().and_then(|a| a.viewer_score));
-                .iter()
-                .find(|r| r.flavor_id == f.id && r.user_id == vid)
-                .map(|r| r.score)
-        });
         out.push(FlavorOut {
             id: f.id,
             name: f.name.clone(),
@@ -372,8 +388,8 @@ pub async fn build_flavors(
             category_name: cat_name,
             category_slug: cat_slug,
             description: f.description.clone(),
-            average_rating: avg_by_flavor.get(&f.id).copied().flatten(),
+            average_rating: agg.as_ref().and_then(RatingAgg::average),
-            followed_average_rating: followed_avg_by_flavor.get(&f.id).copied().flatten(),
+            followed_average_rating: agg.as_ref().and_then(RatingAgg::followed_average),
             user_rating,
             ratings: ratings_by_flavor.remove(&f.id).unwrap_or_default(),
             image_url: flavor_primary_image(
@@ -394,9 +410,7 @@ pub async fn build_flavors(
             is_available: f.is_available,
             is_legacy: f.is_legacy,
             shop_url: f.shop_url.clone(),
-            rating_distribution: dist_by_flavor
+            rating_distribution: agg.map(|a| a.dist).unwrap_or_else(empty_distribution),
-                .remove(&f.id)
-                .unwrap_or_else(empty_distribution),
         });
     }
     Ok(out)

frontend/src/pages/admin/AdminFlavors.tsx

@@ -38,7 +38,7 @@ const AdminFlavors: React.FC = () => {
 
     setIsMerging(true);
     try {
-      await api.post('admin/merge_flavors/', {
+      await api.post('admin-custom/merge_flavors/', {
         keep_id: keepFlavor.id,
         remove_id: removeFlavor.id,
       });