fix pin-and-downgrade

Previously, if you tried to pin to a lower version, we would treat it as
a no-op (and keep you at your current version). Now, we'll perform the
update ("downdate").

This resolves #187.

We've had a few back-and-forths in this code, most recently #137. I
added unit tests to try and put this to bed for good, or at least
prevent further regressions.

I found myself getting kinda confused about the various situations, so I
wrote a novel to explain it to myself. I figured I'd keep it around for
the next person.

pkg/updater/updater.go

@@ -20,7 +20,7 @@ var (
 	Arch string = runtime.GOARCH
 )
 
-var (
+const (
 	DefaultServer string = "http://srv.klo.dev"
 )
 
@@ -75,7 +75,33 @@ func (u *Updater) CheckUpdate(currentVersion string) (bool, error) {
 		return false, fmt.Errorf("invalid version %s: %v", currentVersion, err)
 	}
 
-	return currVersion.LessThan(*latestVersion) || strings.Split(u.CurrentStream, ":")[0] != strings.Split(u.Stream, ":")[0], nil
+	// Given a stream "xxx:yyyy", the qualifier is the "xxx" and the tag is the "yyyy".
+	//
+	// (1) If the qualifiers are different, always update (this is to handle open <--> pro)
+	// Otherwise, check the cli's version against latest. This is a bit trickier:
+	//
+	// (2a) If the tags are the same, then either it's a specific version or it's a monotonic tag like "latest".
+	//   • If it's a monotonic tag, we only want to perform upgrades. A downgrade would be a situation like if we gave
+	//     someone a pre-release, in which case we don't want to downgrade them.
+	//   • If it's a specific version, we can assume that the version will never change.
+	//   • So in either case, we want to only perform upgrades.
+	// (2b) If the tags are different, then someone is either pinning to a specific version, or going from a pinned
+	//     version to a monotonic version. In either case, we should allow downgrades. (Going from pinned to monotonic
+	//     *may* be an incorrect downgrade, with a similar pre-release reason. But if someone has a pre-release, they
+	//     shouldn't be worrying about any upgrade stuff, including not changing their update stream from pinned to
+	//     monotonic.)
+
+	// case (1): different qualifiers always update
+	if strings.Split(u.CurrentStream, ":")[0] != strings.Split(u.Stream, ":")[0] {
+		return true, nil
+	}
+
+	// the qualifiers are the same, so the tags are the same iff the full stream strings are the same
+	if u.CurrentStream == u.Stream {
+		return currVersion.LessThan(*latestVersion), nil // case (2a): only upgrades
+	} else {
+		return !currVersion.Equal(*latestVersion), nil // case (2b): upgrades or downgrades
+	}
 }
 
 // Update performs an update if a newer version is

pkg/updater/updater_test.go

