Skip to content

Commit b2f21b3

Browse files
committed
fix: method-aware retries and five audit findings
- Retries now match the TS/Py idempotency contract: GET, PUT and DELETE retry on connection errors and 408/429/500/502/503/504; POST and PATCH retry only on 429/503, where the server provably did no work. Previously a POST /shorten answered 500 or 504 was replayed and could create duplicate links. - ExportLink hits /api/v1/export/links/{id}: only the per-link route names the download after the link, so aggregate exports of different links no longer overwrite each other on disk. Aggregate slicing stays reachable via Export with a url_id filter. - Error.Code docs and test fixtures now show the backend's real lowercase snake_case codes (conflict, not_found, blocked, ...) with the one uppercase outlier EMAIL_NOT_VERIFIED named. - ListURLsOptions covers the full filter object: CreatedAfter, CreatedBefore, and tri-state PasswordSet / MaxClicksSet via Opt[bool]. - LinkStats and ExportLink reject the aggregate-only short_code and url_id filters client-side instead of letting the endpoint 422. - 451 affordance: IsBlocked predicate plus the ErrLinkBlocked sentinel, attached in newError, for the live safety takedown.
1 parent 87ccc49 commit b2f21b3

14 files changed

Lines changed: 326 additions & 58 deletions

README.md

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@ metadata. Retrieve it with `errors.As`:
169169
| What you get | Where |
170170
| --- | --- |
171171
| HTTP status | `err.StatusCode` |
172-
| Machine-readable code | `err.Code` (open string enum, e.g. `CONFLICT_ERROR`) |
172+
| Machine-readable code | `err.Code` (lowercase snake_case, e.g. `conflict`, `not_found`, `blocked`; the one uppercase outlier is `EMAIL_NOT_VERIFIED`) |
173173
| Human-readable message | `err.Message`, plus `err.Field` on validation errors |
174174
| Request id for support | `err.RequestID` |
175175
| Rate-limit state | `err.RateLimit` (limit, remaining, reset, retry-after) |
@@ -182,13 +182,16 @@ Common branches have predicates and sentinels:
182182
| `spoo.IsRateLimited(err)` | 429: budget exhausted even after retries |
183183
| `errors.Is(err, spoo.ErrSessionExpired)` | the refresh token no longer works; log in again |
184184
| `errors.Is(err, spoo.ErrLinkPasswordProtected)` | the link's stats need the link password |
185+
| `spoo.IsBlocked(err)` | 451: the link was taken down by the safety pipeline |
185186

186187
## Retries
187188

188-
Connection errors, 408, 429, and 5xx responses are retried twice by default
189-
with exponential backoff and jitter. A `Retry-After` header is authoritative
190-
when the server sends one. Configure with `option.WithMaxRetries(n)`; 0
191-
disables retries.
189+
Idempotent requests (GET, PUT, DELETE) are retried twice by default on
190+
connection errors and 408, 429, 500, 502, 503 and 504 responses, with
191+
exponential backoff and jitter. Requests that are not idempotent are only
192+
retried when the server provably did no work (429 and 503). A `Retry-After`
193+
header is authoritative when the server sends one. Configure with
194+
`option.WithMaxRetries(n)`; 0 disables retries.
192195

193196
## Pagination
194197

@@ -247,7 +250,7 @@ file, or database, and rotated tokens persist through it.
247250
| `BulkDelete`, `BulkUpdateStatus`, `BulkUpdateExpiry`, `BulkMoveDomain` | `POST /api/v1/urls/bulk/*` |
248251
| `Stats`, `LinkStats`, `StatsByAlias` | `GET /api/v1/stats`, `GET /api/v1/stats/links/{id}` |
249252
| `PublicStats`, `PublicPreview` | `GET or POST /api/v1/public/stats/{code}`, `GET /api/v1/public/preview/{code}` |
250-
| `Export`, `ExportLink` | `GET /api/v1/export` |
253+
| `Export`, `ExportLink` | `GET /api/v1/export`, `GET /api/v1/export/links/{id}` |
251254
| `EmojiSet` | `GET /api/v1/emoji-set` (ETag-cached) |
252255
| `Me` | `GET /auth/me` |
253256
| `ExchangeDeviceCode`, `RefreshTokens`, `DeviceAuthURL` | `POST /auth/device/token`, `POST /auth/device/refresh` |

