Avoid unnecessary HTTP requests for latest/last_rc (#641)

* Avoid unnecessary HTTP requests for latest/last_rc

The refactoring in #631 introduced a severe performance regression: With latest and last_rc the code traversed *all* GCS buckets for *every* existing Bazel version.
Since we send one HTTP request per bucket, this behavior led to a significant increase in HTTP requests ( >140 instead of 2-3 requests).

This commit restores the previous, correct behavior: Traversal will be stopped as soon as a matching version has been found.

Moreover, this commit adds a test to prevent similar regressions in the future.

Fixes #640

Drive-by fix: Replaced \"%s\" with %q.

* Apply minimal indentation principle

Author: fweikert

bazelisk_version_test.go

@@ -164,6 +164,58 @@ func TestResolveLatestVersion_ShouldOnlyReturnStableReleases(t *testing.T) {
 	}
 }
 
+func TestResolveLatestAvoidsUnnecessaryRequests(t *testing.T) {
+	tests := []struct{
+		name string
+		specifiedVersion string
+		wantVersion string
+	} {
+		{
+			name: "Release",
+			specifiedVersion: "latest",
+			wantVersion: "7.0.0",
+		},
+		{
+			name: "Candidate",
+			specifiedVersion: "last_rc",
+			wantVersion: "7.0.0rc1",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T){
+		s := setUp(t)
+		s.AddVersion("4.0.0", true, []int{1}, nil)
+		s.AddVersion("5.0.0", true, []int{1}, nil)
+		s.AddVersion("6.0.0", true, []int{1}, nil)
+		s.AddVersion("7.0.0", true, []int{1}, nil)
+		s.AddVersion("8.0.0", false, nil, nil)
+		s.Finish()
+
+		gcs := &repositories.GCSRepo{}
+		repos := core.CreateRepositories(gcs, nil, nil, nil, false)
+		gotVersion, _, err := repos.ResolveVersion(tmpDir, versions.BazelUpstream, test.specifiedVersion, config.Null())
+
+		if err != nil {
+			t.Fatalf("Version resolution failed unexpectedly: %v", err)
+		}
+		if gotVersion != test.wantVersion {
+			t.Errorf("Expected version %s, but got %s", test.wantVersion, gotVersion)
+		}
+		/*
+		We expect three requests:
+		- One to get a list of all Bazel version tracks
+		- One to check for the existence of the 8.* release (candidates), which returns no results
+		- One to check for the existence of the 7.* release (candidates), which returns a match
+		*/
+		wantRequests := 3
+		if gotRequests := len(s.Transport.RequestedURLs); gotRequests != wantRequests {
+			t.Errorf("Expected exactly %d requests (one for the top-level, one for 8.0.0, one for 7.0.0), but got %d:\n%s", wantRequests, gotRequests, strings.Join(s.Transport.RequestedURLs, "\n"))
+		}
+	})
+	}
+}
+
 func TestResolveLatestVersion_ShouldFailIfNotEnoughReleases(t *testing.T) {
 	s := setUp(t)
 	s.AddVersion("3.0.0", true, nil, nil)

core/core.go

@@ -332,7 +332,7 @@ func parseBazelForkAndVersion(bazelForkAndVersion string) (string, string, error
 	} else if len(versionInfo) == 2 {
 		bazelFork, bazelVersion = versionInfo[0], versionInfo[1]
 	} else {
-		return "", "", fmt.Errorf("invalid version \"%s\", could not parse version with more than one slash", bazelForkAndVersion)
+		return "", "", fmt.Errorf("invalid version %q, could not parse version with more than one slash", bazelForkAndVersion)
 	}
 
 	return bazelFork, bazelVersion, nil

core/repositories.go

@@ -196,7 +196,7 @@ func resolvePotentiallyRelativeVersion(bazeliskHome string, lister listVersionsF
 
 	index := len(available) - 1 - vi.LatestOffset
 	if index < 0 {
-		return "", fmt.Errorf("cannot resolve version \"%s\": There are not enough matching Bazel releases (%d)", vi.Value, len(available))
+		return "", fmt.Errorf("cannot resolve version %q: There are not enough matching Bazel releases (%d)", vi.Value, len(available))
 	}
 	sorted := versions.GetInAscendingOrder(available)
 	return sorted[index], nil

httputil/fake.go

@@ -9,6 +9,8 @@ import (
 // FakeTransport represents a fake http.Transport that returns prerecorded responses.
 type FakeTransport struct {
 	responses map[string]*responseCollection
+
+	RequestedURLs []string
 }
 
 // NewFakeTransport creates a new FakeTransport instance without any responses.
@@ -38,6 +40,7 @@ func (ft *FakeTransport) AddError(url string, err error) {
 
 // RoundTrip returns a prerecorded response to the given request, if one exists. Otherwise its response indicates 404 - not found.
 func (ft *FakeTransport) RoundTrip(req *http.Request) (*http.Response, error) {
+	ft.RequestedURLs = append(ft.RequestedURLs, req.URL.String())
 	if responses, ok := ft.responses[req.URL.String()]; ok {
 		return responses.Next()
 	}

platforms/platforms.go

@@ -64,7 +64,7 @@ func DetermineArchitecture(osName, version string) (string, error) {
 	case "arm64":
 		machineName = "arm64"
 	default:
-		return "", fmt.Errorf("unsupported machine architecture \"%s\", must be arm64 or x86_64", runtime.GOARCH)
+		return "", fmt.Errorf("unsupported machine architecture %q, must be arm64 or x86_64", runtime.GOARCH)
 	}
 
 	if osName == "darwin" {
@@ -80,7 +80,7 @@ func DetermineOperatingSystem() (string, error) {
 	case "darwin", "linux", "windows":
 		return runtime.GOOS, nil
 	default:
-		return "", fmt.Errorf("unsupported operating system \"%s\", must be Linux, macOS or Windows", runtime.GOOS)
+		return "", fmt.Errorf("unsupported operating system %q, must be Linux, macOS or Windows", runtime.GOOS)
 	}
 }
 

repositories/gcs.go

@@ -157,12 +157,16 @@ func (gcs *GCSRepo) matchingVersions(history []string, opts *core.FilterOpts) ([
 
 		// Ascending list of rc versions, followed by the release version (if it exists) and a rolling identifier (if there are rolling releases).
 		versions := getVersionsFromGCSPrefixes(prefixes)
-		for vpos := len(versions) - 1; vpos >= 0 && len(descendingMatches) < opts.MaxResults; vpos-- {
+		for vpos := len(versions) - 1; vpos >= 0; vpos-- {
 			curr := versions[vpos]
 			if strings.Contains(curr, "rolling") || !opts.Filter(curr) {
 				continue
 			}
+
 			descendingMatches = append(descendingMatches, curr)
+			if len(descendingMatches) == opts.MaxResults {
+				return descendingMatches, nil
+			}
 		}
 	}
 	return descendingMatches, nil

versions/versions.go

@@ -61,7 +61,7 @@ func Parse(fork, version string) (*Info, error) {
 		if m[1] != "" {
 			offset, err := strconv.Atoi(m[1])
 			if err != nil {
-				return nil, fmt.Errorf("invalid version \"%s\", could not parse offset: %v", version, err)
+				return nil, fmt.Errorf("invalid version %q, could not parse offset: %v", version, err)
 			}
 			vi.LatestOffset = offset
 		}