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

feat: better error-handling for non-JSON responses #538

Merged
merged 5 commits into from
Jul 21, 2022

Conversation

kanadgupta
Copy link
Member

@kanadgupta kanadgupta commented Jul 20, 2022

🚥 Fix RM-4868

🧰 Changes

Right now, if we receive an error response from the API when running the openapi command, we have some not-so-great error handling where if the response body isn't JSON, we basically just write it off as a timeout error:

rdme/src/cmds/openapi.js

Lines 111 to 129 in 862d400

async function error(err) {
debug(`error response received with status code ${err.status}`);
try {
const parsedError = await err.json();
debug(`full error response: ${JSON.stringify(parsedError)}`);
return Promise.reject(new APIError(parsedError));
} catch (e) {
debug(`error parsing JSON with message: ${e.message}`);
if (e.message.includes('Unexpected token < in JSON')) {
return Promise.reject(
new Error(
"We're sorry, your upload request timed out. Please try again or split your file up into smaller chunks."
)
);
}
return Promise.reject(new Error('There was an error uploading!'));
}
}

Our debug logs in this situation aren't helpful either: since we blindly use res.json() to parse the response body, our debug logs consistently return an unhelpful Unexpected token < in JSON error.

In this PR, our approach to parsing the response from our API has been improved to better allow us to understand non-JSON response bodies. We now do some smarter inspection of the response to determine whether or not it's a timeout. It probably is in most cases, but now we can actually inspect the response body and determine that.

In order to do this, I refactored our generic handleRes handler to dynamically handle JSON and non-JSON responses better. I was able to eliminate some duplicate code and the openapi command now leverages uses handleRes for error-handling.

🧬 QA & Testing

Do tests pass and do the changes look good?

If you want to try this out with an OAS file that timeouts consistently, check out this comment. I confirmed locally with that file that the timeout error is thrown properly.

Screen Shot 2022-07-20 at 5 54 49 PM

I also confirmed that the debug logs now properly log non-JSON response bodies properly:

image

We already have this package installed as part of `form-data`, but now we're going to use it to parse content-type headers.
This is mostly functionally equivalent, but now we check the Content-Type of the response before parsing its body as JSON. That way we can inspect the body properly.
@kanadgupta kanadgupta added the enhancement New feature or request label Jul 20, 2022
@kanadgupta kanadgupta requested a review from erunion July 20, 2022 22:58
Comment on lines 558 to 562
.reply(400, '<html></html>');
.reply(400, '<title>Application Error</title>');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before, we just assumed any ol' HTML was a timeout. Yikes! Now, we actually look for snippets of the Heroku/Cloudflare error pages.

Copy link
Member

@erunion erunion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome

__tests__/cmds/openapi.test.js Outdated Show resolved Hide resolved
@kanadgupta kanadgupta merged commit 60b7f8f into main Jul 21, 2022
@kanadgupta kanadgupta deleted the kanad/rm-4868-add-better-debugging-for-non-json-api branch July 21, 2022 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants