diff --git a/README.md b/README.md index 8ba26f5..b73742f 100644 --- a/README.md +++ b/README.md @@ -177,7 +177,7 @@ user, err := c.CreateUser(ctx, "site-name", &unifi.User{ ## Plans - [ ] Increase API coverage, or modify code generation to rely on the official UniFi Controller API specifications -- [ ] Improve error handling (currently only basic error handling is implemented and some of the errors are swallowed) +- [x] Improve error handling (currently only basic error handling is implemented and error details are not propagated) - [x] Improve client code for better usability - [x] Support API Key authentication - [ ] Generate client code for currently generated API structures, for use within or outside the Terraform provider diff --git a/unifi/unifi.go b/unifi/unifi.go index 4fc391e..4a4f12d 100644 --- a/unifi/unifi.go +++ b/unifi/unifi.go @@ -42,57 +42,15 @@ const ( UserAgentHeader = "User-Agent" AcceptHeader = "Accept" ContentTypeHeader = "Content-Type" -) -var ( - ErrAuthenticationFailed = errors.New("authentication failed") - ErrNotFound = errors.New("not found") -) - -type APIError struct { - RC string - Message string -} - -func (err *APIError) Error() string { - return err.Message -} - -func (err *APIError) Is(target error) bool { - var apiError *APIError - if errors.As(target, &apiError) { - if err.RC == apiError.RC && err.Message == apiError.Message { - return true - } - } - return false -} - -type Meta struct { - RC string `json:"rc"` - Message string `json:"msg"` -} - -func (m *Meta) error() error { - if m.RC != "ok" { - return &APIError{ - RC: m.RC, - Message: m.Message, - } - } - - return nil -} - -type validationMode string - -const ( SoftValidation validationMode = "soft" HardValidation validationMode = "hard" DisableValidation validationMode = "disable" DefaultValidation validationMode = SoftValidation // TODO: change to hard in next major version ) +type validationMode string + type ClientConfig struct { URL string `validate:"required,http_url"` APIKey string `validate:"required_without_all=User Pass"` @@ -147,8 +105,8 @@ var ( type ServerInfo struct { Up bool `json:"up"` - ServerVersion string `fake:"{appversion}" json:"server_version"` - UUID string `fake:"{uuid}" json:"uuid"` + ServerVersion string `json:"server_version"` + UUID string `json:"uuid"` } type HttpCustomizer func(transport *http.Transport) error @@ -214,39 +172,6 @@ type ResponseErrorHandler interface { HandleError(resp *http.Response) error } -type DefaultResponseErrorHandler struct{} - -func (d *DefaultResponseErrorHandler) HandleError(resp *http.Response) error { - switch resp.StatusCode { - case http.StatusOK: - return nil - case http.StatusNotFound: - return ErrNotFound - case http.StatusUnauthorized: - return ErrAuthenticationFailed - } - errBody := struct { - Meta Meta `json:"Meta"` - Data []struct { - Meta Meta `json:"Meta"` - } `json:"data"` - }{} - if err := json.NewDecoder(resp.Body).Decode(&errBody); err != nil { - return err - } - var apiErr error - if len(errBody.Data) > 0 && errBody.Data[0].Meta.RC == "error" { - // check first error in data, should we look for more than one? - apiErr = errBody.Data[0].Meta.error() - } - if apiErr == nil { - apiErr = errBody.Meta.error() - } - - // TODO: check rc in addition to status code? - return fmt.Errorf("%w (%s) for %s %s", apiErr, resp.Status, resp.Request.Method, resp.Request.URL.String()) -} - // NewClient creates a http.Client with authenticated cookies. // Used to make additional, authenticated requests to the APIs. // Start here. @@ -258,9 +183,6 @@ func NewClient(config *ClientConfig) (*Client, error) { if err := v.Validate(config); err != nil { return nil, fmt.Errorf("failed validating config: %w", err) } - if config.ValidationMode == "" { - config.ValidationMode = DefaultValidation - } u, err := newUnifi(config, v) if err != nil { return nil, fmt.Errorf("failed creating unifi client: %w", err) @@ -351,6 +273,9 @@ func newUnifi(config *ClientConfig, v *validator) (*Client, error) { } else { errorHandler = &DefaultResponseErrorHandler{} } + if config.ValidationMode == "" { + config.ValidationMode = DefaultValidation + } u := &Client{ BaseURL: baseURL, config: config, diff --git a/unifi/unifi_errors.go b/unifi/unifi_errors.go new file mode 100644 index 0000000..c59760d --- /dev/null +++ b/unifi/unifi_errors.go @@ -0,0 +1,169 @@ +package unifi + +import ( + "encoding/json" + "errors" + "fmt" + "net/http" + "strings" +) + +var ErrNotFound = errors.New("not found") + +// TODO old-style error handling to be removed in future versions. +type APIError struct { + RC string + Message string +} + +func (err *APIError) Error() string { + return err.Message +} + +func (err *APIError) Is(target error) bool { + var apiError *APIError + if errors.As(target, &apiError) { + if err.RC == apiError.RC && err.Message == apiError.Message { + return true + } + } + return false +} + +type Meta struct { + RC string `json:"rc"` + Message string `json:"msg"` +} + +func (m *Meta) error() error { + if m.RC != "ok" { + return &APIError{ + RC: m.RC, + Message: m.Message, + } + } + + return nil +} + +type DefaultResponseErrorHandler struct{} + +type apiV2ResponseError struct { + Code string `json:"code"` + ErrorCode int `json:"errorCode"` + Message string `json:"message"` + Details apiV2ResponseErrorDetails `json:"details"` +} + +type apiV2ResponseErrorDetails struct { + // probably there are more fields, but I didn't get any response with more fields + InvalidFields []string `json:"invalid_fields"` +} + +type apiV1ResponseError struct { + Meta Meta `json:"Meta"` + Data []apiV1ResponseErrorData `json:"data"` +} + +type apiV1ResponseErrorData struct { + Meta Meta `json:"Meta"` + ValidationError ServerValidationError `json:"validationError"` + RC string `json:"rc"` + Message string `json:"msg"` +} + +type apiResponseError struct { + apiV1ResponseError + apiV2ResponseError +} + +type ServerValidationError struct { + Field string `json:"field"` + Pattern string `json:"pattern"` +} + +type ServerErrorDetails struct { + Message string + ValidationError ServerValidationError +} + +type ServerError struct { + StatusCode int + RequestMethod string + RequestURL string + Message string + ErrorCode string + Details []ServerErrorDetails +} + +func (s *ServerError) Error() string { + var b strings.Builder + b.WriteString(fmt.Sprintf("Server error (%d) for %s %s: %s", s.StatusCode, s.RequestMethod, s.RequestURL, s.Message)) + for _, detail := range s.Details { + b.WriteString("\n") + if detail.Message != "" { + b.WriteString(detail.Message + ": ") + } + if detail.ValidationError.Field != "" && detail.ValidationError.Pattern != "" { + b.WriteString(fmt.Sprintf("field '%s' should match '%s'", detail.ValidationError.Field, detail.ValidationError.Pattern)) + } else if detail.ValidationError.Field != "" { + b.WriteString(fmt.Sprintf("field '%s' is invalid", detail.ValidationError.Field)) + } else if detail.ValidationError.Pattern != "" { + b.WriteString(fmt.Sprintf("field should match '%s'", detail.ValidationError.Pattern)) + } + } + return b.String() +} + +func parseApiV2Error(err apiV2ResponseError, serverError *ServerError) { + serverError.Message = err.Message + serverError.ErrorCode = err.Code + for _, field := range err.Details.InvalidFields { + details := ServerErrorDetails{} + details.ValidationError.Field = field + serverError.Details = append(serverError.Details, details) + } +} + +func parseApiV1Error(err apiV1ResponseError, serverError *ServerError) { + for _, d := range err.Data { + if d.Meta.RC == "error" || d.RC == "error" { + details := ServerErrorDetails{} + details.Message = d.Message + if details.Message == "" { + details.Message = d.Meta.Message + } + if d.ValidationError.Field != "" || d.ValidationError.Pattern != "" { + details.ValidationError = d.ValidationError + } + serverError.Details = append(serverError.Details, details) + } + } + if serverError.Message == "" { + serverError.Message = err.Meta.Message + } + serverError.ErrorCode = err.Meta.RC +} + +func (d *DefaultResponseErrorHandler) HandleError(resp *http.Response) error { + if resp.StatusCode >= 200 && resp.StatusCode < 300 { + return nil + } + + var errBody apiResponseError + if err := json.NewDecoder(resp.Body).Decode(&errBody); err != nil { + return err + } + serverError := ServerError{ + StatusCode: resp.StatusCode, + RequestMethod: resp.Request.Method, + RequestURL: resp.Request.URL.String(), + } + if errBody.apiV2ResponseError.Code != "" || errBody.apiV2ResponseError.Message != "" { + parseApiV2Error(errBody.apiV2ResponseError, &serverError) + } else { + parseApiV1Error(errBody.apiV1ResponseError, &serverError) + } + + return &serverError +} diff --git a/unifi/unifi_errors_test.go b/unifi/unifi_errors_test.go new file mode 100644 index 0000000..7918708 --- /dev/null +++ b/unifi/unifi_errors_test.go @@ -0,0 +1,189 @@ +package unifi //nolint: testpackage + +import ( + "bytes" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestServerError_Error(t *testing.T) { + t.Parallel() + tests := []struct { + name string + input ServerError + expected string + }{ + { + name: "Error with message and validation error", + input: ServerError{ + StatusCode: 400, + RequestMethod: "GET", + RequestURL: "http://example.com", + Message: "Bad Request", + Details: []ServerErrorDetails{ + { + Message: "Invalid input", + ValidationError: ServerValidationError{ + Field: "username", + Pattern: "^[a-zA-Z0-9]+$", + }, + }, + }, + }, + expected: "Server error (400) for GET http://example.com: Bad Request\nInvalid input: field 'username' should match '^[a-zA-Z0-9]+$'", + }, + { + name: "Error with message only", + input: ServerError{ + StatusCode: 404, + RequestMethod: "POST", + RequestURL: "http://example.com", + Message: "Not Found", + }, + expected: "Server error (404) for POST http://example.com: Not Found", + }, + { + name: "Error with multiple validation errors", + input: ServerError{ + StatusCode: 422, + RequestMethod: "PUT", + RequestURL: "http://example.com", + Message: "Unprocessable Entity", + Details: []ServerErrorDetails{ + { + Message: "Invalid username", + ValidationError: ServerValidationError{ + Field: "username", + Pattern: "^[a-zA-Z0-9]+$", + }, + }, + { + Message: "Invalid email", + ValidationError: ServerValidationError{ + Field: "email", + Pattern: "^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\\.[a-zA-Z]{2,}$", + }, + }, + }, + }, + expected: "Server error (422) for PUT http://example.com: Unprocessable Entity\nInvalid username: field 'username' should match '^[a-zA-Z0-9]+$'\nInvalid email: field 'email' should match '^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\\.[a-zA-Z]{2,}$'", + }, + { + name: "Error with no details", + input: ServerError{ + StatusCode: 500, + RequestMethod: "DELETE", + RequestURL: "http://example.com", + Message: "Internal Server Error", + }, + expected: "Server error (500) for DELETE http://example.com: Internal Server Error", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + a := assert.New(t) + a.Equal(tt.expected, tt.input.Error()) + }) + } +} + +func TestHandleError(t *testing.T) { + t.Parallel() + tests := []struct { + name string + responseBody string + statusCode int + expectedError *ServerError + }{ + { + name: "V2 error with invalid fields", + responseBody: `{ + "code": "error_code", + "message": "error message", + "details": { + "invalid_fields": ["field1", "field2"] + } + }`, + statusCode: 400, + expectedError: &ServerError{ + StatusCode: 400, + RequestMethod: "GET", + RequestURL: "http://example.com", + Message: "error message", + ErrorCode: "error_code", + Details: []ServerErrorDetails{ + {ValidationError: ServerValidationError{Field: "field1"}}, + {ValidationError: ServerValidationError{Field: "field2"}}, + }, + }, + }, + { + name: "V1 error with validation error", + responseBody: `{ + "Meta": {"rc": "error", "msg": "meta error message"}, + "data": [{ + "Meta": {"rc": "error", "msg": "data meta error message"}, + "validationError": {"field": "field1", "pattern": "pattern1"}, + "rc": "error", + "msg": "data error message" + }] + }`, + statusCode: 400, + expectedError: &ServerError{ + StatusCode: 400, + RequestMethod: "GET", + RequestURL: "http://example.com", + Message: "meta error message", + ErrorCode: "error", + Details: []ServerErrorDetails{ + { + Message: "data error message", + ValidationError: ServerValidationError{ + Field: "field1", + Pattern: "pattern1", + }, + }, + }, + }, + }, + { + name: "No error", + responseBody: `{"Meta": {"rc": "ok", "msg": "success"}}`, + statusCode: 200, + expectedError: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + a := assert.New(t) + handler := &DefaultResponseErrorHandler{} + + // Create a new HTTP response recorder + recorder := httptest.NewRecorder() + recorder.WriteHeader(tt.statusCode) + recorder.Body = bytes.NewBufferString(tt.responseBody) + + // Create a new HTTP request + req := httptest.NewRequest(http.MethodGet, "http://example.com", nil) + recorder.Result().Request = req + + // Call the HandleError function + err := handler.HandleError(recorder.Result()) + + if tt.expectedError == nil { + require.NoError(t, err) + } else { + require.Error(t, err) + a.EqualValues(tt.expectedError, err) + } + }) + } +} diff --git a/unifi/user.go b/unifi/user.go index 0690590..c5a609a 100644 --- a/unifi/user.go +++ b/unifi/user.go @@ -58,6 +58,7 @@ func (c *Client) CreateUser(ctx context.Context, site string, d *User) (*User, e return nil, errors.New("malformed group response") } + // TODO verify if this is still needed cause it's the only place where old-style error handling is used if err := respBody.Data[0].Meta.error(); err != nil { return nil, err }