New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #20147 -- Added HttpRequest.headers. #10171

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@santiagobasulto

santiagobasulto commented Jul 10, 2018

Original PR (with discussion included): #9925

I've rebased from master and squashed all the commits into one.

@santiagobasulto

This comment has been minimized.

santiagobasulto commented Jul 10, 2018

👋 folks, there's only one build failing with: ModuleNotFoundError: No module named 'memcache'
I haven't changed any dependency or setup.py file, do you have an idea of what might be going on?

Thanks!

@@ -211,6 +211,17 @@ Requests and Responses
sets the ``Content-Disposition`` header to make the browser ask if the user
wants to download the file. ``FileResponse`` also tries to set the
``Content-Type`` and ``Content-Length`` headers where appropriate.
* Added :attr:`.HttpRequest.headers`.

This comment has been minimized.

@auvipy

auvipy Jul 11, 2018

Contributor

shouldn't this moved to 2.2.txt?

This comment has been minimized.

@auvipy

auvipy Jul 11, 2018

Contributor

you can drop this

This comment has been minimized.

@santiagobasulto

santiagobasulto Jul 13, 2018

🤦🏻‍♂️ I did it again 😔. thanks, i'll fix really quickly

@santiagobasulto santiagobasulto force-pushed the santiagobasulto:ticket_20147 branch from 8ce2c2e to 43ba8e0 Jul 13, 2018

@santiagobasulto

This comment has been minimized.

santiagobasulto commented Jul 13, 2018

There seems to be an error with jenkins not finding the right commit after the rebase. Anybody has an idea how I could fix this?

@auvipy

This comment has been minimized.

Contributor

auvipy commented Jul 13, 2018

everything seems passing

@santiagobasulto

This comment has been minimized.

santiagobasulto commented Jul 13, 2018

You're right, when I submitted the PR originally, it was failing, it's working now 👍

@santiagobasulto

This comment has been minimized.

santiagobasulto commented Jul 13, 2018

@carltongibson @timgraham PR finally rebased and working. Thanks for all your support. Gonna say this once again, this community is amazing.

yield tuple(elem)
class ImmutableCaseInsensitiveDict(Mapping):

This comment has been minimized.

@jschneier

jschneier Jul 16, 2018

Contributor

This isn't actually Immutable. I didn't trace far back enough to see if the WSGI environ is immutable but cursory inspection would indicate that .META is not actually immutable although it would probably be a bad idea to manipulate it (I might be wrong on this point).

return self
class EnvironHeaders(ImmutableCaseInsensitiveDict):

This comment has been minimized.

@adamchainz

adamchainz Jul 25, 2018

Member

this class has a bunch of details of WSGI environ so I think it more accurately belongs in django.http.request

@santiagobasulto

This comment has been minimized.

santiagobasulto commented Sep 17, 2018

Hello @carltongibson @timgraham 👋 ! You think I could merge this one or should I make any other improvement?

@carltongibson

This comment has been minimized.

Member

carltongibson commented Sep 17, 2018

Hi @santiagobasulto. As far as I can see the comments from @jschneier and @adamchainz remain unaddressed.

Just dropping Immutable from the naming may be enough to address @jschneier's point (???)

Overall it seems quite good to me. If you can address the points and then uncheck Patch needs improvement on the ticket, I'll have another look. (Seems like a nice feature for the amount of code.)

@santiagobasulto

This comment has been minimized.

santiagobasulto commented Sep 19, 2018

Great, thanks @carltongibson. I'll get on it and update soon.

@@ -135,7 +135,9 @@ All attributes should be considered read-only, unless stated otherwise.
.. attribute:: HttpRequest.META
A dictionary containing all available HTTP headers. Available headers
A dictionary containing the `WSGI environ \

This comment has been minimized.

@timgraham

timgraham Sep 19, 2018

Member

This change looks unrelated to the patch and should be submitted separately.

Note that :djadmin:`runserver` strips all headers with underscores in the
name, so you won't see them in ``META``. This prevents header-spoofing
based on ambiguity between underscores and dashes both being normalizing to
underscores in WSGI environment variables. It matches the behavior of
Web servers like Nginx and Apache 2.4+.
.. _headers:

This comment has been minimized.

@timgraham

timgraham Sep 19, 2018

Member

This line isn't required.

When accessing elements with ``headers.get``, ``headers[]`` or
checking membership (``in``), case is ignored. Examples::
'User-Agent' in request.headers # True

This comment has been minimized.

@timgraham

timgraham Sep 19, 2018

Member

This block is indent 4 spaces too much.

@@ -806,3 +806,121 @@ def test_request_path_begins_with_two_slashes(self):
for location, expected_url in tests:
with self.subTest(location=location):
self.assertEqual(request.build_absolute_uri(location=location), expected_url)
class CaseInsensitiveHeadersTestCase(SimpleTestCase):

This comment has been minimized.

@timgraham

timgraham Sep 19, 2018

Member

TestCase -> Tests

@@ -135,7 +135,9 @@ All attributes should be considered read-only, unless stated otherwise.
.. attribute:: HttpRequest.META
A dictionary containing all available HTTP headers. Available headers
A dictionary containing the `WSGI environ \
<https://www.python.org/dev/peps/pep-0333/#environ-variables>`_, including all available HTTP headers.

