internal/frontend: move fetchserver and versions to their own packages

Moving fetchserver to its own package will allow us to to
move the fetch logic, which isn't used by cmd/pkgsite because it
doesn't have a postgres database, out of the frontend and pkgsite. The
tests, which depend on a postgres database are moved out too, removing
a few users of the postgres database in the tests. The versions code
is moved into its own package as well so it can be used by the
fetchserver code without depending on the frontend package.

Most of server_test.go, which was testing features that are provided
by what is now the FetchServer has been moved to the fetchserver
package. To ensure that git properly realizes it as a move, the rest
of server_test has been moved to frontend_test.

In some cases I made copies of functions (absoluteTime,
insertTestModules) instead of creating intermediate packages to
contain them.

For golang/go#61399

Change-Id: Ic94419f9d75f766e289cc3fb9a1521173cefb4d1
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/519335
Reviewed-by: Robert Findley <[email protected]>
kokoro-CI: kokoro <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Michael Matloob <[email protected]>

cmd/frontend/main.go

@@ -22,6 +22,7 @@ import (
 	"golang.org/x/pkgsite/internal/fetch"
 	"golang.org/x/pkgsite/internal/fetchdatasource"
 	"golang.org/x/pkgsite/internal/frontend"
+	"golang.org/x/pkgsite/internal/frontend/fetchserver"
 	"golang.org/x/pkgsite/internal/log"
 	"golang.org/x/pkgsite/internal/middleware"
 	"golang.org/x/pkgsite/internal/middleware/timeout"
@@ -100,7 +101,7 @@ func main() {
 		// per-request connection.
 		fetchQueue, err = gcpqueue.New(ctx, cfg, queueName, *workers, expg,
 			func(ctx context.Context, modulePath, version string) (int, error) {
-				return frontend.FetchAndUpdateState(ctx, modulePath, version, proxyClient, sourceClient, db)
+				return fetchserver.FetchAndUpdateState(ctx, modulePath, version, proxyClient, sourceClient, db)
 			})
 		if err != nil {
 			log.Fatalf(ctx, "gcpqueue.New: %v", err)
@@ -115,7 +116,7 @@ func main() {
 	staticSource := template.TrustedSourceFromFlag(flag.Lookup("static").Value)
 	// TODO: Can we use a separate queue for the fetchServer and for the Server?
 	// It would help differentiate ownership.
-	fetchServer := &frontend.FetchServer{
+	fetchServer := &fetchserver.FetchServer{
 		Queue:                fetchQueue,
 		TaskIDChangeInterval: config.TaskIDChangeIntervalFrontend,
 	}
@@ -154,8 +155,8 @@ func main() {
 	views := append(dcensus.ServerViews,
 		postgres.SearchLatencyDistribution,
 		postgres.SearchResponseCount,
-		frontend.FetchLatencyDistribution,
-		frontend.FetchResponseCount,
+		fetchserver.FetchLatencyDistribution,
+		fetchserver.FetchResponseCount,
 		frontend.VersionTypeCount,
 		middleware.CacheResultCount,
 		middleware.CacheErrorCount,

internal/frontend/breadcrumb.go

@@ -8,6 +8,7 @@ import (
 	"path"
 
 	"golang.org/x/pkgsite/internal"
+	"golang.org/x/pkgsite/internal/frontend/versions"
 	"golang.org/x/pkgsite/internal/stdlib"
 	"golang.org/x/pkgsite/internal/version"
 )
@@ -69,7 +70,7 @@ func breadcrumbPath(pkgPath, modPath, requestedVersion string) breadcrumb {
 	for i := 1; i < len(dirs); i++ {
 		href := "/" + dirs[i]
 		if requestedVersion != version.Latest {
-			href += "@" + linkVersion(modPath, requestedVersion, requestedVersion)
+			href += "@" + versions.LinkVersion(modPath, requestedVersion, requestedVersion)
 		}
 		el := dirs[i]
 		if i != len(dirs)-1 {

internal/frontend/client.go

@@ -14,6 +14,7 @@ import (
 
 	"golang.org/x/pkgsite/internal/auth"
 	"golang.org/x/pkgsite/internal/derrors"
+	"golang.org/x/pkgsite/internal/frontend/versions"
 )
 
 // A Client for interacting with the frontend. This is only used for tests.
@@ -40,14 +41,14 @@ func NewClient(url string) *Client {
 
 // GetVersions returns a VersionsDetails for the specified pkgPath.
 // This is only used for tests.
-func (c *Client) GetVersions(pkgPath string) (_ *VersionsDetails, err error) {
+func (c *Client) GetVersions(pkgPath string) (_ *versions.VersionsDetails, err error) {
 	defer derrors.Wrap(&err, "GetVersions(%q)", pkgPath)
 	u := fmt.Sprintf("%s/%s?tab=versions&content=json", c.url, pkgPath)
 	body, err := c.fetchJSONPage(u)
 	if err != nil {
 		return nil, err
 	}
-	var vd VersionsDetails
+	var vd versions.VersionsDetails
 	if err := json.Unmarshal(body, &vd); err != nil {
 		return nil, fmt.Errorf("json.Unmarshal: %v:\nDoes GO_DISCOVERY_SERVE_STATS=true on the frontend?", err)
 	}

internal/frontend/details.go

@@ -15,7 +15,6 @@ import (
 	"golang.org/x/pkgsite/internal/frontend/urlinfo"
 	mstats "golang.org/x/pkgsite/internal/middleware/stats"
 
-	"github.com/google/safehtml/template"
 	"go.opencensus.io/stats"
 	"go.opencensus.io/stats/view"
 	"go.opencensus.io/tag"
@@ -65,7 +64,7 @@ func (s *Server) serveDetails(w http.ResponseWriter, r *http.Request, ds interna
 		}
 	}
 	if !urlinfo.IsSupportedVersion(urlInfo.FullPath, urlInfo.RequestedVersion) {
-		return invalidVersionError(urlInfo.FullPath, urlInfo.RequestedVersion)
+		return serrors.InvalidVersionError(urlInfo.FullPath, urlInfo.RequestedVersion)
 	}
 	if urlPath := stdlibRedirectURL(urlInfo.FullPath); urlPath != "" {
 		http.Redirect(w, r, urlPath, http.StatusMovedPermanently)
@@ -91,30 +90,6 @@ func stdlibRedirectURL(fullPath string) string {
 	return "/" + urlPath2
 }
 
-func invalidVersionError(fullPath, requestedVersion string) error {
-	return &serrors.ServerError{
-		Status: http.StatusBadRequest,
-		Epage: &page.ErrorPage{
-			MessageTemplate: template.MakeTrustedTemplate(`
-					<h3 class="Error-message">{{.Version}} is not a valid semantic version.</h3>
-					<p class="Error-message">
-					  To search for packages like {{.Path}}, <a href="/search?q={{.Path}}">click here</a>.
-					</p>`),
-			MessageData: struct{ Path, Version string }{fullPath, requestedVersion},
-		},
-	}
-}
-
-func datasourceNotSupportedErr() error {
-	return &serrors.ServerError{
-		Status: http.StatusFailedDependency,
-		Epage: &page.ErrorPage{
-			MessageTemplate: template.MakeTrustedTemplate(
-				`<h3 class="Error-message">This page is not supported by this datasource.</h3>`),
-		},
-	}
-}
-
 var (
 	keyVersionType     = tag.MustNewKey("frontend.version_type")
 	versionTypeResults = stats.Int64(

internal/frontend/directory.go

@@ -10,6 +10,7 @@ import (
 	"strings"
 
 	"golang.org/x/pkgsite/internal"
+	"golang.org/x/pkgsite/internal/frontend/versions"
 	"golang.org/x/pkgsite/internal/stdlib"
 	"golang.org/x/pkgsite/internal/version"
 )
@@ -104,7 +105,7 @@ func getNestedModules(ctx context.Context, ds internal.DataSource, um *internal.
 			continue
 		}
 		mods = append(mods, &DirectoryInfo{
-			URL:      constructUnitURL(m.ModulePath, m.ModulePath, version.Latest),
+			URL:      versions.ConstructUnitURL(m.ModulePath, m.ModulePath, version.Latest),
 			Suffix:   suffix,
 			IsModule: true,
 		})
@@ -125,8 +126,8 @@ func getSubdirectories(um *internal.UnitMeta, pkgs []*internal.PackageMeta, requ
 			continue
 		}
 		sdirs = append(sdirs, &DirectoryInfo{
-			URL: constructUnitURL(pm.Path, um.ModulePath,
-				linkVersion(um.ModulePath, requestedVersion, um.Version)),
+			URL: versions.ConstructUnitURL(pm.Path, um.ModulePath,
+				versions.LinkVersion(um.ModulePath, requestedVersion, um.Version)),
 			Suffix:   internal.Suffix(pm.Path, um.Path),
 			Synopsis: pm.Synopsis,
 		})

internal/frontend/404.go → internal/frontend/fetchserver/404.go

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-package frontend
+package fetchserver
 
 import (
 	"context"
@@ -15,7 +15,6 @@ import (
 	"strings"
 	"time"
 
-	"github.com/google/safehtml/template"
 	"github.com/google/safehtml/template/uncheckedconversions"
 	"golang.org/x/pkgsite/internal"
 	"golang.org/x/pkgsite/internal/cookie"
@@ -24,25 +23,12 @@ import (
 	"golang.org/x/pkgsite/internal/frontend/page"
 	"golang.org/x/pkgsite/internal/frontend/serrors"
 	"golang.org/x/pkgsite/internal/frontend/urlinfo"
+	"golang.org/x/pkgsite/internal/frontend/versions"
 	"golang.org/x/pkgsite/internal/log"
 	"golang.org/x/pkgsite/internal/stdlib"
 	"golang.org/x/pkgsite/internal/version"
 )
 
-// errUnitNotFoundWithoutFetch returns a 404 with instructions to the user on
-// how to manually fetch the package. No fetch button is provided. This is used
-// for very large modules or modules that previously 500ed.
-var errUnitNotFoundWithoutFetch = &serrors.ServerError{
-	Status: http.StatusNotFound,
-	Epage: &page.ErrorPage{
-		MessageTemplate: template.MakeTrustedTemplate(`
-					    <h3 class="Error-message">{{.StatusText}}</h3>
-					    <p class="Error-message">Check that you entered the URL correctly or try fetching it following the
-                        <a href="/about#adding-a-package">instructions here</a>.</p>`),
-		MessageData: struct{ StatusText string }{http.StatusText(http.StatusNotFound)},
-	},
-}
-
 // servePathNotFoundPage serves a 404 page for the requested path, or redirects
 // the user to an appropriate location.
 func (s *FetchServer) ServePathNotFoundPage(w http.ResponseWriter, r *http.Request,
@@ -98,7 +84,7 @@ func (s *FetchServer) ServePathNotFoundPage(w http.ResponseWriter, r *http.Reque
 		// We will only reach a 2xx status if we found a row in version_map
 		// matching exactly the requested path.
 		if fr.resolvedVersion != requestedVersion {
-			u := constructUnitURL(fullPath, fr.goModPath, fr.resolvedVersion)
+			u := versions.ConstructUnitURL(fullPath, fr.goModPath, fr.resolvedVersion)
 			http.Redirect(w, r, u, http.StatusFound)
 			return
 		}
@@ -115,16 +101,16 @@ func (s *FetchServer) ServePathNotFoundPage(w http.ResponseWriter, r *http.Reque
 		if fr.goModPath == fullPath {
 			// The redirectPath and the fullpath are the same. Do not redirect
 			// to avoid ending up in a loop.
-			return errUnitNotFoundWithoutFetch
+			return serrors.ErrUnitNotFoundWithoutFetch
 		}
 		vm, err := db.GetVersionMap(ctx, fr.goModPath, version.Latest)
 		if (err != nil && !errors.Is(err, derrors.NotFound)) ||
 			(vm != nil && vm.Status != http.StatusOK) {
 			// We attempted to fetch the canonical module path before and were
 			// not successful. Do not redirect this request.
-			return errUnitNotFoundWithoutFetch
+			return serrors.ErrUnitNotFoundWithoutFetch
 		}
-		u := constructUnitURL(fr.goModPath, fr.goModPath, version.Latest)
+		u := versions.ConstructUnitURL(fr.goModPath, fr.goModPath, version.Latest)
 		cookie.Set(w, cookie.AlternativeModuleFlash, fullPath, u)
 		http.Redirect(w, r, u, http.StatusFound)
 		return nil
@@ -178,14 +164,14 @@ func githubPathRedirect(fullPath string) string {
 	if m[1] != "" {
 		p = m[0] + m[1]
 	}
-	return constructUnitURL(p, p, version.Latest)
+	return versions.ConstructUnitURL(p, p, version.Latest)
 }
 
 // pathNotFoundError returns a page with an option on how to
 // add a package or module to the site.
 func pathNotFoundError(ctx context.Context, fullPath, requestedVersion string) error {
 	if !urlinfo.IsSupportedVersion(fullPath, requestedVersion) {
-		return invalidVersionError(fullPath, requestedVersion)
+		return serrors.InvalidVersionError(fullPath, requestedVersion)
 	}
 	if stdlib.Contains(fullPath) {
 		if experiment.IsActive(ctx, internal.ExperimentEnableStdFrontendFetch) {

internal/frontend/404_test.go → internal/frontend/fetchserver/404_test.go

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-package frontend
+package fetchserver
 
 import (
 	"context"

internal/frontend/fetch.go → internal/frontend/fetchserver/fetch.go

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-package frontend
+package fetchserver
 
 import (
 	"context"
@@ -101,7 +101,7 @@ func (s *FetchServer) ServeFetch(w http.ResponseWriter, r *http.Request, ds inte
 	defer derrors.Wrap(&err, "serveFetch(%q)", r.URL.Path)
 	if _, ok := ds.(internal.PostgresDB); !ok {
 		// There's no reason for other DataSources to need this codepath.
-		return datasourceNotSupportedErr()
+		return serrors.DatasourceNotSupportedError()
 	}
 	if r.Method != http.MethodPost {
 		// If a user makes a GET request, treat this as a request for the

internal/frontend/fetch_test.go → internal/frontend/fetchserver/fetch_test.go

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-package frontend
+package fetchserver
 
 import (
 	"context"
@@ -12,13 +12,19 @@ import (
 	"time"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/safehtml/template"
 	"golang.org/x/pkgsite/internal"
 	"golang.org/x/pkgsite/internal/derrors"
+	"golang.org/x/pkgsite/internal/frontend"
 	"golang.org/x/pkgsite/internal/postgres"
 	"golang.org/x/pkgsite/internal/proxy/proxytest"
+	"golang.org/x/pkgsite/internal/queue"
+	"golang.org/x/pkgsite/internal/source"
 	"golang.org/x/pkgsite/internal/testing/sample"
 	"golang.org/x/pkgsite/internal/testing/testhelper"
 	"golang.org/x/pkgsite/internal/version"
+	"golang.org/x/pkgsite/static"
+	thirdparty "golang.org/x/pkgsite/third_party"
 )
 
 var (
@@ -39,6 +45,45 @@ var (
 	}
 )
 
+func newTestServerWithFetch(t *testing.T, proxyModules []*proxytest.Module, cacher frontend.Cacher) (*frontend.Server, *FetchServer, http.Handler, func()) {
+	t.Helper()
+	proxyClient, teardown := proxytest.SetupTestClient(t, proxyModules)
+	sourceClient := source.NewClient(sourceTimeout)
+	ctx := context.Background()
+
+	q := queue.NewInMemory(ctx, 1, nil,
+		func(ctx context.Context, mpath, version string) (int, error) {
+			return FetchAndUpdateState(ctx, mpath, version, proxyClient, sourceClient, testDB)
+		})
+
+	f := &FetchServer{
+		Queue:                q,
+		TaskIDChangeInterval: 10 * time.Minute,
+	}
+
+	s, err := frontend.NewServer(frontend.ServerConfig{
+		FetchServer:      f,
+		DataSourceGetter: func(context.Context) internal.DataSource { return testDB },
+		Queue:            q,
+		TemplateFS:       template.TrustedFSFromEmbed(static.FS),
+		// Use the embedded FSs here to make sure they're tested.
+		// Integration tests will use the actual directories.
+		StaticFS:     static.FS,
+		ThirdPartyFS: thirdparty.FS,
+		StaticPath:   "../../static",
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	mux := http.NewServeMux()
+	s.Install(mux.Handle, cacher, nil)
+
+	return s, f, mux, func() {
+		teardown()
+		postgres.ResetTestDB(testDB, t)
+	}
+}
+
 func TestFetch(t *testing.T) {
 	for _, test := range []struct {
 		name, fullPath, version, want string
@@ -80,13 +125,13 @@ func TestFetch(t *testing.T) {
 		},
 	} {
 		t.Run(test.name, func(t *testing.T) {
-			s, f, _, teardown := newTestServerWithFetch(t, testModulesForProxy, nil)
+			_, f, _, teardown := newTestServerWithFetch(t, testModulesForProxy, nil)
 			defer teardown()
 
 			ctx, cancel := context.WithTimeout(context.Background(), testFetchTimeout)
 			defer cancel()
 
-			status, responseText := f.fetchAndPoll(ctx, s.getDataSource(ctx), testModulePath, test.fullPath, test.version)
+			status, responseText := f.fetchAndPoll(ctx, testDB, testModulePath, test.fullPath, test.version)
 			if status != http.StatusOK {
 				t.Fatalf("fetchAndPoll(%q, %q, %q) = %d, %s; want status = %d",
 					testModulePath, test.fullPath, test.version, status, responseText, http.StatusOK)
@@ -144,9 +189,9 @@ func TestFetchErrors(t *testing.T) {
 			ctx, cancel := context.WithTimeout(context.Background(), test.fetchTimeout)
 			defer cancel()
 
-			s, f, _, teardown := newTestServerWithFetch(t, testModulesForProxy, nil)
+			_, f, _, teardown := newTestServerWithFetch(t, testModulesForProxy, nil)
 			defer teardown()
-			got, err := f.fetchAndPoll(ctx, s.getDataSource(ctx), test.modulePath, test.fullPath, test.version)
+			got, err := f.fetchAndPoll(ctx, testDB, test.modulePath, test.fullPath, test.version)
 
 			if got != test.want {
 				t.Fatalf("fetchAndPoll(ctx, testDB, q, %q, %q, %q): %d; want = %d",
@@ -181,9 +226,9 @@ func TestFetchPathAlreadyExists(t *testing.T) {
 				t.Fatal(err)
 			}
 
-			s, f, _, teardown := newTestServerWithFetch(t, testModulesForProxy, nil)
+			_, f, _, teardown := newTestServerWithFetch(t, testModulesForProxy, nil)
 			defer teardown()
-			got, _ := f.fetchAndPoll(ctx, s.getDataSource(ctx), sample.ModulePath, sample.PackagePath, sample.VersionString)
+			got, _ := f.fetchAndPoll(ctx, testDB, sample.ModulePath, sample.PackagePath, sample.VersionString)
 			if got != test.want {
 				t.Fatalf("fetchAndPoll for status %d: %d; want = %d)", test.status, got, test.want)
 			}