client.go

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -129,8 +129,9 @@ func (c *Client) request(ctx context.Context, method, path string, query url.Val
129129
}
130130

131131
// send builds and performs one HTTP call, retrying transient failures
132-
// (connection errors, 408, 429, 5xx) with exponential backoff and
133-
// jitter, honoring Retry-After. Callers own the response body.
132+
// with exponential backoff and jitter, honoring Retry-After. What
133+
// counts as transient depends on the method: see
134+
// transport.RetryableStatus. Callers own the response body.
134135
func (c *Client) send(ctx context.Context, method, path string, query url.Values, body any, creds Credentials, extra http.Header) (*http.Response, error) {
135136
u := c.base + path
136137
if len(query) > 0 {
@@ -147,10 +148,12 @@ func (c *Client) send(ctx context.Context, method, path string, query url.Values
147148
for attempt := 0; ; attempt++ {
148149
resp, err := c.sendOnce(ctx, method, u, payload, creds, extra)
149150
if err != nil {
150-
if attempt >= c.maxRetries || ctx.Err() != nil {
151+
// A dropped connection may have reached the server, so
152+
// only idempotent methods replay on transport errors.
153+
if !transport.IdempotentMethod(method) || attempt >= c.maxRetries || ctx.Err() != nil {
151154
return nil, err
152155
}
153-
} else if !transport.RetryableStatus(resp.StatusCode) || attempt >= c.maxRetries {
156+
} else if !transport.RetryableStatus(method, resp.StatusCode) || attempt >= c.maxRetries {
154157
return resp, nil
155158
}
156159
var retryAfter string

client_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ func TestClientHeaderRejectsMalformedVersion(t *testing.T) {
6464
func TestDoParsesErrorEnvelope(t *testing.T) {
6565
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
6666
w.WriteHeader(http.StatusConflict)
67-
w.Write([]byte(`{"error":"alias already taken","code":"CONFLICT_ERROR","field":"alias"}`))
67+
w.Write([]byte(`{"error":"alias already taken","code":"conflict","field":"alias"}`))
6868
}))
6969
defer srv.Close()
7070

@@ -74,7 +74,7 @@ func TestDoParsesErrorEnvelope(t *testing.T) {
7474
if !errors.As(err, &apiErr) {
7575
t.Fatalf("err = %v, want *Error", err)
7676
}
77-
if apiErr.StatusCode != 409 || apiErr.Code != "CONFLICT_ERROR" || apiErr.Message != "alias already taken" {
77+
if apiErr.StatusCode != 409 || apiErr.Code != "conflict" || apiErr.Message != "alias already taken" {
7878
t.Fatalf("unexpected Error: %+v", apiErr)
7979
}
8080
if apiErr.Field != "alias" {
@@ -95,7 +95,7 @@ func TestDoRefreshesOn401AndRetries(t *testing.T) {
9595
}
9696
calls.Add(1)
9797
w.WriteHeader(http.StatusUnauthorized)
98-
w.Write([]byte(`{"error":"token expired","code":"AUTHENTICATION_ERROR"}`))
98+
w.Write([]byte(`{"error":"token expired","code":"authentication_error"}`))
9999
}
100100
}))
101101
defer srv.Close()

device_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ func TestConcurrent401sRefreshOnce(t *testing.T) {
218218
// A second spend of a rotated token is exactly the
219219
// bug: fail the way the backend would.
220220
w.WriteHeader(http.StatusUnauthorized)
221-
w.Write([]byte(`{"error":"invalid refresh token","code":"AUTHENTICATION_ERROR"}`))
221+
w.Write([]byte(`{"error":"invalid refresh token","code":"authentication_error"}`))
222222
return
223223
}
224224
time.Sleep(50 * time.Millisecond) // widen the race window
@@ -229,7 +229,7 @@ func TestConcurrent401sRefreshOnce(t *testing.T) {
229229
return
230230
}
231231
w.WriteHeader(http.StatusUnauthorized)
232-
w.Write([]byte(`{"error":"token expired","code":"AUTHENTICATION_ERROR"}`))
232+
w.Write([]byte(`{"error":"token expired","code":"authentication_error"}`))
233233
}
234234
}))
235235
defer srv.Close()

doc.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,9 +57,11 @@
5757
//
5858
// # Retries
5959
//
60-
// Transient failures (connection errors, 408, 429, 5xx) are retried
61-
// twice by default with exponential backoff and jitter, honoring
62-
// Retry-After. Tune with option.WithMaxRetries.
60+
// Idempotent requests (GET, PUT, DELETE) are retried twice by default
61+
// on connection errors and 408, 429, 500, 502, 503 and 504, with
62+
// exponential backoff and jitter, honoring Retry-After. Requests that
63+
// are not idempotent are only retried when the server provably did no
64+
// work (429 and 503). Tune with option.WithMaxRetries.
6365
//
6466
// # Pagination
6567
//

errors.go

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,14 @@ var (
2323

2424
// ErrLinkPasswordProtected marks a 401 that is a property of the
2525
// link, not of the session: the link's stats require the link
26-
// password, which the SDK does not supply.
26+
// password, supplied via PublicStatsQuery.Password.
2727
ErrLinkPasswordProtected = errors.New("link is password protected")
28+
29+
// ErrLinkBlocked marks a 451: the link was taken down by the
30+
// safety pipeline because its destination was flagged. This is a
31+
// verdict on the link, not a transient failure — see also
32+
// [IsBlocked].
33+
ErrLinkBlocked = errors.New("link is blocked")
2834
)
2935

3036
// ErrTokenSourceRequired is returned by [Client.ForceRefresh] when the
@@ -52,10 +58,13 @@ type RateLimit struct {
5258
type Error struct {
5359
// StatusCode is the HTTP status of the response.
5460
StatusCode int `json:"-"`
55-
// Code is the backend's machine-readable error code, an open enum
56-
// (e.g. "CONFLICT_ERROR", "AUTHENTICATION_ERROR"). Read from the
57-
// body, with the X-Error-Code header as fallback for the
58-
// edge-composed responses whose bodies carry no envelope.
61+
// Code is the backend's machine-readable error code, an open
62+
// string enum in lowercase snake_case: "conflict",
63+
// "authentication_error", "not_found", "rate_limit_exceeded",
64+
// "payload_too_large", "blocked", "gone", and so on. The one
65+
// uppercase outlier is "EMAIL_NOT_VERIFIED". Read from the body,
66+
// with the X-Error-Code header as fallback for the edge-composed
67+
// responses whose bodies carry no envelope.
5968
Code string `json:"code"`
6069
// Message is the human-readable error message.
6170
Message string `json:"error"`
@@ -97,6 +106,15 @@ func IsRateLimited(err error) bool {
97106
return errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusTooManyRequests
98107
}
99108

109+
// IsBlocked reports whether err is an API 451: the link was taken down
110+
// by the safety pipeline. Integrators should branch on this to tell
111+
// "the link was removed" apart from "something broke".
112+
// errors.Is(err, ErrLinkBlocked) reports the same condition.
113+
func IsBlocked(err error) bool {
114+
var apiErr *Error
115+
return errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusUnavailableForLegalReasons
116+
}
117+
100118
// newError builds an *Error from an HTTP error response, consuming (but
101119
// not closing) the body.
102120
func newError(resp *http.Response) *Error {
@@ -108,11 +126,14 @@ func newError(resp *http.Response) *Error {
108126
}
109127
e.RequestID = resp.Header.Get("X-Request-ID")
110128
e.RateLimit = parseRateLimit(resp.Header)
111-
if resp.StatusCode == http.StatusUnauthorized {
129+
switch resp.StatusCode {
130+
case http.StatusUnauthorized:
112131
switch resp.Header.Get("X-Error-Code") {
113132
case "password_required", "invalid_password":
114133
e.sentinel = ErrLinkPasswordProtected
115134
}
135+
case http.StatusUnavailableForLegalReasons:
136+
e.sentinel = ErrLinkBlocked
116137
}
117138
return e
118139
}

errors_test.go

Lines changed: 50 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ func TestErrorParsesRateLimitAndRequestID(t *testing.T) {
1919
w.Header().Set("X-RateLimit-Reset", "1755500000")
2020
w.Header().Set("Retry-After", "30")
2121
w.WriteHeader(http.StatusTooManyRequests)
22-
w.Write([]byte(`{"error":"rate limit exceeded","code":"RATE_LIMIT_ERROR"}`))
22+
w.Write([]byte(`{"error":"rate limit exceeded","code":"rate_limit_exceeded"}`))
2323
}))
2424
defer srv.Close()
2525

@@ -47,6 +47,52 @@ func TestErrorParsesRateLimitAndRequestID(t *testing.T) {
4747
}
4848
}
4949

