Skip to content

Response format change from 3.2.x to 3.3.x #3644

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

Closed
troygrosfield opened this issue Nov 17, 2015 · 19 comments
Closed

Response format change from 3.2.x to 3.3.x #3644

troygrosfield opened this issue Nov 17, 2015 · 19 comments
Labels
Milestone

Comments

@troygrosfield
Copy link

After upgrading DRF from 3.2.5 to 3.3.1 I noticed the api response format changed and no longer represents the format that's shown on the main page.

Api response with DRF 3.2.5 (expected response)

{
    "count": 1,
    "next": null,
    "previous": null,
    "results": [
        // my object results
    ]
}

Api response with DRF 3.3.1 (only returns list/array)

[
    // my object results
]

There is only 1 object returned in the result set so no additional pages exist. However, this change breaks every api consuming caller since the response format has now changed.

@xordoquy
Copy link
Collaborator

You probably missed previous deprecation affecting the pagination.
Have a look at http://www.django-rest-framework.org/topics/3.3-announcement/#deprecations

If your use case doesn't match the above cases feel free to reopen and if possible add a test case so we can figure the issue.

@tomchristie tomchristie reopened this Nov 17, 2015
@tomchristie
Copy link
Member

Reopening to dig a bit deeper. @xordoquy prob correct re. related to deprecation there, but shouldn't be in a position where we've got an altered API response, rather than an error condition (eg hitting an assertion 'this no longer works')

Probably worth a bit more info from @troygrosfield on most minimal way possible to replicate, so we can see if we need to be catching some particular case, and eg raising error XYZ style is no longer supported.

@troygrosfield
Copy link
Author

I'll work on the testcase here in a bit, but you can plug any model into a ViewSet and have it do the same thing (alter the response format as mentioned above).

@xordoquy, the deprecation link you provided didn't mention anything about altering response formats in any case which to me would warrant a major version change.

I'll work on the test case but here was the code I was working with:

from rest_framework.filters import DjangoFilterBackend
from rest_framework.filters import OrderingFilter
from rest_framework.viewsets import ReadOnlyModelViewSet

from my_models.models import MyModel

from .serializers import MyModelSerializer


class MyModelViewSet(ReadOnlyModelViewSet):
    queryset = MyModel.objects.all()
    serializer_class = MyModelSerializer
    permission_classes = []
    filter_backends = (OrderingFilter, DjangoFilterBackend)
    filter_fields = ('some_field',)
    ordering = ('-id',)
    page_query_param = 'p'

@tomchristie
Copy link
Member

@troygrosfield What are your pagination settings for the example given?

@xordoquy
Copy link
Collaborator

Reopening to dig a bit deeper. @xordoquy prob correct re. related to deprecation there, but shouldn't be in a position where we've got an altered API response, rather than an error condition (eg hitting an assertion 'this no longer works')

Makes sense to make it fail loud.
Waiting for the full use case we can reproduce and will retitle the issue by then.

@troygrosfield
Copy link
Author

The current settings are below.

# djangorestframework settings
REST_FRAMEWORK = {
    # Use hyperlinked styles by default.
    # Only used if the `serializer_class` attribute is not set on a view.
    'DEFAULT_MODEL_SERIALIZER_CLASS': 'rest_framework.serializers.HyperlinkedModelSerializer',
    'DEFAULT_FILTER_BACKENDS': ('rest_framework.filters.DjangoFilterBackend',),
    'DEFAULT_RENDERER_CLASSES': (
        'rest_framework.renderers.JSONRenderer',
    ),
    'DEFAULT_AUTHENTICATION_CLASSES': (
        'api.authentication.CustomSessionAuthentication',
        'rest_framework.authentication.BasicAuthentication',
    ),
    # Use Django's standard `django.contrib.auth` permissions,
    # or allow read-only access for unauthenticated users.
    'DEFAULT_PERMISSION_CLASSES': (
        'rest_framework.permissions.IsAdminUser',
    ),
    'PAGINATE_BY': 25,
    'PAGINATE_BY_PARAM': 'ps',
    'SEARCH_PARAM': 'q',
    'MAX_PAGINATE_BY': 100
}

I understand some of those pagination settings have change from the deprecation post you guys outlined which I'm yet to update. However, I also never received any deprecation messages in the console when running the application which I guess I would've expected similar to what django does. Example:

/usr/local/Cellar/python3/3.4.3/Frameworks/Python.framework/Versions/3.4/lib/python3.4/importlib/_bootstrap.py:321: RemovedInDjango19Warning: django.utils.importlib will be removed in Django 1.9.
  return f(*args, **kwds)

@troygrosfield
Copy link
Author

That was the issue was changing the global settings. However, that seems to be a major change as that will most certainly break pagination for all all existing code which will require all DRF consumers to make a change. A 4.0 release version may have been more appropriate.

New global settings (for people that come across this in the future)

# djangorestframework settings
REST_FRAMEWORK = {
    # Use hyperlinked styles by default.
    # Only used if the `serializer_class` attribute is not set on a view.
    'DEFAULT_MODEL_SERIALIZER_CLASS': 'rest_framework.serializers.HyperlinkedModelSerializer',
    'DEFAULT_PAGINATION_CLASS': 'rest_framework.pagination.PageNumberPagination',
    'DEFAULT_FILTER_BACKENDS': ('rest_framework.filters.DjangoFilterBackend',),
    'DEFAULT_RENDERER_CLASSES': (
        'rest_framework.renderers.JSONRenderer',
    ),
    'DEFAULT_AUTHENTICATION_CLASSES': (
        'api.authentication.CustomSessionAuthentication',
        'rest_framework.authentication.BasicAuthentication',
    ),
    # Use Django's standard `django.contrib.auth` permissions,
    # or allow read-only access for unauthenticated users.
    'DEFAULT_PERMISSION_CLASSES': (
        'rest_framework.permissions.IsAdminUser',
    ),
    'PAGE_SIZE': 25,
    'PAGE_SIZE_QUERY_PARAM': 'ps',
    'SEARCH_PARAM': 'q',
    'MAX_PAGE_SIZE': 100
}

