From da4f048f851851b7d72b2825ab7450539311f76d Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sun, 11 Oct 2026 08:50:11 -0400 Subject: [PATCH 1/2] Enforce unique recipe item positions per recipe Add migration 0018 with unique indexes on recipe_ingredients and recipe_steps (recipe_id, position), so a recipe can never hold two ingredients or two steps at one position. The dinner detail read now orders by position alone and drops its read-time duplicate check; a database already holding a duplicate fails the migration. Maestro task 75bb8cc1-fd30-4050-90d2-a1627a865b22 Co-Authored-By: Claude Opus 5.5 --- docs/ARCHITECTURE.md | 11 +-- internal/db/recipes.sql.go | 4 +- internal/dinners/detail.go | 18 +--- internal/server/dinnerdetail_test.go | 26 +---- internal/store/migrate_test.go | 97 ++++++++++++++++++- internal/store/schema_test.go | 4 + .../0018_recipe_item_position_unique.sql | 9 ++ migrations/queries/recipes.sql | 4 +- 8 files changed, 119 insertions(+), 54 deletions(-) create mode 100644 migrations/0018_recipe_item_position_unique.sql diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 539863e..b32ccc4 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -95,8 +95,8 @@ Columns ending in `_ct` hold AES-256-GCM ciphertext (via `secretbox`). | --- | --- | | `household_settings` | One row: timezone, dinner_time (17:30), reminder_time (16:30), review_time (09:00), plan_draft day/time (Fri 09:00), suggestions day/time (Sun 08:00), chat_autosend day/time (Sat 08:00), onboarding_completed_at, updates_seen_at. | | `recipes` | A real recipe page: source_url (unique), site_name, title, total/prep/cook minutes (the page's JSON-LD), yield, raw_jsonld, fetched_at; and its grounding: photo_id, method, difficulty, minutes_total (normalized prep to table). | -| `recipe_ingredients` | recipe_id, position, raw_text, name, qty, unit, note. | -| `recipe_steps` | recipe_id, position, text. | +| `recipe_ingredients` | recipe_id, position (unique per recipe: the recipe order), raw_text, name, qty, unit, note. | +| `recipe_steps` | recipe_id, position (unique per recipe: the recipe order), text. | | `photos` | source_url, local_path, width, height, sha256, fetched_at. | | `dinners` | The meal record and its lifecycle: name, normalized_name, status (`suggested` / `quest_log` / `tried` / `regular` / `archived`), origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, recipe_url (a pasted recipe link awaiting grounding), photo_id, grounding_status (`pending` / `grounded` / `failed` / `manual`), grounding_error (why grounding failed), grounding_revision (raised whenever a dinner is renamed, so only the latest `ground_dinner` job may write it and no candidate offered before the rename grounds it), updated_by. | | `suggestion_batches` | trigger, status, ai_run_id. | @@ -215,10 +215,9 @@ automatic** (D-021). raw_text}]` and `steps` the step texts, both in recipe position order and empty (`[]`) with no recipe, as for a dinner whose grounding is `pending` or `failed`; a recipe entered by hand has only `raw_text`. - Two items at one position leave recipe order unknown, so such a recipe - is 500 rather than shown out of order. `source` is `{site_name}` of the - page the recipe was fetched from, null for a manual recipe or none; it - has no `url`, as the page's link may carry a personal token. + `source` is `{site_name}` of the page the recipe was fetched from, null + for a manual recipe or none; it has no `url`, as the page's link may + carry a personal token. `site_name` is the name the page gives its site (publisher or `og:site_name`) and null otherwise: a page naming no site is stored under its host, which may be the pasted link's — a token in a diff --git a/internal/db/recipes.sql.go b/internal/db/recipes.sql.go index b43b465..cc218c1 100644 --- a/internal/db/recipes.sql.go +++ b/internal/db/recipes.sql.go @@ -220,7 +220,7 @@ func (q *Queries) GetRecipeBySourceURL(ctx context.Context, sourceUrl string) (R const listRecipeIngredients = `-- name: ListRecipeIngredients :many SELECT id, recipe_id, position, raw_text, name, qty, unit, note, created_at, updated_at FROM recipe_ingredients WHERE recipe_id = ? -ORDER BY position, id +ORDER BY position ` func (q *Queries) ListRecipeIngredients(ctx context.Context, recipeID string) ([]RecipeIngredient, error) { @@ -260,7 +260,7 @@ func (q *Queries) ListRecipeIngredients(ctx context.Context, recipeID string) ([ const listRecipeSteps = `-- name: ListRecipeSteps :many SELECT id, recipe_id, position, text, created_at, updated_at FROM recipe_steps WHERE recipe_id = ? -ORDER BY position, id +ORDER BY position ` func (q *Queries) ListRecipeSteps(ctx context.Context, recipeID string) ([]RecipeStep, error) { diff --git a/internal/dinners/detail.go b/internal/dinners/detail.go index 6ccb079..721e15e 100644 --- a/internal/dinners/detail.go +++ b/internal/dinners/detail.go @@ -157,10 +157,7 @@ func readRecipe(ctx context.Context, q *db.Queries, recipeID string, d *Detail) if err != nil { return fmt.Errorf("ingredients: %w", err) } - for i, ing := range ings { - if i > 0 && ing.Position == ings[i-1].Position { - return duplicatePosition("ingredient", ing.Position) - } + for _, ing := range ings { d.Ingredients = append(d.Ingredients, Ingredient{ Name: nullString(ing.Name), Qty: nullString(ing.Qty), @@ -173,10 +170,7 @@ func readRecipe(ctx context.Context, q *db.Queries, recipeID string, d *Detail) if err != nil { return fmt.Errorf("steps: %w", err) } - for i, step := range steps { - if i > 0 && step.Position == steps[i-1].Position { - return duplicatePosition("step", step.Position) - } + for _, step := range steps { d.Steps = append(d.Steps, step.Text) } return nil @@ -283,11 +277,3 @@ func browserHost(u *url.URL) bool { } return true } - -// duplicatePosition is the error for two recipe items of kind at position. -// Position is the only record of recipe order, and the id that breaks the -// tie says nothing about it, so a recipe with a duplicate is refused rather -// than shown in an order nobody can trust. -func duplicatePosition(kind string, position int64) error { - return fmt.Errorf("two %ss at position %d: recipe order is unknown", kind, position) -} diff --git a/internal/server/dinnerdetail_test.go b/internal/server/dinnerdetail_test.go index 1789e1c..5d4e2c4 100644 --- a/internal/server/dinnerdetail_test.go +++ b/internal/server/dinnerdetail_test.go @@ -70,8 +70,8 @@ func TestGetDinnerGroundedSideQuest(t *testing.T) { } for _, stmt := range []string{ `UPDATE dinners SET status = 'quest_log', origin = 'side_quest', is_favorite = 1 WHERE id = ?`, - `UPDATE recipe_ingredients SET position = 1 - position WHERE recipe_id = (SELECT recipe_id FROM dinners WHERE id = ?)`, - `UPDATE recipe_steps SET position = 1 - position WHERE recipe_id = (SELECT recipe_id FROM dinners WHERE id = ?)`, + `UPDATE recipe_ingredients SET position = 9 - position WHERE recipe_id = (SELECT recipe_id FROM dinners WHERE id = ?)`, + `UPDATE recipe_steps SET position = 9 - position WHERE recipe_id = (SELECT recipe_id FROM dinners WHERE id = ?)`, } { if _, err := e.st.Writer.Exec(stmt, id); err != nil { t.Fatal(err) @@ -445,25 +445,3 @@ func TestGetDinnerWhyPickedIsCreatingSuggestion(t *testing.T) { t.Errorf("why_picked = %v, want %q", got.WhyPicked, reason) } } - -// TestGetDinnerDuplicatePositionFails: a recipe with two ingredients or two -// steps at one position has no order anyone can trust, so its detail is a -// 500 rather than the recipe in some order. -func TestGetDinnerDuplicatePositionFails(t *testing.T) { - t.Parallel() - e := newDinnersEnv(t) - for _, table := range []string{"recipe_ingredients", "recipe_steps"} { - id := createDinner(t, e.h, `{"name":"Family chili `+table+`"}`) - if rec, _ := callDinners(t, e.h, http.MethodPost, dinnersPath+"/"+id+"/ground", - `{"manual":{"ingredients":["1 lb beef","1 can beans"],"steps":["Brown.","Simmer."],"minutes":240,"method":"crockpot"}}`); rec.Code != http.StatusOK { - t.Fatalf("POST ground = %d: %s", rec.Code, rec.Body) - } - if _, err := e.st.Writer.Exec(`UPDATE `+table+` SET position = 0 WHERE recipe_id = (SELECT recipe_id FROM dinners WHERE id = ?)`, id); err != nil { - t.Fatal(err) - } - rec, _ := callDinners(t, e.h, http.MethodGet, dinnersPath+"/"+id, "") - if rec.Code != http.StatusInternalServerError || strings.Contains(rec.Body.String(), "Simmer.") || strings.Contains(rec.Body.String(), "1 lb beef") { - t.Errorf("GET with duplicate %s positions = %d: %s, want 500 and no recipe", table, rec.Code, rec.Body) - } - } -} diff --git a/internal/store/migrate_test.go b/internal/store/migrate_test.go index bf09f0c..fe4ab50 100644 --- a/internal/store/migrate_test.go +++ b/internal/store/migrate_test.go @@ -176,8 +176,8 @@ func TestMigrateEmbeddedMigrations(t *testing.T) { if err := Migrate(context.Background(), s.Writer, migrations.FS); err != nil { t.Fatal(err) } - if got := versions(migrationRows(t, s)); got != "0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision,0014_rotation_review_name,0015_google_reconnect_required,0016_shopping_list_untitled,0017_shopping_list_chat_send_unconfirmed" { - t.Errorf("versions = %q, want 0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision,0014_rotation_review_name,0015_google_reconnect_required,0016_shopping_list_untitled,0017_shopping_list_chat_send_unconfirmed", got) + if got := versions(migrationRows(t, s)); got != "0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision,0014_rotation_review_name,0015_google_reconnect_required,0016_shopping_list_untitled,0017_shopping_list_chat_send_unconfirmed,0018_recipe_item_position_unique" { + t.Errorf("versions = %q, want 0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision,0014_rotation_review_name,0015_google_reconnect_required,0016_shopping_list_untitled,0017_shopping_list_chat_send_unconfirmed,0018_recipe_item_position_unique", got) } } @@ -197,8 +197,8 @@ func TestMigrateBootstrapThenEmbeddedInitApplies(t *testing.T) { if err := Migrate(ctx, s.Writer, migrations.FS); err != nil { t.Fatal(err) } - if got := versions(migrationRows(t, s)); got != "0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision,0014_rotation_review_name,0015_google_reconnect_required,0016_shopping_list_untitled,0017_shopping_list_chat_send_unconfirmed" { - t.Errorf("versions = %q, want 0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision,0014_rotation_review_name,0015_google_reconnect_required,0016_shopping_list_untitled,0017_shopping_list_chat_send_unconfirmed", got) + if got := versions(migrationRows(t, s)); got != "0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision,0014_rotation_review_name,0015_google_reconnect_required,0016_shopping_list_untitled,0017_shopping_list_chat_send_unconfirmed,0018_recipe_item_position_unique" { + t.Errorf("versions = %q, want 0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision,0014_rotation_review_name,0015_google_reconnect_required,0016_shopping_list_untitled,0017_shopping_list_chat_send_unconfirmed,0018_recipe_item_position_unique", got) } if !tableExists(t, s, "dinners") { t.Error("table from 0001_init was not created") @@ -922,3 +922,92 @@ func TestMigrateShoppingListChatSendUnconfirmed(t *testing.T) { t.Errorf("shopping list after 0017 = %s, want %s", got, want) } } + +// seedRecipeItems inserts two recipes, each with an ingredient and a step +// at position 0, then the extra statements. +func seedRecipeItems(t *testing.T, s *Store, extra ...string) { + t.Helper() + stmts := []string{ + `INSERT INTO recipes (id, source_url, title, raw_jsonld, fetched_at, created_at, updated_at) + VALUES ('recipe-1', 'https://example.com/chili', 'Chili', '{}', 'f', 'c', 'u')`, + `INSERT INTO recipes (id, source_url, title, raw_jsonld, fetched_at, created_at, updated_at) + VALUES ('recipe-2', 'https://example.com/curry', 'Curry', '{}', 'f', 'c', 'u')`, + `INSERT INTO recipe_ingredients (id, recipe_id, position, raw_text, created_at, updated_at) + VALUES ('ing-1', 'recipe-1', 0, '1 onion', 'c', 'u'), ('ing-2', 'recipe-2', 0, '1 lime', 'c', 'u')`, + `INSERT INTO recipe_steps (id, recipe_id, position, text, created_at, updated_at) + VALUES ('step-1', 'recipe-1', 0, 'Chop.', 'c', 'u'), ('step-2', 'recipe-2', 0, 'Squeeze.', 'c', 'u')`, + } + for _, stmt := range append(stmts, extra...) { + if _, err := s.Writer.Exec(stmt); err != nil { + t.Fatalf("seed: %v", err) + } + } +} + +// TestMigrateRecipeItemPositionUnique applies 0018 over two recipes with +// items at the same position: each recipe keeps its items, and a second +// ingredient or step at a position its recipe already uses is refused. +func TestMigrateRecipeItemPositionUnique(t *testing.T) { + ctx := context.Background() + s := openTestStore(t) + migrateBefore(t, s, "0018") + seedRecipeItems(t, s) + + if err := Migrate(ctx, s.Writer, migrations.FS); err != nil { + t.Fatal(err) + } + + var got string + if err := s.Reader.QueryRowContext(ctx, `SELECT + (SELECT group_concat(id || '@' || recipe_id || ':' || position, ',') FROM (SELECT * FROM recipe_ingredients ORDER BY id)) || ' / ' || + (SELECT group_concat(id || '@' || recipe_id || ':' || position, ',') FROM (SELECT * FROM recipe_steps ORDER BY id))`).Scan(&got); err != nil { + t.Fatal(err) + } + if want := "ing-1@recipe-1:0,ing-2@recipe-2:0 / step-1@recipe-1:0,step-2@recipe-2:0"; got != want { + t.Errorf("recipe items after 0018 = %s, want %s", got, want) + } + + for _, tc := range []struct{ name, stmt string }{ + {"second ingredient at a position", `INSERT INTO recipe_ingredients (id, recipe_id, position, raw_text, created_at, updated_at) + VALUES ('ing-3', 'recipe-1', 0, '2 onions', 'c', 'u')`}, + {"second step at a position", `INSERT INTO recipe_steps (id, recipe_id, position, text, created_at, updated_at) + VALUES ('step-3', 'recipe-1', 0, 'Dice.', 'c', 'u')`}, + {"ingredient moved onto another's position", `UPDATE recipe_ingredients SET recipe_id = 'recipe-1' WHERE id = 'ing-2'`}, + {"step moved onto another's position", `UPDATE recipe_steps SET recipe_id = 'recipe-1' WHERE id = 'step-2'`}, + } { + if _, err := s.Writer.Exec(tc.stmt); err == nil || !strings.Contains(err.Error(), "UNIQUE constraint failed") { + t.Errorf("%s: error %v, want UNIQUE constraint failed", tc.name, err) + } + } +} + +// TestMigrateRecipeItemPositionUniqueRefusesDuplicate applies 0018 over a +// recipe already holding two ingredients, or two steps, at one position: +// its order is unknown, so the migration fails and is not recorded. +func TestMigrateRecipeItemPositionUniqueRefusesDuplicate(t *testing.T) { + for _, tc := range []struct{ table, stmt string }{ + {"recipe_ingredients", `INSERT INTO recipe_ingredients (id, recipe_id, position, raw_text, created_at, updated_at) + VALUES ('ing-3', 'recipe-1', 0, '2 onions', 'c', 'u')`}, + {"recipe_steps", `INSERT INTO recipe_steps (id, recipe_id, position, text, created_at, updated_at) + VALUES ('step-3', 'recipe-1', 0, 'Dice.', 'c', 'u')`}, + } { + t.Run(tc.table, func(t *testing.T) { + ctx := context.Background() + s := openTestStore(t) + migrateBefore(t, s, "0018") + seedRecipeItems(t, s, tc.stmt) + + err := Migrate(ctx, s.Writer, migrations.FS) + if err == nil || !strings.Contains(err.Error(), "UNIQUE constraint failed: "+tc.table+".recipe_id, "+tc.table+".position") { + t.Fatalf("Migrate = %v, want the duplicate position refused", err) + } + var n int + if err := s.Reader.QueryRowContext(ctx, `SELECT count(*) FROM schema_migrations WHERE version = '0018_recipe_item_position_unique'`).Scan(&n); err != nil { + t.Fatal(err) + } + if n != 0 { + t.Error("0018 recorded as applied") + } + }) + } +} diff --git a/internal/store/schema_test.go b/internal/store/schema_test.go index 1607353..f57c212 100644 --- a/internal/store/schema_test.go +++ b/internal/store/schema_test.go @@ -408,6 +408,8 @@ func TestSchemaUniqueConstraintsRejectDuplicates(t *testing.T) { {"notification_outbox", []string{"event_key", "channel"}}, {"schedule_runs", []string{"schedule", "period_key"}}, {"notification_prefs", []string{"event"}}, + {"recipe_ingredients", []string{"recipe_id", "position"}}, + {"recipe_steps", []string{"recipe_id", "position"}}, } s := seededStore(t) for _, c := range cases { @@ -445,6 +447,8 @@ func TestSchemaCompositeUniqueAllowsPartialOverlap(t *testing.T) { {"same plan_date, different dinner", "rotation_reviews", "plan_date", "dinners", map[string]any{"dinner_id": rowID("dinners", 1)}}, {"same event_key, different channel", "notification_outbox", "event_key", "", map[string]any{"channel": "chat"}}, {"same period_key, different schedule", "schedule_runs", "period_key", "", map[string]any{"schedule": "week_rollover"}}, + {"same position, different recipe", "recipe_ingredients", "position", "recipes", map[string]any{"recipe_id": rowID("recipes", 1)}}, + {"same position, different recipe", "recipe_steps", "position", "recipes", map[string]any{"recipe_id": rowID("recipes", 1)}}, } s := seededStore(t) for _, c := range cases { diff --git a/migrations/0018_recipe_item_position_unique.sql b/migrations/0018_recipe_item_position_unique.sql new file mode 100644 index 0000000..1d257d6 --- /dev/null +++ b/migrations/0018_recipe_item_position_unique.sql @@ -0,0 +1,9 @@ +-- 0018_recipe_item_position_unique: position is the only record of recipe +-- order, so a recipe never has two ingredients or two steps at one +-- position. Writers replace a recipe's items whole, numbered from 0, and +-- the detail read orders by position alone. A database already holding a +-- duplicate fails this migration rather than keep an order nobody can +-- trust. + +CREATE UNIQUE INDEX recipe_ingredients_recipe_position ON recipe_ingredients (recipe_id, position); +CREATE UNIQUE INDEX recipe_steps_recipe_position ON recipe_steps (recipe_id, position); diff --git a/migrations/queries/recipes.sql b/migrations/queries/recipes.sql index bbca037..1f17450 100644 --- a/migrations/queries/recipes.sql +++ b/migrations/queries/recipes.sql @@ -48,7 +48,7 @@ INSERT INTO recipe_ingredients ( -- name: ListRecipeIngredients :many SELECT * FROM recipe_ingredients WHERE recipe_id = ? -ORDER BY position, id; +ORDER BY position; -- name: DeleteRecipeIngredients :exec DELETE FROM recipe_ingredients @@ -64,7 +64,7 @@ INSERT INTO recipe_steps ( -- name: ListRecipeSteps :many SELECT * FROM recipe_steps WHERE recipe_id = ? -ORDER BY position, id; +ORDER BY position; -- name: DeleteRecipeSteps :exec DELETE FROM recipe_steps -- 2.55.0 From 3dda308f309a8b60aa867af28db1f42ce08419ee Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sun, 11 Oct 2026 11:52:51 -0400 Subject: [PATCH 2/2] Document the pre-deploy duplicate position check for migration 0018 (Maestro 75bb8cc1) Co-Authored-By: Claude Opus 5.5 --- docs/ARCHITECTURE.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index b32ccc4..a385f4a 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -121,6 +121,16 @@ Columns ending in `_ct` hold AES-256-GCM ciphertext (via `secretbox`). `recipes` holds a fetched recipe page; `dinners` holds the household's meal record and its lifecycle, optionally linked to a recipe and a photo. +Migration `0018_recipe_item_position_unique` makes position unique per +recipe and fails, so the service refuses to start, on a database already +holding a duplicate. Before deploying it, check the database; both queries +must return no rows: + +```sql +SELECT recipe_id, position FROM recipe_ingredients GROUP BY 1, 2 HAVING count(*) > 1; +SELECT recipe_id, position FROM recipe_steps GROUP BY 1, 2 HAVING count(*) > 1; +``` + ## Dinner lifecycle ``` -- 2.55.0