Fixed #22799 Made GET and POST attributes of an HttpRequest object QueryDicts. #2778

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
4 participants
@duncanparkes
Contributor

duncanparkes commented Jun 8, 2014

Previously, GET and POST on an HttpRequest were created in the __init__
method as dictionaries. This was not something you would usually notice
causing trouble in production as you'd only see a WSGIRequest, but in
testing using the test client, calling .getlist on GET or POST
for a request with no get/post data used to result in an AttributeError.

Changed GET and POST on a vanilla HttpRequest object to be mutable
QueryDicts (mutable because the Django tests, and probably many
third party tests were expecting it).

Also added a regression test for this by adding calls to .getlist to
the test on a newly created HttpRequest.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Jun 9, 2014

Member

Hi, it looks like you've sent a pull request without filing a Trac ticket. Please file a ticket to suggest changes.

See also our patch review checklist.

Member

timgraham commented Jun 9, 2014

Hi, it looks like you've sent a pull request without filing a Trac ticket. Please file a ticket to suggest changes.

See also our patch review checklist.

@duncanparkes duncanparkes changed the title from Made GET and POST attributes of an HttpRequest object QueryDicts. to Fixed #22799 Made GET and POST attributes of an HttpRequest object QueryDicts. Jun 9, 2014

@duncanparkes

This comment has been minimized.

Show comment
Hide comment
@duncanparkes

duncanparkes Jun 9, 2014

Contributor

Hi Tim

I hadn't realised from reading the contribution docs that a Trac ticket was necessary. Perhaps I need to file another pull request!

I've now reported this as issue #22799

Cheers,

Duncan

Contributor

duncanparkes commented Jun 9, 2014

Hi Tim

I hadn't realised from reading the contribution docs that a Trac ticket was necessary. Perhaps I need to file another pull request!

I've now reported this as issue #22799

Cheers,

Duncan

@duncanparkes

This comment has been minimized.

Show comment
Hide comment
@duncanparkes

duncanparkes Jun 9, 2014

Contributor

All tests pass on SQLite on my laptop running CrunchBang (essentially Debian Linux).

Contributor

duncanparkes commented Jun 9, 2014

All tests pass on SQLite on my laptop running CrunchBang (essentially Debian Linux).

@bmispelon

View changes

django/http/request.py
@@ -48,7 +48,9 @@ def __init__(self):
# Any variable assignment made here should also happen in
# `WSGIRequest.__init__()`.
- self.GET, self.POST, self.COOKIES, self.META, self.FILES = {}, {}, {}, {}, {}
+ self.GET = QueryDict(str('')).copy()

This comment has been minimized.

@bmispelon

bmispelon Jun 10, 2014

Member

Why the .copy()?

By the way, it seems that the way QueryDict.__init__() is implemented, you can use QueryDict(None) to get an empty QueryDict. Come to think of it, I don't see a reason why QueryDict has any required argument. We could make query_string=None the default and it should work the same.

@bmispelon

bmispelon Jun 10, 2014

Member

Why the .copy()?

By the way, it seems that the way QueryDict.__init__() is implemented, you can use QueryDict(None) to get an empty QueryDict. Come to think of it, I don't see a reason why QueryDict has any required argument. We could make query_string=None the default and it should work the same.

This comment has been minimized.

@duncanparkes

duncanparkes Jun 10, 2014

Contributor

Hi Baptiste

Why the copy()? I read a line in the documentation saying that QueryDict's are immutable unless copied, but I guess that's a description of what happens rather than how to instantiate them. A quick grep through the code revealed some being created like that, but on another look, that's just the tests to make sure copied QueryDicts are mutable. I'll change it to use mutable=True.

I agree it would be nice to remove the required argument, but I'm a bit wary of changing signatures. Could that cause trouble for someone who's instantiating QueryDicts with _args and *_kwargs?

Cheers,

Duncan

@duncanparkes

duncanparkes Jun 10, 2014

Contributor