Thanks for the quick response guys!

@tomchristie
Copy link
Member

Reopening, as should probably still be taking action here, eg.

  • Loud error when PAGINATE_BY set.
  • Loud error when PAGINATE_BY_PARAM set.
  • Loud error when MAX_PAGINATE_BY_SET set.

@troygrosfield
Copy link
Author

👍 agreed.

@tomchristie
Copy link
Member

Furthermore, not convinced on first sight that PAGE_SIZE setting is properly documented (eg no mention in deprecation notes)

@troygrosfield
Copy link
Author

Actually, paging page and page size aren't being honored now. Using the following url (with the settings listed above):

I get the following response:

{
    "count": 25,
    "next": null,
    "previous": null,
    "results": [
        // 25 objects
    ]
}

If I override the page size in the url, shouldn't that be honored?

Also, PAGE_SIZE_QUERY_PARAM isn't documented in the settings page:

Or am I required to override the DEFAULT_PAGINATION_CLASS and there is not more global setting for this?

@troygrosfield
Copy link
Author

Ignore that last comment. I was looking at count as the number of results returned and not as the total count. So that's correct.

@lsanpablo
Copy link
Contributor

Is it okay if i work on this issue?

@xordoquy
Copy link
Collaborator

xordoquy commented Dec 7, 2015

@Cheglader I don't think anyone's working on this at the moment.

@rosscdh
Copy link

rosscdh commented Dec 11, 2015

+1 I cant even get it to return a dict.

djangorestframework==3.3.1

settings

REST_FRAMEWORK = {
    'DEFAULT_AUTHENTICATION_CLASSES': (
        'rest_framework.authentication.SessionAuthentication',
    ),
    'DEFAULT_PERMISSION_CLASSES': (
        'rest_framework.permissions.AllowAny',
    ),
    'DEFAULT_MODEL_SERIALIZER_CLASS':
        'rest_framework.serializers.HyperlinkedModelSerializer',

    'DEFAULT_RENDERER_CLASSES': (
        'rest_framework.renderers.JSONRenderer',
        'rest_framework.renderers.BrowsableAPIRenderer',
    ),
    'DEFAULT_FILTER_BACKENDS': (
        'rest_framework.filters.DjangoFilterBackend',
    ),
    'DEFAULT_PAGINATION_CLASS': 'm1_news.apps.api.pagination.StandardResultsSetPagination',
    'PAGE_SIZE': 25,
}

custom pagination

# -*- coding: utf-8 -*-
from rest_framework.pagination import PageNumberPagination


class StandardResultsSetPagination(PageNumberPagination):
    page_size = 25

still results in

[
  {
    "id": 427,
    ...
  },
]

@rosscdh
Copy link

rosscdh commented Dec 11, 2015

Also getting, when browsing the 'rest_framework.renderers.BrowsableAPIRenderer',

VariableDoesNotExist: Failed lookup for key [form] in u'None'

@xordoquy
Copy link
Collaborator

@rosscdh The discussion group is the best place to take usage questions. Thanks!

@rosscdh
Copy link

rosscdh commented Dec 11, 2015

@xordoquy Im not sure this is a usage problem. Followed docs and notes:

http://www.django-rest-framework.org/topics/3.3-announcement/#deprecations
http://www.django-rest-framework.org/api-guide/pagination/

no joy.

Am a very long time user of DRF. Not a new project.

@xordoquy
Copy link
Collaborator

Unless we have a minimal test case I'm going to consider it's a usage issue.
Using DRF 3.3.1 on the tutorial with

REST_FRAMEWORK = {
    'PAGE_SIZE': 10,
}

gives me:

{
    "count": 5, 
    "previous": null, 
    "results": [
        {
            "url": "http://localhost:8000/users/2/?format=json", 
            "username": "aziz", 
            "snippets": []
        }, 
        {
            "url": "http://localhost:8000/users/3/?format=json", 
            "username": "amy", 
            "snippets": []
        }, 
        {
            "url": "http://localhost:8000/users/4/?format=json", 
            "username": "max", 
            "snippets": []
        }, 
        {
            "url": "http://localhost:8000/users/5/?format=json", 
            "username": "jose", 
            "snippets": []
        }, 
        {
            "url": "http://localhost:8000/users/6/?format=json", 
            "username": "admin", 
            "snippets": [
                "http://localhost:8000/snippets/1/?format=json", 
                "http://localhost:8000/snippets/2/?format=json"
            ]
        }
    ], 
    "next": null
}

Note it's the same with an explicit pagination_class set on the view.

@xordoquy xordoquy modified the milestones: 3.3.2 Release, 3.3.3 Release Dec 14, 2015
@xordoquy xordoquy modified the milestones: 3.3.3 Release, 3.3.2 Release Dec 14, 2015
tomchristie added a commit that referenced this issue Dec 18, 2015
Raise error when setting a removed rest_framework setting for #3644
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants