diff --git a/.gitea/workflows/ci.yml b/.gitea/workflows/ci.yml index eca88bf..7a80d51 100644 --- a/.gitea/workflows/ci.yml +++ b/.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 diff --git a/backend-rs/src/db.rs b/backend-rs/src/db.rs index 6ce9808..7ad6033 100644 --- a/backend-rs/src/db.rs +++ b/backend-rs/src/db.rs @@ -36,5 +36,48 @@ pub async fn connect(database_url: &str) -> anyhow::Result { } } + 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::("", "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(()) +} diff --git a/backend-rs/src/entities/flavor.rs b/backend-rs/src/entities/flavor.rs index 640b874..1c42de1 100644 --- a/backend-rs/src/entities/flavor.rs +++ b/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, + /// 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, /// 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, diff --git a/backend-rs/src/error.rs b/backend-rs/src/error.rs index a4fe064..046a39e 100644 --- a/backend-rs/src/error.rs +++ b/backend-rs/src/error.rs @@ -89,3 +89,10 @@ impl IntoResponse for ApiError { } pub type ApiResult = Result; + +/// 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") +} diff --git a/backend-rs/src/jobs/seed_legacy.rs b/backend-rs/src/jobs/seed_legacy.rs index e859350..1e6d094 100644 --- a/backend-rs/src/jobs/seed_legacy.rs +++ b/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> = 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> { + 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. +fn json_str_list(v: &Option) -> Vec { + 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, diff --git a/backend-rs/src/jobs/sync_flavors.rs b/backend-rs/src/jobs/sync_flavors.rs index 7aed358..228a8b6 100644 --- a/backend-rs/src/jobs/sync_flavors.rs +++ b/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)) diff --git a/backend-rs/src/mentions.rs b/backend-rs/src/mentions.rs index 83c019a..ef21c40 100644 --- a/backend-rs/src/mentions.rs +++ b/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) -> Condition { } } -pub async fn parse_mentions( - db: &DatabaseConnection, +pub async fn parse_mentions( + db: &C, text: &str, actor_id: i32, rating_id: Option, diff --git a/backend-rs/src/merge.rs b/backend-rs/src/merge.rs index 3b09ea2..afac84c 100644 --- a/backend-rs/src/merge.rs +++ b/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 = + 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(); - am.external_id = Set(Some(eid)); + 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, tolerating null/non-array. +fn json_str_list(v: &Option) -> Vec { + match v { + Some(serde_json::Value::Array(arr)) => arr + .iter() + .filter_map(|x| x.as_str().map(String::from)) + .collect(), + _ => Vec::new(), + } +} diff --git a/backend-rs/src/routes/flavors.rs b/backend-rs/src/routes/flavors.rs index 5e7d5d1..db86b4f 100644 --- a/backend-rs/src/routes/flavors.rs +++ b/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); } diff --git a/backend-rs/src/routes/ratings.rs b/backend-rs/src/routes/ratings.rs index 3dd9132..a82b99c 100644 --- a/backend-rs/src/routes/ratings.rs +++ b/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) - .await?; + .insert(&txn) + .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)?; diff --git a/backend-rs/src/routes/users.rs b/backend-rs/src/routes/users.rs index 4e2d7f7..b8d0ac0 100644 --- a/backend-rs/src/routes/users.rs +++ b/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 { diff --git a/backend-rs/src/service.rs b/backend-rs/src/service.rs index 9fee116..afc6fe8 100644 --- a/backend-rs/src/service.rs +++ b/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, + /// The viewer's own score for this flavor, if they rated it. + viewer_score: Option, +} + +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 { + (self.n > 0).then(|| self.sum as f64 / self.n as f64) + } + fn followed_average(&self) -> Option { + (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, viewer_id: Option, followed_ids: &[i32], + include_replies: bool, ) -> ApiResult> { let media = &state.config.media_url; if flavors.is_empty() { @@ -218,17 +257,21 @@ pub async fn build_flavors( .order_by_desc(rating::Column::CreatedAt) .all(&state.db) .await?; - let rating_ids: Vec = ratings.iter().map(|r| r.id).collect(); - // Replies (order: created_at asc) - let replies = if rating_ids.is_empty() { - Vec::new() + // Replies (order: created_at asc) — only loaded for the detail view. + let replies = if include_replies { + let rating_ids: Vec = ratings.iter().map(|r| r.id).collect(); + if rating_ids.is_empty() { + Vec::new() + } else { + Reply::find() + .filter(reply::Column::RatingId.is_in(rating_ids)) + .order_by_asc(reply::Column::CreatedAt) + .all(&state.db) + .await? + } } else { - Reply::find() - .filter(reply::Column::RatingId.is_in(rating_ids)) - .order_by_asc(reply::Column::CreatedAt) - .all(&state.db) - .await? + 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 = flavors.iter().map(|f| (f.id, f)).collect(); + let followed_set: std::collections::HashSet = 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> = HashMap::new(); + let mut agg_by_flavor: HashMap = 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 followed_set: std::collections::HashSet = followed_ids.iter().copied().collect(); - let mut avg_by_flavor: HashMap> = HashMap::new(); - let mut followed_avg_by_flavor: HashMap> = HashMap::new(); - let mut dist_by_flavor: HashMap> = 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; - } - sum += r.score as i64; - n += 1; - if followed_set.contains(&r.user_id) { - fsum += r.score as i64; - fn_ += 1; - } + let a = agg_by_flavor.entry(r.flavor_id).or_default(); + if (1..=10).contains(&r.score) { + *a.dist.get_mut(&r.score.to_string()).unwrap() += 1; + } + a.sum += r.score as i64; + a.n += 1; + if followed_set.contains(&r.user_id) { + a.followed_sum += r.score as i64; + 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| { - ratings - .iter() - .find(|r| r.flavor_id == f.id && r.user_id == vid) - .map(|r| r.score) - }); + let agg = agg_by_flavor.remove(&f.id); + let user_rating = viewer_id.and(agg.as_ref().and_then(|a| a.viewer_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(), - followed_average_rating: followed_avg_by_flavor.get(&f.id).copied().flatten(), + average_rating: agg.as_ref().and_then(RatingAgg::average), + 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 - .remove(&f.id) - .unwrap_or_else(empty_distribution), + rating_distribution: agg.map(|a| a.dist).unwrap_or_else(empty_distribution), }); } Ok(out) diff --git a/frontend/src/pages/admin/AdminFlavors.tsx b/frontend/src/pages/admin/AdminFlavors.tsx index 1c647f4..7f258e8 100644 --- a/frontend/src/pages/admin/AdminFlavors.tsx +++ b/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, });