diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 3a7765c..b2182e7 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -213,10 +213,15 @@ 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`. - `source` is `{url, site_name}` of the page the recipe was fetched from, - null for a manual recipe or none; `why_picked` is the reason of the - suggestion that created the dinner (its earliest), null for one no - suggestion created. An unknown or archived dinner is 404. + 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 (`site_name` null when the page names + none), null for a manual recipe or none; it has no `url`, as the page's + link may carry a personal token. `why_picked` is the reason of the + suggestion that created the dinner — its pending or accepted one, as + declining a suggestion archives its dinner — null for one no suggestion + created; a dinner with more than one such suggestion is 500, as nothing + records which created it. An unknown or archived dinner is 404. - Dinner grounding. `POST /api/v1/dinners` also enqueues a `ground_dinner` job and answers with its `grounding_job_id` without waiting for it (`GET /api/v1/jobs/{id}` reports it); if the job cannot be enqueued it diff --git a/internal/db/querier.go b/internal/db/querier.go index ce49c12..66b6610 100644 --- a/internal/db/querier.go +++ b/internal/db/querier.go @@ -103,9 +103,13 @@ type Querier interface { // Returns sql.ErrNoRows until the row exists. GetChatSettings(ctx context.Context) (ChatSetting, error) GetDinner(ctx context.Context, id string) (Dinner, error) - // The reason of the suggestion that created dinner_id, its earliest. - // Returns sql.ErrNoRows for a dinner no suggestion created. - GetDinnerSuggestionReason(ctx context.Context, dinnerID string) (string, error) + // The reason of each suggestion of dinner_id that can have created it, + // none for a dinner no suggestion created. A suggestion is only ever + // created with a new dinner, and declining one archives its dinner, so a + // live dinner's creating suggestion is pending (the dinner still + // suggested) or accepted; a declined one did not create it. More than one + // row means nothing records which did. + GetDinnerSuggestionReasons(ctx context.Context, dinnerID string) ([]string, error) // Returns sql.ErrNoRows until the row exists. GetHouseholdSettings(ctx context.Context) (HouseholdSetting, error) // Returns sql.ErrNoRows until the row exists. diff --git a/internal/db/suggestions.sql.go b/internal/db/suggestions.sql.go index 68430df..879e606 100644 --- a/internal/db/suggestions.sql.go +++ b/internal/db/suggestions.sql.go @@ -187,20 +187,39 @@ func (q *Queries) GetActiveSuggestionBatch(ctx context.Context) (SuggestionBatch return i, err } -const getDinnerSuggestionReason = `-- name: GetDinnerSuggestionReason :one +const getDinnerSuggestionReasons = `-- name: GetDinnerSuggestionReasons :many SELECT reason FROM suggestions -WHERE dinner_id = ? -ORDER BY created_at, id -LIMIT 1 +WHERE dinner_id = ? AND status IN ('pending', 'accepted') +ORDER BY id ` -// The reason of the suggestion that created dinner_id, its earliest. -// Returns sql.ErrNoRows for a dinner no suggestion created. -func (q *Queries) GetDinnerSuggestionReason(ctx context.Context, dinnerID string) (string, error) { - row := q.db.QueryRowContext(ctx, getDinnerSuggestionReason, dinnerID) - var reason string - err := row.Scan(&reason) - return reason, err +// The reason of each suggestion of dinner_id that can have created it, +// none for a dinner no suggestion created. A suggestion is only ever +// created with a new dinner, and declining one archives its dinner, so a +// live dinner's creating suggestion is pending (the dinner still +// suggested) or accepted; a declined one did not create it. More than one +// row means nothing records which did. +func (q *Queries) GetDinnerSuggestionReasons(ctx context.Context, dinnerID string) ([]string, error) { + rows, err := q.db.QueryContext(ctx, getDinnerSuggestionReasons, dinnerID) + if err != nil { + return nil, err + } + defer rows.Close() + var items []string + for rows.Next() { + var reason string + if err := rows.Scan(&reason); err != nil { + return nil, err + } + items = append(items, reason) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil } const getLatestSuggestionBatch = `-- name: GetLatestSuggestionBatch :one diff --git a/internal/dinners/detail.go b/internal/dinners/detail.go index 47eb043..1d7b726 100644 --- a/internal/dinners/detail.go +++ b/internal/dinners/detail.go @@ -30,15 +30,16 @@ type Detail struct { // the dinner has no recipe. Ingredients []Ingredient `json:"ingredients"` Steps []string `json:"steps"` - // Source is the recipe page the dinner is grounded in; null for a - // recipe entered by hand or a dinner with no recipe. + // Source is the site of the recipe page the dinner is grounded in; + // null for a recipe entered by hand or a dinner with no recipe. Source *Source `json:"source"` GroundingStatus string `json:"grounding_status"` // GroundingError is why grounding failed, one of the Reason codes; // null unless GroundingStatus is failed. GroundingError *string `json:"grounding_error"` - // WhyPicked is the reason of the suggestion that created the dinner; - // null for a dinner no suggestion created. + // WhyPicked is the reason of the suggestion that created the dinner + // (see GetDinnerSuggestionReasons); null for a dinner no suggestion + // created. WhyPicked *string `json:"why_picked"` } @@ -54,9 +55,12 @@ type Ingredient struct { RawText string `json:"raw_text"` } -// Source is the page a recipe was fetched from. +// Source is the site a recipe was fetched from. It has no url: the page's +// link, pasted or offered as a candidate, may carry a personal token, and +// no stored link can be shown to be free of one, so like a dinner's +// recipe_url and a candidate's page it never leaves the server. SiteName +// is null when the page names no site. type Source struct { - URL string `json:"url"` SiteName *string `json:"site_name"` } @@ -113,13 +117,19 @@ func (s *Service) Get(ctx context.Context, id string) (d Detail, err error) { return Detail{}, fmt.Errorf("dinners: read recipe of %s: %w", id, err) } } - reason, err := q.GetDinnerSuggestionReason(ctx, id) - switch { - case err == nil: - d.WhyPicked = &reason - case !errors.Is(err, sql.ErrNoRows): + reasons, err := q.GetDinnerSuggestionReasons(ctx, id) + if err != nil { return Detail{}, fmt.Errorf("dinners: read suggestion reason of %s: %w", id, err) } + switch len(reasons) { + case 0: + case 1: + d.WhyPicked = &reasons[0] + default: + // Nothing records which of them created the dinner, and any + // one picked would be a guess. + return Detail{}, fmt.Errorf("dinners: %s has %d suggestions that could have created it", id, len(reasons)) + } return d, nil } @@ -130,13 +140,16 @@ func readRecipe(ctx context.Context, q *db.Queries, recipeID string, d *Detail) return fmt.Errorf("recipe %s: %w", recipeID, err) } if !IsManualRecipe(rec.SourceUrl) { - d.Source = &Source{URL: rec.SourceUrl, SiteName: nullString(rec.SiteName)} + d.Source = &Source{SiteName: nullString(rec.SiteName)} } ings, err := q.ListRecipeIngredients(ctx, recipeID) if err != nil { return fmt.Errorf("ingredients: %w", err) } - for _, ing := range ings { + for i, ing := range ings { + if i > 0 && ing.Position == ings[i-1].Position { + return duplicatePosition("ingredient", ing.Position) + } d.Ingredients = append(d.Ingredients, Ingredient{ Name: nullString(ing.Name), Qty: nullString(ing.Qty), @@ -149,8 +162,19 @@ func readRecipe(ctx context.Context, q *db.Queries, recipeID string, d *Detail) if err != nil { return fmt.Errorf("steps: %w", err) } - for _, step := range steps { + for i, step := range steps { + if i > 0 && step.Position == steps[i-1].Position { + return duplicatePosition("step", step.Position) + } d.Steps = append(d.Steps, step.Text) } return nil } + +// 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 06c860e..95c635a 100644 --- a/internal/server/dinnerdetail_test.go +++ b/internal/server/dinnerdetail_test.go @@ -1,10 +1,12 @@ package server import ( + "database/sql" "encoding/json" "net/http" "net/http/httptest" "reflect" + "strings" "testing" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/db" @@ -42,7 +44,7 @@ func wantNull(t *testing.T, raw map[string]any, keys ...string) { // TestGetDinnerGroundedSideQuest: a Side quest grounded in a fetched page // shows its photo at both stored sizes, its recipe's ingredients and steps -// in recipe order (not row order), its source page and the reason it was +// in recipe order (not row order), its source site and the reason it was // suggested. func TestGetDinnerGroundedSideQuest(t *testing.T) { t.Parallel() @@ -109,7 +111,7 @@ func TestGetDinnerGroundedSideQuest(t *testing.T) { {Name: str("onion"), Qty: str("1"), RawText: "1 onion"}, }, Steps: []string{"Fry with the garlic.", "Chop the onion."}, - Source: &dinners.Source{URL: curryPage, SiteName: str("Curry House")}, + Source: &dinners.Source{SiteName: str("Curry House")}, GroundingStatus: "grounded", WhyPicked: str(reason), } @@ -213,3 +215,116 @@ func TestGetDinnerNotFound(t *testing.T) { t.Errorf("GET without a session = %d %v, want 401", rec.Code, body) } } + +// TestGetDinnerSourceHidesTokenURL: a recipe grounded in a pasted link that +// carries a token in its path or query shows its site name, but never the +// link or any part of it. +func TestGetDinnerSourceHidesTokenURL(t *testing.T) { + t.Parallel() + e := newDinnersEnv(t) + const ( + pathToken = "path-token-abc123" + queryToken = "query-token-def456" + pathPage = "https://secret-path.example/u/" + pathToken + "/chicken-curry" + queryPage = "https://secret-query.example/curry?access_token=" + queryToken + ) + e.pages.Bodies[pathPage] = recipestest.RecipePage("Chicken Curry", "Curry House", "https://a.example/curry.jpg", 40) + e.pages.Bodies[queryPage] = recipestest.RecipePage("Weeknight Curry", "B Kitchen", "https://b.example/curry.jpg", 25) + e.images.JPEGs["https://a.example/curry.jpg"] = recipestest.JPEG(t, 400, 320, 1) + e.images.JPEGs["https://b.example/curry.jpg"] = recipestest.JPEG(t, 400, 320, 2) + for _, tc := range []struct{ dish, page, site string }{ + {"Chicken curry", pathPage, "Curry House"}, + {"Weeknight curry", queryPage, "B Kitchen"}, + } { + id := createDinner(t, e.h, `{"name":"`+tc.dish+`"}`) // runner not started: stays pending + e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 25)) + if rec, _ := callDinners(t, e.h, http.MethodPost, dinnersPath+"/"+id+"/ground", `{"recipe_url":"`+tc.page+`"}`); rec.Code != http.StatusOK { + t.Fatalf("POST ground %s = %d: %s", tc.page, rec.Code, rec.Body) + } + var stored string + if err := e.st.Reader.QueryRow(`SELECT source_url FROM recipes WHERE id = (SELECT recipe_id FROM dinners WHERE id = ?)`, id).Scan(&stored); err != nil { + t.Fatal(err) + } + if stored != tc.page { + t.Fatalf("stored source_url = %q, want %q", stored, tc.page) + } + + rec, _ := callDinners(t, e.h, http.MethodGet, dinnersPath+"/"+id, "") + if rec.Code != http.StatusOK { + t.Fatalf("GET dinner = %d: %s", rec.Code, rec.Body) + } + for _, secret := range []string{tc.page, "secret-path.example", "secret-query.example", pathToken, queryToken, "access_token", `"url"`} { + if strings.Contains(rec.Body.String(), secret) { + t.Errorf("detail of %s contains %q: %s", tc.dish, secret, rec.Body) + } + } + raw, _ := getDetail(t, e.h, id) + if src, _ := raw["source"].(map[string]any); !reflect.DeepEqual(src, map[string]any{"site_name": tc.site}) { + t.Errorf("source of %s = %v, want only site_name %q", tc.dish, raw["source"], tc.site) + } + } +} + +// TestGetDinnerWhyPickedIsCreatingSuggestion: why_picked is the reason of +// the suggestion that created the dinner, never that of an earlier +// suggestion row for it that was declined. +func TestGetDinnerWhyPickedIsCreatingSuggestion(t *testing.T) { + t.Parallel() + e := newDinnersEnv(t) + id := createDinner(t, e.h, `{"name":"Chicken curry"}`) + if _, err := e.st.Writer.Exec(`UPDATE dinners SET status = 'quest_log', origin = 'side_quest' WHERE id = ?`, id); err != nil { + t.Fatal(err) + } + q := db.New(e.st.Writer) + now := store.Now().String() + batch, err := q.CreateSuggestionBatch(t.Context(), db.CreateSuggestionBatchParams{ID: store.NewID(), Trigger: "manual", CreatedAt: now, UpdatedAt: now}) + if err != nil { + t.Fatal(err) + } + const reason = "Like your Tikka masala, but quicker." + // The declined row is created first, so it is the earliest by both + // created_at and id. + for _, s := range []struct{ reason, status string }{ + {"Passed on last week.", "declined"}, + {reason, "accepted"}, + } { + sid := store.NewID() + if err := q.CreateSuggestion(t.Context(), db.CreateSuggestionParams{ + ID: sid, DinnerID: id, BatchID: batch.ID, Name: "Chicken curry", + Reason: s.reason, CitedRegularIds: "[]", CreatedAt: now, UpdatedAt: now, + }); err != nil { + t.Fatal(err) + } + if n, err := q.DecideSuggestion(t.Context(), db.DecideSuggestionParams{ + Status: s.status, Now: sql.NullString{String: now, Valid: true}, ID: sid, + }); err != nil || n != 1 { + t.Fatalf("decide %s = %d, %v", s.status, n, err) + } + } + + if _, got := getDetail(t, e.h, id); got.WhyPicked == nil || *got.WhyPicked != reason { + 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/migrations/queries/suggestions.sql b/migrations/queries/suggestions.sql index a68cb6a..61aef9b 100644 --- a/migrations/queries/suggestions.sql +++ b/migrations/queries/suggestions.sql @@ -117,10 +117,13 @@ UPDATE dinners SET status = sqlc.arg(status), updated_by = sqlc.arg(updated_by), updated_at = sqlc.arg(updated_at) WHERE id = sqlc.arg(id) AND status = 'suggested'; --- name: GetDinnerSuggestionReason :one --- The reason of the suggestion that created dinner_id, its earliest. --- Returns sql.ErrNoRows for a dinner no suggestion created. +-- name: GetDinnerSuggestionReasons :many +-- The reason of each suggestion of dinner_id that can have created it, +-- none for a dinner no suggestion created. A suggestion is only ever +-- created with a new dinner, and declining one archives its dinner, so a +-- live dinner's creating suggestion is pending (the dinner still +-- suggested) or accepted; a declined one did not create it. More than one +-- row means nothing records which did. SELECT reason FROM suggestions -WHERE dinner_id = ? -ORDER BY created_at, id -LIMIT 1; +WHERE dinner_id = ? AND status IN ('pending', 'accepted') +ORDER BY id;