fix(internal/godocfx): don't save timestamps until modules are successfully processed (#11563)

Author: tbpg

internal/godocfx/index.go

@@ -20,19 +20,23 @@ import (
 	"encoding/json"
 	"log"
 	"net/http"
+	"net/url"
 	"strings"
 	"time"
 )
 
-// indexer gets a limited list of entries from index.golang.org.
-type indexer interface {
-	get(prefixes []string, since time.Time) (entries []indexEntry, last time.Time, err error)
+func mustParse(s string) *url.URL {
+	u, err := url.Parse(s)
+	if err != nil {
+		log.Fatalf("Failed to parse URL: %q", s)
+	}
+	return u
 }
 
 // indexClient is used to access index.golang.org.
-type indexClient struct{}
-
-var _ indexer = indexClient{}
+type indexClient struct {
+	indexURL string
+}
 
 // indexEntry represents a line in the output of index.golang.org/index.
 type indexEntry struct {
@@ -41,47 +45,45 @@ type indexEntry struct {
 	Timestamp time.Time
 }
 
-// newModules returns the new modules with the given prefix.
+// get returns modules with the given prefix that were published since the given timestamp.
 //
-// newModules uses index.golang.org/index?since=timestamp to find new module
-// versions since the given timestamp.
+// get uses index.golang.org/index?since=timestamp to find new module versions.
 //
-// newModules stores the timestamp of the last successful run with tSaver.
-func newModules(ctx context.Context, i indexer, tSaver timeSaver, prefixes []string) ([]indexEntry, error) {
-	since, err := tSaver.get(ctx)
-	if err != nil {
-		return nil, err
-	}
+// get returns the entries and the timestamp of the last seen entry, which may be later
+// than the timestamp of the last returned entry.
+func (ic indexClient) get(ctx context.Context, prefixes []string, since time.Time) ([]indexEntry, time.Time, error) {
 	fiveMinAgo := time.Now().Add(-5 * time.Minute).UTC() // When to stop processing.
 	entries := []indexEntry{}
 	log.Printf("Fetching index.golang.org entries since %s", since.Format(time.RFC3339))
-	count := 0
-	for {
-		count++
+	pages := 0
+	lastSeen := since
+	for !lastSeen.After(fiveMinAgo) {
+		log.Printf("last seen: %v", lastSeen.Format(time.RFC3339))
+		pages++
 		var cur []indexEntry
-		cur, since, err = i.get(prefixes, since)
+		var err error
+		cur, lastSeen, err = ic.getPage(prefixes, lastSeen)
 		if err != nil {
-			return nil, err
+			return nil, time.Time{}, err
 		}
 		entries = append(entries, cur...)
-		if since.After(fiveMinAgo) {
-			break
-		}
-	}
-	log.Printf("Parsed %d index.golang.org pages up to %s", count, since.Format(time.RFC3339))
-	if err := tSaver.put(ctx, since); err != nil {
-		return nil, err
 	}
+	log.Printf("Parsed %d index.golang.org pages from %s to %s", pages, since.Format(time.RFC3339), lastSeen.Format(time.RFC3339))
 
-	return entries, nil
+	return entries, lastSeen, nil
 }
 
 // get fetches a single chronological page of modules from
 // index.golang.org/index.
-func (indexClient) get(prefixes []string, since time.Time) ([]indexEntry, time.Time, error) {
+func (ic indexClient) getPage(prefixes []string, since time.Time) ([]indexEntry, time.Time, error) {
 	entries := []indexEntry{}
-	sinceString := since.Format(time.RFC3339)
-	resp, err := http.Get("https://index.golang.org/index?since=" + sinceString)
+
+	u := mustParse(ic.indexURL)
+	q := u.Query()
+	q.Set("since", since.Format(time.RFC3339))
+	u.RawQuery = q.Encode()
+	log.Printf("Fetching %s", u.String())
+	resp, err := http.Get(u.String())
 	if err != nil {
 		return nil, time.Time{}, err
 	}

internal/godocfx/index_test.go

@@ -16,48 +16,64 @@ package main
 
 import (
 	"context"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
 	"testing"
 	"time"
 )
 
 const wantEntries = 5
 
-type fakeIC struct{}
+// responses is used for faking HTTP responses. Format each value with a timestamp based on whether get() should
+// fetch another page.
+var responses = []string{
+	`{"Path":"golang.org/x/net","Version":"v0.0.0-20180627171509-e514e69ffb8b","Timestamp":"2019-04-10T20:30:42.413493Z"}
+	 {"Path":"golang.org/x/exp/notary","Version":"v0.0.0-20190409044807-56b785ea58b2","Timestamp":"2019-04-10T20:37:01.409908Z"}
+	 {"Path":"golang.org/x/crypto","Version":"v0.0.0-20181025213731-e84da0312774","Timestamp":"2019-04-10T20:40:43.408586Z"}
+	 {"Path":"golang.org/x/net","Version":"v0.0.0-20181213202711-891ebc4b82d6","Timestamp":"2019-04-10T20:40:54.12906Z"}
+	 {"Path":"cloud.google.com/go","Version":"v1.0.0","Timestamp":"%s"}`,
 
-func (f fakeIC) get(prefixes []string, since time.Time) (entries []indexEntry, last time.Time, err error) {
-	e := indexEntry{Timestamp: since.Add(24 * time.Hour)}
-	return []indexEntry{e}, e.Timestamp, nil
+	`{"Path":"cloud.google.com/go/storage","Version":"v1.0.0","Timestamp":"2020-04-10T20:40:43.408586Z"}
+	 {"Path":"cloud.google.com/go/bigquery","Version":"v1.0.0","Timestamp":"2020-04-10T20:40:43.408586Z"}
+	 {"Path":"golang.org/x/crypto","Version":"v0.0.0-20181025213731-e84da0312774","Timestamp":"2020-04-10T20:40:43.408586Z"}
+	 {"Path":"cloud.google.com/go/spanner","Version":"v1.0.0","Timestamp":"2020-04-10T20:40:43.408586Z"}
+	 {"Path":"cloud.google.com/go","Version":"v1.0.0","Timestamp":"%s"}`,
 }
 
-type fakeTS struct {
-	getCalled, putCalled bool
-}
-
-func (f *fakeTS) get(context.Context) (time.Time, error) {
-	f.getCalled = true
-	t := time.Now().Add(-wantEntries * 24 * time.Hour).UTC()
-	return t, nil
-}
+func TestGet(t *testing.T) {
+	numCalls := 0
+	lastTime := time.Now().Add(-time.Minute).Format(time.RFC3339)
+	s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		numCalls++
+		if numCalls == 1 {
+			resp := []byte(fmt.Sprintf(responses[0], time.Now().Add(-time.Hour).Format(time.RFC3339)))
+			w.Write(resp)
+			return
+		}
+		if numCalls == 2 {
+			resp := []byte(fmt.Sprintf(responses[1], lastTime))
+			w.Write(resp)
+			return
+		}
+		t.Fatalf("Unexpected call #%d to server", numCalls)
+	}))
 
-func (f *fakeTS) put(context.Context, time.Time) error {
-	f.putCalled = true
-	return nil
-}
+	ic := indexClient{indexURL: s.URL}
 
-func TestNewModules(t *testing.T) {
-	ic := fakeIC{}
-	ts := &fakeTS{}
-	entries, err := newModules(context.Background(), ic, ts, []string{"cloud.google.com"})
+	entries, last, err := ic.get(context.Background(), []string{"cloud.google.com"}, time.Time{})
 	if err != nil {
-		t.Fatalf("newModules got err: %v", err)
+		t.Fatalf("get got err: %v", err)
 	}
 	if got, want := len(entries), wantEntries; got != want {
-		t.Errorf("newModules got %d entries, want %d", got, want)
+		t.Errorf("get got %d entries, want %d", got, want)
 	}
-	if !ts.getCalled {
-		t.Errorf("fakeTS.get was never called")
+
+	want, err := time.Parse(time.RFC3339, lastTime)
+	if err != nil {
+		t.Fatalf("Failed to parse time: %v", err)
 	}
-	if !ts.putCalled {
-		t.Errorf("fakeTS.put was never called")
+	if !last.Equal(want) {
+		t.Errorf("get got last time %v, want %v", last, lastTime)
 	}
 }

internal/godocfx/main.go

@@ -40,7 +40,6 @@ import (
 	"flag"
 	"fmt"
 	"io"
-	"io/ioutil"
 	"log"
 	"os"
 	"os/exec"
@@ -67,16 +66,25 @@ func main() {
 		log.Fatalf("%s missing required argument: module path/prefix", os.Args[0])
 	}
 
+	tSaver := &dsTimeSaver{projectID: *projectID}
+	var lastSeen time.Time
+
+	ctx := context.Background()
 	modNames := flag.Args()
 	var mods []indexEntry
 	if *newMods {
 		if *projectID == "" {
 			log.Fatal("Must set -project when using -new-modules")
 		}
 		var err error
-		mods, err = newModules(context.Background(), indexClient{}, &dsTimeSaver{projectID: *projectID}, modNames)
+		lastSeen, err = tSaver.get(ctx)
+		if err != nil {
+			log.Fatalf("Failed to get latest timestamp: %v", err)
+		}
+		ic := indexClient{indexURL: "https://index.golang.org/index"}
+		mods, lastSeen, err = ic.get(ctx, modNames, lastSeen)
 		if err != nil {
-			log.Fatal(err)
+			log.Fatalf("Failed to get from index: %v", err)
 		}
 	} else {
 		for _, mod := range modNames {
@@ -108,10 +116,16 @@ func main() {
 	namer := &friendlyAPINamer{
 		metaURL: "https://raw.githubusercontent.com/googleapis/google-cloud-go/main/internal/.repo-metadata-full.json",
 	}
-	optionalExtraFiles := []string{}
-	if ok := processMods(mods, *outDir, namer, optionalExtraFiles, *print); !ok {
+	if ok := processMods(mods, *outDir, namer, nil, *print); !ok {
 		os.Exit(1)
 	}
+
+	// Only save the latest timestamp if we successfully processed all modules.
+	if *newMods {
+		if err := tSaver.put(ctx, lastSeen); err != nil {
+			log.Fatalf("Failed to save latest timestamp: %v", err)
+		}
+	}
 }
 
 func runCmd(dir, name string, args ...string) error {
@@ -121,19 +135,19 @@ func runCmd(dir, name string, args ...string) error {
 	cmd.Stdout = os.Stdout
 	cmd.Stderr = os.Stderr
 	if err := cmd.Start(); err != nil {
-		return fmt.Errorf("Start: %v", err)
+		return fmt.Errorf("cmd.Start: %v", err)
 	}
 	if err := cmd.Wait(); err != nil {
-		return fmt.Errorf("Wait: %s", err)
+		return fmt.Errorf("cmd.Wait: %s", err)
 	}
 	return nil
 }
 
 func processMods(mods []indexEntry, outDir string, namer *friendlyAPINamer, optionalExtraFiles []string, print bool) bool {
 	// Create a temp module so we can get the exact version asked for.
-	workingDir, err := ioutil.TempDir("", "godocfx-*")
+	workingDir, err := os.MkdirTemp("", "godocfx-*")
 	if err != nil {
-		log.Fatalf("ioutil.TempDir: %v", err)
+		log.Fatalf("os.MkdirTemp: %v", err)
 	}
 	// Use a fake module that doesn't start with cloud.google.com/go.
 	runCmd(workingDir, "go", "mod", "init", "cloud.google.com/lets-build-some-docs")