impl multi_value_headers #58
Conversation
| "CloudFront-Viewer-Country":[ | ||
| "US" | ||
| ], | ||
| "":[ |
There was a problem hiding this comment.
I'm not sure if this is a documentation bug but this is what aws has documented I'm not sure what the right external channels are but you may want to leverage internal channels to notify the team that maintains these or clarify in the docs what an empty by present header name means :)
There was a problem hiding this comment.
Good question; will do. Not entirely sure myself...
| @@ -119,11 +165,13 @@ where | |||
| where | |||
| A: MapAccess<'de>, | |||
| { | |||
| let mut headers = http::HeaderMap::new(); | |||
| let mut headers = map | |||
There was a problem hiding this comment.
took a liberty to make a small optimization here. serde's internal map model may provide a size hint for those building up deserialized maps to pre-allocate an appropriately sized map. I'm doing that here.
|
just realized multi_value_headers in the response isn't in this diff. putting that in the oven now. will be ready soon |
|
pushed a change to this branch to add multi value headers in response as well to complete the logical change |
|
Woah, this is great. Thank you! Reviewing. |
|
thanks @davidbarsky let me know if there's anything you'd like to change. I'm trying to break up these changes so they can be reviewed more easily/quickly. Let me know if bigger chunks are desirable and I can switch. |
|
No, these chunks are good—no need to change. I (and Stefano too, to a lesser degree) have been taking a break from work and open source. Things will probably pick up on the new year. Sorry about that. These changes look really solid at my first look; I’ll work through these tomorrow. And give you an definitive answer then. DM or shoot me an email if these languish for more than a day. |
| @@ -119,11 +165,13 @@ where | |||
| where | |||
| A: MapAccess<'de>, | |||
| { | |||
| let mut headers = http::HeaderMap::new(); | |||
| let mut headers = map | |||
| "CloudFront-Viewer-Country":[ | ||
| "US" | ||
| ], | ||
| "":[ |
There was a problem hiding this comment.
Good question; will do. Not entirely sure myself...
Issue #, if available:
Description of changes:
This is a followup to the previous lambda-http pull to add support for multi value headers. note this has no externally visible changes from a user perspective. These were previously represented which http::HeaderMap which abstracts over these cases already. This changes just adds parsing an tests for cases where aws events provide them.
heads up on what to look for next.
I plan on following up quickly with a pull to add support multi value query string parameters then support for transparent alb events. These are roughly the same with some notable differences outlined here, all of which I believe as surmountable. I wanted to get these changes out in iterative chunks go get the merge queue going faster. Expect fast follow ups
By submitting this pull request