Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Allows Content-Length for 304 Not Modified response #2321

Merged
merged 1 commit into from
Feb 15, 2018

Conversation

alefranz
Copy link
Contributor

Resolves #2099

Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

You're getting good at this 😄

@halter73
Copy link
Member

@Tratcher Care to review? Feel free to merge.

@Tratcher Tratcher merged commit 14332c5 into aspnet:dev Feb 15, 2018
@Tratcher Tratcher added this to the 2.1.0-preview2 milestone Feb 15, 2018
@Tratcher
Copy link
Member

Nice and simple

@halter73
Copy link
Member

@Tratcher I was thinking is that maybe we should verify that the actual length of the response body is zero for 304 responses instead of just skipping the response-body-too-short validation.

I didn't bring this up in the PR review because it's a separate issue and we'll probably want to do the same validation for HEAD requests. Do you think this is worth addressing?

@halter73
Copy link
Member

I'm not too familiar with the spec requires wrt this. Is a 304 response ever allowed to have a non-empty response body?

@Tratcher
Copy link
Member

Tratcher commented Feb 15, 2018

304 is pretty special, it's a lot like a head request. It explicitly must not have a response body, but it is allowed to describe the body it would have had if it were a 200.

@alefranz
Copy link
Contributor Author

@halter73 I thought about the response length validation as well, but as you said is a separate issue. Also it can be considered a breaking change.
I'm happy to have a look if I find a clean way to implement it.
Should I open a issue for discussion?

@halter73
Copy link
Member

We're good then. We already log and ignore response body writes for HEAD requests and 204/205/304 responses.

I'm assuming we should not allow a non-zero content length to be set for 204 and 205 responses.

@halter73
Copy link
Member

See here for the logic that decides whether or not to ignore writes to the response body.

@alefranz
Copy link
Contributor Author

304 is include, see here
and it's already handled as write throws if it can't have body.
Tests here

Nothing else to be done, right?

@halter73
Copy link
Member

I think we have everything covered. I just wanted someone to agree that allowing a non-zero Content-Length response header for 204 and 205 responses is silly.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants