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

Set Date and Server headers on response start #862

Merged
merged 1 commit into from
May 26, 2016
Merged

Set Date and Server headers on response start #862

merged 1 commit into from
May 26, 2016

Conversation

@khellang
Copy link
Contributor

@khellang khellang commented May 21, 2016 •
edited
Loading

Closes #223

// @Tratcher

@khellang khellang changed the title Set headers on response start Set Date and Server headers on response start May 21, 2016
khellang
khellang reviewed May 21, 2016
View reviewed changes
src/Microsoft.AspNetCore.Server.Kestrel/Infrastructure/Headers.cs Outdated

namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure
{
class Headers
Copy link
Contributor Author

@khellang khellang May 21, 2016

Choose a reason for hiding this comment

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

Is it worth keeping this class at this point?

Copy link
Contributor

@cesarblum cesarblum May 21, 2016

Choose a reason for hiding this comment

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

Seems rather pointless indeed 😄

@khellang khellang mentioned this pull request May 21, 2016
Merged
@khellang khellang force-pushed the set-headers-on-response-start branch 2 times, most recently from 0b29b07 to b7d7510 Compare May 21, 2016 02:37
cesarblum
cesarblum reviewed May 21, 2016
View reviewed changes
test/Microsoft.AspNetCore.Server.KestrelTests/FrameResponseHeadersTests.cs
{
[Theory]
[InlineData(true)]
[InlineData(false)]
Copy link
Contributor

@cesarblum cesarblum May 21, 2016

Choose a reason for hiding this comment

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

I'd rather have this converted into a test that checks those headers are not there.

@khellang
Copy link
Contributor Author

khellang commented May 23, 2016

🆙📅 - Ready for another review! // @CesarBS

halter73
halter73 reviewed May 24, 2016
View reviewed changes
test/Microsoft.AspNetCore.Server.KestrelTests/BadHttpRequestTests.cs
await connection.ReceiveEnd(
$"Date: {connection.Server.Context.DateHeaderValue}",
"Content-Length: 0",
"Server: Kestrel",
Copy link
Member

@halter73 halter73 May 24, 2016

Choose a reason for hiding this comment

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

Are there any remaining tests the verify that the "Server: Kestrel" header is sent by default?

@khellang khellang force-pushed the set-headers-on-response-start branch from d33cd86 to 55a310a Compare May 24, 2016 22:17
@halter73
Copy link
Member

halter73 commented May 24, 2016

I really think we need a test that verifies the "Server: Kestrel" header is sent by default.

@khellang khellang force-pushed the set-headers-on-response-start branch from a155bd5 to a7ec2ed Compare May 24, 2016 23:29
@khellang
Copy link
Contributor Author

khellang commented May 24, 2016

@halter73 Yep. Added a test that checks the default headers 😄

halter73
halter73 reviewed May 24, 2016
View reviewed changes
test/Microsoft.AspNetCore.Server.KestrelTests/DefaultHeaderTests.cs
await connection.ReceiveEnd(
"HTTP/1.1 200 OK",
$"Date: {testContext.DateHeaderValue}",
"Server: Kestrel",
Copy link
Member

@halter73 halter73 May 24, 2016

Choose a reason for hiding this comment

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

Awesome!

@halter73
Copy link
Member

halter73 commented May 24, 2016

I guess resetting IHttpRequestFeature.Headers between requests can be done in another PR. 🚢

@khellang khellang force-pushed the set-headers-on-response-start branch from a7ec2ed to 4f036a0 Compare May 24, 2016 23:49
@khellang khellang mentioned this pull request May 24, 2016
Closed
@khellang
Copy link
Contributor Author

khellang commented May 24, 2016

Rebased and resolved conflicts.

I guess resetting IHttpRequestFeature.Headers between requests can be done in another PR. 🚢

Filed #879

@cesarblum
Copy link
Contributor

cesarblum commented May 25, 2016

Looks good :shipit:

@halter73
Copy link
Member

halter73 commented May 25, 2016

LGTM too. Can you squash the commits?

@khellang khellang force-pushed the set-headers-on-response-start branch from 4f036a0 to 34d8f6c Compare May 25, 2016 21:48
@khellang
Copy link
Contributor Author

khellang commented May 25, 2016

Can you squash the commits?

Done 👍

@khellang khellang force-pushed the set-headers-on-response-start branch from 34d8f6c to f47c2ed Compare May 26, 2016 08:31
@khellang khellang force-pushed the set-headers-on-response-start branch from f47c2ed to 72cc0ff Compare May 26, 2016 15:38
@halter73 halter73 merged commit 72cc0ff into aspnet:dev May 26, 2016
@halter73
Copy link
Member

halter73 commented May 26, 2016

Thanks!

@khellang khellang deleted the set-headers-on-response-start branch May 26, 2016 22:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@khellang @halter73 @cesarblum @Tratcher @dnfclas