Skip to content

Commit

Permalink
Don't proxy the full response headers (PP-1713) (#2055)
Browse files Browse the repository at this point in the history
This solves an issue we were seeing where the Content-Encoding header is set on the request, but since Python requests automatically decodes the transfer encoding, we were mistakenly forwarding on the Content-Encoding header, without encoded data.
  • Loading branch information
jonathangreen authored Sep 12, 2024
1 parent 21cf29c commit ddbd903
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 3 deletions.
16 changes: 14 additions & 2 deletions src/palace/manager/api/circulation.py
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,14 @@ def response(self) -> Response:
)


class FetchResponse(Response):
"""
Response object that defaults to no mimetype if none is provided.
"""

default_mimetype = None


class FetchFulfillment(UrlFulfillment, LoggerMixin):
"""
Fulfill a loan by fetching a URL and returning the content. This should be
Expand Down Expand Up @@ -314,12 +322,16 @@ def response(self) -> Response:
)
raise

headers = dict(response.headers)
headers = {"Cache-Control": "private"}

if self.content_type:
headers["Content-Type"] = self.content_type
elif "Content-Type" in response.headers:
headers["Content-Type"] = response.headers["Content-Type"]

return Response(response.content, status=response.status_code, headers=headers)
return FetchResponse(
response.content, status=response.status_code, headers=headers
)


class LoanInfo(CirculationInfo):
Expand Down
16 changes: 15 additions & 1 deletion tests/manager/api/test_circulation.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,10 @@ class TestFetchFulfillment:
def test_fetch_fulfillment(self) -> None:
http = MockHTTPClient()
http.queue_response(
204, content="This is some content.", media_type="application/xyz"
204,
content="This is some content.",
media_type="application/xyz",
other_headers={"X-Test": "test"},
)
fulfillment = FetchFulfillment("http://some.location", "foo/bar")
with http.patch():
Expand All @@ -68,6 +71,7 @@ def test_fetch_fulfillment(self) -> None:
# Any content type set on the fulfillment, overrides the content type from the request.
assert response.content_type == "foo/bar"
assert http.requests == ["http://some.location"]
assert "X-Test" not in response.headers

# If no content type is set on the fulfillment, the content type from the request is used.
http = MockHTTPClient()
Expand All @@ -83,6 +87,16 @@ def test_fetch_fulfillment(self) -> None:
[(args, kwargs)] = http.requests_args
assert kwargs["allow_redirects"] is True

# If the content type is not set on the fulfillment, and the response does not have a content type,
# we fall back to no content type.
http = MockHTTPClient()
http.queue_response(200, content="Other content.")
fulfillment = FetchFulfillment("http://some.other.location")
with http.patch():
response = fulfillment.response()
assert isinstance(response, Response)
assert response.content_type is None

def test_fetch_fulfillment_include_headers(self) -> None:
# If include_headers is set, the headers are set when the fetch is made, but
# not included in the response.
Expand Down

0 comments on commit ddbd903

Please sign in to comment.