Skip to content

Fix http.response.template being used with BaseHTTPMiddleware#473

Closed
blueyed wants to merge 4 commits intoKludex:masterfrom
blueyed:template-response
Closed

Fix http.response.template being used with BaseHTTPMiddleware#473
blueyed wants to merge 4 commits intoKludex:masterfrom
blueyed:template-response

Conversation

@blueyed
Copy link
Contributor

@blueyed blueyed commented Apr 9, 2019

Fixes #472.

@blueyed blueyed mentioned this pull request Apr 9, 2019
Closed
@blueyed
Copy link
Contributor Author

blueyed commented Apr 11, 2019

@tomchristie
Would appreciate some insight here.

lovelydinosaur
lovelydinosaur reviewed Apr 17, 2019
View reviewed changes
@blueyed
Copy link
Contributor Author

blueyed commented May 1, 2019

Only style issues.

Awaiting feedback before fixing this.

It is not very helpful btw ;) (should show the diff)

+black starlette tests setup.py --check
would reformat /home/travis/build/encode/starlette/starlette/middleware/base.py
All done! 💥 💔 💥
1 file would be reformatted, 60 files would be left unchanged.

@blueyed blueyed mentioned this pull request May 1, 2019
Closed
@em92
Copy link
Contributor

em92 commented May 1, 2019

It is not very helpful btw ;) (should show the diff)

Running ./scripts/lint does reformat code

@blueyed blueyed force-pushed the template-response branch from e5efd01 to c9b04ba Compare May 13, 2019 14:38
@blueyed blueyed force-pushed the template-response branch from c9b04ba to 958d796 Compare May 13, 2019 14:41
blueyed added a commit to blueyed/starlette that referenced this pull request May 13, 2019
@blueyed
Merge branch 'template-response' into next
e3ec97f 
Kludex#473
blueyed
blueyed commented May 13, 2019
View reviewed changes
starlette/middleware/base.py
raise RuntimeError("No response returned.")
assert message["type"] == "http.response.start"

if "http.response.template" in scope.get("extensions", {}):
Copy link
Contributor Author

@blueyed blueyed May 13, 2019

Choose a reason for hiding this comment

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

This is always True (https://codecov.io/gh/blueyed/starlette/compare/ef49e73e1043fdf7ce409a190ab632cfd568953e...e3ec97fcb16de9f3bb645ddb67495349c124dbb2/diff#D4-50).
I.e. we should maybe test with a TestClient that does not use it?!

@lovelydinosaur
Copy link
Contributor

lovelydinosaur commented May 20, 2019

On reflection, the smarter thing for us to do in middleware would be to wait for the first body message, before figuring out if we should treat it as a streaming response, or a standard response.
If we did that, then we could use Response, TemplateResponse, or StreamingResponse depending on the messages coming down the channel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@lovelydinosaur lovelydinosaur lovelydinosaur left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AssertionError with middleware and TemplateResponse

3 participants

@blueyed @em92 @lovelydinosaur

Comments