Don't return nil, nil for API service methods. Add tests for updating w/ wrong version. (#396)

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)

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)

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)

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)

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)

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)

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

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)

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)

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.

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) {

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) {

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.

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) {

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.

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) {

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.

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) {

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)
 }

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 {