From 73db94bf1835835d1a0f83dfd138780656b563c8 Mon Sep 17 00:00:00 2001 From: Pablo Chacin Date: Fri, 22 Nov 2024 13:46:14 +0100 Subject: [PATCH 1/8] introduce custom Error Signed-off-by: Pablo Chacin --- error.go | 101 ++++++++++++++++++++++++++++++++++++++ error_test.go | 83 +++++++++++++++++++++++++++++++ pkg/api/api.go | 17 ++++++- pkg/client/client.go | 37 +++++++++----- pkg/client/client_test.go | 8 +-- pkg/local/local.go | 6 +-- pkg/server/server.go | 9 ++-- pkg/server/server_test.go | 20 +++----- 8 files changed, 241 insertions(+), 40 deletions(-) create mode 100644 error.go create mode 100644 error_test.go diff --git a/error.go b/error.go new file mode 100644 index 0000000..79a097f --- /dev/null +++ b/error.go @@ -0,0 +1,101 @@ +package k6build + +import ( + "encoding/json" + "errors" + "fmt" +) + +// ErrReasonUnknown signals the reason for an APIError in unknown +var ErrReasonUnknown = errors.New("reason unknown") + +// Error represents an error returned by the build service +// This custom error type facilitates extracting the reason of an error +// by using errors.Unwrap method. +// It also facilitates checking an error (or its reason) using errors.Is by +// comparing the error and its reason. +// This custom type has the following known limitations: +// - A nil Error 'e' will not satisfy errors.Is(e, nil) +// - Is method will not +type Error struct { + Err error `json:"error,omitempty"` + Reason error `json:"reason,omitempty"` +} + +// Error returns the Error as a string +func (e *Error) Error() string { + return fmt.Sprintf("%s: %s", e.Err, e.Reason) +} + +// Is returns true if the target error is the same as the Error or its reason +// It attempts several strategies: +// - compare error and reason to target's Error() +// - unwrap the error and reason and compare to target's Error +// - unwrap target and compares to the error recursively +func (e *Error) Is(target error) bool { + if target == nil { + return false + } + + if e.Err.Error() == target.Error() { + return true + } + + if e.Reason != nil && e.Reason.Error() == target.Error() { + return true + } + + if u := errors.Unwrap(e.Err); u != nil && u.Error() == target.Error() { + return true + } + + if u := errors.Unwrap(e.Reason); u != nil && u.Error() == target.Error() { + return true + } + + return e.Is(errors.Unwrap(target)) +} + +// Unwrap returns the underlying reason for the Error +func (e *Error) Unwrap() error { + return e.Reason +} + +// MarshalJSON implements the json.Marshaler interface for the Error type +func (e *Error) MarshalJSON() ([]byte, error) { + return json.Marshal(&struct { + Err string `json:"error,omitempty"` + Reason string `json:"reason,omitempty"` + }{ + Err: e.Err.Error(), + Reason: e.Reason.Error(), + }) +} + +// UnmarshalJSON implements the json.Unmarshaler interface for the Error type +func (e *Error) UnmarshalJSON(data []byte) error { + val := struct { + Err string `json:"error,omitempty"` + Reason string `json:"reason,omitempty"` + }{} + + if err := json.Unmarshal(data, &val); err != nil { + return err + } + + e.Err = errors.New(val.Err) + e.Reason = errors.New(val.Reason) + return nil +} + +// NewError creates an Error from an error and a reason +// If the reason is nil, ErrReasonUnknown is used +func NewError(err error, reason error) *Error { + if reason == nil { + reason = ErrReasonUnknown + } + return &Error{ + Err: err, + Reason: reason, + } +} diff --git a/error_test.go b/error_test.go new file mode 100644 index 0000000..3e7e21d --- /dev/null +++ b/error_test.go @@ -0,0 +1,83 @@ +package k6build + +import ( + "errors" + "fmt" + "testing" +) + +func Test_APIError(t *testing.T) { + t.Parallel() + + var ( + err = errors.New("error") + reason = errors.New("reason") + ) + + testCases := []struct { + title string + err error + reason error + expectError error + expectReason error + }{ + { + title: "error and reason", + err: err, + reason: reason, + expectError: err, + expectReason: reason, + }, + { + title: "error not reason", + err: err, + reason: nil, + expectError: err, + expectReason: ErrReasonUnknown, + }, + { + title: "wrapped err", + err: fmt.Errorf("wrapped %w", err), + reason: reason, + expectError: err, + expectReason: reason, + }, + { + title: "wrapped reason", + err: errors.New("another error"), + reason: fmt.Errorf("wrapped %w", reason), + expectError: reason, + expectReason: reason, + }, + { + title: "wrapped err in target", + err: err, + reason: reason, + expectError: fmt.Errorf("wrapped %w", err), + expectReason: reason, + }, + { + title: "wrapped reason in target", + err: err, + reason: reason, + expectError: fmt.Errorf("wrapped %w", reason), + expectReason: reason, + }, + } + + for _, tc := range testCases { + t.Run(tc.title, func(t *testing.T) { + t.Parallel() + + apiError := NewError(tc.err, tc.reason) + + if !errors.Is(apiError, tc.expectError) { + t.Fatalf("expected %v got %v", tc.expectError, apiError) + } + + if !errors.Is(errors.Unwrap(apiError), tc.expectReason) { + t.Fatalf("expected %v got %v", tc.expectError, apiError) + } + }) + } +} diff --git a/pkg/api/api.go b/pkg/api/api.go index 065dc9a..a402d4e 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -3,11 +3,22 @@ package api import ( "bytes" + "errors" "fmt" "github.com/grafana/k6build" ) +var ( + // ErrInvalidRequest signals the request could not be processed + // due to erroneous parameters + ErrInvalidRequest = errors.New("invalid request") + // ErrRequestFailed signals the request failed, probably due to a network error + ErrRequestFailed = errors.New("request failed") + // ErrBuildFailed signals the build process failed + ErrBuildFailed = errors.New("build failed") +) + // BuildRequest defines a request to the build service type BuildRequest struct { K6Constrains string `json:"k6,omitempty"` @@ -28,6 +39,10 @@ func (r BuildRequest) String() string { // BuildResponse defines the response for a BuildRequest type BuildResponse struct { - Error string `json:"error,omitempty"` + // If not empty an error occurred processing the request + // This Error can be compared to the errors defined in this package using errors.Is + // to know the type of error, and use Unwrap to obtain its cause if available. + Error *k6build.Error `json:"error,omitempty"` + // Artifact metadata. If an error occurred, content is undefined Artifact k6build.Artifact `json:"artifact,omitempty"` } diff --git a/pkg/client/client.go b/pkg/client/client.go index 2c72718..50e3207 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -14,15 +14,13 @@ import ( "github.com/grafana/k6build/pkg/api" ) +// ErrInvalidConfiguration signals an error in the configuration +var ErrInvalidConfiguration = errors.New("invalid configuration") + const ( defaultAuthType = "Bearer" ) -var ( - ErrBuildFailed = errors.New("build failed") //nolint:revive - ErrRequestFailed = errors.New("request failed") //nolint:revive -) - // BuildServiceClientConfig defines the configuration for accessing a remote build service type BuildServiceClientConfig struct { // URL to build service @@ -39,6 +37,9 @@ type BuildServiceClientConfig struct { // NewBuildServiceClient returns a new client for a remote build service func NewBuildServiceClient(config BuildServiceClientConfig) (k6build.BuildService, error) { + if config.URL == "" { + return nil, ErrInvalidConfiguration + } return &BuildClient{ srvURL: config.URL, auth: config.Authorization, @@ -55,7 +56,11 @@ type BuildClient struct { headers map[string]string } -// Build request building an artidact to a build service +// Build request building an artifact to a build service +// The build service is expected to return a k6build.Artifact +// In case of error, the returned error is expected to match any of the errors +// defined in the api package and calling errors.Unwrap(err) will provide +// the cause, if available. func (r *BuildClient) Build( ctx context.Context, platform string, @@ -70,7 +75,7 @@ func (r *BuildClient) Build( marshaled := &bytes.Buffer{} err := json.NewEncoder(marshaled).Encode(buildRequest) if err != nil { - return k6build.Artifact{}, fmt.Errorf("%w: %w", ErrRequestFailed, err) + return k6build.Artifact{}, k6build.NewError(api.ErrInvalidRequest, err) } url, err := url.Parse(r.srvURL) @@ -81,7 +86,7 @@ func (r *BuildClient) Build( req, err := http.NewRequestWithContext(ctx, http.MethodPost, url.String(), marshaled) if err != nil { - return k6build.Artifact{}, fmt.Errorf("%w: %w", ErrRequestFailed, err) + return k6build.Artifact{}, k6build.NewError(api.ErrRequestFailed, err) } req.Header.Add("Content-Type", "application/json") @@ -101,7 +106,7 @@ func (r *BuildClient) Build( resp, err := http.DefaultClient.Do(req) if err != nil { - return k6build.Artifact{}, fmt.Errorf("%w: %w", ErrRequestFailed, err) + return k6build.Artifact{}, k6build.NewError(api.ErrRequestFailed, err) } defer func() { _ = resp.Body.Close() @@ -110,15 +115,21 @@ func (r *BuildClient) Build( buildResponse := api.BuildResponse{} err = json.NewDecoder(resp.Body).Decode(&buildResponse) if err != nil { - return k6build.Artifact{}, fmt.Errorf("%w: %w", ErrRequestFailed, err) + return k6build.Artifact{}, k6build.NewError(api.ErrRequestFailed, err) } if resp.StatusCode != http.StatusOK { - return k6build.Artifact{}, fmt.Errorf("%w: %s", ErrRequestFailed, buildResponse.Error) + // if an error occurred (e.g. internal server error) the Error may not be available + if buildResponse.Error == nil { + return k6build.Artifact{}, k6build.NewError(api.ErrRequestFailed, errors.New(resp.Status)) + } + + // we expect the Error to have an api.Error we we don't wrap it again + return k6build.Artifact{}, buildResponse.Error } - if buildResponse.Error != "" { - return k6build.Artifact{}, fmt.Errorf("%w: %s", ErrBuildFailed, buildResponse.Error) + if buildResponse.Error != nil { + return k6build.Artifact{}, buildResponse.Error } return buildResponse.Artifact, nil diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index ebc9e31..9ae334a 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -115,9 +115,9 @@ func TestRemote(t *testing.T) { { title: "build request failed", handlers: []requestHandler{ - withResponse(http.StatusOK, api.BuildResponse{Error: ErrBuildFailed.Error()}), + withResponse(http.StatusOK, api.BuildResponse{Error: k6build.NewError(api.ErrBuildFailed, nil)}), }, - expectErr: ErrBuildFailed, + expectErr: api.ErrBuildFailed, }, { title: "auth header", @@ -140,9 +140,9 @@ func TestRemote(t *testing.T) { { title: "failed auth", handlers: []requestHandler{ - withResponse(http.StatusUnauthorized, api.BuildResponse{Error: "Authorization Required"}), + withResponse(http.StatusUnauthorized, api.BuildResponse{Error: k6build.NewError(api.ErrRequestFailed, errors.New("unauthorized"))}), }, - expectErr: ErrRequestFailed, + expectErr: api.ErrRequestFailed, }, { title: "custom headers", diff --git a/pkg/local/local.go b/pkg/local/local.go index cd00cd7..6f24f5f 100644 --- a/pkg/local/local.go +++ b/pkg/local/local.go @@ -208,13 +208,13 @@ func (b *localBuildSrv) Build( //nolint:funlen } if !errors.Is(err, cache.ErrObjectNotFound) { - return k6build.Artifact{}, fmt.Errorf("accessing artifact %w", err) + return k6build.Artifact{}, fmt.Errorf("accessing artifact: %w", err) } artifactBuffer := &bytes.Buffer{} buildInfo, err := b.builder.Build(ctx, buildPlatform, k6Mod.Version, mods, []string{}, artifactBuffer) if err != nil { - return k6build.Artifact{}, fmt.Errorf("building artifact %w", err) + return k6build.Artifact{}, fmt.Errorf("building artifact: %w", err) } // if this is a prerelease, we must use the actual version built @@ -225,7 +225,7 @@ func (b *localBuildSrv) Build( //nolint:funlen artifactObject, err = b.cache.Store(ctx, id, artifactBuffer) if err != nil { - return k6build.Artifact{}, fmt.Errorf("creating object %w", err) + return k6build.Artifact{}, fmt.Errorf("creating object: %w", err) } return k6build.Artifact{ diff --git a/pkg/server/server.go b/pkg/server/server.go index 757cc92..def5b72 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -4,7 +4,6 @@ package server import ( "context" "encoding/json" - "fmt" "io" "log/slog" "net/http" @@ -51,8 +50,8 @@ func (a *APIServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { // ensure errors are reported and logged defer func() { - if resp.Error != "" { - a.log.Error(resp.Error) + if resp.Error != nil { + a.log.Error(resp.Error.Error()) _ = json.NewEncoder(w).Encode(resp) //nolint:errchkjson } }() @@ -61,7 +60,7 @@ func (a *APIServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { err := json.NewDecoder(r.Body).Decode(&req) if err != nil { w.WriteHeader(http.StatusBadRequest) - resp.Error = fmt.Sprintf("invalid request: %s", err.Error()) + resp.Error = k6build.NewError(api.ErrInvalidRequest, err) return } @@ -75,7 +74,7 @@ func (a *APIServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { ) if err != nil { w.WriteHeader(http.StatusBadRequest) - resp.Error = fmt.Sprintf("building artifact: %s", err.Error()) + resp.Error = k6build.NewError(api.ErrBuildFailed, err) return } diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go index aab6782..9765fcb 100644 --- a/pkg/server/server_test.go +++ b/pkg/server/server_test.go @@ -4,9 +4,9 @@ import ( "bytes" "context" "encoding/json" + "errors" "net/http" "net/http/httptest" - "strings" "testing" "github.com/grafana/k6build" @@ -51,14 +51,6 @@ func buildErr( return k6build.Artifact{}, k6build.ErrBuildFailed } -func expectedError(expected string, actual string) bool { - if expected != "" { - return strings.Contains(actual, expected) - } - - return actual == "" -} - func TestAPIServer(t *testing.T) { t.Parallel() @@ -67,7 +59,7 @@ func TestAPIServer(t *testing.T) { build buildFunction req []byte status int - err string + err error artifact k6build.Artifact }{ { @@ -76,7 +68,7 @@ func TestAPIServer(t *testing.T) { req: []byte("{\"Platform\": \"linux/amd64\", \"K6Constrains\": \"v0.1.0\", \"Dependencies\": []}"), status: http.StatusOK, artifact: k6build.Artifact{}, - err: "", + err: nil, }, { title: "build error", @@ -84,7 +76,7 @@ func TestAPIServer(t *testing.T) { req: []byte("{\"Platform\": \"linux/amd64\", \"K6Constrains\": \"v0.1.0\", \"Dependencies\": []}"), status: http.StatusBadRequest, artifact: k6build.Artifact{}, - err: k6build.ErrBuildFailed.Error(), + err: api.ErrBuildFailed, }, { title: "invalid request", @@ -92,7 +84,7 @@ func TestAPIServer(t *testing.T) { req: []byte(""), status: http.StatusBadRequest, artifact: k6build.Artifact{}, - err: "invalid request", + err: api.ErrInvalidRequest, }, } @@ -127,7 +119,7 @@ func TestAPIServer(t *testing.T) { t.Fatalf("decoding response %v", err) } - if !expectedError(tc.err, buildResponse.Error) { + if tc.err != nil && !errors.Is(buildResponse.Error, tc.err) { t.Fatalf("expected error: %q got %q", tc.err, buildResponse.Error) } }) From 0a4c567fd12d8661253f98d89fc5ba912b76b1ff Mon Sep 17 00:00:00 2001 From: Pablo Chacin Date: Mon, 25 Nov 2024 18:30:34 +0100 Subject: [PATCH 2/8] return status ok on build fail Signed-off-by: Pablo Chacin --- pkg/server/server.go | 2 +- pkg/server/server_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/server/server.go b/pkg/server/server.go index def5b72..e9eece9 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -73,7 +73,7 @@ func (a *APIServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { req.Dependencies, ) if err != nil { - w.WriteHeader(http.StatusBadRequest) + w.WriteHeader(http.StatusOK) resp.Error = k6build.NewError(api.ErrBuildFailed, err) return } diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go index 9765fcb..bb6d542 100644 --- a/pkg/server/server_test.go +++ b/pkg/server/server_test.go @@ -74,7 +74,7 @@ func TestAPIServer(t *testing.T) { title: "build error", build: buildFunction(buildErr), req: []byte("{\"Platform\": \"linux/amd64\", \"K6Constrains\": \"v0.1.0\", \"Dependencies\": []}"), - status: http.StatusBadRequest, + status: http.StatusOK, artifact: k6build.Artifact{}, err: api.ErrBuildFailed, }, From 6595e3e9cf1b500561cf8619b312896744c252c7 Mon Sep 17 00:00:00 2001 From: Pablo Chacin Date: Mon, 25 Nov 2024 18:47:37 +0100 Subject: [PATCH 3/8] remove unused error Signed-off-by: Pablo Chacin --- cmd/local/local.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/cmd/local/local.go b/cmd/local/local.go index 6bf453c..881ebb0 100644 --- a/cmd/local/local.go +++ b/cmd/local/local.go @@ -2,7 +2,6 @@ package local import ( - "errors" "fmt" "io" "net/url" @@ -16,8 +15,6 @@ import ( "github.com/spf13/cobra" ) -var ErrTargetPlatformUndefined = errors.New("target platform is required") //nolint:revive - const ( long = ` k6build local builder creates a custom k6 binary artifacts that satisfies certain From 11da63583fd94ae93645f2299be4a44f9b9c9e3f Mon Sep 17 00:00:00 2001 From: Pablo Chacin Date: Mon, 25 Nov 2024 21:19:22 +0100 Subject: [PATCH 4/8] allow multiple reasons for errors Signed-off-by: Pablo Chacin --- error.go | 7 ++-- error_test.go | 89 +++++++++++++++++++++++++-------------------------- 2 files changed, 47 insertions(+), 49 deletions(-) diff --git a/error.go b/error.go index 79a097f..a6392bd 100644 --- a/error.go +++ b/error.go @@ -90,9 +90,10 @@ func (e *Error) UnmarshalJSON(data []byte) error { // NewError creates an Error from an error and a reason // If the reason is nil, ErrReasonUnknown is used -func NewError(err error, reason error) *Error { - if reason == nil { - reason = ErrReasonUnknown +func NewError(err error, reasons ...error) *Error { + reason := ErrReasonUnknown + if len(reasons) > 0 { + reason = NewError(reasons[0], reasons[1:]...) } return &Error{ Err: err, diff --git a/error_test.go b/error_test.go index 3e7e21d..ea5d646 100644 --- a/error_test.go +++ b/error_test.go @@ -6,62 +6,62 @@ import ( "testing" ) -func Test_APIError(t *testing.T) { +func Test_Error(t *testing.T) { t.Parallel() var ( - err = errors.New("error") - reason = errors.New("reason") + err = errors.New("error") + reason = errors.New("reason") + another = errors.New("another reason") ) testCases := []struct { - title string - err error - reason error - expectError error - expectReason error + title string + err error + reasons []error + expect []error }{ { - title: "error and reason", - err: err, - reason: reason, - expectError: err, - expectReason: reason, + title: "error and reason", + err: err, + reasons: []error{reason}, + expect: []error{err, reason}, }, { - title: "error not reason", - err: err, - reason: nil, - expectError: err, - expectReason: ErrReasonUnknown, + title: "error not reason", + err: err, + reasons: nil, + expect: []error{err}, }, { - title: "wrapped err", - err: fmt.Errorf("wrapped %w", err), - reason: reason, - expectError: err, - expectReason: reason, + title: "multiple and reasons", + err: err, + reasons: []error{reason, another}, + expect: []error{err, reason, another}, }, { - title: "wrapped reason", - err: errors.New("another error"), - reason: fmt.Errorf("wrapped %w", reason), - expectError: reason, - expectReason: reason, + title: "wrapped err", + err: fmt.Errorf("wrapped %w", err), + reasons: []error{reason}, + expect: []error{err, reason}, }, { - title: "wrapped err in target", - err: err, - reason: reason, - expectError: fmt.Errorf("wrapped %w", err), - expectReason: reason, + title: "wrapped reason", + err: err, + reasons: []error{fmt.Errorf("wrapped %w", reason)}, + expect: []error{err, reason}, }, { - title: "wrapped reason in target", - err: err, - reason: reason, - expectError: fmt.Errorf("wrapped %w", reason), - expectReason: reason, + title: "wrapped err in target", + err: err, + reasons: []error{reason}, + expect: []error{fmt.Errorf("wrapped %w", err)}, + }, + { + title: "wrapped reason in target", + err: err, + reasons: []error{reason}, + expect: []error{fmt.Errorf("wrapped %w", reason)}, }, } @@ -69,14 +69,11 @@ func Test_APIError(t *testing.T) { t.Run(tc.title, func(t *testing.T) { t.Parallel() - apiError := NewError(tc.err, tc.reason) - - if !errors.Is(apiError, tc.expectError) { - t.Fatalf("expected %v got %v", tc.expectError, apiError) - } - - if !errors.Is(errors.Unwrap(apiError), tc.expectReason) { - t.Fatalf("expected %v got %v", tc.expectError, apiError) + err := NewError(tc.err, tc.reasons...) + for _, expected := range tc.expect { + if !errors.Is(err, expected) { + t.Fatalf("expected %v got %v", expected, err) + } } }) } From f0d2c13e60e3a0c84e762f45b62d6f63155bff64 Mon Sep 17 00:00:00 2001 From: Pablo Chacin Date: Mon, 25 Nov 2024 21:23:21 +0100 Subject: [PATCH 5/8] allow empty reason Signed-off-by: Pablo Chacin --- error.go | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/error.go b/error.go index a6392bd..ef45cf0 100644 --- a/error.go +++ b/error.go @@ -6,9 +6,6 @@ import ( "fmt" ) -// ErrReasonUnknown signals the reason for an APIError in unknown -var ErrReasonUnknown = errors.New("reason unknown") - // Error represents an error returned by the build service // This custom error type facilitates extracting the reason of an error // by using errors.Unwrap method. @@ -24,7 +21,11 @@ type Error struct { // Error returns the Error as a string func (e *Error) Error() string { - return fmt.Sprintf("%s: %s", e.Err, e.Reason) + reason := "" + if e.Reason != nil { + reason = fmt.Sprintf(": %s", e.Reason) + } + return fmt.Sprintf("%s%s", e.Err, reason) } // Is returns true if the target error is the same as the Error or its reason @@ -63,12 +64,16 @@ func (e *Error) Unwrap() error { // MarshalJSON implements the json.Marshaler interface for the Error type func (e *Error) MarshalJSON() ([]byte, error) { + reason := "" + if e.Reason != nil { + reason = e.Reason.Error() + } return json.Marshal(&struct { Err string `json:"error,omitempty"` Reason string `json:"reason,omitempty"` }{ Err: e.Err.Error(), - Reason: e.Reason.Error(), + Reason: reason, }) } @@ -91,7 +96,7 @@ func (e *Error) UnmarshalJSON(data []byte) error { // NewError creates an Error from an error and a reason // If the reason is nil, ErrReasonUnknown is used func NewError(err error, reasons ...error) *Error { - reason := ErrReasonUnknown + var reason error if len(reasons) > 0 { reason = NewError(reasons[0], reasons[1:]...) } From b1ff328ba720ec5240c1bf767490abfab3623cf3 Mon Sep 17 00:00:00 2001 From: Pablo Chacin Date: Tue, 26 Nov 2024 13:30:41 +0100 Subject: [PATCH 6/8] use k6build.Error for wrapping errors Signed-off-by: Pablo Chacin --- pkg/cache/api/api.go | 3 ++- pkg/cache/client/client.go | 29 ++++++++++++++-------------- pkg/cache/client/client_test.go | 9 +++++---- pkg/cache/file/file.go | 23 +++++++++++----------- pkg/cache/server/server.go | 15 ++++++++------- pkg/cache/server/server_test.go | 6 +++--- pkg/local/local.go | 34 ++++++++++++++++++--------------- 7 files changed, 64 insertions(+), 55 deletions(-) diff --git a/pkg/cache/api/api.go b/pkg/cache/api/api.go index c055655..402e488 100644 --- a/pkg/cache/api/api.go +++ b/pkg/cache/api/api.go @@ -2,11 +2,12 @@ package api import ( + "github.com/grafana/k6build" "github.com/grafana/k6build/pkg/cache" ) // CacheResponse is the response to a cache server request type CacheResponse struct { - Error string + Error *k6build.Error Object cache.Object } diff --git a/pkg/cache/client/client.go b/pkg/cache/client/client.go index 6b0a92c..0d77d1b 100644 --- a/pkg/cache/client/client.go +++ b/pkg/cache/client/client.go @@ -10,6 +10,7 @@ import ( "net/http" "net/url" + "github.com/grafana/k6build" "github.com/grafana/k6build/pkg/cache" "github.com/grafana/k6build/pkg/cache/api" ) @@ -36,7 +37,7 @@ type CacheClient struct { // NewCacheClient returns a client for a cache server func NewCacheClient(config CacheClientConfig) (*CacheClient, error) { if _, err := url.Parse(config.Server); err != nil { - return nil, fmt.Errorf("%w: %w", ErrInvalidConfig, err) + return nil, k6build.NewError(ErrInvalidConfig, err) } return &CacheClient{ @@ -51,7 +52,7 @@ func (c *CacheClient) Get(_ context.Context, id string) (cache.Object, error) { // TODO: use http.Request resp, err := http.Get(url) //nolint:gosec,noctx if err != nil { - return cache.Object{}, fmt.Errorf("%w %w", ErrAccessingServer, err) + return cache.Object{}, k6build.NewError(ErrAccessingServer, err) } defer func() { _ = resp.Body.Close() @@ -59,19 +60,19 @@ func (c *CacheClient) Get(_ context.Context, id string) (cache.Object, error) { if resp.StatusCode != http.StatusOK { if resp.StatusCode == http.StatusNotFound { - return cache.Object{}, fmt.Errorf("%w with status", cache.ErrObjectNotFound) + return cache.Object{}, cache.ErrObjectNotFound } - return cache.Object{}, fmt.Errorf("%w with status %s", ErrRequestFailed, resp.Status) + return cache.Object{}, k6build.NewError(ErrRequestFailed, fmt.Errorf("status %s", resp.Status)) } cacheResponse := api.CacheResponse{} err = json.NewDecoder(resp.Body).Decode(&cacheResponse) if err != nil { - return cache.Object{}, fmt.Errorf("%w: %s", ErrInvalidResponse, err.Error()) + return cache.Object{}, k6build.NewError(ErrInvalidResponse, err) } - if cacheResponse.Error != "" { - return cache.Object{}, fmt.Errorf("%w: %s", ErrRequestFailed, cacheResponse.Error) + if cacheResponse.Error != nil { + return cache.Object{}, k6build.NewError(ErrRequestFailed, cacheResponse.Error) } return cacheResponse.Object, nil @@ -86,23 +87,23 @@ func (c *CacheClient) Store(_ context.Context, id string, content io.Reader) (ca content, ) if err != nil { - return cache.Object{}, fmt.Errorf("%w %w", ErrAccessingServer, err) + return cache.Object{}, k6build.NewError(ErrAccessingServer, err) } defer func() { _ = resp.Body.Close() }() if resp.StatusCode != http.StatusOK { - return cache.Object{}, fmt.Errorf("%w with status %s", ErrRequestFailed, resp.Status) + return cache.Object{}, k6build.NewError(ErrRequestFailed, fmt.Errorf("status %s", resp.Status)) } cacheResponse := api.CacheResponse{} err = json.NewDecoder(resp.Body).Decode(&cacheResponse) if err != nil { - return cache.Object{}, fmt.Errorf("%w: %s", ErrInvalidResponse, err.Error()) + return cache.Object{}, k6build.NewError(ErrInvalidResponse, err) } - if cacheResponse.Error != "" { - return cache.Object{}, fmt.Errorf("%w: %s", ErrRequestFailed, cacheResponse.Error) + if cacheResponse.Error != nil { + return cache.Object{}, k6build.NewError(ErrRequestFailed, cacheResponse.Error) } return cacheResponse.Object, nil @@ -112,11 +113,11 @@ func (c *CacheClient) Store(_ context.Context, id string, content io.Reader) (ca func (c *CacheClient) Download(_ context.Context, object cache.Object) (io.ReadCloser, error) { resp, err := http.Get(object.URL) //nolint:noctx,bodyclose if err != nil { - return nil, fmt.Errorf("%w %w", ErrAccessingServer, err) + return nil, k6build.NewError(ErrAccessingServer, err) } if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("%w with status %s", ErrRequestFailed, resp.Status) + return nil, k6build.NewError(ErrRequestFailed, fmt.Errorf("status %s", resp.Status)) } return resp.Request.Body, nil diff --git a/pkg/cache/client/client_test.go b/pkg/cache/client/client_test.go index e4b773e..6ef9ba5 100644 --- a/pkg/cache/client/client_test.go +++ b/pkg/cache/client/client_test.go @@ -9,6 +9,7 @@ import ( "net/http/httptest" "testing" + "github.com/grafana/k6build" "github.com/grafana/k6build/pkg/cache" "github.com/grafana/k6build/pkg/cache/api" ) @@ -58,7 +59,7 @@ func TestCacheClientGet(t *testing.T) { title: "normal get", status: http.StatusOK, resp: &api.CacheResponse{ - Error: "", + Error: nil, Object: cache.Object{}, }, }, @@ -72,7 +73,7 @@ func TestCacheClientGet(t *testing.T) { title: "error accessing object", status: http.StatusInternalServerError, resp: &api.CacheResponse{ - Error: "Error accessing object", + Error: k6build.NewError(cache.ErrAccessingObject), Object: cache.Object{}, }, expectErr: ErrRequestFailed, @@ -112,7 +113,7 @@ func TestCacheClientStore(t *testing.T) { title: "normal response", status: http.StatusOK, resp: &api.CacheResponse{ - Error: "", + Error: nil, Object: cache.Object{}, }, }, @@ -120,7 +121,7 @@ func TestCacheClientStore(t *testing.T) { title: "error creating object", status: http.StatusInternalServerError, resp: &api.CacheResponse{ - Error: "Error creating object", + Error: k6build.NewError(cache.ErrCreatingObject), Object: cache.Object{}, }, expectErr: ErrRequestFailed, diff --git a/pkg/cache/file/file.go b/pkg/cache/file/file.go index 205929d..89658ca 100644 --- a/pkg/cache/file/file.go +++ b/pkg/cache/file/file.go @@ -14,6 +14,7 @@ import ( "strings" "sync" + "github.com/grafana/k6build" "github.com/grafana/k6build/pkg/cache" "github.com/grafana/k6build/pkg/util" ) @@ -33,7 +34,7 @@ func NewTempFileCache() (cache.Cache, error) { func NewFileCache(dir string) (cache.Cache, error) { err := os.MkdirAll(dir, 0o750) if err != nil { - return nil, fmt.Errorf("%w: %w", cache.ErrInitializingCache, err) + return nil, k6build.NewError(cache.ErrInitializingCache, err) } return &Cache{ @@ -45,7 +46,7 @@ func NewFileCache(dir string) (cache.Cache, error) { // Fails if the object already exists func (f *Cache) Store(_ context.Context, id string, content io.Reader) (cache.Object, error) { if id == "" { - return cache.Object{}, fmt.Errorf("%w id cannot be empty", cache.ErrCreatingObject) + return cache.Object{}, fmt.Errorf("%w: id cannot be empty", cache.ErrCreatingObject) } if strings.Contains(id, "/") { @@ -65,12 +66,12 @@ func (f *Cache) Store(_ context.Context, id string, content io.Reader) (cache.Ob // TODO: check permissions err := os.MkdirAll(objectDir, 0o750) if err != nil { - return cache.Object{}, fmt.Errorf("%w: %w", cache.ErrCreatingObject, err) + return cache.Object{}, k6build.NewError(cache.ErrCreatingObject, err) } objectFile, err := os.Create(filepath.Join(objectDir, "data")) //nolint:gosec if err != nil { - return cache.Object{}, fmt.Errorf("%w: %w", cache.ErrCreatingObject, err) + return cache.Object{}, k6build.NewError(cache.ErrCreatingObject, err) } defer objectFile.Close() //nolint:errcheck @@ -79,7 +80,7 @@ func (f *Cache) Store(_ context.Context, id string, content io.Reader) (cache.Ob buff := bytes.Buffer{} _, err = io.Copy(objectFile, io.TeeReader(content, &buff)) if err != nil { - return cache.Object{}, fmt.Errorf("%w: %w", cache.ErrCreatingObject, err) + return cache.Object{}, k6build.NewError(cache.ErrCreatingObject, err) } // calculate checksum @@ -88,7 +89,7 @@ func (f *Cache) Store(_ context.Context, id string, content io.Reader) (cache.Ob // write metadata err = os.WriteFile(filepath.Join(objectDir, "checksum"), []byte(checksum), 0o644) //nolint:gosec if err != nil { - return cache.Object{}, fmt.Errorf("%w: %w", cache.ErrCreatingObject, err) + return cache.Object{}, k6build.NewError(cache.ErrCreatingObject, err) } objectURL, _ := util.URLFromFilePath(objectFile.Name()) @@ -105,16 +106,16 @@ func (f *Cache) Get(_ context.Context, id string) (cache.Object, error) { _, err := os.Stat(objectDir) if errors.Is(err, os.ErrNotExist) { - return cache.Object{}, fmt.Errorf("%w: %s", cache.ErrObjectNotFound, id) + return cache.Object{}, fmt.Errorf("%w (%s)", cache.ErrObjectNotFound, id) } if err != nil { - return cache.Object{}, fmt.Errorf("%w: %w", cache.ErrAccessingObject, err) + return cache.Object{}, k6build.NewError(cache.ErrAccessingObject, err) } checksum, err := os.ReadFile(filepath.Join(objectDir, "checksum")) //nolint:gosec if err != nil { - return cache.Object{}, fmt.Errorf("%w: %w", cache.ErrAccessingObject, err) + return cache.Object{}, k6build.NewError(cache.ErrAccessingObject, err) } objectURL, _ := util.URLFromFilePath(filepath.Join(objectDir, "data")) @@ -129,7 +130,7 @@ func (f *Cache) Get(_ context.Context, id string) (cache.Object, error) { func (f *Cache) Download(_ context.Context, object cache.Object) (io.ReadCloser, error) { url, err := url.Parse(object.URL) if err != nil { - return nil, fmt.Errorf("%w: %w", cache.ErrAccessingObject, err) + return nil, k6build.NewError(cache.ErrAccessingObject, err) } switch url.Scheme { @@ -151,7 +152,7 @@ func (f *Cache) Download(_ context.Context, object cache.Object) (io.ReadCloser, if errors.Is(err, os.ErrNotExist) { return nil, cache.ErrObjectNotFound } - return nil, fmt.Errorf("%w: %w", cache.ErrAccessingObject, err) + return nil, k6build.NewError(cache.ErrAccessingObject, err) } return objectFile, nil diff --git a/pkg/cache/server/server.go b/pkg/cache/server/server.go index 6afd95c..2ac14c6 100644 --- a/pkg/cache/server/server.go +++ b/pkg/cache/server/server.go @@ -10,6 +10,7 @@ import ( "log/slog" "net/http" + "github.com/grafana/k6build" "github.com/grafana/k6build/pkg/cache" "github.com/grafana/k6build/pkg/cache/api" ) @@ -66,8 +67,8 @@ func (s *CacheServer) Get(w http.ResponseWriter, r *http.Request) { id := r.PathValue("id") if id == "" { w.WriteHeader(http.StatusBadRequest) - resp.Error = ErrInvalidRequest.Error() - s.log.Error(resp.Error) + resp.Error = k6build.NewError(ErrInvalidRequest) + s.log.Error(resp.Error.Error()) _ = json.NewEncoder(w).Encode(resp) //nolint:errchkjson return } @@ -81,7 +82,7 @@ func (s *CacheServer) Get(w http.ResponseWriter, r *http.Request) { s.log.Error(err.Error()) w.WriteHeader(http.StatusInternalServerError) } - resp.Error = err.Error() + resp.Error = k6build.NewError(err) _ = json.NewEncoder(w).Encode(resp) //nolint:errchkjson return @@ -106,8 +107,8 @@ func (s *CacheServer) Store(w http.ResponseWriter, r *http.Request) { // ensure errors are reported and logged defer func() { - if resp.Error != "" { - s.log.Error(resp.Error) + if resp.Error != nil { + s.log.Error(resp.Error.Error()) _ = json.NewEncoder(w).Encode(resp) //nolint:errchkjson } }() @@ -115,14 +116,14 @@ func (s *CacheServer) Store(w http.ResponseWriter, r *http.Request) { id := r.PathValue("id") if id == "" { w.WriteHeader(http.StatusBadRequest) - resp.Error = ErrInvalidRequest.Error() + resp.Error = k6build.NewError(ErrInvalidRequest, fmt.Errorf("object id is required")) return } object, err := s.cache.Store(context.Background(), id, r.Body) //nolint:contextcheck if err != nil { w.WriteHeader(http.StatusBadRequest) - resp.Error = err.Error() + resp.Error = k6build.NewError(err) return } diff --git a/pkg/cache/server/server_test.go b/pkg/cache/server/server_test.go index 040f27a..9009194 100644 --- a/pkg/cache/server/server_test.go +++ b/pkg/cache/server/server_test.go @@ -101,7 +101,7 @@ func TestCacheServerGet(t *testing.T) { title string id string status int - epectErr string + epectErr error }{ { title: "return object", @@ -140,7 +140,7 @@ func TestCacheServerGet(t *testing.T) { } if tc.status != http.StatusOK { - if cacheResponse.Error == "" { + if cacheResponse.Error == nil { t.Fatalf("expected error message not none") } return @@ -208,7 +208,7 @@ func TestCacheServerStore(t *testing.T) { } if tc.status != http.StatusOK { - if cacheResponse.Error == "" { + if cacheResponse.Error == nil { t.Fatalf("expected error message not none") } return diff --git a/pkg/local/local.go b/pkg/local/local.go index 6f24f5f..7ef87d1 100644 --- a/pkg/local/local.go +++ b/pkg/local/local.go @@ -30,7 +30,11 @@ const ( ) var ( - ErrInvalidConstrains = errors.New("invalid constrains") //nolint:revive + ErrAccessingArtifact = errors.New("accessing artifact") //nolint:revive + ErrBuildingArtifact = errors.New("building artifact") //nolint:revive + ErrInitializingBuilder = errors.New("initializing builder") //nolint:revive + ErrInvalidParameters = errors.New("invalid build parameters") //nolint:revive + ErrPrereleaseNotAllowed = errors.New("pre-releases are not allowed") //nolint:revive constrainRe = regexp.MustCompile(opRe + verRe + preRe) ) @@ -67,7 +71,7 @@ type localBuildSrv struct { func NewBuildService(ctx context.Context, config BuildServiceConfig) (k6build.BuildService, error) { catalog, err := k6catalog.NewCatalog(ctx, config.Catalog) if err != nil { - return nil, fmt.Errorf("getting catalog %w", err) + return nil, k6build.NewError(ErrInitializingBuilder, err) } builderOpts := k6foundry.NativeBuilderOpts{ @@ -83,7 +87,7 @@ func NewBuildService(ctx context.Context, config BuildServiceConfig) (k6build.Bu builder, err := k6foundry.NewNativeBuilder(ctx, builderOpts) if err != nil { - return nil, fmt.Errorf("creating builder %w", err) + return nil, k6build.NewError(ErrInitializingBuilder, err) } var cache cache.Cache @@ -95,12 +99,12 @@ func NewBuildService(ctx context.Context, config BuildServiceConfig) (k6build.Bu }, ) if err != nil { - return nil, fmt.Errorf("creating cache client %w", err) + return nil, k6build.NewError(ErrInitializingBuilder, err) } } else { cache, err = file.NewFileCache(config.CacheDir) if err != nil { - return nil, fmt.Errorf("creating cache %w", err) + return nil, k6build.NewError(ErrInitializingBuilder, err) } } @@ -116,17 +120,17 @@ func NewBuildService(ctx context.Context, config BuildServiceConfig) (k6build.Bu func DefaultLocalBuildService() (k6build.BuildService, error) { catalog, err := k6catalog.DefaultCatalog() if err != nil { - return nil, fmt.Errorf("creating catalog %w", err) + return nil, k6build.NewError(ErrInitializingBuilder, err) } builder, err := k6foundry.NewDefaultNativeBuilder() if err != nil { - return nil, fmt.Errorf("creating builder %w", err) + return nil, k6build.NewError(ErrInitializingBuilder, err) } cache, err := file.NewTempFileCache() if err != nil { - return nil, fmt.Errorf("creating temp cache %w", err) + return nil, k6build.NewError(ErrInitializingBuilder, err) } return &localBuildSrv{ @@ -144,7 +148,7 @@ func (b *localBuildSrv) Build( //nolint:funlen ) (k6build.Artifact, error) { buildPlatform, err := k6foundry.ParsePlatform(platform) if err != nil { - return k6build.Artifact{}, fmt.Errorf("invalid platform %w", err) + return k6build.Artifact{}, k6build.NewError(ErrInvalidParameters, err) } // sort dependencies to ensure idempotence of build @@ -163,7 +167,7 @@ func (b *localBuildSrv) Build( //nolint:funlen } if prerelease != "" { if !b.allowPrereleases { - return k6build.Artifact{}, fmt.Errorf("%w: pre-releases are not allowed", ErrInvalidConstrains) + return k6build.Artifact{}, k6build.NewError(ErrInvalidParameters, ErrPrereleaseNotAllowed) } k6Mod = k6catalog.Module{Path: k6Path, Version: prerelease} } else { @@ -208,13 +212,13 @@ func (b *localBuildSrv) Build( //nolint:funlen } if !errors.Is(err, cache.ErrObjectNotFound) { - return k6build.Artifact{}, fmt.Errorf("accessing artifact: %w", err) + return k6build.Artifact{}, k6build.NewError(ErrAccessingArtifact, err) } artifactBuffer := &bytes.Buffer{} buildInfo, err := b.builder.Build(ctx, buildPlatform, k6Mod.Version, mods, []string{}, artifactBuffer) if err != nil { - return k6build.Artifact{}, fmt.Errorf("building artifact: %w", err) + return k6build.Artifact{}, k6build.NewError(ErrAccessingArtifact, err) } // if this is a prerelease, we must use the actual version built @@ -225,7 +229,7 @@ func (b *localBuildSrv) Build( //nolint:funlen artifactObject, err = b.cache.Store(ctx, id, artifactBuffer) if err != nil { - return k6build.Artifact{}, fmt.Errorf("creating object: %w", err) + return k6build.Artifact{}, k6build.NewError(ErrAccessingArtifact, err) } return k6build.Artifact{ @@ -269,11 +273,11 @@ func isPrerelease(constrain string) (string, error) { prerelease := matches[preIdx] if op != "" && op != "=" { - return "", fmt.Errorf("%w only exact match is allowed for pre-release versions", ErrInvalidConstrains) + return "", k6build.NewError(ErrInvalidParameters, fmt.Errorf("only exact match is allowed for pre-release versions")) } if ver != "v0.0.0" { - return "", fmt.Errorf("%w prerelease version start with v0.0.0", ErrInvalidConstrains) + return "", k6build.NewError(ErrInvalidParameters, fmt.Errorf("prerelease version must start with v0.0.0")) } return prerelease, nil } From d2d95811770bc4aff8d61cb1e35a6f270f3aa647 Mon Sep 17 00:00:00 2001 From: Pablo Chacin Date: Tue, 26 Nov 2024 13:49:56 +0100 Subject: [PATCH 7/8] revert allowing nil or multiple reasons Signed-off-by: Pablo Chacin --- error.go | 22 ++++------ error_test.go | 71 ++++++++++++++++----------------- pkg/cache/client/client_test.go | 4 +- pkg/cache/server/server.go | 6 +-- 4 files changed, 48 insertions(+), 55 deletions(-) diff --git a/error.go b/error.go index ef45cf0..79a097f 100644 --- a/error.go +++ b/error.go @@ -6,6 +6,9 @@ import ( "fmt" ) +// ErrReasonUnknown signals the reason for an APIError in unknown +var ErrReasonUnknown = errors.New("reason unknown") + // Error represents an error returned by the build service // This custom error type facilitates extracting the reason of an error // by using errors.Unwrap method. @@ -21,11 +24,7 @@ type Error struct { // Error returns the Error as a string func (e *Error) Error() string { - reason := "" - if e.Reason != nil { - reason = fmt.Sprintf(": %s", e.Reason) - } - return fmt.Sprintf("%s%s", e.Err, reason) + return fmt.Sprintf("%s: %s", e.Err, e.Reason) } // Is returns true if the target error is the same as the Error or its reason @@ -64,16 +63,12 @@ func (e *Error) Unwrap() error { // MarshalJSON implements the json.Marshaler interface for the Error type func (e *Error) MarshalJSON() ([]byte, error) { - reason := "" - if e.Reason != nil { - reason = e.Reason.Error() - } return json.Marshal(&struct { Err string `json:"error,omitempty"` Reason string `json:"reason,omitempty"` }{ Err: e.Err.Error(), - Reason: reason, + Reason: e.Reason.Error(), }) } @@ -95,10 +90,9 @@ func (e *Error) UnmarshalJSON(data []byte) error { // NewError creates an Error from an error and a reason // If the reason is nil, ErrReasonUnknown is used -func NewError(err error, reasons ...error) *Error { - var reason error - if len(reasons) > 0 { - reason = NewError(reasons[0], reasons[1:]...) +func NewError(err error, reason error) *Error { + if reason == nil { + reason = ErrReasonUnknown } return &Error{ Err: err, diff --git a/error_test.go b/error_test.go index ea5d646..b66db3a 100644 --- a/error_test.go +++ b/error_test.go @@ -10,58 +10,57 @@ func Test_Error(t *testing.T) { t.Parallel() var ( - err = errors.New("error") - reason = errors.New("reason") - another = errors.New("another reason") + err = errors.New("error") + reason = errors.New("reason") ) testCases := []struct { - title string - err error - reasons []error - expect []error + title string + err error + reason error + expect []error }{ { - title: "error and reason", - err: err, - reasons: []error{reason}, - expect: []error{err, reason}, + title: "error and reason", + err: err, + reason: reason, + expect: []error{err, reason}, }, { - title: "error not reason", - err: err, - reasons: nil, - expect: []error{err}, + title: "error not reason", + err: err, + reason: nil, + expect: []error{err}, }, { - title: "multiple and reasons", - err: err, - reasons: []error{reason, another}, - expect: []error{err, reason, another}, + title: "multiple and reasons", + err: err, + reason: reason, + expect: []error{err, reason}, }, { - title: "wrapped err", - err: fmt.Errorf("wrapped %w", err), - reasons: []error{reason}, - expect: []error{err, reason}, + title: "wrapped err", + err: fmt.Errorf("wrapped %w", err), + reason: reason, + expect: []error{err, reason}, }, { - title: "wrapped reason", - err: err, - reasons: []error{fmt.Errorf("wrapped %w", reason)}, - expect: []error{err, reason}, + title: "wrapped reason", + err: err, + reason: fmt.Errorf("wrapped %w", reason), + expect: []error{err, reason}, }, { - title: "wrapped err in target", - err: err, - reasons: []error{reason}, - expect: []error{fmt.Errorf("wrapped %w", err)}, + title: "wrapped err in target", + err: err, + reason: reason, + expect: []error{fmt.Errorf("wrapped %w", err)}, }, { - title: "wrapped reason in target", - err: err, - reasons: []error{reason}, - expect: []error{fmt.Errorf("wrapped %w", reason)}, + title: "wrapped reason in target", + err: err, + reason: reason, + expect: []error{fmt.Errorf("wrapped %w", reason)}, }, } @@ -69,7 +68,7 @@ func Test_Error(t *testing.T) { t.Run(tc.title, func(t *testing.T) { t.Parallel() - err := NewError(tc.err, tc.reasons...) + err := NewError(tc.err, tc.reason) for _, expected := range tc.expect { if !errors.Is(err, expected) { t.Fatalf("expected %v got %v", expected, err) diff --git a/pkg/cache/client/client_test.go b/pkg/cache/client/client_test.go index 6ef9ba5..8448277 100644 --- a/pkg/cache/client/client_test.go +++ b/pkg/cache/client/client_test.go @@ -73,7 +73,7 @@ func TestCacheClientGet(t *testing.T) { title: "error accessing object", status: http.StatusInternalServerError, resp: &api.CacheResponse{ - Error: k6build.NewError(cache.ErrAccessingObject), + Error: k6build.NewError(cache.ErrAccessingObject, nil), Object: cache.Object{}, }, expectErr: ErrRequestFailed, @@ -121,7 +121,7 @@ func TestCacheClientStore(t *testing.T) { title: "error creating object", status: http.StatusInternalServerError, resp: &api.CacheResponse{ - Error: k6build.NewError(cache.ErrCreatingObject), + Error: k6build.NewError(cache.ErrCreatingObject, nil), Object: cache.Object{}, }, expectErr: ErrRequestFailed, diff --git a/pkg/cache/server/server.go b/pkg/cache/server/server.go index 2ac14c6..f865d8f 100644 --- a/pkg/cache/server/server.go +++ b/pkg/cache/server/server.go @@ -67,7 +67,7 @@ func (s *CacheServer) Get(w http.ResponseWriter, r *http.Request) { id := r.PathValue("id") if id == "" { w.WriteHeader(http.StatusBadRequest) - resp.Error = k6build.NewError(ErrInvalidRequest) + resp.Error = k6build.NewError(ErrInvalidRequest, nil) s.log.Error(resp.Error.Error()) _ = json.NewEncoder(w).Encode(resp) //nolint:errchkjson return @@ -82,7 +82,7 @@ func (s *CacheServer) Get(w http.ResponseWriter, r *http.Request) { s.log.Error(err.Error()) w.WriteHeader(http.StatusInternalServerError) } - resp.Error = k6build.NewError(err) + resp.Error = k6build.NewError(err, nil) _ = json.NewEncoder(w).Encode(resp) //nolint:errchkjson return @@ -123,7 +123,7 @@ func (s *CacheServer) Store(w http.ResponseWriter, r *http.Request) { object, err := s.cache.Store(context.Background(), id, r.Body) //nolint:contextcheck if err != nil { w.WriteHeader(http.StatusBadRequest) - resp.Error = k6build.NewError(err) + resp.Error = k6build.NewError(err, nil) return } From bcb1ac31f21e7ff96ed250c513afb9be8a0f516e Mon Sep 17 00:00:00 2001 From: Pablo Chacin Date: Tue, 26 Nov 2024 14:54:13 +0100 Subject: [PATCH 8/8] fix cache errors Signed-off-by: Pablo Chacin --- pkg/cache/api/api.go | 12 ++++++++++++ pkg/cache/client/client.go | 30 ++++++++++++------------------ pkg/cache/client/client_test.go | 10 +++++----- pkg/cache/server/server.go | 10 ++++------ 4 files changed, 33 insertions(+), 29 deletions(-) diff --git a/pkg/cache/api/api.go b/pkg/cache/api/api.go index 402e488..a0635ef 100644 --- a/pkg/cache/api/api.go +++ b/pkg/cache/api/api.go @@ -2,10 +2,22 @@ package api import ( + "errors" + "github.com/grafana/k6build" "github.com/grafana/k6build/pkg/cache" ) +var ( + // ErrInvalidRequest signals the request could not be processed + // due to erroneous parameters + ErrInvalidRequest = errors.New("invalid request") + // ErrRequestFailed signals the request failed, probably due to a network error + ErrRequestFailed = errors.New("request failed") + // ErrCacheAccess signals the access to the cache failed + ErrCacheAccess = errors.New("cache access failed") +) + // CacheResponse is the response to a cache server request type CacheResponse struct { Error *k6build.Error diff --git a/pkg/cache/client/client.go b/pkg/cache/client/client.go index 0d77d1b..d7fc56c 100644 --- a/pkg/cache/client/client.go +++ b/pkg/cache/client/client.go @@ -15,14 +15,8 @@ import ( "github.com/grafana/k6build/pkg/cache/api" ) -var ( - ErrAccessingServer = errors.New("making request") //nolint:revive - ErrInvalidConfig = errors.New("invalid configuration") //nolint:revive - ErrInvalidRequest = errors.New("invalid request") //nolint:revive - ErrInvalidResponse = errors.New("invalid response") //nolint:revive - ErrRequestFailed = errors.New("request failed") //nolint:revive - -) +// ErrInvalidConfig signals an error with the client configuration +var ErrInvalidConfig = errors.New("invalid configuration") // CacheClientConfig defines the configuration for accessing a remote cache service type CacheClientConfig struct { @@ -52,7 +46,7 @@ func (c *CacheClient) Get(_ context.Context, id string) (cache.Object, error) { // TODO: use http.Request resp, err := http.Get(url) //nolint:gosec,noctx if err != nil { - return cache.Object{}, k6build.NewError(ErrAccessingServer, err) + return cache.Object{}, k6build.NewError(api.ErrRequestFailed, err) } defer func() { _ = resp.Body.Close() @@ -62,17 +56,17 @@ func (c *CacheClient) Get(_ context.Context, id string) (cache.Object, error) { if resp.StatusCode == http.StatusNotFound { return cache.Object{}, cache.ErrObjectNotFound } - return cache.Object{}, k6build.NewError(ErrRequestFailed, fmt.Errorf("status %s", resp.Status)) + return cache.Object{}, k6build.NewError(api.ErrRequestFailed, fmt.Errorf("status %s", resp.Status)) } cacheResponse := api.CacheResponse{} err = json.NewDecoder(resp.Body).Decode(&cacheResponse) if err != nil { - return cache.Object{}, k6build.NewError(ErrInvalidResponse, err) + return cache.Object{}, k6build.NewError(api.ErrRequestFailed, err) } if cacheResponse.Error != nil { - return cache.Object{}, k6build.NewError(ErrRequestFailed, cacheResponse.Error) + return cache.Object{}, cacheResponse.Error } return cacheResponse.Object, nil @@ -87,23 +81,23 @@ func (c *CacheClient) Store(_ context.Context, id string, content io.Reader) (ca content, ) if err != nil { - return cache.Object{}, k6build.NewError(ErrAccessingServer, err) + return cache.Object{}, k6build.NewError(api.ErrRequestFailed, err) } defer func() { _ = resp.Body.Close() }() if resp.StatusCode != http.StatusOK { - return cache.Object{}, k6build.NewError(ErrRequestFailed, fmt.Errorf("status %s", resp.Status)) + return cache.Object{}, k6build.NewError(api.ErrRequestFailed, fmt.Errorf("status %s", resp.Status)) } cacheResponse := api.CacheResponse{} err = json.NewDecoder(resp.Body).Decode(&cacheResponse) if err != nil { - return cache.Object{}, k6build.NewError(ErrInvalidResponse, err) + return cache.Object{}, k6build.NewError(api.ErrRequestFailed, err) } if cacheResponse.Error != nil { - return cache.Object{}, k6build.NewError(ErrRequestFailed, cacheResponse.Error) + return cache.Object{}, cacheResponse.Error } return cacheResponse.Object, nil @@ -113,11 +107,11 @@ func (c *CacheClient) Store(_ context.Context, id string, content io.Reader) (ca func (c *CacheClient) Download(_ context.Context, object cache.Object) (io.ReadCloser, error) { resp, err := http.Get(object.URL) //nolint:noctx,bodyclose if err != nil { - return nil, k6build.NewError(ErrAccessingServer, err) + return nil, k6build.NewError(api.ErrRequestFailed, err) } if resp.StatusCode != http.StatusOK { - return nil, k6build.NewError(ErrRequestFailed, fmt.Errorf("status %s", resp.Status)) + return nil, k6build.NewError(api.ErrRequestFailed, fmt.Errorf("status %s", resp.Status)) } return resp.Request.Body, nil diff --git a/pkg/cache/client/client_test.go b/pkg/cache/client/client_test.go index 8448277..3460067 100644 --- a/pkg/cache/client/client_test.go +++ b/pkg/cache/client/client_test.go @@ -73,10 +73,10 @@ func TestCacheClientGet(t *testing.T) { title: "error accessing object", status: http.StatusInternalServerError, resp: &api.CacheResponse{ - Error: k6build.NewError(cache.ErrAccessingObject, nil), + Error: k6build.NewError(cache.ErrAccessingObject, k6build.ErrReasonUnknown), Object: cache.Object{}, }, - expectErr: ErrRequestFailed, + expectErr: api.ErrRequestFailed, }, } @@ -121,10 +121,10 @@ func TestCacheClientStore(t *testing.T) { title: "error creating object", status: http.StatusInternalServerError, resp: &api.CacheResponse{ - Error: k6build.NewError(cache.ErrCreatingObject, nil), + Error: k6build.NewError(cache.ErrCreatingObject, k6build.ErrReasonUnknown), Object: cache.Object{}, }, - expectErr: ErrRequestFailed, + expectErr: api.ErrRequestFailed, }, } @@ -165,7 +165,7 @@ func TestCacheClientDownload(t *testing.T) { { title: "error creating object", status: http.StatusInternalServerError, - expectErr: ErrRequestFailed, + expectErr: api.ErrRequestFailed, }, } diff --git a/pkg/cache/server/server.go b/pkg/cache/server/server.go index f865d8f..f2a6685 100644 --- a/pkg/cache/server/server.go +++ b/pkg/cache/server/server.go @@ -15,8 +15,6 @@ import ( "github.com/grafana/k6build/pkg/cache/api" ) -var ErrInvalidRequest = errors.New("invalid request") //nolint:revive - // CacheServer implements an http server that handles cache requests type CacheServer struct { baseURL string @@ -67,7 +65,7 @@ func (s *CacheServer) Get(w http.ResponseWriter, r *http.Request) { id := r.PathValue("id") if id == "" { w.WriteHeader(http.StatusBadRequest) - resp.Error = k6build.NewError(ErrInvalidRequest, nil) + resp.Error = k6build.NewError(api.ErrInvalidRequest, fmt.Errorf("object id is required")) s.log.Error(resp.Error.Error()) _ = json.NewEncoder(w).Encode(resp) //nolint:errchkjson return @@ -82,7 +80,7 @@ func (s *CacheServer) Get(w http.ResponseWriter, r *http.Request) { s.log.Error(err.Error()) w.WriteHeader(http.StatusInternalServerError) } - resp.Error = k6build.NewError(err, nil) + resp.Error = k6build.NewError(api.ErrCacheAccess, err) _ = json.NewEncoder(w).Encode(resp) //nolint:errchkjson return @@ -116,14 +114,14 @@ func (s *CacheServer) Store(w http.ResponseWriter, r *http.Request) { id := r.PathValue("id") if id == "" { w.WriteHeader(http.StatusBadRequest) - resp.Error = k6build.NewError(ErrInvalidRequest, fmt.Errorf("object id is required")) + resp.Error = k6build.NewError(api.ErrInvalidRequest, fmt.Errorf("object id is required")) return } object, err := s.cache.Store(context.Background(), id, r.Body) //nolint:contextcheck if err != nil { w.WriteHeader(http.StatusBadRequest) - resp.Error = k6build.NewError(err, nil) + resp.Error = k6build.NewError(api.ErrCacheAccess, err) return }