implement second round of review suggestions

Signed-off-by: Mike Mason <[email protected]>

internal/api/roles.go

@@ -1,10 +1,12 @@
 package api
 
 import (
+	"errors"
 	"net/http"
 	"time"
 
 	"github.com/labstack/echo/v4"
+	"go.infratographer.com/permissions-api/internal/query"
 	"go.infratographer.com/x/gidx"
 	"go.opentelemetry.io/otel/attribute"
 	"go.opentelemetry.io/otel/trace"
@@ -60,7 +62,8 @@ func (r *Router) roleCreate(c echo.Context) error {
 		Name:       role.Name,
 		Actions:    role.Actions,
 		ResourceID: role.ResourceID,
-		Creator:    role.CreatorID,
+		CreatedBy:  role.CreatedBy,
+		UpdatedBy:  role.UpdatedBy,
 		CreatedAt:  role.CreatedAt.Format(time.RFC3339),
 		UpdatedAt:  role.UpdatedAt.Format(time.RFC3339),
 	}
@@ -100,7 +103,12 @@ func (r *Router) roleUpdate(c echo.Context) error {
 	// check on the role resource.
 	resource, err := r.engine.GetRoleResource(ctx, roleResource)
 	if err != nil {
-		return echo.NewHTTPError(http.StatusBadRequest, "error getting resource").SetInternal(err)
+		if errors.Is(err, query.ErrRoleNotFound) {
+			return echo.NewHTTPError(http.StatusNotFound, "resource not found").SetInternal(err)
+		}
+
+		return echo.NewHTTPError(http.StatusInternalServerError, "error getting resource").SetInternal(err)
+
 	}
 
 	if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleUpdate, resource); err != nil {
@@ -117,7 +125,8 @@ func (r *Router) roleUpdate(c echo.Context) error {
 		Name:       role.Name,
 		Actions:    role.Actions,
 		ResourceID: role.ResourceID,
-		Creator:    role.CreatorID,
+		CreatedBy:  role.CreatedBy,
+		UpdatedBy:  role.UpdatedBy,
 		CreatedAt:  role.CreatedAt.Format(time.RFC3339),
 		UpdatedAt:  role.UpdatedAt.Format(time.RFC3339),
 	}
@@ -169,7 +178,8 @@ func (r *Router) roleGet(c echo.Context) error {
 		Name:       role.Name,
 		Actions:    role.Actions,
 		ResourceID: role.ResourceID,
-		Creator:    role.CreatorID,
+		CreatedBy:  role.CreatedBy,
+		UpdatedBy:  role.UpdatedBy,
 		CreatedAt:  role.CreatedAt.Format(time.RFC3339),
 		UpdatedAt:  role.UpdatedAt.Format(time.RFC3339),
 	}
@@ -216,7 +226,8 @@ func (r *Router) rolesList(c echo.Context) error {
 			ID:        role.ID,
 			Name:      role.Name,
 			Actions:   role.Actions,
-			Creator:   role.CreatorID,
+			CreatedBy: role.CreatedBy,
+			UpdatedBy: role.UpdatedBy,
 			CreatedAt: role.CreatedAt.Format(time.RFC3339),
 			UpdatedAt: role.UpdatedAt.Format(time.RFC3339),
 		}

internal/api/types.go

@@ -20,7 +20,8 @@ type roleResponse struct {
 	Actions []string        `json:"actions"`
 
 	ResourceID gidx.PrefixedID `json:"resource_id,omitempty"`
-	Creator    gidx.PrefixedID `json:"creator"`
+	CreatedBy  gidx.PrefixedID `json:"created_by"`
+	UpdatedBy  gidx.PrefixedID `json:"updated_by"`
 	CreatedAt  string          `json:"created_at"`
 	UpdatedAt  string          `json:"updated_at"`
 }

internal/query/mock/mock.go

@@ -54,17 +54,22 @@ func (e *Engine) CreateRole(ctx context.Context, actor, res types.Resource, name
 		ID:        gidx.MustNewID(query.ApplicationPrefix),
 		Name:      name,
 		Actions:   outActions,
-		CreatorID: actor.ID,
+		CreatedBy: actor.ID,
+		UpdatedBy: actor.ID,
 		CreatedAt: time.Now(),
 		UpdatedAt: time.Now(),
 	}
 
 	return role, nil
 }
 
-// UpdateRole returns nothing but satisfies the Engine interface.
+// UpdateRole returns the provided mock results.
 func (e *Engine) UpdateRole(ctx context.Context, actor, roleResource types.Resource, newName string, newActions []string) (types.Role, error) {
-	return types.Role{}, nil
+	args := e.Called(actor, roleResource, newName, newActions)
+
+	retRole := args.Get(0).(types.Role)
+
+	return retRole, args.Error(1)
 }
 
 // GetRole returns nothing but satisfies the Engine interface.

internal/query/relations.go

@@ -306,10 +306,16 @@ func (e *engine) CreateRole(ctx context.Context, actor, res types.Resource, role
 
 		logRollbackErr(e.logger, e.store.RollbackContext(dbCtx))
 
+		// No rollback of spicedb relations are done here.
+		// This does result in dangling unused entries in spicedb,
+		// however there are no assignments to these newly created
+		// and now discarded roles and so they won't be used.
+
 		return types.Role{}, err
 	}
 
-	role.CreatorID = dbRole.CreatorID
+	role.CreatedBy = dbRole.CreatedBy
+	role.UpdatedBy = dbRole.UpdatedBy
 	role.ResourceID = dbRole.ResourceID
 	role.CreatedAt = dbRole.CreatedAt
 	role.UpdatedAt = dbRole.UpdatedAt
@@ -360,48 +366,35 @@ func (e *engine) UpdateRole(ctx context.Context, actor, roleResource types.Resou
 
 	defer span.End()
 
-	role, err := e.GetRole(ctx, roleResource)
+	dbCtx, err := e.store.BeginContext(ctx)
+	if err != nil {
+		return types.Role{}, err
+	}
+
+	role, err := e.GetRole(dbCtx, roleResource)
 	if err != nil {
 		return types.Role{}, err
 	}
 
 	newName = strings.TrimSpace(newName)
 
-	// If the name is the same, then clear the changed name.
-	if role.Name == newName {
-		newName = ""
+	if newName == "" {
+		newName = role.Name
 	}
 
 	addActions, remActions := actionsDiff(role.Actions, newActions)
 
 	// If no changes, return existing role with no changes.
-	if newName == "" && len(addActions) == 0 && len(remActions) == 0 {
+	if newName == role.Name && len(addActions) == 0 && len(remActions) == 0 {
 		return role, nil
 	}
 
-	resourceID := role.ResourceID
-
-	// If the resource id is not found in the permissions database, then we must locate it in the spicedb database.
-	if resourceID == gidx.NullPrefixedID {
-		resource, err := e.GetRoleResource(ctx, roleResource)
-		if err != nil {
-			return types.Role{}, fmt.Errorf("failed to locate roles associated resource: %s: %w", roleResource.ID.String(), err)
-		}
-
-		resourceID = resource.ID
-	}
-
-	resource, err := e.NewResourceFromID(resourceID)
+	resource, err := e.NewResourceFromID(role.ResourceID)
 	if err != nil {
 		return types.Role{}, err
 	}
 
-	dbCtx, err := e.store.BeginContext(ctx)
-	if err != nil {
-		return types.Role{}, err
-	}
-
-	dbRole, err := e.store.UpdateRole(dbCtx, role.ID, newName)
+	dbRole, err := e.store.UpdateRole(dbCtx, actor.ID, role.ID, newName)
 	if err != nil {
 		logRollbackErr(e.logger, e.store.RollbackContext(dbCtx))
 
@@ -432,11 +425,14 @@ func (e *engine) UpdateRole(ctx context.Context, actor, roleResource types.Resou
 
 		logRollbackErr(e.logger, e.store.RollbackContext(dbCtx))
 
+		// TODO: add spicedb rollback logic.
+
 		return types.Role{}, err
 	}
 
 	role.Name = dbRole.Name
-	role.CreatorID = dbRole.CreatorID
+	role.CreatedBy = dbRole.CreatedBy
+	role.UpdatedBy = dbRole.UpdatedBy
 	role.ResourceID = dbRole.ResourceID
 	role.CreatedAt = dbRole.CreatedAt
 	role.UpdatedAt = dbRole.UpdatedAt
@@ -858,7 +854,8 @@ func (e *engine) ListRoles(ctx context.Context, resource types.Resource) ([]type
 	for i, role := range out {
 		if dbRole, ok := rolesByID[role.ID.String()]; ok {
 			role.Name = dbRole.Name
-			role.CreatorID = dbRole.CreatorID
+			role.CreatedBy = dbRole.CreatedBy
+			role.UpdatedBy = dbRole.UpdatedBy
 			role.CreatedAt = dbRole.CreatedAt
 			role.UpdatedAt = dbRole.UpdatedAt
 
@@ -956,7 +953,8 @@ func (e *engine) GetRole(ctx context.Context, roleResource types.Resource) (type
 			Actions: actions,
 
 			ResourceID: dbRole.ResourceID,
-			CreatorID:  dbRole.CreatorID,
+			CreatedBy:  dbRole.CreatedBy,
+			UpdatedBy:  dbRole.UpdatedBy,
 			CreatedAt:  dbRole.CreatedAt,
 			UpdatedAt:  dbRole.UpdatedAt,
 		}, nil

internal/storage/context.go

@@ -39,6 +39,19 @@ func getContextTx(ctx context.Context) (*sql.Tx, error) {
 	}
 }
 
+func getContextDBQuery(ctx context.Context, def DBQuery) (DBQuery, error) {
+	tx, err := getContextTx(ctx)
+
+	switch err {
+	case nil:
+		return tx, nil
+	case ErrorMissingContextTx:
+		return def, nil
+	default:
+		return nil, err
+	}
+}
+
 func commitContextTx(ctx context.Context) error {
 	tx, err := getContextTx(ctx)
 	if err != nil {

internal/storage/errors.go

@@ -27,17 +27,30 @@ var (
 	ErrorInvalidContextTx = errors.New("invalid type for transaction context")
 )
 
+const (
+	pqIndexRolesPrimaryKey     = "roles_pkey"
+	pqIndexRolesResourceIDName = "roles_resource_id_name"
+)
+
+// pqIsRoleAlreadyExistsError checks that the provided error is a postgres error.
+// If so, checks if postgres threw a unique_violation error on the roles primary key index.
+// If postgres has raised a unique violation error on this index it means a record already exists
+// with a matching primary key (role id).
 func pqIsRoleAlreadyExistsError(err error) bool {
 	if pqErr, ok := err.(*pq.Error); ok {
-		return pqErr.Code.Name() == "unique_violation" && pqErr.Constraint == "roles_pkey"
+		return pqErr.Code.Name() == "unique_violation" && pqErr.Constraint == pqIndexRolesPrimaryKey
 	}
 
 	return false
 }
 
+// pqIsRoleNameTakenError checks that the provided error is a postgres error.
+// If so, checks if postgres threw a unique_violation error on the roles resource id name index.
+// If postgres has raised a unique violation error on this index it means a record already exists
+// with the same resource id and role name combination.
 func pqIsRoleNameTakenError(err error) bool {
 	if pqErr, ok := err.(*pq.Error); ok {
-		return pqErr.Code.Name() == "unique_violation" && pqErr.Constraint == "roles_resource_id_name"
+		return pqErr.Code.Name() == "unique_violation" && pqErr.Constraint == pqIndexRolesResourceIDName
 	}
 
 	return false

internal/storage/migrations/20231122000000_initial_schema.sql

@@ -4,13 +4,16 @@ CREATE TABLE "roles" (
   "id" character varying NOT NULL,
   "name" character varying(64) NOT NULL,
   "resource_id" character varying NOT NULL,
-  "creator_id" character varying NOT NULL,
+  "created_by" character varying NOT NULL,
+  "updated_by" character varying NOT NULL,
   "created_at" timestamptz NOT NULL,
   "updated_at" timestamptz NOT NULL,
   PRIMARY KEY ("id")
 );
--- create index "roles_creator_id" to table: "roles"
-CREATE INDEX "roles_creator_id" ON "roles" ("creator_id");
+-- create index "roles_created_by" to table: "roles"
+CREATE INDEX "roles_created_by" ON "roles" ("created_by");
+-- create index "roles_created_by" to table: "roles"
+CREATE INDEX "roles_updated_by" ON "roles" ("updated_by");
 -- create index "roles_created_at" to table: "roles"
 CREATE INDEX "roles_created_at" ON "roles" ("created_at");
 -- create index "roles_updated_at" to table: "roles"
@@ -24,7 +27,9 @@ DROP INDEX "roles_resource_id_name";
 DROP INDEX "roles_updated_at";
 -- reverse: create index "roles_created_at" to table: "roles"
 DROP INDEX "roles_created_at";
--- reverse: create index "roles_creator_id" to table: "roles"
-DROP INDEX "roles_creator_id";
+-- reverse: create index "roles_updated_by" to table: "roles"
+DROP INDEX "roles_updated_by";
+-- reverse: create index "roles_created_by" to table: "roles"
+DROP INDEX "roles_created_by";
 -- reverse: create "roles" table
 DROP TABLE "roles";