Hi Baptiste

Why the copy()? I read a line in the documentation saying that QueryDict's are immutable unless copied, but I guess that's a description of what happens rather than how to instantiate them. A quick grep through the code revealed some being created like that, but on another look, that's just the tests to make sure copied QueryDicts are mutable. I'll change it to use mutable=True.

I agree it would be nice to remove the required argument, but I'm a bit wary of changing signatures. Could that cause trouble for someone who's instantiating QueryDicts with _args and *_kwargs?

Cheers,

Duncan

This comment has been minimized.

@duncanparkes

duncanparkes Jun 10, 2014

Contributor

It might actually be better if .GET and .POST were immutable like the docs suggest they should be, but a handful of Django's own tests fail if we do that, and no doubt other people's would too...

Cheers,

Duncan

@duncanparkes

duncanparkes Jun 10, 2014

Contributor

It might actually be better if .GET and .POST were immutable like the docs suggest they should be, but a handful of Django's own tests fail if we do that, and no doubt other people's would too...

Cheers,

Duncan

This comment has been minimized.

@bmispelon

bmispelon Jun 10, 2014

Member

I hadn't tried to run the test but you're right. Any failing test means we'd be breaking backwards compatibility which is a big no-no.

I hadn't realized that the .copy() gave you a mutable object. That's a pretty quirky API.

Regarding making the query_string optional, I'm trying to think of a situation where we'd be breaking somebody's code but I can't think of any. Do you have a practical example?

@bmispelon

bmispelon Jun 10, 2014

Member

I hadn't tried to run the test but you're right. Any failing test means we'd be breaking backwards compatibility which is a big no-no.

I hadn't realized that the .copy() gave you a mutable object. That's a pretty quirky API.

Regarding making the query_string optional, I'm trying to think of a situation where we'd be breaking somebody's code but I can't think of any. Do you have a practical example?

This comment has been minimized.

@duncanparkes

duncanparkes Jun 10, 2014

Contributor

For reference, the bit of the docs referring to .copy() is here:

https://docs.djangoproject.com/en/dev/ref/request-response/#django.http.QueryDict

It would be nice to make the code reflect the docs and have .GET and .POST immutable on HttpRequest as they are on WSGIRequest, but as per anything backwards incompatible, the relative hassle for people has to be weighed up. Changing it would make some tests fail, leaving it encourages tests which don't reflect reality, but only in odd cases (this is how I came to be looking at this in the first place). It's obviously not the kind of backwards incompatible change that needs doing in a hurry.

I'll have a think about an example for making query_string optional causing trouble.

@duncanparkes

duncanparkes Jun 10, 2014

Contributor

For reference, the bit of the docs referring to .copy() is here:

https://docs.djangoproject.com/en/dev/ref/request-response/#django.http.QueryDict

It would be nice to make the code reflect the docs and have .GET and .POST immutable on HttpRequest as they are on WSGIRequest, but as per anything backwards incompatible, the relative hassle for people has to be weighed up. Changing it would make some tests fail, leaving it encourages tests which don't reflect reality, but only in odd cases (this is how I came to be looking at this in the first place). It's obviously not the kind of backwards incompatible change that needs doing in a hurry.

I'll have a think about an example for making query_string optional causing trouble.

This comment has been minimized.

@duncanparkes

duncanparkes Jun 10, 2014

Contributor

I can't think of even an impractical example of making query_string optional causing a problem, so I've just implemented your suggestion instead.

Cheers,

Duncan

@duncanparkes

duncanparkes Jun 10, 2014

Contributor

I can't think of even an impractical example of making query_string optional causing a problem, so I've just implemented your suggestion instead.

Cheers,

Duncan

@duncanparkes

This comment has been minimized.

Show comment
Hide comment
@duncanparkes

duncanparkes Jun 10, 2014

Contributor

Need for release notes patch noted - I'll sort that this evening.

Cheers,

Duncan

Contributor

duncanparkes commented Jun 10, 2014

Need for release notes patch noted - I'll sort that this evening.

Cheers,

Duncan

@duncanparkes

This comment has been minimized.

Show comment
Hide comment
@duncanparkes

duncanparkes Jun 10, 2014

Contributor

I've rebased in patches to the release docs, so I think everything is now in place. Let me know if not, and thanks for all your help.

Cheers,

Duncan

Contributor

duncanparkes commented Jun 10, 2014

I've rebased in patches to the release docs, so I think everything is now in place. Let me know if not, and thanks for all your help.

Cheers,

Duncan

@charettes

View changes

tests/httpwrappers/tests.py
self.assertRaises(KeyError, q.__getitem__, "foo")
q['name'] = 'john'
self.assertEqual(q['name'], 'john')
def test_mutable_delete(self):
- q = QueryDict(str('')).copy()
+ q = QueryDict().copy()

This comment has been minimized.

@charettes

charettes Jun 10, 2014

Member

mutable=True instead of copy()?

@charettes

charettes Jun 10, 2014

Member

mutable=True instead of copy()?

This comment has been minimized.

@duncanparkes

duncanparkes Jun 10, 2014

Contributor

Sure - that's definitely clearer.

@duncanparkes

duncanparkes Jun 10, 2014

Contributor

Sure - that's definitely clearer.

@charettes

View changes

tests/httpwrappers/tests.py
q['name'] = 'john'
del q['name']
self.assertFalse('name' in q)
def test_basic_mutable_operations(self):
- q = QueryDict(str('')).copy()
+ q = QueryDict().copy()

This comment has been minimized.

@charettes

charettes Jun 10, 2014

Member

Idem.

This comment has been minimized.

@duncanparkes

duncanparkes Jun 10, 2014

Contributor

ditto.

@duncanparkes

duncanparkes Jun 10, 2014

Contributor

ditto.

@bmispelon

This comment has been minimized.

Show comment
Hide comment
@bmispelon

bmispelon Jun 11, 2014

Member

I was looking at the reference doc for QueryDict and I noticed two things which I think we could fix as part of this patch too:

  • QueryDict.__init__ is not documented
  • The sentence "QueryDict instances are immutable, unless you create a copy() of them. That means you can’t change attributes of request.POST and request.GET directly." doesn't seem to be too acurate (since you can pass mutable=True to the constructor).

Would you mind adding that?

Other than that, the patch looks good to me.

Thanks.

Member

bmispelon commented Jun 11, 2014

I was looking at the reference doc for QueryDict and I noticed two things which I think we could fix as part of this patch too:

  • QueryDict.__init__ is not documented
  • The sentence "QueryDict instances are immutable, unless you create a copy() of them. That means you can’t change attributes of request.POST and request.GET directly." doesn't seem to be too acurate (since you can pass mutable=True to the constructor).

Would you mind adding that?

Other than that, the patch looks good to me.

Thanks.

@duncanparkes

This comment has been minimized.

Show comment
Hide comment
@duncanparkes

duncanparkes Jun 11, 2014

Contributor

I'll have a look at those bits of the docs this evening. The QueryDict instances sentence is not totally silly if read in the context of the GET and POST attributes of a WSGIResponse, but yes, it does need a bit of work!

Duncan

Contributor

duncanparkes commented Jun 11, 2014

I'll have a look at those bits of the docs this evening. The QueryDict instances sentence is not totally silly if read in the context of the GET and POST attributes of a WSGIResponse, but yes, it does need a bit of work!

Duncan

@duncanparkes

This comment has been minimized.

Show comment
Hide comment
@duncanparkes

duncanparkes Jun 11, 2014

Contributor

OK, so I've added some documentation for QueryDict.__init__, and tidied up the mentions of mutable a bit. I've also had a little fiddle with the docstring for QueryDict, though I don't have time right now to go through adding docstrings to all the methods in the class.

I also realised that I should have given .FILES a similar treatment, so I've added that in. I've left it as a separate commit for now so you can see what I've done, but 4b307f7 should obviously be rebased into 29b69ff. I changed the tests a tiny bit to distinguish between the ones checking for QueryDict like behaviour and the one checking for MultiValueDict like behaviour.

While thinking about documentation of __init__ for QueryDict I noticed a small optimization to do with encoding in there, so I've added that too.

Cheers,

Duncan

Contributor

duncanparkes commented Jun 11, 2014

OK, so I've added some documentation for QueryDict.__init__, and tidied up the mentions of mutable a bit. I've also had a little fiddle with the docstring for QueryDict, though I don't have time right now to go through adding docstrings to all the methods in the class.

I also realised that I should have given .FILES a similar treatment, so I've added that in. I've left it as a separate commit for now so you can see what I've done, but 4b307f7 should obviously be rebased into 29b69ff. I changed the tests a tiny bit to distinguish between the ones checking for QueryDict like behaviour and the one checking for MultiValueDict like behaviour.

While thinking about documentation of __init__ for QueryDict I noticed a small optimization to do with encoding in there, so I've added that too.

Cheers,

Duncan

@bmispelon

View changes

docs/ref/request-response.txt
-class customized to deal with multiple values for the same key. This is
-necessary because some HTML form elements, notably
-``<select multiple="multiple">``, pass multiple values for the same key.
+of ``django.http.QueryDict``, a dictionary-like

This comment has been minimized.

@bmispelon

bmispelon Jun 15, 2014

Member

Keeping the :class: directive would automatically link to the correct place in the documentation.

@bmispelon

bmispelon Jun 15, 2014

Member

Keeping the :class: directive would automatically link to the correct place in the documentation.

This comment has been minimized.

@duncanparkes

duncanparkes Jun 15, 2014

Contributor

It does, but given that place in only two lines above, I thought the link rather redundant! I'mhappy to put it back in if you really want it, of course!

@duncanparkes

duncanparkes Jun 15, 2014

Contributor

It does, but given that place in only two lines above, I thought the link rather redundant! I'mhappy to put it back in if you really want it, of course!

This comment has been minimized.

@bmispelon

bmispelon Jun 16, 2014

Member

You're completely right, my mistake.

@bmispelon

bmispelon Jun 16, 2014

Member

You're completely right, my mistake.

This comment has been minimized.

@timgraham

timgraham Jun 16, 2014

Member

Some of the line breaks are a little funny (like here). I usually tried to break as close to 80 characters as possible.

@timgraham

timgraham Jun 16, 2014

Member

Some of the line breaks are a little funny (like here). I usually tried to break as close to 80 characters as possible.

@bmispelon

This comment has been minimized.

Show comment
Hide comment
@bmispelon

bmispelon Jun 15, 2014

Member

While 5545ba1348e2fc3d6327cc80fb430c67ba9c5bfe looks like a good idea, it's not strictly equivalent to the previous code so I'm not confident in changing it. Do you mind reverting it?

Member

bmispelon commented Jun 15, 2014

While 5545ba1348e2fc3d6327cc80fb430c67ba9c5bfe looks like a good idea, it's not strictly equivalent to the previous code so I'm not confident in changing it. Do you mind reverting it?

@bmispelon

View changes

docs/ref/request-response.txt
@@ -437,7 +454,8 @@ In addition, ``QueryDict`` has the following methods:
.. method:: QueryDict.copy()
Returns a copy of the object, using ``copy.deepcopy()`` from the Python
- standard library. The copy will be mutable -- that is, you can change its
+ standard library. The copy will be mutable irrespective of mutability

This comment has been minimized.

@bmispelon

bmispelon Jun 15, 2014

Member

The sentence is a bit awkward to read. How about: "This copy will always have mutable=True set." ?

@bmispelon

bmispelon Jun 15, 2014

Member

The sentence is a bit awkward to read. How about: "This copy will always have mutable=True set." ?

@duncanparkes

