diff --git a/coderd/dynamicparameters/error.go b/coderd/dynamicparameters/error.go new file mode 100644 index 0000000000..3af270569a --- /dev/null +++ b/coderd/dynamicparameters/error.go @@ -0,0 +1,129 @@ +package dynamicparameters + +import ( + "fmt" + "net/http" + "sort" + + "github.com/hashicorp/hcl/v2" + + "github.com/coder/coder/v2/codersdk" +) + +func ParameterValidationError(diags hcl.Diagnostics) *DiagnosticError { + return &DiagnosticError{ + Message: "Unable to validate parameters", + Diagnostics: diags, + KeyedDiagnostics: make(map[string]hcl.Diagnostics), + } +} + +func TagValidationError(diags hcl.Diagnostics) *DiagnosticError { + return &DiagnosticError{ + Message: "Failed to parse workspace tags", + Diagnostics: diags, + KeyedDiagnostics: make(map[string]hcl.Diagnostics), + } +} + +type DiagnosticError struct { + // Message is the human-readable message that will be returned to the user. + Message string + // Diagnostics are top level diagnostics that will be returned as "Detail" in the response. + Diagnostics hcl.Diagnostics + // KeyedDiagnostics translate to Validation errors in the response. A key could + // be a parameter name, or a tag name. This allows diagnostics to be more closely + // associated with a specific index/parameter/tag. + KeyedDiagnostics map[string]hcl.Diagnostics +} + +// Error is a pretty bad format for these errors. Try to avoid using this. +func (e *DiagnosticError) Error() string { + var diags hcl.Diagnostics + diags = diags.Extend(e.Diagnostics) + for _, d := range e.KeyedDiagnostics { + diags = diags.Extend(d) + } + + return diags.Error() +} + +func (e *DiagnosticError) HasError() bool { + if e.Diagnostics.HasErrors() { + return true + } + + for _, diags := range e.KeyedDiagnostics { + if diags.HasErrors() { + return true + } + } + return false +} + +func (e *DiagnosticError) Append(key string, diag *hcl.Diagnostic) { + e.Extend(key, hcl.Diagnostics{diag}) +} + +func (e *DiagnosticError) Extend(key string, diag hcl.Diagnostics) { + if e.KeyedDiagnostics == nil { + e.KeyedDiagnostics = make(map[string]hcl.Diagnostics) + } + if _, ok := e.KeyedDiagnostics[key]; !ok { + e.KeyedDiagnostics[key] = hcl.Diagnostics{} + } + e.KeyedDiagnostics[key] = e.KeyedDiagnostics[key].Extend(diag) +} + +func (e *DiagnosticError) Response() (int, codersdk.Response) { + resp := codersdk.Response{ + Message: e.Message, + Validations: nil, + } + + // Sort the parameter names so that the order is consistent. + sortedNames := make([]string, 0, len(e.KeyedDiagnostics)) + for name := range e.KeyedDiagnostics { + sortedNames = append(sortedNames, name) + } + sort.Strings(sortedNames) + + for _, name := range sortedNames { + diag := e.KeyedDiagnostics[name] + resp.Validations = append(resp.Validations, codersdk.ValidationError{ + Field: name, + Detail: DiagnosticsErrorString(diag), + }) + } + + if e.Diagnostics.HasErrors() { + resp.Detail = DiagnosticsErrorString(e.Diagnostics) + } + + return http.StatusBadRequest, resp +} + +func DiagnosticErrorString(d *hcl.Diagnostic) string { + return fmt.Sprintf("%s; %s", d.Summary, d.Detail) +} + +func DiagnosticsErrorString(d hcl.Diagnostics) string { + count := len(d) + switch { + case count == 0: + return "no diagnostics" + case count == 1: + return DiagnosticErrorString(d[0]) + default: + for _, d := range d { + // Render the first error diag. + // If there are warnings, do not priority them over errors. + if d.Severity == hcl.DiagError { + return fmt.Sprintf("%s, and %d other diagnostic(s)", DiagnosticErrorString(d), count-1) + } + } + + // All warnings? ok... + return fmt.Sprintf("%s, and %d other diagnostic(s)", DiagnosticErrorString(d[0]), count-1) + } +} diff --git a/coderd/dynamicparameters/resolver.go b/coderd/dynamicparameters/resolver.go index 3cb9c59f28..7007fccc9f 100644 --- a/coderd/dynamicparameters/resolver.go +++ b/coderd/dynamicparameters/resolver.go @@ -26,45 +26,6 @@ type parameterValue struct { Source parameterValueSource } -type ResolverError struct { - Diagnostics hcl.Diagnostics - Parameter map[string]hcl.Diagnostics -} - -// Error is a pretty bad format for these errors. Try to avoid using this. -func (e *ResolverError) Error() string { - var diags hcl.Diagnostics - diags = diags.Extend(e.Diagnostics) - for _, d := range e.Parameter { - diags = diags.Extend(d) - } - - return diags.Error() -} - -func (e *ResolverError) HasError() bool { - if e.Diagnostics.HasErrors() { - return true - } - - for _, diags := range e.Parameter { - if diags.HasErrors() { - return true - } - } - return false -} - -func (e *ResolverError) Extend(parameterName string, diag hcl.Diagnostics) { - if e.Parameter == nil { - e.Parameter = make(map[string]hcl.Diagnostics) - } - if _, ok := e.Parameter[parameterName]; !ok { - e.Parameter[parameterName] = hcl.Diagnostics{} - } - e.Parameter[parameterName] = e.Parameter[parameterName].Extend(diag) -} - //nolint:revive // firstbuild is a control flag to turn on immutable validation func ResolveParameters( ctx context.Context, @@ -112,10 +73,7 @@ func ResolveParameters( // always be valid. If there is a case where this is not true, then this has to // be changed to allow the build to continue with a different set of values. - return nil, &ResolverError{ - Diagnostics: diags, - Parameter: nil, - } + return nil, ParameterValidationError(diags) } // The user's input now needs to be validated against the parameters. @@ -155,16 +113,13 @@ func ResolveParameters( // are fatal. Additional validation for immutability has to be done manually. output, diags = renderer.Render(ctx, ownerID, values.ValuesMap()) if diags.HasErrors() { - return nil, &ResolverError{ - Diagnostics: diags, - Parameter: nil, - } + return nil, ParameterValidationError(diags) } // parameterNames is going to be used to remove any excess values that were left // around without a parameter. parameterNames := make(map[string]struct{}, len(output.Parameters)) - parameterError := &ResolverError{} + parameterError := ParameterValidationError(nil) for _, parameter := range output.Parameters { parameterNames[parameter.Name] = struct{}{} diff --git a/coderd/httpapi/httperror/responserror.go b/coderd/httpapi/httperror/responserror.go new file mode 100644 index 0000000000..be219f538b --- /dev/null +++ b/coderd/httpapi/httperror/responserror.go @@ -0,0 +1,19 @@ +package httperror + +import ( + "errors" + + "github.com/coder/coder/v2/codersdk" +) + +type Responder interface { + Response() (int, codersdk.Response) +} + +func IsResponder(err error) (Responder, bool) { + var responseErr Responder + if errors.As(err, &responseErr) { + return responseErr, true + } + return nil, false +} diff --git a/coderd/httpapi/httperror/wsbuild.go b/coderd/httpapi/httperror/wsbuild.go index 17436b55d5..24c6985813 100644 --- a/coderd/httpapi/httperror/wsbuild.go +++ b/coderd/httpapi/httperror/wsbuild.go @@ -2,60 +2,17 @@ package httperror import ( "context" - "errors" - "fmt" "net/http" - "sort" - "github.com/hashicorp/hcl/v2" - - "github.com/coder/coder/v2/coderd/dynamicparameters" "github.com/coder/coder/v2/coderd/httpapi" - "github.com/coder/coder/v2/coderd/wsbuilder" "github.com/coder/coder/v2/codersdk" ) func WriteWorkspaceBuildError(ctx context.Context, rw http.ResponseWriter, err error) { - var buildErr wsbuilder.BuildError - if errors.As(err, &buildErr) { - if httpapi.IsUnauthorizedError(err) { - buildErr.Status = http.StatusForbidden - } + if responseErr, ok := IsResponder(err); ok { + code, resp := responseErr.Response() - httpapi.Write(ctx, rw, buildErr.Status, codersdk.Response{ - Message: buildErr.Message, - Detail: buildErr.Error(), - }) - return - } - - var parameterErr *dynamicparameters.ResolverError - if errors.As(err, ¶meterErr) { - resp := codersdk.Response{ - Message: "Unable to validate parameters", - Validations: nil, - } - - // Sort the parameter names so that the order is consistent. - sortedNames := make([]string, 0, len(parameterErr.Parameter)) - for name := range parameterErr.Parameter { - sortedNames = append(sortedNames, name) - } - sort.Strings(sortedNames) - - for _, name := range sortedNames { - diag := parameterErr.Parameter[name] - resp.Validations = append(resp.Validations, codersdk.ValidationError{ - Field: name, - Detail: DiagnosticsErrorString(diag), - }) - } - - if parameterErr.Diagnostics.HasErrors() { - resp.Detail = DiagnosticsErrorString(parameterErr.Diagnostics) - } - - httpapi.Write(ctx, rw, http.StatusBadRequest, resp) + httpapi.Write(ctx, rw, code, resp) return } @@ -64,28 +21,3 @@ func WriteWorkspaceBuildError(ctx context.Context, rw http.ResponseWriter, err e Detail: err.Error(), }) } - -func DiagnosticError(d *hcl.Diagnostic) string { - return fmt.Sprintf("%s; %s", d.Summary, d.Detail) -} - -func DiagnosticsErrorString(d hcl.Diagnostics) string { - count := len(d) - switch { - case count == 0: - return "no diagnostics" - case count == 1: - return DiagnosticError(d[0]) - default: - for _, d := range d { - // Render the first error diag. - // If there are warnings, do not priority them over errors. - if d.Severity == hcl.DiagError { - return fmt.Sprintf("%s, and %d other diagnostic(s)", DiagnosticError(d), count-1) - } - } - - // All warnings? ok... - return fmt.Sprintf("%s, and %d other diagnostic(s)", DiagnosticError(d[0]), count-1) - } -} diff --git a/coderd/wsbuilder/wsbuilder.go b/coderd/wsbuilder/wsbuilder.go index bd36776144..ec0ef4df16 100644 --- a/coderd/wsbuilder/wsbuilder.go +++ b/coderd/wsbuilder/wsbuilder.go @@ -240,6 +240,9 @@ type BuildError struct { } func (e BuildError) Error() string { + if e.Wrapped == nil { + return e.Message + } return e.Wrapped.Error() } @@ -247,6 +250,13 @@ func (e BuildError) Unwrap() error { return e.Wrapped } +func (e BuildError) Response() (int, codersdk.Response) { + return e.Status, codersdk.Response{ + Message: e.Message, + Detail: e.Error(), + } +} + // Build computes and inserts a new workspace build into the database. If authFunc is provided, it also performs // authorization preflight checks. func (b *Builder) Build( diff --git a/coderd/wsbuilder/wsbuilder_test.go b/coderd/wsbuilder/wsbuilder_test.go index f07e2f99f7..41ea3fe2c9 100644 --- a/coderd/wsbuilder/wsbuilder_test.go +++ b/coderd/wsbuilder/wsbuilder_test.go @@ -12,6 +12,7 @@ import ( "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/files" + "github.com/coder/coder/v2/coderd/httpapi/httperror" "github.com/coder/coder/v2/provisionersdk" "github.com/google/uuid" @@ -1000,6 +1001,23 @@ func TestWorkspaceBuildDeleteOrphan(t *testing.T) { }) } +func TestWsbuildError(t *testing.T) { + t.Parallel() + + const msg = "test error" + var buildErr error = wsbuilder.BuildError{ + Status: http.StatusBadRequest, + Message: msg, + } + + respErr, ok := httperror.IsResponder(buildErr) + require.True(t, ok, "should be a Coder SDK error") + + code, resp := respErr.Response() + require.Equal(t, http.StatusBadRequest, code) + require.Equal(t, msg, resp.Message) +} + type txExpect func(mTx *dbmock.MockStore) func expectDB(t *testing.T, opts ...txExpect) *dbmock.MockStore {