Apply suggestions from code review

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

internal/api/rolebindings.go

@@ -14,18 +14,6 @@ import (
 	"go.infratographer.com/permissions-api/internal/types"
 )
 
-func resourceToSubject(subjects []types.RoleBindingSubject) []roleBindingSubject {
-	resp := make([]roleBindingSubject, len(subjects))
-	for i, subj := range subjects {
-		resp[i] = roleBindingSubject{
-			ID:   subj.SubjectResource.ID,
-			Type: subj.SubjectResource.Type,
-		}
-	}
-
-	return resp
-}
-
 func (r *Router) roleBindingCreate(c echo.Context) error {
 	resourceIDStr := c.Param("id")
 
@@ -72,17 +60,16 @@ func (r *Router) roleBindingCreate(c echo.Context) error {
 		return r.errorResponse("error creating role resource", err)
 	}
 
-	subjects := make([]types.RoleBindingSubject, len(body.Subjects))
+	subjects := make([]types.RoleBindingSubject, len(body.SubjectIDs))
 
-	for i, s := range body.Subjects {
-		subj, err := r.engine.NewResourceFromID(s.ID)
+	for i, sid := range body.SubjectIDs {
+		subj, err := r.engine.NewResourceFromID(sid)
 		if err != nil {
 			return r.errorResponse("error creating subject resource", err)
 		}
 
 		subjects[i] = types.RoleBindingSubject{
 			SubjectResource: subj,
-			Condition:       nil,
 		}
 	}
 
@@ -96,11 +83,8 @@ func (r *Router) roleBindingCreate(c echo.Context) error {
 		roleBindingResponse{
 			ID:         rb.ID,
 			ResourceID: rb.ResourceID,
-			Subjects:   resourceToSubject(rb.Subjects),
-			Role: roleBindingResponseRole{
-				ID:   rb.Role.ID,
-				Name: rb.Role.Name,
-			},
+			SubjectIDs: rb.SubjectIDs,
+			RoleID:     rb.RoleID,
 
 			CreatedBy: rb.CreatedBy,
 			UpdatedBy: rb.UpdatedBy,
@@ -151,11 +135,8 @@ func (r *Router) roleBindingsList(c echo.Context) error {
 		resp.Data[i] = roleBindingResponse{
 			ID:         rb.ID,
 			ResourceID: rb.ResourceID,
-			Subjects:   resourceToSubject(rb.Subjects),
-			Role: roleBindingResponseRole{
-				ID:   rb.Role.ID,
-				Name: rb.Role.Name,
-			},
+			SubjectIDs: rb.SubjectIDs,
+			RoleID:     rb.RoleID,
 
 			CreatedBy: rb.CreatedBy,
 			UpdatedBy: rb.UpdatedBy,
@@ -167,7 +148,7 @@ func (r *Router) roleBindingsList(c echo.Context) error {
 	return c.JSON(http.StatusOK, resp)
 }
 
-func (r *Router) roleBindingsDelete(c echo.Context) error {
+func (r *Router) roleBindingDelete(c echo.Context) error {
 	rbID := c.Param("rb_id")
 
 	ctx, span := tracer.Start(
@@ -259,11 +240,8 @@ func (r *Router) roleBindingGet(c echo.Context) error {
 		roleBindingResponse{
 			ID:         rb.ID,
 			ResourceID: rb.ResourceID,
-			Subjects:   resourceToSubject(rb.Subjects),
-			Role: roleBindingResponseRole{
-				ID:   rb.Role.ID,
-				Name: rb.Role.Name,
-			},
+			SubjectIDs: rb.SubjectIDs,
+			RoleID:     rb.RoleID,
 
 			CreatedBy: rb.CreatedBy,
 			UpdatedBy: rb.UpdatedBy,
@@ -318,17 +296,16 @@ func (r *Router) roleBindingUpdate(c echo.Context) error {
 		return r.errorResponse(err.Error(), ErrParsingRequestBody)
 	}
 
-	subjects := make([]types.RoleBindingSubject, len(body.Subjects))
+	subjects := make([]types.RoleBindingSubject, len(body.SubjectIDs))
 
-	for i, s := range body.Subjects {
-		subj, err := r.engine.NewResourceFromID(s.ID)
+	for i, sid := range body.SubjectIDs {
+		subj, err := r.engine.NewResourceFromID(sid)
 		if err != nil {
 			return r.errorResponse("error creating subject resource", err)
 		}
 
 		subjects[i] = types.RoleBindingSubject{
 			SubjectResource: subj,
-			Condition:       nil,
 		}
 	}
 
@@ -342,12 +319,8 @@ func (r *Router) roleBindingUpdate(c echo.Context) error {
 		roleBindingResponse{
 			ID:         rb.ID,
 			ResourceID: rb.ResourceID,
-			Subjects:   resourceToSubject(rb.Subjects),
-
-			Role: roleBindingResponseRole{
-				ID:   rb.Role.ID,
-				Name: rb.Role.Name,
-			},
+			SubjectIDs: rb.SubjectIDs,
+			RoleID:     rb.RoleID,
 
 			CreatedBy: rb.CreatedBy,
 			UpdatedBy: rb.UpdatedBy,

internal/api/router.go

@@ -89,7 +89,7 @@ func (r *Router) Routes(rg *echo.Group) {
 		v2.GET("/resources/:id/role-bindings", r.roleBindingsList)
 		v2.POST("/resources/:id/role-bindings", r.roleBindingCreate)
 		v2.GET("/role-bindings/:rb_id", r.roleBindingGet)
-		v2.DELETE("/role-bindings/:rb_id", r.roleBindingsDelete)
+		v2.DELETE("/role-bindings/:rb_id", r.roleBindingDelete)
 		v2.PATCH("/role-bindings/:rb_id", r.roleBindingUpdate)
 
 		v2.GET("/actions", r.listActions)

internal/api/types.go

@@ -83,30 +83,20 @@ type listRolesV2Role struct {
 
 // RoleBindings
 
-type roleBindingResponseRole struct {
-	ID   gidx.PrefixedID `json:"id"`
-	Name string          `json:"name"`
-}
-
-type roleBindingSubject struct {
-	ID   gidx.PrefixedID `json:"id" binding:"required"`
-	Type string          `json:"type,omitempty"`
-}
-
 type roleBindingRequest struct {
-	RoleID   string               `json:"role_id" binding:"required"`
-	Subjects []roleBindingSubject `json:"subjects" binding:"required"`
+	RoleID     string            `json:"role_id" binding:"required"`
+	SubjectIDs []gidx.PrefixedID `json:"subject_ids" binding:"required"`
 }
 
 type rolebindingUpdateRequest struct {
-	Subjects []roleBindingSubject `json:"subjects" binding:"required"`
+	SubjectIDs []gidx.PrefixedID `json:"subject_ids" binding:"required"`
 }
 
 type roleBindingResponse struct {
-	ID         gidx.PrefixedID         `json:"id"`
-	ResourceID gidx.PrefixedID         `json:"resource_id"`
-	Role       roleBindingResponseRole `json:"role"`
-	Subjects   []roleBindingSubject    `json:"subjects"`
+	ID         gidx.PrefixedID   `json:"id"`
+	ResourceID gidx.PrefixedID   `json:"resource_id"`
+	RoleID     gidx.PrefixedID   `json:"role_id"`
+	SubjectIDs []gidx.PrefixedID `json:"subject_ids"`
 
 	CreatedBy gidx.PrefixedID `json:"created_by"`
 	UpdatedBy gidx.PrefixedID `json:"updated_by"`

internal/query/errors.go

@@ -44,6 +44,9 @@ var (
 	// in the policy
 	ErrRoleV2ResourceNotDefined = errors.New("role v2 resource not defined")
 
+	// ErrDeleteRoleInUse represents an error when a role is in use and cannot be deleted
+	ErrDeleteRoleInUse = fmt.Errorf("%w: role is in use", ErrInvalidArgument)
+
 	// ErrRoleAlreadyExists represents an error when a role already exists
 	ErrRoleAlreadyExists = fmt.Errorf("%w: role already exists", ErrInvalidArgument)
 
@@ -53,4 +56,12 @@ var (
 	// ErrResourceDoesNotSupportRoleBindingV2 represents an error when a role binding
 	// request attempts to use a resource that does not support role binding v2
 	ErrResourceDoesNotSupportRoleBindingV2 = fmt.Errorf("%w: resource does not support role binding v2", ErrInvalidArgument)
+
+	// ErrCreateRoleBindingWithNoSubjects represents an error when a role
+	// binding is created with no subjects
+	ErrCreateRoleBindingWithNoSubjects = fmt.Errorf("%w: role binding must have at least one subject", ErrInvalidArgument)
+
+	// ErrRoleBindingHasNoRelationships represents an internal error when a
+	// role binding has no relationships
+	ErrRoleBindingHasNoRelationships = errors.New("role binding has no relationships")
 )

internal/query/relations.go

@@ -1234,6 +1234,9 @@ func (e *engine) NewResourceFromIDString(id string) (types.Resource, error) {
 
 // rollbackUpdates is a helper function that rolls back a list of
 // relationship updates on spiceDB.
+// Note that we can only rollback spiceDB writes that are serialized,
+// that is, there should only be one write (`client.WriteRelationships` or
+// `e.ApplyUpdates`) over the course of the transaction.
 func (e *engine) rollbackUpdates(ctx context.Context, updates []*pb.RelationshipUpdate) error {
 	updatesLen := len(updates)
 	rollbacks := make([]*pb.RelationshipUpdate, 0, updatesLen)