Skip to content

Commit 9aee7bd

Browse files
ritek01Harness
ritek01
authored and
Harness
committed
fix: [PIPE-32774]: Surface Bitbucket 429 rate-limit errors instead of silently swallowing them (#100002)
* 10a411 refactor: Clean up error handling and remove unused tests in Bitbucket driver * b1a704 fix: [PIPE-32774]: Removed extra comments * 0d256b fix: [PIPE-32774]: Add UT's * ec9df7 fix: [PIPE-32774]: Surface Bitbucket 429 rate-limit errors instead of silently swallowing them
1 parent 3a90520 commit 9aee7bd

4 files changed

Lines changed: 320 additions & 5 deletions

File tree

‎scm/driver/bitbucket/bitbucket.go‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"bytes"
1010
"context"
1111
"encoding/json"
12+
"fmt"
1213
"io"
1314
"mime/multipart"
1415
"net/url"
@@ -164,6 +165,7 @@ func (c *wrapper) do(ctx context.Context, method, path string, in, out interface
164165
} else if res.Status > 300 {
165166
err := new(Error)
166167
json.NewDecoder(res.Body).Decode(err)
168+
err.StatusCode = res.Status
167169
return res, err
168170
}
169171

@@ -199,12 +201,19 @@ type link struct {
199201

200202
// Error represents a Bitbucket error.
201203
type Error struct {
202-
Type string `json:"type"`
203-
Data struct {
204+
StatusCode int // HTTP status code; not from JSON, stamped by do()
205+
Type string `json:"type"`
206+
Data struct {
204207
Message string `json:"message"`
205208
} `json:"error"`
206209
}
207210

208211
func (e *Error) Error() string {
209-
return e.Data.Message
212+
if e.Data.Message != "" {
213+
return e.Data.Message
214+
}
215+
if e.StatusCode > 0 {
216+
return fmt.Sprintf("bitbucket: http status %d", e.StatusCode)
217+
}
218+
return "bitbucket: unknown error"
210219
}

‎scm/driver/bitbucket/error_test.go‎

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
// Copyright 2017 Drone.IO Inc. All rights reserved.
2+
// Use of this source code is governed by a BSD-style
3+
// license that can be found in the LICENSE file.
4+
5+
package bitbucket
6+
7+
import (
8+
"context"
9+
"encoding/json"
10+
"net/http"
11+
"net/http/httptest"
12+
"testing"
13+
)
14+
15+
func TestError_ErrorMessage(t *testing.T) {
16+
tests := []struct {
17+
name string
18+
err Error
19+
want string
20+
}{
21+
{
22+
name: "with body message",
23+
err: Error{
24+
StatusCode: 400,
25+
Type: "error",
26+
Data: struct{ Message string `json:"message"` }{Message: "Bad request"},
27+
},
28+
want: "Bad request",
29+
},
30+
{
31+
name: "status code only, no body message",
32+
err: Error{
33+
StatusCode: 429,
34+
},
35+
want: "bitbucket: http status 429",
36+
},
37+
{
38+
name: "no status code, no body message",
39+
err: Error{},
40+
want: "bitbucket: unknown error",
41+
},
42+
{
43+
name: "status code with empty message",
44+
err: Error{
45+
StatusCode: 500,
46+
Data: struct{ Message string `json:"message"` }{Message: ""},
47+
},
48+
want: "bitbucket: http status 500",
49+
},
50+
{
51+
name: "body message takes precedence over status code",
52+
err: Error{
53+
StatusCode: 403,
54+
Data: struct{ Message string `json:"message"` }{Message: "Forbidden"},
55+
},
56+
want: "Forbidden",
57+
},
58+
}
59+
60+
for _, tt := range tests {
61+
t.Run(tt.name, func(t *testing.T) {
62+
got := tt.err.Error()
63+
if got != tt.want {
64+
t.Errorf("Error.Error() = %q, want %q", got, tt.want)
65+
}
66+
})
67+
}
68+
}
69+
70+
func TestError_StatusCodeStamped(t *testing.T) {
71+
// Verify that do() stamps the HTTP status code on the Error struct
72+
// even when the response body is empty (e.g. 429 rate-limit with no JSON body).
73+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
74+
w.WriteHeader(429)
75+
// empty body — no JSON
76+
}))
77+
defer server.Close()
78+
79+
client, _ := New(server.URL)
80+
wrapper := &wrapper{client}
81+
82+
_, err := wrapper.do(context.Background(), "GET", "2.0/repositories", nil, nil)
83+
if err == nil {
84+
t.Fatal("Expected error for 429 response, got nil")
85+
}
86+
87+
bbErr, ok := err.(*Error)
88+
if !ok {
89+
t.Fatalf("Expected *Error, got %T: %v", err, err)
90+
}
91+
92+
if bbErr.StatusCode != 429 {
93+
t.Errorf("Expected StatusCode=429, got %d", bbErr.StatusCode)
94+
}
95+
96+
if got := bbErr.Error(); got != "bitbucket: http status 429" {
97+
t.Errorf("Expected error message %q, got %q", "bitbucket: http status 429", got)
98+
}
99+
}
100+
101+
func TestError_StatusCodeWithJSONBody(t *testing.T) {
102+
// When the server returns a JSON error body, both StatusCode and Data.Message should be set.
103+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
104+
w.Header().Set("Content-Type", "application/json")
105+
w.WriteHeader(403)
106+
json.NewEncoder(w).Encode(map[string]interface{}{
107+
"error": map[string]string{"message": "Access denied"},
108+
})
109+
}))
110+
defer server.Close()
111+
112+
client, _ := New(server.URL)
113+
wrapper := &wrapper{client}
114+
115+
_, err := wrapper.do(context.Background(), "GET", "2.0/repositories", nil, nil)
116+
if err == nil {
117+
t.Fatal("Expected error for 403 response, got nil")
118+
}
119+
120+
bbErr, ok := err.(*Error)
121+
if !ok {
122+
t.Fatalf("Expected *Error, got %T: %v", err, err)
123+
}
124+
125+
if bbErr.StatusCode != 403 {
126+
t.Errorf("Expected StatusCode=403, got %d", bbErr.StatusCode)
127+
}
128+
129+
// Data.Message should take precedence
130+
if got := bbErr.Error(); got != "Access denied" {
131+
t.Errorf("Expected error %q, got %q", "Access denied", got)
132+
}
133+
}
134+

