Fold PromoteSource into Update
Every PromoteSource caller already loaded the tracker pattern under the same transaction, so a dedicated single-column UPDATE only duplicated machinery and forced callers to learn a second mutation verb. Add `source = @source` to Update's SET clause, mutate Source/UpdatedAt on the receiver, and call Update at the three promotion sites (worker merge loop, worker adoption loop, and reportDetectedTracker). The shouldPromoteSource gate still ranks the candidate against the loaded value; Update is now the single write path that can advance source, with a doc comment spelling out the load-first contract. Re-cast the coredata tests around Update: WritesSource pins the round-trip from receiver to DB, NotFoundForMissingRow preserves the ErrResourceNotFound contract callers rely on. The old OnlyTouchesSourceAndUpdatedAt test was a property of the narrow PromoteSource UPDATE and no longer applies — Update intentionally rewrites the full editable column set from the receiver. Signed-off-by: Émile Ré <emile@probo.com>
This commit is contained in:
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
Reference in New Issue
Block a user