Apply suggestions from code review

Co-authored-by: John Schaeffer <[email protected]>
Signed-off-by: Bailin He <[email protected]>

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

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)

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 {