max / makenotwork
9 files changed,
+138 insertions,
-282 deletions
| @@ -33,37 +33,6 @@ pub async fn get_by_id(pool: &PgPool, section_id: ItemSectionId) -> Result<Optio | |||
| 33 | 33 | } | |
| 34 | 34 | ||
| 35 | 35 | /// Insert a new section for an item. | |
| 36 | - | #[tracing::instrument(skip_all)] | |
| 37 | - | /// Whether another section on the same item already uses `slug`. | |
| 38 | - | /// | |
| 39 | - | /// `exclude_id` lets an update skip the row being edited (so renaming a section | |
| 40 | - | /// to a slug it already owns isn't a self-conflict). Backs the slug-dedup loop | |
| 41 | - | /// in the create/update handlers; the `UNIQUE(item_id, slug)` index remains the | |
| 42 | - | /// race-safe source of truth. | |
| 43 | - | #[tracing::instrument(skip_all)] | |
| 44 | - | pub async fn slug_conflicts( | |
| 45 | - | pool: &PgPool, | |
| 46 | - | item_id: ItemId, | |
| 47 | - | slug: &str, | |
| 48 | - | exclude_id: Option<ItemSectionId>, | |
| 49 | - | ) -> Result<bool> { | |
| 50 | - | let exists: bool = sqlx::query_scalar( | |
| 51 | - | r#" | |
| 52 | - | SELECT EXISTS( | |
| 53 | - | SELECT 1 FROM item_sections | |
| 54 | - | WHERE item_id = $1 AND slug = $2 | |
| 55 | - | AND ($3::uuid IS NULL OR id <> $3) | |
| 56 | - | ) | |
| 57 | - | "#, | |
| 58 | - | ) | |
| 59 | - | .bind(item_id) | |
| 60 | - | .bind(slug) | |
| 61 | - | .bind(exclude_id) | |
| 62 | - | .fetch_one(pool) | |
| 63 | - | .await?; | |
| 64 | - | Ok(exists) | |
| 65 | - | } | |
| 66 | - | ||
| 67 | 36 | pub async fn create( | |
| 68 | 37 | pool: &PgPool, | |
| 69 | 38 | item_id: ItemId, |
| @@ -72,17 +72,15 @@ pub async fn create_item( | |||
| 72 | 72 | ai_tier: AiTier, | |
| 73 | 73 | ai_disclosure: Option<&str>, | |
| 74 | 74 | ) -> Result<DbItem> { | |
| 75 | - | let mut slug = crate::helpers::slugify(title); | |
| 76 | - | ||
| 77 | 75 | // Slug uniqueness is enforced by the per-project unique index: insert the | |
| 78 | 76 | // bare slug, and on a 23505 collision append an incrementing suffix and | |
| 79 | 77 | // retry (`base`, `base-2`, `base-3`, ...). The retry IS the dedup — a | |
| 80 | 78 | // pre-insert `SELECT EXISTS` check would just be a redundant round-trip on | |
| 81 | 79 | // the common (no-collision) path and a TOCTOU on the racy one. | |
| 82 | - | let base_slug = slug.clone(); | |
| 83 | - | let mut suffix = 1u32; | |
| 84 | - | let item = loop { | |
| 85 | - | match sqlx::query_as::<_, DbItem>( | |
| 80 | + | let base = crate::helpers::slugify(title).to_string(); | |
| 81 | + | crate::helpers::insert_with_unique_slug(&base, |slug| async move { | |
| 82 | + | let slug = super::validated_types::Slug::from_trusted(slug); | |
| 83 | + | sqlx::query_as::<_, DbItem>( | |
| 86 | 84 | r#" | |
| 87 | 85 | INSERT INTO items (project_id, title, description, price_cents, item_type, slug, ai_tier, ai_disclosure) | |
| 88 | 86 | VALUES ($1, $2, $3, $4, $5, $6, $7, $8) | |
| @@ -99,22 +97,9 @@ pub async fn create_item( | |||
| 99 | 97 | .bind(ai_disclosure) | |
| 100 | 98 | .fetch_one(pool) | |
| 101 | 99 | .await | |
| 102 | - | { | |
| 103 | - | Ok(item) => break item, | |
| 104 | - | Err(sqlx::Error::Database(db_err)) | |
| 105 | - | if db_err.code().as_deref() == Some("23505") && suffix < 100 => | |
| 106 | - | { | |
| 107 | - | suffix += 1; | |
| 108 | - | slug = super::validated_types::Slug::from_trusted( | |
| 109 | - | format!("{}-{}", base_slug, suffix), | |
| 110 | - | ); | |
| 111 | - | continue; | |
| 112 | - | } | |
| 113 | - | Err(e) => return Err(e.into()), | |
| 114 | - | } | |
| 115 | - | }; | |
| 116 | - | ||
| 117 | - | Ok(item) | |
| 100 | + | .map_err(Into::into) | |
| 101 | + | }) | |
| 102 | + | .await | |
| 118 | 103 | } | |
| 119 | 104 | ||
| 120 | 105 | /// Check whether a slug already exists for a given project. |
| @@ -32,35 +32,6 @@ pub async fn get_by_id(pool: &PgPool, section_id: ProjectSectionId) -> Result<Op | |||
| 32 | 32 | Ok(section) | |
| 33 | 33 | } | |
| 34 | 34 | ||
| 35 | - | /// Whether another section on the same project already uses `slug`. | |
| 36 | - | /// | |
| 37 | - | /// `exclude_id` lets an update skip the row being edited. Backs the slug-dedup | |
| 38 | - | /// loop in the create/update handlers; the `UNIQUE(project_id, slug)` index | |
| 39 | - | /// remains the race-safe source of truth. | |
| 40 | - | #[tracing::instrument(skip_all)] | |
| 41 | - | pub async fn slug_conflicts( | |
| 42 | - | pool: &PgPool, | |
| 43 | - | project_id: ProjectId, | |
| 44 | - | slug: &str, | |
| 45 | - | exclude_id: Option<ProjectSectionId>, | |
| 46 | - | ) -> Result<bool> { | |
| 47 | - | let exists: bool = sqlx::query_scalar( | |
| 48 | - | r#" | |
| 49 | - | SELECT EXISTS( | |
| 50 | - | SELECT 1 FROM project_sections | |
| 51 | - | WHERE project_id = $1 AND slug = $2 | |
| 52 | - | AND ($3::uuid IS NULL OR id <> $3) | |
| 53 | - | ) | |
| 54 | - | "#, | |
| 55 | - | ) | |
| 56 | - | .bind(project_id) | |
| 57 | - | .bind(slug) | |
| 58 | - | .bind(exclude_id) | |
| 59 | - | .fetch_one(pool) | |
| 60 | - | .await?; | |
| 61 | - | Ok(exists) | |
| 62 | - | } | |
| 63 | - | ||
| 64 | 35 | /// Insert a new section for a project. | |
| 65 | 36 | #[tracing::instrument(skip_all)] | |
| 66 | 37 | pub async fn create( |
| @@ -36,6 +36,51 @@ pub static HTTP_CLIENT: std::sync::LazyLock<reqwest::Client> = std::sync::LazyLo | |||
| 36 | 36 | .expect("build shared reqwest client") | |
| 37 | 37 | }); | |
| 38 | 38 | ||
| 39 | + | /// Maximum auto-suffix attempts when allocating a unique slug before giving up. | |
| 40 | + | pub const SLUG_SUFFIX_CAP: u32 = 100; | |
| 41 | + | ||
| 42 | + | /// Whether `e` is a Postgres unique-violation (SQLSTATE 23505) — the slug | |
| 43 | + | /// collision backstop every [`insert_with_unique_slug`] caller relies on. | |
| 44 | + | pub fn is_unique_violation(e: &crate::error::AppError) -> bool { | |
| 45 | + | matches!( | |
| 46 | + | e, | |
| 47 | + | crate::error::AppError::Database(sqlx::Error::Database(db_err)) | |
| 48 | + | if db_err.code().as_deref() == Some("23505") | |
| 49 | + | ) | |
| 50 | + | } | |
| 51 | + | ||
| 52 | + | /// Insert a row whose slug must be unique under a per-parent `UNIQUE` index, | |
| 53 | + | /// auto-suffixing the slug (`base`, `base-2`, `base-3`, ...) on collision. | |
| 54 | + | /// | |
| 55 | + | /// The `UNIQUE` index is the race-safe source of truth: `insert` is retried on a | |
| 56 | + | /// 23505 unique violation, up to [`SLUG_SUFFIX_CAP`] attempts, so a concurrent | |
| 57 | + | /// insert racing between any pre-check and this one can never surface a raw 500. | |
| 58 | + | /// `insert` receives the candidate slug and performs the actual row insert. | |
| 59 | + | /// Collapses the formerly copy-pasted suffix loops across blog posts, items, and | |
| 60 | + | /// item/project sections (ultra-fuzz Run #1 UX, D1) — including the internal | |
| 61 | + | /// content route that previously pre-checked but did not retry the insert. | |
| 62 | + | pub async fn insert_with_unique_slug<T, F, Fut>( | |
| 63 | + | base: &str, | |
| 64 | + | mut insert: F, | |
| 65 | + | ) -> crate::error::Result<T> | |
| 66 | + | where | |
| 67 | + | F: FnMut(String) -> Fut, | |
| 68 | + | Fut: std::future::Future<Output = crate::error::Result<T>>, | |
| 69 | + | { | |
| 70 | + | let mut slug = base.to_string(); | |
| 71 | + | let mut suffix = 1u32; | |
| 72 | + | loop { | |
| 73 | + | match insert(slug.clone()).await { | |
| 74 | + | Ok(value) => return Ok(value), | |
| 75 | + | Err(e) if is_unique_violation(&e) && suffix < SLUG_SUFFIX_CAP => { | |
| 76 | + | suffix += 1; | |
| 77 | + | slug = format!("{base}-{suffix}"); | |
| 78 | + | } | |
| 79 | + | Err(e) => return Err(e), | |
| 80 | + | } | |
| 81 | + | } | |
| 82 | + | } | |
| 83 | + | ||
| 39 | 84 | /// Escape HTML special characters, including `'` so the output is safe in | |
| 40 | 85 | /// single-quoted attribute contexts as well as element text. Use whenever a | |
| 41 | 86 | /// value is interpolated into a raw `format!`-built HTML string instead of an |
| @@ -135,24 +135,11 @@ pub(super) async fn create_blog_post( | |||
| 135 | 135 | validation::validate_blog_post_title(&req.title)?; | |
| 136 | 136 | ||
| 137 | 137 | // Generate or validate slug | |
| 138 | - | let mut slug = match req.slug { | |
| 138 | + | let slug = match req.slug { | |
| 139 | 139 | Some(ref s) if !s.is_empty() => Slug::new(s)?, | |
| 140 | 140 | _ => slugify(&req.title), | |
| 141 | 141 | }; | |
| 142 | 142 | ||
| 143 | - | // Fast path: append suffixes for known slug collisions | |
| 144 | - | if db::blog_posts::blog_post_slug_exists(&state.db, project_id, &slug).await? { | |
| 145 | - | let base = slug.clone(); | |
| 146 | - | let mut counter = 2u32; | |
| 147 | - | loop { | |
| 148 | - | slug = Slug::from_trusted(format!("{}-{}", base, counter)); | |
| 149 | - | if !db::blog_posts::blog_post_slug_exists(&state.db, project_id, &slug).await? { | |
| 150 | - | break; | |
| 151 | - | } | |
| 152 | - | counter += 1; | |
| 153 | - | } | |
| 154 | - | } | |
| 155 | - | ||
| 156 | 143 | let body_markdown = req.body_markdown.as_deref().unwrap_or(""); | |
| 157 | 144 | validation::validate_blog_post_body(body_markdown)?; | |
| 158 | 145 | ||
| @@ -160,34 +147,24 @@ pub(super) async fn create_blog_post( | |||
| 160 | 147 | let body_html = crate::markdown::render_creator_markdown(body_markdown, user.id, cdn_base); | |
| 161 | 148 | let is_published = req.is_published.unwrap_or(false); | |
| 162 | 149 | ||
| 163 | - | // Retry with suffixes if a concurrent request creates the same slug | |
| 164 | - | // between our existence check and insert (TOCTOU race). | |
| 165 | 150 | let web_only = req.web_only.unwrap_or(false); | |
| 166 | 151 | let show_on_landing = req.show_on_landing.unwrap_or(false); | |
| 167 | 152 | ||
| 168 | - | let base_slug = slug.clone(); | |
| 169 | - | let mut suffix = 1u32; | |
| 170 | - | let post = loop { | |
| 171 | - | match db::blog_posts::create_blog_post( | |
| 172 | - | &state.db, project_id, user.id, &req.title, &slug, | |
| 173 | - | body_markdown, &body_html, is_published, web_only, show_on_landing, | |
| 174 | - | ).await { | |
| 175 | - | Ok(post) => break post, | |
| 176 | - | Err(e) => { | |
| 177 | - | let is_slug_conflict = matches!( | |
| 178 | - | &e, | |
| 179 | - | AppError::Database(sqlx::Error::Database(db_err)) | |
| 180 | - | if db_err.code().as_deref() == Some("23505") | |
| 181 | - | ); | |
| 182 | - | if is_slug_conflict && suffix < 100 { | |
| 183 | - | suffix += 1; | |
| 184 | - | slug = Slug::from_trusted(format!("{}-{}", base_slug, suffix)); | |
| 185 | - | continue; | |
| 186 | - | } | |
| 187 | - | return Err(e); | |
| 188 | - | } | |
| 189 | - | } | |
| 190 | - | }; | |
| 153 | + | // The UNIQUE(project_id, slug) index is the race-safe source of truth: | |
| 154 | + | // `insert_with_unique_slug` auto-suffixes (`slug`, `slug-2`, ...) on a 23505 | |
| 155 | + | // collision, covering the TOCTOU race where a concurrent request grabs the | |
| 156 | + | // same slug. | |
| 157 | + | let base = slug.to_string(); | |
| 158 | + | let pool = &state.db; | |
| 159 | + | let (title_s, body_html_s) = (req.title.as_str(), body_html.as_str()); | |
| 160 | + | let post = crate::helpers::insert_with_unique_slug(&base, |slug| async move { | |
| 161 | + | let slug = Slug::from_trusted(slug); | |
| 162 | + | db::blog_posts::create_blog_post( | |
| 163 | + | pool, project_id, user.id, title_s, &slug, | |
| 164 | + | body_markdown, body_html_s, is_published, web_only, show_on_landing, | |
| 165 | + | ).await | |
| 166 | + | }) | |
| 167 | + | .await?; | |
| 191 | 168 | ||
| 192 | 169 | db::projects::bump_cache_generation(&state.db, project_id).await?; | |
| 193 | 170 |
| @@ -121,18 +121,7 @@ pub(super) async fn create_blog_post( | |||
| 121 | 121 | validation::validate_blog_post_body(&req.body_markdown)?; | |
| 122 | 122 | } | |
| 123 | 123 | ||
| 124 | - | let mut slug = helpers::slugify(&req.title); | |
| 125 | - | if db::blog_posts::blog_post_slug_exists(&state.db, req.project_id, &slug).await? { | |
| 126 | - | let base = slug.clone(); | |
| 127 | - | let mut counter = 2u32; | |
| 128 | - | loop { | |
| 129 | - | slug = Slug::from_trusted(format!("{}-{}", base, counter)); | |
| 130 | - | if !db::blog_posts::blog_post_slug_exists(&state.db, req.project_id, &slug).await? { | |
| 131 | - | break; | |
| 132 | - | } | |
| 133 | - | counter += 1; | |
| 134 | - | } | |
| 135 | - | } | |
| 124 | + | let base = helpers::slugify(&req.title).to_string(); | |
| 136 | 125 | ||
| 137 | 126 | let cdn_base = state.config.cdn_base_url.as_deref().unwrap_or("https://cdn.makenot.work"); | |
| 138 | 127 | let body_html = crate::markdown::render_creator_markdown(&req.body_markdown, req.user_id, cdn_base); | |
| @@ -140,18 +129,21 @@ pub(super) async fn create_blog_post( | |||
| 140 | 129 | // If publish_at is set, create as draft and then set the schedule | |
| 141 | 130 | let publish = if req.publish_at.is_some() { false } else { req.publish }; | |
| 142 | 131 | ||
| 143 | - | let post = db::blog_posts::create_blog_post( | |
| 144 | - | &state.db, | |
| 145 | - | req.project_id, | |
| 146 | - | req.user_id, | |
| 147 | - | &req.title, | |
| 148 | - | &slug, | |
| 149 | - | &req.body_markdown, | |
| 150 | - | &body_html, | |
| 151 | - | publish, | |
| 152 | - | false, | |
| 153 | - | false, | |
| 154 | - | ) | |
| 132 | + | // Auto-suffix on a UNIQUE(project_id, slug) collision and retry. The prior | |
| 133 | + | // code pre-checked existence but did NOT retry the insert, so a concurrent | |
| 134 | + | // create racing the same slug surfaced a raw 500 (ultra-fuzz Run #1 UX D1). | |
| 135 | + | let pool = &state.db; | |
| 136 | + | let (project_id, user_id) = (req.project_id, req.user_id); | |
| 137 | + | let (title_s, body_md_s, body_html_s) = | |
| 138 | + | (req.title.as_str(), req.body_markdown.as_str(), body_html.as_str()); | |
| 139 | + | let post = crate::helpers::insert_with_unique_slug(&base, |slug| async move { | |
| 140 | + | let slug = Slug::from_trusted(slug); | |
| 141 | + | db::blog_posts::create_blog_post( | |
| 142 | + | pool, project_id, user_id, title_s, &slug, | |
| 143 | + | body_md_s, body_html_s, publish, false, false, | |
| 144 | + | ) | |
| 145 | + | .await | |
| 146 | + | }) | |
| 155 | 147 | .await?; | |
| 156 | 148 | ||
| 157 | 149 | // Apply scheduled publish time if provided |
| @@ -95,54 +95,24 @@ pub(in crate::routes::api) async fn create_section( | |||
| 95 | 95 | ||
| 96 | 96 | let sort_order = count as i32; | |
| 97 | 97 | ||
| 98 | - | // Resolve a collision-free slug. Two sections whose titles slugify to the | |
| 99 | - | // same value (e.g. "Intro" twice, or any non-Latin titles sharing the | |
| 100 | - | // hash-fallback base) must get an auto-suffixed slug rather than a raw 500 | |
| 101 | - | // from the UNIQUE(item_id, slug) index (ultra-fuzz Run #1 UX). The index | |
| 102 | - | // stays the race-safe source of truth via the 23505 retry below. | |
| 98 | + | // Two sections whose titles slugify to the same value (e.g. "Intro" twice, | |
| 99 | + | // or non-Latin titles sharing the hash-fallback base) get an auto-suffixed | |
| 100 | + | // slug rather than a raw 500 from the UNIQUE(item_id, slug) index (ultra-fuzz | |
| 101 | + | // Run #1 UX). `insert_with_unique_slug` owns the `-N` suffixing and 23505 | |
| 102 | + | // retry, with the index as the race-safe source of truth. | |
| 103 | 103 | let base = slugify(&title).to_string(); | |
| 104 | - | let mut slug = base.clone(); | |
| 105 | - | let mut suffix = 2u32; | |
| 106 | - | while db::item_sections::slug_conflicts(&state.db, item_id, &slug, None).await? { | |
| 107 | - | slug = format!("{base}-{suffix}"); | |
| 108 | - | suffix += 1; | |
| 109 | - | } | |
| 110 | - | ||
| 111 | - | let section = loop { | |
| 112 | - | match db::item_sections::create( | |
| 113 | - | &state.db, | |
| 114 | - | item_id, | |
| 115 | - | &title, | |
| 116 | - | &slug, | |
| 117 | - | &req.body, | |
| 118 | - | sort_order, | |
| 119 | - | ) | |
| 120 | - | .await | |
| 121 | - | { | |
| 122 | - | Ok(section) => break section, | |
| 123 | - | Err(e) if is_slug_conflict(&e) && suffix < 100 => { | |
| 124 | - | slug = format!("{base}-{suffix}"); | |
| 125 | - | suffix += 1; | |
| 126 | - | } | |
| 127 | - | Err(e) => return Err(e), | |
| 128 | - | } | |
| 129 | - | }; | |
| 104 | + | let pool = &state.db; | |
| 105 | + | let (title_s, body_s) = (title.as_str(), req.body.as_str()); | |
| 106 | + | let section = crate::helpers::insert_with_unique_slug(&base, |slug| async move { | |
| 107 | + | db::item_sections::create(pool, item_id, title_s, &slug, body_s, sort_order).await | |
| 108 | + | }) | |
| 109 | + | .await?; | |
| 130 | 110 | ||
| 131 | 111 | db::projects::bump_cache_generation(&state.db, item.project_id).await?; | |
| 132 | 112 | ||
| 133 | 113 | Ok(Json(SectionResponse::from(section))) | |
| 134 | 114 | } | |
| 135 | 115 | ||
| 136 | - | /// Whether an error is a Postgres unique-violation (slug collision on the | |
| 137 | - | /// `UNIQUE(item_id, slug)` index) — the TOCTOU backstop for the dedup loops. | |
| 138 | - | fn is_slug_conflict(e: &AppError) -> bool { | |
| 139 | - | matches!( | |
| 140 | - | e, | |
| 141 | - | AppError::Database(sqlx::Error::Database(db_err)) | |
| 142 | - | if db_err.code().as_deref() == Some("23505") | |
| 143 | - | ) | |
| 144 | - | } | |
| 145 | - | ||
| 146 | 116 | /// List all sections for a given item (public items only; drafts return 404). | |
| 147 | 117 | #[tracing::instrument(skip_all, name = "items::list_sections")] | |
| 148 | 118 | pub(in crate::routes::api) async fn list_sections( | |
| @@ -179,34 +149,16 @@ pub(in crate::routes::api) async fn update_section( | |||
| 179 | 149 | ||
| 180 | 150 | let (item, _) = verify_item_ownership(&state, section.item_id, user.id).await?; | |
| 181 | 151 | ||
| 182 | - | // Same dedup as create, but exclude this row so renaming a section to a | |
| 183 | - | // slug it already owns isn't a self-conflict. | |
| 152 | + | // Same dedup as create. Re-saving the section under its own slug is not a | |
| 153 | + | // conflict (same row), so the bare base succeeds; only a collision with a | |
| 154 | + | // *different* section triggers the `-N` suffix retry. | |
| 184 | 155 | let base = slugify(&title).to_string(); | |
| 185 | - | let mut slug = base.clone(); | |
| 186 | - | let mut suffix = 2u32; | |
| 187 | - | while db::item_sections::slug_conflicts(&state.db, section.item_id, &slug, Some(section_id)).await? { | |
| 188 | - | slug = format!("{base}-{suffix}"); | |
| 189 | - | suffix += 1; | |
| 190 | - | } | |
| 191 | - | ||
| 192 | - | let updated = loop { | |
| 193 | - | match db::item_sections::update( | |
| 194 | - | &state.db, | |
| 195 | - | section_id, | |
| 196 | - | &title, | |
| 197 | - | &slug, | |
| 198 | - | &req.body, | |
| 199 | - | ) | |
| 200 | - | .await | |
| 201 | - | { | |
| 202 | - | Ok(updated) => break updated, | |
| 203 | - | Err(e) if is_slug_conflict(&e) && suffix < 100 => { | |
| 204 | - | slug = format!("{base}-{suffix}"); | |
| 205 | - | suffix += 1; | |
| 206 | - | } | |
| 207 | - | Err(e) => return Err(e), | |
| 208 | - | } | |
| 209 | - | }; | |
| 156 | + | let pool = &state.db; | |
| 157 | + | let (title_s, body_s) = (title.as_str(), req.body.as_str()); | |
| 158 | + | let updated = crate::helpers::insert_with_unique_slug(&base, |slug| async move { | |
| 159 | + | db::item_sections::update(pool, section_id, title_s, &slug, body_s).await | |
| 160 | + | }) | |
| 161 | + | .await?; | |
| 210 | 162 | ||
| 211 | 163 | db::projects::bump_cache_generation(&state.db, item.project_id).await?; | |
| 212 | 164 |
| @@ -91,51 +91,22 @@ pub(super) async fn create_section( | |||
| 91 | 91 | let sort_order = count as i32; | |
| 92 | 92 | ||
| 93 | 93 | // Auto-suffix on slug collision instead of surfacing the UNIQUE(project_id, | |
| 94 | - | // slug) violation as a raw 500 (ultra-fuzz Run #1 UX). The index stays the | |
| 95 | - | // race-safe source of truth via the 23505 retry below. | |
| 94 | + | // slug) violation as a raw 500 (ultra-fuzz Run #1 UX). `insert_with_unique_slug` | |
| 95 | + | // owns the `-N` suffixing and 23505 retry, with the index as the race-safe | |
| 96 | + | // source of truth. | |
| 96 | 97 | let base = slugify(&title).to_string(); | |
| 97 | - | let mut slug = base.clone(); | |
| 98 | - | let mut suffix = 2u32; | |
| 99 | - | while db::project_sections::slug_conflicts(&state.db, project_id, &slug, None).await? { | |
| 100 | - | slug = format!("{base}-{suffix}"); | |
| 101 | - | suffix += 1; | |
| 102 | - | } | |
| 103 | - | ||
| 104 | - | let section = loop { | |
| 105 | - | match db::project_sections::create( | |
| 106 | - | &state.db, | |
| 107 | - | project_id, | |
| 108 | - | &title, | |
| 109 | - | &slug, | |
| 110 | - | &req.body, | |
| 111 | - | sort_order, | |
| 112 | - | ) | |
| 113 | - | .await | |
| 114 | - | { | |
| 115 | - | Ok(section) => break section, | |
| 116 | - | Err(e) if is_slug_conflict(&e) && suffix < 100 => { | |
| 117 | - | slug = format!("{base}-{suffix}"); | |
| 118 | - | suffix += 1; | |
| 119 | - | } | |
| 120 | - | Err(e) => return Err(e), | |
| 121 | - | } | |
| 122 | - | }; | |
| 98 | + | let pool = &state.db; | |
| 99 | + | let (title_s, body_s) = (title.as_str(), req.body.as_str()); | |
| 100 | + | let section = crate::helpers::insert_with_unique_slug(&base, |slug| async move { | |
| 101 | + | db::project_sections::create(pool, project_id, title_s, &slug, body_s, sort_order).await | |
| 102 | + | }) | |
| 103 | + | .await?; | |
| 123 | 104 | ||
| 124 | 105 | db::projects::bump_cache_generation(&state.db, project_id).await?; | |
| 125 | 106 | ||
| 126 | 107 | Ok(Json(SectionResponse::from(section))) | |
| 127 | 108 | } | |
| 128 | 109 | ||
| 129 | - | /// Whether an error is a Postgres unique-violation (slug collision on the | |
| 130 | - | /// `UNIQUE(project_id, slug)` index) — the TOCTOU backstop for the dedup loops. | |
| 131 | - | fn is_slug_conflict(e: &AppError) -> bool { | |
| 132 | - | matches!( | |
| 133 | - | e, | |
| 134 | - | AppError::Database(sqlx::Error::Database(db_err)) | |
| 135 | - | if db_err.code().as_deref() == Some("23505") | |
| 136 | - | ) | |
| 137 | - | } | |
| 138 | - | ||
| 139 | 110 | #[tracing::instrument(skip_all, name = "projects::list_sections")] | |
| 140 | 111 | pub(super) async fn list_sections( | |
| 141 | 112 | State(state): State<AppState>, | |
| @@ -170,34 +141,16 @@ pub(super) async fn update_section( | |||
| 170 | 141 | ||
| 171 | 142 | verify_project_ownership(&state, section.project_id, user.id).await?; | |
| 172 | 143 | ||
| 173 | - | // Same dedup as create, excluding this row so a no-op rename isn't a | |
| 174 | - | // self-conflict. | |
| 144 | + | // Same dedup as create. Re-saving under the section's own slug is not a | |
| 145 | + | // conflict (same row); only a collision with a different section triggers | |
| 146 | + | // the `-N` suffix retry. | |
| 175 | 147 | let base = slugify(&title).to_string(); | |
| 176 | - | let mut slug = base.clone(); | |
| 177 | - | let mut suffix = 2u32; | |
| 178 | - | while db::project_sections::slug_conflicts(&state.db, section.project_id, &slug, Some(section_id)).await? { | |
| 179 | - | slug = format!("{base}-{suffix}"); | |
| 180 | - | suffix += 1; | |
| 181 | - | } | |
| 182 | - | ||
| 183 | - | let updated = loop { | |
| 184 | - | match db::project_sections::update( | |
| 185 | - | &state.db, | |
| 186 | - | section_id, | |
| 187 | - | &title, | |
| 188 | - | &slug, | |
| 189 | - | &req.body, | |
| 190 | - | ) | |
| 191 | - | .await | |
| 192 | - | { | |
| 193 | - | Ok(updated) => break updated, | |
| 194 | - | Err(e) if is_slug_conflict(&e) && suffix < 100 => { | |
| 195 | - | slug = format!("{base}-{suffix}"); | |
| 196 | - | suffix += 1; | |
| 197 | - | } | |
| 198 | - | Err(e) => return Err(e), | |
| 199 | - | } | |
| 200 | - | }; | |
| 148 | + | let pool = &state.db; | |
| 149 | + | let (title_s, body_s) = (title.as_str(), req.body.as_str()); | |
| 150 | + | let updated = crate::helpers::insert_with_unique_slug(&base, |slug| async move { | |
| 151 | + | db::project_sections::update(pool, section_id, title_s, &slug, body_s).await | |
| 152 | + | }) | |
| 153 | + | .await?; | |
| 201 | 154 | ||
| 202 | 155 | db::projects::bump_cache_generation(&state.db, section.project_id).await?; | |
| 203 | 156 |
| @@ -336,8 +336,10 @@ async fn project_section_slug_generation() { | |||
| 336 | 336 | ||
| 337 | 337 | #[tokio::test] | |
| 338 | 338 | async fn project_section_unique_slug_per_project() { | |
| 339 | - | // Two different projects can have sections with the same slug; | |
| 340 | - | // within one project, duplicates fail at the DB unique index. | |
| 339 | + | // Within one project, two sections whose titles slugify to the same value | |
| 340 | + | // must both succeed: the second is auto-suffixed (`-2`) rather than | |
| 341 | + | // surfacing the UNIQUE(project_id, slug) violation as an error. The DB index | |
| 342 | + | // stays the race-safe source of truth via insert_with_unique_slug. | |
| 341 | 343 | let mut h = TestHarness::new().await; | |
| 342 | 344 | let project_id = setup_creator_with_project(&mut h, "psecuniq").await; | |
| 343 | 345 | ||
| @@ -349,6 +351,11 @@ async fn project_section_unique_slug_per_project() { | |||
| 349 | 351 | ) | |
| 350 | 352 | .await; | |
| 351 | 353 | assert!(resp.status.is_success()); | |
| 354 | + | assert!( | |
| 355 | + | resp.text.contains(r#""slug":"privacy-policy""#), | |
| 356 | + | "First section should take the bare slug, got {}", | |
| 357 | + | resp.text | |
| 358 | + | ); | |
| 352 | 359 | ||
| 353 | 360 | let resp = h | |
| 354 | 361 | .client | |
| @@ -358,8 +365,13 @@ async fn project_section_unique_slug_per_project() { | |||
| 358 | 365 | ) | |
| 359 | 366 | .await; | |
| 360 | 367 | assert!( | |
| 361 | - | !resp.status.is_success(), | |
| 362 | - | "Duplicate slug within a project should fail, got {} {}", | |
| 368 | + | resp.status.is_success(), | |
| 369 | + | "Duplicate title should auto-suffix, not fail, got {} {}", | |
| 363 | 370 | resp.status, resp.text | |
| 364 | 371 | ); | |
| 372 | + | assert!( | |
| 373 | + | resp.text.contains(r#""slug":"privacy-policy-2""#), | |
| 374 | + | "Second section should be auto-suffixed to privacy-policy-2, got {}", | |
| 375 | + | resp.text | |
| 376 | + | ); | |
| 365 | 377 | } |