Conversation
| return logger | ||
|
|
||
|
|
||
| D = typing.TypeVar('D') |
There was a problem hiding this comment.
Let's remove this. I think it's fine to sprinkle # type: ignore where needed, and then revise the types later in a way that doesn't hurt readability.
|
|
||
| self.logger: logging.Logger = get_logger(self.log_level) | ||
|
|
||
| @staticmethod |
There was a problem hiding this comment.
The behaviour of the get_header method in the HTTP protocol class is similar enough that this should be moved out into a new utils.py file and then imported in both files. The method is otherwise fine. :)
| scope: Scope | ||
| body: bytes | ||
| text_mime_types: typing.List[str] | ||
| use_multi_value_headers: bool = True |
There was a problem hiding this comment.
Does this need a default? I think it should always be defined in the adapter when creating the protocol instance.
|
|
||
| self.response["body"] = body.decode() | ||
|
|
||
| if self.use_multi_value_headers == False: |
There was a problem hiding this comment.
if not self.use_multi_value_headers
| use_multi_value_headers = True | ||
| multi_value_query_string_params = event.get("multiValueQueryStringParameters") | ||
| elif event.get("queryStringParameters"): | ||
| multi_value_query_string_params = {k:[v] for k, v in event.get("queryStringParameters").items()} |
There was a problem hiding this comment.
If it isn't a lot of effort to include test coverage this, then please add it. Otherwise it's okay to add pragma: no cover for now.
|
@akdor1154 thanks! I've done just a little bit of review with some minor comments, but otherwise this seems like a good change. I'm a bit stretched for time lately, but I'll try to give it a more thorough review soon. Well done. :) |
|
@akdor1154 I've gone through your PR, great work! My only concern is in regards to always returning the def _handler(event, context):
handler = Mangum(app)
response = handler(event, context)
headers = response["headers"]
# do something with the headersBut I think it is okay to get this work in first, I can think more about it before the next release. Are you able to address the comments in the review? Otherwise, I can merge this to main and make those changes myself later. |
|
@akdor1154 just a heads up, I think I will merge this and then push any adjustments separately to prepare for the next release. Please let me know if you intend to push any other changes, otherwise I'll probably tie this off when I next have time. Thanks again! :) |
|
Ah hey, yep go for it. If I get time then I would be setting up some
integration tests before any changes to this pr. Unfortunately work has
pulled me onto something non AWS related so my mind hasn't been in this
space as much as I'd expected.
…On Sat., 1 Aug. 2020, 14:15 Jordan Eremieff, ***@***.***> wrote:
@akdor1154 <https://github.com/akdor1154> just a heads up, I think I will
merge this and then push any adjustments separately to prepare for the next
release. Please let me know if you intend to push any other changes,
otherwise I'll probably tie this off in the next day or two.
Thanks again! :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#133 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABTLYHY53MPQEE5TOUGB2ETR6OJEJANCNFSM4OVG5FOA>
.
|
|
@akdor1154 understandable. Thanks again, I'll go ahead and merge it in as-is when I go to prepare the next release and do the housekeeping at that point. |
|
I think I'll close this one for now because it will end up becoming stale, and I don't think I'll be able to work on it myself (this isn't a rejection of the idea). Feel free to open a fresh PR in future for this. |
2 participants
First up: sorry for dumping this on you with no prior discussion. I think it's the right solution but it is a bigger change than I initially thought would be necessary.
This PR refactors multiValueHeader support.
The main reason is ALBs: while API GW happily deals with responses with a mix of
multiValueHeadersandheaders, an ALB configured with a lambda backend will instead throw up a Bad Gateway error if the ALB's own multi_value_header configuration setting doesn't match what the backend returns. E.g. even the default error handler in mangum will trigger this behaviour behind an ALB configured with multi_value_headers.My approach has thus been:
The reason for the change in approach in test_http.py is that it's easy to lossily post-process multiValueHeaders into headers, but it would be a pain to go the other way and not lose data.
I have also made some changes to adaptor.py to fix the assumption that "headers" and "queryStringParameters" will always be present (in an ALB configured with multi_value_headers, they won't be).
One side-effect of these changes is that in API GW, Mangum will now always respond with
multiValueHeadersand notheaders, becuase API GW always sendsmultiValueHeadersin its events (according to docs).Hope this isn't too horrible to get through :)
Jarrad