This comment has been minimized.

Show comment
Hide comment
@duncanparkes

duncanparkes Jun 16, 2014

Contributor

OK - I'll drop the cleanup commit for now, and then I think we're good to go, are we?

Cheers,

Duncan

Contributor

duncanparkes commented Jun 16, 2014

OK - I'll drop the cleanup commit for now, and then I think we're good to go, are we?

Cheers,

Duncan

@bmispelon

This comment has been minimized.

Show comment
Hide comment
@bmispelon

bmispelon Jun 16, 2014

Member

Yes, I believe so. If all goes well, I'll merge this in a few hours.

Thanks!

Member

bmispelon commented Jun 16, 2014

Yes, I believe so. If all goes well, I'll merge this in a few hours.

Thanks!

@duncanparkes

This comment has been minimized.

Show comment
Hide comment
@duncanparkes

duncanparkes Jun 16, 2014

Contributor

Great. I've just merged the two commits I said I'd merge (just about managed to get the message down to 72 chars).

Let me know if there's anything else you'd like.

Cheers,

Duncan

Contributor

duncanparkes commented Jun 16, 2014

Great. I've just merged the two commits I said I'd merge (just about managed to get the message down to 72 chars).

Let me know if there's anything else you'd like.

Cheers,

Duncan

@timgraham

View changes

docs/ref/request-response.txt
-``QueryDict`` instances are immutable, unless you create a ``copy()`` of them.
-That means you can't change attributes of ``request.POST`` and ``request.GET``
-directly.
+The ``QueryDict``\ s at ``request.POST`` and ``request.GET`` will usually be

This comment has been minimized.

@timgraham

timgraham Jun 16, 2014

Member

clarify "usually"?

@timgraham

timgraham Jun 16, 2014

Member

clarify "usually"?

This comment has been minimized.

@duncanparkes

duncanparkes Jun 16, 2014

Contributor

Gah - I was deliberately trying to avoid that. I think the answer is that they should always be, but as you can see from discussions above, we've decided not to change that as it would be backwards incompatible. I'm not really sure we want to be noting that they could turn up mutable if you're not using WSGIRequest in the documentation though, as that would really be noting a minor bug in the docs.

What do you suggest?

Cheers,

Duncan

@duncanparkes

duncanparkes Jun 16, 2014

Contributor

Gah - I was deliberately trying to avoid that. I think the answer is that they should always be, but as you can see from discussions above, we've decided not to change that as it would be backwards incompatible. I'm not really sure we want to be noting that they could turn up mutable if you're not using WSGIRequest in the documentation though, as that would really be noting a minor bug in the docs.

What do you suggest?

Cheers,

Duncan

This comment has been minimized.

@timgraham

timgraham Jun 16, 2014

Member

something like "request.POST and request.GET will be immutable when accessed in a normal request/response cycle"?

@timgraham

timgraham Jun 16, 2014

Member

something like "request.POST and request.GET will be immutable when accessed in a normal request/response cycle"?

This comment has been minimized.

@duncanparkes

duncanparkes Jun 16, 2014

Contributor

Yes, that's better - thanks.

@duncanparkes

duncanparkes Jun 16, 2014

Contributor

Yes, that's better - thanks.

@timgraham

View changes

docs/ref/request-response.txt
a subclass of dictionary. Exceptions are outlined here:
+.. method:: QueryDict.__init__(query_string=None, mutable=False, encoding=None)
+
+ Instantiates a QueryDict object based on ``query_string``.

This comment has been minimized.

@timgraham

timgraham Jun 16, 2014

Member

`` around QueryDict

@timgraham

timgraham Jun 16, 2014

Member

`` around QueryDict

This comment has been minimized.

@duncanparkes

duncanparkes Jun 16, 2014

Contributor

I've added that - thanks.

@duncanparkes

duncanparkes Jun 16, 2014

Contributor

I've added that - thanks.

