Skip to content

Conversation

@Kludex
Copy link
Owner

@Kludex Kludex commented Jan 7, 2022

Closes #1178 (?)
Closes #1282

@Kludex Kludex requested a review from lovelydinosaur January 7, 2022 12:28
@Kludex Kludex requested a review from lovelydinosaur January 7, 2022 17:24
@Kludex Kludex mentioned this pull request Jan 7, 2022
8 tasks
@TBBle
Copy link

TBBle commented Jan 8, 2022

Looking at this change and prompted by #1395 (comment), what happens if you still pass a body in, i.e. Response(status_code=204, content="hi") in the test?

My skimming of the relevant ASGI 3 spec suggests the ASGI server may add Transfer-Encoding: chunked, which then becomes RFC-invalid again, as the presence of either of Content-Length or Transfer-Encoding signals a body, which is disallowed for 204 and related responses.

Perhaps the implicit expectation is that the ASGI server will use "may" here and recognise response codes for which Transfer-Encoding should not be added (and also implicitly drop the body data, perhaps)? Although that seems like a gap in the spec that it's not explicit, and a gap in behaviour that the same thing is not done for Content-Length, as the same HTTP spec restrictions apply.

I apologise if I'm asking a question with an obvious/known answer already. I'm new to the ASGI ecosystem, and may be misunderstanding the division of responsibility or otherwise.

@lovelydinosaur
Copy link
Contributor

what happens if you still pass a body in, i.e. Response(status_code=204, content="hi") in the test?

Probably you've just got yourself an RFC-invalid response.

We could potentially guard against those kinds of cases, and I'd be happy for us to discuss that as a follow-up issue if anyone's sufficiently motivated to do so.

The most important thing for this pull request tho, is to make sure we're doing the right thing for the sensible cases.

Copy link
Contributor

@lovelydinosaur lovelydinosaur left a comment

Choose a reason for hiding this comment

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

Looks great, yup.

Observations prompted by the PR...

I think our naming of populate_content_length / populate_content_type in the init_headers method isn't really ideal at this point, and I'd also consider us switching it to a plain function rather than a stateful method (because I think it's easier to understand the behaviour then).

Not something to address here, but perhaps worth consideration.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JSONResponse Wrong Content-Length Value Add EmptyResponse class

4 participants