r13746 makes invalid assumption that QUERY_STRING exists

[raylu]

r13746 (from #9966) makes the assumption that QUERY_STRING exists. As this is a ​bad assumption, please apply the attached patch.

See also: #9435.

[raylu]

don't assume QUERY_STRING exists

[Russell Keith-Magee]

Patch needs tests.

A patch is also required against the new-style class-based views, as it makes the same bad (albeit, almost universally honored) assumption.

Technically this is a release blocker too, since it is a new feature in the previous release.

[raylu]

This one line change is tested by the same tests as the rest of r13746.

[Russell Keith-Magee]

@raylu -- Your claim is demonstrably false. If it were true, there would be a failing case in the test suite.

The case of query_string=True and query_string=False may be tested; but the non-existence of QUERY_STRING in the WSGI environment *isn't* tested, because the Django test client sets QUERY_STRING by default. We need a test that demonstrates that if QUERY_STRING *isn't* in the environment, the current code fails, but your patch makes the test pass.

[raylu]

Oh. I see what you mean.

I'm not very familiar with how to write a Django test nor have I any idea what the new class-based views are, so I'll step down.

[Aymeric Augustin]

I had to refactor the test client a bit to write tests for this issue. I modified the default environ dictionary used by django.test.client.RequestFactory to contain exactly the variables required by the WSGI spec (plus HTTP_COOKIE because we want to support coookies here). The test suite still passes :)

This change is slightly backwards incompatible. If third-party code uses Django's RequestFactory or Client for testing, and contains bugs similar to this one (ie. rely on a variable whose presence isn't guaranteed), its tests may fail after this change. I think this minor incompatibility is acceptable in the context of a bug fix. We could even argue it's a good thing to reveal unnoticed bugs.

I made two other small changes:

• CommonMiddleware used if request.GET: to test if there is a query string, and then accessed request.META['QUERY_STRING']. I replaced the check with: if request.META.get('QUERY_STRING', ''): to make it obvious that it's safe to access request.META['QUERY_STRING'] at the next line. This changes very subtly the behavior of "append slash" and "prepend www" redirects. Previously, invalid query strings (like '&') wouldn't be passed on. I can't say if it was intentional. Now they are. It's really an edge case and I think it's worth making this code more explicit. If you disagree, just don't commit this part.
• I stumbled upon an unrelated typo in the generic_views tests and fixed it.

[Preston Holmes]

There could be objections to the changes to the test client, however they seem coupled pretty strongly to testing this patch.

There are a couple small other fixes tacked in, these seem minor enough to allow as riders to this patch.

patch applies to trunk@16829 - tests pass

[raylu]

It seems like in django/middleware/common.py what you really want is

if 'QUERY_STRING' in request.META:

Everywhere else,

args = request.META.get('QUERY_STRING')

seems to make more sense since the check on the next line is merely if args

[Aymeric Augustin]

Replying to raylu:

It seems like in django/middleware/common.py what you really want is
if 'QUERY_STRING' in request.META:

The code is written that way on purpose: if request.META['QUERY_STRING'] is the empty string, I don't want to append a query string.

Thanks for the review!

[Aymeric Augustin]

In [16933]:

Fixed #16705 - Made the test client adhere to the WSGI spec -- in particular, removed the assumption that environQUERY_STRING exists.

[Julien Phalip]

In [17763]:

Fixed #17828 -- Ensured that when a list filter's queryset() method fails, it does so loudly instead of getting swallowed by a IncorrectLookupParameters exception. This also properly fixes #16705, which hadn't been addressed correctly in [16705].

[Julien Phalip]

Sorry, I meant to reference #16716, not this ticket, in the commit message above.