Fix bug: Support list header value on Step Functions ApiGateway:Invoke integration #11681
Conversation
There was a problem hiding this comment.
Welcome to LocalStack! Thanks for raising your first Pull Request and landing in your contributions. Our team will reach out with any reviews or feedbacks that we have shortly. We recommend joining our Slack Community and share your PR on the #community channel to share your contributions with us. Please make sure you are following our contributing guidelines and our Code of Conduct.
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
b8bddba to
135c044
Compare
| @@ -174,6 +174,10 @@ def _headers_of(parameters: TaskParameters) -> Optional[dict]: | |||
| for forbidden_prefix in StateTaskServiceApiGateway._FORBIDDEN_HTTP_HEADERS_PREFIX: | |||
| if key.startswith(forbidden_prefix): | |||
| raise ValueError(f"The 'Headers' field contains unsupported values: {key}") | |||
|
|
|||
| if isinstance(headers.get(key), list): | |||
There was a problem hiding this comment.
We can probably access the value for key in headers just once here.
| @@ -174,6 +174,10 @@ def _headers_of(parameters: TaskParameters) -> Optional[dict]: | |||
| for forbidden_prefix in StateTaskServiceApiGateway._FORBIDDEN_HTTP_HEADERS_PREFIX: | |||
| if key.startswith(forbidden_prefix): | |||
| raise ValueError(f"The 'Headers' field contains unsupported values: {key}") | |||
|
|
|||
| if isinstance(headers.get(key), list): | |||
There was a problem hiding this comment.
Yes, it seems like checking for 1D lists is sufficient in this case, as headers tend to be string or list of strings
| @@ -174,6 +174,10 @@ def _headers_of(parameters: TaskParameters) -> Optional[dict]: | |||
| for forbidden_prefix in StateTaskServiceApiGateway._FORBIDDEN_HTTP_HEADERS_PREFIX: | |||
| if key.startswith(forbidden_prefix): | |||
| raise ValueError(f"The 'Headers' field contains unsupported values: {key}") | |||
|
|
|||
| if isinstance(headers.get(key), list): | |||
| headers[key] = str(headers.get(key)) | |||
There was a problem hiding this comment.
We could consider making the encoding slightly more deliberate here (comma separated string literals), perhaps using a json encoder or ','.join().
07aa296 to
54148cd
Compare
|
|
||
| value = headers.get(key) | ||
| if isinstance(value, list): | ||
| headers[key] = ",".join(value) |
There was a problem hiding this comment.
Thanks for applying the changes! We can probably wrap this string body between brackets ("h1,h2" -> "[h1,h2]"). It's interesting how the test cases did not highlight this formatting change.
There was a problem hiding this comment.
That seems to be breaking other tests 😅
Will check if those can be fixed
Motivation
Fix Issue:11628 and show off
Changes
Testing
Steps To Reproducementioned on this issue