chore: improve dynamic parameter validation errors (#18501)

`BuildError` response from `wsbuilder` does not support rich errors from validation. Changed this to use the `Validations` block of codersdk responses to return all errors for invalid parameters.

coderd/dynamicparameters/resolver.go

@@ -26,6 +26,45 @@ 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,
@@ -35,7 +74,7 @@ func ResolveParameters(
 	previousValues []database.WorkspaceBuildParameter,
 	buildValues []codersdk.WorkspaceBuildParameter,
 	presetValues []database.TemplateVersionPresetParameter,
-) (map[string]string, hcl.Diagnostics) {
+) (map[string]string, error) {
 	previousValuesMap := slice.ToMapFunc(previousValues, func(p database.WorkspaceBuildParameter) (string, string) {
 		return p.Name, p.Value
 	})
@@ -73,7 +112,10 @@ 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, diags
+		return nil, &ResolverError{
+			Diagnostics: diags,
+			Parameter:   nil,
+		}
 	}
 
 	// The user's input now needs to be validated against the parameters.
@@ -113,12 +155,16 @@ 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, diags
+		return nil, &ResolverError{
+			Diagnostics: diags,
+			Parameter:   nil,
+		}
 	}
 
 	// 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{}
 	for _, parameter := range output.Parameters {
 		parameterNames[parameter.Name] = struct{}{}
 
@@ -132,20 +178,22 @@ func ResolveParameters(
 				}
 
 				// An immutable parameter was changed, which is not allowed.
-				// Add the failed diagnostic to the output.
-				diags = diags.Append(&hcl.Diagnostic{
+				// Add a failed diagnostic to the output.
+				parameterError.Extend(parameter.Name, hcl.Diagnostics{
+					&hcl.Diagnostic{
 						Severity: hcl.DiagError,
 						Summary:  "Immutable parameter changed",
 						Detail:   fmt.Sprintf("Parameter %q is not mutable, so it can't be updated after creating a workspace.", parameter.Name),
 						Subject:  src,
+					},
 				})
 			}
 		}
 
 		// TODO: Fix the `hcl.Diagnostics(...)` type casting. It should not be needed.
 		if hcl.Diagnostics(parameter.Diagnostics).HasErrors() {
-			// All validation errors are raised here.
-			diags = diags.Extend(hcl.Diagnostics(parameter.Diagnostics))
+			// All validation errors are raised here for each parameter.
+			parameterError.Extend(parameter.Name, hcl.Diagnostics(parameter.Diagnostics))
 		}
 
 		// If the parameter has a value, but it was not set explicitly by the user at any
@@ -174,8 +222,13 @@ func ResolveParameters(
 		}
 	}
 
+	if parameterError.HasError() {
+		// If there are any errors, return them.
+		return nil, parameterError
+	}
+
 	// Return the values to be saved for the build.
-	return values.ValuesMap(), diags
+	return values.ValuesMap(), nil
 }
 
 type parameterValueMap map[string]parameterValue

coderd/httpapi/httperror/doc.go

@@ -0,0 +1,4 @@
+// Package httperror handles formatting and writing some sentinel errors returned
+// within coder to the API.
+// This package exists outside httpapi to avoid some cyclic dependencies
+package httperror

coderd/httpapi/httperror/wsbuild.go

@@ -0,0 +1,91 @@
+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
+		}
+
+		httpapi.Write(ctx, rw, buildErr.Status, codersdk.Response{
+			Message: buildErr.Message,
+			Detail:  buildErr.Error(),
+		})
+		return
+	}
+
+	var parameterErr *dynamicparameters.ResolverError
+	if errors.As(err, &parameterErr) {
+		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)
+		return
+	}
+
+	httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
+		Message: "Internal error creating workspace build.",
+		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)
+	}
+}

coderd/workspacebuilds.go

@@ -27,6 +27,7 @@ import (
 	"github.com/coder/coder/v2/coderd/database/dbtime"
 	"github.com/coder/coder/v2/coderd/database/provisionerjobs"
 	"github.com/coder/coder/v2/coderd/httpapi"
+	"github.com/coder/coder/v2/coderd/httpapi/httperror"
 	"github.com/coder/coder/v2/coderd/httpmw"
 	"github.com/coder/coder/v2/coderd/notifications"
 	"github.com/coder/coder/v2/coderd/provisionerdserver"
@@ -406,28 +407,8 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) {
 		)
 		return err
 	}, nil)
-	var buildErr wsbuilder.BuildError
-	if xerrors.As(err, &buildErr) {
-		var authErr dbauthz.NotAuthorizedError
-		if xerrors.As(err, &authErr) {
-			buildErr.Status = http.StatusForbidden
-		}
-
-		if buildErr.Status == http.StatusInternalServerError {
-			api.Logger.Error(ctx, "workspace build error", slog.Error(buildErr.Wrapped))
-		}
-
-		httpapi.Write(ctx, rw, buildErr.Status, codersdk.Response{
-			Message: buildErr.Message,
-			Detail:  buildErr.Error(),
-		})
-		return
-	}
 	if err != nil {
-		httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
-			Message: "Error posting new build",
-			Detail:  err.Error(),
-		})
+		httperror.WriteWorkspaceBuildError(ctx, rw, err)
 		return
 	}
 

coderd/workspaces.go

@@ -27,6 +27,7 @@ import (
 	"github.com/coder/coder/v2/coderd/database/dbtime"
 	"github.com/coder/coder/v2/coderd/database/provisionerjobs"
 	"github.com/coder/coder/v2/coderd/httpapi"
+	"github.com/coder/coder/v2/coderd/httpapi/httperror"
 	"github.com/coder/coder/v2/coderd/httpmw"
 	"github.com/coder/coder/v2/coderd/notifications"
 	"github.com/coder/coder/v2/coderd/prebuilds"
@@ -728,21 +729,11 @@ func createWorkspace(
 		)
 		return err
 	}, nil)
-	var bldErr wsbuilder.BuildError
-	if xerrors.As(err, &bldErr) {
-		httpapi.Write(ctx, rw, bldErr.Status, codersdk.Response{
-			Message: bldErr.Message,
-			Detail:  bldErr.Error(),
-		})
-		return
-	}
 	if err != nil {
-		httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
-			Message: "Internal error creating workspace.",
-			Detail:  err.Error(),
-		})
+		httperror.WriteWorkspaceBuildError(ctx, rw, err)
 		return
 	}
+
 	err = provisionerjobs.PostJob(api.Pubsub, *provisionerJob)
 	if err != nil {
 		// Client probably doesn't care about this error, so just log it.

coderd/wsbuilder/wsbuilder.go

@@ -752,20 +752,12 @@ func (b *Builder) getDynamicParameters() (names, values []string, err error) {
 		return nil, nil, BuildError{http.StatusInternalServerError, "failed to check if first build", err}
 	}
 
-	buildValues, diagnostics := dynamicparameters.ResolveParameters(b.ctx, b.workspace.OwnerID, render, firstBuild,
+	buildValues, err := dynamicparameters.ResolveParameters(b.ctx, b.workspace.OwnerID, render, firstBuild,
 		lastBuildParameters,
 		b.richParameterValues,
 		presetParameterValues)
-
-	if diagnostics.HasErrors() {
-		// TODO: Improve the error response. The response should include the validations for each failed
-		//  parameter. The response should also indicate it's a validation error or a more general form failure.
-		//  For now, any error is sufficient.
-		return nil, nil, BuildError{
-			Status:  http.StatusBadRequest,
-			Message: fmt.Sprintf("%d errors occurred while resolving parameters", len(diagnostics)),
-			Wrapped: diagnostics,
-		}
+	if err != nil {
+		return nil, nil, xerrors.Errorf("resolve parameters: %w", err)
 	}
 
 	names = make([]string, 0, len(buildValues))