-
-
Notifications
You must be signed in to change notification settings - Fork 762
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
Allow "Required: False" on Response headers - Fix #1261 #1293
Conversation
Hello @jmcs Any change this gets reviewed and merged? I would like to use in a project and we will need to fork the project to support our Regards |
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.
Hi @miguelgf , thanks for the PR! We have moved from master
to main
, could you rebase your PR (there will probably be a conflict) ?
connexion/decorators/response.py
Outdated
@@ -52,9 +52,10 @@ def validate_response(self, data, status_code, headers, url): | |||
|
|||
if response_definition and response_definition.get("headers"): | |||
# converting to set is needed to support python 2.7 | |||
response_definition_header_keys = set(response_definition.get("headers").keys()) | |||
required_header_keys = {k for (k, v) in response_definition.get("headers").items() | |||
if 'required' not in v or v['required']} |
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.
you can simplify this by using dict.get()
:
if v.get('required')
# or to be explicit:
if v.get('required', False)
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.
Great tip, changed
@Ruwann rebased against |
It seems that your code was correct, but another test failed because, imo, the test itself is incorrect. For OpenAPI 3, it is clearly in the spec what the expected behaviour for optional and required headers is, but it is not very clear for Swagger 2. This issue (OAI/OpenAPI-Specification#321) and this response (OAI/OpenAPI-Specification#321 (comment)) seem to indicate that that the response headers in Swagger 2 are optional. In OpenAPI 3, when the If you are up for it, you can adjust the corresponding test and swagger/openapi. But I'm also happy to do it myself in a PR to your branch or in a follow-up PR. |
@Ruwann feel free to change the code, I won't be able to make any changes until late today. Thanks! |
Co-authored-by: Ruwann <ruwan.lambrichts@ml6.eu>
@Ruwann I've reading what you sent and I see the issue, yes, the test is not testing the proper behaviour, but we can't test it on the Swagger 2, as there is no way to say that a header is required with the Header Object specification (https://swagger.io/specification/v2/#headerObject). This test can be perform on openapi 3, on swagger 2 this test has no sense (every header is always optional, based on their specification). We can change the test so it only pass on OpenAPI 3 and we don't pass it on Swagger 2, what do you think? |
Swagger2 response headers are optional, and have no 'required' attribute. OpenAPI3 response headers are optional by default, but can be specified to be required by setting 'required: true'.
Fixes #1261
Changes proposed in this pull request:
required: False
withoutNonConformingResponseHeaders
error