copy over some tests from alternative implementation #1694
Conversation
| if not response_started: | ||
| raise RuntimeError("No response returned.") |
There was a problem hiding this comment.
I don't think we need this here, we can just let the server handle it which is what would happen if we didn't install this middleware. I think we only do it in BaseHTTPMiddleware out of necessity / implementation detail.
There was a problem hiding this comment.
True, eg. Uvicorn handles that here: https://github.com/encode/uvicorn/blob/f3c33fe7bca90326e38f66181f4623d8558571fb/uvicorn/protocols/http/h11_impl.py#L386-L389
| ) -> None: | ||
| async def dispatch(request: HTTPConnection) -> AsyncGenerator[None, Response]: | ||
| resp = yield | ||
| resp.media_type = "text/csv" |
There was a problem hiding this comment.
This would be strange usage without modifying the response body, wouldn't it? Is there a situation where we'd actually do this w/o having to modify the response body and Content-Type header, which would require pure ASGI middleware (or the body modification API you thought about as a possible future improvement)?
There was a problem hiding this comment.
Yeah I'm not sure what the use case would be, but you are able to set the attribute, so maybe we should error in this situation instead by overriding Response.__setattr__?
There was a problem hiding this comment.
I'm not sure we need to be that defensive, to be honest. If we're clear enough in the documentation (which ought to be written before we consider merging this anyway, right?) that this is a "simplified response interface, allowing to inspect the status code and headers, or tweak headers", then maybe that's enough.
Though, now that I think of it — what if users do want to inspect response.body in the "on response" part of the dispatch flow? Right now, it's empty, and actually makes no sense because this is called before we start processing any chunk of the body. Hmm.
There was a problem hiding this comment.
Yeah I don't think there's any behavior that's right or wrong here, it's just about what will be least confusing.
|
I think you copied most of this over, I'll close this, thanks @florimondmanca |
Copying over tests from my implementation.
This drops the
contextvartests which in my opinion are unnecessary with this implementation.It also adds types to the test file.
There's two main test cases being added:
Response.media_type. In my implementation I was detecting this and overwriting the header, but I guess we could subclassResponseand raise an exception if someone attempts to assign it?