From eb713b7a97e019913f4ca86c8c6d97a2644e2fe8 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Thu, 20 Aug 2020 15:49:16 +0200 Subject: [PATCH 1/7] use micro error codes and id --- pkg/proto/v0/settings.pb.micro_test.go | 16 ++++++++-------- pkg/service/v0/service.go | 10 +++++----- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/pkg/proto/v0/settings.pb.micro_test.go b/pkg/proto/v0/settings.pb.micro_test.go index ed04dd55f6..86e58591b5 100644 --- a/pkg/proto/v0/settings.pb.micro_test.go +++ b/pkg/proto/v0/settings.pb.micro_test.go @@ -103,7 +103,7 @@ func TestSettingsBundleProperties(t *testing.T) { "सिम्प्ले-display-name", "सिम्प्ले-extension-name", "सिम्प्ले", - merrors.New("", "extension: must be in a valid format; name: must be in a valid format.", 0), + merrors.New("ocis-settings", "extension: must be in a valid format; name: must be in a valid format.", 400), }, { "UTF validation on display name", @@ -111,7 +111,7 @@ func TestSettingsBundleProperties(t *testing.T) { "सिम्प्ले-display-name", "simple-extension-name", "123e4567-e89b-12d3-a456-426652340000", - merrors.New("", "name: must be in a valid format.", 0), + merrors.New("ocis-settings", "name: must be in a valid format.", 400), }, { "extension name with ../ in the name", @@ -119,7 +119,7 @@ func TestSettingsBundleProperties(t *testing.T) { "simple-display-name", "../folder-a-level-higher-up", "123e4567-e89b-12d3-a456-426652340000", - merrors.New("", "extension: must be in a valid format.", 0), + merrors.New("ocis-settings", "extension: must be in a valid format.", 400), }, { "extension name with \\ in the name", @@ -127,7 +127,7 @@ func TestSettingsBundleProperties(t *testing.T) { "simple-display-name", "\\", "123e4567-e89b-12d3-a456-426652340000", - merrors.New("", "extension: must be in a valid format.", 0), + merrors.New("ocis-settings", "extension: must be in a valid format.", 400), }, { "spaces are disallowed in bundle names", @@ -135,7 +135,7 @@ func TestSettingsBundleProperties(t *testing.T) { "simple display name", "simple extension name", "123e4567-e89b-12d3-a456-426652340000", - merrors.New("", "extension: must be in a valid format; name: must be in a valid format.", 0), + merrors.New("ocis-settings", "extension: must be in a valid format; name: must be in a valid format.", 400), }, { "spaces are allowed in display names", @@ -151,7 +151,7 @@ func TestSettingsBundleProperties(t *testing.T) { "simple-display-name", "", "123e4567-e89b-12d3-a456-426652340000", - merrors.New("", "extension: cannot be blank.", 0), + merrors.New("ocis-settings", "extension: cannot be blank.", 400), }, { "display name missing", @@ -159,7 +159,7 @@ func TestSettingsBundleProperties(t *testing.T) { "", "simple-extension-name", "123e4567-e89b-12d3-a456-426652340000", - merrors.New("", "display_name: cannot be blank.", 0), + merrors.New("ocis-settings", "display_name: cannot be blank.", 400), }, { "UUID missing (omitted on bundles)", @@ -216,7 +216,7 @@ func TestSettingsBundleWithoutSettings(t *testing.T) { response, err := cl.SaveBundle(context.Background(), &createRequest) assert.Error(t, err) assert.Nil(t, response) - assert.Equal(t, merrors.New("", "extension: cannot be blank; name: cannot be blank; settings: cannot be blank.", 0), err) + assert.Equal(t, merrors.New("ocis-settings", "extension: cannot be blank; name: cannot be blank; settings: cannot be blank.", 400), err) os.RemoveAll(dataStore) } diff --git a/pkg/service/v0/service.go b/pkg/service/v0/service.go index 3ef35340f4..b67d28c7d7 100644 --- a/pkg/service/v0/service.go +++ b/pkg/service/v0/service.go @@ -52,11 +52,11 @@ func NewService(cfg *config.Config, logger log.Logger) Service { func (g Service) SaveBundle(c context.Context, req *proto.SaveBundleRequest, res *proto.SaveBundleResponse) error { cleanUpResource(c, req.Bundle.Resource) if validationError := validateSaveBundle(req); validationError != nil { - return merrors.FromError(validationError) + return merrors.FromError(merrors.BadRequest("ocis-settings", "%s", validationError)) } r, err := g.manager.WriteBundle(req.Bundle) if err != nil { - return merrors.FromError(err) + return merrors.FromError(merrors.InternalServerError("ocis-settings", "%s", err)) } res.Bundle = r return nil @@ -118,15 +118,15 @@ func (g Service) SaveValue(c context.Context, req *proto.SaveValueRequest, res * cleanUpResource(c, req.Value.Resource) // TODO: we need to check, if the authenticated user has permission to write the value for the specified resource (e.g. global, file with id xy, ...) if validationError := validateSaveValue(req); validationError != nil { - return merrors.FromError(validationError) + return merrors.FromError(merrors.BadRequest("ocis-settings", "%s", validationError)) } r, err := g.manager.WriteValue(req.Value) if err != nil { - return merrors.FromError(err) + return merrors.FromError(merrors.BadRequest("ocis-settings", "%s", err)) } valueWithIdentifier, err := g.getValueWithIdentifier(r) if err != nil { - return merrors.FromError(err) + return merrors.FromError(merrors.NotFound("ocis-settings", "%s", err)) } res.Value = valueWithIdentifier return nil From 02408dc1eef157a6d08ece48beed2ed602d8b71c Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Thu, 20 Aug 2020 15:50:33 +0200 Subject: [PATCH 2/7] badrequest > internalservererror --- pkg/service/v0/service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/service/v0/service.go b/pkg/service/v0/service.go index b67d28c7d7..79ec67ae00 100644 --- a/pkg/service/v0/service.go +++ b/pkg/service/v0/service.go @@ -56,7 +56,7 @@ func (g Service) SaveBundle(c context.Context, req *proto.SaveBundleRequest, res } r, err := g.manager.WriteBundle(req.Bundle) if err != nil { - return merrors.FromError(merrors.InternalServerError("ocis-settings", "%s", err)) + return merrors.FromError(merrors.BadRequest("ocis-settings", "%s", err)) } res.Bundle = r return nil From 32434fa96c6c7d4b0bb4cd94a13efdfff1464971 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Thu, 20 Aug 2020 16:04:52 +0200 Subject: [PATCH 3/7] adjust error codes --- pkg/service/v0/service.go | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/pkg/service/v0/service.go b/pkg/service/v0/service.go index 79ec67ae00..179b921bfa 100644 --- a/pkg/service/v0/service.go +++ b/pkg/service/v0/service.go @@ -52,11 +52,11 @@ func NewService(cfg *config.Config, logger log.Logger) Service { func (g Service) SaveBundle(c context.Context, req *proto.SaveBundleRequest, res *proto.SaveBundleResponse) error { cleanUpResource(c, req.Bundle.Resource) if validationError := validateSaveBundle(req); validationError != nil { - return merrors.FromError(merrors.BadRequest("ocis-settings", "%s", validationError)) + return merrors.BadRequest("ocis-settings", "%s", validationError) } r, err := g.manager.WriteBundle(req.Bundle) if err != nil { - return merrors.FromError(merrors.BadRequest("ocis-settings", "%s", err)) + return merrors.BadRequest("ocis-settings", "%s", err) } res.Bundle = r return nil @@ -65,11 +65,11 @@ func (g Service) SaveBundle(c context.Context, req *proto.SaveBundleRequest, res // GetBundle implements the BundleServiceHandler interface func (g Service) GetBundle(c context.Context, req *proto.GetBundleRequest, res *proto.GetBundleResponse) error { if validationError := validateGetBundle(req); validationError != nil { - return merrors.FromError(validationError) + return merrors.BadRequest("ocis-settings", "%s", validationError) } r, err := g.manager.ReadBundle(req.BundleId) if err != nil { - return merrors.FromError(err) + return merrors.NotFound("ocis-settings", "%s", err) } res.Bundle = r return nil @@ -80,11 +80,11 @@ func (g Service) ListBundles(c context.Context, req *proto.ListBundlesRequest, r // fetch all bundles req.AccountUuid = getValidatedAccountUUID(c, req.AccountUuid) if validationError := validateListBundles(req); validationError != nil { - return merrors.FromError(validationError) + return merrors.BadRequest("ocis-settings", "%s", validationError) } bundles, err := g.manager.ListBundles(proto.Bundle_TYPE_DEFAULT) if err != nil { - return merrors.FromError(err) + return merrors.NotFound("ocis-settings", "%s", err) } res.Bundles = bundles return nil @@ -94,11 +94,11 @@ func (g Service) ListBundles(c context.Context, req *proto.ListBundlesRequest, r func (g Service) AddSettingToBundle(c context.Context, req *proto.AddSettingToBundleRequest, res *proto.AddSettingToBundleResponse) error { cleanUpResource(c, req.Setting.Resource) if validationError := validateAddSettingToBundle(req); validationError != nil { - return merrors.FromError(validationError) + return merrors.BadRequest("ocis-settings", "%s", validationError) } r, err := g.manager.AddSettingToBundle(req.BundleId, req.Setting) if err != nil { - return merrors.FromError(err) + return merrors.BadRequest("ocis-settings", "%s", err) } res.Setting = r return nil @@ -107,7 +107,7 @@ func (g Service) AddSettingToBundle(c context.Context, req *proto.AddSettingToBu // RemoveSettingFromBundle implements the BundleServiceHandler interface func (g Service) RemoveSettingFromBundle(c context.Context, req *proto.RemoveSettingFromBundleRequest, _ *empty.Empty) error { if validationError := validateRemoveSettingFromBundle(req); validationError != nil { - return merrors.FromError(validationError) + return merrors.BadRequest("ocis-settings", "%s", validationError) } return g.manager.RemoveSettingFromBundle(req.BundleId, req.SettingId) } @@ -118,15 +118,15 @@ func (g Service) SaveValue(c context.Context, req *proto.SaveValueRequest, res * cleanUpResource(c, req.Value.Resource) // TODO: we need to check, if the authenticated user has permission to write the value for the specified resource (e.g. global, file with id xy, ...) if validationError := validateSaveValue(req); validationError != nil { - return merrors.FromError(merrors.BadRequest("ocis-settings", "%s", validationError)) + return merrors.BadRequest("ocis-settings", "%s", validationError) } r, err := g.manager.WriteValue(req.Value) if err != nil { - return merrors.FromError(merrors.BadRequest("ocis-settings", "%s", err)) + return merrors.BadRequest("ocis-settings", "%s", err) } valueWithIdentifier, err := g.getValueWithIdentifier(r) if err != nil { - return merrors.FromError(merrors.NotFound("ocis-settings", "%s", err)) + return merrors.NotFound("ocis-settings", "%s", err) } res.Value = valueWithIdentifier return nil @@ -135,15 +135,15 @@ func (g Service) SaveValue(c context.Context, req *proto.SaveValueRequest, res * // GetValue implements the ValueServiceHandler interface func (g Service) GetValue(c context.Context, req *proto.GetValueRequest, res *proto.GetValueResponse) error { if validationError := validateGetValue(req); validationError != nil { - return merrors.FromError(validationError) + return merrors.BadRequest("ocis-settings", "%s", validationError) } r, err := g.manager.ReadValue(req.Id) if err != nil { - return merrors.FromError(err) + return merrors.NotFound("ocis-settings", "%s", err) } valueWithIdentifier, err := g.getValueWithIdentifier(r) if err != nil { - return merrors.FromError(err) + return merrors.NotFound("ocis-settings", "%s", err) } res.Value = valueWithIdentifier return nil @@ -153,13 +153,13 @@ func (g Service) GetValue(c context.Context, req *proto.GetValueRequest, res *pr func (g Service) GetValueByUniqueIdentifiers(ctx context.Context, in *proto.GetValueByUniqueIdentifiersRequest, res *proto.GetValueResponse) error { v, err := g.manager.ReadValueByUniqueIdentifiers(in.AccountUuid, in.SettingId) if err != nil { - return merrors.FromError(err) + return merrors.NotFound("ocis-settings", "%s", err) } if v.BundleId != "" { valueWithIdentifier, err := g.getValueWithIdentifier(v) if err != nil { - return merrors.FromError(err) + return merrors.NotFound("ocis-settings", "%s", err) } res.Value = valueWithIdentifier @@ -171,11 +171,11 @@ func (g Service) GetValueByUniqueIdentifiers(ctx context.Context, in *proto.GetV func (g Service) ListValues(c context.Context, req *proto.ListValuesRequest, res *proto.ListValuesResponse) error { req.AccountUuid = getValidatedAccountUUID(c, req.AccountUuid) if validationError := validateListValues(req); validationError != nil { - return merrors.FromError(validationError) + return merrors.BadRequest("ocis-settings", "%s", validationError) } r, err := g.manager.ListValues(req.BundleId, req.AccountUuid) if err != nil { - return merrors.FromError(err) + return merrors.NotFound("ocis-settings", "%s", err) } var result []*proto.ValueWithIdentifier for _, value := range r { From 83eb86b5eb7b593c88e05ce89cabcadef8f6ae2a Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Thu, 20 Aug 2020 16:15:06 +0200 Subject: [PATCH 4/7] missing error on handlers --- pkg/service/v0/service.go | 52 +++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/pkg/service/v0/service.go b/pkg/service/v0/service.go index 179b921bfa..6bb8b1e5f6 100644 --- a/pkg/service/v0/service.go +++ b/pkg/service/v0/service.go @@ -188,34 +188,15 @@ func (g Service) ListValues(c context.Context, req *proto.ListValuesRequest, res return nil } -func (g Service) getValueWithIdentifier(value *proto.Value) (*proto.ValueWithIdentifier, error) { - bundle, err := g.manager.ReadBundle(value.BundleId) - if err != nil { - return nil, err - } - setting, err := g.manager.ReadSetting(value.SettingId) - if err != nil { - return nil, err - } - return &proto.ValueWithIdentifier{ - Identifier: &proto.Identifier{ - Extension: bundle.Extension, - Bundle: bundle.Name, - Setting: setting.Name, - }, - Value: value, - }, nil -} - // ListRoles implements the RoleServiceHandler interface func (g Service) ListRoles(c context.Context, req *proto.ListBundlesRequest, res *proto.ListBundlesResponse) error { req.AccountUuid = getValidatedAccountUUID(c, req.AccountUuid) if validationError := validateListRoles(req); validationError != nil { - return merrors.FromError(validationError) + return merrors.BadRequest("ocis-settings", "%s", validationError) } r, err := g.manager.ListBundles(proto.Bundle_TYPE_ROLE) if err != nil { - return merrors.FromError(err) + return merrors.NotFound("ocis-settings", "%s", err) } res.Bundles = r return nil @@ -225,11 +206,11 @@ func (g Service) ListRoles(c context.Context, req *proto.ListBundlesRequest, res func (g Service) ListRoleAssignments(c context.Context, req *proto.ListRoleAssignmentsRequest, res *proto.ListRoleAssignmentsResponse) error { req.AccountUuid = getValidatedAccountUUID(c, req.AccountUuid) if validationError := validateListRoleAssignments(req); validationError != nil { - return merrors.FromError(validationError) + return merrors.BadRequest("ocis-settings", "%s", validationError) } r, err := g.manager.ListRoleAssignments(req.AccountUuid) if err != nil { - return merrors.FromError(err) + return merrors.NotFound("ocis-settings", "%s", err) } res.Assignments = r return nil @@ -239,11 +220,11 @@ func (g Service) ListRoleAssignments(c context.Context, req *proto.ListRoleAssig func (g Service) AssignRoleToUser(c context.Context, req *proto.AssignRoleToUserRequest, res *proto.AssignRoleToUserResponse) error { req.AccountUuid = getValidatedAccountUUID(c, req.AccountUuid) if validationError := validateAssignRoleToUser(req); validationError != nil { - return merrors.FromError(validationError) + return merrors.BadRequest("ocis-settings", "%s", validationError) } r, err := g.manager.WriteRoleAssignment(req.AccountUuid, req.RoleId) if err != nil { - return merrors.FromError(err) + return merrors.BadRequest("ocis-settings", "%s", err) } res.Assignment = r return nil @@ -252,7 +233,7 @@ func (g Service) AssignRoleToUser(c context.Context, req *proto.AssignRoleToUser // RemoveRoleFromUser implements the RoleServiceHandler interface func (g Service) RemoveRoleFromUser(c context.Context, req *proto.RemoveRoleFromUserRequest, _ *empty.Empty) error { if validationError := validateRemoveRoleFromUser(req); validationError != nil { - return merrors.FromError(validationError) + return merrors.BadRequest("ocis-settings", "%s", validationError) } return g.manager.RemoveRoleAssignment(req.Id) } @@ -274,3 +255,22 @@ func getValidatedAccountUUID(c context.Context, accountUUID string) string { } return accountUUID } + +func (g Service) getValueWithIdentifier(value *proto.Value) (*proto.ValueWithIdentifier, error) { + bundle, err := g.manager.ReadBundle(value.BundleId) + if err != nil { + return nil, err + } + setting, err := g.manager.ReadSetting(value.SettingId) + if err != nil { + return nil, err + } + return &proto.ValueWithIdentifier{ + Identifier: &proto.Identifier{ + Extension: bundle.Extension, + Bundle: bundle.Name, + Setting: setting.Name, + }, + Value: value, + }, nil +} From 5e8adce357758e3f3eee27301823416747aaedb3 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Thu, 20 Aug 2020 16:17:43 +0200 Subject: [PATCH 5/7] not my day --- pkg/service/v0/service.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/service/v0/service.go b/pkg/service/v0/service.go index 6bb8b1e5f6..fddc07f5f2 100644 --- a/pkg/service/v0/service.go +++ b/pkg/service/v0/service.go @@ -235,7 +235,10 @@ func (g Service) RemoveRoleFromUser(c context.Context, req *proto.RemoveRoleFrom if validationError := validateRemoveRoleFromUser(req); validationError != nil { return merrors.BadRequest("ocis-settings", "%s", validationError) } - return g.manager.RemoveRoleAssignment(req.Id) + if err := g.manager.RemoveRoleAssignment(req.Id); err != nil { + return merrors.BadRequest("ocis-settings", "%s", err) + } + return nil } // cleanUpResource makes sure that the account uuid of the authenticated user is injected if needed. From e4c603e7b5a23a2c2e7bb64bdd79744d0712e357 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Thu, 20 Aug 2020 16:19:44 +0200 Subject: [PATCH 6/7] leftover error handling --- pkg/service/v0/service.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/service/v0/service.go b/pkg/service/v0/service.go index fddc07f5f2..a3a5c0d3dd 100644 --- a/pkg/service/v0/service.go +++ b/pkg/service/v0/service.go @@ -109,7 +109,9 @@ func (g Service) RemoveSettingFromBundle(c context.Context, req *proto.RemoveSet if validationError := validateRemoveSettingFromBundle(req); validationError != nil { return merrors.BadRequest("ocis-settings", "%s", validationError) } - return g.manager.RemoveSettingFromBundle(req.BundleId, req.SettingId) + if err := g.manager.RemoveSettingFromBundle(req.BundleId, req.SettingId); err != nil { + return merrors.BadRequest("ocis-settings", "%s", err) + } } // SaveValue implements the ValueServiceHandler interface From 0001bc8937005ffd36015b4a1f85e6455cdd45cb Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Thu, 20 Aug 2020 16:22:52 +0200 Subject: [PATCH 7/7] fix govet --- pkg/service/v0/service.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/service/v0/service.go b/pkg/service/v0/service.go index a3a5c0d3dd..c46138a73e 100644 --- a/pkg/service/v0/service.go +++ b/pkg/service/v0/service.go @@ -112,6 +112,8 @@ func (g Service) RemoveSettingFromBundle(c context.Context, req *proto.RemoveSet if err := g.manager.RemoveSettingFromBundle(req.BundleId, req.SettingId); err != nil { return merrors.BadRequest("ocis-settings", "%s", err) } + + return nil } // SaveValue implements the ValueServiceHandler interface