From 5dff7825d81551d443f600041454de248ecb4690 Mon Sep 17 00:00:00 2001 From: orchestrator-agent <20+orchestrator-agent@code.cubecraftcreations.com> Date: Tue, 6 Oct 2026 20:26:56 -0400 Subject: [PATCH] Integration fix 2: conditional recipe repairs and no pasted links in logs (Feature 12 review F1, F2) (#96) Co-authored-by: orchestrator-agent <20+orchestrator-agent@code.cubecraftcreations.com> --- docs/ARCHITECTURE.md | 11 +- internal/db/dinners.sql.go | 68 +++++++--- internal/db/querier.go | 7 +- internal/dinners/grounding.go | 122 +++++++++++++---- internal/dinners/grounding_test.go | 201 +++++++++++++++++++++++++++++ internal/recipes/ground.go | 2 +- internal/server/dinners.go | 16 ++- internal/server/grounding_test.go | 109 ++++++++++++++++ migrations/queries/dinners.sql | 34 +++-- 9 files changed, 509 insertions(+), 61 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 9ee7df7..161102d 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -250,7 +250,16 @@ automatic** (D-021). `search_failed` / `ai_failed` (only the reason code is logged, never the provider's text), and grounding a candidate or link that runs past 90 seconds (fetch, photo and `normalize_recipe`, all in the request) 504 - `ground_timeout`, leaving the dinner unchanged. + `ground_timeout`, leaving the dinner unchanged. An archived dinner is 409 + `dinner_archived`. A candidate or link is written only if the dinner is + still as it was before grounding began: one renamed or grounded some + other way meanwhile is 409 `ground_conflict`, and one archived meanwhile + 409 `dinner_archived`, either leaving the dinner unchanged. A method, + minutes or difficulty a member edits meanwhile is kept over the recipe's. + Any other failure grounding a candidate or link, or recording it on the + dinner, is 500, logged with a fixed reason and the dinner id only — + never the link, which can carry a personal token, or dependency error + text. - `/health` — liveness. - Everything else serves the embedded PWA with SPA fallback. diff --git a/internal/db/dinners.sql.go b/internal/db/dinners.sql.go index ff1200e..74c929d 100644 --- a/internal/db/dinners.sql.go +++ b/internal/db/dinners.sql.go @@ -248,46 +248,76 @@ func (q *Queries) GetDinner(ctx context.Context, id string) (Dinner, error) { const groundDinner = `-- name: GroundDinner :one UPDATE dinners -SET recipe_id = ?, +SET recipe_id = ?1, recipe_url = NULL, - photo_id = ?, - method = ?, - minutes_total = ?, - difficulty = ?, - grounding_status = ?, + photo_id = ?2, + method = CASE WHEN method IS ?3 + THEN ?4 ELSE method END, + minutes_total = CASE WHEN minutes_total IS ?5 + THEN ?6 ELSE minutes_total END, + difficulty = CASE WHEN difficulty IS ?7 + THEN ?8 ELSE difficulty END, + grounding_status = ?9, grounding_error = NULL, - updated_by = ?, - updated_at = ? -WHERE id = ? + updated_by = ?10, + updated_at = ?11 +WHERE id = ?12 + AND status <> 'archived' + AND name = ?13 + AND normalized_name = ?14 + AND grounding_status = ?15 + AND grounding_revision = ?16 + AND recipe_id IS ?17 RETURNING id, name, normalized_name, status, origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, photo_id, grounding_status, updated_by, created_at, updated_at, recipe_url, grounding_error, grounding_revision ` type GroundDinnerParams struct { - RecipeID sql.NullString - PhotoID sql.NullString - Method sql.NullString - MinutesTotal sql.NullInt64 - Difficulty sql.NullString - GroundingStatus string - UpdatedBy sql.NullString - UpdatedAt string - ID string + RecipeID sql.NullString + PhotoID sql.NullString + ObservedMethod sql.NullString + Method sql.NullString + ObservedMinutesTotal sql.NullInt64 + MinutesTotal sql.NullInt64 + ObservedDifficulty sql.NullString + Difficulty sql.NullString + GroundingStatus string + UpdatedBy sql.NullString + UpdatedAt string + ID string + ObservedName string + ObservedNormalizedName string + ObservedGroundingStatus string + ObservedGroundingRevision int64 + ObservedRecipeID sql.NullString } // Records a dinner's grounding by hand (grounded or manual) and clears any // grounding_error and pasted recipe_url: the recipe chosen replaces the -// link. An unknown id returns sql.ErrNoRows. +// link. It writes only an unarchived dinner still as it was observed +// before grounding (same name, normalized_name, grounding_status, +// grounding_revision and recipe_id), so a dinner renamed, archived or +// grounded some other way meanwhile is left as it is. That dinner, or an +// unknown id, returns sql.ErrNoRows. A method, minutes_total or difficulty +// no longer as observed is kept: a member edited it while it was grounding. func (q *Queries) GroundDinner(ctx context.Context, arg GroundDinnerParams) (Dinner, error) { row := q.db.QueryRowContext(ctx, groundDinner, arg.RecipeID, arg.PhotoID, + arg.ObservedMethod, arg.Method, + arg.ObservedMinutesTotal, arg.MinutesTotal, + arg.ObservedDifficulty, arg.Difficulty, arg.GroundingStatus, arg.UpdatedBy, arg.UpdatedAt, arg.ID, + arg.ObservedName, + arg.ObservedNormalizedName, + arg.ObservedGroundingStatus, + arg.ObservedGroundingRevision, + arg.ObservedRecipeID, ) var i Dinner err := row.Scan( diff --git a/internal/db/querier.go b/internal/db/querier.go index 382e233..0a27a86 100644 --- a/internal/db/querier.go +++ b/internal/db/querier.go @@ -69,7 +69,12 @@ type Querier interface { GetRecipeSearchSettings(ctx context.Context) (RecipeSearchSetting, error) // Records a dinner's grounding by hand (grounded or manual) and clears any // grounding_error and pasted recipe_url: the recipe chosen replaces the - // link. An unknown id returns sql.ErrNoRows. + // link. It writes only an unarchived dinner still as it was observed + // before grounding (same name, normalized_name, grounding_status, + // grounding_revision and recipe_id), so a dinner renamed, archived or + // grounded some other way meanwhile is left as it is. That dinner, or an + // unknown id, returns sql.ErrNoRows. A method, minutes_total or difficulty + // no longer as observed is kept: a member edited it while it was grounding. GroundDinner(ctx context.Context, arg GroundDinnerParams) (Dinner, error) // Records the background grounding of an unarchived dinner still pending at // the grounding_revision it was grounded for; a dinner grounded some other diff --git a/internal/dinners/grounding.go b/internal/dinners/grounding.go index 9fd4115..c978505 100644 --- a/internal/dinners/grounding.go +++ b/internal/dinners/grounding.go @@ -76,6 +76,18 @@ var groundTimeout = GroundTimeout // runs past GroundTimeout; the dinner is left as it was. var ErrGroundTimeout = errors.New("dinners: grounding the recipe took too long") +// ErrGroundFailed wraps any unexpected failure grounding a dinner by hand. +// Its text may quote the link and dependency errors, so it is never +// logged. +var ErrGroundFailed = errors.New("dinners: grounding the recipe failed") + +// ErrArchived is grounding an archived dinner by hand. +var ErrArchived = errors.New("dinner archived") + +// ErrGroundConflict is a candidate or link grounded for a dinner that was +// renamed or grounded some other way while it was grounding. +var ErrGroundConflict = errors.New("dinner changed while its recipe was grounding") + // GroundJob is the KindGroundDinner payload: the dinner to ground and its // grounding_revision when the job was enqueued. type GroundJob struct { @@ -323,11 +335,17 @@ type ManualRecipe struct { // Ground grounds dinner id as f says, recording updatedBy. A candidate or // link is grounded with recipes.Grounder.GroundURL and sets // grounding_status grounded, with the recipe's photo, method, minutes and -// difficulty. A manual recipe is stored as typed — no model runs — and -// sets grounding_status manual with its method and minutes; it has no -// photo, and the dinner keeps its difficulty. An invalid f is a -// *ValidationError; a page that cannot ground the dinner is a -// *recipes.ErrUngroundable and changes nothing. +// difficulty; one a member changed while it was grounding is kept. A +// manual recipe is stored as typed — no model runs — and sets +// grounding_status manual with its method and minutes; it has no photo, +// and the dinner keeps its difficulty. An invalid f is a *ValidationError +// and an archived dinner ErrArchived. A failure grounding a candidate or +// link, or recording it on the dinner, wraps ErrGroundFailed; a page that +// cannot ground the dinner is also a *recipes.ErrUngroundable. A dinner +// renamed, archived or grounded some other way while a candidate or link +// was grounding is ErrGroundConflict or ErrArchived. A missing dinner is +// ErrNotFound; any other failure, by candidate, link or manual recipe, +// wraps ErrGroundFailed. None of these changes the dinner. func (s *Service) Ground(ctx context.Context, id string, f Fix, updatedBy string) (Dinner, error) { if updatedBy == "" { return Dinner{}, errors.New("dinners: ground: no member to record as updated_by") @@ -339,9 +357,16 @@ func (s *Service) Ground(ctx context.Context, id string, f Fix, updatedBy string if manual != nil { return s.groundManual(ctx, id, *manual, updatedBy) } - if _, err := s.get(ctx, id); err != nil { + cur, err := s.get(ctx, id) + if errors.Is(err, ErrNotFound) { return Dinner{}, err } + if err != nil { + return Dinner{}, fmt.Errorf("%w: dinner %s: %w", ErrGroundFailed, id, err) + } + if cur.Status == StatusArchived { + return Dinner{}, ErrArchived + } groundCtx, cancel := context.WithTimeout(ctx, groundTimeout) defer cancel() gr, err := s.grounder.GroundURL(groundCtx, link) @@ -349,30 +374,57 @@ func (s *Service) Ground(ctx context.Context, id string, f Fix, updatedBy string if ctx.Err() == nil && errors.Is(groundCtx.Err(), context.DeadlineExceeded) { return Dinner{}, ErrGroundTimeout } - return Dinner{}, fmt.Errorf("dinners: ground %s: %w", id, err) + return Dinner{}, fmt.Errorf("%w: dinner %s: %w", ErrGroundFailed, id, err) } row, err := db.New(s.st.Writer).GroundDinner(ctx, db.GroundDinnerParams{ - RecipeID: sql.NullString{String: gr.RecipeID, Valid: true}, - PhotoID: sql.NullString{String: gr.PhotoID, Valid: true}, - Method: sql.NullString{String: gr.Method, Valid: true}, - MinutesTotal: nullInt(gr.MinutesTotal), - Difficulty: sql.NullString{String: gr.Difficulty, Valid: true}, - GroundingStatus: GroundingGrounded, - UpdatedBy: sql.NullString{String: updatedBy, Valid: true}, - UpdatedAt: store.Now().String(), - ID: id, + RecipeID: sql.NullString{String: gr.RecipeID, Valid: true}, + PhotoID: sql.NullString{String: gr.PhotoID, Valid: true}, + Method: sql.NullString{String: gr.Method, Valid: true}, + MinutesTotal: nullInt(gr.MinutesTotal), + Difficulty: sql.NullString{String: gr.Difficulty, Valid: true}, + GroundingStatus: GroundingGrounded, + UpdatedBy: sql.NullString{String: updatedBy, Valid: true}, + UpdatedAt: store.Now().String(), + ID: cur.ID, + ObservedName: cur.Name, + ObservedNormalizedName: cur.NormalizedName, + ObservedGroundingStatus: cur.GroundingStatus, + ObservedGroundingRevision: cur.GroundingRevision, + ObservedRecipeID: cur.RecipeID, + ObservedMethod: cur.Method, + ObservedMinutesTotal: cur.MinutesTotal, + ObservedDifficulty: cur.Difficulty, }) if errors.Is(err, sql.ErrNoRows) { - return Dinner{}, ErrNotFound + return Dinner{}, s.groundRejected(ctx, id) } if err != nil { - return Dinner{}, fmt.Errorf("dinners: ground %s: %w", id, err) + return Dinner{}, fmt.Errorf("%w: dinner %s: record: %w", ErrGroundFailed, id, err) } return fromRow(row), nil } +// groundRejected is the error for dinner id when it was not as observed +// before grounding: ErrNotFound, ErrArchived or ErrGroundConflict. Failing +// to read it wraps ErrGroundFailed. +func (s *Service) groundRejected(ctx context.Context, id string) error { + cur, err := s.get(ctx, id) + if errors.Is(err, ErrNotFound) { + return err + } + if err != nil { + return fmt.Errorf("%w: dinner %s: %w", ErrGroundFailed, id, err) + } + if cur.Status == StatusArchived { + return ErrArchived + } + return ErrGroundConflict +} + // groundManual stores m as dinner id's recipe, with its ingredients and -// steps, and records it on the dinner, all in one transaction. +// steps, and records it on the dinner, all in one transaction. A missing +// or archived dinner is ErrNotFound or ErrArchived; any other failure +// wraps ErrGroundFailed. func (s *Service) groundManual(ctx context.Context, id string, m ManualRecipe, updatedBy string) (Dinner, error) { var row db.Dinner err := s.st.WithTx(ctx, func(tx *sql.Tx) error { @@ -384,6 +436,9 @@ func (s *Service) groundManual(ctx context.Context, id string, m ManualRecipe, u if err != nil { return fmt.Errorf("dinners: read %s: %w", id, err) } + if cur.Status == StatusArchived { + return ErrArchived + } ld, err := json.Marshal(manualJSONLD{ Context: "https://schema.org", Type: "Recipe", @@ -439,23 +494,34 @@ func (s *Service) groundManual(ctx context.Context, id string, m ManualRecipe, u } } row, err = q.GroundDinner(ctx, db.GroundDinnerParams{ - RecipeID: sql.NullString{String: rec.ID, Valid: true}, - Method: method, - MinutesTotal: minutes, - Difficulty: cur.Difficulty, - GroundingStatus: GroundingManual, - UpdatedBy: sql.NullString{String: updatedBy, Valid: true}, - UpdatedAt: now, - ID: cur.ID, + RecipeID: sql.NullString{String: rec.ID, Valid: true}, + Method: method, + MinutesTotal: minutes, + Difficulty: cur.Difficulty, + GroundingStatus: GroundingManual, + UpdatedBy: sql.NullString{String: updatedBy, Valid: true}, + UpdatedAt: now, + ID: cur.ID, + ObservedName: cur.Name, + ObservedNormalizedName: cur.NormalizedName, + ObservedGroundingStatus: cur.GroundingStatus, + ObservedGroundingRevision: cur.GroundingRevision, + ObservedRecipeID: cur.RecipeID, + ObservedMethod: cur.Method, + ObservedMinutesTotal: cur.MinutesTotal, + ObservedDifficulty: cur.Difficulty, }) if err != nil { return fmt.Errorf("dinners: ground %s: %w", id, err) } return nil }) - if err != nil { + if errors.Is(err, ErrNotFound) || errors.Is(err, ErrArchived) { return Dinner{}, err } + if err != nil { + return Dinner{}, fmt.Errorf("%w: dinner %s: %w", ErrGroundFailed, id, err) + } return fromRow(row), nil } diff --git a/internal/dinners/grounding_test.go b/internal/dinners/grounding_test.go index 2e14546..2578083 100644 --- a/internal/dinners/grounding_test.go +++ b/internal/dinners/grounding_test.go @@ -1106,3 +1106,204 @@ func TestGroundTimesOut(t *testing.T) { t.Errorf("grounding_status = %s, want failed as before", row.GroundingStatus) } } + +// TestFixInFlightWhenDinnerChanges: a link still grounding when its dinner +// is renamed, archived or grounded some other way writes nothing; the +// change made meanwhile is kept. +func TestFixInFlightWhenDinnerChanges(t *testing.T) { + t.Parallel() + tests := []struct { + name string + change func(t *testing.T, e *env, id string) + want error + // kept checks the dinner shows the change, not the link's recipe. + kept func(row db.Dinner) bool + }{ + {"renamed", func(t *testing.T, e *env, id string) { + if _, err := e.svc.Update(t.Context(), id, dinners.Patch{Name: ptr("Chicken curry")}, member); err != nil { + t.Fatal(err) + } + }, dinners.ErrGroundConflict, func(row db.Dinner) bool { + return row.Name == "Chicken curry" && row.GroundingStatus == dinners.GroundingFailed && !row.RecipeID.Valid + }}, + {"renamed to the same normalized name", func(t *testing.T, e *env, id string) { + if _, err := e.svc.Update(t.Context(), id, dinners.Patch{Name: ptr("Easy Grandma's stew")}, member); err != nil { + t.Fatal(err) + } + }, dinners.ErrGroundConflict, func(row db.Dinner) bool { + return row.Name == "Easy Grandma's stew" && row.GroundingStatus == dinners.GroundingFailed && !row.RecipeID.Valid + }}, + {"archived", func(t *testing.T, e *env, id string) { + if _, err := e.svc.Archive(t.Context(), id, member); err != nil { + t.Fatal(err) + } + }, dinners.ErrArchived, func(row db.Dinner) bool { + return row.Status == dinners.StatusArchived && row.GroundingStatus == dinners.GroundingFailed && !row.RecipeID.Valid + }}, + {"grounded by hand", func(t *testing.T, e *env, id string) { + if _, err := e.svc.Ground(t.Context(), id, dinners.Fix{Manual: &dinners.ManualRecipe{ + Ingredients: []string{"1 lb beef"}, Minutes: ptr(240), Method: ptr("crockpot"), + }}, member); err != nil { + t.Fatal(err) + } + }, dinners.ErrGroundConflict, func(row db.Dinner) bool { + return row.GroundingStatus == dinners.GroundingManual && row.Method.String == "crockpot" + }}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + e := newEnv(t) + id := failedDinner(t, e, "Grandma's stew") + e.model.QueueGenerate(recipestest.NormalizedReply("oven", 150)) + e.pages.Gate = make(chan struct{}) + + done := make(chan error, 1) + go func() { + _, err := e.svc.Ground(context.Background(), id, dinners.Fix{RecipeURL: ptr(stew)}, member) + done <- err + }() + e.waitFetched(t, stew) + tt.change(t, e, id) + close(e.pages.Gate) + + if err := <-done; !errors.Is(err, tt.want) { + t.Fatalf("err = %v, want %v", err, tt.want) + } + if row := e.dinner(t, id); !tt.kept(row) { + t.Errorf("dinner = %+v, want the %s change kept and the link's recipe not written", row, tt.name) + } + }) + } +} + +// TestFixInFlightKeepsMemberEdits: a method, minutes or difficulty a +// member sets while a link is grounding is kept; a field they left alone +// takes the link's recipe, even one already set before grounding began. +func TestFixInFlightKeepsMemberEdits(t *testing.T) { + t.Parallel() + // Every field is set before grounding, then one is changed in flight. + before := dinners.Patch{Method: ptr("stovetop"), MinutesTotal: ptr(45), Difficulty: ptr("easy")} + tests := []struct { + name string + edit dinners.Patch + field string + }{ + {"method", dinners.Patch{Method: ptr("grill")}, "method"}, + {"minutes", dinners.Patch{MinutesTotal: ptr(20)}, "minutes_total"}, + {"difficulty", dinners.Patch{Difficulty: ptr("medium")}, "difficulty"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + e := newEnv(t) + id := failedDinner(t, e, "Grandma's stew") + if _, err := e.svc.Update(t.Context(), id, before, member); err != nil { + t.Fatal(err) + } + e.model.QueueGenerate(recipestest.NormalizedReply("oven", 150)) + e.pages.Gate = make(chan struct{}) + + done := make(chan error, 1) + go func() { + _, err := e.svc.Ground(context.Background(), id, dinners.Fix{RecipeURL: ptr(stew)}, member) + done <- err + }() + e.waitFetched(t, stew) + if _, err := e.svc.Update(t.Context(), id, tt.edit, member); err != nil { + t.Fatal(err) + } + close(e.pages.Gate) + + if err := <-done; err != nil { + t.Fatal(err) + } + row := e.dinner(t, id) + if row.GroundingStatus != dinners.GroundingGrounded || !row.RecipeID.Valid { + t.Fatalf("dinner = %+v, want grounded in the link's recipe", row) + } + var recipe fields + if err := e.st.Reader.QueryRowContext(t.Context(), + `SELECT method, minutes_total, difficulty FROM recipes WHERE id = ?`, row.RecipeID.String, + ).Scan(&recipe.method, &recipe.minutes, &recipe.difficulty); err != nil { + t.Fatal(err) + } + old, edited := patchFields(before), patchFields(tt.edit) + want := recipe + switch tt.field { + case "method": + want.method = edited.method + case "minutes_total": + want.minutes = edited.minutes + case "difficulty": + want.difficulty = edited.difficulty + } + if recipe.method == old.method || recipe.minutes == old.minutes || recipe.difficulty == old.difficulty { + t.Fatalf("recipe %+v shares a value with the dinner before grounding %+v", recipe, old) + } + got := fields{row.Method.String, row.MinutesTotal.Int64, row.Difficulty.String} + if got != want { + t.Errorf("method, minutes, difficulty = %+v, want %+v: the %s edit kept, the rest from the recipe", got, want, tt.field) + } + }) + } +} + +// fields are a dinner's or recipe's method, minutes_total and difficulty. +type fields struct { + method string + minutes int64 + difficulty string +} + +// patchFields returns the fields p sets, zero where it sets none. +func patchFields(p dinners.Patch) fields { + var f fields + if p.Method != nil { + f.method = *p.Method + } + if p.MinutesTotal != nil { + f.minutes = int64(*p.MinutesTotal) + } + if p.Difficulty != nil { + f.difficulty = *p.Difficulty + } + return f +} + +// TestFixArchivedDinner: an archived dinner cannot be grounded by hand. +func TestFixArchivedDinner(t *testing.T) { + t.Parallel() + e := newEnv(t) + id := failedDinner(t, e, "Grandma's stew") + if _, err := e.svc.Archive(t.Context(), id, member); err != nil { + t.Fatal(err) + } + before := e.dinner(t, id) + for _, f := range []dinners.Fix{ + {RecipeURL: ptr(stew)}, + {Manual: &dinners.ManualRecipe{Ingredients: []string{"1 lb beef"}, Minutes: ptr(240), Method: ptr("crockpot")}}, + } { + if _, err := e.svc.Ground(t.Context(), id, f, member); !errors.Is(err, dinners.ErrArchived) { + t.Errorf("fix %+v: err = %v, want ErrArchived", f, err) + } + } + if got := e.pages.Fetched(); len(got) != 0 { + t.Errorf("fetched %q for an archived dinner", got) + } + if after := e.dinner(t, id); after != before { + t.Errorf("archived dinner changed: %+v, was %+v", after, before) + } +} + +// waitFetched waits until rawURL has been fetched. +func (e *env) waitFetched(t *testing.T, rawURL string) { + t.Helper() + deadline := time.Now().Add(10 * time.Second) + for !slices.Contains(e.pages.Fetched(), rawURL) { + if time.Now().After(deadline) { + t.Fatalf("%s never fetched", rawURL) + } + time.Sleep(5 * time.Millisecond) + } +} diff --git a/internal/recipes/ground.go b/internal/recipes/ground.go index b5e5b16..6b14436 100644 --- a/internal/recipes/ground.go +++ b/internal/recipes/ground.go @@ -266,7 +266,7 @@ func (g *Grounder) GroundURL(ctx context.Context, rawURL string) (Grounded, erro } gr, reason, err := g.ground(ctx, rawURL, r) if err != nil { - return Grounded{}, fmt.Errorf("recipes: ground %s: %w", rawURL, err) + return Grounded{}, fmt.Errorf("recipes: ground pasted link: %w", err) } if reason != nil { return ungroundable(reason) diff --git a/internal/server/dinners.go b/internal/server/dinners.go index 185b823..b6c627f 100644 --- a/internal/server/dinners.go +++ b/internal/server/dinners.go @@ -200,7 +200,11 @@ func actorOrFail(w http.ResponseWriter, r *http.Request, logger *slog.Logger) (a // writeDinnerError maps a dinners.Service error to its API error. A page // that cannot ground the dinner is 422 ungroundable; recipe search not set // up is 409 not_configured; a search or model failure is 502; grounding by -// hand that runs past dinners.GroundTimeout is 504 ground_timeout. +// hand that runs past dinners.GroundTimeout is 504 ground_timeout; grounding +// an archived dinner is 409 dinner_archived and one changed while grounding +// 409 ground_conflict. Any other grounding failure, by candidate, link or +// manual recipe, wraps dinners.ErrGroundFailed and is 500, logged by reason +// and dinner id only. func writeDinnerError(w http.ResponseWriter, r *http.Request, logger *slog.Logger, action string, err error) { var invalid *dinners.ValidationError var dup *dinners.DuplicateError @@ -220,6 +224,16 @@ func writeDinnerError(w http.ResponseWriter, r *http.Request, logger *slog.Logge case errors.Is(err, recipes.ErrNormalizeFailed): logger.WarnContext(r.Context(), action, "reason", "normalize_recipe failed") writeError(w, r, logger, http.StatusBadGateway, "ai_failed", "the AI provider failed to read the recipe") + case errors.Is(err, dinners.ErrGroundFailed): + // The error may quote the pasted link, which can carry a personal + // token, and dependency errors, so only the reason and dinner are + // logged. + logger.ErrorContext(r.Context(), action, "reason", "ground_failed", "dinner_id", chi.URLParam(r, "id")) + writeError(w, r, logger, http.StatusInternalServerError, "internal", "internal error") + case errors.Is(err, dinners.ErrArchived): + writeError(w, r, logger, http.StatusConflict, "dinner_archived", "the dinner is archived") + case errors.Is(err, dinners.ErrGroundConflict): + writeError(w, r, logger, http.StatusConflict, "ground_conflict", "the dinner changed while its recipe was grounding; reload and try again") case errors.Is(err, dinners.ErrGroundTimeout): logger.WarnContext(r.Context(), action, "reason", "ground_timeout") writeError(w, r, logger, http.StatusGatewayTimeout, "ground_timeout", "grounding the recipe took too long; try again") diff --git a/internal/server/grounding_test.go b/internal/server/grounding_test.go index 1365f9e..e7333d9 100644 --- a/internal/server/grounding_test.go +++ b/internal/server/grounding_test.go @@ -224,6 +224,115 @@ func TestPostGroundViaRecipeURL(t *testing.T) { } } +// TestPostGroundViaRecipeURLStorageFailure: a pasted link that fails for a +// reason other than its recipe is a 500 logged by reason and dinner id, +// never with the link, which can carry a personal token, or the dependency +// error. +func TestPostGroundViaRecipeURLStorageFailure(t *testing.T) { + t.Parallel() + e := newDinnersEnv(t).withRecipes(t) + const token = "personal-token-abc123" + link := stewPage + "?access_token=" + token + e.pages.Bodies[link] = e.pages.Bodies[stewPage] + id := createDinner(t, e.h, `{"name":"Sunday stew"}`) + e.model.QueueGenerate(recipestest.NormalizedReply("oven", 150)) + if _, err := e.st.Writer.ExecContext(t.Context(), + `CREATE TRIGGER refuse_recipes BEFORE INSERT ON recipes BEGIN SELECT RAISE(ABORT, 'disk full'); END`); err != nil { + t.Fatal(err) + } + + rec, body := callDinners(t, e.h, http.MethodPost, dinnersPath+"/"+id+"/ground", `{"recipe_url":"`+link+`"}`) + if rec.Code != http.StatusInternalServerError || errorCode(body) != "internal" || strings.Contains(rec.Body.String(), token) { + t.Errorf("POST = %d %s, want 500 internal without the link", rec.Code, rec.Body) + } + logs := e.logs.String() + if strings.Contains(logs, token) || strings.Contains(logs, "c.example") || strings.Contains(logs, "disk full") || + !strings.Contains(logs, "reason=ground_failed") || !strings.Contains(logs, "dinner_id="+id) { + t.Errorf("logs = %q, want reason=ground_failed and the dinner id, without the link or dependency error", logs) + } +} + +// TestPostGroundViaRecipeURLDinnerWriteFailure: a pasted link whose recipe +// is stored but whose dinner cannot be written is a 500 logged by reason +// and dinner id, never with the dependency error. +func TestPostGroundViaRecipeURLDinnerWriteFailure(t *testing.T) { + t.Parallel() + e := newDinnersEnv(t).withRecipes(t) + id := createDinner(t, e.h, `{"name":"Sunday stew"}`) + e.model.QueueGenerate(recipestest.NormalizedReply("oven", 150)) + if _, err := e.st.Writer.ExecContext(t.Context(), + `CREATE TRIGGER refuse_grounding BEFORE UPDATE OF recipe_id ON dinners WHEN NEW.recipe_id IS NOT NULL BEGIN SELECT RAISE(ABORT, 'disk full'); END`); err != nil { + t.Fatal(err) + } + + rec, body := callDinners(t, e.h, http.MethodPost, dinnersPath+"/"+id+"/ground", `{"recipe_url":"`+stewPage+`"}`) + if rec.Code != http.StatusInternalServerError || errorCode(body) != "internal" { + t.Errorf("POST = %d %s, want 500 internal", rec.Code, rec.Body) + } + var stored int + if err := e.st.Reader.QueryRowContext(t.Context(), `SELECT COUNT(*) FROM recipes WHERE source_url = ?`, stewPage).Scan(&stored); err != nil || stored != 1 { + t.Fatalf("recipes stored for the link = %d (%v), want 1: the failure must be the dinner write", stored, err) + } + logs := e.logs.String() + if strings.Contains(logs, "disk full") || !strings.Contains(logs, "reason=ground_failed") || !strings.Contains(logs, "dinner_id="+id) { + t.Errorf("logs = %q, want reason=ground_failed and the dinner id, without the dependency error", logs) + } +} + +// TestPostGroundUnexpectedFailures: grounding by hand that fails reading +// the dinner, or storing a manual recipe, is a 500 logged by reason and +// dinner id, never with the database error. +func TestPostGroundUnexpectedFailures(t *testing.T) { + t.Parallel() + const ( + // Renaming a column every dinner read selects fails the read. + breakRead = `ALTER TABLE dinners RENAME COLUMN cuisine TO cuisine_gone` + // The trigger's message stands in for the database error. + breakStore = `CREATE TRIGGER refuse_recipes BEFORE INSERT ON recipes BEGIN SELECT RAISE(ABORT, 'disk full'); END` + manual = `{"manual":{"ingredients":["1 lb beef"],"steps":["Brown."],"minutes":240,"method":"crockpot"}}` + ) + for _, tc := range []struct { + name, sabotage, fix, leak string + }{ + {"link read", breakRead, `{"recipe_url":"` + stewPage + `"}`, "cuisine"}, + {"manual read", breakRead, manual, "cuisine"}, + {"manual store", breakStore, manual, "disk full"}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + e := newDinnersEnv(t).withRecipes(t) + id := createDinner(t, e.h, `{"name":"Sunday stew"}`) + if _, err := e.st.Writer.ExecContext(t.Context(), tc.sabotage); err != nil { + t.Fatal(err) + } + + rec, body := callDinners(t, e.h, http.MethodPost, dinnersPath+"/"+id+"/ground", tc.fix) + if rec.Code != http.StatusInternalServerError || errorCode(body) != "internal" { + t.Errorf("POST = %d %s, want 500 internal", rec.Code, rec.Body) + } + logs := e.logs.String() + if strings.Contains(logs, tc.leak) || !strings.Contains(logs, "reason=ground_failed") || !strings.Contains(logs, "dinner_id="+id) { + t.Errorf("logs = %q, want reason=ground_failed and the dinner id, without the database error", logs) + } + }) + } +} + +// TestPostGroundArchived: grounding an archived dinner by hand is a 409. +func TestPostGroundArchived(t *testing.T) { + t.Parallel() + e := newDinnersEnv(t) + id := createDinner(t, e.h, `{"name":"Family chili"}`) + if rec, _ := callDinners(t, e.h, http.MethodDelete, dinnersPath+"/"+id, ""); rec.Code != http.StatusOK { + t.Fatalf("DELETE = %d: %s", rec.Code, rec.Body) + } + rec, body := callDinners(t, e.h, http.MethodPost, dinnersPath+"/"+id+"/ground", + `{"manual":{"ingredients":["1 lb beef"],"minutes":240,"method":"crockpot"}}`) + if rec.Code != http.StatusConflict || errorCode(body) != "dinner_archived" { + t.Errorf("POST = %d %v, want 409 dinner_archived", rec.Code, body) + } +} + func TestPostGroundViaManual(t *testing.T) { t.Parallel() e := newDinnersEnv(t) diff --git a/migrations/queries/dinners.sql b/migrations/queries/dinners.sql index e11dec1..43e96ae 100644 --- a/migrations/queries/dinners.sql +++ b/migrations/queries/dinners.sql @@ -84,19 +84,33 @@ RETURNING *; -- name: GroundDinner :one -- Records a dinner's grounding by hand (grounded or manual) and clears any -- grounding_error and pasted recipe_url: the recipe chosen replaces the --- link. An unknown id returns sql.ErrNoRows. +-- link. It writes only an unarchived dinner still as it was observed +-- before grounding (same name, normalized_name, grounding_status, +-- grounding_revision and recipe_id), so a dinner renamed, archived or +-- grounded some other way meanwhile is left as it is. That dinner, or an +-- unknown id, returns sql.ErrNoRows. A method, minutes_total or difficulty +-- no longer as observed is kept: a member edited it while it was grounding. UPDATE dinners -SET recipe_id = ?, +SET recipe_id = sqlc.arg(recipe_id), recipe_url = NULL, - photo_id = ?, - method = ?, - minutes_total = ?, - difficulty = ?, - grounding_status = ?, + photo_id = sqlc.arg(photo_id), + method = CASE WHEN method IS sqlc.narg(observed_method) + THEN sqlc.arg(method) ELSE method END, + minutes_total = CASE WHEN minutes_total IS sqlc.narg(observed_minutes_total) + THEN sqlc.arg(minutes_total) ELSE minutes_total END, + difficulty = CASE WHEN difficulty IS sqlc.narg(observed_difficulty) + THEN sqlc.arg(difficulty) ELSE difficulty END, + grounding_status = sqlc.arg(grounding_status), grounding_error = NULL, - updated_by = ?, - updated_at = ? -WHERE id = ? + updated_by = sqlc.arg(updated_by), + updated_at = sqlc.arg(updated_at) +WHERE id = sqlc.arg(id) + AND status <> 'archived' + AND name = sqlc.arg(observed_name) + AND normalized_name = sqlc.arg(observed_normalized_name) + AND grounding_status = sqlc.arg(observed_grounding_status) + AND grounding_revision = sqlc.arg(observed_grounding_revision) + AND recipe_id IS sqlc.narg(observed_recipe_id) RETURNING *; -- name: GroundPendingDinner :execrows