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: add error handling for limit errors #210

Merged
merged 5 commits into from
Nov 10, 2023

Conversation

nick-watson
Copy link
Contributor

@nick-watson nick-watson commented Nov 7, 2023

  • Ready for review
  • Linked to Jira ticket

What does this PR do?

  • Adds error handling for catalogue (json api format) errors to throw errors with a detail
  • Handles analysis and create/extend bundle requests to detail limit errors thrown by SAA and FBS
  • Will be used in this PR for the CLI to display error to the user
  • User will see error message + user friendly detail + link to error catalogue
  • Jira

Screenshots

Screenshot 2023-11-08 at 17 17 05

@nick-watson nick-watson force-pushed the feat/handle-limit-catalogue-errors branch from 73ed509 to a95f1ef Compare November 7, 2023 19:19
@nick-watson nick-watson changed the title feat: add error handling for json api errors feat: add error handling for limit errors Nov 7, 2023
@nick-watson nick-watson force-pushed the feat/handle-limit-catalogue-errors branch 2 times, most recently from 6d79ba4 to 569d212 Compare November 8, 2023 13:07
@nick-watson nick-watson force-pushed the feat/handle-limit-catalogue-errors branch from 569d212 to 89ee52f Compare November 8, 2023 13:11
@nick-watson nick-watson marked this pull request as ready for review November 8, 2023 17:16
@nick-watson nick-watson requested a review from a team as a code owner November 8, 2023 17:16
src/http.ts Outdated Show resolved Hide resolved
src/needle.ts Outdated Show resolved Hide resolved
src/needle.ts Outdated Show resolved Hide resolved
src/http.ts Outdated Show resolved Hide resolved
src/http.ts Show resolved Hide resolved
src/http.ts Show resolved Hide resolved
const errors = response?.body?.errors;

if (isJsonApiErrors(errors)) {
jsonApiError = errors[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This behaviour (only handling first error) is not documented anywhere.
I don't see cases where we have more than one error from the API TBH, but know the JSON API requires arrays.
If someone were to change the API to actually return more than one error they will probably not realise the client can only handle one though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I guess this is a limitation right now, where would you think we can document it?

Copy link
Contributor

Choose a reason for hiding this comment

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

The good thing is if we have more than 1 error, nothing breaks. They will be ignored.

Perhaps a single comment from the service that throws the error?

@nick-watson nick-watson merged commit 9453361 into master Nov 10, 2023
4 checks passed
@nick-watson nick-watson deleted the feat/handle-limit-catalogue-errors branch November 10, 2023 14:42
@snyksec
Copy link

snyksec commented Nov 10, 2023

🎉 This PR is included in version 4.23.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants