From 199915c85d6c31f20b9d0101a8df99cf4bb288c1 Mon Sep 17 00:00:00 2001
From: Mike Mason <mimason@equinix.com>
Date: Thu, 14 Dec 2023 00:22:28 +0000
Subject: [PATCH] add missing rollback comments and ensure tests are checking
 results properly

Signed-off-by: Mike Mason <mimason@equinix.com>
---
 internal/query/relations.go      | 28 ++++++++++++++++++----------
 internal/query/relations_test.go | 16 ++++++++++++----
 2 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/internal/query/relations.go b/internal/query/relations.go
index 927856249..508de160c 100644
--- a/internal/query/relations.go
+++ b/internal/query/relations.go
@@ -330,27 +330,27 @@ func actionsDiff(oldActions, newActions []string) ([]string, []string) {
 		return nil, nil
 	}
 
-	old := make(map[string]bool, len(oldActions))
-	new := make(map[string]bool, len(newActions))
+	old := make(map[string]struct{}, len(oldActions))
+	new := make(map[string]struct{}, len(newActions))
 
 	var add, rem []string
 
 	for _, action := range oldActions {
-		old[action] = true
+		old[action] = struct{}{}
 	}
 
 	for _, action := range newActions {
-		new[action] = true
+		new[action] = struct{}{}
 
 		// If the new action is not in the old actions, then we need to add the action.
-		if !old[action] {
+		if _, ok := old[action]; !ok {
 			add = append(add, action)
 		}
 	}
 
 	for _, action := range oldActions {
 		// If the old action is not in the new actions, then we need to remove it.
-		if !new[action] {
+		if _, ok := new[action]; !ok {
 			rem = append(rem, action)
 		}
 	}
@@ -425,7 +425,10 @@ func (e *engine) UpdateRole(ctx context.Context, actor, roleResource types.Resou
 
 		logRollbackErr(e.logger, e.store.RollbackContext(dbCtx))
 
-		// TODO: add spicedb rollback logic.
+		// At this point, spicedb changes have already been applied.
+		// Attempting to rollback could result in failures that could result in the same situation.
+		//
+		// TODO: add spicedb rollback logic along with rollback failure scenarios.
 
 		return types.Role{}, err
 	}
@@ -846,13 +849,13 @@ func (e *engine) ListRoles(ctx context.Context, resource types.Resource) ([]type
 
 	out := relationshipsToRoles(relationships)
 
-	rolesByID := make(map[string]storage.Role, len(dbRoles))
+	rolesByID := make(map[gidx.PrefixedID]storage.Role, len(dbRoles))
 	for _, role := range dbRoles {
-		rolesByID[role.ID.String()] = role
+		rolesByID[role.ID] = role
 	}
 
 	for i, role := range out {
-		if dbRole, ok := rolesByID[role.ID.String()]; ok {
+		if dbRole, ok := rolesByID[role.ID]; ok {
 			role.Name = dbRole.Name
 			role.CreatedBy = dbRole.CreatedBy
 			role.UpdatedBy = dbRole.UpdatedBy
@@ -1075,6 +1078,11 @@ func (e *engine) DeleteRole(ctx context.Context, roleResource types.Resource) er
 
 		logRollbackErr(e.logger, e.store.RollbackContext(dbCtx))
 
+		// At this point, spicedb changes have already been applied.
+		// Attempting to rollback could result in failures that could result in the same situation.
+		//
+		// TODO: add spicedb rollback logic along with rollback failure scenarios.
+
 		return err
 	}
 
diff --git a/internal/query/relations_test.go b/internal/query/relations_test.go
index 5b2bde791..afa88eaa6 100644
--- a/internal/query/relations_test.go
+++ b/internal/query/relations_test.go
@@ -214,6 +214,8 @@ func TestRoleUpdate(t *testing.T) {
 	require.NoError(t, err)
 	actorRes, err := e.NewResourceFromID(gidx.MustNewID("idntusr"))
 	require.NoError(t, err)
+	actorUpdateRes, err := e.NewResourceFromID(gidx.MustNewID("idntusr"))
+	require.NoError(t, err)
 
 	role, err := e.CreateRole(ctx, actorRes, tenRes, "test", []string{"loadbalancer_get"})
 	require.NoError(t, err)
@@ -226,15 +228,21 @@ func TestRoleUpdate(t *testing.T) {
 			Name:  "UpdateMissingRole",
 			Input: gidx.MustNewID(RolePrefix),
 			CheckFn: func(ctx context.Context, t *testing.T, res testingx.TestResult[types.Role]) {
-				assert.Error(t, res.Err)
+				require.Error(t, res.Err)
+				assert.ErrorIs(t, res.Err, ErrRoleNotFound)
 			},
 		},
 		{
 			Name:  "UpdateSuccess",
 			Input: role.ID,
 			CheckFn: func(ctx context.Context, t *testing.T, res testingx.TestResult[types.Role]) {
-				assert.NoError(t, res.Err)
-				require.Equal(t, "test2", res.Success.Name)
+				require.NoError(t, res.Err)
+				assert.Equal(t, "test2", res.Success.Name)
+				assert.Equal(t, role.Actions, res.Success.Actions)
+				assert.Equal(t, role.CreatedBy, res.Success.CreatedBy)
+				assert.Equal(t, actorUpdateRes.ID, res.Success.UpdatedBy)
+				assert.Equal(t, role.CreatedAt, res.Success.CreatedAt)
+				assert.NotEqual(t, role.UpdatedAt, res.Success.UpdatedAt)
 			},
 		},
 	}
@@ -247,7 +255,7 @@ func TestRoleUpdate(t *testing.T) {
 			}
 		}
 
-		_, err = e.UpdateRole(ctx, actorRes, roleResource, "test2", nil)
+		_, err = e.UpdateRole(ctx, actorUpdateRes, roleResource, "test2", nil)
 		if err != nil {
 			return testingx.TestResult[types.Role]{
 				Err: err,