From 2c3fbe302425bbfe7f29f60781c1d40d2bfb1d56 Mon Sep 17 00:00:00 2001 From: Todd Knight Date: Tue, 9 Jun 2020 10:51:50 -0700 Subject: [PATCH] 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. --- api/error_details.gen.go | 6 +- api/field_error.gen.go | 36 +++++ api/internal/genapi/input.go | 11 ++ go.mod | 1 + internal/gen/controller/api/error.pb.go | 150 +++++++++++++----- .../proto/local/controller/api/v1/error.proto | 8 +- .../servers/controller/handlers/errors.go | 13 +- .../controller/handlers/errors_test.go | 107 +++++++------ .../host_catalogs/host_catalog_service.go | 94 +++++------ .../handlers/projects/project_service.go | 77 ++++----- 10 files changed, 317 insertions(+), 186 deletions(-) create mode 100644 api/field_error.gen.go diff --git a/api/error_details.gen.go b/api/error_details.gen.go index c9443a314..a1d43e438 100644 --- a/api/error_details.gen.go +++ b/api/error_details.gen.go @@ -12,9 +12,9 @@ import ( type ErrorDetails struct { defaultFields []string - TraceId *string `json:"TraceId,omitempty"` - RequestId *string `json:"request_id,omitempty"` - RequestFields []string `json:"request_fields,omitempty"` + TraceId *string `json:"TraceId,omitempty"` + RequestId *string `json:"request_id,omitempty"` + RequestFields []*FieldError `json:"request_fields,omitempty"` } func (s *ErrorDetails) SetDefault(key string) { diff --git a/api/field_error.gen.go b/api/field_error.gen.go new file mode 100644 index 000000000..8d42db7c9 --- /dev/null +++ b/api/field_error.gen.go @@ -0,0 +1,36 @@ +// Code generated by "make api"; DO NOT EDIT. +package api + +import ( + "encoding/json" + + "github.com/fatih/structs" + + "github.com/hashicorp/watchtower/api/internal/strutil" +) + +type FieldError struct { + defaultFields []string + + Name *string `json:"name,omitempty"` + Description *string `json:"description,omitempty"` +} + +func (s *FieldError) SetDefault(key string) { + s.defaultFields = strutil.AppendIfMissing(s.defaultFields, key) +} + +func (s *FieldError) UnsetDefault(key string) { + s.defaultFields = strutil.StrListDelete(s.defaultFields, key) +} + +func (s FieldError) MarshalJSON() ([]byte, error) { + m := structs.Map(s) + if m == nil { + m = make(map[string]interface{}) + } + for _, k := range s.defaultFields { + m[k] = nil + } + return json.Marshal(m) +} diff --git a/api/internal/genapi/input.go b/api/internal/genapi/input.go index dd2616852..b228c9fb4 100644 --- a/api/internal/genapi/input.go +++ b/api/internal/genapi/input.go @@ -37,6 +37,17 @@ var inputStructs = []*structInfo{ "", templateTypeResource, }, + { + os.Getenv("GEN_BASEPATH") + "/internal/gen/controller/api/error.pb.go", + "FieldError", + os.Getenv("GEN_BASEPATH") + "/api/field_error.gen.go", + "FieldError", + "api", + "", + "", + "", + templateTypeResource, + }, { os.Getenv("GEN_BASEPATH") + "/internal/gen/controller/api/resources/hosts/host.pb.go", "Host", diff --git a/go.mod b/go.mod index 0ab0c3f09..b48f25639 100644 --- a/go.mod +++ b/go.mod @@ -16,6 +16,7 @@ require ( github.com/golang-migrate/migrate/v4 v4.11.0 github.com/golang-sql/civil v0.0.0-20190719163853-cb61b32ac6fe github.com/golang/protobuf v1.4.2 + github.com/google/go-cmp v0.4.0 github.com/grpc-ecosystem/grpc-gateway v1.14.6 github.com/hashicorp/dbassert v0.0.0-20200602132714-30dfb78f0133 github.com/hashicorp/errwrap v1.0.0 diff --git a/internal/gen/controller/api/error.pb.go b/internal/gen/controller/api/error.pb.go index a117b9a63..fd3c66c73 100644 --- a/internal/gen/controller/api/error.pb.go +++ b/internal/gen/controller/api/error.pb.go @@ -30,9 +30,9 @@ type ErrorDetails struct { sizeCache protoimpl.SizeCache unknownFields protoimpl.UnknownFields - TraceId string `protobuf:"bytes,1,opt,name=TraceId,proto3" json:"TraceId,omitempty"` - RequestId string `protobuf:"bytes,2,opt,name=request_id,proto3" json:"request_id,omitempty"` - RequestFields []string `protobuf:"bytes,3,rep,name=request_fields,proto3" json:"request_fields,omitempty"` + TraceId string `protobuf:"bytes,1,opt,name=TraceId,proto3" json:"TraceId,omitempty"` + RequestId string `protobuf:"bytes,2,opt,name=request_id,proto3" json:"request_id,omitempty"` + RequestFields []*FieldError `protobuf:"bytes,3,rep,name=request_fields,proto3" json:"request_fields,omitempty"` } func (x *ErrorDetails) Reset() { @@ -81,13 +81,69 @@ func (x *ErrorDetails) GetRequestId() string { return "" } -func (x *ErrorDetails) GetRequestFields() []string { +func (x *ErrorDetails) GetRequestFields() []*FieldError { if x != nil { return x.RequestFields } return nil } +// FieldErrors contains error information on a per field basis. +type FieldError struct { + state protoimpl.MessageState + sizeCache protoimpl.SizeCache + unknownFields protoimpl.UnknownFields + + Name string `protobuf:"bytes,1,opt,name=name,proto3" json:"name,omitempty"` + Description string `protobuf:"bytes,2,opt,name=description,proto3" json:"description,omitempty"` +} + +func (x *FieldError) Reset() { + *x = FieldError{} + if protoimpl.UnsafeEnabled { + mi := &file_controller_api_v1_error_proto_msgTypes[1] + ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) + ms.StoreMessageInfo(mi) + } +} + +func (x *FieldError) String() string { + return protoimpl.X.MessageStringOf(x) +} + +func (*FieldError) ProtoMessage() {} + +func (x *FieldError) ProtoReflect() protoreflect.Message { + mi := &file_controller_api_v1_error_proto_msgTypes[1] + if protoimpl.UnsafeEnabled && x != nil { + ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) + if ms.LoadMessageInfo() == nil { + ms.StoreMessageInfo(mi) + } + return ms + } + return mi.MessageOf(x) +} + +// Deprecated: Use FieldError.ProtoReflect.Descriptor instead. +func (*FieldError) Descriptor() ([]byte, []int) { + return file_controller_api_v1_error_proto_rawDescGZIP(), []int{1} +} + +func (x *FieldError) GetName() string { + if x != nil { + return x.Name + } + return "" +} + +func (x *FieldError) GetDescription() string { + if x != nil { + return x.Description + } + return "" +} + // Error is returned by the JSON API when an error occurs. type Error struct { state protoimpl.MessageState @@ -108,7 +164,7 @@ type Error struct { func (x *Error) Reset() { *x = Error{} if protoimpl.UnsafeEnabled { - mi := &file_controller_api_v1_error_proto_msgTypes[1] + mi := &file_controller_api_v1_error_proto_msgTypes[2] ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) ms.StoreMessageInfo(mi) } @@ -121,7 +177,7 @@ func (x *Error) String() string { func (*Error) ProtoMessage() {} func (x *Error) ProtoReflect() protoreflect.Message { - mi := &file_controller_api_v1_error_proto_msgTypes[1] + mi := &file_controller_api_v1_error_proto_msgTypes[2] if protoimpl.UnsafeEnabled && x != nil { ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) if ms.LoadMessageInfo() == nil { @@ -134,7 +190,7 @@ func (x *Error) ProtoReflect() protoreflect.Message { // Deprecated: Use Error.ProtoReflect.Descriptor instead. func (*Error) Descriptor() ([]byte, []int) { - return file_controller_api_v1_error_proto_rawDescGZIP(), []int{1} + return file_controller_api_v1_error_proto_rawDescGZIP(), []int{2} } func (x *Error) GetStatus() int32 { @@ -171,27 +227,33 @@ var file_controller_api_v1_error_proto_rawDesc = []byte{ 0x0a, 0x1d, 0x63, 0x6f, 0x6e, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x65, 0x72, 0x2f, 0x61, 0x70, 0x69, 0x2f, 0x76, 0x31, 0x2f, 0x65, 0x72, 0x72, 0x6f, 0x72, 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x12, 0x11, 0x63, 0x6f, 0x6e, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x65, 0x72, 0x2e, 0x61, 0x70, 0x69, 0x2e, - 0x76, 0x31, 0x22, 0x70, 0x0a, 0x0c, 0x45, 0x72, 0x72, 0x6f, 0x72, 0x44, 0x65, 0x74, 0x61, 0x69, - 0x6c, 0x73, 0x12, 0x18, 0x0a, 0x07, 0x54, 0x72, 0x61, 0x63, 0x65, 0x49, 0x64, 0x18, 0x01, 0x20, - 0x01, 0x28, 0x09, 0x52, 0x07, 0x54, 0x72, 0x61, 0x63, 0x65, 0x49, 0x64, 0x12, 0x1e, 0x0a, 0x0a, - 0x72, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x5f, 0x69, 0x64, 0x18, 0x02, 0x20, 0x01, 0x28, 0x09, - 0x52, 0x0a, 0x72, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x5f, 0x69, 0x64, 0x12, 0x26, 0x0a, 0x0e, - 0x72, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x5f, 0x66, 0x69, 0x65, 0x6c, 0x64, 0x73, 0x18, 0x03, - 0x20, 0x03, 0x28, 0x09, 0x52, 0x0e, 0x72, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x5f, 0x66, 0x69, - 0x65, 0x6c, 0x64, 0x73, 0x22, 0x88, 0x01, 0x0a, 0x05, 0x45, 0x72, 0x72, 0x6f, 0x72, 0x12, 0x16, - 0x0a, 0x06, 0x73, 0x74, 0x61, 0x74, 0x75, 0x73, 0x18, 0x01, 0x20, 0x01, 0x28, 0x05, 0x52, 0x06, - 0x73, 0x74, 0x61, 0x74, 0x75, 0x73, 0x12, 0x12, 0x0a, 0x04, 0x63, 0x6f, 0x64, 0x65, 0x18, 0x02, - 0x20, 0x01, 0x28, 0x09, 0x52, 0x04, 0x63, 0x6f, 0x64, 0x65, 0x12, 0x18, 0x0a, 0x07, 0x6d, 0x65, - 0x73, 0x73, 0x61, 0x67, 0x65, 0x18, 0x03, 0x20, 0x01, 0x28, 0x09, 0x52, 0x07, 0x6d, 0x65, 0x73, - 0x73, 0x61, 0x67, 0x65, 0x12, 0x39, 0x0a, 0x07, 0x64, 0x65, 0x74, 0x61, 0x69, 0x6c, 0x73, 0x18, - 0x04, 0x20, 0x01, 0x28, 0x0b, 0x32, 0x1f, 0x2e, 0x63, 0x6f, 0x6e, 0x74, 0x72, 0x6f, 0x6c, 0x6c, - 0x65, 0x72, 0x2e, 0x61, 0x70, 0x69, 0x2e, 0x76, 0x31, 0x2e, 0x45, 0x72, 0x72, 0x6f, 0x72, 0x44, - 0x65, 0x74, 0x61, 0x69, 0x6c, 0x73, 0x52, 0x07, 0x64, 0x65, 0x74, 0x61, 0x69, 0x6c, 0x73, 0x42, - 0x41, 0x5a, 0x3f, 0x67, 0x69, 0x74, 0x68, 0x75, 0x62, 0x2e, 0x63, 0x6f, 0x6d, 0x2f, 0x68, 0x61, - 0x73, 0x68, 0x69, 0x63, 0x6f, 0x72, 0x70, 0x2f, 0x77, 0x61, 0x74, 0x63, 0x68, 0x74, 0x6f, 0x77, - 0x65, 0x72, 0x2f, 0x69, 0x6e, 0x74, 0x65, 0x72, 0x6e, 0x61, 0x6c, 0x2f, 0x67, 0x65, 0x6e, 0x2f, - 0x63, 0x6f, 0x6e, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x65, 0x72, 0x2f, 0x61, 0x70, 0x69, 0x3b, 0x61, - 0x70, 0x69, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, + 0x76, 0x31, 0x22, 0x8f, 0x01, 0x0a, 0x0c, 0x45, 0x72, 0x72, 0x6f, 0x72, 0x44, 0x65, 0x74, 0x61, + 0x69, 0x6c, 0x73, 0x12, 0x18, 0x0a, 0x07, 0x54, 0x72, 0x61, 0x63, 0x65, 0x49, 0x64, 0x18, 0x01, + 0x20, 0x01, 0x28, 0x09, 0x52, 0x07, 0x54, 0x72, 0x61, 0x63, 0x65, 0x49, 0x64, 0x12, 0x1e, 0x0a, + 0x0a, 0x72, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x5f, 0x69, 0x64, 0x18, 0x02, 0x20, 0x01, 0x28, + 0x09, 0x52, 0x0a, 0x72, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x5f, 0x69, 0x64, 0x12, 0x45, 0x0a, + 0x0e, 0x72, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x5f, 0x66, 0x69, 0x65, 0x6c, 0x64, 0x73, 0x18, + 0x03, 0x20, 0x03, 0x28, 0x0b, 0x32, 0x1d, 0x2e, 0x63, 0x6f, 0x6e, 0x74, 0x72, 0x6f, 0x6c, 0x6c, + 0x65, 0x72, 0x2e, 0x61, 0x70, 0x69, 0x2e, 0x76, 0x31, 0x2e, 0x46, 0x69, 0x65, 0x6c, 0x64, 0x45, + 0x72, 0x72, 0x6f, 0x72, 0x52, 0x0e, 0x72, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x5f, 0x66, 0x69, + 0x65, 0x6c, 0x64, 0x73, 0x22, 0x42, 0x0a, 0x0a, 0x46, 0x69, 0x65, 0x6c, 0x64, 0x45, 0x72, 0x72, + 0x6f, 0x72, 0x12, 0x12, 0x0a, 0x04, 0x6e, 0x61, 0x6d, 0x65, 0x18, 0x01, 0x20, 0x01, 0x28, 0x09, + 0x52, 0x04, 0x6e, 0x61, 0x6d, 0x65, 0x12, 0x20, 0x0a, 0x0b, 0x64, 0x65, 0x73, 0x63, 0x72, 0x69, + 0x70, 0x74, 0x69, 0x6f, 0x6e, 0x18, 0x02, 0x20, 0x01, 0x28, 0x09, 0x52, 0x0b, 0x64, 0x65, 0x73, + 0x63, 0x72, 0x69, 0x70, 0x74, 0x69, 0x6f, 0x6e, 0x22, 0x88, 0x01, 0x0a, 0x05, 0x45, 0x72, 0x72, + 0x6f, 0x72, 0x12, 0x16, 0x0a, 0x06, 0x73, 0x74, 0x61, 0x74, 0x75, 0x73, 0x18, 0x01, 0x20, 0x01, + 0x28, 0x05, 0x52, 0x06, 0x73, 0x74, 0x61, 0x74, 0x75, 0x73, 0x12, 0x12, 0x0a, 0x04, 0x63, 0x6f, + 0x64, 0x65, 0x18, 0x02, 0x20, 0x01, 0x28, 0x09, 0x52, 0x04, 0x63, 0x6f, 0x64, 0x65, 0x12, 0x18, + 0x0a, 0x07, 0x6d, 0x65, 0x73, 0x73, 0x61, 0x67, 0x65, 0x18, 0x03, 0x20, 0x01, 0x28, 0x09, 0x52, + 0x07, 0x6d, 0x65, 0x73, 0x73, 0x61, 0x67, 0x65, 0x12, 0x39, 0x0a, 0x07, 0x64, 0x65, 0x74, 0x61, + 0x69, 0x6c, 0x73, 0x18, 0x04, 0x20, 0x01, 0x28, 0x0b, 0x32, 0x1f, 0x2e, 0x63, 0x6f, 0x6e, 0x74, + 0x72, 0x6f, 0x6c, 0x6c, 0x65, 0x72, 0x2e, 0x61, 0x70, 0x69, 0x2e, 0x76, 0x31, 0x2e, 0x45, 0x72, + 0x72, 0x6f, 0x72, 0x44, 0x65, 0x74, 0x61, 0x69, 0x6c, 0x73, 0x52, 0x07, 0x64, 0x65, 0x74, 0x61, + 0x69, 0x6c, 0x73, 0x42, 0x41, 0x5a, 0x3f, 0x67, 0x69, 0x74, 0x68, 0x75, 0x62, 0x2e, 0x63, 0x6f, + 0x6d, 0x2f, 0x68, 0x61, 0x73, 0x68, 0x69, 0x63, 0x6f, 0x72, 0x70, 0x2f, 0x77, 0x61, 0x74, 0x63, + 0x68, 0x74, 0x6f, 0x77, 0x65, 0x72, 0x2f, 0x69, 0x6e, 0x74, 0x65, 0x72, 0x6e, 0x61, 0x6c, 0x2f, + 0x67, 0x65, 0x6e, 0x2f, 0x63, 0x6f, 0x6e, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x65, 0x72, 0x2f, 0x61, + 0x70, 0x69, 0x3b, 0x61, 0x70, 0x69, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, } var ( @@ -206,18 +268,20 @@ func file_controller_api_v1_error_proto_rawDescGZIP() []byte { return file_controller_api_v1_error_proto_rawDescData } -var file_controller_api_v1_error_proto_msgTypes = make([]protoimpl.MessageInfo, 2) +var file_controller_api_v1_error_proto_msgTypes = make([]protoimpl.MessageInfo, 3) var file_controller_api_v1_error_proto_goTypes = []interface{}{ (*ErrorDetails)(nil), // 0: controller.api.v1.ErrorDetails - (*Error)(nil), // 1: controller.api.v1.Error + (*FieldError)(nil), // 1: controller.api.v1.FieldError + (*Error)(nil), // 2: controller.api.v1.Error } var file_controller_api_v1_error_proto_depIdxs = []int32{ - 0, // 0: controller.api.v1.Error.details:type_name -> controller.api.v1.ErrorDetails - 1, // [1:1] is the sub-list for method output_type - 1, // [1:1] is the sub-list for method input_type - 1, // [1:1] is the sub-list for extension type_name - 1, // [1:1] is the sub-list for extension extendee - 0, // [0:1] is the sub-list for field type_name + 1, // 0: controller.api.v1.ErrorDetails.request_fields:type_name -> controller.api.v1.FieldError + 0, // 1: controller.api.v1.Error.details:type_name -> controller.api.v1.ErrorDetails + 2, // [2:2] is the sub-list for method output_type + 2, // [2:2] is the sub-list for method input_type + 2, // [2:2] is the sub-list for extension type_name + 2, // [2:2] is the sub-list for extension extendee + 0, // [0:2] is the sub-list for field type_name } func init() { file_controller_api_v1_error_proto_init() } @@ -239,6 +303,18 @@ func file_controller_api_v1_error_proto_init() { } } file_controller_api_v1_error_proto_msgTypes[1].Exporter = func(v interface{}, i int) interface{} { + switch v := v.(*FieldError); i { + case 0: + return &v.state + case 1: + return &v.sizeCache + case 2: + return &v.unknownFields + default: + return nil + } + } + file_controller_api_v1_error_proto_msgTypes[2].Exporter = func(v interface{}, i int) interface{} { switch v := v.(*Error); i { case 0: return &v.state @@ -257,7 +333,7 @@ func file_controller_api_v1_error_proto_init() { GoPackagePath: reflect.TypeOf(x{}).PkgPath(), RawDescriptor: file_controller_api_v1_error_proto_rawDesc, NumEnums: 0, - NumMessages: 2, + NumMessages: 3, NumExtensions: 0, NumServices: 0, }, diff --git a/internal/proto/local/controller/api/v1/error.proto b/internal/proto/local/controller/api/v1/error.proto index 104601145..b0b69d7bc 100644 --- a/internal/proto/local/controller/api/v1/error.proto +++ b/internal/proto/local/controller/api/v1/error.proto @@ -7,7 +7,13 @@ option go_package = "github.com/hashicorp/watchtower/internal/gen/controller/api message ErrorDetails { string TraceId = 1 [json_name="TraceId"]; string request_id = 2 [json_name="request_id"]; - repeated string request_fields = 3 [json_name="request_fields"]; + repeated FieldError request_fields = 3 [json_name="request_fields"]; +} + +// FieldErrors contains error information on a per field basis. +message FieldError { + string name = 1; + string description= 2; } // Error is returned by the JSON API when an error occurs. diff --git a/internal/servers/controller/handlers/errors.go b/internal/servers/controller/handlers/errors.go index a1e0e822d..b51df9bc4 100644 --- a/internal/servers/controller/handlers/errors.go +++ b/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()}) } } } diff --git a/internal/servers/controller/handlers/errors_test.go b/internal/servers/controller/handlers/errors_test.go index 9eff9abab..e8c4a6974 100644 --- a/internal/servers/controller/handlers/errors_test.go +++ b/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())) }) } } diff --git a/internal/servers/controller/handlers/host_catalogs/host_catalog_service.go b/internal/servers/controller/handlers/host_catalogs/host_catalog_service.go index c7d175d62..7846ae65b 100644 --- a/internal/servers/controller/handlers/host_catalogs/host_catalog_service.go +++ b/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 } diff --git a/internal/servers/controller/handlers/projects/project_service.go b/internal/servers/controller/handlers/projects/project_service.go index 88423b7ec..b6ab812af 100644 --- a/internal/servers/controller/handlers/projects/project_service.go +++ b/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{} }