From 57192a49f9504032e176ac0a8e72fb534a8b8c62 Mon Sep 17 00:00:00 2001 From: Masahiko AMANO Date: Mon, 15 Jun 2026 18:09:31 +0300 Subject: [PATCH] fix(backend): apply auto-tag rule to existing files on creation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CreateRule accepted apply_to_existing but ignored it, so enabling the checkbox while creating a rule never retroactively tagged files already carrying the when-tag — only activating an existing rule did. Extract the retroactive expansion into TagRuleRepo.ApplyToExisting (reused by SetActive) and call it from CreateRule when the rule is active, inside one transaction so a file is never left half-tagged. Mirrors SetRuleActive semantics, including following only active downstream rules. Co-Authored-By: Claude Opus 4.8 --- backend/internal/db/postgres/tag_repo.go | 13 ++- backend/internal/integration/server_test.go | 88 +++++++++++++++++++++ backend/internal/port/repository.go | 4 + backend/internal/service/tag_service.go | 34 +++++--- 4 files changed, 126 insertions(+), 13 deletions(-) diff --git a/backend/internal/db/postgres/tag_repo.go b/backend/internal/db/postgres/tag_repo.go index 329db6e..256ff68 100644 --- a/backend/internal/db/postgres/tag_repo.go +++ b/backend/internal/db/postgres/tag_repo.go @@ -604,10 +604,14 @@ WHERE when_tag_id = $1 AND then_tag_id = $2` if !active || !applyToExisting { return nil } + return r.ApplyToExisting(ctx, whenTagID, thenTagID) +} - // Retroactively apply the full transitive expansion of thenTagID to all - // files that already carry whenTagID. The recursive CTE walks active rules - // starting from thenTagID (mirrors the Go expandTagSet BFS). +// ApplyToExisting retroactively applies the full transitive expansion of +// thenTagID to all files that already carry whenTagID. The recursive CTE walks +// active rules starting from thenTagID (mirrors the Go expandTagSet BFS), so +// inactive downstream rules are not followed. Idempotent via ON CONFLICT. +func (r *TagRuleRepo) ApplyToExisting(ctx context.Context, whenTagID, thenTagID uuid.UUID) error { const retroQuery = ` WITH RECURSIVE expansion(tag_id) AS ( SELECT $2::uuid @@ -624,8 +628,9 @@ CROSS JOIN expansion e WHERE ft.tag_id = $1 ON CONFLICT DO NOTHING` + q := connOrTx(ctx, r.pool) if _, err := q.Exec(ctx, retroQuery, whenTagID, thenTagID); err != nil { - return fmt.Errorf("TagRuleRepo.SetActive retroactive apply: %w", err) + return fmt.Errorf("TagRuleRepo.ApplyToExisting: %w", err) } return nil } diff --git a/backend/internal/integration/server_test.go b/backend/internal/integration/server_test.go index 8439968..cfcae04 100644 --- a/backend/internal/integration/server_test.go +++ b/backend/internal/integration/server_test.go @@ -674,6 +674,94 @@ func TestTagRuleActivateApplyToExisting(t *testing.T) { assert.ElementsMatch(t, []string{"animal", "living-thing", "organism"}, tagNames()) } +// TestTagRuleCreateApplyToExisting verifies that *creating* a rule with +// apply_to_existing=true retroactively tags existing files — the same contract +// as activating one (TestTagRuleActivateApplyToExisting), exercised through the +// POST path. Also checks that apply_to_existing=false and an inactive rule both +// leave existing files untouched. +func TestTagRuleCreateApplyToExisting(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test in short mode") + } + + h := setupSuite(t) + tok := h.login("admin", "admin") + + mkTag := func(name string) string { + resp := h.doJSON("POST", "/tags", map[string]any{"name": name}, tok) + require.Equal(t, http.StatusCreated, resp.StatusCode, resp.String()) + var obj map[string]any + resp.decode(t, &obj) + return obj["id"].(string) + } + tagA := mkTag("animal") + tagB := mkTag("living-thing") + tagC := mkTag("organism") + + // Rule B→C: active, so it fires transitively once B lands on the file. + resp := h.doJSON("POST", "/tags/"+tagB+"/rules", map[string]any{ + "then_tag_id": tagC, + "is_active": true, + }, tok) + require.Equal(t, http.StatusCreated, resp.StatusCode, resp.String()) + + // A file carrying only tag A — no A→B rule exists yet. + file := h.uploadJPEG(tok, "cat.jpg") + fileID := file["id"].(string) + resp = h.doJSON("PUT", "/files/"+fileID+"/tags", map[string]any{ + "tag_ids": []string{tagA}, + }, tok) + require.Equal(t, http.StatusOK, resp.StatusCode, resp.String()) + + tagNames := func() []string { + r := h.doJSON("GET", "/files/"+fileID+"/tags", nil, tok) + require.Equal(t, http.StatusOK, r.StatusCode) + var items []any + r.decode(t, &items) + names := make([]string, 0, len(items)) + for _, it := range items { + names = append(names, it.(map[string]any)["name"].(string)) + } + return names + } + assert.ElementsMatch(t, []string{"animal"}, tagNames()) + + // Creating an INACTIVE rule, even with apply_to_existing=true, must not tag. + resp = h.doJSON("POST", "/tags/"+tagA+"/rules", map[string]any{ + "then_tag_id": tagB, + "is_active": false, + "apply_to_existing": true, + }, tok) + require.Equal(t, http.StatusCreated, resp.StatusCode, resp.String()) + assert.ElementsMatch(t, []string{"animal"}, tagNames(), "inactive rule must not tag existing files") + + // Drop it so we can recreate as active. + resp = h.doJSON("DELETE", "/tags/"+tagA+"/rules/"+tagB, nil, tok) + require.Equal(t, http.StatusNoContent, resp.StatusCode, resp.String()) + + // Creating an ACTIVE rule with apply_to_existing=false leaves the file alone. + resp = h.doJSON("POST", "/tags/"+tagA+"/rules", map[string]any{ + "then_tag_id": tagB, + "is_active": true, + "apply_to_existing": false, + }, tok) + require.Equal(t, http.StatusCreated, resp.StatusCode, resp.String()) + assert.ElementsMatch(t, []string{"animal"}, tagNames(), "apply_to_existing=false must not touch existing files") + + resp = h.doJSON("DELETE", "/tags/"+tagA+"/rules/"+tagB, nil, tok) + require.Equal(t, http.StatusNoContent, resp.StatusCode, resp.String()) + + // Creating A→B active WITH apply_to_existing=true: the file gets B directly + // and C transitively via the already-active B→C rule. + resp = h.doJSON("POST", "/tags/"+tagA+"/rules", map[string]any{ + "then_tag_id": tagB, + "is_active": true, + "apply_to_existing": true, + }, tok) + require.Equal(t, http.StatusCreated, resp.StatusCode, resp.String()) + assert.ElementsMatch(t, []string{"animal", "living-thing", "organism"}, tagNames()) +} + // TestTagAutoRule verifies that adding a tag automatically applies then_tags. func TestTagAutoRule(t *testing.T) { if testing.Short() { diff --git a/backend/internal/port/repository.go b/backend/internal/port/repository.go index ab45c0d..5bd7e1b 100644 --- a/backend/internal/port/repository.go +++ b/backend/internal/port/repository.go @@ -101,6 +101,10 @@ type TagRuleRepo interface { // are both true, the full transitive expansion of thenTagID is retroactively // applied to all files that already carry whenTagID. SetActive(ctx context.Context, whenTagID, thenTagID uuid.UUID, active, applyToExisting bool) error + // ApplyToExisting retroactively applies the full transitive expansion of + // thenTagID (following active rules) to every file that already carries + // whenTagID. Used when a rule is created or activated with apply_to_existing. + ApplyToExisting(ctx context.Context, whenTagID, thenTagID uuid.UUID) error Delete(ctx context.Context, whenTagID, thenTagID uuid.UUID) error } diff --git a/backend/internal/service/tag_service.go b/backend/internal/service/tag_service.go index 61bffee..beeae6d 100644 --- a/backend/internal/service/tag_service.go +++ b/backend/internal/service/tag_service.go @@ -192,16 +192,32 @@ func (s *TagService) ListRules(ctx context.Context, tagID uuid.UUID) ([]domain.T return s.rules.ListByTag(ctx, tagID) } -// CreateRule adds a tag rule. If applyToExisting is true, the then_tag is -// retroactively applied to all files that already carry the when_tag. -// Retroactive application requires a FileRepo; it is deferred until wired -// in a future iteration (see port.FileRepo.ListByTag). -func (s *TagService) CreateRule(ctx context.Context, whenTagID, thenTagID uuid.UUID, isActive, _ bool) (*domain.TagRule, error) { - return s.rules.Create(ctx, domain.TagRule{ - WhenTagID: whenTagID, - ThenTagID: thenTagID, - IsActive: isActive, +// CreateRule adds a tag rule. When the rule is active and applyToExisting is +// true, the full transitive expansion of thenTagID is retroactively applied to +// every file already carrying whenTagID — same semantics as activating an +// existing rule via SetRuleActive. The insert and retroactive apply run in one +// transaction so a file is never left half-tagged. +func (s *TagService) CreateRule(ctx context.Context, whenTagID, thenTagID uuid.UUID, isActive, applyToExisting bool) (*domain.TagRule, error) { + var created *domain.TagRule + err := s.tx.WithTx(ctx, func(ctx context.Context) error { + rule, err := s.rules.Create(ctx, domain.TagRule{ + WhenTagID: whenTagID, + ThenTagID: thenTagID, + IsActive: isActive, + }) + if err != nil { + return err + } + created = rule + if isActive && applyToExisting { + return s.rules.ApplyToExisting(ctx, whenTagID, thenTagID) + } + return nil }) + if err != nil { + return nil, err + } + return created, nil } // SetRuleActive toggles a rule's is_active flag and returns the updated rule.