Cache Go modules that differ only in case apart

internal/handler/go.go

@@ -1,11 +1,13 @@
 package handler
 
 import (
+	"crypto/sha256"
+	"encoding/hex"
 	"errors"
-	"fmt"
 	"net/http"
 	"strings"
 
+	"github.com/git-pkgs/proxy/internal/database"
 	"github.com/git-pkgs/proxy/internal/retention"
 	"github.com/git-pkgs/registries/fetch"
 )
@@ -119,12 +121,13 @@ func (h *GoHandler) handleRequest(w http.ResponseWriter, r *http.Request) {
 func (h *GoHandler) handleDownload(w http.ResponseWriter, r *http.Request, module, version string) {
 	// Decode module path (! followed by lowercase = uppercase)
 	decodedModule := decodeGoModule(module)
-	filename := fmt.Sprintf("%s@%s.zip", lastComponent(decodedModule), version)
+	filename := goZipFilename(decodedModule, version)
 
 	h.proxy.Logger.Info("go module download request",
 		"module", decodedModule, "version", version)
 
 	downloadURL := h.upstreamURL + r.URL.Path
+	h.discardOtherModulesZip(decodedModule, module, version, filename)
 	result, err := h.proxy.GetOrFetchArtifactFromURL(
 		r.Context(), "golang", decodedModule, version, filename, downloadURL,
 	)
@@ -148,6 +151,55 @@ func (h *GoHandler) handleDownload(w http.ResponseWriter, r *http.Request, modul
 	ServeArtifactRequest(w, r, result)
 }
 
+// goZipFilename is the name a module zip is cached under. Module paths are
+// case-sensitive, but the zip is stored under the module's PURL, which
+// lowercases golang names, so github.com/Sirupsen/logrus and
+// github.com/sirupsen/logrus share one package. A path with uppercase
+// letters therefore gets a hash of its exact spelling in the filename. An
+// all-lowercase path keeps the plain name, so existing cache entries stay
+// valid.
+func goZipFilename(module, version string) string {
+	base := lastComponent(module) + "@" + version
+	if module == strings.ToLower(module) {
+		return base + ".zip"
+	}
+	digest := sha256.Sum256([]byte(module))
+	return base + "~" + hex.EncodeToString(digest[:goModuleHashBytes]) + ".zip"
+}
+
+// goModuleHashBytes is how much of a module path's hash a cached zip's
+// filename carries: enough to tell apart paths that differ only in case.
+const goModuleHashBytes = 8
+
+// discardOtherModulesZip drops a cached zip that another module stored
+// under filename. Before module paths kept their case, a module with
+// uppercase letters was cached under the plain filename of its lowercase
+// twin, and whichever came first was served for both. The upstream URL a
+// zip was fetched from names its module in escaped form, so a record whose
+// URL names a different module is discarded and fetched again. Only the
+// module path is compared, so pointing upstream.go elsewhere keeps the cache.
+func (h *GoHandler) discardOtherModulesZip(module, escapedModule, version, filename string) {
+	_, versionPURL, err := packagePURLStrings("golang", module, version)
+	if err != nil {
+		return
+	}
+	record, err := h.proxy.DB.GetArtifact(versionPURL, filename)
+	if err != nil || record == nil || !record.StoragePath.Valid || record.Source == database.SourceLocal {
+		return
+	}
+	// Only a URL that names another Go module proves the zip is someone
+	// else's; a record of unknown origin is kept.
+	if !strings.Contains(record.UpstreamURL, "/@v/") ||
+		strings.HasSuffix(record.UpstreamURL, "/"+escapedModule+"/@v/"+version+".zip") {
+		return
+	}
+	h.proxy.Logger.Warn("cached zip belongs to a module that differs only in case, discarding",
+		"module", module, "version", version, "cached_from", record.UpstreamURL)
+	if err := h.proxy.DB.DiscardArtifact(versionPURL, filename, record.StoragePath.String); err != nil {
+		h.proxy.Logger.Warn("failed to discard cached zip", "module", module, "version", version, "error", err)
+	}
+}
+
 // proxyCached forwards a request with metadata caching.
 func (h *GoHandler) proxyCached(w http.ResponseWriter, r *http.Request, cacheKey string) {
 	h.proxy.ProxyCached(w, r, h.upstreamURL+r.URL.Path, "golang", cacheKey, "*/*")

internal/handler/go_case_test.go

@@ -0,0 +1,135 @@
+package handler
+
+import (
+	"context"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"sync/atomic"
+	"testing"
+
+	"github.com/git-pkgs/registries/fetch"
+)
+
+const (
+	upperLogrusPath = "/github.com/!sirupsen/logrus/@v/v1.0.6.zip"
+	lowerLogrusPath = "/github.com/sirupsen/logrus/@v/v1.0.6.zip"
+)
+
+func TestGoZipFilenameKeepsCase(t *testing.T) {
+	lower := goZipFilename("github.com/sirupsen/logrus", "v1.0.6")
+	if lower != "logrus@v1.0.6.zip" {
+		t.Errorf("lowercase module filename = %q, want the plain name", lower)
+	}
+	upper := goZipFilename("github.com/Sirupsen/logrus", "v1.0.6")
+	if upper == lower || !strings.HasPrefix(upper, "logrus@v1.0.6~") || !strings.HasSuffix(upper, ".zip") {
+		t.Errorf("uppercase module filename = %q, want logrus@v1.0.6~{hash}.zip", upper)
+	}
+	if other := goZipFilename("github.com/SIRUPSEN/logrus", "v1.0.6"); other == upper {
+		t.Errorf("differently cased modules share filename %q", other)
+	}
+}
+
+// goCaseUpstream serves different bytes for each module path and counts
+// requests per path.
+func goCaseUpstream(t *testing.T) (*httptest.Server, map[string]*atomic.Int32) {
+	t.Helper()
+	counts := map[string]*atomic.Int32{upperLogrusPath: {}, lowerLogrusPath: {}}
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		count, ok := counts[r.URL.Path]
+		if !ok {
+			http.NotFound(w, r)
+			return
+		}
+		count.Add(1)
+		_, _ = io.WriteString(w, "zip of "+r.URL.Path)
+	}))
+	t.Cleanup(srv.Close)
+	return srv, counts
+}
+
+func newGoCaseHandler(t *testing.T, upstream *httptest.Server) (http.Handler, *Proxy) {
+	t.Helper()
+	proxy, _, store, _ := setupTestProxy(t)
+	store.seekable = true
+	proxy.HTTPClient = upstream.Client()
+	fetcher := fetch.NewFetcher(fetch.WithHTTPClient(upstream.Client()), fetch.WithMaxRetries(0))
+	proxy.Fetcher = fetcher
+	t.Cleanup(func() { _ = fetcher.Close() })
+	return NewGoHandlerWithUpstream(proxy, "http://localhost", upstream.URL).Routes(), proxy
+}
+
+func getGo(h http.Handler, path string) *httptest.ResponseRecorder {
+	w := httptest.NewRecorder()
+	h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, path, nil))
+	return w
+}
+
+func TestGoModulesDifferingInCaseAreCachedApart(t *testing.T) {
+	upstream, counts := goCaseUpstream(t)
+	h, _ := newGoCaseHandler(t, upstream)
+
+	for range 2 {
+		for _, path := range []string{upperLogrusPath, lowerLogrusPath} {
+			if w := getGo(h, path); w.Code != http.StatusOK || w.Body.String() != "zip of "+path {
+				t.Fatalf("GET %s: status = %d, body = %q", path, w.Code, w.Body)
+			}
+		}
+	}
+	for path, count := range counts {
+		if n := count.Load(); n != 1 {
+			t.Errorf("%s fetched %d times, want once and then cached", path, n)
+		}
+	}
+}
+
+// Before module paths kept their case, the uppercase module was cached under
+// the lowercase module's plain filename. The lowercase module must not be
+// served those bytes.
+func TestGoZipCachedForAnotherModuleIsRefetched(t *testing.T) {
+	upstream, counts := goCaseUpstream(t)
+	h, proxy := newGoCaseHandler(t, upstream)
+	seedGoZip(t, proxy, "https://proxy.golang.org"+upperLogrusPath, "zip of "+upperLogrusPath)
+
+	if w := getGo(h, lowerLogrusPath); w.Code != http.StatusOK || w.Body.String() != "zip of "+lowerLogrusPath {
+		t.Fatalf("status = %d, body = %q, want the lowercase module's zip", w.Code, w.Body)
+	}
+	if n := counts[lowerLogrusPath].Load(); n != 1 {
+		t.Errorf("lowercase module fetched %d times, want 1", n)
+	}
+}
+
+// A zip cached from another upstream host for the same module stays valid:
+// only the module path decides whether a cached zip belongs to the request.
+func TestGoZipFromAnotherUpstreamHostIsKept(t *testing.T) {
+	upstream, counts := goCaseUpstream(t)
+	h, proxy := newGoCaseHandler(t, upstream)
+	seedGoZip(t, proxy, "https://old-proxy.example"+lowerLogrusPath, "zip of "+lowerLogrusPath)
+
+	if w := getGo(h, lowerLogrusPath); w.Code != http.StatusOK || w.Body.String() != "zip of "+lowerLogrusPath {
+		t.Fatalf("status = %d, body = %q", w.Code, w.Body)
+	}
+	if n := counts[lowerLogrusPath].Load(); n != 0 {
+		t.Errorf("zip fetched %d times, want it served from the cache", n)
+	}
+}
+
+// seedGoZip caches content as github.com/sirupsen/logrus v1.0.6 under the
+// plain filename, recorded as fetched from upstreamURL.
+func seedGoZip(t *testing.T, proxy *Proxy, upstreamURL, content string) {
+	t.Helper()
+	const storagePath = "golang/logrus/v1.0.6/seed/logrus@v1.0.6.zip"
+	store := proxy.Storage.(*mockStorage)
+	if _, _, err := store.Store(context.Background(), storagePath, strings.NewReader(content)); err != nil {
+		t.Fatalf("seeding storage: %v", err)
+	}
+	pkgPURL, versionPURL, err := packagePURLStrings("golang", "github.com/sirupsen/logrus", "v1.0.6")
+	if err != nil {
+		t.Fatalf("packagePURLStrings: %v", err)
+	}
+	artifact := testArtifact(content, versionPURL, "logrus@v1.0.6.zip", "application/zip")
+	if err := proxy.updateCacheDB("golang", "github.com/sirupsen/logrus", pkgPURL, upstreamURL, storagePath, artifact, ""); err != nil {
+		t.Fatalf("seeding cache record: %v", err)
+	}
+}

