Use entity tag to expire sample apps cache

3 files changed, 192 insertions, 67 deletions

diff --git a/client/go/cmd/clone.go b/client/go/cmd/clone.go
index 66e17d2523f..03a55d839d6 100644
--- a/client/go/cmd/clone.go
+++ b/client/go/cmd/clone.go
@@ -6,14 +6,13 @@ package cmd
 
 import (
 	"archive/zip"
-	"errors"
 	"fmt"
 	"io"
-	"io/ioutil"
 	"log"
 	"net/http"
 	"os"
 	"path/filepath"
+	"sort"
 	"strings"
 	"time"
 
@@ -21,10 +20,12 @@ import (
 	"github.com/spf13/cobra"
 )
 
+const sampleAppsNamePrefix = "sample-apps-master"
+
 func newCloneCmd(cli *CLI) *cobra.Command {
 	var (
-		listApps   bool
-		forceClone bool
+		listApps bool
+		noCache  bool
 	)
 	cmd := &cobra.Command{
 		Use:   "clone sample-application-path target-directory",
@@ -55,31 +56,37 @@ variable.`,
 			if len(args) != 2 {
 				return fmt.Errorf("expected exactly 2 arguments, got %d", len(args))
 			}
-			cloner := &cloner{cli: cli, discardCache: forceClone}
+			cloner := &cloner{cli: cli, noCache: noCache}
 			return cloner.Clone(args[0], args[1])
 		},
 	}
 	cmd.Flags().BoolVarP(&listApps, "list", "l", false, "List available sample applications")
-	cmd.Flags().BoolVarP(&forceClone, "force", "f", false, "Ignore cache and force downloading the latest sample application from GitHub")
+	cmd.Flags().BoolVarP(&noCache, "force", "f", false, "Ignore cache and force downloading the latest sample application from GitHub")
 	return cmd
 }
 
 type cloner struct {
-	cli          *CLI
-	discardCache bool
+	cli     *CLI
+	noCache bool
+}
+
+type zipFile struct {
+	path    string
+	etag    string
+	modTime time.Time
 }
 
 // Clone copies the application identified by applicationName into given path. If the cached copy of sample applications
-// has expired, it will be downloaded from GitHub automatically.
+// has expired (as determined by its entity tag), a current copy will be downloaded from GitHub automatically.
 func (c *cloner) Clone(applicationName, path string) error {
-	zipName, err := c.zipName()
+	zipPath, err := c.zipPath()
 	if err != nil {
 		return err
 	}
 
-	r, err := zip.OpenReader(zipName)
+	r, err := zip.OpenReader(zipPath)
 	if err != nil {
-		return fmt.Errorf("could not open sample apps zip '%s': %w", color.CyanString(zipName), err)
+		return fmt.Errorf("could not open sample apps zip '%s': %w", color.CyanString(zipPath), err)
 	}
 	defer r.Close()
 
@@ -100,6 +107,7 @@ func (c *cloner) Clone(applicationName, path string) error {
 			}
 		}
 	}
+
 	if !found {
 		return errHint(fmt.Errorf("could not find source application '%s'", color.CyanString(applicationName)), "Use -f to ignore the cache")
 	} else {
@@ -108,71 +116,147 @@ func (c *cloner) Clone(applicationName, path string) error {
 	return nil
 }
 
-func (c *cloner) useCache(stat os.FileInfo) (bool, error) {
-	if c.discardCache {
-		return false, nil
+// zipPath returns the path to the latest sample application ZIP file.
+func (c *cloner) zipPath() (string, error) {
+	zipFiles, err := c.listZipFiles()
+	if err != nil {
+		return "", nil
+	}
+	cacheCandidates := zipFiles
+	if c.noCache {
+		cacheCandidates = nil
+	}
+	zipPath, cacheHit, err := c.downloadZip(cacheCandidates)
+	if err != nil {
+		if cacheHit {
+			c.cli.printWarning(err)
+		} else {
+			return "", err
+		}
 	}
-	expiry := stat.ModTime().Add(time.Hour * 168) // 1 week
-	return stat.Size() > 0 && time.Now().Before(expiry), nil
+	if cacheHit {
+		log.Print(color.YellowString("Using cached sample apps ..."))
+	}
+	// Remove obsolete files
+	for _, zf := range zipFiles {
+		if zf.path != zipPath {
+			os.Remove(zf.path)
+		}
+	}
+	return zipPath, nil
 }
 
-func (c *cloner) downloadZip(dst string) error {
-	f, err := ioutil.TempFile(filepath.Dir(dst), "sample-apps")
+// listZipFiles list all sample apps ZIP files found in cacheDir.
+func (c *cloner) listZipFiles() ([]zipFile, error) {
+	dirEntries, err := os.ReadDir(c.cli.config.cacheDir)
 	if err != nil {
-		return fmt.Errorf("could not create temporary file: %w", err)
+		return nil, err
+	}
+	var zipFiles []zipFile
+	for _, entry := range dirEntries {
+		ext := filepath.Ext(entry.Name())
+		if ext != ".zip" {
+			continue
+		}
+		if !strings.HasPrefix(entry.Name(), sampleAppsNamePrefix) {
+			continue
+		}
+		fi, err := entry.Info()
+		if err != nil {
+			return nil, err
+		}
+		name := fi.Name()
+		etag := ""
+		parts := strings.Split(name, "_")
+		if len(parts) == 2 {
+			etag = strings.TrimSuffix(parts[1], ext)
+		}
+		zipFiles = append(zipFiles, zipFile{
+			path:    filepath.Join(c.cli.config.cacheDir, name),
+			etag:    etag,
+			modTime: fi.ModTime(),
+		})
 	}
-	defer f.Close()
-	return c.cli.spinner(c.cli.Stderr, color.YellowString("Downloading sample apps ..."), func() error {
+	return zipFiles, nil
+}
+
+// downloadZip conditionally downloads the latest sample apps ZIP file. If any of the ZIP files among cacheFiles are
+// usable, downloading is skipped.
+func (c *cloner) downloadZip(cachedFiles []zipFile) (string, bool, error) {
+	zipPath := ""
+	etag := ""
+	sort.Slice(cachedFiles, func(i, j int) bool { return cachedFiles[i].modTime.Before(cachedFiles[j].modTime) })
+	if len(cachedFiles) > 0 {
+		latest := cachedFiles[len(cachedFiles)-1]
+		zipPath = latest.path
+		etag = latest.etag
+	}
+	// The latest cached file, if any, is considered a hit until we have downloaded a fresh one. This allows us to use
+	// the cached copy if GitHub is unavailable.
+	cacheHit := zipPath != ""
+	err := c.cli.spinner(c.cli.Stderr, color.YellowString("Downloading sample apps ..."), func() error {
 		request, err := http.NewRequest("GET", "https://github.com/vespa-engine/sample-apps/archive/refs/heads/master.zip", nil)
 		if err != nil {
 			return fmt.Errorf("invalid url: %w", err)
 		}
+		if etag != "" {
+			request.Header = make(http.Header)
+			request.Header.Set("if-none-match", fmt.Sprintf(`W/"%s"`, etag))
+		}
 		response, err := c.cli.httpClient.Do(request, time.Minute*60)
 		if err != nil {
 			return fmt.Errorf("could not download sample apps: %w", err)
 		}
 		defer response.Body.Close()
+		if response.StatusCode == http.StatusNotModified { // entity tag matched so our cached copy is current
+			return nil
+		}
 		if response.StatusCode != http.StatusOK {
 			return fmt.Errorf("could not download sample apps: github returned status %d", response.StatusCode)
 		}
-		if _, err := io.Copy(f, response.Body); err != nil {
-			return fmt.Errorf("could not write sample apps to file: %s: %w", f.Name(), err)
-		}
-		f.Close()
-		if err := os.Rename(f.Name(), dst); err != nil {
-			return fmt.Errorf("could not move sample apps to cache path")
+		etag = trimEntityTagID(response.Header.Get("etag"))
+		newPath, err := c.writeZip(response.Body, etag)
+		if err != nil {
+			return err
 		}
+		zipPath = newPath
+		cacheHit = false
 		return nil
 	})
+	return zipPath, cacheHit, err
 }
 
-func (c *cloner) zipName() (string, error) {
-	cacheDir, err := vespaCliCacheDir(c.cli.Environment)
+// writeZip atomically writes the contents of reader zipReader to a file in the CLI cache directory.
+func (c *cloner) writeZip(zipReader io.Reader, etag string) (string, error) {
+	f, err := os.CreateTemp(c.cli.config.cacheDir, "sample-apps-tmp-")
 	if err != nil {
-		return "", err
-	}
-	dst := filepath.Join(cacheDir, "sample-apps-master.zip")
-	cacheExists := true
-	stat, err := os.Stat(dst)
-	if errors.Is(err, os.ErrNotExist) {
-		cacheExists = false
-	} else if err != nil {
-		return "", fmt.Errorf("could not stat existing cache file: %w", err)
-	}
-	if cacheExists {
-		useCache, err := c.useCache(stat)
-		if err != nil {
-			return "", errHint(fmt.Errorf("could not determine cache status: %w", err), "Try ignoring the cache with the -f flag")
-		}
-		if useCache {
-			log.Print(color.YellowString("Using cached sample apps ..."))
-			return dst, nil
+		return "", fmt.Errorf("could not create temporary file: %w", err)
+	}
+	cleanTemp := true
+	defer func() {
+		f.Close()
+		if cleanTemp {
+			os.Remove(f.Name())
 		}
+	}()
+	if _, err := io.Copy(f, zipReader); err != nil {
+		return "", fmt.Errorf("could not write sample apps to file: %s: %w", f.Name(), err)
+	}
+	f.Close()
+	path := filepath.Join(c.cli.config.cacheDir, sampleAppsNamePrefix)
+	if etag != "" {
+		path += "_" + etag
 	}
-	if err := c.downloadZip(dst); err != nil {
-		return "", fmt.Errorf("could not fetch sample apps: %w", err)
+	path += ".zip"
+	if err := os.Rename(f.Name(), path); err != nil {
+		return "", fmt.Errorf("could not move sample apps to %s", path)
 	}
-	return dst, nil
+	cleanTemp = false
+	return path, nil
+}
+
+func trimEntityTagID(s string) string {
+	return strings.TrimSuffix(strings.TrimPrefix(s, `W/"`), `"`)
 }
 
 func copy(f *zip.File, destinationDir string, zipEntryPrefix string) error {
diff --git a/client/go/cmd/clone_test.go b/client/go/cmd/clone_test.go
index 9587a1435d3..1971e0032a4 100644
--- a/client/go/cmd/clone_test.go
+++ b/client/go/cmd/clone_test.go
@@ -5,9 +5,10 @@
 package cmd
 
 import (
-	"io/ioutil"
+	"net/http"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -21,28 +22,60 @@ func TestClone(t *testing.T) {
 }
 
 func assertCreated(sampleAppName string, app string, t *testing.T) {
-	appCached := app + "-cache"
+	tempDir := t.TempDir()
+	app1 := filepath.Join(tempDir, "app1")
 	defer os.RemoveAll(app)
-	defer os.RemoveAll(appCached)
 
 	httpClient := &mock.HTTPClient{}
-	testdata, err := ioutil.ReadFile(filepath.Join("testdata", "sample-apps-master.zip"))
+	cli, stdout, stderr := newTestCLI(t)
+	cli.httpClient = httpClient
+	testdata, err := os.ReadFile(filepath.Join("testdata", "sample-apps-master.zip"))
 	require.Nil(t, err)
+
+	// Initial cloning. GitHub includes the ETag header, but we don't require it
 	httpClient.NextResponseBytes(200, testdata)
+	require.Nil(t, cli.Run("clone", sampleAppName, app1))
+	assert.Equal(t, "Created "+app1+"\n", stdout.String())
+	assertFiles(t, app1)
 
-	cli, stdout, _ := newTestCLI(t)
-	cli.httpClient = httpClient
-	err = cli.Run("clone", sampleAppName, app)
-	assert.Nil(t, err)
+	// Clone with cache hit
+	httpClient.NextStatus(http.StatusNotModified)
+	stdout.Reset()
+	app2 := filepath.Join(tempDir, "app2")
+	require.Nil(t, cli.Run("clone", sampleAppName, app2))
+	assert.Equal(t, "Using cached sample apps ...\nCreated "+app2+"\n", stdout.String())
+	assertFiles(t, app2)
 
-	assert.Equal(t, "Created "+app+"\n", stdout.String())
-	assertFiles(t, app)
+	// Clone while ignoring cache
+	headers := make(http.Header)
+	headers.Set("etag", `W/"id1"`)
+	httpClient.NextResponse(mock.HTTPResponse{Status: 200, Body: testdata, Header: headers})
+	stdout.Reset()
+	app3 := filepath.Join(tempDir, "app3")
+	require.Nil(t, cli.Run("clone", "-f", sampleAppName, app3))
+	assert.Equal(t, "Created "+app3+"\n", stdout.String())
+	assertFiles(t, app3)
 
+	// Cloning falls back to cached copy if GitHub is unavailable
+	httpClient.NextStatus(500)
 	stdout.Reset()
-	err = cli.Run("clone", sampleAppName, appCached)
-	assert.Nil(t, err)
-	assert.Equal(t, "Using cached sample apps ...\nCreated "+appCached+"\n", stdout.String())
-	assertFiles(t, appCached)
+	app4 := filepath.Join(tempDir, "app4")
+	require.Nil(t, cli.Run("clone", "-f=false", sampleAppName, app4))
+	assert.Equal(t, "Warning: could not download sample apps: github returned status 500\n", stderr.String())
+	assert.Equal(t, "Using cached sample apps ...\nCreated "+app4+"\n", stdout.String())
+	assertFiles(t, app4)
+
+	// The only cached file is the latest one
+	dirEntries, err := os.ReadDir(cli.config.cacheDir)
+	require.Nil(t, err)
+	var zipFiles []string
+	for _, de := range dirEntries {
+		name := de.Name()
+		if strings.HasPrefix(name, sampleAppsNamePrefix) {
+			zipFiles = append(zipFiles, name)
+		}
+	}
+	assert.Equal(t, []string{"sample-apps-master_id1.zip"}, zipFiles)
 }
 
 func assertFiles(t *testing.T, app string) {
@@ -50,10 +83,12 @@ func assertFiles(t *testing.T, app string) {
 	assert.True(t, util.PathExists(filepath.Join(app, "src", "main", "application")))
 	assert.True(t, util.IsDirectory(filepath.Join(app, "src", "main", "application")))
 
-	servicesStat, _ := os.Stat(filepath.Join(app, "src", "main", "application", "services.xml"))
+	servicesStat, err := os.Stat(filepath.Join(app, "src", "main", "application", "services.xml"))
+	require.Nil(t, err)
 	servicesSize := int64(1772)
 	assert.Equal(t, servicesSize, servicesStat.Size())
 
-	scriptStat, _ := os.Stat(filepath.Join(app, "bin", "convert-msmarco.sh"))
+	scriptStat, err := os.Stat(filepath.Join(app, "bin", "convert-msmarco.sh"))
+	require.Nil(t, err)
 	assert.Equal(t, os.FileMode(0755), scriptStat.Mode())
 }
diff --git a/client/go/cmd/config.go b/client/go/cmd/config.go
index 75bd9959280..b75ae046534 100644
--- a/client/go/cmd/config.go
+++ b/client/go/cmd/config.go
@@ -95,6 +95,7 @@ $ vespa config get target`,
 
 type Config struct {
 	homeDir     string
+	cacheDir    string
 	environment map[string]string
 	bindings    ConfigBindings
 	createDirs  bool
@@ -131,8 +132,13 @@ func loadConfig(environment map[string]string, bindings ConfigBindings) (*Config
 	if err != nil {
 		return nil, fmt.Errorf("could not detect config directory: %w", err)
 	}
+	cacheDir, err := vespaCliCacheDir(environment)
+	if err != nil {
+		return nil, fmt.Errorf("could not detect cache directory: %w", err)
+	}
 	c := &Config{
 		homeDir:     home,
+		cacheDir:    cacheDir,
 		environment: environment,
 		bindings:    bindings,
 		createDirs:  true,