feat: Add version-based data parsing (#1746)

* feat: Add version-based data parsing

This commit introduces the capability to parse data files from GitHub releases based on their semantic version tag. This is crucial for handling evolving data schemas from upstream sources like `web-features`.

Key changes:

-   **`lib/gh`**: `DownloadFileFromRelease` now returns a `*gh.File` struct containing both the file contents and the release's semver tag.

-   **`web_feature_consumer`**: A new `parseByVersion` method uses the release tag to select the appropriate parser. It currently supports routing to V2 and V3 parsers for `web-features` data, ensuring backward compatibility and readiness for the new schema.

-   **`bcd_consumer`**: Updated to use the new `*gh.File` struct, though it does not yet perform version-based parsing.

-   **Testing**: New tests have been added to validate the version-routing logic in the `web_feature_consumer`.

* address feedback

Author: jcscottiii

lib/gh/download_file_from_release.go

@@ -18,9 +18,12 @@ import (
 	"context"
 	"errors"
 	"io"
+	"log/slog"
 	"net/http"
+	"strings"
 
 	"github.com/google/go-github/v73/github"
+	"golang.org/x/mod/semver"
 )
 
 var (
@@ -31,11 +34,22 @@ var (
 	ErrFatalError            = errors.New("fatal error using github")
 )
 
+// ReleaseFile represents a file in a given Github release.
+type ReleaseFile struct {
+	Contents io.ReadCloser
+	Info     ReleaseInfo
+}
+
+type ReleaseInfo struct {
+	// If the tag is valid, the will be non null.
+	Tag *string
+}
+
 func (c *Client) DownloadFileFromRelease(
 	ctx context.Context,
 	owner, repo string,
 	httpClient *http.Client,
-	filePattern string) (io.ReadCloser, error) {
+	filePattern string) (*ReleaseFile, error) {
 	release, _, err := c.repoClient.GetLatestRelease(ctx, owner, repo)
 	if err != nil {
 		// nolint: exhaustruct // WONTFIX. This is an external package. Cannot control it.
@@ -84,5 +98,24 @@ func (c *Client) DownloadFileFromRelease(
 		return nil, errors.Join(ErrUnableToDownloadAsset, err)
 	}
 
-	return resp.Body, nil
+	// Returns a tag or empty string if not found.
+	tagName := release.GetTagName()
+	// In the event the tag is missing the prefix "v", add it.
+	// According https://pkg.go.dev/golang.org/x/mod/semver, the version must start with v
+	if len(tagName) > 0 && !strings.HasPrefix(tagName, "v") {
+		tagName = "v" + tagName
+	}
+	var tagNamePtr *string
+	if semver.IsValid(tagName) {
+		tagNamePtr = &tagName
+	} else {
+		slog.WarnContext(ctx, "invalid tag. it will not be used", "tag", tagName)
+	}
+
+	return &ReleaseFile{
+		Contents: resp.Body,
+		Info: ReleaseInfo{
+			Tag: tagNamePtr,
+		},
+	}, nil
 }

lib/gh/download_file_from_release_test.go

@@ -21,6 +21,7 @@ import (
 	"net/http"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-github/v73/github"
 )
 
@@ -34,6 +35,12 @@ func checkIfFileIsReadable(t *testing.T, file io.Reader) {
 	}
 }
 
+func checkIfTagIsPopulated(t *testing.T, file *ReleaseFile) {
+	if file.Info.Tag == nil || *file.Info.Tag == "" {
+		t.Error("tag is empty")
+	}
+}
+
 func TestDownloadFileFromReleaseWebFeatures(t *testing.T) {
 	t.Skip("Used for debugging purposes.")
 	client := NewClient("")
@@ -44,14 +51,14 @@ func TestDownloadFileFromReleaseWebFeatures(t *testing.T) {
 		t.Errorf("unexpected error: %s", err.Error())
 	} else {
 		// Close to be safe at the end.
-		defer file.Close()
-		checkIfFileIsReadable(t, file)
+		defer file.Contents.Close()
+		checkIfFileIsReadable(t, file.Contents)
+		checkIfTagIsPopulated(t, file)
 	}
 }
 
 func TestDownloadFileFromReleaseBrowserCompatData(t *testing.T) {
 	t.Skip("Used for debugging purposes.")
-	t.Skip("Cannot remove until https://github.com/mdn/browser-compat-data/issues/22675 is fixed")
 	client := NewClient("")
 	ctx := context.Background()
 	httpClient := http.DefaultClient
@@ -60,8 +67,9 @@ func TestDownloadFileFromReleaseBrowserCompatData(t *testing.T) {
 		t.Errorf("unexpected error: %s", err.Error())
 	} else {
 		// Close to be safe at the end.
-		defer file.Close()
-		checkIfFileIsReadable(t, file)
+		defer file.Contents.Close()
+		checkIfFileIsReadable(t, file.Contents)
+		checkIfTagIsPopulated(t, file)
 	}
 }
 
@@ -113,6 +121,7 @@ func TestMockDownloadFileFromRelease(t *testing.T) {
 		cfg           mockGetLatestReleaseConfig
 		roundTripCfg  *mockRoundTripperConfig
 		expectedError error
+		expectedFile  *ReleaseFile
 	}{
 		{
 			name: "successful download",
@@ -127,6 +136,7 @@ func TestMockDownloadFileFromRelease(t *testing.T) {
 							BrowserDownloadURL: valuePtr("http://example.com/file.txt"),
 						},
 					},
+					TagName: valuePtr("v1.0.0"),
 				},
 				err: nil,
 			},
@@ -135,10 +145,51 @@ func TestMockDownloadFileFromRelease(t *testing.T) {
 				//nolint: exhaustruct
 				resp: &http.Response{
 					StatusCode: http.StatusOK,
+					Body:       io.NopCloser(nil),
 				},
 				err: nil,
 			},
 			expectedError: nil,
+			expectedFile: &ReleaseFile{
+				Contents: io.NopCloser(nil),
+				Info: ReleaseInfo{
+					Tag: valuePtr("v1.0.0"),
+				},
+			},
+		},
+		{
+			name: "successful download without leading v in tag",
+			cfg: mockGetLatestReleaseConfig{
+				expectedOwner: "owner",
+				expectedRepo:  "repo",
+				//nolint: exhaustruct
+				release: &github.RepositoryRelease{
+					Assets: []*github.ReleaseAsset{
+						{
+							Name:               valuePtr("file.txt"),
+							BrowserDownloadURL: valuePtr("http://example.com/file.txt"),
+						},
+					},
+					TagName: valuePtr("2.0.0"),
+				},
+				err: nil,
+			},
+			roundTripCfg: &mockRoundTripperConfig{
+				expectedURL: "http://example.com/file.txt",
+				//nolint: exhaustruct
+				resp: &http.Response{
+					StatusCode: http.StatusOK,
+					Body:       io.NopCloser(nil),
+				},
+				err: nil,
+			},
+			expectedError: nil,
+			expectedFile: &ReleaseFile{
+				Contents: io.NopCloser(nil),
+				Info: ReleaseInfo{
+					Tag: valuePtr("v2.0.0"),
+				},
+			},
 		},
 		{
 			name: "rate limit with github api",
@@ -151,6 +202,7 @@ func TestMockDownloadFileFromRelease(t *testing.T) {
 			},
 			roundTripCfg:  nil,
 			expectedError: ErrRateLimit,
+			expectedFile:  nil,
 		},
 		{
 			name: "unknown error with github api",
@@ -162,6 +214,7 @@ func TestMockDownloadFileFromRelease(t *testing.T) {
 			},
 			roundTripCfg:  nil,
 			expectedError: ErrFatalError,
+			expectedFile:  nil,
 		},
 		{
 			name: "missing asset",
@@ -174,6 +227,7 @@ func TestMockDownloadFileFromRelease(t *testing.T) {
 			},
 			roundTripCfg:  nil,
 			expectedError: ErrAssetNotFound,
+			expectedFile:  nil,
 		},
 		{
 			name: "request.DO() fails",
@@ -198,6 +252,7 @@ func TestMockDownloadFileFromRelease(t *testing.T) {
 				err:  errors.New("something went wrong"),
 			},
 			expectedError: ErrUnableToDownloadAsset,
+			expectedFile:  nil,
 		},
 		{
 			name: "failed to download",
@@ -224,6 +279,7 @@ func TestMockDownloadFileFromRelease(t *testing.T) {
 				err: nil,
 			},
 			expectedError: ErrUnableToDownloadAsset,
+			expectedFile:  nil,
 		},
 	}
 	for _, tc := range testCases {
@@ -241,7 +297,7 @@ func TestMockDownloadFileFromRelease(t *testing.T) {
 
 			httpClient := http.DefaultClient
 			httpClient.Transport = &rt
-			_, err := client.DownloadFileFromRelease(
+			file, err := client.DownloadFileFromRelease(
 				context.Background(),
 				"owner",
 				"repo",
@@ -251,6 +307,9 @@ func TestMockDownloadFileFromRelease(t *testing.T) {
 			if !errors.Is(err, tc.expectedError) {
 				t.Errorf("unexpected error expected: %v received: %v", tc.expectedError, err)
 			}
+			if diff := cmp.Diff(tc.expectedFile, file); diff != "" {
+				t.Errorf("unexpected file (-want +got):\n%s", diff)
+			}
 		})
 	}
 }

lib/go.mod

@@ -68,6 +68,7 @@ require (
 	go.opentelemetry.io/otel/exporters/stdout/stdoutlog v0.13.0 // indirect
 	go.opentelemetry.io/otel/log v0.13.0 // indirect
 	go.opentelemetry.io/otel/sdk/log v0.13.0 // indirect
+	golang.org/x/mod v0.27.0 // indirect
 	google.golang.org/appengine/v2 v2.0.6 // indirect
 )
 

lib/go.sum

@@ -1253,6 +1253,8 @@ golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91
 golang.org/x/mod v0.7.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs=
 golang.org/x/mod v0.8.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs=
 golang.org/x/mod v0.9.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs=
+golang.org/x/mod v0.27.0 h1:kb+q2PyFnEADO2IEF935ehFUXlWiNjJWtRNgBLSfbxQ=
+golang.org/x/mod v0.27.0/go.mod h1:rWI627Fq0DEoudcK+MBkNkCe0EetEaDSwJJkCcjpazc=
 golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
 golang.org/x/net v0.0.0-20180826012351-8a410e7b638d/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
 golang.org/x/net v0.0.0-20190108225652-1e06a53dbb7e/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=

workflows/steps/services/bcd_consumer/pkg/workflow/bcd_releases_worker.go

@@ -20,6 +20,7 @@ import (
 	"net/http"
 
 	"github.com/GoogleChrome/webstatus.dev/lib/gcpspanner/spanneradapters/bcdconsumertypes"
+	"github.com/GoogleChrome/webstatus.dev/lib/gh"
 	"github.com/GoogleChrome/webstatus.dev/workflows/steps/services/bcd_consumer/pkg/data"
 )
 
@@ -45,7 +46,7 @@ type DataGetter interface {
 		ctx context.Context,
 		owner, repo string,
 		httpClient *http.Client,
-		filePattern string) (io.ReadCloser, error)
+		filePattern string) (*gh.ReleaseFile, error)
 }
 
 // DataParser describes the behavior to read raw bytes into the expected BCDData struct.
@@ -106,7 +107,7 @@ func (p BCDJobProcessor) Process(
 	}
 
 	// Step 2. Parse the file.
-	data, err := p.dataParser.Parse(file)
+	data, err := p.dataParser.Parse(file.Contents)
 	if err != nil {
 		return err
 	}

workflows/steps/services/bcd_consumer/pkg/workflow/bcd_releases_worker_test.go

@@ -27,6 +27,7 @@ import (
 
 	"github.com/GoogleChrome/webstatus.dev/lib/gcpspanner/spanneradapters/bcdconsumertypes"
 	"github.com/GoogleChrome/webstatus.dev/lib/gen/jsonschema/mdn__browser_compat_data"
+	"github.com/GoogleChrome/webstatus.dev/lib/gh"
 	"github.com/GoogleChrome/webstatus.dev/workflows/steps/services/bcd_consumer/pkg/data"
 )
 
@@ -56,7 +57,7 @@ type mockDownloadFileFromReleaseConfig struct {
 	repoOwner   string
 	repoName    string
 	filePattern string
-	fakeFile    io.ReadCloser
+	fakeFile    *gh.ReleaseFile
 	err         error
 }
 
@@ -69,7 +70,7 @@ func (m *MockDataGetter) DownloadFileFromRelease(
 	_ context.Context,
 	owner, repo string,
 	_ *http.Client,
-	filePattern string) (io.ReadCloser, error) {
+	filePattern string) (*gh.ReleaseFile, error) {
 	if m.mockDownloadFileFromReleaseCfg.repoOwner != owner ||
 		m.mockDownloadFileFromReleaseCfg.repoName != repo ||
 		m.mockDownloadFileFromReleaseCfg.filePattern != filePattern {
@@ -97,7 +98,8 @@ func (m *MockDataParser) Parse(in io.ReadCloser) (*data.BCDData, error) {
 		m.t.Errorf("unable to read file")
 	}
 	if m.mockParseCfg.expectedFileContents != string(fileContents) {
-		m.t.Error("unexpected file contents")
+		m.t.Errorf("unexpected file contents. want: %s, got: %s",
+			m.mockParseCfg.expectedFileContents, string(fileContents))
 	}
 
 	return m.mockParseCfg.ret, m.mockParseCfg.err
@@ -205,6 +207,16 @@ func getSampleReleases() []bcdconsumertypes.BrowserRelease {
 }
 
 func TestProcess(t *testing.T) {
+	// Create a function to generate a file because Contents can only be read once
+	testFileFn := func() *gh.ReleaseFile {
+		return &gh.ReleaseFile{
+			Contents: io.NopCloser(strings.NewReader("success")),
+			Info: gh.ReleaseInfo{
+				Tag: nil,
+			},
+		}
+	}
+
 	testCases := []processWorkflowTest{
 		{
 			name: "successful process",
@@ -215,7 +227,7 @@ func TestProcess(t *testing.T) {
 				repoOwner:   repoOwner,
 				repoName:    repoName,
 				filePattern: filePattern,
-				fakeFile:    io.NopCloser(strings.NewReader("success")),
+				fakeFile:    testFileFn(),
 				err:         nil,
 			},
 			mockParseCfg: &mockParseConfig{
@@ -244,7 +256,7 @@ func TestProcess(t *testing.T) {
 				repoOwner:   repoOwner,
 				repoName:    repoName,
 				filePattern: filePattern,
-				fakeFile:    io.NopCloser(strings.NewReader("success")),
+				fakeFile:    testFileFn(),
 				err:         errTestGetter,
 			},
 			mockParseCfg:                 nil,
@@ -261,7 +273,7 @@ func TestProcess(t *testing.T) {
 				repoOwner:   repoOwner,
 				repoName:    repoName,
 				filePattern: filePattern,
-				fakeFile:    io.NopCloser(strings.NewReader("success")),
+				fakeFile:    testFileFn(),
 				err:         nil,
 			},
 			mockParseCfg: &mockParseConfig{
@@ -282,7 +294,7 @@ func TestProcess(t *testing.T) {
 				repoOwner:   repoOwner,
 				repoName:    repoName,
 				filePattern: filePattern,
-				fakeFile:    io.NopCloser(strings.NewReader("success")),
+				fakeFile:    testFileFn(),
 				err:         nil,
 			},
 			mockParseCfg: &mockParseConfig{
@@ -308,7 +320,7 @@ func TestProcess(t *testing.T) {
 				repoOwner:   repoOwner,
 				repoName:    repoName,
 				filePattern: filePattern,
-				fakeFile:    io.NopCloser(strings.NewReader("success")),
+				fakeFile:    testFileFn(),
 				err:         nil,
 			},
 			mockParseCfg: &mockParseConfig{

workflows/steps/services/web_feature_consumer/cmd/job/main.go

@@ -92,6 +92,7 @@ func main() {
 		spanneradapters.NewWebFeatureGroupsConsumer(spannerClient),
 		spanneradapters.NewWebFeatureSnapshotsConsumer(spannerClient),
 		data.Parser{},
+		data.V3Parser{},
 	)
 
 	// Job Generation