50+
// 451 is the live safety takedown: integrators branch on it to tell
51+
// "the link was removed" apart from "something broke".
52+
func TestBlocked451(t *testing.T) {
53+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
54+
w.Header().Set("X-Error-Code", "blocked")
55+
w.WriteHeader(http.StatusUnavailableForLegalReasons)
56+
w.Write([]byte(`{"error":"This link has been blocked","code":"blocked"}`))
57+
}))
58+
defer srv.Close()
59+
60+
c := NewClient(option.WithBaseURL(srv.URL))
61+
_, err := c.PublicStats(context.Background(), "scam", PublicStatsQuery{})
62+
if !IsBlocked(err) {
63+
t.Fatalf("err = %v, want IsBlocked", err)
64+
}
65+
if !errors.Is(err, ErrLinkBlocked) {
66+
t.Fatalf("err = %v, want ErrLinkBlocked sentinel", err)
67+
}
68+
var apiErr *Error
69+
if !errors.As(err, &apiErr) || apiErr.Code != "blocked" {
70+
t.Fatalf("err = %v, want code blocked", err)
71+
}
72+
if IsBlocked(&Error{StatusCode: 404}) {
73+
t.Fatal("404 must not read as blocked")
74+
}
75+
}
76+
77+
// An edge-composed 451 whose body is HTML still yields a usable code
78+
// via the X-Error-Code header fallback.
79+
func TestBlocked451EdgeComposedBody(t *testing.T) {
80+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
81+
w.Header().Set("X-Error-Code", "blocked")
82+
w.Header().Set("Content-Type", "text/html")
83+
w.WriteHeader(http.StatusUnavailableForLegalReasons)
84+
w.Write([]byte(`<!doctype html><title>451</title>`))
85+
}))
86+
defer srv.Close()
87+
88+
c := NewClient(option.WithBaseURL(srv.URL))
89+
_, err := c.Me(context.Background())
90+
var apiErr *Error
91+
if !errors.As(err, &apiErr) || apiErr.Code != "blocked" || !IsBlocked(err) {
92+
t.Fatalf("err = %v, want header-derived blocked code", err)
93+
}
94+
}
95+
5096
func TestIsRateLimitedRejectsOtherErrors(t *testing.T) {
5197
if IsRateLimited(errors.New("nope")) {
5298
t.Fatal("plain errors must not read as rate-limited")
@@ -65,11 +111,11 @@ func TestRefreshRejectionIsSessionExpired(t *testing.T) {
65111
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
66112
if r.URL.Path == "/auth/device/refresh" {
67113
w.WriteHeader(http.StatusUnauthorized)
68-
w.Write([]byte(`{"error":"invalid refresh token","code":"AUTHENTICATION_ERROR"}`))
114+
w.Write([]byte(`{"error":"invalid refresh token","code":"authentication_error"}`))
69115
return
70116
}
71117
w.WriteHeader(http.StatusUnauthorized)
72-
w.Write([]byte(`{"error":"token expired","code":"AUTHENTICATION_ERROR"}`))
118+
w.Write([]byte(`{"error":"token expired","code":"authentication_error"}`))
73119
}))
74120
defer srv.Close()
75121

@@ -89,7 +135,7 @@ func TestRefreshRejectionIsSessionExpired(t *testing.T) {
89135
func TestPlain401IsNotSessionExpired(t *testing.T) {
90136
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
91137
w.WriteHeader(http.StatusUnauthorized)
92-
w.Write([]byte(`{"error":"authentication required","code":"AUTHENTICATION_ERROR"}`))
138+
w.Write([]byte(`{"error":"authentication required","code":"authentication_error"}`))
93139
}))
94140
defer srv.Close()
95141

