From cd4fe8f65ea3294a05e64d96b1f0d95460a3a29b Mon Sep 17 00:00:00 2001
From: Louis Garman <louisgarman@gmail.com>
Date: Wed, 23 Oct 2024 10:44:58 +0100
Subject: [PATCH 1/2] wip

---
 internal/configversion/tfe.go    |  5 +--
 internal/daemon/config.go        |  2 +-
 internal/daemon/daemon_test.go   |  2 +-
 internal/errors.go               | 46 ++++++++++++---------------
 internal/http/decode/decoders.go |  4 +--
 internal/notifications/config.go | 10 +++---
 internal/notifications/tfe.go    |  2 +-
 internal/run/tfe.go              |  8 +++--
 internal/state/tfe.go            |  5 ++-
 internal/state/version.go        |  4 +--
 internal/team/team.go            |  2 +-
 internal/tfeapi/errors.go        | 54 +++++++++++++++++++-------------
 internal/variable/tfe.go         | 29 +++++++----------
 internal/vcsprovider/tfe.go      |  8 ++---
 internal/workspace/tfe.go        |  7 ++---
 internal/workspace/workspace.go  | 14 +++------
 16 files changed, 95 insertions(+), 107 deletions(-)

diff --git a/internal/configversion/tfe.go b/internal/configversion/tfe.go
index 8a25d66d6..4e6f0c493 100644
--- a/internal/configversion/tfe.go
+++ b/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
diff --git a/internal/daemon/config.go b/internal/daemon/config.go
index 9d68c65e9..0934255f7 100644
--- a/internal/daemon/config.go
+++ b/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
diff --git a/internal/daemon/daemon_test.go b/internal/daemon/daemon_test.go
index c6b1b7673..395732e1e 100644
--- a/internal/daemon/daemon_test.go
+++ b/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))
 }
diff --git a/internal/errors.go b/internal/errors.go
index 12323ada8..e9775c4ef 100644
--- a/internal/errors.go
+++ b/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 _, i := range append(moreTargets, target) {
+		if errors.Is(err, i) {
+			return true
+		}
+	}
+	return false
 }
diff --git a/internal/http/decode/decoders.go b/internal/http/decode/decoders.go
index 3be1fcdaf..92c0d1cb8 100644
--- a/internal/http/decode/decoders.go
+++ b/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
 	}
diff --git a/internal/notifications/config.go b/internal/notifications/config.go
index 2476a0669..03fe7d671 100644
--- a/internal/notifications/config.go
+++ b/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")
diff --git a/internal/notifications/tfe.go b/internal/notifications/tfe.go
index 32771f6ee..26d810aeb 100644
--- a/internal/notifications/tfe.go
+++ b/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
 	}
 
diff --git a/internal/run/tfe.go b/internal/run/tfe.go
index 70c1b54d9..e7e2604ff 100644
--- a/internal/run/tfe.go
+++ b/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
 	}
 
diff --git a/internal/state/tfe.go b/internal/state/tfe.go
index 3f53181ea..483a36b7b 100644
--- a/internal/state/tfe.go
+++ b/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,7 +77,7 @@ 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.
@@ -86,7 +85,7 @@ func (a *tfe) createVersion(w http.ResponseWriter, r *http.Request) {
 	// 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
 	}
 
diff --git a/internal/state/version.go b/internal/state/version.go
index 681dc7b39..fb02093a8 100644
--- a/internal/state/version.go
+++ b/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)
diff --git a/internal/team/team.go b/internal/team/team.go
index 8f36492d9..7279efa17 100644
--- a/internal/team/team.go
+++ b/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 {
diff --git a/internal/tfeapi/errors.go b/internal/tfeapi/errors.go
index 6e2869ee2..6540266db 100644
--- a/internal/tfeapi/errors.go
+++ b/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)
 }
diff --git a/internal/variable/tfe.go b/internal/variable/tfe.go
index 06e8a4401..2395e664d 100644
--- a/internal/variable/tfe.go
+++ b/internal/variable/tfe.go
@@ -4,7 +4,6 @@ import (
 	"errors"
 	"net/http"
 
-	"github.com/leg100/otf/internal"
 	"github.com/leg100/otf/internal/tfeapi"
 	"github.com/leg100/otf/internal/tfeapi/types"
 
@@ -487,22 +486,16 @@ 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)
+	maxErrors := []error{
+		ErrVariableDescriptionMaxExceeded,
+		ErrVariableKeyMaxExceeded,
+		ErrVariableValueMaxExceeded,
+	}
+	for _, maxError := range maxErrors {
+		if errors.Is(err, maxError) {
+			tfeapi.Error(w, err, tfeapi.WithStatus(http.StatusUnprocessableEntity))
+			return
+		}
 	}
+	tfeapi.Error(w, err)
 }
diff --git a/internal/vcsprovider/tfe.go b/internal/vcsprovider/tfe.go
index 0f613733b..6d2830a44 100644
--- a/internal/vcsprovider/tfe.go
+++ b/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
 	}
 
diff --git a/internal/workspace/tfe.go b/internal/workspace/tfe.go
index ae9cd09ba..3e089e9a3 100644
--- a/internal/workspace/tfe.go
+++ b/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)
 		}
diff --git a/internal/workspace/workspace.go b/internal/workspace/workspace.go
index 27f858542..3ba48b47e 100644
--- a/internal/workspace/workspace.go
+++ b/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,

From 3cc876c1a7eef1803da37e21d8147738b0b4fa39 Mon Sep 17 00:00:00 2001
From: Louis Garman <louisgarman@gmail.com>
Date: Wed, 23 Oct 2024 10:48:38 +0100
Subject: [PATCH 2/2] wip

---
 internal/errors.go       |  4 ++--
 internal/variable/tfe.go | 13 +++++--------
 2 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/internal/errors.go b/internal/errors.go
index e9775c4ef..7b0b4b0d3 100644
--- a/internal/errors.go
+++ b/internal/errors.go
@@ -76,8 +76,8 @@ func (e *ErrMissingParameter) Error() string {
 // 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 _, i := range append(moreTargets, target) {
-		if errors.Is(err, i) {
+	for _, t := range append(moreTargets, target) {
+		if errors.Is(err, t) {
 			return true
 		}
 	}
diff --git a/internal/variable/tfe.go b/internal/variable/tfe.go
index 2395e664d..b069506e2 100644
--- a/internal/variable/tfe.go
+++ b/internal/variable/tfe.go
@@ -1,9 +1,9 @@
 package variable
 
 import (
-	"errors"
 	"net/http"
 
+	"github.com/leg100/otf/internal"
 	"github.com/leg100/otf/internal/tfeapi"
 	"github.com/leg100/otf/internal/tfeapi/types"
 
@@ -486,16 +486,13 @@ func (a *tfe) convertVariable(from *Variable, scrubSensitiveValue bool) *types.V
 }
 
 func variableError(w http.ResponseWriter, err error) {
-	maxErrors := []error{
+	if internal.ErrorIs(err,
 		ErrVariableDescriptionMaxExceeded,
 		ErrVariableKeyMaxExceeded,
 		ErrVariableValueMaxExceeded,
-	}
-	for _, maxError := range maxErrors {
-		if errors.Is(err, maxError) {
-			tfeapi.Error(w, err, tfeapi.WithStatus(http.StatusUnprocessableEntity))
-			return
-		}
+	) {
+		tfeapi.Error(w, err, tfeapi.WithStatus(http.StatusUnprocessableEntity))
+		return
 	}
 	tfeapi.Error(w, err)
 }