Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fetch error handling is incorrect #1655

Open
flevi29 opened this issue May 19, 2024 · 0 comments · May be fixed by #1656
Open

Fetch error handling is incorrect #1655

flevi29 opened this issue May 19, 2024 · 0 comments · May be fixed by #1656
Labels
bug Something isn't working

Comments

@flevi29
Copy link
Collaborator

flevi29 commented May 19, 2024

In the code below we're assuming that if response.json() throws an error, it means we have a communication error. This is incorrect, it will only throw if the request body isn't valid JSON (https://fetch.spec.whatwg.org/#dom-body-json). As long as we can be 100% sure that Meilisearch client will only respond with a valid JSON error body text, this will never throw. If that isn't the case, maybe we should instead use response.text().

async function httpResponseErrorHandler(response: Response): Promise<Response> {
if (!response.ok) {
let responseBody
try {
// If it is not possible to parse the return body it means there is none
// In which case it is a communication error with the Meilisearch instance
responseBody = await response.json()
} catch (e: any) {
// Not sure on how to test this part of the code.
throw new MeiliSearchCommunicationError(
response.statusText,
response,
response.url
)
}
// If the body is parsable, then it means Meilisearch returned a body with
// information on the error.
throw new MeiliSearchApiError(responseBody, response.status)
}
return response
}

A communication error, or a network error would rather occur in the code below, before it gets to httpResponseErrorHandler, as described in the docs, and would be caught below and processed by httpErrorHandler.

A fetch() promise only rejects when the request fails, for example, because of a badly-formed request URL or a network error. A fetch() promise does not reject if the server responds with HTTP status codes that indicate errors (404, 504, etc.). Instead, a then() handler must check the Response.ok and/or Response.status properties.

const response = await result.then((res: any) =>
httpResponseErrorHandler(res)
)
const parsedBody = await response.json().catch(() => undefined)
return parsedBody
} catch (e: any) {
const stack = e.stack
httpErrorHandler(e, stack, constructURL.toString())
}

Now, this httpErrorHandler messes with the error in such a way that information gets lost, which is another bug in itself, related to and detailed in #1612, and that's another issue that I should probably address separately.

@curquiza curquiza added the bug Something isn't working label May 20, 2024
@flevi29 flevi29 linked a pull request May 22, 2024 that will close this issue
@flevi29 flevi29 linked a pull request May 22, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants