fix: report 409 when cancel or force cancel not allowed

internal/configversion/tfe.go

@@ -153,10 +153,7 @@ func (a *tfe) uploadConfigurationVersion() http.HandlerFunc {
 		if _, err := io.Copy(buf, r.Body); err != nil {
 			maxBytesError := &http.MaxBytesError{}
 			if errors.As(err, &maxBytesError) {
-				tfeapi.Error(w, &internal.HTTPError{
-					Code:    422,
-					Message: fmt.Sprintf("config exceeds maximum size (%d bytes)", a.maxConfigSize),
-				})
+				tfeapi.Error(w, err, tfeapi.WithStatus(http.StatusUnprocessableEntity))
 				// Terraform CLI only informs the user that an HTTP 422 response
 				// was received, and they aren't informed that their config
 				// exceeds the max size. To help them diagnose the cause, the

internal/daemon/config.go

@@ -66,7 +66,7 @@ func ApplyDefaults(cfg *Config) {
 
 func (cfg *Config) Valid() error {
 	if cfg.Secret == nil {
-		return &internal.MissingParameterError{Parameter: "secret"}
+		return &internal.ErrMissingParameter{Parameter: "secret"}
 	}
 	if len(cfg.Secret) != 16 {
 		return ErrInvalidSecretLength

internal/daemon/daemon_test.go

@@ -11,7 +11,7 @@ import (
 )
 
 func TestDaemon_MissingSecretError(t *testing.T) {
-	var missing *internal.MissingParameterError
+	var missing *internal.ErrMissingParameter
 	_, err := New(context.Background(), logr.Discard(), Config{})
 	require.True(t, errors.As(err, &missing))
 }

internal/errors.go

@@ -54,38 +54,32 @@ var (
 	ErrInvalidRepo = errors.New("repository path is invalid")
 )
 
-type (
-	HTTPError struct {
-		Code    int
-		Message string
-	}
-
-	// MissingParameterError occurs when the caller has failed to provide a
-	// required parameter
-	MissingParameterError struct {
-		Parameter string
-	}
-
-	// ForeignKeyError occurs when there is a foreign key violation.
-	ForeignKeyError struct {
-		*pgconn.PgError
-	}
-
-	InvalidParameterError string
-)
+// ForeignKeyError occurs when there is a foreign key violation.
+type ForeignKeyError struct {
+	*pgconn.PgError
+}
 
-func (e InvalidParameterError) Error() string {
-	return string(e)
+func (e *ForeignKeyError) Error() string {
+	return e.Detail
 }
 
-func (e *HTTPError) Error() string {
-	return e.Message
+// ErrMissingParameter occurs when the user has failed to provide a
+// required parameter
+type ErrMissingParameter struct {
+	Parameter string
 }
 
-func (e *MissingParameterError) Error() string {
+func (e *ErrMissingParameter) Error() string {
 	return fmt.Sprintf("required parameter missing: %s", e.Parameter)
 }
 
-func (e *ForeignKeyError) Error() string {
-	return e.Detail
+// ErrorIs is a modification to the upstream errors.Is, allowing multiple
+// targets to be checked.
+func ErrorIs(err error, target error, moreTargets ...error) bool {
+	for _, t := range append(moreTargets, target) {
+		if errors.Is(err, t) {
+			return true
+		}
+	}
+	return false
 }

internal/http/decode/decoders.go

@@ -89,14 +89,14 @@ func Param(name string, r *http.Request) (string, error) {
 	if v, ok := mux.Vars(r)[name]; ok {
 		return v, nil
 	}
-	return "", &internal.MissingParameterError{Parameter: name}
+	return "", &internal.ErrMissingParameter{Parameter: name}
 }
 
 func decode(dst interface{}, src map[string][]string) error {
 	if err := decoder.Decode(dst, src); err != nil {
 		var emptyField schema.EmptyFieldError
 		if errors.As(err, &emptyField) {
-			return &internal.MissingParameterError{Parameter: emptyField.Key}
+			return &internal.ErrMissingParameter{Parameter: emptyField.Key}
 		}
 		return err
 	}

internal/notifications/config.go

@@ -3,12 +3,10 @@ package notifications
 import (
 	"errors"
 	"fmt"
-	"net/url"
-	"time"
-
 	"log/slog"
-
+	"net/url"
 	"slices"
+	"time"
 
 	"github.com/leg100/otf/internal"
 	"github.com/leg100/otf/internal/run"
@@ -120,10 +118,10 @@ func NewConfig(workspaceID string, opts CreateConfigOptions) (*Config, error) {
 		return nil, err
 	}
 	if opts.Enabled == nil {
-		return nil, &internal.MissingParameterError{Parameter: "enabled"}
+		return nil, &internal.ErrMissingParameter{Parameter: "enabled"}
 	}
 	if opts.Name == nil {
-		return nil, &internal.MissingParameterError{Parameter: "name"}
+		return nil, &internal.ErrMissingParameter{Parameter: "name"}
 	}
 	if *opts.Name == "" {
 		return nil, fmt.Errorf("name cannot be an empty string")

internal/notifications/tfe.go

@@ -38,7 +38,7 @@ func (a *tfe) createNotification(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	if params.DestinationType == nil {
-		tfeapi.Error(w, &internal.MissingParameterError{Parameter: "destination_type"})
+		tfeapi.Error(w, &internal.ErrMissingParameter{Parameter: "destination_type"})
 		return
 	}
 

internal/run/tfe.go

@@ -58,7 +58,7 @@ func (a *tfe) createRun(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	if params.Workspace == nil {
-		tfeapi.Error(w, &internal.MissingParameterError{Parameter: "workspace"})
+		tfeapi.Error(w, &internal.ErrMissingParameter{Parameter: "workspace"})
 		return
 	}
 
@@ -215,7 +215,11 @@ func (a *tfe) cancelRun(w http.ResponseWriter, r *http.Request) {
 	}
 
 	if err = a.Cancel(r.Context(), id); err != nil {
-		tfeapi.Error(w, err)
+		if internal.ErrorIs(err, ErrRunCancelNotAllowed, ErrRunForceCancelNotAllowed) {
+			tfeapi.Error(w, err, tfeapi.WithStatus(http.StatusConflict))
+		} else {
+			tfeapi.Error(w, err)
+		}
 		return
 	}
 

internal/state/tfe.go

@@ -60,7 +60,6 @@ func (a *tfe) addHandlers(r *mux.Router) {
 	// https://github.com/leg100/otf/issues/626) OTF accepts the upload but does
 	// nothing with it.
 	signed.HandleFunc("/state-versions/{id}/upload/json", func(http.ResponseWriter, *http.Request) {})
-
 }
 
 func (a *tfe) createVersion(w http.ResponseWriter, r *http.Request) {
@@ -78,15 +77,15 @@ func (a *tfe) createVersion(w http.ResponseWriter, r *http.Request) {
 
 	// required options
 	if opts.Serial == nil {
-		tfeapi.Error(w, &internal.MissingParameterError{Parameter: "serial"})
+		tfeapi.Error(w, &internal.ErrMissingParameter{Parameter: "serial"})
 		return
 	}
 	// TFE docs say md5 is a required option yet the state itself is optional.
 	// OTF follows this behavour, mandating the md5 parameter, but it is only
 	// actually used if the state is also provided at creation-time. If the
 	// state is only later uploaded, the md5 is not used.
 	if opts.MD5 == nil {
-		tfeapi.Error(w, &internal.MissingParameterError{Parameter: "md5"})
+		tfeapi.Error(w, &internal.ErrMissingParameter{Parameter: "md5"})
 		return
 	}
 

internal/state/version.go

@@ -81,10 +81,10 @@ type (
 // new create a new state version
 func (f *factory) new(ctx context.Context, opts CreateStateVersionOptions) (*Version, error) {
 	if opts.WorkspaceID == nil {
-		return nil, &internal.MissingParameterError{Parameter: "workspace_id"}
+		return nil, &internal.ErrMissingParameter{Parameter: "workspace_id"}
 	}
 	if opts.Serial == nil {
-		return nil, &internal.MissingParameterError{Parameter: "serial"}
+		return nil, &internal.ErrMissingParameter{Parameter: "serial"}
 	}
 	// Serial should be greater than or equal to current serial
 	current, err := f.db.getCurrentVersion(ctx, *opts.WorkspaceID)

internal/team/team.go

@@ -78,7 +78,7 @@ type (
 func newTeam(organization string, opts CreateTeamOptions) (*Team, error) {
 	// required parameters
 	if opts.Name == nil {
-		return nil, &internal.MissingParameterError{Parameter: "name"}
+		return nil, &internal.ErrMissingParameter{Parameter: "name"}
 	}
 	// default parameters
 	if opts.Visibility == nil {

internal/tfeapi/errors.go

@@ -16,38 +16,48 @@ var codes = map[error]int{
 	internal.ErrConflict:                http.StatusConflict,
 }
 
+// lookupHTTPCode maps an OTF domain error to a http status code
 func lookupHTTPCode(err error) int {
-	if v, ok := codes[err]; ok {
-		return v
+	for otfError, httpError := range codes {
+		if errors.Is(err, otfError) {
+			return httpError
+		}
 	}
 	return http.StatusInternalServerError
 }
 
-// Error writes an HTTP response with a JSON-API encoded error.
-func Error(w http.ResponseWriter, err error) {
-	var (
-		httpError *internal.HTTPError
-		missing   *internal.MissingParameterError
-		code      int
-	)
-	// If error is type internal.HTTPError then extract its status code
-	if errors.As(err, &httpError) {
-		code = httpError.Code
-	} else if errors.As(err, &missing) {
-		// report missing parameter errors as a 422
-		code = http.StatusUnprocessableEntity
-	} else {
-		code = lookupHTTPCode(err)
+type ErrorOption func(*jsonapi.Error)
+
+func WithStatus(httpStatusCode int) ErrorOption {
+	return func(err *jsonapi.Error) {
+		err.Status = &httpStatusCode
 	}
-	b, err := jsonapi.Marshal(&jsonapi.Error{
-		Status: &code,
-		Title:  http.StatusText(code),
+}
+
+// Error writes an HTTP response with a JSON-API encoded error.
+func Error(w http.ResponseWriter, err error, opts ...ErrorOption) {
+	jsonapiError := &jsonapi.Error{
 		Detail: err.Error(),
-	})
+	}
+	for _, fn := range opts {
+		fn(jsonapiError)
+	}
+	if jsonapiError.Status == nil {
+		var missing *internal.ErrMissingParameter
+		if errors.As(err, &missing) {
+			// report missing parameter errors as a 422
+			jsonapiError.Status = internal.Int(http.StatusUnprocessableEntity)
+		} else {
+			jsonapiError.Status = internal.Int(lookupHTTPCode(err))
+		}
+	}
+	jsonapiError.Title = http.StatusText(*jsonapiError.Status)
+
+	b, err := jsonapi.Marshal(jsonapiError)
 	if err != nil {
 		panic(err)
 	}
 	w.Header().Set("Content-type", mediaType)
-	w.WriteHeader(code)
+	w.WriteHeader(*jsonapiError.Status)
 	w.Write(b)
 }

internal/variable/tfe.go

@@ -1,7 +1,6 @@
 package variable
 
 import (
-	"errors"
 	"net/http"
 
 	"github.com/leg100/otf/internal"
@@ -487,22 +486,13 @@ func (a *tfe) convertVariable(from *Variable, scrubSensitiveValue bool) *types.V
 }
 
 func variableError(w http.ResponseWriter, err error) {
-	var isUnprocessableError bool
-	if errors.Is(err, ErrVariableDescriptionMaxExceeded) {
-		isUnprocessableError = true
-	}
-	if errors.Is(err, ErrVariableKeyMaxExceeded) {
-		isUnprocessableError = true
-	}
-	if errors.Is(err, ErrVariableValueMaxExceeded) {
-		isUnprocessableError = true
-	}
-	if isUnprocessableError {
-		tfeapi.Error(w, &internal.HTTPError{
-			Message: err.Error(),
-			Code:    http.StatusUnprocessableEntity,
-		})
-	} else {
-		tfeapi.Error(w, err)
+	if internal.ErrorIs(err,
+		ErrVariableDescriptionMaxExceeded,
+		ErrVariableKeyMaxExceeded,
+		ErrVariableValueMaxExceeded,
+	) {
+		tfeapi.Error(w, err, tfeapi.WithStatus(http.StatusUnprocessableEntity))
+		return
 	}
+	tfeapi.Error(w, err)
 }

internal/vcsprovider/tfe.go

@@ -48,19 +48,19 @@ func (a *tfe) createOAuthClient(w http.ResponseWriter, r *http.Request) {
 
 	// required parameters
 	if params.OAuthToken == nil {
-		tfeapi.Error(w, &internal.MissingParameterError{Parameter: "oauth-token-string"})
+		tfeapi.Error(w, &internal.ErrMissingParameter{Parameter: "oauth-token-string"})
 		return
 	}
 	if params.ServiceProvider == nil {
-		tfeapi.Error(w, &internal.MissingParameterError{Parameter: "service-provider"})
+		tfeapi.Error(w, &internal.ErrMissingParameter{Parameter: "service-provider"})
 		return
 	}
 	if params.APIURL == nil {
-		tfeapi.Error(w, &internal.MissingParameterError{Parameter: "api-url"})
+		tfeapi.Error(w, &internal.ErrMissingParameter{Parameter: "api-url"})
 		return
 	}
 	if params.HTTPURL == nil {
-		tfeapi.Error(w, &internal.MissingParameterError{Parameter: "http-url"})
+		tfeapi.Error(w, &internal.ErrMissingParameter{Parameter: "http-url"})
 		return
 	}
 

internal/workspace/tfe.go

@@ -282,11 +282,8 @@ func (a *tfe) unlock(w http.ResponseWriter, r *http.Request, force bool) {
 
 	ws, err := a.Unlock(r.Context(), id, nil, force)
 	if err != nil {
-		if errors.Is(err, ErrWorkspaceAlreadyUnlocked) || errors.Is(err, ErrWorkspaceLockedByRun) {
-			tfeapi.Error(w, &internal.HTTPError{
-				Code:    http.StatusConflict,
-				Message: err.Error(),
-			})
+		if internal.ErrorIs(err, ErrWorkspaceAlreadyUnlocked, ErrWorkspaceLockedByRun) {
+			tfeapi.Error(w, err, tfeapi.WithStatus(http.StatusConflict))
 		} else {
 			tfeapi.Error(w, err)
 		}

internal/workspace/workspace.go

@@ -4,12 +4,10 @@ package workspace
 import (
 	"errors"
 	"fmt"
-	"regexp"
-	"time"
-
 	"log/slog"
-
+	"regexp"
 	"slices"
+	"time"
 
 	"github.com/gobwas/glob"
 	"github.com/leg100/otf/internal"
@@ -27,9 +25,7 @@ const (
 	MinTerraformVersion     = "1.2.0"
 )
 
-var (
-	apiTestTerraformVersions = []string{"0.10.0", "0.11.0", "0.11.1"}
-)
+var apiTestTerraformVersions = []string{"0.10.0", "0.11.0", "0.11.1"}
 
 type (
 	// Workspace is a terraform workspace.
@@ -436,10 +432,10 @@ func (ws *Workspace) Update(opts UpdateOptions) (*bool, error) {
 func (ws *Workspace) addConnection(opts *ConnectOptions) error {
 	// must specify both repo and vcs provider ID
 	if opts.RepoPath == nil {
-		return &internal.MissingParameterError{Parameter: "repo_path"}
+		return &internal.ErrMissingParameter{Parameter: "repo_path"}
 	}
 	if opts.VCSProviderID == nil {
-		return &internal.MissingParameterError{Parameter: "vcs_provider_id"}
+		return &internal.ErrMissingParameter{Parameter: "vcs_provider_id"}
 	}
 	ws.Connection = &Connection{
 		Repo:          *opts.RepoPath,