diff --git a/internal/frontend/details.go b/internal/frontend/details.go index ac437b5a..2f8cec7f 100644 --- a/internal/frontend/details.go +++ b/internal/frontend/details.go @@ -96,6 +96,13 @@ func (s *Server) servePackagePage(w http.ResponseWriter, r *http.Request, pkgPat s.serveDirectoryPage(w, r, pkgPath, modulePath, version) return } + // Temporary patch: StatusNotFound should not be used unless we're setting + // the Location header, but we can't set it earlier or we'll change the + // handling logic. + // TODO(b/144031201): remove this after refactoring. + if code == http.StatusSeeOther { + code = http.StatusNotFound + } s.serveErrorPage(w, r, code, epage) return } @@ -160,6 +167,13 @@ func (s *Server) serveModulePage(w http.ResponseWriter, r *http.Request, moduleP return modulePath, err }) if code != http.StatusOK { + // Temporary patch: StatusNotFound should not be used unless we're setting + // the Location header, but we can't set it earlier or we'll change the + // handling logic. + // TODO(b/144031201): remove this after refactoring. + if code == http.StatusSeeOther { + code = http.StatusNotFound + } s.serveErrorPage(w, r, code, epage) return } diff --git a/internal/testing/htmlcheck/htmlcheck.go b/internal/testing/htmlcheck/htmlcheck.go index a9697667..d6cc7668 100644 --- a/internal/testing/htmlcheck/htmlcheck.go +++ b/internal/testing/htmlcheck/htmlcheck.go @@ -8,6 +8,7 @@ package htmlcheck import ( "fmt" + "io" "regexp" "strings" @@ -18,6 +19,16 @@ import ( // A Checker is a function from an HTML node to an error describing a failure. type Checker func(*html.Node) error +// Run is a convenience function to run the checker against HTML read from +// reader. +func Run(reader io.Reader, checker Checker) error { + node, err := html.Parse(reader) + if err != nil { + return err + } + return checker(node) +} + // In returns a Checker that applies the given checkers to the first node // matching the CSS selector. The empty selector denotes the entire subtree of // the Checker's argument node. diff --git a/internal/testing/integration/frontend_test.go b/internal/testing/integration/frontend_test.go new file mode 100644 index 00000000..7163c0ca --- /dev/null +++ b/internal/testing/integration/frontend_test.go @@ -0,0 +1,186 @@ +// Copyright 2019 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package integration + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "golang.org/x/discovery/internal/etl" + "golang.org/x/discovery/internal/frontend" + "golang.org/x/discovery/internal/middleware" + "golang.org/x/discovery/internal/postgres" + "golang.org/x/discovery/internal/proxy" + "golang.org/x/discovery/internal/testing/htmlcheck" + "golang.org/x/discovery/internal/testing/testhelper" +) + +var ( + in = htmlcheck.In + hasText = htmlcheck.HasText +) + +func TestModulePackageDirectoryResolution(t *testing.T) { + // The shared test state sets up the following scenario to exercise the types + // of problems discussed in b/143814014: a directory that becomes a package, + // and then becomes a directory again. Specifically: + // + at v1.2.3, github.com/golang/found/dir is a directory (containing dir/pkg) + // + at v1.2.4, github.com/golang/found/dir is a package + // + at v1.2.5, github.com/golang/found/dir is again just a directory + versions := []*proxy.TestVersion{ + proxy.NewTestVersion(t, "github.com/golang/found", "v1.2.3", map[string]string{ + "found.go": "package found\nconst Value = 123", + "dir/pkg/pkg.go": "package pkg\nconst Value = 321", + "LICENSE": testhelper.MITLicense, + }), + proxy.NewTestVersion(t, "github.com/golang/found", "v1.2.4", map[string]string{ + "found.go": "package found\nconst Value = 124", + "dir/pkg/pkg.go": "package pkg\nconst Value = 421", + "dir/dir.go": "package dir\nconst Value = \"I'm a package!\"", + "LICENSE": testhelper.MITLicense, + }), + proxy.NewTestVersion(t, "github.com/golang/found", "v1.2.5", map[string]string{ + "found.go": "package found\nconst Value = 125", + "dir/pkg/pkg.go": "package pkg\nconst Value = 521", + "LICENSE": testhelper.MITLicense, + }), + } + + tests := []struct { + // Test description. + desc string + // URL Path (relative to the test server) to check + urlPath string + // The expected HTTP status code. + wantCode int + // If non-nil, used to verify the resulting page. + want htmlcheck.Checker + }{ + { + desc: "missing module", + urlPath: "/mod/github.com/golang/missing", + wantCode: http.StatusNotFound, + }, + { + desc: "latest module", + urlPath: "/mod/github.com/golang/found", + wantCode: http.StatusOK, + want: in(".DetailsHeader", hasText("v1.2.5")), + }, + { + desc: "versioned module", + urlPath: "/mod/github.com/golang/found@v1.2.3", + wantCode: http.StatusOK, + want: in(".DetailsHeader", hasText("v1.2.3")), + }, + { + desc: "non-existent version", + urlPath: "/mod/github.com/golang/found@v1.1.3", + wantCode: http.StatusNotFound, + want: in(".Content", + hasText("not available"), + hasText("other versions of this module")), + }, + { + desc: "latest package", + urlPath: "/github.com/golang/found/dir/pkg", + wantCode: http.StatusOK, + want: in("", + in(".DetailsHeader", hasText("v1.2.5")), + in(".DetailsContent", hasText("521"))), + }, + { + desc: "earlier package", + urlPath: "/github.com/golang/found@v1.2.3/dir/pkg", + wantCode: http.StatusOK, + want: in("", + in(".DetailsHeader", hasText("v1.2.3")), + in(".DetailsContent", hasText("321"))), + }, + // This test fails, because the fetchPackageOrModule logic instead returns + // 404 and suggests a search for the later package. + // TODO(b/143814014): uncomment once this bug is fixed. + // { + // desc: "dir is initially a directory", + // urlPath: "/github.com/golang/found@v1.2.3/dir", + // wantCode: http.StatusOK, + // want: in(".Directories", hasText("pkg")), + // }, + { + desc: "dir becomes a package", + urlPath: "/github.com/golang/found@v1.2.4/dir", + wantCode: http.StatusOK, + want: in("", + in(".DetailsHeader", hasText("v1.2.4")), + in(".DetailsContent", hasText("I'm a package"))), + }, + // This test fails, because fetchPackageOrMOdule again suggests a search + // for the (now earlier) package. + // TODO(b/143814014): uncomment once this bug is fixed. + // { + // desc: "dir becomes a directory again", + // urlPath: "/github.com/golang/found@v1.2.5/dir", + // wantCode: http.StatusOK, + // want: in(".Directories", hasText("pkg")), + // }, + // This test fails, because we currently go to the latest version of + // found/dir, which is v1.2.4. We should instead serve the directory from + // the latest version of the module + // TODO(b/143814014): uncomment once this bug is fixed. + // { + // desc: "latest directory", + // urlPath: "/github.com/golang/found/dir", + // wantCode: http.StatusOK, + // want: in(".Directories", hasText("pkg")), + // }, + } + s, err := frontend.NewServer(testDB, nil, "../../../content/static", false) + if err != nil { + t.Fatal(err) + } + mux := http.NewServeMux() + s.Install(mux.Handle, nil) + handler := middleware.LatestVersion(s.LatestVersion)(mux) + ts := httptest.NewServer(handler) + processVersions(context.Background(), t, versions) + defer postgres.ResetTestDB(testDB, t) + + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + resp, err := http.Get(ts.URL + test.urlPath) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + if resp.StatusCode != test.wantCode { + t.Errorf("GET %s returned status %d, want %d", test.urlPath, resp.StatusCode, test.wantCode) + } + if test.want != nil { + if err := htmlcheck.Run(resp.Body, test.want); err != nil { + t.Error(err) + } + } + }) + } + +} + +func processVersions(ctx context.Context, t *testing.T, testVersions []*proxy.TestVersion) { + t.Helper() + proxyClient, teardown := proxy.SetupTestProxy(t, testVersions) + defer teardown() + + for _, tv := range testVersions { + v, _, err := etl.FetchVersion(ctx, tv.ModulePath, tv.Version, proxyClient) + if err != nil { + t.Fatal(err) + } + if err := testDB.InsertVersion(ctx, v); err != nil { + t.Fatal(err) + } + } +}