Invalid Argument errors provide per field details. (#110)

* Invalid Argument errors now return a per field error message.

* Creating an API error struct.

* SDK uses field error details proto.

* Adding tests specifically for handler helper error messages resulting in expected API output.

* Inlining the error checks into the table driven tests.  Use protocmp for assertions to make printing out diffs easier to parse.

* Aggregate the invalid argument fields detected to be wrong.

* Make the returned order of fields deterministic in the error details..

* Fixing formatting issue in errors_test.go.

* Test to confirm unimplemented results in a 405 and not 404.

internal/servers/controller/handlers/errors.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"sort"
 
 	"github.com/grpc-ecosystem/grpc-gateway/runtime"
 	"github.com/hashicorp/go-hclog"
@@ -14,16 +15,20 @@ import (
 	"google.golang.org/grpc/status"
 )
 
+// NotFoundError returns an ApiError indicating a resource couldn't be found.
 func NotFoundErrorf(msg string, a ...interface{}) error {
 	return status.Errorf(codes.NotFound, msg, a...)
 }
 
-func InvalidArgumentErrorf(msg string, fields []string) error {
+func InvalidArgumentErrorf(msg string, fields map[string]string) error {
 	st := status.New(codes.InvalidArgument, msg)
 	br := &errdetails.BadRequest{}
-	for _, f := range fields {
-		br.FieldViolations = append(br.FieldViolations, &errdetails.BadRequest_FieldViolation{Field: f})
+	for k, v := range fields {
+		br.FieldViolations = append(br.FieldViolations, &errdetails.BadRequest_FieldViolation{Field: k, Description: v})
 	}
+	sort.Slice(br.FieldViolations, func(i, j int) bool {
+		return br.FieldViolations[i].GetField() < br.FieldViolations[j].GetField()
+	})
 	st, err := st.WithDetails(br)
 	if err != nil {
 		hclog.Default().Error("failure building status with details", "details", br, "error", err)
@@ -51,7 +56,7 @@ func statusErrorToApiError(s *status.Status) *pb.Error {
 				if apiErr.Details == nil {
 					apiErr.Details = &pb.ErrorDetails{}
 				}
-				apiErr.Details.RequestFields = append(apiErr.Details.RequestFields, fv.GetField())
+				apiErr.Details.RequestFields = append(apiErr.Details.RequestFields, &pb.FieldError{Name: fv.GetField(), Description: fv.GetDescription()})
 			}
 		}
 	}

internal/servers/controller/handlers/errors_test.go

@@ -2,100 +2,113 @@ package handlers
 
 import (
 	"context"
+	"errors"
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"testing"
 
-	"github.com/golang/protobuf/proto"
+	"github.com/google/go-cmp/cmp"
 	"github.com/grpc-ecosystem/grpc-gateway/runtime"
 	"github.com/hashicorp/go-hclog"
 	pb "github.com/hashicorp/watchtower/internal/gen/controller/api"
 	"github.com/stretchr/testify/assert"
-	sdpb "google.golang.org/genproto/googleapis/rpc/errdetails"
+	"github.com/stretchr/testify/require"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
+	"google.golang.org/protobuf/testing/protocmp"
 )
 
 func TestApiErrorHandler(t *testing.T) {
+	assert, require := assert.New(t), require.New(t)
 	ctx := context.Background()
 	req, err := http.NewRequest("GET", "madeup/for/the/test", nil)
-	if err != nil {
-		t.Fatalf("Couldn't create test request")
-	}
+	require.NoError(err)
 	mux := runtime.NewServeMux()
-	_, outMarsh := runtime.MarshalerForRequest(mux, req)
+	inMarsh, outMarsh := runtime.MarshalerForRequest(mux, req)
 
 	tested := ErrorHandler(hclog.L())
 
 	testCases := []struct {
-		name          string
-		err           error
-		statusDetails []proto.Message
-		expected      *pb.Error
+		name     string
+		err      error
+		expected *pb.Error
 	}{
 		{
 			name: "Not Found",
-			err:  status.Error(codes.NotFound, "test"),
+			err:  NotFoundErrorf("Test"),
 			expected: &pb.Error{
-				Status:  404,
-				Code:    codes.NotFound.String(),
-				Message: "test",
+				Status:  http.StatusNotFound,
+				Code:    "NotFound",
+				Message: "Test",
+			},
+		},
+		{
+			name: "Invalid Fields",
+			err: InvalidArgumentErrorf("Test", map[string]string{
+				"k1": "v1",
+				"k2": "v2",
+			}),
+			expected: &pb.Error{
+				Status:  http.StatusBadRequest,
+				Code:    "InvalidArgument",
+				Message: "Test",
+				Details: &pb.ErrorDetails{
+					RequestFields: []*pb.FieldError{
+						{
+							Name:        "k1",
+							Description: "v1",
+						},
+						{
+							Name:        "k2",
+							Description: "v2",
+						},
+					},
+				},
 			},
 		},
 		{
 			name: "GrpcGateway Routing Error",
 			err:  runtime.ErrUnknownURI,
 			expected: &pb.Error{
-				Status:  404,
-				Code:    codes.NotFound.String(),
+				Status:  http.StatusNotFound,
+				Code:    "NotFound",
 				Message: http.StatusText(http.StatusNotFound),
 			},
 		},
 		{
-			name: "Invalid Fields",
-			err:  status.Error(codes.InvalidArgument, "test"),
-			statusDetails: []proto.Message{
-				&sdpb.BadRequest{
-					FieldViolations: []*sdpb.BadRequest_FieldViolation{
-						{Field: "first"},
-						{Field: "second"},
-					},
-				},
-			},
+			name: "Unimplemented error",
+			err:  status.Error(codes.Unimplemented, "Test"),
 			expected: &pb.Error{
-				Status:  400,
-				Code:    codes.InvalidArgument.String(),
-				Message: "test",
-				Details: &pb.ErrorDetails{
-					RequestFields: []string{"first", "second"},
-				},
+				Status:  http.StatusMethodNotAllowed,
+				Code:    "Unimplemented",
+				Message: "Test",
+			},
+		},
+		{
+			name: "Unknown error",
+			err:  errors.New("Some random error"),
+			expected: &pb.Error{
+				Status:  http.StatusInternalServerError,
+				Code:    "Unknown",
+				Message: "Some random error",
 			},
 		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			assert := assert.New(t)
-
-			if tc.statusDetails != nil {
-				s, ok := status.FromError(tc.err)
-				assert.True(ok)
-				s, err := s.WithDetails(tc.statusDetails...)
-				assert.NoError(err)
-				tc.err = s.Err()
-			}
-
 			w := httptest.NewRecorder()
 			tested(ctx, mux, outMarsh, w, req, tc.err)
 			resp := w.Result()
 			assert.EqualValues(tc.expected.Status, resp.StatusCode)
 
 			got, err := ioutil.ReadAll(resp.Body)
-			assert.NoError(err)
-			want, err := outMarsh.Marshal(tc.expected)
-			t.Logf("Got marshaled error: %q", want)
-			assert.NoError(err)
-			assert.JSONEq(string(want), string(got))
+			require.NoError(err)
+
+			gotErr := &pb.Error{}
+			err = inMarsh.Unmarshal(got, gotErr)
+			require.NoError(err)
+			assert.Empty(cmp.Diff(tc.expected, gotErr, protocmp.Transform()))
 		})
 	}
 }

internal/servers/controller/handlers/host_catalogs/host_catalog_service.go

@@ -92,7 +92,7 @@ func (s Service) ListHostCatalogs(ctx context.Context, req *pbs.ListHostCatalogs
 func (s Service) GetHostCatalog(ctx context.Context, req *pbs.GetHostCatalogRequest) (*pbs.GetHostCatalogResponse, error) {
 	ct := typeFromId(req.GetId())
 	if ct == unknownType {
-		return nil, handlers.InvalidArgumentErrorf("Unknown host catalog type.", []string{"id"})
+		return nil, handlers.InvalidArgumentErrorf("Invalid argument provided.", map[string]string{"id": "Improperly formatted identifier used."})
 	}
 	if err := validateGetRequest(req, ct); err != nil {
 		return nil, err
@@ -123,7 +123,7 @@ func (s Service) CreateHostCatalog(ctx context.Context, req *pbs.CreateHostCatal
 func (s Service) UpdateHostCatalog(ctx context.Context, req *pbs.UpdateHostCatalogRequest) (*pbs.UpdateHostCatalogResponse, error) {
 	ct := typeFromId(req.GetId())
 	if ct == unknownType {
-		return nil, handlers.InvalidArgumentErrorf("Unknown host catalog type.", []string{"id"})
+		return nil, handlers.InvalidArgumentErrorf("Invalid argument provided.", map[string]string{"id": "Improperly formatted identifier used."})
 	}
 	if err := validateUpdateRequest(req, ct); err != nil {
 		return nil, err
@@ -139,7 +139,7 @@ func (s Service) UpdateHostCatalog(ctx context.Context, req *pbs.UpdateHostCatal
 func (s Service) DeleteHostCatalog(ctx context.Context, req *pbs.DeleteHostCatalogRequest) (*pbs.DeleteHostCatalogResponse, error) {
 	ct := typeFromId(req.GetId())
 	if ct == unknownType {
-		return nil, handlers.InvalidArgumentErrorf("Unknown host catalog type.", []string{"id"})
+		return nil, handlers.InvalidArgumentErrorf("Invalid argument provided.", map[string]string{"id": "Improperly formatted identifier used."})
 	}
 	if err := validateDeleteRequest(req, ct); err != nil {
 		return nil, err
@@ -249,10 +249,10 @@ func toDbUpdateMask(paths []string) ([]string, error) {
 		}
 	}
 	if len(invalid) > 0 {
-		return nil, handlers.InvalidArgumentErrorf(fmt.Sprintf("Invalid fields passed in update_update mask: %v.", invalid), []string{"update_mask"})
+		return nil, handlers.InvalidArgumentErrorf("Invalid argument provided.", map[string]string{"update_mask": fmt.Sprintf("Invalid paths provided: %q", invalid)})
 	}
 	if len(dbPaths) == 0 {
-		return nil, handlers.InvalidArgumentErrorf("No valid paths included in the update mask.", []string{"update_mask"})
+		return nil, handlers.InvalidArgumentErrorf("Invalid argument provided.", map[string]string{"update_mask": "No valid paths provided."})
 	}
 	return dbPaths, nil
 }
@@ -281,55 +281,51 @@ func toProto(in *static.HostCatalog) *pb.HostCatalog {
 //  * The type asserted by the ID and/or field is known
 //  * If relevant, the type derived from the id prefix matches what is claimed by the type field
 func validateGetRequest(req *pbs.GetHostCatalogRequest, ct catalogType) error {
-	if err := validateAncestors(req); err != nil {
-		return err
-	}
+	badFields := validateAncestors(req)
 	if !validId(req.GetId(), ct.idPrefix()) {
-		return handlers.InvalidArgumentErrorf("Improperly formatted identifier.", []string{"id"})
+		badFields["id"] = "Invalid formatted identifier."
+	}
+	if len(badFields) > 0 {
+		return handlers.InvalidArgumentErrorf("Invalid arguments provided.", badFields)
 	}
 	return nil
 }
 
 func validateCreateRequest(req *pbs.CreateHostCatalogRequest) error {
-	if err := validateAncestors(req); err != nil {
-		return err
-	}
+	badFields := validateAncestors(req)
 	item := req.GetItem()
 	if item == nil {
-		return handlers.InvalidArgumentErrorf("The catalog's fields must be set to something.", []string{"item"})
+		badFields["item"] = "This field is required."
 	}
 	if item.GetType() == nil {
-		return handlers.InvalidArgumentErrorf("Type must be specified when creating a host catalog.", []string{"type"})
+		badFields["type"] = "This field is required."
 	}
 	if typeFromTypeField(item.GetType().GetValue()) == unknownType {
-		return handlers.InvalidArgumentErrorf("Provided host catalog type is unknown.", []string{"type"})
+		badFields["type"] = "Provided type is unknown."
 	}
-	var immutableFieldsSet []string
 	if item.GetId() != "" {
-		immutableFieldsSet = append(immutableFieldsSet, "id")
+		badFields["id"] = "This field is read only."
 	}
 	if item.GetCreatedTime() != nil {
-		immutableFieldsSet = append(immutableFieldsSet, "created_time")
+		badFields["created_time"] = "This field is read only."
 	}
 	if item.GetUpdatedTime() != nil {
-		immutableFieldsSet = append(immutableFieldsSet, "updated_time")
+		badFields["updated_time"] = "This field is read only."
 	}
-	if len(immutableFieldsSet) > 0 {
-		return handlers.InvalidArgumentErrorf("Cannot specify read only fields at creation time.", immutableFieldsSet)
+	if len(badFields) > 0 {
+		return handlers.InvalidArgumentErrorf("Invalid arguments provided.", badFields)
 	}
 	return nil
 }
 
 func validateUpdateRequest(req *pbs.UpdateHostCatalogRequest, ct catalogType) error {
-	if err := validateAncestors(req); err != nil {
-		return err
-	}
+	badFields := validateAncestors(req)
 	if !validId(req.GetId(), ct.idPrefix()) {
-		return handlers.InvalidArgumentErrorf("Improperly formatted identifier.", []string{"id"})
+		badFields["id"] = "The field is incorrectly formatted."
 	}
 
 	if req.GetUpdateMask() == nil {
-		return handlers.InvalidArgumentErrorf("UpdateMask not provided but is required to update a host.", []string{"update_mask"})
+		badFields["update_mask"] = "This field is required."
 	}
 
 	item := req.GetItem()
@@ -338,35 +334,32 @@ func validateUpdateRequest(req *pbs.UpdateHostCatalogRequest, ct catalogType) er
 		// the mask will be marked as unset.
 		return nil
 	}
-	if item.GetId() != "" && item.GetId() != req.GetId() {
-		return handlers.InvalidArgumentErrorf("Id in provided item and url do not match.", []string{"id"})
-	}
-	var immutableFieldsSet []string
 	if item.GetType() != nil {
-		immutableFieldsSet = append(immutableFieldsSet, "type")
+		badFields["type"] = "This is a read only field and cannot be specified in an update request."
 	}
 	if item.GetId() != "" {
-		immutableFieldsSet = append(immutableFieldsSet, "id")
+		badFields["id"] = "This is a read only field and cannot be specified in an update request."
 	}
 	if item.GetCreatedTime() != nil {
-		immutableFieldsSet = append(immutableFieldsSet, "created_time")
+		badFields["created_time"] = "This is a read only field and cannot be specified in an update request."
 	}
 	if item.GetUpdatedTime() != nil {
-		immutableFieldsSet = append(immutableFieldsSet, "updated_time")
+		badFields["updated_time"] = "This is a read only field and cannot be specified in an update request."
 	}
-	if len(immutableFieldsSet) > 0 {
-		return handlers.InvalidArgumentErrorf("Cannot specify read only fields at update time.", immutableFieldsSet)
+	if len(badFields) > 0 {
+		return handlers.InvalidArgumentErrorf("Errors in provided fields.", badFields)
 	}
 
 	return nil
 }
 
 func validateDeleteRequest(req *pbs.DeleteHostCatalogRequest, ct catalogType) error {
-	if err := validateAncestors(req); err != nil {
-		return err
-	}
+	badFields := validateAncestors(req)
 	if !validId(req.GetId(), ct.idPrefix()) {
-		return handlers.InvalidArgumentErrorf("Improperly formatted identifier.", []string{"id"})
+		badFields["id"] = "The field is incorrectly formatted."
+	}
+	if len(badFields) > 0 {
+		return handlers.InvalidArgumentErrorf("Invalid arguments provided.", badFields)
 	}
 	return nil
 }
@@ -385,24 +378,23 @@ type ancestorProvider interface {
 }
 
 // validateAncestors verifies that the ancestors of this call are set and formatted correctly.
-func validateAncestors(r ancestorProvider) error {
+func validateAncestors(r ancestorProvider) map[string]string {
+	badFields := make(map[string]string)
 	if r.GetOrgId() == "" {
-		return handlers.InvalidArgumentErrorf("Missing organization id.", []string{orgIdFieldName})
+		badFields[orgIdFieldName] = "The field is missing but required."
 	}
 	if r.GetProjectId() == "" {
-		return handlers.InvalidArgumentErrorf("Missing project id.", []string{projectIdFieldName})
+		badFields[projectIdFieldName] = "The field is missing but required."
+	}
+	if len(badFields) > 0 {
+		return badFields
 	}
 
-	var badFormat []string
 	if !validId(r.GetOrgId(), "o_") {
-		badFormat = append(badFormat, orgIdFieldName)
+		badFields[orgIdFieldName] = "The field is incorrectly formatted."
 	}
 	if !validId(r.GetProjectId(), "p_") {
-		badFormat = append(badFormat, projectIdFieldName)
+		badFields[projectIdFieldName] = "The field is incorrectly formatted."
 	}
-	if len(badFormat) > 0 {
-		return handlers.InvalidArgumentErrorf("Improperly formatted identifier.", badFormat)
-	}
-
-	return nil
+	return badFields
 }

internal/servers/controller/handlers/projects/project_service.go

@@ -152,7 +152,7 @@ func (s Service) updateInRepo(ctx context.Context, orgID, projId string, mask []
 		return nil, err
 	}
 	if len(dbMask) == 0 {
-		return nil, handlers.InvalidArgumentErrorf("No valid fields included in the update mask.", []string{"update_mask"})
+		return nil, handlers.InvalidArgumentErrorf("No valid fields included in the update mask.", map[string]string{"update_mask": "No valid paths provided in the update mask."})
 	}
 	repo, err := s.repo()
 	if err != nil {
@@ -194,7 +194,7 @@ func toDbUpdateMask(paths []string) ([]string, error) {
 		}
 	}
 	if len(invalid) > 0 {
-		return nil, handlers.InvalidArgumentErrorf(fmt.Sprintf("Invalid fields passed in update_update mask: %v.", invalid), []string{"update_mask"})
+		return nil, handlers.InvalidArgumentErrorf(fmt.Sprintf("Invalid fields passed in update_update mask: %v.", invalid), map[string]string{"update_mask": fmt.Sprintf("Unknown paths provided in update mask: %q", strings.Join(invalid, ","))})
 	}
 	return dbPaths, nil
 }
@@ -219,85 +219,76 @@ func toProto(in *iam.Scope) *pb.Project {
 //  * The path passed in is correctly formatted
 //  * All required parameters are set
 //  * There are no conflicting parameters provided
-// TODO: Populate the error in a way to allow it to be converted to the previously described error format and include all invalid fields instead of just the most recent.
 func validateGetRequest(req *pbs.GetProjectRequest) error {
-	if err := validateAncestors(req); err != nil {
-		return err
+	badFields := validateAncestors(req)
+	if !validId(req.GetId(), "p_") {
+		badFields["id"] = "Invalid formatted project id."
 	}
-	if !validId(req.GetId(), iam.ProjectScope.Prefix()+"_") {
-		return handlers.InvalidArgumentErrorf("Improperly formatted identifier.", []string{"id"})
+	if len(badFields) > 0 {
+		return handlers.InvalidArgumentErrorf("Improperly formatted identifier.", badFields)
 	}
 	return nil
 }
 
 func validateCreateRequest(req *pbs.CreateProjectRequest) error {
-	if err := validateAncestors(req); err != nil {
-		return err
-	}
+	badFields := validateAncestors(req)
 	item := req.GetItem()
-	if item == nil {
-		return handlers.InvalidArgumentErrorf("A project's fields must be set to something.", []string{"item"})
-	}
-	var immutableFieldsSet []string
 	if item.GetId() != "" {
-		immutableFieldsSet = append(immutableFieldsSet, "id")
+		badFields["id"] = "This is a read only field."
 	}
 	if item.GetCreatedTime() != nil {
-		immutableFieldsSet = append(immutableFieldsSet, "created_time")
+		badFields["created_time"] = "This is a read only field."
 	}
 	if item.GetUpdatedTime() != nil {
-		immutableFieldsSet = append(immutableFieldsSet, "updated_time")
+		badFields["updated_time"] = "This is a read only field."
 	}
-	if len(immutableFieldsSet) > 0 {
-		return handlers.InvalidArgumentErrorf("Cannot specify read only fields at creation time.", immutableFieldsSet)
+	if len(badFields) > 0 {
+		return handlers.InvalidArgumentErrorf("Argument errors found in the request.", badFields)
 	}
 	return nil
 }
 
 func validateUpdateRequest(req *pbs.UpdateProjectRequest) error {
-	if err := validateAncestors(req); err != nil {
-		return err
+	badFields := validateAncestors(req)
+	if !validId(req.GetId(), "p_") {
+		badFields["project_id"] = "Improperly formatted path identifier."
 	}
-	if !validId(req.GetId(), iam.ProjectScope.Prefix()+"_") {
-		return handlers.InvalidArgumentErrorf("Improperly formatted identifier.", []string{"id"})
-	}
-
 	if req.GetUpdateMask() == nil {
-		return handlers.InvalidArgumentErrorf("UpdateMask not provided but is required to update a project.", []string{"update_mask"})
+		badFields["update_mask"] = "UpdateMask not provided but is required to update a project."
 	}
 
 	item := req.GetItem()
 	if item == nil {
+		if len(badFields) > 0 {
+			return handlers.InvalidArgumentErrorf("Errors in provided fields.", badFields)
+		}
 		// It is legitimate for no item to be specified in an update request as it indicates all fields provided in
 		// the mask will be marked as unset.
 		return nil
 	}
-	if item.GetId() != "" && item.GetId() != req.GetId() {
-		return handlers.InvalidArgumentErrorf("Id in provided item and url do not match.", []string{"id"})
-	}
-	var immutableFieldsSet []string
 	if item.GetId() != "" {
-		immutableFieldsSet = append(immutableFieldsSet, "id")
+		badFields["id"] = "This is a read only field and cannot be specified in an update request."
 	}
 	if item.GetCreatedTime() != nil {
-		immutableFieldsSet = append(immutableFieldsSet, "created_time")
+		badFields["created_time"] = "This is a read only field and cannot be specified in an update request."
 	}
 	if item.GetUpdatedTime() != nil {
-		immutableFieldsSet = append(immutableFieldsSet, "updated_time")
+		badFields["updated_time"] = "This is a read only field and cannot be specified in an update request."
 	}
-	if len(immutableFieldsSet) > 0 {
-		return handlers.InvalidArgumentErrorf("Cannot specify read only fields at update time.", immutableFieldsSet)
+	if len(badFields) > 0 {
+		return handlers.InvalidArgumentErrorf("Errors in provided fields.", badFields)
 	}
 
 	return nil
 }
 
 func validateDeleteRequest(req *pbs.DeleteProjectRequest) error {
-	if err := validateAncestors(req); err != nil {
-		return err
+	badFields := validateAncestors(req)
+	if !validId(req.GetId(), "p_") {
+		badFields["id"] = "Incorrectly formatted project."
 	}
-	if !validId(req.GetId(), iam.ProjectScope.Prefix()+"_") {
-		return handlers.InvalidArgumentErrorf("Improperly formatted id.", []string{"id"})
+	if len(badFields) > 0 {
+		return handlers.InvalidArgumentErrorf("Errors in provided fields.", badFields)
 	}
 	return nil
 }
@@ -315,12 +306,12 @@ type ancestorProvider interface {
 }
 
 // validateAncestors verifies that the ancestors of this call are properly set and provided.
-func validateAncestors(r ancestorProvider) error {
+func validateAncestors(r ancestorProvider) map[string]string {
 	if r.GetOrgId() == "" {
-		return handlers.InvalidArgumentErrorf("Missing organization id.", []string{"org_id"})
+		return map[string]string{"org_id": "Missing organization id."}
 	}
 	if !validId(r.GetOrgId(), iam.OrganizationScope.Prefix()+"_") {
-		return handlers.InvalidArgumentErrorf("Poorly formatted org id.", []string{"org_id"})
+		return map[string]string{"org_id": "Improperly formatted identifier."}
 	}
-	return nil
+	return map[string]string{}
 }