Skip to content

Commit

Permalink
feat: do not duplicate error message
Browse files Browse the repository at this point in the history
  • Loading branch information
byashimov committed Dec 17, 2024
1 parent 72397c2 commit 8c74e58
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 13 deletions.
4 changes: 2 additions & 2 deletions client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,14 +134,14 @@ func TestServiceCreateErrorsRetries(t *testing.T) {
{
Name: "with errors field, list",
ResponseBody: `{"message": "Something went wrong", "errors": ["oh!", "no!"]}`,
ErrorExpect: `[500 ServiceCreate]: Something went wrong: ["oh!","no!"]`,
ErrorExpect: `[500 ServiceCreate]: Something went wrong (["oh!","no!"])`,
RetryMax: 1,
CallsExpect: 2,
},
{
Name: "with errors field, string",
ResponseBody: `{"message": "Something went wrong", "errors": "wow!"}`,
ErrorExpect: `[500 ServiceCreate]: Something went wrong: "wow!"`,
ErrorExpect: `[500 ServiceCreate]: Something went wrong ("wow!")`,
RetryMax: 1,
CallsExpect: 2,
},
Expand Down
25 changes: 15 additions & 10 deletions error.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,24 @@ type Error struct {

// Error concatenates all the fields.
func (e Error) Error() string {
// Must not use `%q` here which will escape every quote in the string,
// which might break external substring checks
msg := fmt.Sprintf(`[%d %s]: %s`, e.Status, e.OperationID, e.Message)
if e.Errors == nil {
return msg
}
msg := e.Message
if e.Errors != nil {
// Appends Errors if they don't contain the message
// Otherwise, it will be duplicated
b, err := json.Marshal(e.Errors)
errMsg := string(b)
if err != nil {
errMsg = err.Error()
}

errMerged, err := json.Marshal(e.Errors)
if err != nil {
errMerged = []byte(err.Error())
if !strings.Contains(errMsg, msg) {
msg = fmt.Sprintf("%s (%s)", msg, errMsg)
}
}

return fmt.Sprintf(`%s: %s`, msg, errMerged)
// Must not use `%q` here which will escape every quote in the string,
// which might break external substring checks
return fmt.Sprintf(`[%d %s]: %s`, e.Status, e.OperationID, msg)
}

// IsNotFound returns true if the specified error has status 404
Expand Down
29 changes: 28 additions & 1 deletion error_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ func TestFromResponse(t *testing.T) {
}
]
}`),
expectStringer: `[404 UserAuth]: Account does not exist: [{"error_code":"account_not_found","message":"Account does not exist","status":404}]`,
expectStringer: `[404 UserAuth]: Account does not exist`,
expectBytes: nil,
expectErr: Error{
Message: "Account does not exist",
Expand Down Expand Up @@ -179,6 +179,33 @@ func TestFromResponse(t *testing.T) {
Status: http.StatusBadRequest,
},
},
{
name: "add errors, because message is different",
operationID: "UserAuth",
statusCode: http.StatusNotFound,
body: []byte(`{
"message": "Oh no!",
"errors": [
{
"error_code": "account_not_found",
"message": "Account does not exist",
"status": 404
}
]
}`),
expectStringer: `[404 UserAuth]: Oh no! ([{"error_code":"account_not_found","message":"Account does not exist","status":404}])`,
expectBytes: nil,
expectErr: Error{
Message: "Oh no!",
Errors: []any{map[string]any{
"error_code": "account_not_found",
"message": "Account does not exist",
"status": float64(404),
}},
OperationID: "UserAuth",
Status: http.StatusNotFound,
},
},
}

for _, opt := range cases {
Expand Down

0 comments on commit 8c74e58

Please sign in to comment.