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

✨ Handle NotFound and UnprocessableEntity errors in middleware #3327

Merged
merged 13 commits into from
Nov 5, 2024

Conversation

ff137
Copy link
Contributor

@ff137 ff137 commented Nov 5, 2024

Closes #3322

Currently, NotFound and UnprocessableEntity errors would print stack traces in the logs. It is more sensible to record these as info logs, since they typically represent bad requests / client errors.

This PR contributes changes so that the new behaviour will print messages like:

Not Found error occurred during GET /connections/abe80270-051c-415b-9bb1-b74a86846276: Record not found: connection/abe80270-051c-415b-9bb1-b74a86846276
Unprocessable Entity occurred during GET /connections: {'querystring': {'their_public_did': ['Value None is not a valid DID']}}

A util method extract_validation_error_message was added to help print this marshmallow validation error, so that the logs still provide the error context previously contained in the stack trace.


🎨 Other minor amendments included as well:

  • logger.warn is deprecated in favour of logger.warning, so replaced one instance
  • there were 2 error logs ("Note: setting ledger to read-only mode"). I opted to set these to warning level
  • refactored ready_middleware to reduce complexity
  • modified a fatal log to critical instead of error level

@ff137
Copy link
Contributor Author

ff137 commented Nov 5, 2024

5.0% Duplication on New Code (required ≤ 3%)

Can we increase this threshold to something more pragmatic? 10-20% would make sense

@jamshale
Copy link
Contributor

jamshale commented Nov 5, 2024

5.0% Duplication on New Code (required ≤ 3%)

Can we increase this threshold to something more pragmatic? 10-20% would make sense

We can, but it's a bit of an annoying process because none of the maintainers have admin rights on the sonarcloud project right now. There is ways to set setting in the sonar-project.properties file and the github action but I haven't experimented much with this. For now I think we'll just have to comment when you think something should be ignored, but I would like to clean some of these reports up in the future.

jamshale
jamshale previously approved these changes Nov 5, 2024
Copy link
Contributor

@jamshale jamshale left a comment

Choose a reason for hiding this comment

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

👍

Copy link

sonarcloud bot commented Nov 5, 2024

@jamshale jamshale merged commit bb63ff1 into openwallet-foundation:main Nov 5, 2024
10 checks passed
@ff137 ff137 mentioned this pull request Nov 6, 2024
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.

Improve exception handling to reduce unnecessary stack traces
2 participants