diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 9794d18..161102d 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -254,10 +254,12 @@ automatic** (D-021). `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. Any other - failure grounding a candidate or link 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. + 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 83457a3..74c929d 100644 --- a/internal/db/dinners.sql.go +++ b/internal/db/dinners.sql.go @@ -251,32 +251,40 @@ UPDATE dinners SET recipe_id = ?1, recipe_url = NULL, photo_id = ?2, - method = ?3, - minutes_total = ?4, - difficulty = ?5, - grounding_status = ?6, + 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 = ?7, - updated_at = ?8 -WHERE id = ?9 + updated_by = ?10, + updated_at = ?11 +WHERE id = ?12 AND status <> 'archived' - AND normalized_name = ?10 - AND grounding_status = ?11 - AND grounding_revision = ?12 - AND recipe_id IS ?13 + 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 + 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 @@ -286,21 +294,26 @@ type GroundDinnerParams struct { // 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. It writes only an unarchived dinner still as it was observed -// before grounding (same normalized_name, grounding_status, +// 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. +// 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, diff --git a/internal/db/querier.go b/internal/db/querier.go index 0d8f5d6..0a27a86 100644 --- a/internal/db/querier.go +++ b/internal/db/querier.go @@ -70,10 +70,11 @@ type Querier interface { // 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. It writes only an unarchived dinner still as it was observed - // before grounding (same normalized_name, grounding_status, + // 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. + // 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 4bf7e70..19b0936 100644 --- a/internal/dinners/grounding.go +++ b/internal/dinners/grounding.go @@ -335,14 +335,16 @@ 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 and an archived dinner ErrArchived. A failure grounding -// a candidate or link 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. None of these changes the dinner. +// 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. 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") @@ -380,27 +382,35 @@ func (s *Service) Ground(ctx context.Context, id string, f Fix, updatedBy string 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{}, 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. +// 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 err != nil { + 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 } @@ -486,10 +496,14 @@ func (s *Service) groundManual(ctx context.Context, id string, m ManualRecipe, u 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) diff --git a/internal/dinners/grounding_test.go b/internal/dinners/grounding_test.go index 1f837a7..733f245 100644 --- a/internal/dinners/grounding_test.go +++ b/internal/dinners/grounding_test.go @@ -1126,6 +1126,13 @@ func TestFixInFlightWhenDinnerChanges(t *testing.T) { }, 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) @@ -1170,6 +1177,40 @@ func TestFixInFlightWhenDinnerChanges(t *testing.T) { } } +// TestFixInFlightKeepsMemberEdits: a method or difficulty a member sets +// while a link is grounding is kept; what they left alone takes the link's +// recipe. +func TestFixInFlightKeepsMemberEdits(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) + if _, err := e.svc.Update(t.Context(), id, dinners.Patch{Method: ptr("grill"), Difficulty: ptr("easy")}, 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.Errorf("dinner = %+v, want grounded in the link's recipe", row) + } + if row.Method.String != "grill" || row.Difficulty.String != "easy" || row.MinutesTotal.Int64 != 150 { + t.Errorf("method, difficulty, minutes = %q, %q, %d; want grill and easy kept, 150 from the recipe", + row.Method.String, row.Difficulty.String, row.MinutesTotal.Int64) + } +} + // TestFixArchivedDinner: an archived dinner cannot be grounded by hand. func TestFixArchivedDinner(t *testing.T) { t.Parallel() diff --git a/internal/server/grounding_test.go b/internal/server/grounding_test.go index 9d3747d..ed02531 100644 --- a/internal/server/grounding_test.go +++ b/internal/server/grounding_test.go @@ -252,6 +252,33 @@ func TestPostGroundViaRecipeURLStorageFailure(t *testing.T) { } } +// 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) + } +} + // TestPostGroundArchived: grounding an archived dinner by hand is a 409. func TestPostGroundArchived(t *testing.T) { t.Parallel() diff --git a/migrations/queries/dinners.sql b/migrations/queries/dinners.sql index 93ed6e3..43e96ae 100644 --- a/migrations/queries/dinners.sql +++ b/migrations/queries/dinners.sql @@ -85,23 +85,28 @@ RETURNING *; -- 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. It writes only an unarchived dinner still as it was observed --- before grounding (same normalized_name, grounding_status, +-- 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. +-- 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 = sqlc.arg(recipe_id), recipe_url = NULL, photo_id = sqlc.arg(photo_id), - method = sqlc.arg(method), - minutes_total = sqlc.arg(minutes_total), - difficulty = sqlc.arg(difficulty), + 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 = 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)