Skip to content

Commit 21556db

Browse files
EmyrkEmyrk
committed
chore: improve dynamic parameter validation errors
1 parent c1d05a0 commit 21556db

File tree

7 files changed

+159
-54
lines changed

7 files changed

+159
-54
lines changed

‎coderd/dynamicparameters/resolver.go‎

Lines changed: 64 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,45 @@ type parameterValue struct {
2626
Source parameterValueSource
2727
}
2828

29+
type ResolverError struct {
30+
Diagnostics hcl.Diagnostics
31+
Parameter map[string]hcl.Diagnostics
32+
}
33+
34+
// Error is a pretty bad format for these errors. Try to avoid using this.
35+
func (e *ResolverError) Error() string {
36+
var diags hcl.Diagnostics
37+
diags = diags.Extend(e.Diagnostics)
38+
for _, d := range e.Parameter {
39+
diags = diags.Extend(d)
40+
}
41+
42+
return diags.Error()
43+
}
44+
45+
func (e *ResolverError) HasError() bool {
46+
if e.Diagnostics.HasErrors() {
47+
return true
48+
}
49+
50+
for _, diags := range e.Parameter {
51+
if diags.HasErrors() {
52+
return true
53+
}
54+
}
55+
return false
56+
}
57+
58+
func (e *ResolverError) Extend(parameterName string, diag hcl.Diagnostics) {
59+
if e.Parameter == nil {
60+
e.Parameter = make(map[string]hcl.Diagnostics)
61+
}
62+
if _, ok := e.Parameter[parameterName]; !ok {
63+
e.Parameter[parameterName] = hcl.Diagnostics{}
64+
}
65+
e.Parameter[parameterName] = e.Parameter[parameterName].Extend(diag)
66+
}
67+
2968
//nolint:revive // firstbuild is a control flag to turn on immutable validation
3069
func ResolveParameters(
3170
ctx context.Context,
@@ -73,7 +112,10 @@ func ResolveParameters(
73112
// always be valid. If there is a case where this is not true, then this has to
74113
// be changed to allow the build to continue with a different set of values.
75114

76-
return nil, diags
115+
return nil, &ResolverError{
116+
Diagnostics: diags,
117+
Parameter: nil,
118+
}
77119
}
78120

79121
// The user's input now needs to be validated against the parameters.
@@ -113,12 +155,16 @@ func ResolveParameters(
113155
// are fatal. Additional validation for immutability has to be done manually.
114156
output, diags = renderer.Render(ctx, ownerID, values.ValuesMap())
115157
if diags.HasErrors() {
116-
return nil, diags
158+
return nil, &ResolverError{
159+
Diagnostics: diags,
160+
Parameter: nil,
161+
}
117162
}
118163

119164
// parameterNames is going to be used to remove any excess values that were left
120165
// around without a parameter.
121166
parameterNames := make(map[string]struct{}, len(output.Parameters))
167+
parameterError := &ResolverError{}
122168
for _, parameter := range output.Parameters {
123169
parameterNames[parameter.Name] = struct{}{}
124170

@@ -132,20 +178,22 @@ func ResolveParameters(
132178
}
133179

134180
// An immutable parameter was changed, which is not allowed.
135-
// Add the failed diagnostic to the output.
136-
diags = diags.Append(&hcl.Diagnostic{
137-
Severity: hcl.DiagError,
138-
Summary: "Immutable parameter changed",
139-
Detail: fmt.Sprintf("Parameter %q is not mutable, so it can't be updated after creating a workspace.", parameter.Name),
140-
Subject: src,
181+
// Add a failed diagnostic to the output.
182+
parameterError.Extend(parameter.Name, hcl.Diagnostics{
183+
&hcl.Diagnostic{
184+
Severity: hcl.DiagError,
185+
Summary: "Immutable parameter changed",
186+
Detail: fmt.Sprintf("Parameter %q is not mutable, so it can't be updated after creating a workspace.", parameter.Name),
187+
Subject: src,
188+
},
141189
})
142190
}
143191
}
144192

145193
// TODO: Fix the `hcl.Diagnostics(...)` type casting. It should not be needed.
146194
if hcl.Diagnostics(parameter.Diagnostics).HasErrors() {
147-
// All validation errors are raised here.
148-
diags = diags.Extend(hcl.Diagnostics(parameter.Diagnostics))
195+
// All validation errors are raised here for each parameter.
196+
parameterError.Extend(parameter.Name, hcl.Diagnostics(parameter.Diagnostics))
149197
}
150198

151199
// If the parameter has a value, but it was not set explicitly by the user at any
@@ -174,8 +222,13 @@ func ResolveParameters(
174222
}
175223
}
176224

225+
if parameterError.HasError() {
226+
// If there are any errors, return them.
227+
return nil, parameterError
228+
}
229+
177230
// Return the values to be saved for the build.
178-
return values.ValuesMap(), diags
231+
return values.ValuesMap(), nil
179232
}
180233

181234
type parameterValueMap map[string]parameterValue

‎coderd/httpapi/httperror/doc.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
// Package httperror handles formatting and writing some sentinel errors returned
2+
// within coder to the API.
3+
// This package exists outside httpapi to avoid some cyclic dependencies
4+
package httperror

‎coderd/httpapi/httperror/wsbuild.go‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
package httperror
2+
3+
import (
4+
"context"
5+
"errors"
6+
"fmt"
7+
"net/http"
8+
9+
"github.com/hashicorp/hcl/v2"
10+
11+
"github.com/coder/coder/v2/coderd/dynamicparameters"
12+
"github.com/coder/coder/v2/coderd/httpapi"
13+
"github.com/coder/coder/v2/coderd/wsbuilder"
14+
"github.com/coder/coder/v2/codersdk"
15+
)
16+
17+
func WriteWorkspaceBuildError(ctx context.Context, rw http.ResponseWriter, err error) {
18+
var buildErr wsbuilder.BuildError
19+
if errors.As(err, &buildErr) {
20+
if httpapi.IsUnauthorizedError(err) {
21+
buildErr.Status = http.StatusForbidden
22+
}
23+
24+
httpapi.Write(ctx, rw, buildErr.Status, codersdk.Response{
25+
Message: buildErr.Message,
26+
Detail: buildErr.Error(),
27+
})
28+
return
29+
}
30+
31+
var parameterErr *dynamicparameters.ResolverError
32+
if errors.As(err, &parameterErr) {
33+
resp := codersdk.Response{
34+
Message: "Unable to validate parameters",
35+
Validations: nil,
36+
}
37+
38+
for name, diag := range parameterErr.Parameter {
39+
resp.Validations = append(resp.Validations, codersdk.ValidationError{
40+
Field: name,
41+
Detail: DiagnosticsErrorString(diag),
42+
})
43+
}
44+
45+
if parameterErr.Diagnostics.HasErrors() {
46+
resp.Detail = DiagnosticsErrorString(parameterErr.Diagnostics)
47+
}
48+
49+
httpapi.Write(ctx, rw, http.StatusBadRequest, resp)
50+
return
51+
}
52+
53+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
54+
Message: "Internal error creating workspace build.",
55+
Detail: err.Error(),
56+
})
57+
}
58+
59+
func DiagnosticError(d *hcl.Diagnostic) string {
60+
return fmt.Sprintf("%s; %s", d.Summary, d.Detail)
61+
}
62+
63+
func DiagnosticsErrorString(d hcl.Diagnostics) string {
64+
count := len(d)
65+
switch {
66+
case count == 0:
67+
return "no diagnostics"
68+
case count == 1:
69+
return DiagnosticError(d[0])
70+
default:
71+
for _, d := range d {
72+
// Render the first error diag.
73+
// If there are warnings, do not priority them over errors.
74+
if d.Severity == hcl.DiagError {
75+
return fmt.Sprintf("%s, and %d other diagnostic(s)", DiagnosticError(d), count-1)
76+
}
77+
}
78+
79+
// All warnings? ok...
80+
return fmt.Sprintf("%s, and %d other diagnostic(s)", DiagnosticError(d[0]), count-1)
81+
}
82+
}

‎coderd/workspacebuilds.go‎

Lines changed: 2 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import (
2727
"github.com/coder/coder/v2/coderd/database/dbtime"
2828
"github.com/coder/coder/v2/coderd/database/provisionerjobs"
2929
"github.com/coder/coder/v2/coderd/httpapi"
30+
"github.com/coder/coder/v2/coderd/httpapi/httperror"
3031
"github.com/coder/coder/v2/coderd/httpmw"
3132
"github.com/coder/coder/v2/coderd/notifications"
3233
"github.com/coder/coder/v2/coderd/provisionerdserver"
@@ -410,28 +411,8 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) {
410411
)
411412
return err
412413
}, nil)
413-
var buildErr wsbuilder.BuildError
414-
if xerrors.As(err, &buildErr) {
415-
var authErr dbauthz.NotAuthorizedError
416-
if xerrors.As(err, &authErr) {
417-
buildErr.Status = http.StatusForbidden
418-
}
419-
420-
if buildErr.Status == http.StatusInternalServerError {
421-
api.Logger.Error(ctx, "workspace build error", slog.Error(buildErr.Wrapped))
422-
}
423-
424-
httpapi.Write(ctx, rw, buildErr.Status, codersdk.Response{
425-
Message: buildErr.Message,
426-
Detail: buildErr.Error(),
427-
})
428-
return
429-
}
430414
if err != nil {
431-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
432-
Message: "Error posting new build",
433-
Detail: err.Error(),
434-
})
415+
httperror.WriteWorkspaceBuildError(ctx, rw, err)
435416
return
436417
}
437418

‎coderd/workspaces.go‎

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import (
2727
"github.com/coder/coder/v2/coderd/database/dbtime"
2828
"github.com/coder/coder/v2/coderd/database/provisionerjobs"
2929
"github.com/coder/coder/v2/coderd/httpapi"
30+
"github.com/coder/coder/v2/coderd/httpapi/httperror"
3031
"github.com/coder/coder/v2/coderd/httpmw"
3132
"github.com/coder/coder/v2/coderd/notifications"
3233
"github.com/coder/coder/v2/coderd/prebuilds"
@@ -732,21 +733,11 @@ func createWorkspace(
732733
)
733734
return err
734735
}, nil)
735-
var bldErr wsbuilder.BuildError
736-
if xerrors.As(err, &bldErr) {
737-
httpapi.Write(ctx, rw, bldErr.Status, codersdk.Response{
738-
Message: bldErr.Message,
739-
Detail: bldErr.Error(),
740-
})
741-
return
742-
}
743736
if err != nil {
744-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
745-
Message: "Internal error creating workspace.",
746-
Detail: err.Error(),
747-
})
737+
httperror.WriteWorkspaceBuildError(ctx, rw, err)
748738
return
749739
}
740+
750741
err = provisionerjobs.PostJob(api.Pubsub, *provisionerJob)
751742
if err != nil {
752743
// Client probably doesn't care about this error, so just log it.

‎coderd/wsbuilder/wsbuilder.go‎

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -760,20 +760,13 @@ func (b *Builder) getDynamicParameters() (names, values []string, err error) {
760760
return nil, nil, BuildError{http.StatusInternalServerError, "failed to check if first build", err}
761761
}
762762

763-
buildValues, diagnostics := dynamicparameters.ResolveParameters(b.ctx, b.workspace.OwnerID, render, firstBuild,
763+
buildValues, err := dynamicparameters.ResolveParameters(b.ctx, b.workspace.OwnerID, render, firstBuild,
764764
lastBuildParameters,
765765
b.richParameterValues,
766766
presetParameterValues)
767767

768-
if diagnostics.HasErrors() {
769-
// TODO: Improve the error response. The response should include the validations for each failed
770-
// parameter. The response should also indicate it's a validation error or a more general form failure.
771-
// For now, any error is sufficient.
772-
return nil, nil, BuildError{
773-
Status: http.StatusBadRequest,
774-
Message: fmt.Sprintf("%d errors occurred while resolving parameters", len(diagnostics)),
775-
Wrapped: diagnostics,
776-
}
768+
if err != nil {
769+
return nil, nil, xerrors.Errorf("resolve parameters: %w", err)
777770
}
778771

779772
names = make([]string, 0, len(buildValues))

‎coderd/wsbuilder/wsbuilderror/builderror.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
package wsbuilderror

0 commit comments

Comments
 (0)