opensource/mattermost
3
0
Fork 0
mirror of https://github.com/mattermost/mattermost.git synced 2026-08-31 01:11:29 +08:00
Code Issues Packages Projects Releases Wiki Activity

Preserve 429/503 retry status codes through DoActionRequest (#36700)

Browse Source 
* Preserve 429/503 retry status codes through DoActionRequest

DoActionRequest collapsed all plugin non-200 responses to 400, losing
the retry semantics carried by 429 and 503 (RFC 6585, RFC 7231). This
preserves 429/503 verbatim, maps other 5xx to 502 Bad Gateway, and
leaves other non-200 responses wrapped as 400.

* update api documentation for new errors

* fix tests affected by change

---------

Co-authored-by: Mattermost Build <build@mattermost.com>
Co-authored-by: Scott Bishel <scott.bishel@mattermost.com>
This commit is contained in:
Adam Schildkraut
2026-07-11 04:19:25 +10:00
committed by GitHub
parent 14288c742b
commit af49e8dc80
5 changed files with 151 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files
api/v4/source/actions.yaml
+44
View File
@@ -155,6 +155,28 @@
$ref: "#/components/responses/Unauthorized"
"403":
$ref: "#/components/responses/Forbidden"
"429":
description: The upstream integration rate-limited the request. The
original status code is preserved so clients can honor retry
semantics.
content:
application/json:
schema:
$ref: "#/components/schemas/AppError"
"502":
description: The upstream integration returned a 5xx (other than 503).
Surfaced as Bad Gateway because the failure is upstream of Mattermost.
content:
application/json:
schema:
$ref: "#/components/schemas/AppError"
"503":
description: The upstream integration is unavailable. The original
status code is preserved so clients can honor retry semantics.
content:
application/json:
schema:
$ref: "#/components/schemas/AppError"
/api/v4/actions/dialogs/lookup:
post:
tags:
@@ -227,6 +249,28 @@
$ref: "#/components/responses/Unauthorized"
"403":
$ref: "#/components/responses/Forbidden"
"429":
description: The upstream integration rate-limited the request. The
original status code is preserved so clients can honor retry
semantics.
content:
application/json:
schema:
$ref: "#/components/schemas/AppError"
"502":
description: The upstream integration returned a 5xx (other than 503).
Surfaced as Bad Gateway because the failure is upstream of Mattermost.
content:
application/json:
schema:
$ref: "#/components/schemas/AppError"
"503":
description: The upstream integration is unavailable. The original
status code is preserved so clients can honor retry semantics.
content:
application/json:
schema:
$ref: "#/components/schemas/AppError"
/api/v4/actions/dialogs/execute:
post:
tags:
api/v4/source/posts.yaml
+22
View File
@@ -1000,6 +1000,28 @@
$ref: "#/components/responses/Unauthorized"
"403":
$ref: "#/components/responses/Forbidden"
"429":
description: The upstream integration rate-limited the request. The
original status code is preserved so clients can honor retry
semantics.
content:
application/json:
schema:
$ref: "#/components/schemas/AppError"
"502":
description: The upstream integration returned a 5xx (other than 503).
Surfaced as Bad Gateway because the failure is upstream of Mattermost.
content:
application/json:
schema:
$ref: "#/components/schemas/AppError"
"503":
description: The upstream integration is unavailable. The original
status code is preserved so clients can honor retry semantics.
content:
application/json:
schema:
$ref: "#/components/schemas/AppError"
"/api/v4/posts/ids":
post:
tags:
server/channels/api4/integration_action_test.go
+2 -2
View File
@@ -1485,8 +1485,8 @@ func TestExecuteDialogAction(t *testing.T) {
resp, err := client.DoAPIPost(context.Background(), route, string(body))
require.Error(t, err)
require.NotNil(t, resp)
// DoActionRequest returns a 400 AppError on non-200 upstream responses;
// DoActionRequest maps upstream 5xx (other than 429/503) to 502 Bad Gateway;
// the handler propagates it unchanged.
assert.Equal(t, http.StatusBadRequest, resp.StatusCode)
assert.Equal(t, http.StatusBadGateway, resp.StatusCode)
})
}
server/channels/app/integration_action.go
+13 -1
View File
@@ -162,7 +162,19 @@ func (a *App) DoActionRequest(rctx request.CTX, rawURL string, body []byte) (*ht
}
if resp.StatusCode != http.StatusOK {
return resp, model.NewAppError("DoActionRequest", "api.post.do_action.action_integration.app_error", nil, fmt.Sprintf("status=%v", resp.StatusCode), http.StatusBadRequest)
// Preserve 429 and 503 because they carry retry semantics (RFC 6585,
// RFC 7231) that downstream HTTP clients legitimately need. Map other
// 5xx responses to 502 Bad Gateway since the failure is upstream of
// MM. Sanitize all other non-200 responses to 400 so plugin-specific
// 4xx codes do not leak misleading semantics to MM API clients.
status := http.StatusBadRequest
switch {
case resp.StatusCode == http.StatusTooManyRequests, resp.StatusCode == http.StatusServiceUnavailable:
status = resp.StatusCode
case resp.StatusCode >= 500:
status = http.StatusBadGateway
}
return resp, model.NewAppError("DoActionRequest", "api.post.do_action.action_integration.app_error", nil, fmt.Sprintf("status=%v", resp.StatusCode), status)
}
return resp, nil
server/channels/app/integration_action_test.go
+70 -2
View File
@@ -1636,6 +1636,74 @@ func TestDoActionRequest(t *testing.T) {
resp.Body.Close()
})
t.Run("should preserve 429 status code from plugin", func(t *testing.T) {
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusTooManyRequests)
_, _ = w.Write([]byte("Rate limited"))
}))
defer ts.Close()
requestBody := []byte(`{"test": "data"}`)
resp, err := th.App.DoActionRequest(th.Context, ts.URL, requestBody)
require.NotNil(t, err)
require.NotNil(t, resp)
assert.Equal(t, http.StatusTooManyRequests, resp.StatusCode)
assert.Equal(t, http.StatusTooManyRequests, err.StatusCode)
assert.Contains(t, err.Error(), "status=429")
resp.Body.Close()
})
t.Run("should preserve 503 status code from plugin", func(t *testing.T) {
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusServiceUnavailable)
_, _ = w.Write([]byte("Service unavailable"))
}))
defer ts.Close()
requestBody := []byte(`{"test": "data"}`)
resp, err := th.App.DoActionRequest(th.Context, ts.URL, requestBody)
require.NotNil(t, err)
require.NotNil(t, resp)
assert.Equal(t, http.StatusServiceUnavailable, resp.StatusCode)
assert.Equal(t, http.StatusServiceUnavailable, err.StatusCode)
assert.Contains(t, err.Error(), "status=503")
resp.Body.Close()
})
t.Run("should map other 5xx status codes to 502 Bad Gateway", func(t *testing.T) {
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusInternalServerError)
_, _ = w.Write([]byte("Internal error"))
}))
defer ts.Close()
requestBody := []byte(`{"test": "data"}`)
resp, err := th.App.DoActionRequest(th.Context, ts.URL, requestBody)
require.NotNil(t, err)
require.NotNil(t, resp)
assert.Equal(t, http.StatusInternalServerError, resp.StatusCode)
assert.Equal(t, http.StatusBadGateway, err.StatusCode)
assert.Contains(t, err.Error(), "status=500")
resp.Body.Close()
})
t.Run("should sanitize other 4xx status codes to 400", func(t *testing.T) {
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusNotFound)
_, _ = w.Write([]byte("Not found"))
}))
defer ts.Close()
requestBody := []byte(`{"test": "data"}`)
resp, err := th.App.DoActionRequest(th.Context, ts.URL, requestBody)
require.NotNil(t, err)
require.NotNil(t, resp)
assert.Equal(t, http.StatusNotFound, resp.StatusCode)
assert.Equal(t, http.StatusBadRequest, err.StatusCode)
assert.Contains(t, err.Error(), "status=404")
resp.Body.Close()
})
t.Run("should handle invalid URL", func(t *testing.T) {
requestBody := []byte(`{"test": "data"}`)
resp, err := th.App.DoActionRequest(th.Context, "invalid-url", requestBody)
@@ -3994,7 +4062,7 @@ func TestExecuteDialogAction(t *testing.T) {
_, appErr := th.App.ExecuteDialogAction(th.Context, th.BasicUser.Id, req)
require.NotNil(t, appErr)
// DoActionRequest maps non-200 upstream responses to a 400 AppError.
assert.Equal(t, http.StatusBadRequest, appErr.StatusCode)
// DoActionRequest maps upstream 5xx (other than 429/503) to 502 Bad Gateway.
assert.Equal(t, http.StatusBadGateway, appErr.StatusCode)
})
}