-
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
Support for response multiValueHeaders locally #842
Conversation
Hi @jfuss, could you let me know what else is required for this? Thanks! |
@martysweet Apologies on the slow response here. I am talking a look at this now. |
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.
I have a couple comments to help simplify the code further.
|
||
# Get the first part of the content-type header, to allow for extras such as text/html; charset=utf-8 | ||
content_type = lamba_response_headers['Content-Type'].split(";", 1)[0] | ||
best_match_mimetype = flask_request.accept_mimetypes.best_match([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.
How did you validate this is the correct behavior?
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.
A MIME type should contain only type/subtype
.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/MIME_types
However, the Content-Type header contains the MIME, then potentially extra charset information:
https://www.ietf.org/rfc/rfc2045.txt (page 12)
In the Augmented BNF notation of RFC 822, a Content-Type header field
value is defined as follows:
content := "Content-Type" ":" type "/" subtype
*(";" parameter)
; Matching of media type and subtype
; is ALWAYS case-insensitive.
Passing this extra charset information into flask_request.accept_mimetypes.best_match
results in Flask returning 500.
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.
@martysweet I should have been a little clearer. Does this behavior match API Gateway's? Meaning do they only look at the first Content-Type Header or do they compare with the list of all headers? Did we match documentation or what is observed from the service?
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.
@jfuss Ah understood. Looking into it, this might be more complex than just looking at Content-Type
, though a bit outside of this PR.
So your question is, if multiple Content-Type Headers are returned to API Gateway, which one is used for the comparison against the binary types list?
I can't find anything in the docs, will have to run some tests.
Docs regarding Accept
and Content-Type
(which doesn't answer the question).
https://docs.aws.amazon.com/apigateway/latest/developerguide/api-gateway-payload-encodings-configure-with-console.html
https://docs.aws.amazon.com/apigateway/latest/developerguide/api-gateway-payload-encodings-workflow.html
@martysweet I really would love for this to work. when I'm testing it using both your branch here, and apigw, they do act different. this code creates a comma-seperated list in a single header (which chrome, at least, doesnt like), and apigw creates multiple headers with the same key (in my tests I'm using set-cookie, but I'm sure it's for anything that it applies to) |
@everyonce, this isn't true, actually. |
Closing in favour of #1166 |
Issue #, if available:
Addresses is #830, supporting multiValueHeaders locally.
Description of changes:
Merges lamdba response headers and multiValueHeaders in accordance with guidance from here.
https://docs.aws.amazon.com/apigateway/latest/developerguide/set-up-lambda-proxy-integrations.html#api-gateway-simple-proxy-for-lambda-output-format
Within base64 logic, splits Content-Type by ;, to prevent issues with malformed Content types.
content_type = lamba_response_headers['Content-Type'].split(";", 1)[0]
_should_base64_decode_body
change - Guidance neededChecklist:
make pr
passesPlease let me know what else is required/missing! 😄 This PR plays well with #741, which is for requests.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.