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(