Django

Code

Ticket #2970 (closed: fixed)

Opened 2 years ago

Last modified 11 months ago

[patch] HttpResponse should treat headers case-insensitively

Reported by: adrian Assigned to: PhiR
Milestone: Component: HTTP handling
Version: Keywords: sprintsept14
Cc: Triage Stage: Design decision needed
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

Description

(Submitted by Bryan O'Sullivan, who had problems with Trac's spam filter.)

The HttpResponse implementation uses a normal Python dict to store headers. This makes it necessary to have every component that sets headers in a response agree on the exact case to use for every header name. I found this problem while trying to glue a WSGI app into a Django app, only to discover that one believes that a header should be named "content-type", while the other prefers "Content-Type". This results in a HTTP response that contains both headers, leading to undefined (and currently bad) results on the client side.

Probably the HttpResponse class should use a dict-like object that preserves, but ignores, case, so that "Foo" and "fOo" will map to the same item.

Attachments

case_insensitive_dict.py (3.0 kB) - added by SmileyChris on 10/31/06 15:56:44.
Here's a (2.3 compatible) case insensitive dict I whipped up
http_response_insensitive.patch (1.2 kB) - added by SmileyChris on 10/31/06 16:05:50.
Here's the patch
http_response_insensitive.2.patch (1.2 kB) - added by SmileyChris on 11/06/06 16:31:15.
more backwardsly-compatible
http_response_insensitive.3.patch (7.2 kB) - added by SmileyChris on 11/07/06 13:22:14.
Tests and included CaseInsensitiveDict?
2970.diff (1.4 kB) - added by PhiR on 09/14/07 10:37:01.
refactoring using a basic python dict with lowercase keys

Change History

10/31/06 15:56:44 changed by SmileyChris

  • attachment case_insensitive_dict.py added.

Here's a (2.3 compatible) case insensitive dict I whipped up

10/31/06 16:05:50 changed by SmileyChris

  • attachment http_response_insensitive.patch added.

Here's the patch

10/31/06 16:06:48 changed by SmileyChris

  • summary changed from HttpResponse should treat headers case-insensitively to [patch] HttpResponse should treat headers case-insensitively.

Patch doesn't contain case_insensitive_dict.py, so you'll need to put that in django.utils yourself.

11/06/06 13:55:20 changed by Bastian Kleineidam <calvin@debian.org>

The second part of your patch modifying has_header() is not backwards compatible, and could break existing applications if they used a plain dictionary for header values

If the patch is applied, this should be noted in the documentation.

11/06/06 16:31:15 changed by SmileyChris

  • attachment http_response_insensitive.2.patch added.

more backwardsly-compatible

11/06/06 16:31:52 changed by SmileyChris

Good point, Bastian. This patch should keep you happier :)

11/06/06 20:06:48 changed by jacob

Looks like a great idea. Can we get some unit tests for both the new data structure and the HttpResponse behavior? Oh, and a revised patch that moved CaseInsensitiveDict into django.utils.datastructures would be nice.

11/07/06 13:22:14 changed by SmileyChris

  • attachment http_response_insensitive.3.patch added.

Tests and included CaseInsensitiveDict?

11/07/06 13:26:57 changed by SmileyChris

Here you go, Jacob.

I haven't included any tests for the HttpResponse behaviour specifically. If you check out the code, it's just doing a basic property(get, set) to keep the identical behaviour as before. That, and I didn't know exactly what tests you'd want to run - I haven't changed any external behaviour apart from the case insensitivity which is being 100% done in the CaseInsensitiveDict.

11/07/06 13:35:30 changed by jacob

Awesome; thanks. I'll check this in later tonight.

01/17/07 20:56:54 changed by SmileyChris

  • stage changed from Unreviewed to Ready for checkin.

02/12/07 22:57:59 changed by mtredinnick

  • owner changed from adrian to mtredinnick.
  • stage changed from Ready for checkin to Design decision needed.

There's a lot of processing overhead going on in the CaseInsensitiveDict class just to preserve the case of the original key. I can't think of a reason when this is really needed (we will document that the headers are case-insensitive so that people won't be surprised when headers.keys() returns all lower-case). Am I missing some obvious use-case beyond "it might be nice"?

If there isn't a strong reason for keeping the original key case, I'd like to replace CaseInsensitiveDict with something like this Python cookbook recipe (I'll use one of the versions from later in the comments, since they fix problems with the earlier version). That code just looks a bit easier to read.

I'll move this to "design decision needed" for the time being. Chris, if I'm missing a good reason why you have that functionality, feel free to point it out. I'm willing to admit to being dense sometimes. No need to go to all the trouble of updating the patch if we can just make the switch: I'll just drop in the replacement and some tests when I do the commit.

02/13/07 00:55:16 changed by SmileyChris

You are right, preserving case it probably doesn't matter... in which case, yea there's probably a simpler way to do it. I was just blindly following Adrian's suggestion to "preserve":

Probably the HttpResponse? class should use a dict-like object that preserves, but ignores, case, so that "Foo" and "fOo" will map to the same item.

09/14/07 10:37:01 changed by PhiR

  • attachment 2970.diff added.

refactoring using a basic python dict with lowercase keys

09/14/07 10:39:51 changed by PhiR

  • keywords set to sprintsept14.
  • owner changed from nobody to PhiR.
  • component changed from Core framework to HTTP handling.

The new patch has less overhead and has_header() should be much faster than before. http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2 says that field names are case-insensitive so there's no need at all to preserve case.

09/14/07 15:34:32 changed by jacob

  • status changed from new to closed.
  • resolution set to fixed.

(In [6212]) Fixed #2970: made HttpResponse headers case-insensitive. Thanks to SmileyChris? for the original patch and PhiR for the final one.

09/18/07 10:20:39 changed by calvin@debian.org

  • status changed from closed to reopened.
  • resolution deleted.

In my experience changing header names to lower-case will break some clients or servers (let alone bad programmed proxies).

To follow the reference, only key lookup must be case insensitive. Otherwise the application should preserve the original header value. So please consider reverting the __setitem__ change and the changed "Content-Type" case.

10/20/07 00:58:48 changed by mtredinnick

  • status changed from reopened to closed.
  • resolution set to fixed.

(In [6546]) Changed the way we handle HTTP headers internally so that they appear case-insensitive, but the original case is preserved for output. This increases the chances of working with non-compliant clients without changing the external interface. Fixed #2970.


Add/Change #2970 ([patch] HttpResponse should treat headers case-insensitively)




Change Properties
Action