From d85ed3910707c757c3246d27a52acab01b133691 Mon Sep 17 00:00:00 2001
From: Bailin He <15058035+bailinhe@users.noreply.github.com>
Date: Mon, 6 May 2024 14:50:11 -0400
Subject: [PATCH] Apply suggestions from code review

Co-authored-by: John Schaeffer <john@schaeffer.io>
Signed-off-by: Bailin He <15058035+bailinhe@users.noreply.github.com>
---
 internal/query/relations.go     |  4 +---
 internal/query/roles_v2.go      | 42 ++++++++++++++++-----------------
 internal/storage/rolebinding.go |  2 +-
 3 files changed, 23 insertions(+), 25 deletions(-)

diff --git a/internal/query/relations.go b/internal/query/relations.go
index e969ddeb..2acbd1c9 100644
--- a/internal/query/relations.go
+++ b/internal/query/relations.go
@@ -1252,9 +1252,7 @@ func (e *engine) rollbackUpdates(ctx context.Context, updates []*pb.Relationship
 		var op pb.RelationshipUpdate_Operation
 
 		switch u.Operation {
-		case pb.RelationshipUpdate_OPERATION_CREATE:
-			fallthrough
-		case pb.RelationshipUpdate_OPERATION_TOUCH:
+		case pb.RelationshipUpdate_OPERATION_CREATE, pb.RelationshipUpdate_OPERATION_TOUCH:
 			op = pb.RelationshipUpdate_OPERATION_DELETE
 		case pb.RelationshipUpdate_OPERATION_DELETE:
 			op = pb.RelationshipUpdate_OPERATION_TOUCH
diff --git a/internal/query/roles_v2.go b/internal/query/roles_v2.go
index 78a58be8..e13f4282 100644
--- a/internal/query/roles_v2.go
+++ b/internal/query/roles_v2.go
@@ -352,6 +352,26 @@ func (e *engine) DeleteRoleV2(ctx context.Context, roleResource types.Resource)
 	ctx, span := e.tracer.Start(ctx, "engine.DeleteRoleV2")
 	defer span.End()
 
+	dbCtx, err := e.store.BeginContext(ctx)
+	if err != nil {
+		span.RecordError(err)
+		span.SetStatus(codes.Error, err.Error())
+
+		return err
+	}
+
+	err = e.store.LockRoleForUpdate(dbCtx, roleResource.ID)
+	if err != nil {
+		sErr := fmt.Errorf("failed to lock role: %s: %w", roleResource.ID, err)
+
+		span.RecordError(sErr)
+		span.SetStatus(codes.Error, sErr.Error())
+
+		logRollbackErr(e.logger, e.store.RollbackContext(dbCtx))
+
+		return err
+	}
+
 	// find all the bindings for the role
 	findBindingsFilter := &pb.RelationshipFilter{
 		ResourceType:     e.namespaced(e.rbac.RoleBindingResource.Name),
@@ -362,7 +382,7 @@ func (e *engine) DeleteRoleV2(ctx context.Context, roleResource types.Resource)
 		},
 	}
 
-	bindings, err := e.readRelationships(ctx, findBindingsFilter)
+	bindings, err := e.readRelationships(dbCtx, findBindingsFilter)
 	if err != nil {
 		span.RecordError(err)
 		span.SetStatus(codes.Error, err.Error())
@@ -379,26 +399,6 @@ func (e *engine) DeleteRoleV2(ctx context.Context, roleResource types.Resource)
 		return err
 	}
 
-	dbCtx, err := e.store.BeginContext(ctx)
-	if err != nil {
-		span.RecordError(err)
-		span.SetStatus(codes.Error, err.Error())
-
-		return err
-	}
-
-	err = e.store.LockRoleForUpdate(dbCtx, roleResource.ID)
-	if err != nil {
-		sErr := fmt.Errorf("failed to lock role: %s: %w", roleResource.ID, err)
-
-		span.RecordError(sErr)
-		span.SetStatus(codes.Error, sErr.Error())
-
-		logRollbackErr(e.logger, e.store.RollbackContext(dbCtx))
-
-		return err
-	}
-
 	dbRole, err := e.store.GetRoleByID(dbCtx, roleResource.ID)
 	if err != nil {
 		span.RecordError(err)
diff --git a/internal/storage/rolebinding.go b/internal/storage/rolebinding.go
index 1fbe5dc6..6b85181a 100644
--- a/internal/storage/rolebinding.go
+++ b/internal/storage/rolebinding.go
@@ -85,7 +85,7 @@ func (e *engine) ListResourceRoleBindings(ctx context.Context, resourceID gidx.P
 
 	rows, err := db.QueryContext(ctx, `
 		SELECT id, resource_id, created_by, updated_by, created_at, updated_at
-		FROM rolebindings WHERE resource_id = $1
+		FROM rolebindings WHERE resource_id = $1 ORDER BY created_at ASC
 		`, resourceID.String(),
 	)
 	if err != nil {