Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#16705 closed Bug (fixed)

r13746 makes invalid assumption that QUERY_STRING exists

Reported by: raylu Owned by: Aymeric Augustin
Component: Generic views Version: dev
Severity: Release blocker Keywords:
Cc: preston@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Ramiro Morales)

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

See also: #9435.

Attachments (2)

13746_query_string.patch (459 bytes ) - added by raylu 13 years ago.
don't assume QUERY_STRING exists
16705.patch (5.8 KB ) - added by Aymeric Augustin 13 years ago.

Download all attachments as: .zip

Change History (16)

by raylu, 13 years ago

Attachment: 13746_query_string.patch added

don't assume QUERY_STRING exists

comment:1 by Russell Keith-Magee, 13 years ago

Easy pickings: set
Needs tests: set
Patch needs improvement: set
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

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.

comment:2 by raylu, 13 years ago

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.

Version 0, edited 13 years ago by raylu (next)

comment:3 by Russell Keith-Magee, 13 years ago

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

comment:4 by raylu, 13 years ago

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.

comment:5 by Aymeric Augustin, 13 years ago

Owner: changed from nobody to Aymeric Augustin

comment:6 by Aymeric Augustin, 13 years ago

Easy pickings: unset
Patch needs improvement: unset

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.

by Aymeric Augustin, 13 years ago

Attachment: 16705.patch added

comment:7 by Preston Holmes, 13 years ago

Cc: preston@… added

comment:8 by Preston Holmes, 13 years ago

Needs tests: unset
Triage Stage: AcceptedReady for checkin

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

comment:9 by raylu, 13 years ago

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

comment:10 by Ramiro Morales, 13 years ago

Description: modified (diff)

in reply to:  9 comment:11 by Aymeric Augustin, 13 years ago

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!

comment:12 by Aymeric Augustin, 13 years ago

Resolution: fixed
Status: newclosed

In [16933]:

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

comment:13 by Julien Phalip, 13 years ago

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

comment:14 by Julien Phalip, 13 years ago

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

Note: See TracTickets for help on using tickets.
Back to Top