Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QUERY_PARAMS empty when running tests, after Upgrading to 2.3.13 #1461

Closed
hagsteel opened this issue Mar 7, 2014 · 13 comments · Fixed by #1463
Closed

QUERY_PARAMS empty when running tests, after Upgrading to 2.3.13 #1461

hagsteel opened this issue Mar 7, 2014 · 13 comments · Fixed by #1463

Comments

@hagsteel
Copy link

hagsteel commented Mar 7, 2014

After upgrading from 2.3.12 to 2.3.13 the QUERY_PARAMS are always empty.

class MyViewSet(viewsets.ReadOnlyModelViewSet):
    ...

    def get_queryset(self):
        a_param = self.request.QUERY_PARAMS.get('foo')
        if a_param == 'bar':
            ...

Has the functionality changed around this? I saw nothing in the release notes.

NOTE This only affects testing

@inglesp
Copy link
Contributor

inglesp commented Mar 7, 2014

QUERY_PARAMS shouldn't be empty. Do you have a failing test case?

@syphar
Copy link
Contributor

syphar commented Mar 7, 2014

There was a user on the ML with the same problem

@hagsteel
Copy link
Author

hagsteel commented Mar 7, 2014

I didn't want to create a pull request as I wasn't entirely sure I hadn't missed something but I wrote a test for it here:
https://github.com/jonashagstedt/django-rest-framework

here is the actual test
https://github.com/jonashagstedt/django-rest-framework/blob/master/rest_framework/tests/test_not_empty_query_params.py

@hagsteel
Copy link
Author

hagsteel commented Mar 7, 2014

Looking at the request factory in Django there is a slight discrepancy.

The test passes and query params works if I change the code to this (all tests pass as well):

class RequestFactory(DjangoRequestFactory):
    def generic(self, method, path,
            data='', content_type='application/octet-stream', **extra):
        parsed = urlparse.urlparse(path)
        data = force_bytes_or_smart_bytes(data, settings.DEFAULT_CHARSET)
        r = {
            'PATH_INFO':      self._get_path(parsed),
            'REQUEST_METHOD': str(method),
        }
        if data:
            r.update({
                'CONTENT_LENGTH': len(data),
                'CONTENT_TYPE':   str(content_type),
                'wsgi.input':     FakePayload(data),
            })
        elif django.VERSION <= (1, 4):
            # For 1.3 we need an empty WSGI payload
            r.update({
                'wsgi.input': FakePayload('')
            })
        r.update(extra)
        if not r.get('QUERY_STRING'):
            query_string = force_bytes_or_smart_bytes(parsed[4])
            # WSGI requires latin-1 encoded strings. See get_path_info().
            if six.PY3:
                query_string = query_string.decode('iso-8859-1')
            r['QUERY_STRING'] = query_string
        return self.request(**r)

@tomchristie
Copy link
Member

@jonashagstedt - Indeed - issue with this commit 1319da5

@hagsteel
Copy link
Author

hagsteel commented Mar 7, 2014

As this issue only affects the tests I've changed the title of this issue. Feel free to close it as it's probably not too important as test can be rewritten to use the data param.

use:

APIClient().get('url', data={'foo': 'bar'})

instead of:

APIClient().get('url?foo=bar')

@tomchristie
Copy link
Member

Feel free to close it

Nonononono. 'url?foo=bar' is perfectly valid usage - it's us that's done something wrong, not you.

See https://docs.djangoproject.com/en/dev/topics/testing/tools/#django.test.Client.get - "If you already have the GET arguments in URL-encoded form, you can use that encoding instead of using the data argument. "

@xordoquy
Copy link
Collaborator

xordoquy commented Mar 7, 2014

@tomchristie that's for the client, but the Django's request factory doesn't support it any longer in 1.7 which is why this has been changed.
However, I also agree it's a bad move from myself not considering it will affect more than just the DRF test suite.

@paulmelnikow
Copy link
Contributor

I'm seeing this issue in tests after upgrading to 2.3.13 as well.

@xordoquy
Copy link
Collaborator

xordoquy commented Mar 7, 2014

If one of you can confirm that this is fixed with the PR #1463 I'll merge.

@paulmelnikow
Copy link
Contributor

Yes, confirmed fixed with that commit. Thanks!

@xordoquy
Copy link
Collaborator

xordoquy commented Mar 7, 2014

Thanks. It just got merged

@egomezsumma
Copy link

As this issue only affects the tests I've changed the title of this issue. Feel free to close it as it's probably not too important as test can be rewritten to use the data param.

use:

APIClient().get('url', data={'foo': 'bar'})

instead of:

APIClient().get('url?foo=bar')

This is not a solution, im using a PUT request, and those cant have url param (indeed the param is json with list in it)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants