Merged
refactor(api): extract decodeAPIError helper, eliminate 6x error decoding duplication #361
luojiyinAdd on Jul 8
refactor(api): extract decodeAPIError helper, eliminate 6x error decoding duplication #361
Merged
Merged fromluojiyin87/clirefactor/decode-api-errorintoatomgit-cli/climain
Total: 2 files changed+117-36
| @@ -117,12 +117,7 @@ func (c *Client) REST(method, path string, body interface{}, response interface{ | |||
| 117 | 117 | ||
| 118 | // Check for errors | 118 | // Check for errors |
| 119 | if resp.StatusCode >= 400 { | 119 | if resp.StatusCode >= 400 { |
| 120 | - var apiErr APIError | 120 | + return decodeAPIError(respBody, resp.StatusCode, resp.Status) |
| 121 | - if err := json.Unmarshal(respBody, &apiErr); err == nil && (apiErr.Message != "" || apiErr.ErrorMessage != "" || apiErr.ErrorName != "" || apiErr.ErrorCodeName != "") { | ||
| 122 | - apiErr.StatusCode = resp.StatusCode | ||
| 123 | - return &apiErr | ||
| 124 | - } | ||
| 125 | - return &APIError{StatusCode: resp.StatusCode, Message: resp.Status} | ||
| 126 | } | 121 | } |
| 127 | 122 | ||
| 128 | // Parse response | 123 | // Parse response |
| @@ -177,12 +172,7 @@ func (c *Client) RawREST(method, endpoint string, body io.Reader, headers map[st | |||
| 177 | return nil, fmt.Errorf("failed to read response: %w", err) | 172 | return nil, fmt.Errorf("failed to read response: %w", err) |
| 178 | } | 173 | } |
| 179 | if resp.StatusCode >= 400 { | 174 | if resp.StatusCode >= 400 { |
| 180 | - var apiErr APIError | 175 | + return nil, decodeAPIError(respBody, resp.StatusCode, resp.Status) |
| 181 | - if err := json.Unmarshal(respBody, &apiErr); err == nil && (apiErr.Message != "" || apiErr.ErrorMessage != "" || apiErr.ErrorName != "" || apiErr.ErrorCodeName != "") { | ||
| 182 | - apiErr.StatusCode = resp.StatusCode | ||
| 183 | - return nil, &apiErr | ||
| 184 | - } | ||
| 185 | - return nil, &APIError{StatusCode: resp.StatusCode, Message: resp.Status} | ||
| 186 | } | 176 | } |
| 187 | 177 | ||
| 188 | return &RawResponse{ | 178 | return &RawResponse{ |
| @@ -243,12 +233,7 @@ func (c *Client) GetText(path string) (string, error) { | |||
| 243 | } | 233 | } |
| 244 | 234 | ||
| 245 | if resp.StatusCode >= 400 { | 235 | if resp.StatusCode >= 400 { |
| 246 | - var apiErr APIError | 236 | + return "", decodeAPIError(respBody, resp.StatusCode, resp.Status) |
| 247 | - if err := json.Unmarshal(respBody, &apiErr); err == nil && (apiErr.Message != "" || apiErr.ErrorMessage != "" || apiErr.ErrorName != "" || apiErr.ErrorCodeName != "") { | ||
| 248 | - apiErr.StatusCode = resp.StatusCode | ||
| 249 | - return "", &apiErr | ||
| 250 | - } | ||
| 251 | - return "", &APIError{StatusCode: resp.StatusCode, Message: resp.Status} | ||
| 252 | } | 237 | } |
| 253 | 238 | ||
| 254 | return string(respBody), nil | 239 | return string(respBody), nil |
| @@ -288,12 +273,7 @@ func (c *Client) PostForm(path string, formValues url.Values, response interface | |||
| 288 | } | 273 | } |
| 289 | 274 | ||
| 290 | if resp.StatusCode >= 400 { | 275 | if resp.StatusCode >= 400 { |
| 291 | - var apiErr APIError | 276 | + return decodeAPIError(respBody, resp.StatusCode, resp.Status) |
| 292 | - if err := json.Unmarshal(respBody, &apiErr); err == nil && (apiErr.Message != "" || apiErr.ErrorMessage != "" || apiErr.ErrorName != "" || apiErr.ErrorCodeName != "") { | ||
| 293 | - apiErr.StatusCode = resp.StatusCode | ||
| 294 | - return &apiErr | ||
| 295 | - } | ||
| 296 | - return &APIError{StatusCode: resp.StatusCode, Message: resp.Status} | ||
| 297 | } | 277 | } |
| 298 | 278 | ||
| 299 | if response != nil && len(respBody) > 0 { | 279 | if response != nil && len(respBody) > 0 { |
| @@ -353,12 +333,7 @@ func (c *Client) PatchForm(path string, formValues url.Values, response interfac | |||
| 353 | 333 | ||
| 354 | // Check for errors | 334 | // Check for errors |
| 355 | if resp.StatusCode >= 400 { | 335 | if resp.StatusCode >= 400 { |
| 356 | - var apiErr APIError | 336 | + return decodeAPIError(respBody, resp.StatusCode, resp.Status) |
| 357 | - if err := json.Unmarshal(respBody, &apiErr); err == nil && (apiErr.Message != "" || apiErr.ErrorMessage != "" || apiErr.ErrorName != "" || apiErr.ErrorCodeName != "") { | ||
| 358 | - apiErr.StatusCode = resp.StatusCode | ||
| 359 | - return &apiErr | ||
| 360 | - } | ||
| 361 | - return &APIError{StatusCode: resp.StatusCode, Message: resp.Status} | ||
| 362 | } | 337 | } |
| 363 | 338 | ||
| 364 | // Parse response | 339 | // Parse response |
| @@ -442,12 +417,7 @@ func (c *Client) UploadAsset(path, filename string, content []byte, contentType | |||
| 442 | 417 | ||
| 443 | // Check for errors | 418 | // Check for errors |
| 444 | if resp.StatusCode >= 400 { | 419 | if resp.StatusCode >= 400 { |
| 445 | - var apiErr APIError | 420 | + return nil, decodeAPIError(respBody, resp.StatusCode, resp.Status) |
| 446 | - if err := json.Unmarshal(respBody, &apiErr); err == nil && (apiErr.Message != "" || apiErr.ErrorMessage != "" || apiErr.ErrorName != "" || apiErr.ErrorCodeName != "") { | ||
| 447 | - apiErr.StatusCode = resp.StatusCode | ||
| 448 | - return nil, &apiErr | ||
| 449 | - } | ||
| 450 | - return nil, &APIError{StatusCode: resp.StatusCode, Message: resp.Status} | ||
| 451 | } | 421 | } |
| 452 | 422 | ||
| 453 | // Parse response | 423 | // Parse response |
| @@ -487,6 +457,19 @@ func (e *APIError) Error() string { | |||
| 487 | return fmt.Sprintf("HTTP %d: %s", e.StatusCode, msg) | 457 | return fmt.Sprintf("HTTP %d: %s", e.StatusCode, msg) |
| 488 | } | 458 | } |
| 489 | 459 | ||
| 460 | +// decodeAPIError attempts to decode an API error from the response body. | ||
| 461 | +// Returns *APIError if the body contains a recognized error structure, | ||
| 462 | +// otherwise returns a generic error with the HTTP status. | ||
| 463 | +func decodeAPIError(respBody []byte, statusCode int, status string) error { | ||
| 464 | + var apiErr APIError | ||
| 465 | + if err := json.Unmarshal(respBody, &apiErr); err == nil && | ||
| 466 | + (apiErr.Message != "" || apiErr.ErrorMessage != "" || apiErr.ErrorName != "" || apiErr.ErrorCodeName != "") { | ||
| 467 | + apiErr.StatusCode = statusCode | ||
| 468 | + return &apiErr | ||
| 469 | + } | ||
| 470 | + return &APIError{StatusCode: statusCode, Message: status} | ||
| 471 | +} | ||
| 472 | + | ||
| 490 | // BuildURL builds a URL with path parameters | 473 | // BuildURL builds a URL with path parameters |
| 491 | func BuildURL(base string, params map[string]string) string { | 474 | func BuildURL(base string, params map[string]string) string { |
| 492 | for k, v := range params { | 475 | for k, v := range params { |
| @@ -202,6 +202,104 @@ func contains(s, substr string) bool { | |||
| 202 | return false | 202 | return false |
| 203 | } | 203 | } |
| 204 | 204 | ||
| 205 | +func TestDecodeAPIError(t *testing.T) { | ||
| 206 | + tests := []struct { | ||
| 207 | + name string | ||
| 208 | + body string | ||
| 209 | + statusCode int | ||
| 210 | + status string | ||
| 211 | + wantMsg string | ||
| 212 | + wantAPIErr bool | ||
| 213 | + }{ | ||
| 214 | + { | ||
| 215 | + name: "message field", | ||
| 216 | + body: `{"message":"Not Found"}`, | ||
| 217 | + statusCode: 404, | ||
| 218 | + status: "404 Not Found", | ||
| 219 | + wantMsg: "Not Found", | ||
| 220 | + wantAPIErr: true, | ||
| 221 | + }, | ||
| 222 | + { | ||
| 223 | + name: "error_message field", | ||
| 224 | + body: `{"error_message":"Internal Server Error"}`, | ||
| 225 | + statusCode: 500, | ||
| 226 | + status: "500 Internal Server Error", | ||
| 227 | + wantMsg: "Internal Server Error", | ||
| 228 | + wantAPIErr: true, | ||
| 229 | + }, | ||
| 230 | + { | ||
| 231 | + name: "error field", | ||
| 232 | + body: `{"error":"unauthorized"}`, | ||
| 233 | + statusCode: 401, | ||
| 234 | + status: "401 Unauthorized", | ||
| 235 | + wantMsg: "unknown error", | ||
| 236 | + wantAPIErr: true, | ||
| 237 | + }, | ||
| 238 | + { | ||
| 239 | + name: "error_code_name field", | ||
| 240 | + body: `{"error_code_name":"NOT_FOUND"}`, | ||
| 241 | + statusCode: 404, | ||
| 242 | + status: "404 Not Found", | ||
| 243 | + wantMsg: "unknown error", | ||
| 244 | + wantAPIErr: true, | ||
| 245 | + }, | ||
| 246 | + { | ||
| 247 | + name: "non-JSON body returns generic error", | ||
| 248 | + body: `plain text error`, | ||
| 249 | + statusCode: 502, | ||
| 250 | + status: "502 Bad Gateway", | ||
| 251 | + wantMsg: "502 Bad Gateway", | ||
| 252 | + wantAPIErr: true, | ||
| 253 | + }, | ||
| 254 | + { | ||
| 255 | + name: "JSON but no error fields", | ||
| 256 | + body: `{"data":"ok"}`, | ||
| 257 | + statusCode: 400, | ||
| 258 | + status: "400 Bad Request", | ||
| 259 | + wantMsg: "400 Bad Request", | ||
| 260 | + wantAPIErr: true, | ||
| 261 | + }, | ||
| 262 | + { | ||
| 263 | + name: "multiple error fields", | ||
| 264 | + body: `{"message":"Forbidden","error":"forbidden","error_code":403}`, | ||
| 265 | + statusCode: 403, | ||
| 266 | + status: "403 Forbidden", | ||
| 267 | + wantMsg: "Forbidden", | ||
| 268 | + wantAPIErr: true, | ||
| 269 | + }, | ||
| 270 | + } | ||
| 271 | + | ||
| 272 | + for _, tt := range tests { | ||
| 273 | + t.Run(tt.name, func(t *testing.T) { | ||
| 274 | + err := decodeAPIError([]byte(tt.body), tt.statusCode, tt.status) | ||
| 275 | + if err == nil { | ||
| 276 | + t.Fatal("decodeAPIError() returned nil") | ||
| 277 | + } | ||
| 278 | + | ||
| 279 | + if tt.wantAPIErr { | ||
| 280 | + var apiErr *APIError | ||
| 281 | + if !errors.As(err, &apiErr) { | ||
| 282 | + t.Fatalf("expected *APIError, got %T", err) | ||
| 283 | + } | ||
| 284 | + if apiErr.StatusCode != tt.statusCode { | ||
| 285 | + t.Errorf("StatusCode = %d, want %d", apiErr.StatusCode, tt.statusCode) | ||
| 286 | + } | ||
| 287 | + if !contains(apiErr.Error(), tt.wantMsg) { | ||
| 288 | + t.Errorf("Error() = %q, want containing %q", apiErr.Error(), tt.wantMsg) | ||
| 289 | + } | ||
| 290 | + } else { | ||
| 291 | + var apiErr *APIError | ||
| 292 | + if errors.As(err, &apiErr) { | ||
| 293 | + t.Errorf("expected generic error, got *APIError") | ||
| 294 | + } | ||
| 295 | + if !contains(err.Error(), tt.wantMsg) { | ||
| 296 | + t.Errorf("Error() = %q, want containing %q", err.Error(), tt.wantMsg) | ||
| 297 | + } | ||
| 298 | + } | ||
| 299 | + }) | ||
| 300 | + } | ||
| 301 | +} | ||
| 302 | + | ||
| 205 | func TestRawREST(t *testing.T) { | 303 | func TestRawREST(t *testing.T) { |
| 206 | handler := testutil.MockAPIHandler() | 304 | handler := testutil.MockAPIHandler() |
| 207 | mockClient := testutil.NewTestHTTPClient(handler) | 305 | mockClient := testutil.NewTestHTTPClient(handler) |