From c71f0623fb3ba6bd0c8f6848cb5c03f379da3d7a Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Sun, 9 Aug 2026 00:29:16 +0200 Subject: [PATCH] feat(updatecheck): add Checker package for GitHub release comparison MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second piece of #591. Standalone package (pkg/service/updatecheck): GitHub releases API client, golang.org/x/mod/semver comparison (promoted from indirect to direct dependency), persisted state via datastore's UpdateCheckState. Dev/(devel)/dirty current versions skip the comparison entirely rather than guessing; prereleases are excluded even though GitHub's /releases/latest endpoint shouldn't return one anyway (defensive). Deliberately decoupled from handlers.Server/main.go: repo and current version are constructor arguments, not hardcoded, so a future CLI-side check could reuse this as a plain import rather than a rewrite (open question 2 in the design doc). Not wired into the service yet — nothing calls NewChecker/CheckNow outside tests. Refs #591 --- go.mod | 2 +- pkg/service/updatecheck/updatecheck.go | 237 +++++++++++++++ pkg/service/updatecheck/updatecheck_test.go | 307 ++++++++++++++++++++ 3 files changed, 545 insertions(+), 1 deletion(-) create mode 100644 pkg/service/updatecheck/updatecheck.go create mode 100644 pkg/service/updatecheck/updatecheck_test.go diff --git a/go.mod b/go.mod index 3b18799..e29a6ea 100644 --- a/go.mod +++ b/go.mod @@ -16,6 +16,7 @@ require ( github.com/srwiley/rasterx v0.0.0-20220730225603-2ab79fcdd4ef github.com/urfave/cli/v2 v2.27.7 golang.org/x/crypto v0.54.0 + golang.org/x/mod v0.38.0 golang.org/x/net v0.57.0 golang.org/x/term v0.45.0 ) @@ -32,7 +33,6 @@ require ( github.com/gobwas/ws v1.4.0 // indirect github.com/xrash/smetrics v0.0.0-20250705151800-55b8f293f342 // indirect golang.org/x/image v0.44.0 // indirect - golang.org/x/mod v0.38.0 // indirect golang.org/x/sync v0.22.0 // indirect golang.org/x/sys v0.47.0 // indirect golang.org/x/text v0.40.0 // indirect diff --git a/pkg/service/updatecheck/updatecheck.go b/pkg/service/updatecheck/updatecheck.go new file mode 100644 index 0000000..3cd3aba --- /dev/null +++ b/pkg/service/updatecheck/updatecheck.go @@ -0,0 +1,237 @@ +// Package updatecheck implements the opt-in periodic check against a +// GitHub repo's latest release (#591, +// _/i591/design-update-check.md). Deliberately generic (repo and current +// version are constructor arguments, not hardcoded) and decoupled from +// handlers.Server/main.go globals, so other binaries could construct their +// own Checker later without a rewrite — see the design doc's answer to +// open question 2 (CLI-only users). +package updatecheck + +import ( + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "strings" + "sync" + "time" + + "golang.org/x/mod/semver" + + "github.com/gesellix/bose-soundtouch/pkg/service/datastore" +) + +const defaultTimeout = 5 * time.Second + +const defaultBaseURL = "https://api.github.com" + +// Result is the outcome of the most recent check. +type Result struct { + Available bool `json:"available"` + CurrentVersion string `json:"current_version"` + LatestVersion string `json:"latest_version,omitempty"` + ReleaseURL string `json:"release_url,omitempty"` + CheckedAt time.Time `json:"checked_at"` +} + +// Checker checks a GitHub repo's latest release against the running +// version. Safe for concurrent use. +type Checker struct { + mu sync.RWMutex + repo string // "owner/repo" + currentVersion string + httpClient *http.Client + ds *datastore.DataStore + baseURL string + last Result +} + +// NewChecker constructs a Checker for repo (e.g. "gesellix/Bose-SoundTouch") +// against currentVersion, seeding its last-known result from ds's persisted +// UpdateCheckState if present (so a restart doesn't lose "already knew +// about vX.Y.Z" until the next tick). ds may be nil (state just won't +// persist across restarts). +func NewChecker(ds *datastore.DataStore, repo, currentVersion string) *Checker { + c := &Checker{ + repo: repo, + currentVersion: currentVersion, + httpClient: &http.Client{Timeout: defaultTimeout}, + ds: ds, + baseURL: defaultBaseURL, + last: Result{CurrentVersion: currentVersion}, + } + + if ds == nil { + return c + } + + state, err := ds.GetUpdateCheckState() + if err != nil || state.LastSeenVersion == "" { + return c + } + + c.last.LatestVersion = state.LastSeenVersion + + if ts, parseErr := time.Parse(time.RFC3339, state.LastCheckedAt); parseErr == nil { + c.last.CheckedAt = ts + } + + if normalizedCurrent, ok := normalizeVersion(currentVersion); ok { + if normalizedLatest, ok2 := normalizeVersion(state.LastSeenVersion); ok2 { + c.last.Available = semver.Compare(normalizedLatest, normalizedCurrent) > 0 + } + } + + return c +} + +// SetBaseURL overrides the GitHub API base URL. Test-only — not exposed via +// config, since there's exactly one GitHub to check against in production. +func (c *Checker) SetBaseURL(url string) { + c.mu.Lock() + defer c.mu.Unlock() + + c.baseURL = url +} + +// SetTimeout overrides the HTTP client timeout (production always uses +// defaultTimeout). Test-only, to exercise timeout handling without a +// multi-second test. +func (c *Checker) SetTimeout(d time.Duration) { + c.mu.Lock() + defer c.mu.Unlock() + + c.httpClient.Timeout = d +} + +// LastResult returns the outcome of the most recent check (or the +// persisted-state-seeded value if CheckNow hasn't run yet this process). +func (c *Checker) LastResult() Result { + c.mu.RLock() + defer c.mu.RUnlock() + + return c.last +} + +// CheckNow performs one check against the GitHub API, updates LastResult, +// and persists the outcome (if ds is non-nil and a latest version was +// found). Returns an error only on a genuine fetch/parse failure — a +// current version that can't be meaningfully compared (dev/(devel)/dirty +// builds) is not an error, it's a no-op "skip the check" result, per the +// design doc's answer on non-release builds. +func (c *Checker) CheckNow(ctx context.Context) (Result, error) { + result := Result{CurrentVersion: c.currentVersion, CheckedAt: time.Now()} + + normalizedCurrent, ok := normalizeVersion(c.currentVersion) + if !ok { + c.setLast(result) + return result, nil + } + + release, err := c.fetchLatestRelease(ctx) + if err != nil { + return Result{}, err + } + + if !release.Prerelease { + result.LatestVersion = release.TagName + result.ReleaseURL = release.HTMLURL + + if normalizedLatest, ok := normalizeVersion(release.TagName); ok { + result.Available = semver.Compare(normalizedLatest, normalizedCurrent) > 0 + } + } + + c.setLast(result) + c.persist(result) + + return result, nil +} + +func (c *Checker) setLast(result Result) { + c.mu.Lock() + defer c.mu.Unlock() + + c.last = result +} + +// persist saves the outcome, but only when a latest version was actually +// found — a transient fetch failure (already excluded, CheckNow returns +// before calling this) or a defensive prerelease-only response must not +// overwrite previously-known-good state with emptiness. +func (c *Checker) persist(result Result) { + if c.ds == nil || result.LatestVersion == "" { + return + } + + _ = c.ds.SaveUpdateCheckState(datastore.UpdateCheckState{ + LastCheckedAt: result.CheckedAt.UTC().Format(time.RFC3339), + LastSeenVersion: result.LatestVersion, + }) +} + +type githubRelease struct { + TagName string `json:"tag_name"` + Prerelease bool `json:"prerelease"` + HTMLURL string `json:"html_url"` +} + +func (c *Checker) fetchLatestRelease(ctx context.Context) (githubRelease, error) { + c.mu.RLock() + baseURL := c.baseURL + c.mu.RUnlock() + + url := fmt.Sprintf("%s/repos/%s/releases/latest", baseURL, c.repo) + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return githubRelease{}, err + } + + req.Header.Set("User-Agent", "AfterTouch-update-check") + req.Header.Set("Accept", "application/vnd.github+json") + + resp, err := c.httpClient.Do(req) + if err != nil { + return githubRelease{}, err + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusOK { + return githubRelease{}, fmt.Errorf("github releases API returned HTTP %d", resp.StatusCode) + } + + body, err := io.ReadAll(resp.Body) + if err != nil { + return githubRelease{}, fmt.Errorf("read response: %w", err) + } + + var release githubRelease + if err := json.Unmarshal(body, &release); err != nil { + return githubRelease{}, fmt.Errorf("parse response: %w", err) + } + + return release, nil +} + +// normalizeVersion reports whether v can be meaningfully compared as +// semver, and its normalized ("v"-prefixed) form if so. Deliberately +// treats dev/(devel)/dirty builds as unparseable rather than guessing — +// see the design doc. +func normalizeVersion(v string) (string, bool) { + v = strings.TrimSpace(v) + if v == "" || v == "dev" || v == "(devel)" || strings.Contains(v, "dirty") { + return "", false + } + + if !strings.HasPrefix(v, "v") { + v = "v" + v + } + + if !semver.IsValid(v) { + return "", false + } + + return v, true +} diff --git a/pkg/service/updatecheck/updatecheck_test.go b/pkg/service/updatecheck/updatecheck_test.go new file mode 100644 index 0000000..276f549 --- /dev/null +++ b/pkg/service/updatecheck/updatecheck_test.go @@ -0,0 +1,307 @@ +package updatecheck + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "testing" + "time" + + "github.com/gesellix/bose-soundtouch/pkg/service/datastore" +) + +func TestNormalizeVersion(t *testing.T) { + cases := []struct { + in string + wantOK bool + wantOut string + }{ + {"v1.2.3", true, "v1.2.3"}, + {"1.2.3", true, "v1.2.3"}, // missing "v" prefix gets added + {"dev", false, ""}, + {"(devel)", false, ""}, + {"v0.120.1-0.20260808211626-abcdef123456+dirty", false, ""}, + {"", false, ""}, + {"not-a-version", false, ""}, + } + + for _, tc := range cases { + out, ok := normalizeVersion(tc.in) + if ok != tc.wantOK { + t.Errorf("normalizeVersion(%q): ok = %v, want %v", tc.in, ok, tc.wantOK) + } + if ok && out != tc.wantOut { + t.Errorf("normalizeVersion(%q) = %q, want %q", tc.in, out, tc.wantOut) + } + } +} + +func newTestServer(t *testing.T, status int, body string) *httptest.Server { + t.Helper() + + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if ua := r.Header.Get("User-Agent"); ua == "" { + t.Error("expected a User-Agent header to be set") + } + w.WriteHeader(status) + _, _ = w.Write([]byte(body)) + })) +} + +func newTestDataStore(t *testing.T) *datastore.DataStore { + t.Helper() + + tempDir, err := os.MkdirTemp("", "updatecheck-test") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + t.Cleanup(func() { os.RemoveAll(tempDir) }) + + return datastore.NewDataStore(tempDir) +} + +func TestCheckNow_NewerVersionAvailable(t *testing.T) { + server := newTestServer(t, http.StatusOK, `{"tag_name":"v1.1.0","prerelease":false,"html_url":"https://example.invalid/v1.1.0"}`) + defer server.Close() + + ds := newTestDataStore(t) + c := NewChecker(ds, "owner/repo", "v1.0.0") + c.SetBaseURL(server.URL) + + result, err := c.CheckNow(context.Background()) + if err != nil { + t.Fatalf("CheckNow failed: %v", err) + } + + if !result.Available { + t.Error("Expected Available=true for a newer release") + } + if result.LatestVersion != "v1.1.0" { + t.Errorf("Expected LatestVersion v1.1.0, got %q", result.LatestVersion) + } + if result.ReleaseURL != "https://example.invalid/v1.1.0" { + t.Errorf("Expected ReleaseURL to be set, got %q", result.ReleaseURL) + } + + if got := c.LastResult(); got != result { + t.Errorf("LastResult() = %+v, want %+v", got, result) + } + + persisted, err := ds.GetUpdateCheckState() + if err != nil { + t.Fatalf("GetUpdateCheckState failed: %v", err) + } + if persisted.LastSeenVersion != "v1.1.0" { + t.Errorf("Expected persisted LastSeenVersion v1.1.0, got %q", persisted.LastSeenVersion) + } +} + +func TestCheckNow_CurrentVersionIsUpToDate(t *testing.T) { + server := newTestServer(t, http.StatusOK, `{"tag_name":"v1.0.0","prerelease":false,"html_url":"https://example.invalid/v1.0.0"}`) + defer server.Close() + + c := NewChecker(newTestDataStore(t), "owner/repo", "v1.0.0") + c.SetBaseURL(server.URL) + + result, err := c.CheckNow(context.Background()) + if err != nil { + t.Fatalf("CheckNow failed: %v", err) + } + + if result.Available { + t.Error("Expected Available=false when already on the latest version") + } +} + +func TestCheckNow_OlderReleaseThanCurrent(t *testing.T) { + // e.g. a beta/main build ahead of the last tagged release. + server := newTestServer(t, http.StatusOK, `{"tag_name":"v0.9.0","prerelease":false}`) + defer server.Close() + + c := NewChecker(newTestDataStore(t), "owner/repo", "v1.0.0") + c.SetBaseURL(server.URL) + + result, err := c.CheckNow(context.Background()) + if err != nil { + t.Fatalf("CheckNow failed: %v", err) + } + + if result.Available { + t.Error("Expected Available=false when the release is older than current") + } +} + +func TestCheckNow_PrereleaseExcluded(t *testing.T) { + server := newTestServer(t, http.StatusOK, `{"tag_name":"v2.0.0","prerelease":true,"html_url":"https://example.invalid/v2.0.0"}`) + defer server.Close() + + c := NewChecker(newTestDataStore(t), "owner/repo", "v1.0.0") + c.SetBaseURL(server.URL) + + result, err := c.CheckNow(context.Background()) + if err != nil { + t.Fatalf("CheckNow failed: %v", err) + } + + if result.Available { + t.Error("Expected Available=false for a prerelease, even though it's semver-newer") + } + if result.LatestVersion != "" { + t.Errorf("Expected no LatestVersion recorded for a prerelease-only response, got %q", result.LatestVersion) + } +} + +func TestCheckNow_DirtyCurrentVersionSkipsWithoutError(t *testing.T) { + // Server would answer, but must never be called for a non-release build. + called := false + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"tag_name":"v9.9.9","prerelease":false}`)) + })) + defer server.Close() + + c := NewChecker(newTestDataStore(t), "owner/repo", "v1.0.0-0.20260101000000-abcdef+dirty") + c.SetBaseURL(server.URL) + + result, err := c.CheckNow(context.Background()) + if err != nil { + t.Fatalf("Expected no error for a dirty current version, got: %v", err) + } + if result.Available { + t.Error("Expected Available=false, dirty builds must skip the comparison") + } + if called { + t.Error("Expected no HTTP call for a dirty current version") + } +} + +func TestCheckNow_TimeoutReturnsError(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + time.Sleep(200 * time.Millisecond) + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"tag_name":"v1.1.0","prerelease":false}`)) + })) + defer server.Close() + + c := NewChecker(newTestDataStore(t), "owner/repo", "v1.0.0") + c.SetBaseURL(server.URL) + c.SetTimeout(20 * time.Millisecond) + + if _, err := c.CheckNow(context.Background()); err == nil { + t.Error("Expected a timeout error") + } +} + +func TestCheckNow_MalformedJSONReturnsError(t *testing.T) { + server := newTestServer(t, http.StatusOK, `not json`) + defer server.Close() + + c := NewChecker(newTestDataStore(t), "owner/repo", "v1.0.0") + c.SetBaseURL(server.URL) + + if _, err := c.CheckNow(context.Background()); err == nil { + t.Error("Expected an error for a malformed JSON response") + } +} + +func TestCheckNow_NonOKStatusReturnsError(t *testing.T) { + server := newTestServer(t, http.StatusInternalServerError, `oops`) + defer server.Close() + + c := NewChecker(newTestDataStore(t), "owner/repo", "v1.0.0") + c.SetBaseURL(server.URL) + + if _, err := c.CheckNow(context.Background()); err == nil { + t.Error("Expected an error for a non-200 response") + } +} + +func TestCheckNow_FailureDoesNotOverwritePersistedState(t *testing.T) { + ds := newTestDataStore(t) + if err := ds.SaveUpdateCheckState(datastore.UpdateCheckState{ + LastCheckedAt: "2026-08-01T00:00:00Z", + LastSeenVersion: "v1.1.0", + }); err != nil { + t.Fatalf("Failed to seed state: %v", err) + } + + server := newTestServer(t, http.StatusInternalServerError, `oops`) + defer server.Close() + + c := NewChecker(ds, "owner/repo", "v1.0.0") + c.SetBaseURL(server.URL) + + if _, err := c.CheckNow(context.Background()); err == nil { + t.Fatal("Expected an error from the failing server") + } + + persisted, err := ds.GetUpdateCheckState() + if err != nil { + t.Fatalf("GetUpdateCheckState failed: %v", err) + } + if persisted.LastSeenVersion != "v1.1.0" { + t.Errorf("Expected previously-persisted state to survive a failed check, got %+v", persisted) + } +} + +// TestNewChecker_SeedsFromPersistedState verifies a restarted process picks +// up "already knew about vX.Y.Z" from a previous run without needing to +// call CheckNow first. +func TestNewChecker_SeedsFromPersistedState(t *testing.T) { + ds := newTestDataStore(t) + checkedAt := time.Date(2026, 8, 1, 12, 0, 0, 0, time.UTC) + + if err := ds.SaveUpdateCheckState(datastore.UpdateCheckState{ + LastCheckedAt: checkedAt.Format(time.RFC3339), + LastSeenVersion: "v1.1.0", + }); err != nil { + t.Fatalf("Failed to seed state: %v", err) + } + + c := NewChecker(ds, "owner/repo", "v1.0.0") + result := c.LastResult() + + if !result.Available { + t.Error("Expected a seeded Checker to report Available=true") + } + if result.LatestVersion != "v1.1.0" { + t.Errorf("Expected seeded LatestVersion v1.1.0, got %q", result.LatestVersion) + } + if !result.CheckedAt.Equal(checkedAt) { + t.Errorf("Expected seeded CheckedAt %v, got %v", checkedAt, result.CheckedAt) + } +} + +func TestNewChecker_NilDataStoreIsSafe(t *testing.T) { + c := NewChecker(nil, "owner/repo", "v1.0.0") + + result := c.LastResult() + if result.Available { + t.Error("Expected a fresh Checker with no datastore to report Available=false") + } +} + +// Sanity check that the JSON tags on Result round-trip as expected — a +// contract worth pinning if this ever gets exposed via an HTTP handler. +func TestResult_JSONShape(t *testing.T) { + r := Result{Available: true, CurrentVersion: "v1.0.0", LatestVersion: "v1.1.0", ReleaseURL: "https://example.invalid"} + + data, err := json.Marshal(r) + if err != nil { + t.Fatalf("Marshal failed: %v", err) + } + + var got map[string]interface{} + if err := json.Unmarshal(data, &got); err != nil { + t.Fatalf("Unmarshal failed: %v", err) + } + + for _, key := range []string{"available", "current_version", "latest_version", "release_url", "checked_at"} { + if _, ok := got[key]; !ok { + t.Errorf("Expected JSON key %q in marshaled Result", key) + } + } +}