-
Notifications
You must be signed in to change notification settings - Fork 290
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
Python: Fix header conversion to byte-pair on scope building #2125
base: main
Are you sure you want to change the base?
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code change makes sense. Ideally we ought to add a test.
@@ -25,7 +25,7 @@ def acquire_js_buffer(pybuffer): | |||
def request_to_scope(req, env, ws=False): | |||
from js import URL | |||
|
|||
headers = [tuple(x) for x in req.headers] | |||
headers = [(k.lower().encode(), v.encode()) for k, v in req.headers] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In lieu of adding a test, can we add a comment here with an example that cares about this being bytes so that we can add a test ourselves later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It assumes FastAPI so that would be a main concern, but its for ASGI spec though so should I add for FastAPI or just asserting for spec?
Like, anything for
@app.get("/example")
async def example(request: Request):
request.headers.get("content-type") # this will error if header is not bytes.
perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also is there way to test this locally? I want to build miniflare with master workerd, but building workers-sdk doesnt works..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can build workerd by itself and then set an environment variable to tell wrangler to use your custom build. @dom96 are there good instructions somewhere that we can point contributors to?
I guess I should check it out and test it myself...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be easier to build and run workerd locally. These are probably the best instructions we have for doing this: https://github.com/cloudflare/workerd?tab=readme-ov-file#configuring-workerd.
We also have some sample Python worker configs in the repo already, for example https://github.com/cloudflare/workerd/tree/main/samples/pyodide-fastapi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, so I can use workerd directly! Thanks, now I can test few things.
That's not the only trouble w/ ASGI and binary strings. We also need to decode resulting headers in
|
Confirmed! This patch fixed the problem with b'' I've seen before |
Yes, right. I fixed it. Thanks! |
src/pyodide/internal/asgi.py
Outdated
@@ -111,7 +115,7 @@ async def send(got): | |||
nonlocal headers | |||
if got["type"] == "http.response.start": | |||
status = got["status"] | |||
headers = got["headers"] | |||
headers = [(k.decode(), v.decode()) for k, v in got['headers']] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@morgan9e I'd use double quotes here to be compliant w/ folks' formatting standards
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working on it!
8182a0c
to
090673d
Compare
Since #2096 merged, we're actually running Python tests on workerd so it should be possible to add a test now. |
Agreed we should make it independent. Currently the only thing in there that is dependent on fastapi is |
Workerd uses Pyodide JSProxy object when passing
fetch(request, env)
to Python, but JSProxy objects translates into string, not byte-string.In order to match ASGI specification, it needs to be translated into byte-pair when building scopes.