Respond with problems by default in aiohttp. #952
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix the exception handling so that if someone raises either HTTPRedirection (3xx) or HTTPSuccessful (2xx) exceptions, they do not get transformed into a problem with 500 Internal Server Error.
Fix the order of the problem middlewares so that the oauth_problem_middleware can handle the auth problems first. Also make sure to catch any werkzeug Exceptions, as OAuthProblem subclasses a werkzeug exception, and others might mistakenly use the werkzeug exceptions instead of the aiohttp exceptions.
The generic werkzeug handling works better because it generates valid problem json. oauth_problem_middleware just passed the description in the response body with the problem json content type. Also renames error_to_problem_middleware to probems_middleware.
cognifloyd
force-pushed
the
aiohttp_problems
branch
from
0c5c646
to
b41bbe4
Compare
Also, clean up the detail output to ensure that detail messages are helpful by default.
cognifloyd
force-pushed
the
aiohttp_problems
branch
2 times, most recently
from
561e467
to
39b7564
Compare
The test cannot pass without revisiting how aiohttp_api uses _cast_body. Instead of using the response content_type, it is casting the body based on the openapi spec defined mediatype, which does not match in this case when trying to return application/problem+json. This also leaves an open question: How should problems be serialized if the spec defines a format other than application/problem+json, or application/json, etc? Here, a simple plain/text triggers the issue. Ultimate reason for skipping the test (recorded in the test): aiohttp_api.get_connexion_response uses _cast_body to stringify the dict directly instead of using json.dumps. This differs from flask usage, where there is no _cast_body.
cognifloyd
force-pushed
the
aiohttp_problems
branch
from
1be46a3
to
ecf2361
Compare
|
This is ready for review, and hopefully to be merged :) |
jmcs
approved these changes
Jun 14, 2019
|
👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
AioHttp is not responding with problems yet. This uses an aiohttp middleware to handle exceptions and return a problem response.
Done:
test_errors.Open questions:
Changes proposed in this pull request:
oauth_problem_middlewarein favor of handling all the problem output inproblems_middlewarebecauseoauth_problem_middlewaredidn't actually return valid problem json, but this does.