Skip to content
This repository has been archived by the owner on Jan 24, 2021. It is now read-only.

Overwrite Content-Type from headers #1723

Merged
merged 2 commits into from Oct 17, 2014

Conversation

albertjan
Copy link
Contributor

Should overwrite the content-type from the headers when it's set there. Don't you think?

{
get
{
var header = Headers.Keys.FirstOrDefault(h => h.ToLowerInvariant() == "content-type");
Copy link
Member

Choose a reason for hiding this comment

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

h => String.Equals(h, "Content-Type", StringComparison.OrdinalIgnoreCase)

@khellang
Copy link
Member

HTTP header fields are case sensitive, so there should be no need to do any case magic. Just grab Content-Type out of there 😉

@khellang
Copy link
Member

Never mind. Can't find it in the new RFCs but according to RFC2616 they're case-insensitive:

Each header field consists of a name followed by a colon (":") and the field value. Field names are case-insensitive.

I'd prefer the @jchannon way though 😄

@albertjan albertjan force-pushed the overwrite-content-type branch 2 times, most recently from ed91d21 to 015e055 Compare October 16, 2014 16:57
@albertjan
Copy link
Contributor Author

@khellang @jchannon Better?

@albertjan
Copy link
Contributor Author

Might be better to use a case insensitive dictionary for the headers. But we might break al sorts of apps sneakily relying on case-sensitive headers.

@jchannon
Copy link
Member

Needs to be ordinal ignore case

@albertjan
Copy link
Contributor Author

@jchannon Sorry 😄

Response response = value;

// When
response.Headers.Add("content-type", "application/json");
Copy link
Member

Choose a reason for hiding this comment

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

Nitpicking: I would wEiRDcaSe the "content-type" string to demonstrate that case is ignored

@khellang
Copy link
Member

Looks like the response headers haven't received the same love as the request headers. Those are kept in a case-insensitive dictionary. See RequestHeaders.

@albertjan
Copy link
Contributor Author

@khellang soundslike a plan. Not worried about breaking stuff?

@khellang
Copy link
Member

I don't mind. I'd actually love to see a ResponseHeaders as well, with typed properties, but that could wait for another PR 😉

@albertjan
Copy link
Contributor Author

I was feeling a bit of scope creep going on 😄

@albertjan
Copy link
Contributor Author

@khellang this looks a lot cleaner

@horsdal
Copy link
Member

horsdal commented Oct 17, 2014

Looks good now 👍

horsdal added a commit that referenced this pull request Oct 17, 2014
Overwrite Content-Type from headers
@horsdal horsdal merged commit ec9edda into NancyFx:master Oct 17, 2014
@khellang khellang added this to the 1.0 Alpha milestone Oct 17, 2014
@grumpydev grumpydev modified the milestones: 1.0 Alpha, 1.0 Feb 4, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants