Skip to content

chore: create interface for pkgs to return codersdk errors #18719

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+177 −119
Open

chore: create interface for pkgs to return codersdk errors #18719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 124 additions & 0 deletions coderd/dynamicparameters/error.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
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 string
Diagnostics hcl.Diagnostics
KeyedDiagnostics map[string]hcl.Diagnostics
Comment on lines +31 to +32
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: What's the difference between Diagnostics and KeyedDiagnostics?
When should you use one or the other?

}

// 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)
}
Comment on lines +59 to +71
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit/quibble: Similarly here, when should you use Append() vs Extend()?
I would expect to use Append() like this:

var derr DiagnosticError
thing, diags := preview.Frobulate()
if diags.HasErrors() {
  derr.Append(diags...)
}


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)
}
}
51 changes: 3 additions & 48 deletions coderd/dynamicparameters/resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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{}{}

Expand Down
19 changes: 19 additions & 0 deletions coderd/httpapi/httperror/responserror.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package httperror

import (
"errors"

"github.com/coder/coder/v2/codersdk"
)

type CoderSDKError interface {
Response() (int, codersdk.Response)
}
Comment on lines +9 to +11
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should probably be named Responder or Responser?


func IsCoderSDKError(err error) (CoderSDKError, bool) {
var responseErr CoderSDKError
if errors.As(err, &responseErr) {
return responseErr, true
}
return nil, false
}
74 changes: 3 additions & 71 deletions coderd/httpapi/httperror/wsbuild.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

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 responseErr, ok := IsCoderSDKError(err); ok {
code, resp := responseErr.Response()

if parameterErr.Diagnostics.HasErrors() {
resp.Detail = DiagnosticsErrorString(parameterErr.Diagnostics)
}

httpapi.Write(ctx, rw, http.StatusBadRequest, resp)
httpapi.Write(ctx, rw, code, resp)
return
}

Expand All @@ -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)
}
}
10 changes: 10 additions & 0 deletions coderd/wsbuilder/wsbuilder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,13 +240,23 @@ type BuildError struct {
}

func (e BuildError) Error() string {
if e.Wrapped == nil {
return e.Message
}
return e.Wrapped.Error()
}

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(
Expand Down
18 changes: 18 additions & 0 deletions coderd/wsbuilder/wsbuilder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.IsCoderSDKError(buildErr)
require.True(t, ok, "should be a Coder SDK error")

code, resp := respErr.Response()
require.Equal(t, http.StatusBadRequest, code)
require.Equal(t, resp.Message, msg)
}

type txExpect func(mTx *dbmock.MockStore)

func expectDB(t *testing.T, opts ...txExpect) *dbmock.MockStore {
Expand Down
Loading