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

Improve error handling. #172

Merged
merged 4 commits into from
Aug 27, 2024
Merged

Improve error handling. #172

merged 4 commits into from
Aug 27, 2024

Conversation

davidlehn
Copy link
Member

  • Attempt to improve error handling.
  • One goal is to return 400 instead of 500 for some common errors.
  • I wasn't sure where to put failure tests. Made a "misc" file that can run a handful of simple tests and check for errors. I think the boilerplate copied from another file could be simplified. Help wanted.
  • I'm not sure what this API should do for errors. Should any details be returned to client? There are many possible causes of errors coming from the issuer endpoint code. jsonld lib, vc lib, other libs could fail on input data. What should be exposed? How do we check for errors from libs like vc that only use generic 'Error' or 'TypeError'?

Addresses: #169.

lib/http.js Outdated Show resolved Hide resolved
Copy link
Contributor

@aljones15 aljones15 left a comment

Choose a reason for hiding this comment

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

Approving provided Author feels this is ready.

@aljones15
Copy link
Contributor

@davidlehn this might be something for a subsequent PR, but according to the VC-API this should be returning the verification results in the body of the response. Not sure if this PR is doing that.

@davidlehn
Copy link
Member Author

I was hoping for a bit more discussion here. This does handle one specific jsonld.js error code. But it doesn't handle any others, or any of the generic vc errors, so the first simple tests I wrote still fail. I'm not sure where the line is on exposing more details with cause, or what might need to be filtered out from being returned to clients.

@dlongley
Copy link
Member

@davidlehn,

I think we can add better error support incrementally -- just having what you have here is really useful so we should not wait on it.

davidlehn and others added 3 commits August 26, 2024 19:04
- Return 400 instead of 500 for some common errors.
Co-authored-by: Dave Longley <[email protected]>
- Don't wrap BedrockErrors.
- Special case error names that shouold be 'DataError'.
- Add more complex stack trace stripping.
- Update tests.
@davidlehn davidlehn force-pushed the improve-error-handling branch from eb53408 to a667db8 Compare August 26, 2024 23:05
@davidlehn davidlehn marked this pull request as ready for review August 26, 2024 23:08
@davidlehn davidlehn requested a review from dlongley August 26, 2024 23:08
@codecov-commenter
Copy link

codecov-commenter commented Aug 26, 2024

Codecov Report

Attention: Patch coverage is 90.76923% with 6 lines in your changes missing coverage. Please review.

Project coverage is 90.72%. Comparing base (c40f64d) to head (356d5fb).

Files Patch % Lines
lib/http.js 90.76% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #172      +/-   ##
==========================================
+ Coverage   90.53%   90.72%   +0.19%     
==========================================
  Files          17       17              
  Lines        3170     3235      +65     
==========================================
+ Hits         2870     2935      +65     
  Misses        300      300              
Files Coverage Δ
lib/http.js 89.88% <90.76%> (+0.50%) ⬆️

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c40f64d...356d5fb. Read the comment docs.

@davidlehn
Copy link
Member Author

This code got a bit tricky. There are a variety of errors that could be thrown, and it's unclear how dependent other code might be on what this API returns. This is trying to improve it a bit. The bedrock-vc-delivery code has similar work and there is some DRY opportunity here probably by moving some of the common code to a bedrock helper if possible. Suggestions or patches welcome.

throw error;
}

function _stripStackTrace(error) {
Copy link
Member

Choose a reason for hiding this comment

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

This implementation needs to be updated (it was taken from a source where some bugs were discovered):

https://github.com/digitalbazaar/bedrock-vc-delivery/blob/main/lib/helpers.js#L151-L170

Once that's updated, I think this is ready to go.

Copy link
Member Author

Choose a reason for hiding this comment

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

Aligned more with that code. jsonld.js doc loader is using .details.cause for not found errors, which also need stripping. Not sure if that's an issue in the other library as well.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's probably fine.

- Use `serialize-error` and allow list of error keys when create error.
- More alignment with bedrock-vc-delivery error code.
@davidlehn davidlehn merged commit 4b29431 into main Aug 27, 2024
4 checks passed
@davidlehn davidlehn deleted the improve-error-handling branch August 27, 2024 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants