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.

It does not make the assumption since .get() returns None rather than throwing an exception. The None is checked on the very next line.

[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.