Bug: Compression is applied after request caching

[chris-telemetry]

The CompressionMiddleware applies compression after CacheMiddleware caches the response and, on subsequent requests that hit the cache, applies compression every time.

I think in a perfect world the response would be cached after it is sent though I realize this comes with significant implementation issues.

Here's an easy way to reproduce:

from typing import Any
from starlite import (
    Request,
    Starlite,
    get,
)
from starlite.config.cache import CacheConfig
from starlite.config.compression import CompressionConfig
from starlite.config.logging import LoggingConfig
from starlite.datastructures.headers import MutableScopeHeaders
from starlite.middleware.base import MiddlewareProtocol
import uvicorn
from rich import print

from starlite.types.asgi_types import ASGIApp, Message, Receive, Scope, Send

@get('/test_txt', cache=True)
async def handle(request: Request) -> Any:
    with open('test.txt', 'r') as f:
        return f.read()
    
async def after_response(request: Request) -> None:
    cached_val = await request.app.cache.get('/test_txt')
    print('Cached val length', len(cached_val))

class InspectBodyMiddleware(MiddlewareProtocol):
    def __init__(self, app: ASGIApp) -> None:
        super().__init__(app)
        self.app = app

    async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
        if scope["type"] == "http":
            async def send_wrapper(message: Message) -> None:
                if message['type'] == "http.response.body":
                    print('Response val length',len(message['body']))
                await send(message)

            await self.app(scope, receive, send_wrapper)
        else:
            await self.app(scope, receive, send)

app = Starlite(
    route_handlers=[handle],
    after_response=after_response,
    logging_config=LoggingConfig(),
    cache_config=CacheConfig(),
    compression_config=CompressionConfig(backend="gzip", gzip_compress_level=9),
    middleware=[InspectBodyMiddleware]
)

def main() -> None:
    uvicorn.run("main:app", host="0.0.0.0", port=8001, reload=True) 

if __name__ == '__main__':
    main()

The output for me, using a 1MB file for test.txt is:

Response val length 499774
Cached val length 1057092

[provinzkraut]

So, the issue here is that there is no CacheMiddleware. Caching is handled within the responses, so naturally happens before the middleware stack is called.

[chris-telemetry]

I understand why it happens but I don't think it should happen. It seriously diminishes the value of Caching.

If I want a compressed response in my cache I have to do this:

def set(self, key: str, value: Any, expiration: int):
        response: Response = pickle.loads(value)
        data = gzip.compress(response.body, compresslevel=9)
        response.body = data
        response.headers["content-encoding"] = "gzip"
        response.headers["transfer-encoding"] = "chunked"
        re_pickled = pickle.dumps(response)

[provinzkraut]

I understand why it happens but I don't think it should happen

Not disagreeing with you, I was just adding some context!

[Goldziher]

I think the solution would be to caching into Middleware. This will be the most flexible option here.

Thoughts on this?

This is related also to the discussion in #1304

[provinzkraut]

I think the solution would be to caching into Middleware. This will be the most flexible option here.

I agree. It could be set up similar to the AllowedHostsMiddleware so the current configuration interface would stay the same.

This is related also to the discussion in #1304

A bit but I don't think it's a blocker. All proposals currently in discussion there would have limited impact on this and would work just fine.

[provinzkraut]

A bit but I don't think it's a blocker. All proposals currently in discussion there would have limited impact on this and would work just fine.

It would still be easier though to add that one first if we chose to accept it 👀

[provinzkraut]

So, I created a draft of this back in #1399, which I believe would solve this issue.

[provinzkraut]

@chris-telemetry I finally got around to fix this one (=
I will be included in the 2.2 release!