This comment has been minimized.

@timgraham

timgraham Sep 19, 2018

Member

Documentation should be wrapped at 79 characters.

def test_list(self):
self.assertEqual(
sorted(list(self.dict_1)),
sorted(['Accept', 'content-type']))

This comment has been minimized.

@timgraham

timgraham Sep 19, 2018

Member

trailing ) goes on the next line (check your entire patch for this style)

}))
def test_equals(self):
self.assertTrue(self.dict_1 == {

This comment has been minimized.

@timgraham

timgraham Sep 19, 2018

Member

Why not assertEqual?

self.assertEqual(self.dict_1['Content-type'], 'text/html')
def test_membership(self):
self.assertTrue('Accept' in self.dict_1)

This comment has been minimized.

@timgraham

timgraham Sep 19, 2018

Member

assertIn

'HTTP_ACCEPT': '*',
'HTTP_HOST': 'example.com',
'HTTP_USER_AGENT': 'python-requests/1.2.0',

This comment has been minimized.

@timgraham

timgraham Sep 19, 2018

Member

blank line not needed

# .get():
request.headers.get('User-Agent') # Returns the value of User-Agent
request.headers.get('user-agent') # Returns the value of User-Agent

This comment has been minimized.

@timgraham

timgraham Sep 19, 2018

Member

one blank line please

@carltongibson

This comment has been minimized.

Member

carltongibson commented Nov 20, 2018

Hi @santiagobasulto.

This still looks good. It would be a super addition for v2.2. Are you up for pushing it over the finish line? (If not we'll see if we can pick it up...)

Re @jschneier's query on immutability, from PEP 3333 (the WSGI spec):

The environ parameter is a dictionary object, containing CGI-style environment variables. This object must be a builtin Python dictionary (not a subclass, UserDict or other dictionary emulation), and the application is allowed to modify the dictionary in any way it desires.

So it's not immutable, by design.

Your ImmutableCaseInsensitiveDict implements Mapping (which is read-only, vs MutableMapping) so maybe just CaseInsensitiveMapping, not worrying about actual immutability.
Request.headers is then (via your EnvironHeaders) a case-insensitive, read-only accessor. (Just what we want.)

(Anybody wanting to actually mutate Request.META — Why? 🙂 — can access it directly.)

Moving EnvironHeaders as per @adamchainz's review and making the adjustments Tim suggested shouldn't leave it far off.

@santiagobasulto

This comment has been minimized.

santiagobasulto commented Nov 20, 2018

Hey @carltongibson! Yes, I'll get to this immediately and push the fixes by tomorrow. Sorry, I've been quite busy lately but I don't want to keep postponing this.

self.assertEqual(dict(request.headers), {
'Content-Type': 'text/html',
'Content-Length': '100',
'Accept': '*', 'Host': 'example.com',

This comment has been minimized.

@zachborboa

zachborboa Dec 6, 2018

Contributor
Suggested change Beta
'Accept': '*', 'Host': 'example.com',
'Accept': '*',
'Host': 'example.com',

This comment has been minimized.

@santiagobasulto
self.assertEqual(dict(request.headers), {
'Content-Type': 'text/html',
'Content-Length': '100',
'Accept': '*', 'Host': 'example.com',

This comment has been minimized.

@zachborboa

zachborboa Dec 6, 2018

Contributor
Suggested change Beta
'Accept': '*', 'Host': 'example.com',
'Accept': '*',
'Host': 'example.com',

@santiagobasulto santiagobasulto force-pushed the santiagobasulto:ticket_20147 branch from 5c8152e to 47729a7 Dec 9, 2018

@santiagobasulto

This comment has been minimized.

santiagobasulto commented Dec 9, 2018

Hey folks! Thanks @carltongibson for the support so far. Quick summary: master rebased, comments by @timgraham and @adamchainz addressed. I've just rebased and pushed but seems like something is broken (there have been many changes since my first commit). I'll fix and this should be done by today.

@timgraham timgraham changed the title from Fixes #20147 -- Added Request.headers to Fixed #20147 -- Added HttpRequest.headers. Dec 9, 2018

Fixes #20147 -- Added Request.headers
Provides an alternative to request.META to access HTTP headers,
implemented using an immutable case-insensitive dictionary.
Targeting Django release 2.2.

@santiagobasulto santiagobasulto force-pushed the santiagobasulto:ticket_20147 branch from 47729a7 to ac67813 Dec 9, 2018

@carltongibson

This comment has been minimized.

Member

carltongibson commented Dec 9, 2018

Hi Santiago. Thanks for the work! I will review on Tuesday, so no panic. 🙂

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