From f392e33dc8e573cd18c8ed3c1cd87c315f7c282f Mon Sep 17 00:00:00 2001 From: James Scott Date: Tue, 30 Dec 2025 18:19:39 +0000 Subject: [PATCH] refactor(featurelistdiff): Differentiate between removed and deleted features Introduces a clear distinction between features that are truly deleted from the database versus those that are simply no longer present in a given search result (which could be due to a move, split, or rename). Previously, the reconciler would mark a feature as "Removed" and set its reason to "Deleted" if it no longer existed. This approach was ambiguous because the feature remained in the `Removed` list, which is primarily intended for features that might have been moved or split. This change introduces a new `Deleted` slice to the `FeatureDiff` struct. The reconciler logic is updated to check for a feature's existence when it's reported as removed. If it returns an `ErrEntityDoesNotExist`, the feature is moved from the `Removed` list to the new `Deleted` list. This ensures the `Removed` list now accurately contains only features that need to be further analyzed for potential moves or splits, making the diffing process more robust and the resulting data structure clearer. --- .../featurelistdiff/v1/reconciler.go | 19 ++++-- .../featurelistdiff/v1/reconciler_test.go | 67 ++++++++++++++++++- lib/blobtypes/featurelistdiff/v1/types.go | 16 ++++- .../featurelistdiff/v1/types_test.go | 35 +++++++++- lib/workertypes/types.go | 38 +++++++++++ lib/workertypes/types_test.go | 17 ++++- .../event_producer/pkg/producer/diff_test.go | 1 + .../pkg/dispatcher/dispatcher_test.go | 3 + 8 files changed, 183 insertions(+), 13 deletions(-) diff --git a/lib/blobtypes/featurelistdiff/v1/reconciler.go b/lib/blobtypes/featurelistdiff/v1/reconciler.go index cc180bc78..14b50779f 100644 --- a/lib/blobtypes/featurelistdiff/v1/reconciler.go +++ b/lib/blobtypes/featurelistdiff/v1/reconciler.go @@ -38,16 +38,18 @@ func (w *FeatureDiffWorkflow) ReconcileHistory(ctx context.Context) error { // Phase 1: Investigation // Iterate through all removed features to build a map of their historical outcomes. - for i := range w.diff.Removed { - r := &w.diff.Removed[i] - + remainingRemoved := make([]FeatureRemoved, 0, len(w.diff.Removed)) + for _, r := range w.diff.Removed { // Check the current status of the removed feature ID in the database. result, err := w.fetcher.GetFeature(ctx, r.ID) if err != nil { // If the entity is completely gone from the DB, it's a true deletion. - // We update the reason to allow for specific UI messaging (e.g. "Deleted from platform"). if errors.Is(err, backendtypes.ErrEntityDoesNotExist) { - r.Reason = ReasonDeleted + w.diff.Deleted = append(w.diff.Deleted, FeatureDeleted{ + ID: r.ID, + Name: r.Name, + Reason: ReasonDeleted, + }) continue } @@ -55,6 +57,9 @@ func (w *FeatureDiffWorkflow) ReconcileHistory(ctx context.Context) error { return err } + // If the feature was not deleted, keep it in the list to check for moves/splits. + remainingRemoved = append(remainingRemoved, r) + // Update the visitor context so it knows which OldID owns the result we are about to visit. visitor.currentID = r.ID @@ -63,6 +68,10 @@ func (w *FeatureDiffWorkflow) ReconcileHistory(ctx context.Context) error { return err } } + // Update the diff with the (now smaller) list of removed items to check. + if len(remainingRemoved) > 0 { + w.diff.Removed = remainingRemoved + } // Phase 2: Correlation // If we found any history records, try to match them with the 'Added' list. diff --git a/lib/blobtypes/featurelistdiff/v1/reconciler_test.go b/lib/blobtypes/featurelistdiff/v1/reconciler_test.go index 3f6249a89..6493de594 100644 --- a/lib/blobtypes/featurelistdiff/v1/reconciler_test.go +++ b/lib/blobtypes/featurelistdiff/v1/reconciler_test.go @@ -63,6 +63,7 @@ func TestReconcileHistory(t *testing.T) { name: "Scenario 1: Feature Moved (Rename)", initialDiff: &FeatureDiff{ Removed: []FeatureRemoved{{ID: "old-id", Name: "Old Name", Reason: ReasonUnmatched}}, + Deleted: nil, Added: []FeatureAdded{{ID: "new-id", Name: "New Name", Reason: ReasonNewMatch, Docs: nil}}, QueryChanged: false, Modified: nil, @@ -84,6 +85,7 @@ func TestReconcileHistory(t *testing.T) { QueryChanged: false, Modified: nil, Splits: nil, + Deleted: nil, }, wantErr: false, }, @@ -99,6 +101,7 @@ func TestReconcileHistory(t *testing.T) { Modified: nil, Moves: nil, Splits: nil, + Deleted: nil, }, mockResults: map[string]*backendtypes.GetFeatureResult{ "monolith": backendtypes.NewGetFeatureResult( @@ -127,6 +130,7 @@ func TestReconcileHistory(t *testing.T) { QueryChanged: false, Modified: nil, Moves: nil, + Deleted: nil, }, wantErr: false, }, @@ -142,6 +146,7 @@ func TestReconcileHistory(t *testing.T) { Modified: nil, Moves: nil, Splits: nil, + Deleted: nil, }, mockResults: map[string]*backendtypes.GetFeatureResult{ "monolith": backendtypes.NewGetFeatureResult( @@ -170,6 +175,7 @@ func TestReconcileHistory(t *testing.T) { QueryChanged: false, Modified: nil, Moves: nil, + Deleted: nil, }, wantErr: false, }, @@ -182,6 +188,7 @@ func TestReconcileHistory(t *testing.T) { Modified: nil, Moves: nil, Splits: nil, + Deleted: nil, }, mockResults: map[string]*backendtypes.GetFeatureResult{ "removed-id": backendtypes.NewGetFeatureResult( @@ -208,26 +215,29 @@ func TestReconcileHistory(t *testing.T) { Modified: nil, Moves: nil, Splits: nil, + Deleted: nil, }, wantErr: false, }, { name: "Scenario 5: Hard Delete (EntityDoesNotExist)", initialDiff: &FeatureDiff{ - Removed: []FeatureRemoved{{ID: "deleted-id", Name: "Deleted Feature", Reason: ReasonUnmatched}}, + Deleted: []FeatureDeleted{{ID: "deleted-id", Name: "Deleted Feature", Reason: ReasonDeleted}}, Added: nil, QueryChanged: false, Modified: nil, Moves: nil, Splits: nil, + Removed: nil, }, mockResults: nil, mockErrors: map[string]error{ "deleted-id": backendtypes.ErrEntityDoesNotExist, }, expectedDiff: &FeatureDiff{ - // Remains in Removed list, but Reason updated to Deleted - Removed: []FeatureRemoved{{ID: "deleted-id", Name: "Deleted Feature", Reason: ReasonDeleted}}, + // Should be moved to Deleted list + Removed: nil, + Deleted: []FeatureDeleted{{ID: "deleted-id", Name: "Deleted Feature", Reason: ReasonDeleted}}, Added: nil, QueryChanged: false, Modified: nil, @@ -247,6 +257,7 @@ func TestReconcileHistory(t *testing.T) { Modified: nil, Moves: nil, Splits: nil, + Deleted: nil, }, mockResults: map[string]*backendtypes.GetFeatureResult{ "old-id": backendtypes.NewGetFeatureResult( @@ -261,6 +272,7 @@ func TestReconcileHistory(t *testing.T) { Modified: nil, Moves: nil, Splits: nil, + Deleted: nil, }, wantErr: false, }, @@ -273,6 +285,7 @@ func TestReconcileHistory(t *testing.T) { Modified: nil, Moves: nil, Splits: nil, + Deleted: nil, }, mockResults: nil, mockErrors: map[string]error{ @@ -292,6 +305,7 @@ func TestReconcileHistory(t *testing.T) { Modified: nil, Moves: nil, Splits: nil, + Deleted: nil, }, mockResults: map[string]*backendtypes.GetFeatureResult{ "monolith": backendtypes.NewGetFeatureResult( @@ -310,6 +324,7 @@ func TestReconcileHistory(t *testing.T) { Modified: nil, Moves: nil, Splits: nil, + Deleted: nil, }, wantErr: false, }, @@ -327,6 +342,7 @@ func TestReconcileHistory(t *testing.T) { Modified: nil, Moves: nil, Splits: nil, + Deleted: nil, }, mockResults: map[string]*backendtypes.GetFeatureResult{ "old-id": backendtypes.NewGetFeatureResult( @@ -343,6 +359,51 @@ func TestReconcileHistory(t *testing.T) { QueryChanged: false, Modified: nil, Splits: nil, + Deleted: nil, + }, + wantErr: false, + }, + { + name: "Scenario 10: Mixed Removed and Deleted", + initialDiff: &FeatureDiff{ + Removed: []FeatureRemoved{ + {ID: "deleted-id", Name: "Deleted Feature", Reason: ReasonUnmatched}, + {ID: "removed-id", Name: "Removed Feature", Reason: ReasonUnmatched}, + }, + Added: nil, + QueryChanged: false, + Modified: nil, + Moves: nil, + Splits: nil, + Deleted: nil, + }, + mockResults: map[string]*backendtypes.GetFeatureResult{ + "removed-id": backendtypes.NewGetFeatureResult( + backendtypes.NewRegularFeatureResult(&backend.Feature{ + FeatureId: "removed-id", + Name: "", + Spec: nil, + Baseline: nil, + BrowserImplementations: nil, + Discouraged: nil, + Usage: nil, + Wpt: nil, + VendorPositions: nil, + DeveloperSignals: nil, + }), + ), + }, + mockErrors: map[string]error{ + "deleted-id": backendtypes.ErrEntityDoesNotExist, + }, + expectedDiff: &FeatureDiff{ + Removed: []FeatureRemoved{{ID: "removed-id", Name: "Removed Feature", Reason: ReasonUnmatched}}, + Deleted: []FeatureDeleted{{ID: "deleted-id", Name: "Deleted Feature", Reason: ReasonDeleted}}, + Added: nil, + QueryChanged: false, + Modified: nil, + Moves: nil, + Splits: nil, }, wantErr: false, }, diff --git a/lib/blobtypes/featurelistdiff/v1/types.go b/lib/blobtypes/featurelistdiff/v1/types.go index a09a1fb26..271e7b09e 100644 --- a/lib/blobtypes/featurelistdiff/v1/types.go +++ b/lib/blobtypes/featurelistdiff/v1/types.go @@ -94,6 +94,7 @@ type FeatureDiff struct { QueryChanged bool `json:"queryChanged,omitempty"` Added []FeatureAdded `json:"added,omitempty"` Removed []FeatureRemoved `json:"removed,omitempty"` + Deleted []FeatureDeleted `json:"deleted,omitempty"` Modified []FeatureModified `json:"modified,omitempty"` Moves []FeatureMoved `json:"moves,omitempty"` Splits []FeatureSplit `json:"splits,omitempty"` @@ -125,6 +126,13 @@ func (d *FeatureDiff) Sort() { return d.Removed[i].ID < d.Removed[j].ID }) + sort.Slice(d.Deleted, func(i, j int) bool { + if d.Deleted[i].Name != d.Deleted[j].Name { + return d.Deleted[i].Name < d.Deleted[j].Name + } + + return d.Deleted[i].ID < d.Deleted[j].ID + }) sort.Slice(d.Modified, func(i, j int) bool { if d.Modified[i].Name != d.Modified[j].Name { return d.Modified[i].Name < d.Modified[j].Name @@ -182,6 +190,12 @@ type FeatureRemoved struct { Reason ChangeReason `json:"reason"` } +type FeatureDeleted struct { + ID string `json:"id"` + Name string `json:"name"` + Reason ChangeReason `json:"reason"` +} + type FeatureMoved struct { FromID string `json:"fromId"` ToID string `json:"toId"` @@ -223,7 +237,7 @@ func (d FeatureDiff) HasChanges() bool { } func (d FeatureDiff) HasDataChanges() bool { - return len(d.Added) > 0 || len(d.Removed) > 0 || + return len(d.Added) > 0 || len(d.Removed) > 0 || len(d.Deleted) > 0 || len(d.Modified) > 0 || len(d.Moves) > 0 || len(d.Splits) > 0 } diff --git a/lib/blobtypes/featurelistdiff/v1/types_test.go b/lib/blobtypes/featurelistdiff/v1/types_test.go index 831ecb8a4..c75802da2 100644 --- a/lib/blobtypes/featurelistdiff/v1/types_test.go +++ b/lib/blobtypes/featurelistdiff/v1/types_test.go @@ -36,6 +36,7 @@ func TestHasChanges(t *testing.T) { Modified: nil, Moves: nil, Splits: nil, + Deleted: nil, }, expected: false, }, @@ -48,6 +49,7 @@ func TestHasChanges(t *testing.T) { Modified: nil, Moves: nil, Splits: nil, + Deleted: nil, }, expected: true, }, @@ -60,6 +62,7 @@ func TestHasChanges(t *testing.T) { Modified: nil, Moves: nil, Splits: nil, + Deleted: nil, }, expected: true, }, @@ -72,6 +75,22 @@ func TestHasChanges(t *testing.T) { Modified: nil, Moves: nil, Splits: nil, + Deleted: nil, + }, + expected: true, + }, + { + name: "Deleted", + diff: FeatureDiff{ + QueryChanged: false, + Added: nil, + Removed: nil, + Modified: nil, + Moves: nil, + Splits: nil, + Deleted: []FeatureDeleted{ + {ID: "1", Name: "A", Reason: ReasonDeleted}, + }, }, expected: true, }, @@ -101,8 +120,9 @@ func TestHasChanges(t *testing.T) { BrowserChanges: nil, DocsChange: nil, }}, - Moves: nil, - Splits: nil, + Moves: nil, + Splits: nil, + Deleted: nil, }, expected: true, }, @@ -115,6 +135,7 @@ func TestHasChanges(t *testing.T) { Modified: nil, Moves: []FeatureMoved{{FromID: "A", ToID: "B", FromName: "A", ToName: "B"}}, Splits: nil, + Deleted: nil, }, expected: true, }, @@ -127,6 +148,7 @@ func TestHasChanges(t *testing.T) { Modified: nil, Moves: nil, Splits: []FeatureSplit{{FromID: "A", FromName: "A", To: nil}}, + Deleted: nil, }, expected: true, }, @@ -176,6 +198,10 @@ func TestFeatureDiff_Sort(t *testing.T) { To: nil, }, }, + Deleted: []FeatureDeleted{ + {ID: "2", Name: "B", Reason: ReasonDeleted}, + {ID: "1", Name: "A", Reason: ReasonDeleted}, + }, } diff.Sort() @@ -190,6 +216,11 @@ func TestFeatureDiff_Sort(t *testing.T) { t.Errorf("Removed sort failed: %+v", diff.Removed) } + // Deleted: A(1), B(2) + if diff.Deleted[0].ID != "1" || diff.Deleted[1].ID != "2" { + t.Errorf("Deleted sort failed: %+v", diff.Deleted) + } + // Modified: A(1), B(2) if diff.Modified[0].ID != "1" || diff.Modified[1].ID != "2" { t.Errorf("Modified sort failed: %+v", diff.Modified) diff --git a/lib/workertypes/types.go b/lib/workertypes/types.go index d4d27f097..ab0bd3e62 100644 --- a/lib/workertypes/types.go +++ b/lib/workertypes/types.go @@ -78,6 +78,7 @@ type SummaryCategories struct { QueryChanged int `json:"query_changed,omitzero"` Added int `json:"added,omitzero"` Removed int `json:"removed,omitzero"` + Deleted int `json:"deleted,omitzero"` Moved int `json:"moved,omitzero"` Split int `json:"split,omitzero"` Updated int `json:"updated,omitzero"` @@ -138,6 +139,7 @@ const ( type BrowserValue struct { Status BrowserStatus `json:"status"` Version *string `json:"version,omitempty"` + Date *time.Time `json:"date,omitempty"` } type BrowserName string @@ -160,6 +162,7 @@ const ( SummaryHighlightTypeChanged SummaryHighlightType = "Changed" SummaryHighlightTypeMoved SummaryHighlightType = "Moved" SummaryHighlightTypeSplit SummaryHighlightType = "Split" + SummaryHighlightTypeDeleted SummaryHighlightType = "Deleted" ) type SplitChange struct { @@ -250,6 +253,10 @@ func (g FeatureDiffV1SummaryGenerator) calculateCategoriesAndText(d v1.FeatureDi parts = append(parts, fmt.Sprintf("%d features removed", len(d.Removed))) c.Removed = len(d.Removed) } + if len(d.Deleted) > 0 { + parts = append(parts, fmt.Sprintf("%d features deleted", len(d.Deleted))) + c.Deleted = len(d.Deleted) + } if len(d.Moves) > 0 { parts = append(parts, fmt.Sprintf("%d features moved/renamed", len(d.Moves))) c.Moved = len(d.Moves) @@ -298,6 +305,10 @@ func (g FeatureDiffV1SummaryGenerator) generateHighlights(d v1.FeatureDiff) ([]S return highlights, true } + if highlights, truncated = g.processDeleted(highlights, d.Deleted); truncated { + return highlights, true + } + if highlights, truncated = g.processMoves(highlights, d.Moves); truncated { return highlights, true } @@ -423,6 +434,28 @@ func (g FeatureDiffV1SummaryGenerator) processRemoved(highlights []SummaryHighli return highlights, false } +func (g FeatureDiffV1SummaryGenerator) processDeleted(highlights []SummaryHighlight, + deleted []v1.FeatureDeleted) ([]SummaryHighlight, bool) { + for _, r := range deleted { + if len(highlights) >= MaxHighlights { + return highlights, true + } + highlights = append(highlights, SummaryHighlight{ + Type: SummaryHighlightTypeDeleted, + FeatureID: r.ID, + FeatureName: r.Name, + DocLinks: nil, + Moved: nil, + Split: nil, + BaselineChange: nil, + NameChange: nil, + BrowserChanges: nil, + }) + } + + return highlights, false +} + func (g FeatureDiffV1SummaryGenerator) processMoves(highlights []SummaryHighlight, moves []v1.FeatureMoved) ([]SummaryHighlight, bool) { for _, m := range moves { @@ -524,6 +557,7 @@ func toBrowserValue(s v1.BrowserState) BrowserValue { val := BrowserValue{ Status: BrowserStatusUnknown, Version: nil, + Date: nil, } if s.Status.IsSet { switch s.Status.Value { @@ -537,6 +571,10 @@ func toBrowserValue(s v1.BrowserState) BrowserValue { val.Version = s.Version.Value } + if s.Date.IsSet { + val.Date = s.Date.Value + } + return val } diff --git a/lib/workertypes/types_test.go b/lib/workertypes/types_test.go index 3263072bb..b2919c09c 100644 --- a/lib/workertypes/types_test.go +++ b/lib/workertypes/types_test.go @@ -60,6 +60,7 @@ func TestParseEventSummary(t *testing.T) { QueryChanged: 0, Added: 0, Removed: 0, + Deleted: 0, Moved: 0, Split: 0, Updated: 0, @@ -121,6 +122,7 @@ func TestParseEventSummary(t *testing.T) { func TestGenerateJSONSummaryFeatureDiffV1(t *testing.T) { newlyDate := time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC) + browserImplDate := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC) tests := []struct { name string diff v1.FeatureDiff @@ -136,6 +138,7 @@ func TestGenerateJSONSummaryFeatureDiffV1(t *testing.T) { Modified: nil, Moves: nil, Splits: nil, + Deleted: nil, }, expected: `{"schemaVersion":"v1","text":"No changes detected","truncated":false,"highlights":null}`, expectedError: nil, @@ -153,6 +156,9 @@ func TestGenerateJSONSummaryFeatureDiffV1(t *testing.T) { Removed: []v1.FeatureRemoved{ {ID: "3", Name: "C", Reason: v1.ReasonUnmatched}, }, + Deleted: []v1.FeatureDeleted{ + {ID: "4", Name: "D", Reason: v1.ReasonDeleted}, + }, Moves: []v1.FeatureMoved{ {FromID: "4", ToID: "5", FromName: "D", ToName: "E"}, }, @@ -192,7 +198,7 @@ func TestGenerateJSONSummaryFeatureDiffV1(t *testing.T) { Version: generic.UnsetOpt[*string](), }, To: v1.BrowserState{ Status: generic.SetOpt(v1.Available), - Date: generic.UnsetOpt[*time.Time](), + Date: generic.SetOpt(&browserImplDate), Version: generic.SetOpt(generic.ValuePtr("123")), }}, v1.ChromeAndroid: nil, @@ -220,11 +226,12 @@ func TestGenerateJSONSummaryFeatureDiffV1(t *testing.T) { expected: `{ "schemaVersion": "v1", "text": "Search criteria updated, 2 features added, 1 features removed, ` + - `1 features moved/renamed, 1 features split, 3 features updated", + `1 features deleted, 1 features moved/renamed, 1 features split, 3 features updated", "categories": { "query_changed": 1, "added": 2, "removed": 1, + "deleted": 1, "moved": 1, "split": 1, "updated": 3, @@ -258,6 +265,7 @@ func TestGenerateJSONSummaryFeatureDiffV1(t *testing.T) { "status": "unavailable" }, "to": { + "date": "2024-01-01T00:00:00Z", "status": "available", "version": "123" } @@ -294,6 +302,11 @@ func TestGenerateJSONSummaryFeatureDiffV1(t *testing.T) { "type": "Removed", "feature_id": "3", "feature_name": "C" + }, + { + "type": "Deleted", + "feature_id": "4", + "feature_name": "D" }, { "type": "Moved", diff --git a/workers/event_producer/pkg/producer/diff_test.go b/workers/event_producer/pkg/producer/diff_test.go index bfb265a48..348229a42 100644 --- a/workers/event_producer/pkg/producer/diff_test.go +++ b/workers/event_producer/pkg/producer/diff_test.go @@ -163,6 +163,7 @@ func TestV1DiffSerializer_Serialize(t *testing.T) { diff := &featurelistdiffv1.FeatureDiff{ QueryChanged: false, Added: []featurelistdiffv1.FeatureAdded{{ID: "feat-a", Name: "Feature A", Reason: "", Docs: nil}}, + Deleted: nil, Removed: nil, Modified: nil, Moves: nil, diff --git a/workers/push_delivery/pkg/dispatcher/dispatcher_test.go b/workers/push_delivery/pkg/dispatcher/dispatcher_test.go index 7d1fe8853..20db9fa6b 100644 --- a/workers/push_delivery/pkg/dispatcher/dispatcher_test.go +++ b/workers/push_delivery/pkg/dispatcher/dispatcher_test.go @@ -71,6 +71,7 @@ func createTestSummary(hasChanges bool) workertypes.EventSummary { categories := workertypes.SummaryCategories{ QueryChanged: 0, Added: 0, + Deleted: 0, Removed: 0, Moved: 0, Split: 0, @@ -429,10 +430,12 @@ func newBaselineValue(status workertypes.BaselineStatus) workertypes.BaselineVal func newBrowserValue(status workertypes.BrowserStatus) workertypes.BrowserValue { version := "100" + testDate := time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC) return workertypes.BrowserValue{ Status: status, Version: &version, + Date: &testDate, } }