From b3d787c97b2dc548e2a4b036b48c9d498bcacefe Mon Sep 17 00:00:00 2001 From: dailz Date: Fri, 10 Apr 2026 13:43:17 +0800 Subject: [PATCH] fix(slurm): parse structured errors from non-2xx Slurm API responses Replace ErrorResponse with SlurmAPIError that extracts structured errors/warnings from JSON body when Slurm returns non-2xx (e.g. 404 with valid JSON). Add IsNotFound helper for fallback logic. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- internal/slurm/client_test.go | 8 +- internal/slurm/errors.go | 73 +++++++++-- internal/slurm/errors_test.go | 220 ++++++++++++++++++++++++++++++++++ 3 files changed, 284 insertions(+), 17 deletions(-) create mode 100644 internal/slurm/errors_test.go diff --git a/internal/slurm/client_test.go b/internal/slurm/client_test.go index 18f0725..aba6222 100644 --- a/internal/slurm/client_test.go +++ b/internal/slurm/client_test.go @@ -137,11 +137,11 @@ func TestClient_ErrorHandling(t *testing.T) { t.Fatal("expected error for 500 response") } - errorResp, ok := err.(*ErrorResponse) + errorResp, ok := err.(*SlurmAPIError) if !ok { - t.Fatalf("expected *ErrorResponse, got %T", err) + t.Fatalf("expected *SlurmAPIError, got %T", err) } - if errorResp.Response.StatusCode != 500 { - t.Errorf("expected status 500, got %d", errorResp.Response.StatusCode) + if errorResp.StatusCode != 500 { + t.Errorf("expected status 500, got %d", errorResp.StatusCode) } } diff --git a/internal/slurm/errors.go b/internal/slurm/errors.go index 3e5e537..85c5aa5 100644 --- a/internal/slurm/errors.go +++ b/internal/slurm/errors.go @@ -1,38 +1,85 @@ package slurm import ( + "encoding/json" "fmt" "io" "net/http" ) -// ErrorResponse represents an error returned by the Slurm REST API. -type ErrorResponse struct { - Response *http.Response - Message string +// errorResponseFields is used to parse errors/warnings from a Slurm API error body. +type errorResponseFields struct { + Errors OpenapiErrors `json:"errors,omitempty"` + Warnings OpenapiWarnings `json:"warnings,omitempty"` } -func (r *ErrorResponse) Error() string { +// SlurmAPIError represents a structured error returned by the Slurm REST API. +// It captures both the HTTP details and the parsed Slurm error array when available. +type SlurmAPIError struct { + Response *http.Response + StatusCode int + Errors OpenapiErrors + Warnings OpenapiWarnings + Message string // raw body fallback when JSON parsing fails +} + +func (e *SlurmAPIError) Error() string { + if len(e.Errors) > 0 { + first := e.Errors[0] + detail := "" + if first.Error != nil { + detail = *first.Error + } else if first.Description != nil { + detail = *first.Description + } + if detail != "" { + return fmt.Sprintf("%v %v: %d %s", + e.Response.Request.Method, e.Response.Request.URL, + e.StatusCode, detail) + } + } return fmt.Sprintf("%v %v: %d %s", - r.Response.Request.Method, r.Response.Request.URL, - r.Response.StatusCode, r.Message) + e.Response.Request.Method, e.Response.Request.URL, + e.StatusCode, e.Message) +} + +// IsNotFound reports whether err is a SlurmAPIError with HTTP 404 status. +func IsNotFound(err error) bool { + if apiErr, ok := err.(*SlurmAPIError); ok { + return apiErr.StatusCode == http.StatusNotFound + } + return false } // CheckResponse checks the API response for errors. It returns nil if the // response is a 2xx status code. For non-2xx codes, it reads the response -// body and returns an ErrorResponse. +// body, attempts to parse structured Slurm errors, and returns a SlurmAPIError. func CheckResponse(r *http.Response) error { if c := r.StatusCode; c >= 200 && c <= 299 { return nil } - errorResponse := &ErrorResponse{Response: r} data, err := io.ReadAll(r.Body) if err != nil || len(data) == 0 { - errorResponse.Message = r.Status - return errorResponse + return &SlurmAPIError{ + Response: r, + StatusCode: r.StatusCode, + Message: r.Status, + } } - errorResponse.Message = string(data) - return errorResponse + apiErr := &SlurmAPIError{ + Response: r, + StatusCode: r.StatusCode, + Message: string(data), + } + + // Try to extract structured errors/warnings from JSON body. + var fields errorResponseFields + if json.Unmarshal(data, &fields) == nil { + apiErr.Errors = fields.Errors + apiErr.Warnings = fields.Warnings + } + + return apiErr } diff --git a/internal/slurm/errors_test.go b/internal/slurm/errors_test.go new file mode 100644 index 0000000..7dd70bc --- /dev/null +++ b/internal/slurm/errors_test.go @@ -0,0 +1,220 @@ +package slurm + +import ( + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +func TestCheckResponse(t *testing.T) { + tests := []struct { + name string + statusCode int + body string + wantErr bool + wantStatusCode int + wantErrors int + wantWarnings int + wantMessageContains string + }{ + { + name: "2xx response returns nil", + statusCode: http.StatusOK, + body: `{"meta":{}}`, + wantErr: false, + }, + { + name: "404 with valid JSON error body", + statusCode: http.StatusNotFound, + body: `{"errors":[{"description":"Unable to query JobId=198","error_number":2017,"error":"Invalid job id specified","source":"_handle_job_get"}],"warnings":[]}`, + wantErr: true, + wantStatusCode: 404, + wantErrors: 1, + wantWarnings: 0, + wantMessageContains: "Invalid job id specified", + }, + { + name: "500 with non-JSON body", + statusCode: http.StatusInternalServerError, + body: "internal server error", + wantErr: true, + wantStatusCode: 500, + wantErrors: 0, + wantWarnings: 0, + wantMessageContains: "internal server error", + }, + { + name: "503 with empty body returns http.Status text", + statusCode: http.StatusServiceUnavailable, + body: "", + wantErr: true, + wantStatusCode: 503, + wantErrors: 0, + wantWarnings: 0, + wantMessageContains: "503 Service Unavailable", + }, + { + name: "400 with valid JSON but empty errors array", + statusCode: http.StatusBadRequest, + body: `{"errors":[],"warnings":[]}`, + wantErr: true, + wantStatusCode: 400, + wantErrors: 0, + wantWarnings: 0, + wantMessageContains: `{"errors":[],"warnings":[]}`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rec := httptest.NewRecorder() + rec.WriteHeader(tt.statusCode) + if tt.body != "" { + rec.Body.WriteString(tt.body) + } + + err := CheckResponse(rec.Result()) + + if (err != nil) != tt.wantErr { + t.Fatalf("CheckResponse() error = %v, wantErr %v", err, tt.wantErr) + } + if !tt.wantErr { + return + } + + apiErr, ok := err.(*SlurmAPIError) + if !ok { + t.Fatalf("expected *SlurmAPIError, got %T", err) + } + + if apiErr.StatusCode != tt.wantStatusCode { + t.Errorf("StatusCode = %d, want %d", apiErr.StatusCode, tt.wantStatusCode) + } + if len(apiErr.Errors) != tt.wantErrors { + t.Errorf("len(Errors) = %d, want %d", len(apiErr.Errors), tt.wantErrors) + } + if len(apiErr.Warnings) != tt.wantWarnings { + t.Errorf("len(Warnings) = %d, want %d", len(apiErr.Warnings), tt.wantWarnings) + } + if !strings.Contains(apiErr.Message, tt.wantMessageContains) { + t.Errorf("Message = %q, want to contain %q", apiErr.Message, tt.wantMessageContains) + } + }) + } +} + +func TestIsNotFound(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + { + name: "404 SlurmAPIError returns true", + err: &SlurmAPIError{StatusCode: http.StatusNotFound}, + want: true, + }, + { + name: "500 SlurmAPIError returns false", + err: &SlurmAPIError{StatusCode: http.StatusInternalServerError}, + want: false, + }, + { + name: "200 SlurmAPIError returns false", + err: &SlurmAPIError{StatusCode: http.StatusOK}, + want: false, + }, + { + name: "plain error returns false", + err: fmt.Errorf("some error"), + want: false, + }, + { + name: "nil returns false", + err: nil, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := IsNotFound(tt.err); got != tt.want { + t.Errorf("IsNotFound() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestSlurmAPIError_Error(t *testing.T) { + fakeReq := httptest.NewRequest("GET", "http://localhost/slurm/v0.0.40/job/123", nil) + + tests := []struct { + name string + err *SlurmAPIError + wantContains []string + }{ + { + name: "with Error field set", + err: &SlurmAPIError{ + Response: &http.Response{Request: fakeReq}, + StatusCode: http.StatusNotFound, + Errors: OpenapiErrors{{Error: Ptr("Job not found")}}, + Message: "raw body", + }, + wantContains: []string{"404", "Job not found"}, + }, + { + name: "with Description field set when Error is nil", + err: &SlurmAPIError{ + Response: &http.Response{Request: fakeReq}, + StatusCode: http.StatusBadRequest, + Errors: OpenapiErrors{{Description: Ptr("Unable to query")}}, + Message: "raw body", + }, + wantContains: []string{"400", "Unable to query"}, + }, + { + name: "with both Error and Description nil falls through to Message", + err: &SlurmAPIError{ + Response: &http.Response{Request: fakeReq}, + StatusCode: http.StatusInternalServerError, + Errors: OpenapiErrors{{}}, + Message: "something went wrong", + }, + wantContains: []string{"500", "something went wrong"}, + }, + { + name: "with empty Errors slice falls through to Message", + err: &SlurmAPIError{ + Response: &http.Response{Request: fakeReq}, + StatusCode: http.StatusServiceUnavailable, + Errors: OpenapiErrors{}, + Message: "service unavailable fallback", + }, + wantContains: []string{"503", "service unavailable fallback"}, + }, + { + name: "with non-empty Errors but empty detail string falls through to Message", + err: &SlurmAPIError{ + Response: &http.Response{Request: fakeReq}, + StatusCode: http.StatusBadGateway, + Errors: OpenapiErrors{{ErrorNumber: Ptr(int32(42))}}, + Message: "gateway error detail", + }, + wantContains: []string{"502", "gateway error detail"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.err.Error() + for _, substr := range tt.wantContains { + if !strings.Contains(got, substr) { + t.Errorf("Error() = %q, want to contain %q", got, substr) + } + } + }) + } +}