Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#16705 closed Bug (fixed)

r13746 makes invalid assumption that QUERY_STRING exists

Reported by: raylu Owned by: aaugustin
Component: Generic views Version: master
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)

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 4 years ago.
don't assume QUERY_STRING exists
16705.patch (5.8 KB) - added by aaugustin 4 years ago.

Download all attachments as: .zip

Change History (16)

Changed 4 years ago by raylu

don't assume QUERY_STRING exists

comment:1 Changed 4 years ago by russellm

  • Easy pickings set
  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement set
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 4 years ago by raylu

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

Last edited 4 years ago by raylu (previous) (diff)

comment:3 Changed 4 years ago by russellm

@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 Changed 4 years ago by 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.

comment:5 Changed 4 years ago by aaugustin

  • Owner changed from nobody to aaugustin

comment:6 Changed 4 years ago by aaugustin

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

Changed 4 years ago by aaugustin

comment:7 Changed 3 years ago by ptone

  • Cc preston@… added

comment:8 Changed 3 years ago by ptone

  • Needs tests unset
  • Triage Stage changed from Accepted to Ready 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 follow-up: Changed 3 years ago by raylu

It seems like in django/middleware/ 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 Changed 3 years ago by ramiro

  • Description modified (diff)

comment:11 in reply to: ↑ 9 Changed 3 years ago by aaugustin

Replying to raylu:

It seems like in django/middleware/ 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 Changed 3 years ago by aaugustin

  • Resolution set to fixed
  • Status changed from new to closed

In [16933]:

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

comment:13 Changed 3 years ago by julien

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 Changed 3 years ago by julien

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