+ ``mutable=True`` to its ``__init__()``.
+
+ Strings for setting both keys and values will be converted from ``encoding``
+ to unicode. If encoding is not set, it defaults to :setting:`DEFAULT_CHARSET`.

This comment has been minimized.

@timgraham

timgraham Jun 16, 2014

Member

Should add a .. versionchanged:: 1.8 note for the query_string=None change.

@timgraham

timgraham Jun 16, 2014

Member

Should add a .. versionchanged:: 1.8 note for the query_string=None change.

This comment has been minimized.

@duncanparkes

duncanparkes Jun 16, 2014

Contributor

Good point - I've added a note.

@duncanparkes

duncanparkes Jun 16, 2014

Contributor

Good point - I've added a note.

@duncanparkes

This comment has been minimized.

Show comment
Hide comment
@duncanparkes

duncanparkes Jun 16, 2014

Contributor

@timgraham I can't find your note about line lengths, but I've rejigged things so that the breaks are just before 80 characters where possible.

Contributor

duncanparkes commented Jun 16, 2014

@timgraham I can't find your note about line lengths, but I've rejigged things so that the breaks are just before 80 characters where possible.

@timgraham

View changes

docs/releases/1.8.txt
@@ -194,6 +194,17 @@ Requests and Responses
<django.http.HttpRequest.build_absolute_uri>` method now handles paths
starting with ``//`` correctly.
+* The ``query_string`` argument of :class:`~django.http.QueryDict`
+ is now optional, defaulting

This comment has been minimized.

@timgraham

timgraham Jun 16, 2014

Member

line break

@timgraham

timgraham Jun 16, 2014

Member

line break

@timgraham

View changes

docs/releases/1.8.txt
+
+* The ``GET`` and ``POST`` attributes of an :class:`~django.http.HttpRequest`
+ object are now :class:`~django.http.QueryDict`\ s rather than dictionaries,
+ and the ``FILES`` attribute is now a ``MultiValueDict``.

This comment has been minimized.

@timgraham

timgraham Jun 16, 2014

Member

line break

@timgraham

timgraham Jun 16, 2014

Member

line break

This comment has been minimized.

@duncanparkes

duncanparkes Jun 16, 2014

Contributor

Sorry - missed the ones in the release notes. Now done.

Thanks!

Duncan

@duncanparkes

duncanparkes Jun 16, 2014

Contributor

Sorry - missed the ones in the release notes. Now done.

Thanks!

Duncan

Duncan Parkes added some commits Jun 11, 2014

Duncan Parkes
Improved documentation for QueryDict.
Added __init__ method to the documentation.
Reworded some parts around mutability of QueryDicts.

Also improved the docstring of the QueryDict class a bit.
Duncan Parkes
Made GET and POST on HttpRequest QueryDicts, and FILES a MultiValueDict.
Previously, GET, POST and FILES on an HttpRequest were created in
the __init__ method as dictionaries. This was not something you would
usually notice causing trouble in production as you'd only see a
WSGIRequest, but in testing using the test client, calling .getlist
on GET, POST, or FILES for a request with no get/post data used to
result in an AttributeError.

Changed GET and POST on a vanilla HttpRequest object to be mutable
QueryDicts (mutable because the Django tests, and probably many
third party tests, were expecting it). Changed FILES to be a
MultiValueDict.

Also added a regression test for this by adding calls to .urlencode on
GET and POST, and to .getlist on FILES, to the test on a newly created
HttpRequest.
Duncan Parkes
Made query_string argument to QueryDict optional defaulting to None.
Previously, a blank QueryDict could be created by calling

QueryDict(str('')) or QueryDict(None)

It seems rather nicer to just allow the query_string argument to
default to None so that we can just write QueryDict().
@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Jun 25, 2014

Member

merged in 7f4e2ef, d68987a, and fd4ccd0.

Member

timgraham commented Jun 25, 2014

merged in 7f4e2ef, d68987a, and fd4ccd0.

@timgraham timgraham closed this Jun 25, 2014

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