From 0bee55aa193b39b86d7f6ce614f4365a46d79fa4 Mon Sep 17 00:00:00 2001 From: Hugo Date: Thu, 19 Jan 2023 21:12:04 +0000 Subject: [PATCH] fix(target): Correct accounting of updated rows when deleting address (#2799) If an update call was made to a target that removed an address, for a target that already did not have an address, the incorrect number of rows updated was being reported. During the update the target's version was correctly being updated, resulting in one row being updated. However then the delete call would result in zero rows being deleted, since there was no address associated with the target, and this would replace the value for the returned rowsUpdated. This resulted in the target service thinking that no update happened, and it would report an error. --- internal/target/repository.go | 8 +++-- internal/tests/api/targets/target_test.go | 39 +++++++++++++++++++++++ 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/internal/target/repository.go b/internal/target/repository.go index d929b4a425..cad16b4b0f 100644 --- a/internal/target/repository.go +++ b/internal/target/repository.go @@ -615,13 +615,17 @@ func (r *Repository) UpdateTarget(ctx context.Context, target Target, version ui var err error address = allocTargetAddress() address.TargetAddress.TargetId = t.GetPublicId() - rowsUpdated, err = w.Delete(ctx, address, db.WithOplog(oplogWrapper, address.oplog(oplog.OpType_OP_TYPE_DELETE))) + rowsDeleted, err := w.Delete(ctx, address, db.WithOplog(oplogWrapper, address.oplog(oplog.OpType_OP_TYPE_DELETE))) if err != nil { return errors.Wrap(ctx, err, op, errors.WithMsg("unable to delete target address")) } - if rowsUpdated > 1 { + if rowsDeleted > 1 { return errors.New(ctx, errors.MultipleRecords, op, "more than 1 target resource would have been deleted") } + // If the only update was deleting an address, consider this as one "row" being updated. + if rowsUpdated == 0 && rowsDeleted == 1 { + rowsUpdated = 1 + } default: address, err = fetchAddress(ctx, read, t.GetPublicId()) if err != nil && !errors.IsNotFoundError(err) { diff --git a/internal/tests/api/targets/target_test.go b/internal/tests/api/targets/target_test.go index f96734e6e5..90685d7a31 100644 --- a/internal/tests/api/targets/target_test.go +++ b/internal/tests/api/targets/target_test.go @@ -546,6 +546,45 @@ func TestCreateTarget_DirectlyAttachedAddress(t *testing.T) { } } +func TestUpdateTarget_DeleteAddress(t *testing.T) { + tc := controller.NewTestController(t, nil) + t.Cleanup(tc.Shutdown) + + client := tc.Client() + at := tc.Token() + client.SetToken(at.Token) + _, proj := iam.TestScopes(t, tc.IamRepo(), iam.WithUserId(at.UserId)) + tClient := targets.NewClient(client) + + assert, require := assert.New(t), require.New(t) + + // Create Target with address + addr := "127.0.0.1" + createResp, err := tClient.Create(tc.Context(), "tcp", proj.PublicId, + targets.WithName("test_target_addr_delete"), targets.WithAddress(addr), targets.WithTcpTargetDefaultPort(22)) + require.NoError(err) + require.NotNil(createResp) + assert.Equal(addr, createResp.GetItem().Address) + + // Update to delete address (set to null) + updateResp, err := tClient.Update(tc.Context(), createResp.Item.Id, createResp.Item.Version, targets.DefaultAddress()) + require.NoError(err) + require.NotNil(updateResp) + assert.Empty(updateResp.GetItem().Address) + + // Do it again with same version, should error + _, err = tClient.Update(tc.Context(), createResp.Item.Id, createResp.Item.Version, targets.DefaultAddress()) + require.Error(err) + + // Do it again with the correct version, ensure this is not an error. + secondUpdateResp, err := tClient.Update(tc.Context(), createResp.Item.Id, updateResp.Item.Version, targets.DefaultAddress()) + require.NoError(err) + require.NotNil(secondUpdateResp) + assert.Empty(secondUpdateResp.GetItem().Address) + + assert.NotEqual(updateResp.Item.Version, secondUpdateResp.Item.Version) +} + func comparableSlice(in []*targets.Target) []targets.Target { var filtered []targets.Target for _, i := range in {