From 35138dce0b118277032a7ec06ecda078d0b1ab39 Mon Sep 17 00:00:00 2001 From: Julian Dice <19397727+windoze95@users.noreply.github.com> Date: Tue, 7 Jul 2026 20:39:11 -0500 Subject: [PATCH] fix(recipes): make URL import idempotent per-user (no duplicate saves) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Source-URL uniqueness was enforced only on the extraction cache (canonical_recipes.normalized_url), NOT on a user's collection — so saving the same recipe twice (e.g. an MCP agent calling save_recipe more than once) created duplicate recipe rows pointing at the same canonical. There was no per-user dedup anywhere: ImportFromURL -> createImportedRecipe -> CreateRecipe is a plain insert. - Add RecipeRepo.GetUserRecipeByCanonical(userID, canonicalID) (repo + mock). - In createImportedRecipe, when canonicalID != nil (URL imports), return the user's existing recipe for that canonical instead of inserting a duplicate. Vision/copypasta/video imports (nil canonical) and fork/regen/generate are unaffected — they legitimately create new rows. - Mock CreateRecipe now populates CreatedByID from the association (mirrors GORM) so lookups behave like the DB. - Dedup-lookup failure logs and proceeds (never blocks an import). A partial unique index on (created_by_id, canonical_id) is a sensible follow-up once existing dupes are cleaned. Test: import same canonical twice -> 1 row + same id returned; different canonical -> new row. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/repository/interfaces.go | 5 +++ internal/repository/recipe.go | 23 +++++++++++++ internal/service/import.go | 15 +++++++++ internal/service/import_service_test.go | 45 +++++++++++++++++++++++++ internal/testutil/mocks.go | 19 +++++++++++ 5 files changed, 107 insertions(+) diff --git a/internal/repository/interfaces.go b/internal/repository/interfaces.go index 550e727..c45f8a2 100644 --- a/internal/repository/interfaces.go +++ b/internal/repository/interfaces.go @@ -11,6 +11,11 @@ import ( type RecipeRepo interface { GetUserRecipes(userID uint, page, pageSize int) ([]models.Recipe, int64, error) GetRecipeByID(recipeID uint) (*models.Recipe, error) + // GetUserRecipeByCanonical returns the user's existing non-deleted recipe for + // the given canonical entry, or (nil, nil) if none. Used to keep URL imports + // idempotent per-user (saving the same recipe twice returns the original + // instead of creating a duplicate). + GetUserRecipeByCanonical(userID, canonicalID uint) (*models.Recipe, error) CreateRecipe(recipe *models.Recipe) error DeleteRecipe(recipeID uint) error UpdateRecipeTitle(recipe *models.Recipe, title string) error diff --git a/internal/repository/recipe.go b/internal/repository/recipe.go index 3ff2aaa..73d1a87 100755 --- a/internal/repository/recipe.go +++ b/internal/repository/recipe.go @@ -79,6 +79,29 @@ func (r *RecipeRepository) GetRecipeByID(recipeID uint) (*models.Recipe, error) return &recipe, nil } +// GetUserRecipeByCanonical returns the user's oldest non-deleted recipe for the +// given canonical entry, or (nil, nil) if none. Preloads the same associations as +// GetRecipeByID so the result can be passed straight to ToRecipeResponse. Used to +// make URL imports idempotent per-user (no duplicate rows on a repeat save). +func (r *RecipeRepository) GetUserRecipeByCanonical(userID, canonicalID uint) (*models.Recipe, error) { + var recipe models.Recipe + err := r.DB.Preload("Hashtags"). + Preload("Canonical"). + Preload("CreatedBy", func(db *gorm.DB) *gorm.DB { + return db.Select("ID", "Username") + }). + Where("created_by_id = ? AND canonical_id = ?", userID, canonicalID). + Order("created_at ASC"). + First(&recipe).Error + if err != nil { + if errors.Is(err, gorm.ErrRecordNotFound) { + return nil, nil + } + return nil, err + } + return &recipe, nil +} + // CreateRecipe creates a new recipe. func (r *RecipeRepository) CreateRecipe(recipe *models.Recipe) error { // Start a new transaction diff --git a/internal/service/import.go b/internal/service/import.go index edfb02c..93d442a 100644 --- a/internal/service/import.go +++ b/internal/service/import.go @@ -1222,6 +1222,21 @@ func (s *ImportService) createImportedRecipe(ctx context.Context, recipeDef *mod return nil, 0, fmt.Errorf("recipe title is required") } + // Idempotent URL import: if this user already saved this canonical recipe, + // return the existing one instead of creating a duplicate. URL imports carry a + // canonicalID; vision/copypasta/video imports pass nil (each is a distinct + // capture) and fork/regen/generate don't come through here — so this only + // dedupes true URL re-saves (e.g. an agent calling save_recipe twice). + if canonicalID != nil { + if existing, err := s.RecipeRepo.GetUserRecipeByCanonical(user.ID, *canonicalID); err != nil { + log.Warn("dedup lookup failed; proceeding with import", zap.Error(err)) + } else if existing != nil { + log.Info("import matches an existing saved recipe; returning it (no duplicate)", + zap.Uint("recipe_id", existing.ID), zap.Uint("canonical_id", *canonicalID)) + return s.RecipeService.ToRecipeResponse(existing), existing.ID, nil + } + } + // Idempotent safety net: ensure every imported recipe carries normalized // measurement fields even if it reached here without going through a // builder (e.g. the manual path). Detach the ingredient slice first so a diff --git a/internal/service/import_service_test.go b/internal/service/import_service_test.go index 0f010d7..19c6ab8 100644 --- a/internal/service/import_service_test.go +++ b/internal/service/import_service_test.go @@ -78,6 +78,51 @@ func TestImportFromText_Success(t *testing.T) { } } +// TestImportIdempotent_SameCanonicalNoDuplicate guards the per-user save-dedup: +// saving the same URL/canonical twice must return the existing recipe, not create +// a second row (e.g. an agent calling save_recipe more than once). +func TestImportIdempotent_SameCanonicalNoDuplicate(t *testing.T) { + repo := testutil.NewMockRecipeRepo() + svc := newTestImportService(repo, nil, nil) + user := testutil.TestUser() + + def := &models.RecipeDef{ + Title: "One Pan Salmon Dinner", + Ingredients: models.Ingredients{{Name: "salmon"}, {Name: "potatoes"}}, + Instructions: []string{"Roast everything on a sheet pan."}, + } + canonicalID := uint(4242) + + resp1, id1, err := svc.createImportedRecipe(context.Background(), def, user, models.RecipeTypeImportLink, "https://example.com/salmon", "", &canonicalID, nil, "") + if err != nil { + t.Fatalf("first import error: %v", err) + } + if len(repo.Recipes) != 1 { + t.Fatalf("after first import, recipes = %d, want 1", len(repo.Recipes)) + } + + // Same canonical again → existing recipe returned, no new row. + resp2, id2, err := svc.createImportedRecipe(context.Background(), def, user, models.RecipeTypeImportLink, "https://example.com/salmon", "", &canonicalID, nil, "") + if err != nil { + t.Fatalf("second import error: %v", err) + } + if len(repo.Recipes) != 1 { + t.Errorf("after second import, recipes = %d, want 1 (no duplicate)", len(repo.Recipes)) + } + if id2 != id1 || resp2.ID != resp1.ID { + t.Errorf("second import returned id %d / %q, want the existing %d / %q", id2, resp2.ID, id1, resp1.ID) + } + + // A different canonical for the same user still creates a new recipe. + otherCanonical := uint(9999) + if _, _, err := svc.createImportedRecipe(context.Background(), def, user, models.RecipeTypeImportLink, "https://example.com/other", "", &otherCanonical, nil, ""); err != nil { + t.Fatalf("third import error: %v", err) + } + if len(repo.Recipes) != 2 { + t.Errorf("after a different canonical, recipes = %d, want 2", len(repo.Recipes)) + } +} + func TestImportFromText_MetricUser(t *testing.T) { repo := testutil.NewMockRecipeRepo() mockText := &testutil.MockTextProvider{ diff --git a/internal/testutil/mocks.go b/internal/testutil/mocks.go index eae27d8..bc5a7d2 100644 --- a/internal/testutil/mocks.go +++ b/internal/testutil/mocks.go @@ -282,6 +282,20 @@ func (m *MockRecipeRepo) GetRecipeByID(recipeID uint) (*models.Recipe, error) { return r, nil } +func (m *MockRecipeRepo) GetUserRecipeByCanonical(userID, canonicalID uint) (*models.Recipe, error) { + m.mu.Lock() + defer m.mu.Unlock() + var match *models.Recipe + for _, r := range m.Recipes { + if r.CreatedByID == userID && r.CanonicalID != nil && *r.CanonicalID == canonicalID { + if match == nil || r.ID < match.ID { + match = r + } + } + } + return match, nil +} + func (m *MockRecipeRepo) CreateRecipe(recipe *models.Recipe) error { if m.CreateRecipeErr != nil { return m.CreateRecipeErr @@ -289,6 +303,11 @@ func (m *MockRecipeRepo) CreateRecipe(recipe *models.Recipe) error { m.mu.Lock() defer m.mu.Unlock() + // Mirror GORM: populate the FK from the association so lookups by + // created_by_id (e.g. GetUserRecipeByCanonical) behave like the real DB. + if recipe.CreatedByID == 0 && recipe.CreatedBy != nil { + recipe.CreatedByID = recipe.CreatedBy.ID + } recipe.ID = m.NextID m.NextID++ m.Recipes[recipe.ID] = recipe