refactor: Use sync for browser feature availabilities (#1905)

* refactor: Use sync for browser feature availabilities

This commit refactors the code to use a new sync method for updating
browser feature availabilities. The new method, SyncBrowserFeatureAvailabilities,
allows for batch updates, which improves performance by reducing the number
of database calls. And also helps in the cases when a feature's availability regresses for a browser.

The following changes were made:
- Replaced UpsertBrowserFeatureAvailability with SyncBrowserFeatureAvailabilities
  in all the test files.
- Updated the main logic to use the new sync method.
- Updated the data loading scripts to use the new sync method.

* simplify

Author: jcscottiii

lib/gcpspanner/browser_availabilities.go

@@ -17,87 +17,122 @@ package gcpspanner
 import (
 	"context"
 	"fmt"
+	"log/slog"
 
 	"cloud.google.com/go/spanner"
 )
 
 const browserFeatureAvailabilitiesTable = "BrowserFeatureAvailabilities"
 
-// Implements the entityMapper interface for BrowserFeatureAvailability and SpannerBrowserFeatureAvailability.
-type browserFeatureAvailabilityMapper struct{}
+// spannerBrowserFeatureAvailability is a wrapper for the browser availability
+// information for a feature stored in spanner.
+type spannerBrowserFeatureAvailability struct {
+	WebFeatureID   string `spanner:"WebFeatureID"`
+	BrowserName    string `spanner:"BrowserName"`
+	BrowserVersion string `spanner:"BrowserVersion"`
+}
 
-func (m browserFeatureAvailabilityMapper) Table() string {
-	return browserFeatureAvailabilitiesTable
+// BrowserFeatureAvailability contains availability information for a particular
+// feature in a browser.
+type BrowserFeatureAvailability struct {
+	BrowserName    string
+	BrowserVersion string
 }
 
-type browserFeatureAvailabilityKey struct {
-	WebFeatureID string
-	BrowserName  string
+// Implements the syncableEntityMapper interface for BrowserFeatureAvailability and spannerBrowserFeatureAvailability.
+type browserFeatureAvailabilitySpannerMapper struct{}
+
+// PreDeleteHook is a no-op for browser feature availabilities.
+func (m browserFeatureAvailabilitySpannerMapper) PreDeleteHook(
+	_ context.Context,
+	_ *Client,
+	_ []spannerBrowserFeatureAvailability,
+) ([]ExtraMutationsGroup, error) {
+	return nil, nil
 }
 
-func (m browserFeatureAvailabilityMapper) SelectOne(key browserFeatureAvailabilityKey) spanner.Statement {
-	stmt := spanner.NewStatement(fmt.Sprintf(`
+// GetChildDeleteKeyMutations returns nil as there are no child delete mutations for browser feature availabilities.
+func (m browserFeatureAvailabilitySpannerMapper) GetChildDeleteKeyMutations(
+	_ context.Context,
+	_ *Client,
+	_ []spannerBrowserFeatureAvailability,
+) ([]ExtraMutationsGroup, error) {
+	return nil, nil
+}
+
+func (m browserFeatureAvailabilitySpannerMapper) Table() string {
+	return browserFeatureAvailabilitiesTable
+}
+
+func (m browserFeatureAvailabilitySpannerMapper) SelectAll() spanner.Statement {
+	return spanner.NewStatement(fmt.Sprintf(`
 	SELECT
 		WebFeatureID, BrowserName, BrowserVersion
-	FROM %s
-	WHERE WebFeatureID = @webFeatureID AND BrowserName = @browserName
-	LIMIT 1`, m.Table()))
-	parameters := map[string]interface{}{
-		"webFeatureID": key.WebFeatureID,
-		"browserName":  key.BrowserName,
-	}
-	stmt.Params = parameters
-
-	return stmt
+	FROM %s`, m.Table()))
 }
 
-func (m browserFeatureAvailabilityMapper) GetKeyFromExternal(
-	in spannerBrowserFeatureAvailability) browserFeatureAvailabilityKey {
-	return browserFeatureAvailabilityKey{
-		WebFeatureID: in.WebFeatureID,
-		BrowserName:  in.BrowserName,
-	}
+func (m browserFeatureAvailabilitySpannerMapper) GetKeyFromExternal(in spannerBrowserFeatureAvailability) string {
+	return fmt.Sprintf("%s-%s", in.WebFeatureID, in.BrowserName)
 }
 
-func (m browserFeatureAvailabilityMapper) DeleteKey(key browserFeatureAvailabilityKey) spanner.Key {
-	return spanner.Key{key.WebFeatureID, key.BrowserName}
+func (m browserFeatureAvailabilitySpannerMapper) GetKeyFromInternal(in spannerBrowserFeatureAvailability) string {
+	return fmt.Sprintf("%s-%s", in.WebFeatureID, in.BrowserName)
 }
 
-// spannerBrowserFeatureAvailability is a wrapper for the browser availability
-// information for a feature stored in spanner.
-type spannerBrowserFeatureAvailability struct {
-	WebFeatureID string
-	BrowserFeatureAvailability
+func (m browserFeatureAvailabilitySpannerMapper) MergeAndCheckChanged(
+	in spannerBrowserFeatureAvailability, existing spannerBrowserFeatureAvailability) (
+	spannerBrowserFeatureAvailability, bool) {
+	merged := spannerBrowserFeatureAvailability{
+		WebFeatureID:   existing.WebFeatureID,
+		BrowserName:    existing.BrowserName,
+		BrowserVersion: in.BrowserVersion,
+	}
+	hasChanged := merged.BrowserVersion != existing.BrowserVersion
+
+	return merged, hasChanged
 }
 
-// BrowserFeatureAvailability contains availability information for a particular
-// feature in a browser.
-type BrowserFeatureAvailability struct {
-	BrowserName    string
-	BrowserVersion string
+func (m browserFeatureAvailabilitySpannerMapper) DeleteMutation(
+	in spannerBrowserFeatureAvailability) *spanner.Mutation {
+	return spanner.Delete(browserFeatureAvailabilitiesTable, spanner.Key{in.WebFeatureID, in.BrowserName})
 }
 
-// UpsertBrowserFeatureAvailability will upsert the given browser feature availability.
-func (c *Client) UpsertBrowserFeatureAvailability(
+// SyncBrowserFeatureAvailabilities reconciles the BrowserFeatureAvailabilities table with the provided
+// list of availabilities.
+func (c *Client) SyncBrowserFeatureAvailabilities(
 	ctx context.Context,
-	webFeatureID string,
-	input BrowserFeatureAvailability) error {
-	id, err := c.GetIDFromFeatureKey(ctx, NewFeatureKeyFilter(webFeatureID))
+	availabilities map[string][]BrowserFeatureAvailability,
+) error {
+	featureIDandKeys, err := c.FetchAllWebFeatureIDsAndKeys(ctx)
 	if err != nil {
 		return err
 	}
-	if id == nil {
-		return ErrInternalQueryFailure
+
+	featureKeyToID := make(map[string]string, len(featureIDandKeys))
+	for _, item := range featureIDandKeys {
+		featureKeyToID[item.FeatureKey] = item.ID
 	}
-	featureAvailability := spannerBrowserFeatureAvailability{
-		WebFeatureID:               *id,
-		BrowserFeatureAvailability: input,
+
+	var spannerAvailabilities []spannerBrowserFeatureAvailability
+	for featureKey, featureAvailabilities := range availabilities {
+		featureID, ok := featureKeyToID[featureKey]
+		if !ok {
+			slog.WarnContext(ctx, "unable to find feature id for feature key", "featureKey", featureKey)
+
+			continue
+		}
+		for _, availability := range featureAvailabilities {
+			spannerAvailabilities = append(spannerAvailabilities, spannerBrowserFeatureAvailability{
+				WebFeatureID:   featureID,
+				BrowserName:    availability.BrowserName,
+				BrowserVersion: availability.BrowserVersion,
+			})
+		}
 	}
 
-	return newUniqueEntityWriter[
-		browserFeatureAvailabilityMapper,
-		spannerBrowserFeatureAvailability,
-		spannerBrowserFeatureAvailability](c).upsertUniqueKey(ctx, featureAvailability)
+	synchronizer := newEntitySynchronizer[browserFeatureAvailabilitySpannerMapper](c)
+
+	return synchronizer.Sync(ctx, spannerAvailabilities)
 }
 
 func (c *Client) fetchAllBrowserAvailabilitiesWithTransaction(

lib/gcpspanner/browser_availabilities_test.go

@@ -17,49 +17,35 @@ package gcpspanner
 import (
 	"context"
 	"errors"
+	"maps"
 	"slices"
 	"testing"
 
 	"cloud.google.com/go/spanner"
 	"google.golang.org/api/iterator"
 )
 
-func getSampleBrowserAvailabilities() []struct {
-	FeatureKey string
-	BrowserFeatureAvailability
-} {
-	return []struct {
-		FeatureKey string
-		BrowserFeatureAvailability
-	}{
-		{
-			BrowserFeatureAvailability: BrowserFeatureAvailability{
+func getSampleBrowserAvailabilities() map[string][]BrowserFeatureAvailability {
+	return map[string][]BrowserFeatureAvailability{
+		"feature1": {
+			{
 				BrowserName:    "fooBrowser",
 				BrowserVersion: "0.0.0",
 			},
-			FeatureKey: "feature1",
-		},
-		{
-			BrowserFeatureAvailability: BrowserFeatureAvailability{
+			{
 				BrowserName:    "barBrowser",
 				BrowserVersion: "0.0.0",
 			},
-			FeatureKey: "feature1",
 		},
-		{
-			BrowserFeatureAvailability: BrowserFeatureAvailability{
+		"feature2": {
+			{
 				BrowserName:    "barBrowser",
 				BrowserVersion: "2.0.0",
 			},
-			FeatureKey: "feature2",
-		},
-		{
-			BrowserFeatureAvailability: BrowserFeatureAvailability{
+			{
 				BrowserName:    "fooBrowser",
 				BrowserVersion: "1.0.0",
 			},
-
-			FeatureKey: "feature2",
 		},
 	}
 }
@@ -86,7 +72,7 @@ func setupRequiredTablesForBrowserFeatureAvailability(
 // Helper method to get all the Availabilities in a stable order.
 func (c *Client) ReadAllAvailabilities(ctx context.Context, _ *testing.T) ([]BrowserFeatureAvailability, error) {
 	stmt := spanner.NewStatement(
-		"SELECT * FROM BrowserFeatureAvailabilities ORDER BY BrowserVersion ASC, BrowserName ASC")
+		"SELECT BrowserName, BrowserVersion FROM BrowserFeatureAvailabilities ORDER BY BrowserVersion ASC, BrowserName ASC")
 	iter := c.Single().Query(ctx, stmt)
 	defer iter.Stop()
 
@@ -99,27 +85,25 @@ func (c *Client) ReadAllAvailabilities(ctx context.Context, _ *testing.T) ([]Bro
 		if err != nil {
 			return nil, errors.Join(ErrInternalQueryFailure, err)
 		}
-		var availability spannerBrowserFeatureAvailability
+		var availability BrowserFeatureAvailability
 		if err := row.ToStruct(&availability); err != nil {
 			return nil, errors.Join(ErrInternalQueryFailure, err)
 		}
-		ret = append(ret, availability.BrowserFeatureAvailability)
+		ret = append(ret, availability)
 	}
 
 	return ret, nil
 }
 
-func TestUpsertBrowserFeatureAvailability(t *testing.T) {
+func TestSyncBrowserFeatureAvailabilities(t *testing.T) {
 	restartDatabaseContainer(t)
 	ctx := context.Background()
 	setupRequiredTablesForBrowserFeatureAvailability(ctx, spannerClient, t)
 	sampleAvailabilities := getSampleBrowserAvailabilities()
-	for _, availability := range sampleAvailabilities {
-		err := spannerClient.UpsertBrowserFeatureAvailability(
-			ctx, availability.FeatureKey, availability.BrowserFeatureAvailability)
-		if err != nil {
-			t.Errorf("unexpected error during insert. %s", err.Error())
-		}
+	err := spannerClient.SyncBrowserFeatureAvailabilities(
+		ctx, sampleAvailabilities)
+	if err != nil {
+		t.Errorf("unexpected error during insert. %s", err.Error())
 	}
 
 	expectedPage := []BrowserFeatureAvailability{
@@ -151,10 +135,19 @@ func TestUpsertBrowserFeatureAvailability(t *testing.T) {
 	}
 
 	// Update the availability info for feature1 on barBrowser to a later version
-	err = spannerClient.UpsertBrowserFeatureAvailability(ctx, "feature1", BrowserFeatureAvailability{
-		BrowserName:    "barBrowser",
-		BrowserVersion: "1.0.0",
-	})
+	updatedAvailabilities := maps.Clone(sampleAvailabilities)
+	updatedAvailabilities["feature1"] = []BrowserFeatureAvailability{
+		{
+			BrowserName:    "fooBrowser",
+			BrowserVersion: "0.0.0",
+		},
+		{
+			BrowserName:    "barBrowser",
+			BrowserVersion: "1.0.0",
+		},
+	}
+
+	err = spannerClient.SyncBrowserFeatureAvailabilities(ctx, updatedAvailabilities)
 	if err != nil {
 		t.Errorf("unexpected error during update. %s", err.Error())
 	}
@@ -186,4 +179,31 @@ func TestUpsertBrowserFeatureAvailability(t *testing.T) {
 	if !slices.Equal(expectedPage, availabilities) {
 		t.Errorf("unequal availabilities.\nexpected %+v\nreceived %+v", expectedPage, availabilities)
 	}
+
+	// Remove the availability info for feature1 on barBrowser
+	regressedAvailabilities := maps.Clone(updatedAvailabilities)
+	delete(regressedAvailabilities, "feature1")
+
+	err = spannerClient.SyncBrowserFeatureAvailabilities(ctx, regressedAvailabilities)
+	if err != nil {
+		t.Errorf("unexpected error during update. %s", err.Error())
+	}
+
+	expectedPage = []BrowserFeatureAvailability{
+		{
+			BrowserName:    "fooBrowser",
+			BrowserVersion: "1.0.0",
+		},
+		{
+			BrowserName:    "barBrowser",
+			BrowserVersion: "2.0.0",
+		},
+	}
+	availabilities, err = spannerClient.ReadAllAvailabilities(ctx, t)
+	if err != nil {
+		t.Errorf("unexpected error during read all. %s", err.Error())
+	}
+	if !slices.Equal(expectedPage, availabilities) {
+		t.Errorf("unequal availabilities.\nexpected %+v\nreceived %+v", expectedPage, availabilities)
+	}
 }