From 5b9899d47aebde0bbff5670a82d005dbdb80ce44 Mon Sep 17 00:00:00 2001 From: James Scott Date: Thu, 5 Feb 2026 06:34:41 +0000 Subject: [PATCH] fix(comparator): fix change detection for schema evolution and cold starts This change fixes a bug where new browser implementation data was not triggering notifications if the feature previously had no browser data (the "Cold Start" problem). It also standardizes how optional fields are compared to support clean schema evolution. Key Changes: - **Comparator Logic (`lib/blobtypes/v1/comparator.go`):** - Refactored `compareBrowserImpls` to remove an early return when the old state is unset. It now correctly detects when browser data is added for the first time. - Standardized all `compareXYZ` methods to explicitly handle the four states of `OptionallySet` fields: Unset/Unset (No Change), Unset/Set (Added), Set/Unset (Removed), and Set/Set (Value comparison). - Implemented "Quiet Rollout" logic for browsers: additions are ignored if the status is "Unavailable" and no other details (version/date) are present, reducing noise during backfills. - Refactored Name comparison to be robust against Unset states. - **Documentation:** - Added a "Guide for Adding New Fields" to `comparator.go` to ensure future maintainers follow the same robust comparison patterns. - Documented the "Quiet Rollout" and "Cold Start" philosophies. - **Testing:** - Added test cases to verify parent-level container additions (e.g., adding the first `BrowserImplementations` or `BaselineStatus` to a feature). - Verified that all transitions (Unset <-> Set) for names and baseline statuses trigger correct changes. --- .../featurelistdiff/v1/comparator.go | 163 +++++++++++++++--- .../featurelistdiff/v1/comparator_test.go | 109 +++++++++++- 2 files changed, 246 insertions(+), 26 deletions(-) diff --git a/lib/blobtypes/featurelistdiff/v1/comparator.go b/lib/blobtypes/featurelistdiff/v1/comparator.go index 4e072d392..204b3e037 100644 --- a/lib/blobtypes/featurelistdiff/v1/comparator.go +++ b/lib/blobtypes/featurelistdiff/v1/comparator.go @@ -24,6 +24,28 @@ import ( "github.com/GoogleChrome/webstatus.dev/lib/workertypes/comparables" ) +// CalculateDiff computes the difference between two sets of features (old and new snapshots). +// +// Comparison Logic & Schema Evolution: +// The comparator relies heavily on generic.OptionallySet[T] to handle schema evolution. +// 1. Cold Start / New Fields: If a field (like BaselineStatus) was "Unset" in the old snapshot +// (e.g. because it didn't exist in the schema then) and is "Set" in the new snapshot, +// this is treated as a Change. This ensures users are notified when new data becomes available. +// 2. Quiet Rollouts (Browsers): A specific exception exists for BrowserImplementations. +// If a browser moves from "Unset" to "Set(Unavailable)" with no extra details, we IGNORE it. +// This prevents spamming users when we add a new browser column to the DB that is mostly empty. +// +// Guide for Adding New Fields: +// When adding a new field to comparables.Feature: +// 1. Wrap it in generic.OptionallySet[T]. +// 2. In your compareXYZ function, explicitly handle the 4 transition cases: +// - !old.IsSet && !new.IsSet: Return nil (No change). +// - !old.IsSet && new.IsSet: Return Change (Added). This handles the "Cold Start". +// - old.IsSet && !new.IsSet: Return Change (Removed). +// - Both Set: Compare actual values. +// 3. Consider "Quiet Rollout": If your new field will be backfilled with "default/empty" values +// (like "Unavailable" or "Unknown") that users shouldn't be bothered about, add a check +// in the "Added" case to return no change for those specific values. func (w *FeatureDiffWorkflow) CalculateDiff(oldMap, newMap map[string]comparables.Feature) { for id, newF := range newMap { oldF, exists := oldMap[id] @@ -78,8 +100,17 @@ func compareFeature(oldF, newF comparables.Feature) (FeatureModified, bool) { hasMods := false // 1. Name Change - if oldF.Name.IsSet && oldF.Name.Value != newF.Name.Value { - mod.NameChange = &Change[string]{From: oldF.Name.Value, To: newF.Name.Value} + oldName := "" + if oldF.Name.IsSet { + oldName = oldF.Name.Value + } + newName := "" + if newF.Name.IsSet { + newName = newF.Name.Value + } + + if oldName != newName { + mod.NameChange = &Change[string]{From: oldName, To: newName} hasMods = true } @@ -110,15 +141,38 @@ func compareFeature(oldF, newF comparables.Feature) (FeatureModified, bool) { func compareBaseline( oldStatus, newStatus generic.OptionallySet[comparables.BaselineState], ) (*Change[BaselineState], bool) { - if oldStatus.IsSet { - oldBase := oldStatus.Value - newBase := newStatus.Value - if oldBase.Status.IsSet && oldBase.Status.Value != newBase.Status.Value { - return &Change[BaselineState]{ - From: toV1BaselineState(oldBase), - To: toV1BaselineState(newBase), - }, true - } + if !oldStatus.IsSet && !newStatus.IsSet { + return nil, false + } + + // Case 2: Added (Old Unset, New Set) + if !oldStatus.IsSet && newStatus.IsSet { + zero := new(BaselineState) + + return &Change[BaselineState]{ + From: *zero, // Zero value represents "None" + To: toV1BaselineState(newStatus.Value), + }, true + } + + // Case 3: Removed (Old Set, New Unset) + if oldStatus.IsSet && !newStatus.IsSet { + zero := new(BaselineState) + + return &Change[BaselineState]{ + From: toV1BaselineState(oldStatus.Value), + To: *zero, // Zero value represents "None" + }, true + } + + // Case 4: Both Set -> Compare Values + oldBase := oldStatus.Value + newBase := newStatus.Value + if oldBase.Status.IsSet && oldBase.Status.Value != newBase.Status.Value { + return &Change[BaselineState]{ + From: toV1BaselineState(oldBase), + To: toV1BaselineState(newBase), + }, true } return nil, false @@ -165,12 +219,15 @@ func compareBrowserImpls( changes := make(map[SupportedBrowsers]*Change[BrowserState]) hasChanged := false - if !oldImpls.IsSet { - return changes, false + var oldB comparables.BrowserImplementations + if oldImpls.IsSet { + oldB = oldImpls.Value } - oldB := oldImpls.Value - newB := newImpls.Value + var newB comparables.BrowserImplementations + if newImpls.IsSet { + newB = newImpls.Value + } browserMap := map[SupportedBrowsers]struct { Old generic.OptionallySet[comparables.BrowserState] @@ -197,13 +254,14 @@ func compareBrowserImpls( // compareDocs checks for changes in the documentation links. func compareDocs(oldDocs, newDocs generic.OptionallySet[comparables.Docs]) (*Change[Docs], bool) { - if !oldDocs.IsSet || !oldDocs.Value.MdnDocs.IsSet { + oldMdnDocs := resolveMdnDocs(oldDocs) + newMdnDocs := resolveMdnDocs(newDocs) + if len(oldMdnDocs) == 0 && len(newMdnDocs) == 0 { return nil, false } - oldMdnDocs := oldDocs.Value.MdnDocs.Value - newMdnDocs := newDocs.Value.MdnDocs.Value + // Sort both lists for deterministic comparison sortMDNDocs := func(a, b comparables.MdnDoc) int { return cmp.Compare(a.URL.Value, b.URL.Value) } @@ -213,16 +271,30 @@ func compareDocs(oldDocs, newDocs generic.OptionallySet[comparables.Docs]) (*Cha mdnDocsEqual := func(a, b comparables.MdnDoc) bool { return a.URL.Value == b.URL.Value } + if !slices.EqualFunc(oldMdnDocs, newMdnDocs, mdnDocsEqual) { + // Construct the Change object using the original wrappers (or create valid wrappers) return &Change[Docs]{ - From: toV1Docs(oldDocs.Value), - To: toV1Docs(newDocs.Value), + From: toV1DocsFromList(oldMdnDocs), + To: toV1DocsFromList(newMdnDocs), }, true } return nil, false } +// resolveMdnDocs extracts the MDN doc list safely from the nested Option structure. +func resolveMdnDocs(docs generic.OptionallySet[comparables.Docs]) []comparables.MdnDoc { + if !docs.IsSet { + return nil + } + if !docs.Value.MdnDocs.IsSet { + return nil + } + + return docs.Value.MdnDocs.Value +} + func toV1Docs(d comparables.Docs) Docs { var mdnDocs []MdnDoc if d.MdnDocs.IsSet { @@ -238,6 +310,20 @@ func toV1Docs(d comparables.Docs) Docs { return Docs{MdnDocs: mdnDocs} } +// toV1DocsFromList creates a V1 Docs struct directly from a slice of comparables.MdnDoc. +func toV1DocsFromList(list []comparables.MdnDoc) Docs { + mdnDocs := make([]MdnDoc, 0, len(list)) + for _, doc := range list { + mdnDocs = append(mdnDocs, MdnDoc{ + URL: doc.URL.Value, + Title: doc.Title.Value, + Slug: doc.Slug.Value, + }) + } + + return Docs{MdnDocs: mdnDocs} +} + func toV1BrowserImplementationStatus(status generic.OptionallySet[backend.BrowserImplementationStatus]) generic. OptionallySet[BrowserImplementationStatus] { if !status.IsSet { @@ -284,9 +370,44 @@ func toV1BrowserChange(change *Change[comparables.BrowserState]) *Change[Browser func compareBrowserState( oldB, newB generic.OptionallySet[comparables.BrowserState], ) (*Change[comparables.BrowserState], bool) { - if !oldB.IsSet { + // Case 1: Both Unset -> No Change + if !oldB.IsSet && !newB.IsSet { return nil, false } + + // Case 2: Added (Old Unset, New Set) + if !oldB.IsSet && newB.IsSet { + // Quiet Rollout Support: + // If a new browser is added to the system (Unset -> Set), we only report it + // if it provides meaningful info (Available, or has Version/Date). + // If it's just "Unavailable" with no other info, we treat it as no change + // to avoid spamming the user with "New Browser Added: Unavailable" notifications. + val := newB.Value + isUnavailable := val.Status.IsSet && val.Status.Value == backend.Unavailable + hasDetails := val.Version.IsSet || val.Date.IsSet + + if isUnavailable && !hasDetails { + return nil, false + } + zero := new(comparables.BrowserState) + + return &Change[comparables.BrowserState]{ + From: *zero, // Zero value represents "None" + To: newB.Value, + }, true + } + + // Case 3: Removed (Old Set, New Unset) + if oldB.IsSet && !newB.IsSet { + zero := new(comparables.BrowserState) + + return &Change[comparables.BrowserState]{ + From: oldB.Value, + To: *zero, // Zero value represents "None" + }, true + } + + // Case 4: Both Set -> Compare Values // Check Status isChanged := oldB.Value.Status.IsSet && oldB.Value.Status.Value != newB.Value.Status.Value // Check Version diff --git a/lib/blobtypes/featurelistdiff/v1/comparator_test.go b/lib/blobtypes/featurelistdiff/v1/comparator_test.go index 9d07def02..93ff27f13 100644 --- a/lib/blobtypes/featurelistdiff/v1/comparator_test.go +++ b/lib/blobtypes/featurelistdiff/v1/comparator_test.go @@ -148,6 +148,38 @@ func TestCompareFeature_NameChange(t *testing.T) { } } +func TestCompareFeature_Name_Added(t *testing.T) { + oldF := newBaseFeature("1", "", "limited") + oldF.Name = generic.UnsetOpt[string]() + + newF := newBaseFeature("1", "New Name", "limited") + + mod, changed := compareFeature(oldF, newF) + + if !changed { + t.Fatal("Name Added should trigger a change") + } + if mod.NameChange.From != "" || mod.NameChange.To != "New Name" { + t.Errorf("NameChange Added mismatch: got %+v", mod.NameChange) + } +} + +func TestCompareFeature_Name_Removed(t *testing.T) { + oldF := newBaseFeature("1", "Old Name", "limited") + + newF := newBaseFeature("1", "", "limited") + newF.Name = generic.UnsetOpt[string]() + + mod, changed := compareFeature(oldF, newF) + + if !changed { + t.Fatal("Name Removed should trigger a change") + } + if mod.NameChange.From != "Old Name" || mod.NameChange.To != "" { + t.Errorf("NameChange Removed mismatch: got %+v", mod.NameChange) + } +} + func TestCompareFeature_BaselineChange(t *testing.T) { oldF := newBaseFeature("1", "A", "limited") newF := newBaseFeature("1", "A", "widely") @@ -255,14 +287,32 @@ func TestCompareFeature_QuietRollout_NewBrowser(t *testing.T) { // Old feature is missing any data for Chrome oldF := newBaseFeature("1", "A", "limited") - // New feature now has data for Chrome + // New feature now has data for Chrome, but it's Unavailable (and no details) newF := newBaseFeature("1", "A", "limited") - newF.BrowserImpls.Value.Chrome = newBrowserState(backend.Available, nil, nil) + newF.BrowserImpls.Value.Chrome = newBrowserState(backend.Unavailable, nil, nil) _, changed := compareFeature(oldF, newF) if changed { - t.Error("quiet rollout of a new browser should not trigger a change") + t.Error("quiet rollout of a new browser (Unavailable) should not trigger a change") + } +} + +func TestCompareFeature_NewBrowser_Available(t *testing.T) { + // Old feature is missing any data for Chrome + oldF := newBaseFeature("1", "A", "limited") + + // New feature now has data for Chrome, and it is Available + newF := newBaseFeature("1", "A", "limited") + newF.BrowserImpls.Value.Chrome = newBrowserState(backend.Available, nil, nil) + + mod, changed := compareFeature(oldF, newF) + + if !changed { + t.Fatal("Unset -> Available should trigger a change") + } + if len(mod.BrowserChanges) == 0 { + t.Error("BrowserChanges should be populated") } } @@ -271,9 +321,9 @@ func TestCompareFeature_QuietRollout_NewTopLevelField(t *testing.T) { oldF := newBaseFeature("1", "A", "limited") oldF.BrowserImpls = generic.UnsetOpt[comparables.BrowserImplementations]() - // New feature now has the struct and data for a browser + // New feature now has the struct and data for a browser (Unavailable) newF := newBaseFeature("1", "A", "limited") - newF.BrowserImpls.Value.Chrome = newBrowserState(backend.Available, nil, nil) + newF.BrowserImpls.Value.Chrome = newBrowserState(backend.Unavailable, nil, nil) _, changed := compareFeature(oldF, newF) @@ -282,6 +332,55 @@ func TestCompareFeature_QuietRollout_NewTopLevelField(t *testing.T) { } } +func TestCompareFeature_BrowserImpls_Added_Available(t *testing.T) { + // Old feature is missing the entire BrowserImpls struct (Unset) + oldF := newBaseFeature("1", "A", "limited") + oldF.BrowserImpls = generic.UnsetOpt[comparables.BrowserImplementations]() + + // New feature has the struct AND data for Chrome (Available) + newF := newBaseFeature("1", "A", "limited") + newF.BrowserImpls = generic.OptionallySet[comparables.BrowserImplementations]{ + IsSet: true, + Value: comparables.BrowserImplementations{ + Chrome: newBrowserState(backend.Available, nil, nil), + ChromeAndroid: unsetBrowserState(), + Edge: unsetBrowserState(), + Firefox: unsetBrowserState(), + FirefoxAndroid: unsetBrowserState(), + Safari: unsetBrowserState(), + SafariIos: unsetBrowserState(), + }, + } + + mod, changed := compareFeature(oldF, newF) + + if !changed { + t.Fatal("Unset Parent -> Available Child should trigger a change") + } + if len(mod.BrowserChanges) == 0 { + t.Error("BrowserChanges should be populated") + } +} + +func TestCompareFeature_BaselineStatus_Added(t *testing.T) { + oldF := newBaseFeature("1", "A", "limited") + oldF.BaselineStatus = generic.UnsetOpt[comparables.BaselineState]() + + newF := newBaseFeature("1", "A", "widely") + + mod, changed := compareFeature(oldF, newF) + + if !changed { + t.Fatal("Baseline Unset -> Set should trigger a change") + } + if mod.BaselineChange == nil { + t.Fatal("BaselineChange should be populated") + } + if mod.BaselineChange.To.Status.Value != Widely { + t.Errorf("Baseline status mismatch: got %v, want %v", mod.BaselineChange.To.Status.Value, Widely) + } +} + // --- Test Helpers --- func newBrowserState(