From 21f5cc274fc1613786e412cdc93c14d415d779f0 Mon Sep 17 00:00:00 2001 From: Todd Knight Date: Mon, 21 Sep 2020 12:09:30 -0700 Subject: [PATCH] Don't return nil, nil for API service methods. Add tests for updating w/ wrong version. (#396) --- api/accounts/account_test.go | 6 ++++++ api/groups/group_test.go | 6 ++++++ api/hostcatalogs/host_catalog_test.go | 6 ++++++ api/hosts/host_test.go | 6 ++++++ api/hostsets/host_set_test.go | 6 ++++++ api/roles/role_test.go | 6 ++++++ api/scopes/scope_test.go | 6 ++++++ api/targets/target_test.go | 8 +++++++- api/users/user_test.go | 6 ++++++ .../handlers/accounts/account_service.go | 2 +- .../handlers/authmethods/authmethod_service.go | 2 +- .../handlers/authtokens/authtoken_service.go | 2 +- .../handlers/groups/group_service.go | 2 +- .../host_catalogs/host_catalog_service.go | 2 +- .../handlers/host_sets/host_set_service.go | 2 +- .../controller/handlers/hosts/host_service.go | 2 +- .../controller/handlers/roles/role_service.go | 2 +- .../handlers/scopes/scope_service.go | 2 +- .../handlers/targets/target_service.go | 18 ++++++------------ internal/servers/worker/session.go | 2 +- 20 files changed, 71 insertions(+), 23 deletions(-) diff --git a/api/accounts/account_test.go b/api/accounts/account_test.go index f4cb02771..a2af3d76a 100644 --- a/api/accounts/account_test.go +++ b/api/accounts/account_test.go @@ -174,6 +174,12 @@ func TestErrors(t *testing.T) { assert.Nil(apiErr) assert.NotNil(u) + // Updating the wrong version should fail. + _, apiErr, err = accountClient.Update(tc.Context(), u.Item.Id, 73, accounts.WithName("anything")) + require.NoError(err) + assert.NotNil(apiErr) + assert.EqualValues(http.StatusNotFound, apiErr.Status) + // Create another resource with the same name. _, apiErr, err = accountClient.Create(tc.Context(), amId, accounts.WithPasswordAccountLoginName("first")) require.NoError(err) diff --git a/api/groups/group_test.go b/api/groups/group_test.go index 927f03acc..a6ea635f0 100644 --- a/api/groups/group_test.go +++ b/api/groups/group_test.go @@ -197,6 +197,12 @@ func TestErrors(t *testing.T) { assert.Nil(apiErr) assert.NotNil(g) + // Updating the wrong version should fail. + _, apiErr, err = groupClient.Update(tc.Context(), g.Item.Id, 73, groups.WithName("anything")) + require.NoError(err) + assert.NotNil(apiErr) + assert.EqualValues(http.StatusNotFound, apiErr.Status) + // Create another resource with the same name. _, apiErr, err = groupClient.Create(tc.Context(), tt.scopeId, groups.WithName("first")) require.NoError(err) diff --git a/api/hostcatalogs/host_catalog_test.go b/api/hostcatalogs/host_catalog_test.go index 0cdaf560b..48ae95bbe 100644 --- a/api/hostcatalogs/host_catalog_test.go +++ b/api/hostcatalogs/host_catalog_test.go @@ -137,6 +137,12 @@ func TestErrors(t *testing.T) { assert.Nil(apiErr) assert.NotNil(hc) + // Updating the wrong version should fail. + _, apiErr, err = pc.Update(tc.Context(), hc.Item.Id, 73, hostcatalogs.WithName("anything")) + require.NoError(err) + assert.NotNil(apiErr) + assert.EqualValues(http.StatusNotFound, apiErr.Status) + _, apiErr, err = pc.Create(tc.Context(), "static", proj.GetPublicId(), hostcatalogs.WithName("foo")) require.NoError(err) assert.NotNil(apiErr) diff --git a/api/hosts/host_test.go b/api/hosts/host_test.go index 5e9ed4749..e1d65b001 100644 --- a/api/hosts/host_test.go +++ b/api/hosts/host_test.go @@ -155,6 +155,12 @@ func TestErrors(t *testing.T) { require.Nil(apiErr) assert.NotNil(h) + // Updating the wrong version should fail. + _, apiErr, err = hClient.Update(tc.Context(), h.Item.Id, 73, hosts.WithName("anything")) + require.NoError(err) + assert.NotNil(apiErr) + assert.EqualValues(http.StatusNotFound, apiErr.Status) + _, apiErr, err = hClient.Create(tc.Context(), hc.Item.Id, hosts.WithName("foo"), hosts.WithStaticHostAddress("someaddress")) require.NoError(err) assert.NotNil(apiErr) diff --git a/api/hostsets/host_set_test.go b/api/hostsets/host_set_test.go index 8c08d2b33..31541df42 100644 --- a/api/hostsets/host_set_test.go +++ b/api/hostsets/host_set_test.go @@ -200,6 +200,12 @@ func TestErrors(t *testing.T) { require.Nil(apiErr) assert.NotNil(h) + // Updating the wrong version should fail. + _, apiErr, err = hClient.Update(tc.Context(), h.Item.Id, 73, hostsets.WithName("anything")) + require.NoError(err) + assert.NotNil(apiErr) + assert.EqualValues(http.StatusNotFound, apiErr.Status) + h, apiErr, err = hClient.Create(tc.Context(), hc.Item.Id, hostsets.WithName("foo")) require.NoError(err) assert.NotNil(apiErr) diff --git a/api/roles/role_test.go b/api/roles/role_test.go index dbeccf823..27ef1ca9a 100644 --- a/api/roles/role_test.go +++ b/api/roles/role_test.go @@ -292,6 +292,12 @@ func TestErrors(t *testing.T) { assert.Nil(apiErr) assert.NotNil(u) + // Updating the wrong version should fail. + _, apiErr, err = roleClient.Update(tc.Context(), u.Item.Id, 73, roles.WithName("anything")) + require.NoError(err) + assert.NotNil(apiErr) + assert.EqualValues(http.StatusNotFound, apiErr.Status) + // Create another resource with the same name. _, apiErr, err = roleClient.Create(tc.Context(), tt.scopeId, roles.WithName("first")) require.NoError(err) diff --git a/api/scopes/scope_test.go b/api/scopes/scope_test.go index 97746f7c1..c2ef484e8 100644 --- a/api/scopes/scope_test.go +++ b/api/scopes/scope_test.go @@ -123,6 +123,12 @@ func TestErrors(t *testing.T) { assert.NotNil(createdProj) assert.Nil(apiErr) + // Updating the wrong version should fail. + _, apiErr, err = scps.Update(tc.Context(), createdProj.Item.Id, 73, scopes.WithName("anything")) + require.NoError(err) + assert.NotNil(apiErr) + assert.EqualValues(http.StatusNotFound, apiErr.Status) + _, apiErr, err = scps.Read(tc.Context(), "p_doesntexis") require.NoError(err) // TODO: Should this be nil instead of just a Project that has no values set diff --git a/api/targets/target_test.go b/api/targets/target_test.go index cd333a715..a6f7eb7f0 100644 --- a/api/targets/target_test.go +++ b/api/targets/target_test.go @@ -170,7 +170,6 @@ func TestCrud(t *testing.T) { assert.EqualValues(http.StatusNotFound, apiErr.Status) } -// TODO: Get better coverage for expected errors and error formats. func TestSet_Errors(t *testing.T) { assert, require := assert.New(t), require.New(t) tc := controller.NewTestController(t, nil) @@ -187,6 +186,13 @@ func TestSet_Errors(t *testing.T) { require.NoError(err) require.Nil(apiErr) assert.NotNil(tar) + + // Updating the wrong version should fail. + _, apiErr, err = tarClient.Update(tc.Context(), tar.Item.Id, 73, targets.WithName("anything")) + require.NoError(err) + assert.NotNil(apiErr) + assert.EqualValues(http.StatusNotFound, apiErr.Status) + tar, apiErr, err = tarClient.Create(tc.Context(), "tcp", proj.GetPublicId(), targets.WithName("foo")) require.NoError(err) assert.NotNil(apiErr) diff --git a/api/users/user_test.go b/api/users/user_test.go index c7f97dc84..662e84d6f 100644 --- a/api/users/user_test.go +++ b/api/users/user_test.go @@ -134,6 +134,12 @@ func TestErrors(t *testing.T) { assert.Nil(apiErr) assert.NotNil(u) + // Updating the wrong version should fail. + _, apiErr, err = userClient.Update(tc.Context(), u.Item.Id, 73, users.WithName("anything")) + require.NoError(err) + assert.NotNil(apiErr) + assert.EqualValues(http.StatusNotFound, apiErr.Status) + // Create another resource with the same name. _, apiErr, err = userClient.Create(tc.Context(), org.GetPublicId(), users.WithName("first")) require.NoError(err) diff --git a/internal/servers/controller/handlers/accounts/account_service.go b/internal/servers/controller/handlers/accounts/account_service.go index 56e9490db..ded13e3be 100644 --- a/internal/servers/controller/handlers/accounts/account_service.go +++ b/internal/servers/controller/handlers/accounts/account_service.go @@ -129,7 +129,7 @@ func (s Service) DeleteAccount(ctx context.Context, req *pbs.DeleteAccountReques if err != nil { return nil, err } - return nil, nil + return &pbs.DeleteAccountResponse{}, nil } // ChangePassword implements the interface pbs.AccountServiceServer. diff --git a/internal/servers/controller/handlers/authmethods/authmethod_service.go b/internal/servers/controller/handlers/authmethods/authmethod_service.go index 4df2d4faa..55bc8a3b7 100644 --- a/internal/servers/controller/handlers/authmethods/authmethod_service.go +++ b/internal/servers/controller/handlers/authmethods/authmethod_service.go @@ -134,7 +134,7 @@ func (s Service) DeleteAuthMethod(ctx context.Context, req *pbs.DeleteAuthMethod if err != nil { return nil, err } - return nil, nil + return &pbs.DeleteAuthMethodResponse{}, nil } func (s Service) getFromRepo(ctx context.Context, id string) (*pb.AuthMethod, error) { diff --git a/internal/servers/controller/handlers/authtokens/authtoken_service.go b/internal/servers/controller/handlers/authtokens/authtoken_service.go index 13801c622..730d0fb4d 100644 --- a/internal/servers/controller/handlers/authtokens/authtoken_service.go +++ b/internal/servers/controller/handlers/authtokens/authtoken_service.go @@ -87,7 +87,7 @@ func (s Service) DeleteAuthToken(ctx context.Context, req *pbs.DeleteAuthTokenRe if err != nil { return nil, err } - return nil, nil + return &pbs.DeleteAuthTokenResponse{}, nil } func (s Service) getFromRepo(ctx context.Context, id string) (*pb.AuthToken, error) { diff --git a/internal/servers/controller/handlers/groups/group_service.go b/internal/servers/controller/handlers/groups/group_service.go index 1bad2f46f..235e63663 100644 --- a/internal/servers/controller/handlers/groups/group_service.go +++ b/internal/servers/controller/handlers/groups/group_service.go @@ -130,7 +130,7 @@ func (s Service) DeleteGroup(ctx context.Context, req *pbs.DeleteGroupRequest) ( if err != nil { return nil, err } - return nil, nil + return &pbs.DeleteGroupResponse{}, nil } // AddGroupMembers implements the interface pbs.GroupServiceServer. 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 972a54d3f..bef9edbb0 100644 --- a/internal/servers/controller/handlers/host_catalogs/host_catalog_service.go +++ b/internal/servers/controller/handlers/host_catalogs/host_catalog_service.go @@ -135,7 +135,7 @@ func (s Service) DeleteHostCatalog(ctx context.Context, req *pbs.DeleteHostCatal if err != nil { return nil, err } - return nil, nil + return &pbs.DeleteHostCatalogResponse{}, nil } func (s Service) getFromRepo(ctx context.Context, id string) (*pb.HostCatalog, error) { diff --git a/internal/servers/controller/handlers/host_sets/host_set_service.go b/internal/servers/controller/handlers/host_sets/host_set_service.go index 5a376dcce..cd33fe629 100644 --- a/internal/servers/controller/handlers/host_sets/host_set_service.go +++ b/internal/servers/controller/handlers/host_sets/host_set_service.go @@ -130,7 +130,7 @@ func (s Service) DeleteHostSet(ctx context.Context, req *pbs.DeleteHostSetReques if err != nil { return nil, err } - return nil, nil + return &pbs.DeleteHostSetResponse{}, nil } // AddHostSetHosts implements the interface pbs.HostSetServiceServer. diff --git a/internal/servers/controller/handlers/hosts/host_service.go b/internal/servers/controller/handlers/hosts/host_service.go index b1efc3219..f4d32d4ad 100644 --- a/internal/servers/controller/handlers/hosts/host_service.go +++ b/internal/servers/controller/handlers/hosts/host_service.go @@ -132,7 +132,7 @@ func (s Service) DeleteHost(ctx context.Context, req *pbs.DeleteHostRequest) (*p if err != nil { return nil, err } - return nil, nil + return &pbs.DeleteHostResponse{}, nil } func (s Service) getFromRepo(ctx context.Context, id string) (*pb.Host, error) { diff --git a/internal/servers/controller/handlers/roles/role_service.go b/internal/servers/controller/handlers/roles/role_service.go index 13f94fc58..3c9a4acc0 100644 --- a/internal/servers/controller/handlers/roles/role_service.go +++ b/internal/servers/controller/handlers/roles/role_service.go @@ -131,7 +131,7 @@ func (s Service) DeleteRole(ctx context.Context, req *pbs.DeleteRoleRequest) (*p if err != nil { return nil, err } - return nil, nil + return &pbs.DeleteRoleResponse{}, nil } // AddRolePrincipals implements the interface pbs.RoleServiceServer. diff --git a/internal/servers/controller/handlers/scopes/scope_service.go b/internal/servers/controller/handlers/scopes/scope_service.go index 99708293b..ae8bfb870 100644 --- a/internal/servers/controller/handlers/scopes/scope_service.go +++ b/internal/servers/controller/handlers/scopes/scope_service.go @@ -136,7 +136,7 @@ func (s Service) DeleteScope(ctx context.Context, req *pbs.DeleteScopeRequest) ( if err != nil { return nil, err } - return nil, nil + return &pbs.DeleteScopeResponse{}, nil } func (s Service) getFromRepo(ctx context.Context, id string) (*pb.Scope, error) { diff --git a/internal/servers/controller/handlers/targets/target_service.go b/internal/servers/controller/handlers/targets/target_service.go index f039e6b9a..7a7004048 100644 --- a/internal/servers/controller/handlers/targets/target_service.go +++ b/internal/servers/controller/handlers/targets/target_service.go @@ -120,9 +120,6 @@ func (s Service) GetTarget(ctx context.Context, req *pbs.GetTargetRequest) (*pbs if err != nil { return nil, err } - if u == nil { - return nil, nil - } u.Scope = authResults.Scope return &pbs.GetTargetResponse{Item: u}, nil } @@ -157,9 +154,6 @@ func (s Service) UpdateTarget(ctx context.Context, req *pbs.UpdateTargetRequest) if err != nil { return nil, err } - if u == nil { - return nil, nil - } u.Scope = authResults.Scope return &pbs.UpdateTargetResponse{Item: u}, nil } @@ -177,7 +171,7 @@ func (s Service) DeleteTarget(ctx context.Context, req *pbs.DeleteTargetRequest) if err != nil { return nil, err } - return nil, nil + return &pbs.DeleteTargetResponse{}, nil } // AddTargetHostSets implements the interface pbs.TargetServiceServer. @@ -265,12 +259,12 @@ func (s Service) AuthorizeSession(ctx context.Context, req *pbs.AuthorizeSession t, hostSets, err := repo.LookupTarget(ctx, req.GetId()) if err != nil { if errors.Is(err, db.ErrRecordNotFound) { - return nil, nil + return nil, handlers.NotFoundErrorf("Target %q not found.", req.GetId()) } return nil, err } if t == nil { - return nil, nil + return nil, handlers.NotFoundErrorf("Target %q not found.", req.GetId()) } // Instantiate some repos @@ -427,12 +421,12 @@ func (s Service) getFromRepo(ctx context.Context, id string) (*pb.Target, error) u, m, err := repo.LookupTarget(ctx, id) if err != nil { if errors.Is(err, db.ErrRecordNotFound) { - return nil, nil + return nil, handlers.NotFoundErrorf("Target %q doesn't exist.", id) } return nil, err } if u == nil { - return nil, nil + return nil, handlers.NotFoundErrorf("Target %q doesn't exist.", id) } return toProto(u, m) } @@ -513,7 +507,7 @@ func (s Service) updateInRepo(ctx context.Context, scopeId, id string, mask []st return nil, status.Errorf(codes.Internal, "Unable to update target: %v.", err) } if rowsUpdated == 0 { - return nil, nil + return nil, handlers.NotFoundErrorf("Target %q not found or incorrect version provided.", id) } return toProto(out, m) } diff --git a/internal/servers/worker/session.go b/internal/servers/worker/session.go index a5bb4777b..acdd9519d 100644 --- a/internal/servers/worker/session.go +++ b/internal/servers/worker/session.go @@ -228,7 +228,7 @@ func (w *Worker) closeConnection(ctx context.Context, req *pbs.CloseConnectionRe } func (w *Worker) closeConnections(ctx context.Context, closeMap map[string]string) error { - w.logger.Trace("marking connections as closed", "session_and_connection_ids", fmt.Sprint("%#v", closeMap)) + w.logger.Trace("marking connections as closed", "session_and_connection_ids", fmt.Sprintf("%#v", closeMap)) closeData := make([]*pbs.CloseConnectionRequestData, 0, len(closeMap)) for connId := range closeMap {