karpetrosyan / hishel

An elegant HTTP Cache implementation for HTTPX and HTTP Core.
https://hishel.com
BSD 3-Clause "New" or "Revised" License
175 stars 22 forks source link

Revalidation requests should pass through extensions from caller. #248

Closed hartym closed 4 months ago

hartym commented 4 months ago

Fixes #247 behaviour so that transport implementation behaves the same as pool implementation.

I used unittest.mock.AsyncMock and its sync counterpart to check that the extensions are actually passed to the decorated transport/pool, I know this is not an existing practice in the current codebase, please tell me if you have an alternative for this.

karpetrosyan commented 4 months ago

Can we use something like this, as an alternative ?

import hishel
from hishel._utils import BaseClock
import httpx

from unittest.mock import Mock

class ExtensionMirrorTransport(hishel.MockTransport):

    def handle_request(self, request: httpx.Request) -> httpx.Response:
        response = super().handle_request(request)
        response.extensions["request_extensions"] = request.extensions
        return response

def test_transport_revalidation_forward_extensions():
    class MockedClock(BaseClock):
        current = 1440504000  # Mon, 25 Aug 2015 12:00:00 GMT

        def now(self) -> int:
            return self.current

    clock = MockedClock()
    controller = hishel.Controller(clock=clock)

    with ExtensionMirrorTransport() as transport:
        transport.add_responses(
            [
                httpx.Response(
                    200,
                    headers=[
                        (b"Cache-Control", b"max-age=1"),
                        (b"Date", b"Mon, 25 Aug 2015 12:00:00 GMT"),
                    ],
                ),
                httpx.Response(
                    304,
                    headers=[
                        (b"Cache-Control", b"max-age=1"),
                        (b"Date", b"Mon, 25 Aug 2015 12:00:01 GMT"),
                    ],
            ]
        )
        with hishel.CacheTransport(
            transport=transport, controller=controller, storage=hishel.InMemoryStorage()
        ) as cache_transport:
            # first request with extensions
            response = cache_transport.handle_request(
                httpx.Request("GET", "https://www.example.com", extensions={"foo": "bar"})
            )
            assert response.extensions["request_extensions"]["foo"] == "bar"

            # cache expires
            clock.current += 1

            # second request with extensions that should be passed to revalidation request
            response = cache_transport.handle_request(
                httpx.Request("GET", "https://www.example.com", extensions={"foo": "baz"})
            )
            assert response.extensions["revalidated"] is True
            assert response.extensions["request_extensions"]["foo"] == "baz"
hartym commented 4 months ago
    def handle_request(self, request: httpx.Request) -> httpx.Response:
        response = super().handle_request(request)
        response.extensions["request_extensions"] = request.extensions
        return response

Ok

hartym commented 4 months ago

Does not really work though, because if the revalidation response is not sent back (because revalidation indicated the stale cached version could be used, for example), then this "mirror" won't be available from the caller. using the response as a bearer looks clumsy anyway, I'll try another approach.

hartym commented 4 months ago

Any chance to get this merged/released? Is there anything more you're waiting from me on this regard? Thanks!

karpetrosyan commented 4 months ago

Sorry for delayed answer, I have tried to figure out why extension mirroring didn't work. I guess it's not because we are getting the response directly from the cache, it's because we are getting the original response, and not the response of the revalidation request, so yes, your solution seems like fixed that problem.

karpetrosyan commented 4 months ago

Thanks for the PR 🙏