Support for Multi-Value Headers and Query String Parameters by medinarrior · Pull Request #741 · aws/aws-sam-cli

medinarrior · 2018-10-30T20:27:34Z

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.

sanathkr · 2018-10-31T02:36:03Z

@jfuss already started taking a peek at this PR. I will assign to him for review

jfuss

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:

https://github.com/awslabs/aws-sam-cli/blob/develop/tests/functional/local/apigw/test_local_apigw_service.py#L119

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

samcli/local/apigw/local_apigw_service.py

tests/unit/local/apigw/test_local_apigw_service.py

medinarrior · 2018-11-01T13:34:03Z

Still working with integ-test and func-tests.

jfuss · 2018-11-01T14:13:57Z

@medinarrior Sounds good. Let me know if you run into trouble with them.

medinarrior · 2018-11-06T16:01:15Z

Made changes, let me know if there is more to do :)

jfuss · 2018-12-03T18:30:27Z

@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.

medinarrior · 2018-12-03T18:50:44Z

@jfuss Not able to see CodeBuild results.

jfuss · 2018-12-03T18:52:59Z

@medinarrior Ignore the codebuild. We need to remove it.

jfuss

Just a couple small comments. This looks great and an awesome addition!

samcli/local/apigw/local_apigw_service.py

tests/integration/local/start_api/test_start_api.py

medinarrior · 2018-12-11T19:33:36Z

@jfuss Let me know if there is something I need to change :)

jfuss · 2019-01-08T16:09:55Z

@medinarrior Appologizes on the time it has taken to look at your update. I am reviewing now.

jfuss

Just a small comment/question/discussion point.

jfuss · 2019-01-08T16:58:39Z

samcli/local/apigw/local_apigw_service.py

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.

Good idea, what message should be displayed? Agree, it's time to consider patching Flask or rewriting the local service.

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?

jfuss · 2019-05-08T15:53:40Z

There was a conversation earlier that suggested Flask does not support multiple headers but a recent experiment does show this will work.

from flask import Flask, request
app = Flask(__name__)

@app.route("/")
def hello():
     return "get: " + json.dumps(request.headers.get('Accept')) + "\n" + \
     "get_all: " + json.dumps(request.headers.get_all('Accept')) + "\n" + \
     "getlist: " + json.dumps(request.headers.getlist('Accept')) + "\n" + \
     "environ: " + json.dumps(request.environ['HTTP_ACCEPT']) + "\n"

flask run

Produces:

curl -s http://127.0.0.1:5000 -H'Accept: Working' -H'Accept: Not Working'
get: "Working,Not Working"
get_all: ["Working,Not Working"]
getlist: ["Working,Not Working"]
environ: "Working,Not Working"

…eaders

sriram-mv

In general this looks good to me. 👍

sriram-mv · 2019-05-31T20:41:44Z

samcli/local/events/api_event.py

                     "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)


do we need dict(self.multi_value_query_string_params) again? we do validation above to make sure its a dict.

Probably because someone could mutate the public variable on the object after initializing?

sriram-mv · 2019-05-31T20:42:57Z

samcli/local/events/api_event.py

+                     "multiValueQueryStringParameters": dict(self.multi_value_query_string_params)
+                     if self.multi_value_query_string_params else None,
                     "headers": dict(self.headers) if self.headers else None,
+                     "multiValueHeaders": dict(self.multi_value_headers) if self.multi_value_headers else None,


jfuss already reviewed this long back and the feedback has been addressed

jfuss suggested changes Oct 31, 2018

View reviewed changes

jfuss self-assigned this Oct 31, 2018

jfuss previously requested changes Dec 3, 2018

View reviewed changes

Sarke mentioned this pull request Dec 5, 2018

Support for multiValueHeaders locally #830

Closed

martysweet mentioned this pull request Dec 7, 2018

Support for response multiValueHeaders locally #842

Closed

5 tasks

jfuss reviewed Jan 8, 2019

View reviewed changes

This was referenced May 8, 2019

Support QueryStringParameters for tools that have not yet support MultiValueQueryStringParameters awslabs/aws-lambda-go-api-proxy#37

Closed

Add support for query string parameters again awslabs/aws-lambda-go-api-proxy#38

Merged

Luis Alberto Medina Bravo and others added 9 commits May 23, 2019 14:37

Support for Multi-Value Headers and Query String Parameters

27adafd

Query String Parameters as lists

cbed252

APIGW events for sam local generate-event command

b487fd4

Single-value & Multi-value Query String into one function, same for H…

1caeef1

…eaders

Single-value & Multi-value Query String into one function, same for H…

0619e5c

…eaders

Check multi-value query & headers for integ-test

3663ec7

Rebuild

7676bbd

Code Review

4d27d89

Add tests for multiValueHeaders

10e2f95

sriram-mv reviewed May 31, 2019

View reviewed changes

sanathkr approved these changes Jun 6, 2019

View reviewed changes

sanathkr merged commit 5171db2 into aws:develop Jun 6, 2019

Conversation

medinarrior commented Oct 30, 2018

Uh oh!

sanathkr commented Oct 31, 2018

Uh oh!

jfuss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

medinarrior commented Nov 1, 2018

Uh oh!

jfuss commented Nov 1, 2018

Uh oh!

medinarrior commented Nov 6, 2018

Uh oh!

jfuss commented Dec 3, 2018

Uh oh!

medinarrior commented Dec 3, 2018

Uh oh!

jfuss commented Dec 3, 2018

Uh oh!

jfuss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

medinarrior commented Dec 11, 2018

Uh oh!

jfuss commented Jan 8, 2019

Uh oh!

jfuss left a comment

Choose a reason for hiding this comment

Uh oh!

jfuss Jan 8, 2019

Choose a reason for hiding this comment

Uh oh!

medinarrior Jan 15, 2019

Choose a reason for hiding this comment

Uh oh!

jfuss Jan 15, 2019

Choose a reason for hiding this comment

Uh oh!

jfuss commented May 8, 2019

Uh oh!

sriram-mv left a comment

Choose a reason for hiding this comment

Uh oh!

sriram-mv May 31, 2019

Choose a reason for hiding this comment

Uh oh!

sanathkr Jun 6, 2019

Choose a reason for hiding this comment

Uh oh!

sriram-mv May 31, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants