internal/api: refactor error handling to increase consistency

- Refactored error handling to avoid leaking internal implementation
  details. Database and system errors are masked by falling back to
  standard HTTP statuses but still logging the entire error context.
- User facing error messages can now be specified within the Error
  struct.
- Added helpers in types.go to simplify error construction.
- Updated ServeModuleVersions to explicitly return 404 when no versions
  are found.
- Expanded test coverage in api_test.go to include 404 and 400 edge
  cases.

Change-Id: I89c4be3941126c15df6aefdd21e4bbd2d3b23be1
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/764820
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Ethan Lee <ethanalee@google.com>
kokoro-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>

5 files changed, 130 insertions, 38 deletions

diff --git a/internal/api/api.go b/internal/api/api.go
index 7a643271..87339e19 100644
--- a/internal/api/api.go
+++ b/internal/api/api.go
@@ -17,6 +17,7 @@ import (
 	"golang.org/x/pkgsite/internal"
 	"golang.org/x/pkgsite/internal/derrors"
 	"golang.org/x/pkgsite/internal/godoc"
+	"golang.org/x/pkgsite/internal/log"
 	"golang.org/x/pkgsite/internal/stdlib"
 	"golang.org/x/pkgsite/internal/version"
 	"golang.org/x/pkgsite/internal/vuln"
@@ -35,7 +36,7 @@ func ServePackage(w http.ResponseWriter, r *http.Request, ds internal.DataSource
 
 	pkgPath := trimPath(r, "/v1/package/")
 	if pkgPath == "" {
-		return fmt.Errorf("%w: missing package path", derrors.InvalidArgument)
+		return BadRequest("missing package path")
 	}
 
 	var params PackageParams
@@ -62,7 +63,7 @@ func ServePackage(w http.ResponseWriter, r *http.Request, ds internal.DataSource
 	bc := internal.BuildContext{GOOS: params.GOOS, GOARCH: params.GOARCH}
 	unit, err := ds.GetUnit(r.Context(), um, fs, bc)
 	if err != nil {
-		return fmt.Errorf("%w: %s", derrors.Unknown, err.Error())
+		return err
 	}
 
 	resp, err := unitToPackage(unit, params)
@@ -79,7 +80,7 @@ func ServeModule(w http.ResponseWriter, r *http.Request, ds internal.DataSource)
 
 	modulePath := trimPath(r, "/v1/module/")
 	if modulePath == "" {
-		return fmt.Errorf("%w: missing module path", derrors.InvalidArgument)
+		return BadRequest("missing module path")
 	}
 
 	var params ModuleParams
@@ -154,7 +155,7 @@ func ServeModuleVersions(w http.ResponseWriter, r *http.Request, ds internal.Dat
 
 	path := trimPath(r, "/v1/versions/")
 	if path == "" {
-		return fmt.Errorf("%w: missing path", derrors.InvalidArgument)
+		return BadRequest("missing path")
 	}
 
 	var params VersionsParams
@@ -166,6 +167,10 @@ func ServeModuleVersions(w http.ResponseWriter, r *http.Request, ds internal.Dat
 	if err != nil {
 		return err
 	}
+	// If there are no versions for the path, then the module doesn't exist.
+	if len(infos) == 0 {
+		return fmt.Errorf("module %q: %w", path, derrors.NotFound)
+	}
 
 	if params.Filter != "" {
 		infos = filter(infos, func(info *internal.ModuleInfo) bool {
@@ -188,7 +193,7 @@ func ServeModulePackages(w http.ResponseWriter, r *http.Request, ds internal.Dat
 
 	modulePath := trimPath(r, "/v1/packages/")
 	if modulePath == "" {
-		return fmt.Errorf("%w: missing module path", derrors.InvalidArgument)
+		return BadRequest("missing module path")
 	}
 
 	var params PackagesParams
@@ -236,11 +241,11 @@ func ServeSearch(w http.ResponseWriter, r *http.Request, ds internal.DataSource)
 
 	var params SearchParams
 	if err := ParseParams(r.URL.Query(), &params); err != nil {
-		return fmt.Errorf("%w: %s", derrors.InvalidArgument, err.Error())
+		return err
 	}
 
 	if params.Query == "" {
-		return fmt.Errorf("%w: missing query", derrors.InvalidArgument)
+		return BadRequest("missing query")
 	}
 
 	dbresults, err := ds.Search(r.Context(), params.Query, internal.SearchOptions{
@@ -285,7 +290,7 @@ func ServePackageSymbols(w http.ResponseWriter, r *http.Request, ds internal.Dat
 
 	pkgPath := trimPath(r, "/v1/symbols/")
 	if pkgPath == "" {
-		return fmt.Errorf("%w: missing package path", derrors.InvalidArgument)
+		return BadRequest("missing package path")
 	}
 
 	var params SymbolsParams
@@ -335,7 +340,7 @@ func ServePackageImportedBy(w http.ResponseWriter, r *http.Request, ds internal.
 
 	pkgPath := trimPath(r, "/v1/imported-by/")
 	if pkgPath == "" {
-		return fmt.Errorf("%w: missing package path", derrors.InvalidArgument)
+		return BadRequest("missing package path")
 	}
 
 	var params ImportedByParams
@@ -396,7 +401,7 @@ func ServeVulnerabilities(vc *vuln.Client) func(w http.ResponseWriter, r *http.R
 
 		modulePath := trimPath(r, "/v1/vulns/")
 		if modulePath == "" {
-			return fmt.Errorf("%w: missing module path", derrors.InvalidArgument)
+			return BadRequest("missing module path")
 		}
 
 		var params VulnParams
@@ -405,7 +410,7 @@ func ServeVulnerabilities(vc *vuln.Client) func(w http.ResponseWriter, r *http.R
 		}
 
 		if vc == nil {
-			return fmt.Errorf("%w: vulnerability data not available", derrors.Unsupported)
+			return InternalServerError("vulnerability client is nil")
 		}
 
 		requestedVersion := params.Version
@@ -525,15 +530,17 @@ func serveJSON(w http.ResponseWriter, status int, data any, cacheDur time.Durati
 	return err
 }
 
-func ServeError(w http.ResponseWriter, err error) error {
+func ServeError(w http.ResponseWriter, r *http.Request, err error) error {
 	var aerr *Error
 	if !errors.As(err, &aerr) {
 		status := derrors.ToStatus(err)
 		aerr = &Error{
 			Code:    status,
-			Message: err.Error(),
+			Message: strings.ToLower(http.StatusText(status)),
+			err:     err,
 		}
 	}
+	log.Errorf(r.Context(), "API error %d: %v", aerr.Code, aerr)
 	return serveJSON(w, aerr.Code, aerr, noCache)
 }
 
@@ -575,7 +582,7 @@ func paginate[T any](all []T, lp ListParams, defaultLimit int) (PaginatedRespons
 // unitToPackage processes unit documentation into a Package struct.
 func unitToPackage(unit *internal.Unit, params PackageParams) (*Package, error) {
 	if params.Examples && params.Doc == "" {
-		return nil, fmt.Errorf("%w: examples require doc format to be specified", derrors.InvalidArgument)
+		return nil, BadRequest("examples require doc format to be specified")
 	}
 
 	// Although unit.Documentation is a slice, it will
@@ -638,7 +645,7 @@ func renderDocumentation(unit *internal.Unit, d *internal.Documentation, format
 	// result.
 	gpkg, err := godoc.DecodePackage(d.Source)
 	if err != nil {
-		return "", fmt.Errorf("%w: %s", derrors.Unknown, err.Error())
+		return "", fmt.Errorf("renderDocumentation: %w", err)
 	}
 	innerPath := internal.Suffix(unit.Path, unit.ModulePath)
 	modInfo := &godoc.ModuleInfo{ModulePath: unit.ModulePath, ResolvedVersion: unit.Version}
@@ -656,10 +663,10 @@ func renderDocumentation(unit *internal.Unit, d *internal.Documentation, format
 	case "html":
 		r = newHTMLRenderer(gpkg.Fset, &sb)
 	default:
-		return "", fmt.Errorf("%w: bad doc format: need one of 'text', 'md', 'markdown' or 'html'", derrors.InvalidArgument)
+		return "", BadRequest("bad doc format: need one of 'text', 'md', 'markdown' or 'html'")
 	}
 	if err := renderDoc(dpkg, r, examples); err != nil {
-		return "", fmt.Errorf("%w: %s", derrors.Unknown, err.Error())
+		return "", fmt.Errorf("renderDoc: %w", err)
 	}
 	return sb.String(), nil
 }
diff --git a/internal/api/api_test.go b/internal/api/api_test.go
index 0a66895c..285005de 100644
--- a/internal/api/api_test.go
+++ b/internal/api/api_test.go
@@ -15,6 +15,7 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"golang.org/x/pkgsite/internal"
 	"golang.org/x/pkgsite/internal/osv"
 	"golang.org/x/pkgsite/internal/testing/fakedatasource"
@@ -245,10 +246,16 @@ func TestServePackage(t *testing.T) {
 			wantStatus: http.StatusBadRequest,
 			want: &Error{
 				Code:    http.StatusBadRequest,
-				Message: "ServePackage: invalid argument: examples require doc format to be specified",
+				Message: "examples require doc format to be specified",
 			},
 		},
 		{
+			name:       "package not found",
+			url:        "/v1/package/nonexistent.com/pkg",
+			wantStatus: http.StatusNotFound,
+			want:       &Error{Code: 404, Message: "not found"},
+		},
+		{
 			name:       "doc without examples",
 			url:        "/v1/package/example.com/ex/pkg?version=v1.0.0&doc=text&examples=false",
 			wantStatus: http.StatusOK,
@@ -269,7 +276,7 @@ func TestServePackage(t *testing.T) {
 			wantStatus: http.StatusBadRequest,
 			want: &Error{
 				Code:    http.StatusBadRequest,
-				Message: "ServePackage: invalid argument: bad doc format: need one of 'text', 'md', 'markdown' or 'html'",
+				Message: "bad doc format: need one of 'text', 'md', 'markdown' or 'html'",
 			},
 		},
 		{
@@ -328,7 +335,7 @@ func TestServePackage(t *testing.T) {
 			w := httptest.NewRecorder()
 
 			if err := ServePackage(w, r, ds); err != nil {
-				ServeError(w, err)
+				ServeError(w, r, err)
 			}
 
 			if w.Code != test.wantStatus {
@@ -340,7 +347,7 @@ func TestServePackage(t *testing.T) {
 				if err != nil {
 					t.Fatal(err)
 				}
-				if diff := cmp.Diff(test.want, got); diff != "" {
+				if diff := cmp.Diff(test.want, got, cmpopts.IgnoreUnexported(Error{})); diff != "" {
 					t.Errorf("mismatch (-want +got):\n%s", diff)
 				}
 			}
@@ -423,7 +430,19 @@ func TestServeModule(t *testing.T) {
 			name:       "bad version",
 			url:        "/v1/module/example.com?version=nope",
 			wantStatus: http.StatusNotFound,
-			want:       &Error{Code: 404, Message: "ServeModule: could not find module for import path example.com: not found"},
+			want:       &Error{Code: 404, Message: "not found"},
+		},
+		{
+			name:       "module not found",
+			url:        "/v1/module/nonexistent.com",
+			wantStatus: http.StatusNotFound,
+			want:       &Error{Code: 404, Message: "not found"},
+		},
+		{
+			name:       "missing module path",
+			url:        "/v1/module/",
+			wantStatus: http.StatusBadRequest,
+			want:       &Error{Code: 400, Message: "missing module path"},
 		},
 		{
 			name:       "module with readme",
@@ -466,7 +485,7 @@ func TestServeModule(t *testing.T) {
 			w := httptest.NewRecorder()
 
 			if err := ServeModule(w, r, ds); err != nil {
-				ServeError(w, err)
+				ServeError(w, r, err)
 			}
 
 			if w.Code != test.wantStatus {
@@ -478,7 +497,7 @@ func TestServeModule(t *testing.T) {
 				if err != nil {
 					t.Fatalf("unmarshaling: %v", err)
 				}
-				if diff := cmp.Diff(test.want, got); diff != "" {
+				if diff := cmp.Diff(test.want, got, cmpopts.IgnoreUnexported(Error{})); diff != "" {
 					t.Errorf("mismatch (-want +got):\n%s", diff)
 				}
 			}
@@ -517,6 +536,18 @@ func TestServeModuleVersions(t *testing.T) {
 			wantCount:  3,
 		},
 		{
+			name:       "module not found",
+			url:        "/v1/versions/nonexistent.com",
+			wantStatus: http.StatusNotFound,
+			want:       &Error{Code: 404, Message: "not found"},
+		},
+		{
+			name:       "missing module path",
+			url:        "/v1/versions/",
+			wantStatus: http.StatusBadRequest,
+			want:       &Error{Code: 400, Message: "missing module path"},
+		},
+		{
 			name:       "with limit",
 			url:        "/v1/versions/example.com?limit=1",
 			wantStatus: http.StatusOK,
@@ -534,7 +565,7 @@ func TestServeModuleVersions(t *testing.T) {
 			w := httptest.NewRecorder()
 
 			if err := ServeModuleVersions(w, r, ds); err != nil {
-				ServeError(w, err)
+				ServeError(w, r, err)
 			}
 
 			if w.Code != test.wantStatus {
@@ -597,6 +628,18 @@ func TestServeModulePackages(t *testing.T) {
 			wantTotal:  2,
 		},
 		{
+			name:       "module not found",
+			url:        "/v1/packages/nonexistent.com?version=v1.0.0",
+			wantStatus: http.StatusNotFound,
+			want:       &Error{Code: 404, Message: "not found"},
+		},
+		{
+			name:       "missing module path",
+			url:        "/v1/packages/",
+			wantStatus: http.StatusBadRequest,
+			want:       &Error{Code: 400, Message: "missing module path"},
+		},
+		{
 			name:       "filtering",
 			url:        "/v1/packages/example.com?version=v1.0.0&filter=sub",
 			wantStatus: http.StatusOK,
@@ -631,7 +674,7 @@ func TestServeModulePackages(t *testing.T) {
 			w := httptest.NewRecorder()
 
 			if err := ServeModulePackages(w, r, ds); err != nil {
-				ServeError(w, err)
+				ServeError(w, r, err)
 			}
 
 			if w.Code != test.wantStatus {
@@ -715,7 +758,7 @@ func TestServeSearch(t *testing.T) {
 
 			err := ServeSearch(w, r, ds)
 			if err != nil {
-				ServeError(w, err)
+				ServeError(w, r, err)
 			}
 
 			if w.Code != test.wantStatus {
@@ -788,7 +831,7 @@ func TestServeSearchPagination(t *testing.T) {
 			w := httptest.NewRecorder()
 
 			if err := ServeSearch(w, r, ds); err != nil {
-				ServeError(w, err)
+				ServeError(w, r, err)
 			}
 
 			if w.Code != http.StatusOK {
@@ -873,6 +916,18 @@ func TestServePackageSymbols(t *testing.T) {
 			wantName:   "LinuxSym",
 		},
 		{
+			name:       "package not found",
+			url:        "/v1/symbols/nonexistent.com/pkg?version=v1.0.0",
+			wantStatus: http.StatusNotFound,
+			want:       &Error{Code: 404, Message: "not found"},
+		},
+		{
+			name:       "missing package path",
+			url:        "/v1/symbols/",
+			wantStatus: http.StatusBadRequest,
+			want:       &Error{Code: 400, Message: "missing package path"},
+		},
+		{
 			name:       "explicit linux",
 			url:        "/v1/symbols/example.com/pkg?version=v1.0.0&goos=linux&goarch=amd64",
 			wantStatus: http.StatusOK,
@@ -906,7 +961,7 @@ func TestServePackageSymbols(t *testing.T) {
 			wantStatus: http.StatusNotFound,
 			want: &Error{
 				Code:    http.StatusNotFound,
-				Message: "ServePackageSymbols: not found",
+				Message: "not found",
 			},
 		},
 	} {
@@ -915,7 +970,7 @@ func TestServePackageSymbols(t *testing.T) {
 			w := httptest.NewRecorder()
 
 			if err := ServePackageSymbols(w, r, ds); err != nil {
-				ServeError(w, err)
+				ServeError(w, r, err)
 			}
 
 			if w.Code != test.wantStatus {
@@ -928,7 +983,7 @@ func TestServePackageSymbols(t *testing.T) {
 					if err := json.Unmarshal(w.Body.Bytes(), &got); err != nil {
 						t.Fatalf("json.Unmarshal: %v", err)
 					}
-					if diff := cmp.Diff(want, &got); diff != "" {
+					if diff := cmp.Diff(want, &got, cmpopts.IgnoreUnexported(Error{})); diff != "" {
 						t.Errorf("mismatch (-want +got):\n%s", diff)
 					}
 				}
@@ -990,7 +1045,7 @@ func TestServePackageImportedBy(t *testing.T) {
 			w := httptest.NewRecorder()
 
 			if err := ServePackageImportedBy(w, r, ds); err != nil {
-				ServeError(w, err)
+				ServeError(w, r, err)
 			}
 
 			if w.Code != test.wantStatus {
@@ -1053,7 +1108,7 @@ func TestServeVulnerabilities(t *testing.T) {
 			w := httptest.NewRecorder()
 
 			if err := ServeVulnerabilities(vc)(w, r, ds); err != nil {
-				ServeError(w, err)
+				ServeError(w, r, err)
 			}
 
 			if w.Code != test.wantStatus {
diff --git a/internal/api/params.go b/internal/api/params.go
index 6858de5e..d0403fe2 100644
--- a/internal/api/params.go
+++ b/internal/api/params.go
@@ -9,8 +9,6 @@ import (
 	"net/url"
 	"reflect"
 	"strconv"
-
-	"golang.org/x/pkgsite/internal/derrors"
 )
 
 // ListParams are common pagination and filtering parameters.
@@ -85,10 +83,10 @@ type VulnParams struct {
 func ParseParams(v url.Values, dst any) error {
 	val := reflect.ValueOf(dst)
 	if val.Kind() != reflect.Pointer || val.Elem().Kind() != reflect.Struct {
-		return fmt.Errorf("%w: dst must be a pointer to a struct", derrors.InvalidArgument)
+		return InternalServerError("dst must be a pointer to a struct")
 	}
 	if err := parseValue(v, val.Elem()); err != nil {
-		return fmt.Errorf("%w: %v", derrors.InvalidArgument, err)
+		return BadRequest("%v", err)
 	}
 	return nil
 }
diff --git a/internal/api/types.go b/internal/api/types.go
index be923e4c..a29d7572 100644
--- a/internal/api/types.go
+++ b/internal/api/types.go
@@ -4,6 +4,12 @@
 
 package api
 
+import (
+	"fmt"
+	"net/http"
+	"strings"
+)
+
 // Package is the response for /v1/package/{packagePath}.
 type Package struct {
 	Path              string    `json:"path"`
@@ -91,12 +97,38 @@ type Error struct {
 	Code       int         `json:"code"`
 	Message    string      `json:"message"`
 	Candidates []Candidate `json:"candidates,omitempty"`
+
+	err error // Unexported field for internal tracking
 }
 
 func (e *Error) Error() string {
+	if e.err != nil {
+		return e.err.Error()
+	}
 	return e.Message
 }
 
+func (e *Error) Unwrap() error {
+	return e.err
+}
+
+// BadRequest returns an Error with StatusBadRequest.
+func BadRequest(format string, args ...any) *Error {
+	return &Error{
+		Code:    http.StatusBadRequest,
+		Message: fmt.Sprintf(format, args...),
+	}
+}
+
+// InternalServerError returns an Error with StatusInternalServerError.
+func InternalServerError(format string, args ...any) *Error {
+	return &Error{
+		Code:    http.StatusInternalServerError,
+		Message: strings.ToLower(http.StatusText(http.StatusInternalServerError)),
+		err:     fmt.Errorf(format, args...),
+	}
+}
+
 // Candidate is a potential resolution for an ambiguous path.
 type Candidate struct {
 	ModulePath  string `json:"modulePath"`
diff --git a/internal/frontend/server.go b/internal/frontend/server.go
index 87f3dc50..374e59fd 100644
--- a/internal/frontend/server.go
+++ b/internal/frontend/server.go
@@ -547,7 +547,7 @@ func (s *Server) apiHandler(f func(w http.ResponseWriter, r *http.Request, ds in
 	return func(w http.ResponseWriter, r *http.Request) {
 		ds := s.getDataSource(r.Context())
 		if err := f(w, r, ds); err != nil {
-			api.ServeError(w, err)
+			api.ServeError(w, r, err)
 		}
 	}
 }