@@ -0,0 +1,237 @@
+package updater
+
+import (
+	"encoding/json"
+	"github.com/stretchr/testify/assert"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+)
+
+func TestCheckUpdate(t *testing.T) {
+	type updateInputs struct {
+		buildStream    string
+		currentVersion string
+		checkStream    string
+	}
+	cases := []struct {
+		name string
+		cli  updateInputs
+		// serverResponse is the server's answer for the latest version, or empty string to represent an error
+		serverResponse string
+		expectUpdate   bool
+		expectError    bool
+	}{
+		{
+			name: `same stream, server has same version`,
+			cli: updateInputs{
+				buildStream:    `open:latest`,
+				currentVersion: `0.5.0`,
+				checkStream:    `open:latest`,
+			},
+			serverResponse: `0.5.0`,
+			expectUpdate:   false,
+		},
+		{
+			name: `same stream, server has newer version`,
+			cli: updateInputs{
+				buildStream:    `open:latest`,
+				currentVersion: `0.5.0`,
+				checkStream:    `open:latest`,
+			},
+			serverResponse: `0.5.1`,
+			expectUpdate:   true,
+		},
+		{
+			name: `same stream, server has older version`,
+			cli: updateInputs{
+				buildStream:    `open:latest`,
+				currentVersion: `0.5.1`,
+				checkStream:    `open:latest`,
+			},
+			serverResponse: `0.5.0`,
+			// This is typically unexpected, but can happen in development, or if we give someone a pre-release.
+			// We should not downgrade in either of those cases.
+			expectUpdate: false,
+		},
+		{
+			name: `different stream, server has newer version`,
+			cli: updateInputs{
+				buildStream:    `open:latest`,
+				currentVersion: `0.5.0`,
+				checkStream:    `pro:latest`,
+			},
+			serverResponse: `0.5.1`,
+			expectUpdate:   true,
+		},
+		{
+			name: `different stream, server has same version`,
+			cli: updateInputs{
+				buildStream:    `open:latest`,
+				currentVersion: `0.5.0`,
+				checkStream:    `pro:latest`,
+			},
+			serverResponse: `0.5.0`,
+			expectUpdate:   true, // both are 0.5.0, but we want to update to switch editions
+		},
+		{
+			name: `different stream, server has older version`,
+			cli: updateInputs{
+				buildStream:    `open:latest`,
+				currentVersion: `0.5.1`,
+				checkStream:    `pro:latest`,
+			},
+			serverResponse: `0.5.1`,
+			expectUpdate:   true,
+		},
+		{
+			name: `pin to same stream, same version`,
+			cli: updateInputs{
+				buildStream:    `open:latest`,
+				currentVersion: `0.5.0`,
+				checkStream:    `open:v0.5.0`,
+			},
+			serverResponse: `0.5.0`,
+			// it's important that this is false, or else the user will keep getting messages to the effect of
+			// "you're on v0.1.2, but there's a new version, v0.1.2".
+			expectUpdate: false,
+		},
+		{
+			name: `pin to same stream, newer version`,
+			cli: updateInputs{
+				buildStream:    `open:latest`,
+				currentVersion: `0.5.0`,
+				checkStream:    `open:v0.5.1`,
+			},
+			serverResponse: `0.5.1`,
+			expectUpdate:   true,
+		},
+		{
+			name: `pin to same stream, older version`,
+			cli: updateInputs{
+				buildStream:    `open:latest`,
+				currentVersion: `0.5.1`,
+				checkStream:    `open:v0.5.0`,
+			},
+			serverResponse: `0.5.0`,
+			expectUpdate:   true,
+		},
+		{
+			name: `pin to different stream, same version`,
+			cli: updateInputs{
+				buildStream:    `open:latest`,
+				currentVersion: `0.5.0`,
+				checkStream:    `pro:v0.5.0`,
+			},
+			serverResponse: `0.5.0`,
+			expectUpdate:   true,
+		},
+		{
+			name: `pin to different stream, newer version`,
+			cli: updateInputs{
+				buildStream:    `open:latest`,
+				currentVersion: `0.5.0`,
+				checkStream:    `pro:v0.5.1`,
+			},
+			serverResponse: `0.5.1`,
+			expectUpdate:   true,
+		},
+		{
+			name: `pin to different stream, older version`,
+			cli: updateInputs{
+				buildStream:    `open:latest`,
+				currentVersion: `0.5.1`,
+				checkStream:    `pro:v0.5.0`,
+			},
+			serverResponse: `0.5.0`,
+			expectUpdate:   true,
+		},
+		{
+			name: `pin to non-existent version`,
+			cli: updateInputs{
+				buildStream:    `open:latest`,
+				currentVersion: `0.5.1`,
+				checkStream:    `open:v0.5.0`,
+			},
+			expectError: true,
+		},
+	}
+	for _, tt := range cases {
+		t.Run(tt.name, func(t *testing.T) {
+			assert := assert.New(t)
+
+			handler := interactions{assert: assert}
+			rr := requestResponse{
+				inUri:     `/update/check-latest-version?stream=` + tt.cli.checkStream,
+				inMethod:  http.MethodGet,
+				outStatus: http.StatusInternalServerError, // will be overwritten if serverResponse != ""
+			}
+			if tt.serverResponse != "" {
+				rr.outStatus = http.StatusOK
+				rr.outBody = map[string]string{`latest_version`: tt.serverResponse}
+			}
+			handler.interactions = append(handler.interactions, rr)
+			server := httptest.NewServer(&handler)
+			defer server.Close()
+
+			updater := Updater{
+				ServerURL:     server.URL,
+				Stream:        tt.cli.checkStream,
+				CurrentStream: tt.cli.buildStream,
+			}
+			needsUpdate, e := updater.CheckUpdate(tt.cli.currentVersion)
+
+			if t.Failed() {
+				return // this means the mocked server failed (ie ran out of request-responses)
+			}
+			if !assert.Empty(handler.interactions, "didn't see all expected interactions") {
+				return
+			}
+			if tt.expectError {
+				assert.Error(e)
+			} else {
+				if assert.NoError(e) {
+					assert.Equal(needsUpdate, tt.expectUpdate)
+				}
+			}
+		})
+	}
+}
+
+type (
+	requestResponse struct {
+		inMethod  string
+		inUri     string
+		outStatus int
+		outBody   any
+	}
+
+	interactions struct {
+		assert       *assert.Assertions
+		interactions []requestResponse
+	}
+)
+
+func (s *interactions) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	if len(s.interactions) == 0 {
+		s.assert.Fail("no interactions left")
+		w.WriteHeader(http.StatusInternalServerError)
+		return
+	}
+	curr := s.interactions[0]
+	s.interactions = s.interactions[1:]
+
+	if s.assert.Equal(curr.inMethod, r.Method) && s.assert.Equal(curr.inUri, r.URL.RequestURI()) {
+		body, err := json.Marshal(curr.outBody)
+		if !s.assert.NoError(err) {
+			w.WriteHeader(http.StatusInternalServerError)
+			return
+		}
+		w.WriteHeader(curr.outStatus)
+		_, err = w.Write(body)
+		s.assert.NoError(err)
+	} else {
+		s.assert.Fail("no interactions left")
+		w.WriteHeader(http.StatusInternalServerError)
+	}
+}