Integration fix 3: keep recipe_url out of dinner responses (Feature 12 review F1) (#97)

Co-authored-by: orchestrator-agent <20+orchestrator-agent@code.cubecraftcreations.com>

internal/dinners/dinners.go

@@ -16,7 +16,6 @@ import (
 	"slices"
 	"strings"
 	"sync"
-	"unicode"
 	"unicode/utf8"
 
 	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/harness"
@@ -55,7 +54,9 @@ var listable = []string{StatusRegular, StatusQuestLog}
 // ErrNotFound is returned for an unknown dinner id.
 var ErrNotFound = errors.New("dinner not found")
 
-// Dinner is a dinner as the API shows it.
+// Dinner is a dinner as the API shows it. It has no recipe_url: a pasted
+// link may carry a personal token, so it stays in the database for
+// grounding and never leaves the server.
 type Dinner struct {
 	ID              string  `json:"id"`
 	Name            string  `json:"name"`
@@ -67,7 +68,6 @@ type Dinner struct {
 	Cuisine         *string `json:"cuisine"`
 	IsFavorite      bool    `json:"is_favorite"`
 	RecipeID        *string `json:"recipe_id"`
-	RecipeURL       *string `json:"recipe_url"`
 	PhotoURL        *string `json:"photo_url"`
 	GroundingStatus string  `json:"grounding_status"`
 	// GroundingError is why grounding failed, one of the Reason codes;
@@ -78,7 +78,10 @@ type Dinner struct {
 	UpdatedAt      string  `json:"updated_at"`
 }
 
-// NewRegular is a Regular to add: exactly one of Name and RecipeURL.
+// NewRegular is a Regular to add: its Name, which the household always
+// gives, and optionally the RecipeURL to ground it in. The name is never
+// taken from the link: a pasted link may carry a personal token anywhere in
+// it, and the name is shown in every dinner response.
 type NewRegular struct {
 	Name      *string
 	RecipeURL *string
@@ -170,10 +173,9 @@ type Created struct {
 	GroundingJobID *string `json:"grounding_job_id"`
 }
 
-// CreateRegular adds a Regular named by r.Name, or by the last path segment
-// of r.RecipeURL (whose recipe grounding later replaces), with status and
-// origin regular and grounding_status pending, recording updatedBy, and
-// enqueues the KindGroundDinner job that grounds it. It returns without
+// CreateRegular adds a Regular named r.Name, keeping r.RecipeURL for
+// grounding when given, with status and origin regular and grounding_status
+// pending, recording updatedBy, and enqueues the KindGroundDinner job that grounds it. It returns without
 // waiting for the job. A name repeating an unarchived dinner is a
 // *DuplicateError. When the job cannot be enqueued, the stored dinner is
 // returned with grounding_status failed (ReasonGroundingError) and no job
@@ -223,34 +225,24 @@ func (s *Service) createRegular(ctx context.Context, r NewRegular, updatedBy str
 	if updatedBy == "" {
 		return db.Dinner{}, errors.New("dinners: create: no member to record as updated_by")
 	}
-	var name string
+	if r.Name == nil {
+		return db.Dinner{}, invalid("name", "give the dinner's name, with or without a recipe_url")
+	}
+	name, err := checkName(*r.Name)
+	if err != nil {
+		return db.Dinner{}, err
+	}
 	var recipeURL sql.NullString
-	switch {
-	case r.Name != nil && r.RecipeURL != nil:
-		return db.Dinner{}, invalid("name", "give a name or a recipe_url, not both")
-	case r.Name != nil:
-		n, err := checkName(*r.Name)
-		if err != nil {
-			return db.Dinner{}, err
-		}
-		name = n
-	case r.RecipeURL != nil:
+	if r.RecipeURL != nil {
 		u, err := checkRecipeURL(*r.RecipeURL)
 		if err != nil {
 			return db.Dinner{}, err
 		}
-		n, ok := nameFromURL(u)
-		if !ok {
-			return db.Dinner{}, invalid("recipe_url", "the link does not name a dinner; add it by name instead")
-		}
-		name = n
 		recipeURL = sql.NullString{String: u.String(), Valid: true}
-	default:
-		return db.Dinner{}, invalid("name", "give a name or a recipe_url")
 	}
 
 	var row db.Dinner
-	err := s.st.WithTx(ctx, func(tx *sql.Tx) error {
+	err = s.st.WithTx(ctx, func(tx *sql.Tx) error {
 		q := db.New(tx)
 		if err := checkUnique(ctx, q, "", name, recipeURL); err != nil {
 			return err
@@ -495,36 +487,6 @@ func checkRecipeURL(raw string) (*url.URL, error) {
 	return u, nil
 }
 
-// nameFromURL names a dinner from the last path segment of u: separators
-// become spaces, a file extension and words of only digits are dropped and
-// the first letter is capitalized, so
-// https://example.com/recipes/12345/chicken-tikka-masala/ is
-// "Chicken tikka masala". ok is false when no usable name is left.
-func nameFromURL(u *url.URL) (name string, ok bool) {
-	segments := strings.FieldsFunc(u.Path, func(r rune) bool { return r == '/' })
-	if len(segments) == 0 {
-		return "", false
-	}
-	slug := segments[len(segments)-1]
-	if i := strings.LastIndexByte(slug, '.'); i > 0 {
-		slug = slug[:i]
-	}
-	var words []string
-	for _, w := range strings.FieldsFunc(slug, func(r rune) bool { return r == '-' || r == '_' || r == '+' || unicode.IsSpace(r) }) {
-		if strings.IndexFunc(w, func(r rune) bool { return !unicode.IsDigit(r) }) >= 0 {
-			words = append(words, strings.ToLower(w))
-		}
-	}
-	name = strings.Join(words, " ")
-	if r, size := utf8.DecodeRuneInString(name); r != utf8.RuneError {
-		name = string(unicode.ToUpper(r)) + name[size:]
-	}
-	if _, err := checkName(name); err != nil {
-		return "", false
-	}
-	return name, true
-}
-
 // checkUnique returns a *DuplicateError when name is harness.Similar to an
 // unarchived dinner other than self, or recipeURL is one's recipe link: the
 // source_url of the recipe it is grounded in, else the recipe_url still
@@ -565,7 +527,6 @@ func fromRow(r db.Dinner) Dinner {
 		Cuisine:         nullString(r.Cuisine),
 		IsFavorite:      r.IsFavorite == 1,
 		RecipeID:        nullString(r.RecipeID),
-		RecipeURL:       nullString(r.RecipeUrl),
 		GroundingStatus: r.GroundingStatus,
 		GroundingError:  nullString(r.GroundingError),
 		UpdatedBy:       nullString(r.UpdatedBy),

internal/dinners/dinners_test.go

@@ -1,33 +1,74 @@
-package dinners
+package dinners_test
 
 import (
-	"net/url"
+	"encoding/json"
+	"errors"
+	"strings"
 	"testing"
+
+	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/dinners"
 )
 
-func TestNameFromURL(t *testing.T) {
+// TestCreateRegularNeverNamesFromLink: a Regular added with a recipe link
+// is named only by the household, so no part of the link — host, path,
+// query or fragment — is stored as its name or shown in a dinner or a
+// duplicate error, even a token of plain letters in the path.
+func TestCreateRegularNeverNamesFromLink(t *testing.T) {
 	t.Parallel()
-	tests := []struct {
-		link string
-		want string // "" means no usable name
-	}{
-		{"https://example.com/recipes/12345/chicken-tikka-masala/", "Chicken tikka masala"},
-		{"https://example.com/recipe/beef_stew.html", "Beef stew"},
-		{"https://example.com/2024/05/best-ever-chili-123", "Best ever chili"},
-		{"https://example.com/crème-brûlée", "Crème brûlée"},
-		{"https://example.com/recipes/chili?utm=1#notes", "Chili"},
-		{"https://example.com/", ""},
-		{"https://example.com/recipe/98765", ""},
-		{"https://example.com/the-best-easy-recipe", ""},
-	}
-	for _, tt := range tests {
-		u, err := url.Parse(tt.link)
-		if err != nil {
-			t.Fatal(err)
-		}
-		got, ok := nameFromURL(u)
-		if got != tt.want || ok != (tt.want != "") {
-			t.Errorf("nameFromURL(%s) = %q, %v; want %q", tt.link, got, ok, tt.want)
+	e := newUnstartedEnv(t)
+	const link = "https://c.example/stew-key-abcdefghijklmno?sig=zyxwvut#frag"
+	leaks := []string{"c.example", "key", "abcdefghijklmno", "sig", "zyxwvut", "frag"}
+	check := func(what, text string) {
+		t.Helper()
+		lower := strings.ToLower(text)
+		for _, leak := range leaks {
+			if strings.Contains(lower, leak) {
+				t.Errorf("%s contains %q: %s", what, leak, text)
+			}
 		}
 	}
+
+	_, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(link)}, member)
+	var verr *dinners.ValidationError
+	if !errors.As(err, &verr) {
+		t.Fatalf("link without a name: err = %v, want a *ValidationError", err)
+	}
+	check("validation error", verr.Error())
+
+	c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr(" Sunday stew "), RecipeURL: ptr(link)}, member)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if c.Name != "Sunday stew" {
+		t.Errorf("name = %q, want the household's %q", c.Name, "Sunday stew")
+	}
+	created, err := json.Marshal(c)
+	if err != nil {
+		t.Fatal(err)
+	}
+	check("create response", string(created))
+	row := e.dinner(t, c.ID)
+	check("stored name", row.Name+" "+row.NormalizedName)
+	if row.RecipeUrl.String != link {
+		t.Errorf("stored recipe_url = %q, want %q kept for grounding", row.RecipeUrl.String, link)
+	}
+
+	list, err := e.svc.List(t.Context(), dinners.StatusRegular)
+	if err != nil {
+		t.Fatal(err)
+	}
+	listed, err := json.Marshal(list)
+	if err != nil {
+		t.Fatal(err)
+	}
+	check("list response", string(listed))
+
+	// The same link under another name is still a duplicate, and its error
+	// names the existing dinner by the household's name.
+	_, err = e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Weeknight chili"), RecipeURL: ptr(link)}, member)
+	var dup *dinners.DuplicateError
+	if !errors.As(err, &dup) || dup.Existing.ID != c.ID {
+		t.Fatalf("same link again: err = %v, want a duplicate of %s", err, c.ID)
+	}
+	check("duplicate error", dup.Error())
 }

internal/dinners/grounding_test.go

@@ -194,7 +194,7 @@ func TestCreateByRecipeURLGroundsThatURL(t *testing.T) {
 	e := newEnv(t)
 	e.model.QueueGenerate(recipestest.NormalizedReply("oven", 150))
 
-	c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(stew)}, member)
+	c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Beef stew"), RecipeURL: ptr(stew)}, member)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -223,7 +223,7 @@ func TestCreateGroundingFailureRecordsReason(t *testing.T) {
 		reason string
 	}{
 		{"no candidates", dinners.NewRegular{Name: ptr("Unknown dish")}, nil, dinners.ReasonNoRecipe},
-		{"unusable link", dinners.NewRegular{RecipeURL: ptr("https://gone.example/recipes/lost-soup")}, nil, dinners.ReasonRecipeUnusable},
+		{"unusable link", dinners.NewRegular{Name: ptr("Lost soup"), RecipeURL: ptr("https://gone.example/recipes/lost-soup")}, nil, dinners.ReasonRecipeUnusable},
 		{"search fails", dinners.NewRegular{Name: ptr("Chicken curry")}, func(e *env) { e.search.Err = recipes.ErrSearchRateLimited }, dinners.ReasonSearchFailed},
 		{"model fails", dinners.NewRegular{Name: ptr("Chicken curry")}, func(e *env) {
 			// One provider failure per candidate tried.
@@ -375,7 +375,7 @@ func TestRenameLinkWhilePendingGroundsNewName(t *testing.T) {
 	e.search.Results["Beef stew"] = []recipes.Candidate{{URL: stew}}
 	e.model.QueueGenerate(recipestest.NormalizedReply("oven", 150))
 
-	c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(curryA)}, member)
+	c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Chicken curry"), RecipeURL: ptr(curryA)}, member)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -383,8 +383,11 @@ func TestRenameLinkWhilePendingGroundsNewName(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if d.GroundingStatus != dinners.GroundingPending || d.RecipeURL != nil {
-		t.Fatalf("renamed = %+v, want still pending with no recipe_url", d)
+	if d.GroundingStatus != dinners.GroundingPending {
+		t.Fatalf("renamed = %+v, want still pending", d)
+	}
+	if row, err := db.New(e.st.Reader).GetDinner(t.Context(), c.ID); err != nil || row.RecipeUrl.Valid {
+		t.Fatalf("renamed row = %+v (err %v), want no recipe_url", row, err)
 	}
 	e.start(t)
 
@@ -737,7 +740,7 @@ func TestFixReplacesPendingLink(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
 			e := newEnv(t)
-			c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(lost)}, member)
+			c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Lamb roast"), RecipeURL: ptr(lost)}, member)
 			if err != nil {
 				t.Fatal(err)
 			}
@@ -749,19 +752,22 @@ func TestFixReplacesPendingLink(t *testing.T) {
 				t.Fatal(err)
 			}
 			e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 25))
-			d, err := e.svc.Ground(t.Context(), c.ID, tt.fix, member)
+			if _, err := e.svc.Ground(t.Context(), c.ID, tt.fix, member); err != nil {
+				t.Fatal(err)
+			}
+			row, err := db.New(e.st.Reader).GetDinner(t.Context(), c.ID)
 			if err != nil {
 				t.Fatal(err)
 			}
-			if d.RecipeURL != nil {
-				t.Errorf("recipe_url = %q after the fix, want the unusable link dropped", *d.RecipeURL)
+			if row.RecipeUrl.Valid {
+				t.Errorf("recipe_url = %q after the fix, want the unusable link dropped", row.RecipeUrl.String)
 			}
 
-			if _, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(lost)}, member); err != nil {
+			if _, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Lentil soup"), RecipeURL: ptr(lost)}, member); err != nil {
 				t.Errorf("adding the dropped link again: %v, want added", err)
 			}
 			if tt.linkTaken != "" {
-				_, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(tt.linkTaken)}, member)
+				_, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Curry night"), RecipeURL: ptr(tt.linkTaken)}, member)
 				var dup *dinners.DuplicateError
 				if !errors.As(err, &dup) || dup.Existing.ID != c.ID {
 					t.Errorf("adding the fixed dinner's recipe link: err = %v, want a duplicate of %s", err, c.ID)