Skip to content
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

Add support for multi value header responses #1214

Merged
merged 2 commits into from
Sep 13, 2019

Conversation

stealthycoin
Copy link
Contributor

API Gateway added support for a multi-value header dictionary whcih can
be returned. This commit adds support for it in Chalice.

implements #1205

API Gateway docs: https://docs.aws.amazon.com/apigateway/latest/developerguide/set-up-lambda-proxy-integrations.html#api-gateway-simple-proxy-for-lambda-output-format

@codecov-io
Copy link

codecov-io commented Aug 14, 2019

Codecov Report

Merging #1214 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1214      +/-   ##
==========================================
+ Coverage   96.04%   96.04%   +<.01%     
==========================================
  Files          28       28              
  Lines        5229     5238       +9     
  Branches      668      670       +2     
==========================================
+ Hits         5022     5031       +9     
  Misses        134      134              
  Partials       73       73
Impacted Files Coverage Δ
chalice/app.py 97.46% <100%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 562f280...2abf2f6. Read the comment docs.

@stealthycoin
Copy link
Contributor Author

The other option here would be to overload the headers dict and just look at if its a string or an list. Would be a better API. The reason I didn't do that is last time we tried to be clever and preserve backwards compatibility we caused problems that needed fixing.

Thoughts? This case seems simpler than the last one (#1165)

@stealthycoin stealthycoin force-pushed the multi-value-response branch 2 times, most recently from e1ba0d2 to 939724d Compare August 27, 2019 17:10
@jamesls
Copy link
Member

jamesls commented Sep 1, 2019

The reason I didn't do that is last time we tried to be clever and preserve backwards compatibility we caused problems that needed fixing.

Are you referring to #1165? If so, I think that's a little different in the sense that we're not changing any existing behavior and just supporting a case we previously weren't (a list value in the headers dict). Given we're not messing around with types or anything, I think this is much safer.

Plus, reading the doc page you linked, the interaction between headers and multiValueHeaders seems tricky and could be confusing fore people.

My preference is to just make a list-value automatically work. What do others think?

@kyleknap
Copy link
Contributor

kyleknap commented Sep 2, 2019

My preference is to just make a list-value automatically work. What do others think?

Yeah let's just make lists work because:

  1. I agree with James it is a safer change than the request one.
  2. It is consistent with what we do for requests and would be more intuitive.

API Gateway added support for a multi-value header dictionary whcih can
be returned. This commit adds support for it in Chalice.
@stealthycoin stealthycoin merged commit 1791175 into aws:master Sep 13, 2019
@stealthycoin stealthycoin deleted the multi-value-response branch September 13, 2019 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants