Use searchcache results if summary file is old format

api/README.md

@@ -71,7 +71,7 @@ __`pr`__ (Beta): GitHub PR number. Shows runs for commits that belong to the PR.
         "os_version": "4.4",
         "revision": "2bd11b91d4",
         "full_revision_hash": "2bd11b91d490ddd5237bcb6d8149a7f25faaa101",
-        "results_url": "https://storage.googleapis.com/wptd/2bd11b91d4/chrome-stable-linux-summary.json.gz",
+        "results_url": "https://storage.googleapis.com/wptd/2bd11b91d4/chrome-stable-linux-summary_v2.json.gz",
         "created_at": "2018-06-05T08:27:30.627865Z",
         "raw_results_url": "https://storage.googleapis.com/wptd-results/2bd11b91d490ddd5237bcb6d8149a7f25faaa101/chrome_67.0.3396.62_linux_4.4/report.json"
       }
@@ -97,7 +97,7 @@ https://wpt.fyi/api/runs/5184362994728960
       "os_version": "4.4",
       "revision": "2bd11b91d4",
       "full_revision_hash": "2bd11b91d490ddd5237bcb6d8149a7f25faaa101",
-      "results_url": "https://storage.googleapis.com/wptd/2bd11b91d4/chrome-stable-linux-summary.json.gz",
+      "results_url": "https://storage.googleapis.com/wptd/2bd11b91d4/chrome-stable-linux-summary_v2.json.gz",
       "created_at": "2018-06-05T08:27:30.627865Z",
       "raw_results_url": "https://storage.googleapis.com/wptd-results/2bd11b91d490ddd5237bcb6d8149a7f25faaa101/chrome_67.0.3396.62_linux_4.4/report.json"
     }
@@ -128,7 +128,7 @@ https://wpt.fyi/api/run?sha=latest&product=chrome
       "os_version": "4.4",
       "revision": "2bd11b91d4",
       "full_revision_hash": "2bd11b91d490ddd5237bcb6d8149a7f25faaa101",
-      "results_url": "https://storage.googleapis.com/wptd/2bd11b91d4/chrome-stable-linux-summary.json.gz",
+      "results_url": "https://storage.googleapis.com/wptd/2bd11b91d4/chrome-stable-linux-summary_v2.json.gz",
       "created_at": "2018-06-05T08:27:30.627865Z",
       "raw_results_url": "https://storage.googleapis.com/wptd-results/2bd11b91d490ddd5237bcb6d8149a7f25faaa101/chrome_67.0.3396.62_linux_4.4/report.json"
     }
@@ -196,7 +196,7 @@ __`sha`__ : SHA[0:10] of the TestRun to fetch, or the keyword `latest`. Defaults
 
 https://wpt.fyi/api/results?product=chrome
 
-<details><summary><b>Example JSON</b> (from the summary.json.gz output)</summary>
+<details><summary><b>Example JSON</b> (from the summary_v2.json.gz output)</summary>
 
     {
       "/css/css-text/i18n/css3-text-line-break-opclns-213.html": [1, 1],
@@ -406,7 +406,7 @@ NOTE: structured search queries are not supported.
       "os_version": "16.04",
       "revision": "2dda7b8c10",
       "full_revision_hash": "2dda7b8c10c7566fa6167a32b09c85d51baf2a85",
-      "results_url": "https:\/\/storage.googleapis.com\/wptd-staging\/2dda7b8c10c7566fa6167a32b09c85d51baf2a85\/chrome-68.0.3440.106-linux-16.04-edf200244e-summary.json.gz",
+      "results_url": "https:\/\/storage.googleapis.com\/wptd-staging\/2dda7b8c10c7566fa6167a32b09c85d51baf2a85\/chrome-68.0.3440.106-linux-16.04-edf200244e-summary_v2.json.gz",
       "created_at": "2018-08-17T08:12:29.219847Z",
       "time_start": "2018-08-17T06:26:52.33Z",
       "time_end": "2018-08-17T07:50:09.155Z",
@@ -425,7 +425,7 @@ NOTE: structured search queries are not supported.
       "os_version": "16.04",
       "revision": "2dda7b8c10",
       "full_revision_hash": "2dda7b8c10c7566fa6167a32b09c85d51baf2a85",
-      "results_url": "https:\/\/storage.googleapis.com\/wptd-staging\/2dda7b8c10c7566fa6167a32b09c85d51baf2a85\/firefox-61.0.2-linux-16.04-75ff911c43-summary.json.gz",
+      "results_url": "https:\/\/storage.googleapis.com\/wptd-staging\/2dda7b8c10c7566fa6167a32b09c85d51baf2a85\/firefox-61.0.2-linux-16.04-75ff911c43-summary_v2.json.gz",
       "created_at": "2018-08-17T08:31:38.580221Z",
       "time_start": "2018-08-17T06:47:29.643Z",
       "time_end": "2018-08-17T08:15:18.612Z",

api/query/query.go

@@ -8,13 +8,16 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http"
+	"net/url"
 	"strconv"
+	"strings"
 	"sync"
 
 	"github.com/web-platform-tests/wpt.fyi/shared"
 )
 
-// SummaryResult is the format of the data from summary files generated with the newest aggregation method.
+// SummaryResult is the format of the data from summary files generated with the
+// newest aggregation method.
 type SummaryResult struct {
 	// Status represents the 1-2 character abbreviation for the status of the test.
 	Status string `json:"s"`
@@ -23,22 +26,23 @@ type SummaryResult struct {
 }
 
 // summary is the golang type for the JSON format in pass/total summary files.
-// It has an old structure and a new structure - each which represent summary files
-// that match the old or new summary format.
-type summary struct {
-	// oldFormat This holds summary information if the data is aggregated with the old method.
-	// TODO (danielrsmith): This format should be removed once old summary files are invalidated.
-	oldFormat map[string][]int
-	// newFormat This holds summary information if the data is aggregated with the new method.
-	newFormat map[string]SummaryResult
-}
+type summary map[string]SummaryResult
+
 type queryHandler struct {
 	store      shared.Datastore
 	dataSource shared.CachedStore
 	client     *http.Client
 	logger     shared.Logger
 }
 
+// SummaryError holds information on whether a summary was found as valid or not.
+type SummaryError struct {
+	// BadVersion represents whether the summary has a valid URL.
+	BadVersion bool
+	// Err is the error that occurred while trying to check the summary file URL.
+	Err error
+}
+
 func (qh queryHandler) processInput(w http.ResponseWriter, r *http.Request) (*shared.QueryFilter, shared.TestRuns, []summary, error) {
 	filters, err := shared.ParseQueryFilterParams(r.URL.Query())
 	if err != nil {
@@ -61,6 +65,31 @@ func (qh queryHandler) processInput(w http.ResponseWriter, r *http.Request) (*sh
 	return &filters, testRuns, summaries, nil
 }
 
+func (qh queryHandler) validateSummaryVersions(v url.Values, logger shared.Logger) SummaryError {
+	filters, err := shared.ParseQueryFilterParams(v)
+	if err != nil {
+		return SummaryError{BadVersion: false, Err: err}
+	}
+	testRuns, _, err := qh.getRunsAndFilters(filters)
+	if err != nil {
+		return SummaryError{BadVersion: false, Err: err}
+	}
+
+	for _, testRun := range testRuns {
+		summaryURL := shared.GetResultsURL(testRun, "")
+		if !qh.summaryIsValid(summaryURL) {
+			logger.Infof("summary URL has invalid suffix: %s", summaryURL)
+			return SummaryError{BadVersion: true, Err: nil}
+		}
+	}
+	return SummaryError{BadVersion: false, Err: nil}
+}
+
+func (qh queryHandler) summaryIsValid(summaryURL string) bool {
+	// All new summary URLs end with "-summary_v2.json.gz". Any others are invalid.
+	return strings.HasSuffix(summaryURL, "-summary_v2.json.gz")
+}
+
 func (qh queryHandler) getRunsAndFilters(in shared.QueryFilter) (shared.TestRuns, shared.QueryFilter, error) {
 	filters := in
 	var testRuns shared.TestRuns
@@ -108,25 +137,17 @@ func (qh queryHandler) loadSummaries(testRuns shared.TestRuns) ([]summary, error
 			defer wg.Done()
 
 			var data []byte
-			s := summary{
-				oldFormat: nil,
-				newFormat: nil,
-			}
+			s := summary{}
 			data, loadErr := qh.loadSummary(testRun)
 			if err == nil && loadErr != nil {
 				err = fmt.Errorf("Failed to load test run %v: %s", testRun.ID, loadErr.Error())
 				return
 			}
 			// Try to unmarshal the json using the new aggregation structure.
-			marshalErr := json.Unmarshal(data, &s.newFormat)
+			marshalErr := json.Unmarshal(data, &s)
 			if err == nil && marshalErr != nil {
-				// If that failed, this is likely an old summary format.
-				// Umarshal using the old structure.
-				oldMarshalErr := json.Unmarshal(data, &s.oldFormat)
-				if oldMarshalErr != nil {
-					err = oldMarshalErr
-					return
-				}
+				err = marshalErr
+				return
 			}
 			summaries[i] = s
 		}(i, testRun)
@@ -137,15 +158,15 @@ func (qh queryHandler) loadSummaries(testRuns shared.TestRuns) ([]summary, error
 }
 
 func (qh queryHandler) loadSummary(testRun shared.TestRun) ([]byte, error) {
-	mkey := getRedisKey(testRun)
+	mkey := getSummaryFileRedisKey(testRun)
 	url := shared.GetResultsURL(testRun, "")
 	var data []byte
 	err := qh.dataSource.Get(mkey, url, &data)
 	return data, err
 }
 
-func getRedisKey(testRun shared.TestRun) string {
-	return "RESULTS_SUMMARY-" + strconv.FormatInt(testRun.ID, 10)
+func getSummaryFileRedisKey(testRun shared.TestRun) string {
+	return "RESULTS_SUMMARY_v2-" + strconv.FormatInt(testRun.ID, 10)
 }
 
 func isRequestCacheable(r *http.Request) bool {

api/query/query_test.go

@@ -20,18 +20,18 @@ import (
 )
 
 func TestGetRedisKey(t *testing.T) {
-	assert.Equal(t, "RESULTS_SUMMARY-1", getRedisKey(shared.TestRun{
+	assert.Equal(t, "RESULTS_SUMMARY_v2-1", getSummaryFileRedisKey(shared.TestRun{
 		ID: 1,
 	}))
 }
 
-func TestLoadOldSummaries_success(t *testing.T) {
+func TestLoadSummaries_success(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	defer mockCtrl.Finish()
 
 	urls := []string{
-		"https://example.com/1-summary.json.gz",
-		"https://example.com/2-summary.json.gz",
+		"https://example.com/1-summary_v2.json.gz",
+		"https://example.com/2-summary_v2.json.gz",
 	}
 	testRuns := []shared.TestRun{
 		{
@@ -44,64 +44,8 @@ func TestLoadOldSummaries_success(t *testing.T) {
 		},
 	}
 	keys := []string{
-		getRedisKey(testRuns[0]),
-		getRedisKey(testRuns[1]),
-	}
-
-	cachedStore := sharedtest.NewMockCachedStore(mockCtrl)
-	sh := unstructuredSearchHandler{queryHandler{dataSource: cachedStore}}
-	summaryBytes := [][]byte{
-		[]byte(`{"/a/b/c":[1,2]}`),
-		[]byte(`{"/x/y/z":[3,4]}`),
-	}
-	summaries := []summary{
-		{
-			oldFormat: map[string][]int{"/a/b/c": {1, 2}},
-			newFormat: map[string]SummaryResult{"/a/b/c": {Status: "", Counts: []int(nil)}},
-		},
-		{
-			oldFormat: map[string][]int{"/x/y/z": {3, 4}},
-			newFormat: map[string]SummaryResult{"/x/y/z": {Status: "", Counts: []int(nil)}},
-		},
-	}
-
-	bindCopySlice := func(i int) func(_, _, _ interface{}) {
-		return func(cid, sid, iv interface{}) {
-			ptr := iv.(*[]byte)
-			*ptr = summaryBytes[i]
-		}
-	}
-	for i, key := range keys {
-		cachedStore.EXPECT().Get(key, urls[i], gomock.Any()).Do(bindCopySlice(i)).Return(nil)
-	}
-
-	ss, err := sh.loadSummaries(testRuns)
-	assert.Nil(t, err)
-	assert.Equal(t, summaries[0], ss[0])
-	assert.Equal(t, summaries[1], ss[1])
-}
-
-func TestLoadNewSummaries_success(t *testing.T) {
-	mockCtrl := gomock.NewController(t)
-	defer mockCtrl.Finish()
-
-	urls := []string{
-		"https://example.com/1-summary.json.gz",
-		"https://example.com/2-summary.json.gz",
-	}
-	testRuns := []shared.TestRun{
-		{
-			ID:         1,
-			ResultsURL: urls[0],
-		},
-		{
-			ID:         2,
-			ResultsURL: urls[1],
-		},
-	}
-	keys := []string{
-		getRedisKey(testRuns[0]),
-		getRedisKey(testRuns[1]),
+		getSummaryFileRedisKey(testRuns[0]),
+		getSummaryFileRedisKey(testRuns[1]),
 	}
 
 	cachedStore := sharedtest.NewMockCachedStore(mockCtrl)
@@ -111,14 +55,8 @@ func TestLoadNewSummaries_success(t *testing.T) {
 		[]byte(`{"/x/y/z":{"s":"E","c":[3,4]}}`),
 	}
 	summaries := []summary{
-		{
-			oldFormat: nil,
-			newFormat: map[string]SummaryResult{"/a/b/c": {Status: "O", Counts: []int{1, 2}}},
-		},
-		{
-			oldFormat: nil,
-			newFormat: map[string]SummaryResult{"/x/y/z": {Status: "E", Counts: []int{3, 4}}},
-		},
+		map[string]SummaryResult{"/a/b/c": {Status: "O", Counts: []int{1, 2}}},
+		map[string]SummaryResult{"/x/y/z": {Status: "E", Counts: []int{3, 4}}},
 	}
 
 	bindCopySlice := func(i int) func(_, _, _ interface{}) {
@@ -142,8 +80,8 @@ func TestLoadSummaries_fail(t *testing.T) {
 	defer mockCtrl.Finish()
 
 	urls := []string{
-		"https://example.com/1-summary.json.gz",
-		"https://example.com/2-summary.json.gz",
+		"https://example.com/1-summary_v2.json.gz",
+		"https://example.com/2-summary_v2.json.gz",
 	}
 	testRuns := []shared.TestRun{
 		{
@@ -156,14 +94,14 @@ func TestLoadSummaries_fail(t *testing.T) {
 		},
 	}
 	keys := []string{
-		getRedisKey(testRuns[0]),
-		getRedisKey(testRuns[1]),
+		getSummaryFileRedisKey(testRuns[0]),
+		getSummaryFileRedisKey(testRuns[1]),
 	}
 
 	cachedStore := sharedtest.NewMockCachedStore(mockCtrl)
 	sh := unstructuredSearchHandler{queryHandler{dataSource: cachedStore}}
 	summaryBytes := [][]byte{
-		[]byte(`{"/a/b/c":[1,2]}`),
+		[]byte(`{"/a/b/c":{"s":"O","c":[1,2]}}`),
 	}
 
 	storeMiss := errors.New("No such summary file")
@@ -177,6 +115,19 @@ func TestLoadSummaries_fail(t *testing.T) {
 	assert.Contains(t, err.Error(), storeMiss.Error())
 }
 
+func TestSummaryIsValid_v1(t *testing.T) {
+	qh := queryHandler{}
+	// Summaries without the "_v2" suffix should not be used.
+	url := "https://example.com/invalid-summary.json.gz"
+	assert.False(t, qh.summaryIsValid(url))
+}
+
+func TestSummaryIsValid_v2(t *testing.T) {
+	qh := queryHandler{}
+	url := "https://example.com/valid-summary_v2.json.gz"
+	assert.True(t, qh.summaryIsValid(url))
+}
+
 func TestGetRunsAndFilters_default(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	defer mockCtrl.Finish()
@@ -190,8 +141,8 @@ func TestGetRunsAndFilters_default(t *testing.T) {
 
 	runIDs := []int64{1, 2}
 	urls := []string{
-		"https://example.com/1-summary.json.gz",
-		"https://example.com/2-summary.json.gz",
+		"https://example.com/1-summary_v2.json.gz",
+		"https://example.com/2-summary_v2.json.gz",
 	}
 	chrome, _ := shared.ParseProductSpec("chrome")
 	edge, _ := shared.ParseProductSpec("edge")
@@ -240,8 +191,8 @@ func TestGetRunsAndFilters_specificRunIDs(t *testing.T) {
 
 	runIDs := []int64{1, 2}
 	urls := []string{
-		"https://example.com/1-summary.json.gz",
-		"https://example.com/2-summary.json.gz",
+		"https://example.com/1-summary_v2.json.gz",
+		"https://example.com/2-summary_v2.json.gz",
 	}
 	chrome, _ := shared.ParseProductSpec("chrome")
 	edge, _ := shared.ParseProductSpec("edge")