internal/server/retention_golang_test.go

@@ -74,19 +74,27 @@ func golangGet(t *testing.T, url string) {
 }
 
 // golangArtifact returns the record of the zip named filename.
+// golangFilenameMatch matches a zip's cached filename: the plain name, or,
+// for a module path with capitals, the plain name with a hash of the path.
+const golangFilenameMatch = `(filename = ? OR filename LIKE ?)`
+
+func golangFilenameArgs(filename string) []any {
+	return []any{filename, strings.TrimSuffix(filename, ".zip") + "~%.zip"}
+}
+
 func golangArtifact(t *testing.T, db *database.DB, filename string) database.Artifact {
 	t.Helper()
 	var art database.Artifact
-	if err := db.Get(&art, db.Rebind(`SELECT * FROM artifacts WHERE filename = ?`), filename); err != nil {
+	if err := db.Get(&art, db.Rebind(`SELECT * FROM artifacts WHERE `+golangFilenameMatch), golangFilenameArgs(filename)...); err != nil {
 		t.Fatalf("reading artifact %s: %v", filename, err)
 	}
 	return art
 }
 
 func golangBackdate(t *testing.T, db *database.DB, filename string, fetched, accessed time.Time) {
 	t.Helper()
-	query := db.Rebind(`UPDATE artifacts SET fetched_at = ?, last_accessed_at = ? WHERE filename = ?`)
-	if _, err := db.Exec(query, fetched, accessed, filename); err != nil {
+	query := db.Rebind(`UPDATE artifacts SET fetched_at = ?, last_accessed_at = ? WHERE ` + golangFilenameMatch)
+	if _, err := db.Exec(query, append([]any{fetched, accessed}, golangFilenameArgs(filename)...)...); err != nil {
 		t.Fatalf("backdating %s: %v", filename, err)
 	}
 }