Copilot commented on code in PR #3317:
URL: https://github.com/apache/apisix-dashboard/pull/3317#discussion_r2931381067
##########
e2e/utils/req.ts:
##########
@@ -27,30 +27,65 @@ export const getPlaywrightRequestAdapter = (
ctx: APIRequestContext
): AxiosAdapter => {
return async (config) => {
- const { url, data, baseURL } = config;
+ const { url, data, baseURL, method } = config;
if (typeof url === 'undefined') {
throw new Error('Need to provide a url');
}
type Payload = Parameters<APIRequestContext['fetch']>[1];
const payload: Payload = {
headers: config.headers,
- method: config.method,
- failOnStatusCode: true,
+ method,
+ failOnStatusCode: false,
data,
};
const urlWithBase = `${baseURL}${url}`;
const res = await ctx.fetch(urlWithBase, payload);
+ const status = res.status();
try {
- return {
+ // Idempotent DELETE: Treat 404 as 200 OK
+ if (method?.toLowerCase() === 'delete' && status === 404) {
+ console.warn(`[e2eReq] Ignored 404 on DELETE for ${urlWithBase},
treating as 200 OK.`);
+ return {
+ data: {},
+ status: 200,
+ statusText: 'OK',
+ headers: {},
+ config,
+ };
+ }
Review Comment:
The request adapter globally treats **all** `DELETE` 404 responses as
success by returning a synthetic `200 OK`. This can mask genuine failures
(wrong URL, wrong environment, unexpected missing resource) and make assertions
around deletion semantics impossible. Prefer handling idempotent deletes at the
call site (e.g., in cleanup helpers) or make this behavior opt-in (config flag
/ header) rather than unconditional for every DELETE.
##########
src/apis/upstreams.ts:
##########
@@ -77,6 +77,17 @@ export const deleteAllUpstreams = async (req: AxiosInstance)
=> {
req.delete(`${API_UPSTREAMS}/${d.value.id}`).catch((err) => {
// Ignore 404 errors as the resource might have been deleted
if (axios.isAxiosError(err) && err.response?.status === 404) return;
+ // Ignore 400 errors only when the upstream is still referenced by
routes/services
+ if (
+ axios.isAxiosError(err) &&
+ err.response?.status === 400 &&
+ typeof err.response?.data?.error_msg === 'string' &&
+ err.response.data.error_msg.includes('still being used')
+ ) {
+ // eslint-disable-next-line no-console
+ console.warn(`[e2eReq] Upstream ${d.value.id} is still being used,
could not delete`);
+ return;
+ }
Review Comment:
`deleteAllUpstreams` now swallows 400s based on a substring match
(`error_msg.includes('still being used')`) and logs via `console.warn`. This is
brittle (backend message changes/localization) and can leave upstreams behind
while still reporting success, which can cause later E2E runs to start from a
dirty state. Consider making this a hard failure with a clearer error, or move
this e2e-only tolerance + logging into e2e utilities (and/or return the list of
undeleted upstream IDs so callers can decide).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