export.go

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -26,23 +26,29 @@ type ExportFile struct {
2626
// Export downloads account-wide stats in the given format (json, csv,
2727
// xlsx, xml). Auth is required — anonymous export no longer exists.
2828
// Slice to specific links with the short_code / url_id filters on
29-
// [StatsQuery].
29+
// [StatsQuery]; note the aggregate route reports a generic filename
30+
// regardless of slicing, so single-link exports belong on ExportLink.
3031
func (c *Client) Export(ctx context.Context, q StatsQuery, format string) (*ExportFile, error) {
31-
return c.export(ctx, q.values(), format)
32+
return c.export(ctx, "/api/v1/export", q.values(), format)
3233
}
3334

34-
// ExportLink downloads one owned link's stats by its url id (resolve
35-
// an alias with ResolveAlias first). Unknown and foreign ids yield an
36-
// empty slice of that link, consistent with the slicing filters.
35+
// ExportLink downloads one owned link's stats by its url id via the
36+
// per-link route, whose server-suggested filename carries the link's
37+
// identity (the aggregate route names every download the same, so
38+
// saved files would silently overwrite each other). Resolve an alias
39+
// with ResolveAlias first; unknown and foreign ids both 404. The
40+
// short_code / url_id slicing filters are aggregate-only here too.
3741
func (c *Client) ExportLink(ctx context.Context, urlID string, q StatsQuery, format string) (*ExportFile, error) {
38-
v := q.values()
39-
v.Set("url_id", urlID)
40-
return c.export(ctx, v, format)
42+
if err := q.validatePerLink(); err != nil {
43+
return nil, err
44+
}
45+
path := "/api/v1/export/links/" + url.PathEscape(urlID)
46+
return c.export(ctx, path, q.values(), format)
4147
}
4248

43-
func (c *Client) export(ctx context.Context, v url.Values, format string) (*ExportFile, error) {
49+
func (c *Client) export(ctx context.Context, path string, v url.Values, format string) (*ExportFile, error) {
4450
v.Set("format", format)
45-
resp, err := c.request(ctx, http.MethodGet, "/api/v1/export", v, nil)
51+
resp, err := c.request(ctx, http.MethodGet, path, v, nil)
4652
if err != nil {
4753
return nil, err
4854
}

internal/transport/transport.go

Lines changed: 28 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,34 @@ const (
2020
retryMaxDelay = 10 * time.Second
2121
)
2222

23-
// RetryableStatus reports whether a response status is worth retrying:
24-
// timeouts, rate limits, and server-side failures.
25-
func RetryableStatus(status int) bool {
26-
return status == http.StatusRequestTimeout ||
27-
status == http.StatusTooManyRequests ||
28-
status >= 500
23+
// IdempotentMethod reports whether an HTTP method is safe to replay
24+
// unconditionally. The set matches the TS and Python SDKs exactly:
25+
// GET, PUT, DELETE.
26+
func IdempotentMethod(method string) bool {
27+
switch method {
28+
case http.MethodGet, http.MethodPut, http.MethodDelete:
29+
return true
30+
}
31+
return false
32+
}
33+
34+
// RetryableStatus reports whether a response status is worth retrying
35+
// for the given method. Idempotent methods retry on 408, 429, 500,
36+
// 502, 503 and 504. Non-idempotent methods (POST, PATCH) retry only on
37+
// 429 and 503, the statuses where the server provably did no work; a
38+
// replayed POST after a 500 or 504 could have created the resource
39+
// twice. The sets match the TS and Python SDKs exactly.
40+
func RetryableStatus(method string, status int) bool {
41+
switch status {
42+
case http.StatusTooManyRequests, http.StatusServiceUnavailable:
43+
return true
44+
case http.StatusRequestTimeout,
45+
http.StatusInternalServerError,
46+
http.StatusBadGateway,
47+
http.StatusGatewayTimeout:
48+
return IdempotentMethod(method)
49+
}
50+
return false
2951
}
3052

3153
// RetryDelay computes the wait before retry number attempt+1. A

0 commit comments

Comments
 (0)