‎scm/driver/stash/error_test.go‎

Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,166 @@
1+
// Copyright 2017 Drone.IO Inc. All rights reserved.
2+
// Use of this source code is governed by a BSD-style
3+
// license that can be found in the LICENSE file.
4+
5+
package stash
6+
7+
import (
8+
"context"
9+
"encoding/json"
10+
"net/http"
11+
"net/http/httptest"
12+
"testing"
13+
)
14+
15+
func TestError_WithErrorsSlice(t *testing.T) {
16+
e := &Error{
17+
Status: 400,
18+
Errors: []struct {
19+
Message string `json:"message"`
20+
ExceptionName string `json:"exceptionName"`
21+
CurrentVersion int `json:"currentVersion"`
22+
ExpectedVersion int `json:"expectedVersion"`
23+
}{
24+
{Message: "field is required"},
25+
},
26+
}
27+
28+
if got := e.Error(); got != "field is required" {
29+
t.Errorf("Error() = %q, want %q", got, "field is required")
30+
}
31+
}
32+
33+
func TestError_WithMessage(t *testing.T) {
34+
e := &Error{
35+
Status: 404,
36+
Message: "Repository not found",
37+
}
38+
39+
want := "bitbucket server: status: 404 message: Repository not found"
40+
if got := e.Error(); got != want {
41+
t.Errorf("Error() = %q, want %q", got, want)
42+
}
43+
}
44+
45+
func TestError_StatusCodeOnly(t *testing.T) {
46+
e := &Error{
47+
Status: 429,
48+
}
49+
50+
want := "bitbucket server: http status 429"
51+
if got := e.Error(); got != want {
52+
t.Errorf("Error() = %q, want %q", got, want)
53+
}
54+
}
55+
56+
func TestError_UnknownError(t *testing.T) {
57+
e := &Error{}
58+
59+
want := "bitbucket server: unknown error"
60+
if got := e.Error(); got != want {
61+
t.Errorf("Error() = %q, want %q", got, want)
62+
}
63+
}
64+
65+
func TestError_StatusStampedOnEmptyBody(t *testing.T) {
66+
// Verify that do() stamps the HTTP status on Error.Status
67+
// when the response body is empty (e.g. 429 rate-limit).
68+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
69+
w.WriteHeader(429)
70+
// empty body
71+
}))
72+
defer server.Close()
73+
74+
client, _ := New(server.URL)
75+
wrapper := &wrapper{client}
76+
77+
_, err := wrapper.do(context.Background(), "GET", "rest/api/1.0/repos", nil, nil)
78+
if err == nil {
79+
t.Fatal("Expected error for 429 response, got nil")
80+
}
81+
82+
stashErr, ok := err.(*Error)
83+
if !ok {
84+
t.Fatalf("Expected *Error, got %T: %v", err, err)
85+
}
86+
87+
if stashErr.Status != 429 {
88+
t.Errorf("Expected Status=429, got %d", stashErr.Status)
89+
}
90+
91+
want := "bitbucket server: http status 429"
92+
if got := stashErr.Error(); got != want {
93+
t.Errorf("Expected error %q, got %q", want, got)
94+
}
95+
}
96+
97+
func TestError_StatusFromJSONPreserved(t *testing.T) {
98+
// When the JSON body includes a status-code field, it should NOT be overwritten.
99+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
100+
w.Header().Set("Content-Type", "application/json")
101+
w.WriteHeader(400)
102+
json.NewEncoder(w).Encode(map[string]interface{}{
103+
"status-code": 400,
104+
"message": "Bad request from API",
105+
})
106+
}))
107+
defer server.Close()
108+
109+
client, _ := New(server.URL)
110+
wrapper := &wrapper{client}
111+
112+
_, err := wrapper.do(context.Background(), "GET", "rest/api/1.0/repos", nil, nil)
113+
if err == nil {
114+
t.Fatal("Expected error for 400 response, got nil")
115+
}
116+
117+
stashErr, ok := err.(*Error)
118+
if !ok {
119+
t.Fatalf("Expected *Error, got %T: %v", err, err)
120+
}
121+
122+
// Status from JSON body should be preserved (not overwritten by HTTP status)
123+
if stashErr.Status != 400 {
124+
t.Errorf("Expected Status=400 from JSON, got %d", stashErr.Status)
125+
}
126+
127+
want := "bitbucket server: status: 400 message: Bad request from API"
128+
if got := stashErr.Error(); got != want {
129+
t.Errorf("Expected error %q, got %q", want, got)
130+
}
131+
}
132+
133+
func TestError_StatusFallbackWhenJSONHasNoStatus(t *testing.T) {
134+
// When JSON body has message but no status-code, HTTP status should be used as fallback.
135+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
136+
w.Header().Set("Content-Type", "application/json")
137+
w.WriteHeader(503)
138+
json.NewEncoder(w).Encode(map[string]interface{}{
139+
"message": "Service unavailable",
140+
})
141+
}))
142+
defer server.Close()
143+
144+
client, _ := New(server.URL)
145+
wrapper := &wrapper{client}
146+
147+
_, err := wrapper.do(context.Background(), "GET", "rest/api/1.0/repos", nil, nil)
148+
if err == nil {
149+
t.Fatal("Expected error for 503 response, got nil")
150+
}
151+
152+
stashErr, ok := err.(*Error)
153+
if !ok {
154+
t.Fatalf("Expected *Error, got %T: %v", err, err)
155+
}
156+
157+
// HTTP status should be stamped as fallback since JSON has no status-code
158+
if stashErr.Status != 503 {
159+
t.Errorf("Expected Status=503 (fallback from HTTP), got %d", stashErr.Status)
160+
}
161+
162+
want := "bitbucket server: status: 503 message: Service unavailable"
163+
if got := stashErr.Error(); got != want {
164+
t.Errorf("Expected error %q, got %q", want, got)
165+
}
166+
}

‎scm/driver/stash/stash.go‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,9 @@ func (c *wrapper) do(ctx context.Context, method, path string, in, out interface
120120
} else if res.Status > 300 {
121121
err := new(Error)
122122
json.NewDecoder(res.Body).Decode(err)
123+
if err.Status == 0 {
124+
err.Status = res.Status
125+
}
123126
return res, err
124127
}
125128

@@ -164,9 +167,12 @@ type Error struct {
164167
func (e *Error) Error() string {
165168
if len(e.Errors) == 0 {
166169
if len(e.Message) > 0 {
167-
return fmt.Sprintf("bitbucket: status: %d message: %s", e.Status, e.Message)
170+
return fmt.Sprintf("bitbucket server: status: %d message: %s", e.Status, e.Message)
171+
}
172+
if e.Status > 0 {
173+
return fmt.Sprintf("bitbucket server: http status %d", e.Status)
168174
}
169-
return "bitbucket: undefined error"
175+
return "bitbucket server: unknown error"
170176
}
171177
return e.Errors[0].Message
172178
}

0 commit comments

Comments
 (0)