-
Notifications
You must be signed in to change notification settings - Fork 403
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(api-gateway): add common HTTP service errors #506
Conversation
Changes: - Add base ServiceError exception for rest api service errors - BaseRequestError for 400s - UnauthorizedError for 401s - NotFoundError for 404s - InternalServerError for 500s
@heitorlessa I am not sure which of the top service errors to include. So if there are more or less I can update it. Otherwise I can add some docs on usage. |
Codecov Report
@@ Coverage Diff @@
## develop #506 +/- ##
========================================
Coverage 99.90% 99.90%
========================================
Files 105 107 +2
Lines 4238 4266 +28
Branches 208 208
========================================
+ Hits 4234 4262 +28
Misses 1 1
Partials 3 3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks for adding this one in, Mike! Two minors only - 1/ Moving exceptions to exceptions.py
, and 2/ Whether we want to have content_types.py
in anticipation that binary mime types will be asked
Fun fact I saw this week which I didn't know is that Python has HTTP Status as a built-in enum. I think the way it's now suffice tbh but thought you'd want to know: from http import HTTPStatus
HTTPStatus.BAD_REQUEST.value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explained why exceptions wouldn't be reused in AppSync. Exceptions is the only change pending
To make things easier in terms of import, I'm totally fine with having the localized from aws_lambda_powertools.event_handler.api_gateway import ApiGatewayResolver, NotFoundError For those wanting more explicitness, they could: from aws_lambda_powertools.event_handler.api_gateway import ApiGatewayResolver
from aws_lambda_powertools.event_handler.api_gateway.exceptions import NotFoundError Alternatively, if you want to keep exceptions within the utility namespace, then we'd need more explicit names to not confuse customers when to use them in AppSync, API Gateway, from aws_lambda_powertools.event_handler.api_gateway import ApiGatewayResolver
from aws_lambda_powertools.event_handler.exceptions import ApiGatewayNotFoundError |
Merging as-is and fixing docstrings to not confuse AppSync customers accidentally using these exceptions meant for API GW/ALB in |
@@ -0,0 +1,2 @@ | |||
APPLICATION_JSON = "application/json" | |||
PLAIN_TEXT = "plain/text" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be “text/plain”
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching it, fixed now in develop w/ a note on mimetypes
lib.
Issue #, if available:
Pairs with #507
Description of changes:
Changes:
Example usage for a unauthorized error
will return:
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.