-
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 Multi-Value Headers and Query String Parameters #741
Support for Multi-Value Headers and Query String Parameters #741
Conversation
@jfuss already started taking a peek at this PR. I will assign to him for review |
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 is looking great. A couple small comments/suggestions. Please make sure you are running integ-test and func-tests. This change will break any tests that we have which asserts the event object being passed into the function.
Add tests for multi headers and query string params:
Adjust this test to check both the query string and the multi query strings: https://github.com/awslabs/aws-sam-cli/blob/develop/tests/integration/local/start_api/test_start_api.py#L409. Adding a integ-test case for multi headers will also be beneficial.
Would you be able to adjust the APIGW events for sam local generate-event
command as apart of this as well? This will keep the multi query string and headers changes together: https://github.com/awslabs/aws-sam-cli/tree/develop/samcli/commands/local/lib/generated_sample_events/events/apigateway
Still working with integ-test and func-tests. |
@medinarrior Sounds good. Let me know if you run into trouble with them. |
Made changes, let me know if there is more to do :) |
@medinarrior Taking another pass through this now. Could you rebase when you get a chance? I don't think there should be any conflicts looking at the files. |
@jfuss Not able to see CodeBuild results. |
@medinarrior Ignore the codebuild. We need to remove it. |
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.
Just a couple small comments. This looks great and an awesome addition!
@jfuss Let me know if there is something I need to change :) |
@medinarrior Appologizes on the time it has taken to look at your update. I am reviewing 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.
Just a small comment/question/discussion point.
|
||
# Multi-value request headers is not really supported by Flask. | ||
# See https://github.com/pallets/flask/issues/850 | ||
for header_key in flask_request.headers.keys(): |
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.
What do you think about logging some information about not fully supporting multiple headers? We would have to print this each time, since we have no idea if the customer is impacted but would result in two things:
- It would give the customer some information when invoking (ideally linking out to a Github issue). This could result in less of a "Wait is this me or is this SAM CLI not giving the function the headers".
- It would give us (SAM CLI) a way to get feedback on how many customers are impacted by this through the Github Issue +1s.
I know this is current state and I think the only alternative is to rewrite the local service or possibly patch Flask in our implementation so we can support this. I just want to try and avoid the confusion and frustration that may come about from this.
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.
Good idea, what message should be displayed? Agree, it's time to consider patching Flask or rewriting the local 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.
Maybe something like:
"WARNING: Multi-value request headers are not fully supported. See issue: for more details."
We would just need to create the issue with some details on how Flask doesn't support this and therefore we currently can't.
Thoughts?
There was a conversation earlier that suggested Flask does not support multiple headers but a recent experiment does show this will work.
Produces:
|
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.
In general this looks good to me. 👍
@@ -191,7 +204,10 @@ def to_dict(self): | |||
"resource": self.resource, | |||
"requestContext": request_context_dict, | |||
"queryStringParameters": dict(self.query_string_params) if self.query_string_params else None, | |||
"multiValueQueryStringParameters": dict(self.multi_value_query_string_params) |
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 we need dict(self.multi_value_query_string_params)
again? we do validation above to make sure its a dict.
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.
Probably because someone could mutate the public variable on the object after initializing?
"headers": dict(self.headers) if self.headers else None, | ||
"multiValueHeaders": dict(self.multi_value_headers) if self.multi_value_headers else None, |
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.
same here
jfuss already reviewed this long back and the feedback has been addressed
Issue #735
Description of changes: Adding support for multi-value query string parameters.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.