diff --git a/pkg/cookiebanner/pattern_analysis_worker.go b/pkg/cookiebanner/pattern_analysis_worker.go index 979d03a04..16d7e4e18 100644 --- a/pkg/cookiebanner/pattern_analysis_worker.go +++ b/pkg/cookiebanner/pattern_analysis_worker.go @@ -202,7 +202,10 @@ func (h *patternAnalysisHandler) Process(ctx context.Context, banner coredata.Co } if shouldPromoteSource(globPattern.Source, source) { - if err := globPattern.PromoteSource(ctx, tx, scope, *source, now); err != nil { + globPattern.Source = source + globPattern.UpdatedAt = now + + if err := globPattern.Update(ctx, tx, scope); err != nil { return fmt.Errorf("cannot promote source on glob pattern %q: %w", key.template, err) } } @@ -784,12 +787,15 @@ func (h *patternAnalysisHandler) adoptUncategorisedPatterns( // non-uncategorised category. Without this, a // PRE_EXISTING glob never advances to SCRIPT/EXTENSION // even though new SDK-observed exacts confirm the - // stronger signal. PromoteSource mutates match.Source - // in place, so subsequent adoptions against the same - // glob ratchet correctly (PRE_EXISTING → EXTENSION → - // SCRIPT) without redundant writes. + // stronger signal. We mutate match.Source in place + // before calling Update so subsequent adoptions against + // the same glob ratchet correctly (PRE_EXISTING → + // EXTENSION → SCRIPT) without redundant writes. if shouldPromoteSource(match.Source, ep.Source) { - if err := match.PromoteSource(ctx, tx, scope, *ep.Source, time.Now()); err != nil { + match.Source = ep.Source + match.UpdatedAt = time.Now() + + if err := match.Update(ctx, tx, scope); err != nil { return false, fmt.Errorf("cannot promote source on glob pattern %q: %w", match.Pattern, err) } } diff --git a/pkg/cookiebanner/pattern_analysis_worker_process_test.go b/pkg/cookiebanner/pattern_analysis_worker_process_test.go index bcfa0d695..729eedf52 100644 --- a/pkg/cookiebanner/pattern_analysis_worker_process_test.go +++ b/pkg/cookiebanner/pattern_analysis_worker_process_test.go @@ -521,7 +521,8 @@ func TestPatternAnalysisWorker_AdoptionPromotesSourceCrossCategory(t *testing.T) // detected_tracker straight to the glob, so no new exact is // created and the merge/adoption loops in // patternAnalysisHandler.Process never see the new signal. Without -// the in-line PromoteSource call in reportDetectedTracker, only +// the in-line source promotion in reportDetectedTracker (which +// mutates matchedPattern.Source and writes via Update), only // last_matched_at would advance — source would stay stuck at // PRE_EXISTING despite the new SDK-observed evidence. This test // pins the same-category promotion path; the cross-category gap diff --git a/pkg/cookiebanner/service.go b/pkg/cookiebanner/service.go index badcadaa8..430d446bf 100644 --- a/pkg/cookiebanner/service.go +++ b/pkg/cookiebanner/service.go @@ -2223,7 +2223,10 @@ func (s *Service) reportDetectedTracker( // items without a source and weaker re-detections cost // nothing. if shouldPromoteSource(matchedPattern.Source, info.Source) { - if err := matchedPattern.PromoteSource(ctx, tx, scope, *info.Source, now); err != nil { + matchedPattern.Source = info.Source + matchedPattern.UpdatedAt = now + + if err := matchedPattern.Update(ctx, tx, scope); err != nil { return fmt.Errorf("cannot promote source on matched tracker pattern %q: %w", matchedPattern.Pattern, err) } } diff --git a/pkg/coredata/tracker_pattern.go b/pkg/coredata/tracker_pattern.go index f29a80918..099b99c08 100644 --- a/pkg/coredata/tracker_pattern.go +++ b/pkg/coredata/tracker_pattern.go @@ -491,52 +491,12 @@ ON CONFLICT (cookie_banner_id, tracker_type, pattern, COALESCE(max_age_seconds, return result.RowsAffected() > 0, nil } -// PromoteSource overwrites the row's source column with newSource and -// refreshes updated_at. The caller is responsible for ranking -// newSource against the existing value before invoking this method — -// see shouldPromoteSource in pkg/cookiebanner. Returns ErrResourceNotFound -// if no row with the receiver's ID exists in scope. -func (tp *TrackerPattern) PromoteSource( - ctx context.Context, - tx pg.Tx, - scope Scoper, - newSource CookieSource, - updatedAt time.Time, -) error { - q := ` -UPDATE tracker_patterns -SET - source = @source, - updated_at = @updated_at -WHERE - %s - AND id = @id -` - - q = fmt.Sprintf(q, scope.SQLFragment()) - - args := pgx.StrictNamedArgs{ - "id": tp.ID, - "source": newSource, - "updated_at": updatedAt, - } - maps.Copy(args, scope.SQLArguments()) - - result, err := tx.Exec(ctx, q, args) - if err != nil { - return fmt.Errorf("cannot promote tracker pattern source: %w", err) - } - - if result.RowsAffected() == 0 { - return ErrResourceNotFound - } - - tp.Source = &newSource - tp.UpdatedAt = updatedAt - - return nil -} - +// Update rewrites the editable columns of the receiver's row, +// including `source`. Callers MUST load the pattern under the same +// transaction before mutating fields and calling Update, otherwise +// stale local values will clobber concurrent writes. To advance +// `source`, gate the assignment behind shouldPromoteSource in +// pkg/cookiebanner — there is no DB-side ranking. func (tp *TrackerPattern) Update( ctx context.Context, tx pg.Tx, @@ -550,6 +510,7 @@ SET max_age_seconds = @max_age_seconds, description = @description, excluded = @excluded, + source = @source, last_matched_at = @last_matched_at, updated_at = @updated_at WHERE @@ -566,6 +527,7 @@ WHERE "max_age_seconds": tp.MaxAgeSeconds, "description": tp.Description, "excluded": tp.Excluded, + "source": tp.Source, "last_matched_at": tp.LastMatchedAt, "updated_at": tp.UpdatedAt, } diff --git a/pkg/coredata/tracker_pattern_promote_test.go b/pkg/coredata/tracker_pattern_test.go similarity index 67% rename from pkg/coredata/tracker_pattern_promote_test.go rename to pkg/coredata/tracker_pattern_test.go index 152aae604..b22f51db8 100644 --- a/pkg/coredata/tracker_pattern_promote_test.go +++ b/pkg/coredata/tracker_pattern_test.go @@ -50,7 +50,7 @@ func seedTrackerPatternFixture(t *testing.T, ctx context.Context, client *pg.Cli org := &coredata.Organization{ ID: organizationID, TenantID: tenantID, - Name: "TrackerPattern Promote Test Org", + Name: "TrackerPattern Test Org", CreatedAt: now, UpdatedAt: now, } @@ -61,10 +61,10 @@ func seedTrackerPatternFixture(t *testing.T, ctx context.Context, client *pg.Cli banner := &coredata.CookieBanner{ ID: cookieBannerID, OrganizationID: organizationID, - Name: "TrackerPattern Promote Test Banner", - Origin: "https://promote-test.example.com", + Name: "TrackerPattern Test Banner", + Origin: "https://tracker-pattern-test.example.com", State: coredata.CookieBannerStateActive, - CookiePolicyURL: "https://promote-test.example.com/cookies", + CookiePolicyURL: "https://tracker-pattern-test.example.com/cookies", ConsentExpiryDays: 180, ShowBranding: false, DefaultLanguage: "en", @@ -162,7 +162,15 @@ func seedTrackerPattern( return tp } -func TestTrackerPattern_PromoteSource_OverwritesSource(t *testing.T) { +// TestTrackerPattern_Update_WritesSource pins the source-promotion +// path now folded into Update: load the row, bump Source, call +// Update, and verify the new value lands in the DB. This is the +// invariant the pattern-analysis worker and reportDetectedTracker +// rely on when promoting PRE_EXISTING → SCRIPT/EXTENSION; if Update +// stops writing `source`, every "ratchet the signal" call site +// silently regresses without the wrapping `shouldPromoteSource` +// gate noticing. +func TestTrackerPattern_Update_WritesSource(t *testing.T) { t.Parallel() client := newTestPgClient(t) @@ -180,79 +188,63 @@ func TestTrackerPattern_PromoteSource_OverwritesSource(t *testing.T) { ) bumpedAt := time.Now().UTC().Add(time.Hour).Truncate(time.Microsecond) + newSource := coredata.CookieSourceScript require.NoError(t, client.WithTx(ctx, func(ctx context.Context, tx pg.Tx) error { - return tp.PromoteSource(ctx, tx, fx.scope, coredata.CookieSourceScript, bumpedAt) + var loaded coredata.TrackerPattern + if err := loaded.LoadByID(ctx, tx, fx.scope, tp.ID); err != nil { + return err + } + + loaded.Source = &newSource + loaded.UpdatedAt = bumpedAt + + return loaded.Update(ctx, tx, fx.scope) })) - require.NotNil(t, tp.Source) - assert.Equal(t, coredata.CookieSourceScript, *tp.Source, "receiver must reflect the new source") - assert.True(t, tp.UpdatedAt.Equal(bumpedAt), "receiver must reflect the new updated_at") - - loaded := &coredata.TrackerPattern{} + reloaded := &coredata.TrackerPattern{} require.NoError(t, client.WithConn(ctx, func(ctx context.Context, conn pg.Querier) error { - return loaded.LoadByID(ctx, conn, fx.scope, tp.ID) + return reloaded.LoadByID(ctx, conn, fx.scope, tp.ID) })) - require.NotNil(t, loaded.Source) - assert.Equal(t, coredata.CookieSourceScript, *loaded.Source, "DB row must reflect the promoted source") - assert.True(t, loaded.UpdatedAt.Equal(bumpedAt), "DB row must reflect the new updated_at") + require.NotNil(t, reloaded.Source) + assert.Equal(t, coredata.CookieSourceScript, *reloaded.Source, "DB row must reflect the new source") + assert.True(t, reloaded.UpdatedAt.Equal(bumpedAt), "DB row must reflect the new updated_at") } -func TestTrackerPattern_PromoteSource_OnlyTouchesSourceAndUpdatedAt(t *testing.T) { +// TestTrackerPattern_Update_NotFoundForMissingRow pins the +// ErrResourceNotFound contract: callers like the worker and +// reportDetectedTracker assume an unmatched UPDATE surfaces as +// ErrResourceNotFound so they can distinguish "row vanished mid-txn" +// from arbitrary pg errors. +func TestTrackerPattern_Update_NotFoundForMissingRow(t *testing.T) { t.Parallel() client := newTestPgClient(t) ctx := context.Background() fx := seedTrackerPatternFixture(t, ctx, client) - tp := seedTrackerPattern( - t, - ctx, - client, - fx, - "*_token", - coredata.TrackerPatternMatchTypeGlob, - coredata.CookieSourcePreExisting, - ) - - originalCategory := tp.CookieCategoryID - originalDisplay := tp.DisplayName - originalMaxAge := tp.MaxAgeSeconds - originalExcluded := tp.Excluded - originalDescription := tp.Description - - bumpedAt := time.Now().UTC().Add(2 * time.Hour).Truncate(time.Microsecond) - - require.NoError(t, client.WithTx(ctx, func(ctx context.Context, tx pg.Tx) error { - return tp.PromoteSource(ctx, tx, fx.scope, coredata.CookieSourceExtension, bumpedAt) - })) - - loaded := &coredata.TrackerPattern{} - - require.NoError(t, client.WithConn(ctx, func(ctx context.Context, conn pg.Querier) error { - return loaded.LoadByID(ctx, conn, fx.scope, tp.ID) - })) - - assert.Equal(t, originalCategory, loaded.CookieCategoryID, "category must be untouched") - assert.Equal(t, originalDisplay, loaded.DisplayName, "display_name must be untouched") - assert.Equal(t, originalMaxAge, loaded.MaxAgeSeconds, "max_age_seconds must be untouched") - assert.Equal(t, originalExcluded, loaded.Excluded, "excluded must be untouched") - assert.Equal(t, originalDescription, loaded.Description, "description must be untouched") -} - -func TestTrackerPattern_PromoteSource_NotFoundForMissingRow(t *testing.T) { - t.Parallel() - - client := newTestPgClient(t) - ctx := context.Background() - fx := seedTrackerPatternFixture(t, ctx, client) - - tp := &coredata.TrackerPattern{ID: gid.New(fx.scope.GetTenantID(), coredata.TrackerPatternEntityType)} + maxAge := 3600 + source := coredata.CookieSourceScript + now := time.Now().UTC().Truncate(time.Microsecond) + tp := &coredata.TrackerPattern{ + ID: gid.New(fx.scope.GetTenantID(), coredata.TrackerPatternEntityType), + OrganizationID: fx.organizationID, + CookieBannerID: fx.cookieBannerID, + CookieCategoryID: fx.cookieCategoryID, + TrackerType: coredata.TrackerTypeCookie, + Pattern: "*_ghost", + MatchType: coredata.TrackerPatternMatchTypeGlob, + DisplayName: "*_ghost", + MaxAgeSeconds: &maxAge, + Source: &source, + CreatedAt: now, + UpdatedAt: now, + } err := client.WithTx(ctx, func(ctx context.Context, tx pg.Tx) error { - return tp.PromoteSource(ctx, tx, fx.scope, coredata.CookieSourceScript, time.Now().UTC()) + return tp.Update(ctx, tx, fx.scope) }) assert.ErrorIs(t, err, coredata.ErrResourceNotFound)