Skip to content

Support multiValueHeaders on Response#129

Merged
jordaneremieff merged 2 commits intoKludex:mainfrom
koxudaxi:support_multi_value_headers
Jun 25, 2020
Merged

Support multiValueHeaders on Response#129
jordaneremieff merged 2 commits intoKludex:mainfrom
koxudaxi:support_multi_value_headers

Conversation

@koxudaxi
Copy link
Copy Markdown
Collaborator

@jordaneremieff
I suggest to support multiValueHeaders on Response.
I want to set the same-name cookies.
FastAPI + uvicorn can return same-name cookies.
But, Mangum returned only the last cookie. the first cookie is dropped.
Mangum should support multiValueHeaders to resolve the problem.

This change affects a little bit of performance.
If You prefer this PR, then I will add test the code.

How do you think about it?

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

example

from fastapi import FastAPI
from starlette.responses import Response
from mangum import Mangum
app = FastAPI()

@app.get('/')
def get_root():
    response = Response()
    response.set_cookie('test', 'value1')
    response.set_cookie('test', 'value2')
    return response


if __name__ == '__main__':
    import uvicorn
    uvicorn.run(app)
else:
    handler = Mangum(app)

output

curl -v http://127.0.0.1:8000/
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to 127.0.0.1 (127.0.0.1) port 8000 (#0)
> GET / HTTP/1.1
> Host: 127.0.0.1:8000
> User-Agent: curl/7.58.0
> Accept: */*
> 
< HTTP/1.1 200 OK
< date: Tue, 23 Jun 2020 15:19:09 GMT
< server: uvicorn
< set-cookie: test=value1; Path=/; SameSite=lax
< set-cookie: test=value2; Path=/; SameSite=lax
< transfer-encoding: chunked
< 
* Connection #0 to host 127.0.0.1 left intact

@jordaneremieff
Copy link
Copy Markdown
Collaborator

@koxudaxi Looks like a good addition, I will merge this when it is ready. Thanks!

@koxudaxi koxudaxi changed the title [WIP] Support multiValueHeaders on Response Support multiValueHeaders on Response Jun 24, 2020
@koxudaxi
Copy link
Copy Markdown
Collaborator Author

@jordaneremieff
I have added unit tests.

By the way, Why CI does not run for this PR?

@jordaneremieff
Copy link
Copy Markdown
Collaborator

@koxudaxi thanks. Not sure why it doesn't appear on Github, but the build does run successfully in CI https://travis-ci.org/github/erm/mangum/builds/701506590. I'll try to review this soon then will merge.

@jordaneremieff jordaneremieff merged commit bac03b7 into Kludex:main Jun 25, 2020
four43 pushed a commit to four43/mangum that referenced this pull request Mar 27, 2021
four43 pushed a commit to four43/mangum that referenced this pull request Aug 20, 2021
khamaileon pushed a commit to khamaileon/mangum that referenced this pull request Jan 13, 2024
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.

2 participants