-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Local support for response multiValueHeaders #1166
Conversation
@dsanders11 I believe |
@jfuss, I think you may have missed that That PR also added a functional test for the case-insensitivity, which this PR continues to pass. |
@dsanders11 Yes, yes I did. I think that generally makes sense. Sorry for the oversight. |
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.
Overall this looks pretty good. My main request is to add additional integ tests that cover this multiple header case(s). A good place to put them would be under this test class: https://github.com/awslabs/aws-sam-cli/blob/develop/tests/integration/local/start_api/test_start_api.py#L291 We will need to add another function under the testdata directory so that it returns the multiple headers back from the function.
@@ -271,11 +274,44 @@ def _should_base64_decode_body(binary_types, flask_request, lamba_response_heade | |||
True if the body from the request should be converted to binary, otherwise false | |||
|
|||
""" | |||
best_match_mimetype = flask_request.accept_mimetypes.best_match([lamba_response_headers["Content-Type"]]) | |||
best_match_mimetype = flask_request.accept_mimetypes.best_match(lamba_response_headers.get_all("Content-Type")) |
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.
Do you know if this is the right behavior that matches API Gateway? Seems reasonable just want to make sure we emulate the service as closely as possible.
I assume by the testdata directory you mean https://github.com/awslabs/aws-sam-cli/tree/develop/tests/integration/testdata/start_api? Taking a quick look, I see there's a |
Correct. That is the test data for that Test Class I linked. The |
@jfuss, thanks for the info. I'll try to get a test written there in the next couple days, and confirm the behavior of |
This is an even bigger issue now, since v0.16.0 adds overly strict checks on the response, and errors with any that contain See: https://github.com/awslabs/aws-sam-cli/blob/43a7c37/samcli/local/apigw/local_apigw_service.py#L242 |
@dsanders11: how's this looking? I'm blocked on testing golang lambda handlers until this is merged. EDIT: I was able to revert to
(this should've been easier, but now I'm unblocked 😄) |
fec27f9
to
d53225c
Compare
@dsanders11 I added integ tests and rebased on top of develop (required a force push sorry). Not fair to put you in the hot path due to something we just released. @sanathkr, @awood45, or @thesriram Can you take over reviewing as a different set of eyes. |
@jfuss, I was actually writing the integration test today (no big deal), but got sidetracked investigating the That led to me noticing that API Gateway doesn't seem to be behaving like the documentation says it should (and how this PR is written). Emphasis on the important part: From my testing, here's what I'm seeing in the AWS Console when testing:
Am I misreading the documentation, or is that not behaving like it says it should behave? It's merging the values from P.S. Regarding the breakage for some people, seems like you should be able to grab the very isolated change of allowing |
@dsanders11 My understanding from the docs is: the final header list is the set of headers and multiValueHeaders. I think what it’s trying to say is if multiValueHeaders also has text/html in the list (in your example), text/html would only appear once is the final headers that apigateway returns. |
@jfuss, that does seem to be the actual behavior on testing. Unfortunate that the documentation is ambiguous as both @martysweet and I interpreted it differently. I think it's the plural nature of 'only the values from multiValueHeaders will appear in the merged list.' I've updated the code to act more like API Gateway does in actuality. This includes mimicking the order of header values when the same header appears in both The integration test will need to be updated to match this change. Unfortunately I'm out of time to work on this anymore this week. I spent an hour trying to get base64 decoding working on API Gateway so I could confirm behavior, but was not able to do so despite seemingly following everything to a T. As such that part of this PR is still a question mark on if it exactly follows API Gateway. |
@dsanders11 We really appreciate you (and @martysweet) diving into and ensuring this accurately reflects Api Gateway's behavior. I can take over the last bit here. |
Is this the root cause of #1190? If yes, can this be merged and released soon as this is causing issues during our development. |
Issue #, if available:
#830
Description of changes:
This is a rework of #842 by @martysweet, which seems to have stalled. This PR uses Flask's built-in method of providing multiple header values, which makes it act like API GW. In particular, multi-value headers are folded into a comma-separated list unless it's
Set-Cookie
which is a special-case.There's two parts to this PR. First drops
CaseInsensitiveDict
as it doesn't seem to be necessary. TheHeaders
class in Flask is already case-insensitive and swapping inHeaders
in the test case forCaseInsensitiveDict
passed the tests. There are some minor differences:Headers
isn't JSON serializable by defaultHeaders
won't pass equality comparison with a dictHeaders
raiseswerkzeug.exceptions.BadRequestKeyError
(a subclass ofKeyError
) on bad keyThose could be addressed with a custom subclass of
Headers
if necessary, but at the moment all tests pass just fine.The second commit is mostly the same as #842 but uses the default behavior of
Headers
to handle multi-value headers instead of manually merging them as a comma-separated list. This lets Flask do the special-case forSet-Cookie
.@martysweet, @jfuss, @everyonce, you guys want to take a look since you were active on #842?
Checklist:
make pr
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.