Skip to content
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

Implements Response.from_response and support for cf opts in Py fetch. #3229

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 40 additions & 8 deletions src/pyodide/internal/workers.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from contextlib import ExitStack, contextmanager
from enum import StrEnum
from http import HTTPMethod, HTTPStatus
from typing import TypedDict, Unpack
from typing import Any, TypedDict, Unpack

import js

Expand All @@ -21,10 +21,32 @@
Headers = dict[str, str] | list[tuple[str, str]]


# https://developers.cloudflare.com/workers/runtime-apis/request/#the-cf-property-requestinitcfproperties
class RequestInitCfProperties(TypedDict, total=False):
apps: bool | None
cacheEverything: bool | None
cacheKey: str | None
cacheTags: list[str] | None
cacheTtl: int
cacheTtlByStatus: dict[str, int]
image: (
Any | None
) # TODO: https://developers.cloudflare.com/images/transform-images/transform-via-workers/
mirage: bool | None
polish: str | None
resolveOverride: str | None
scrapeShield: bool | None
webp: bool | None


# This matches the Request options:
# https://developers.cloudflare.com/workers/runtime-apis/request/#options
class FetchKwargs(TypedDict, total=False):
headers: Headers | None
body: "Body | None"
method: HTTPMethod = HTTPMethod.GET
redirect: str | None
cf: RequestInitCfProperties | None


# TODO: Pyodide's FetchResponse.headers returns a dict[str, str] which means
Expand Down Expand Up @@ -99,7 +121,7 @@ def __init__(
self,
body: Body,
status: HTTPStatus | int = HTTPStatus.OK,
statusText="",
status_text="",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting is inconsistent here, does our lint check not check formatting?

headers: Headers = None,
):
"""
Expand All @@ -108,7 +130,7 @@ def __init__(
Based on the JS API of the same name:
https://developer.mozilla.org/en-US/docs/Web/API/Response/Response.
"""
options = self._create_options(status, statusText, headers)
options = self._create_options(status, status_text, headers)

# Initialise via the FetchResponse super-class which gives us access to
# methods that we would ordinarily have to redeclare.
Expand All @@ -117,15 +139,25 @@ def __init__(
)
super().__init__(js_resp.url, js_resp)

@classmethod
def from_response(cls, body: Body, response: "Response") -> "Response":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from_response is a bit of an odd name. Is there an existing api like this? I would think to make it a method called replace_body. e.g,

def replace_body(self, new_body: Body) -> "Response":
    """
    Returns a new Response object with the same headers but an updated body
    """"

b = body.js_object if isinstance(body, FormData) else body
result = cls.__new__(cls)
js_resp = js.Response.new(b, response.js_object)
super(Response, result).__init__(js_resp.url, js_resp)
Comment on lines +145 to +147
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain the reasoning for this? Ideally we should switch to:

Suggested change
result = cls.__new__(cls)
js_resp = js.Response.new(b, response.js_object)
super(Response, result).__init__(js_resp.url, js_resp)
js_resp = js.Response.new(b, response.js_object)
return cls(js_resp.url, js_resp)

or type(self)(js_resp.url, js_resp) assuming we make it into an instance method. If this is different in some important way we'll need a comment. Particularly, I would expect super(cls) instead of super(Response) which skips the __init__ functions of any subclasses.

return result

@staticmethod
def _create_options(
status: HTTPStatus | int = HTTPStatus.OK, statusText="", headers: Headers = None
status: HTTPStatus | int = HTTPStatus.OK,
status_text="",
headers: Headers = None,
):
options = {
"status": status.value if isinstance(status, HTTPStatus) else status,
}
if len(statusText) > 0:
options["statusText"] = statusText
if len(status_text) > 0:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does the same thing afaict:

Suggested change
if len(status_text) > 0:
if status_text:

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explicit is always better than implicit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it's most common in python code to check if a container is non empty with if container:. I think you'll find this suggested in most style guides as the preferred way.

options["statusText"] = status_text
if headers:
if isinstance(headers, list):
# We should have a list[tuple[str, str]]
Expand Down Expand Up @@ -154,10 +186,10 @@ def redirect(url: str, status: HTTPStatus | int = HTTPStatus.FOUND):
def json(
data: str | dict[str, str],
status: HTTPStatus | int = HTTPStatus.OK,
statusText="",
status_text="",
headers: Headers = None,
):
options = Response._create_options(status, statusText, headers)
options = Response._create_options(status, status_text, headers)
with _manage_pyproxies() as pyproxies:
try:
return js.Response.json(
Expand Down
34 changes: 32 additions & 2 deletions src/workerd/server/tests/python/sdk/worker.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,11 @@ async def on_fetch(request):
elif request.url.endswith("/undefined_opts"):
# This tests two things:
# * `Response.redirect` static method
# * that other options can be passed into `fetch`
resp = await fetch("https://example.com/redirect", redirect="manual")
# * that other options can be passed into `fetch` (so that we can support
# new options without updating this code)
resp = await fetch(
"https://example.com/redirect", redirect="manual", foobarbaz=42
)
Comment on lines +54 to +58
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could test that we receive foobarbaz on the other side by mocking the JS fetch. In this case, we just test that we don't crash with this option but we could discard it without failing the test?

return resp
elif request.url.endswith("/response_inherited"):
expected = "test123"
Expand Down Expand Up @@ -89,6 +92,14 @@ async def on_fetch(request):
assert data["blob.py"].content_type == "text/python"
assert data["metadata"].name == "metadata.json"

return Response("success")
elif request.url.endswith("/cf_opts"):
resp = await fetch(
"http://example.com/redirect",
redirect="manual",
cf={"cacheTtl": 5, "cacheEverything": True, "cacheKey": "someCustomKey"},
)
assert resp.status == 301
return Response("success")
else:
resp = await fetch("https://example.com/sub")
Expand Down Expand Up @@ -273,6 +284,23 @@ async def can_request_form_data_blob(env):
assert text == "success"


async def response_from_unit_tests(env):
response = Response("test", status=201, status_text="Created")
cloned = Response.from_response("other", response)
assert cloned.status == 201
assert cloned.status_text == "Created"
t = await cloned.text()
assert t == "other"


async def can_use_cf_fetch_opts(env):
response = await env.SELF.fetch(
"http://example.com/cf_opts",
)
text = await response.text()
assert text == "success"


async def test(ctrl, env):
await can_return_custom_fetch_response(env)
await can_modify_response(env)
Expand All @@ -287,3 +315,5 @@ async def test(ctrl, env):
await form_data_unit_tests(env)
await blob_unit_tests(env)
await can_request_form_data_blob(env)
await response_from_unit_tests(env)
await can_use_cf_fetch_opts(env)
Loading