Skip to content

Fix included resources not being included on list #307

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Feb 22, 2017
Merged

Fix included resources not being included on list #307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
77868fc
Fix included resources not being included on list
rpatterson Dec 5, 2016
f0fa0e7
Fix included resources M2M type (TODO test)
rpatterson Feb 13, 2017
c4513e7
PR #307: Remove TODO comment
rpatterson Feb 22, 2017
a4e71d5
PR #307: Fix included_resources assumption
rpatterson Feb 22, 2017
ede0ad4
PR #307: Fix redundant try/except
rpatterson Feb 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions example/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,9 @@ class Meta:
'authors', 'comments', 'featured', 'suggested',)
meta_fields = ('body_format',)

class JSONAPIMeta:
included_resources = ['comments']


class AuthorBioSerializer(serializers.ModelSerializer):

Expand Down
7 changes: 0 additions & 7 deletions example/tests/integration/test_includes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,9 @@

from example.tests.utils import load_json

try:
from unittest import mock
except ImportError:
import mock

pytestmark = pytest.mark.django_db


@mock.patch('rest_framework_json_api.utils.get_default_included_resources_from_serializer', new=lambda s: ['comments'])
def test_default_included_data_on_list(multiple_entries, client):
return test_included_data_on_list(multiple_entries=multiple_entries, client=client, query='?page_size=5')

Expand All @@ -28,7 +22,6 @@ def test_included_data_on_list(multiple_entries, client, query='?include=comment
assert comment_count == expected_comment_count, 'List comment count is incorrect'


@mock.patch('rest_framework_json_api.utils.get_default_included_resources_from_serializer', new=lambda s: ['comments'])
def test_default_included_data_on_detail(single_entry, client):
return test_included_data_on_detail(single_entry=single_entry, client=client, query='')

Expand Down
9 changes: 9 additions & 0 deletions example/tests/integration/test_non_paginated_responses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
from django.core.urlresolvers import reverse
from django.conf import settings

try:
from unittest import mock
except ImportError:
import mock

import pytest

from example.views import EntryViewSet
Expand All @@ -12,6 +17,10 @@


# rf == request_factory
@mock.patch(
'rest_framework_json_api.utils'
'.get_default_included_resources_from_serializer',
new=lambda s: [])
def test_multiple_entries_no_pagination(multiple_entries, rf):

expected = {
Expand Down
10 changes: 9 additions & 1 deletion example/tests/integration/test_pagination.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,19 @@
from django.core.urlresolvers import reverse

try:
from unittest import mock
except ImportError:
import mock

import pytest
from example.tests.utils import dump_json, redump_json

pytestmark = pytest.mark.django_db


@mock.patch(
'rest_framework_json_api.utils'
'.get_default_included_resources_from_serializer',
new=lambda s: [])
def test_pagination_with_single_entry(single_entry, client):

expected = {
Expand Down
15 changes: 11 additions & 4 deletions rest_framework_json_api/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,13 @@ def get_related_resource_type(relation):
relation_model = relation.model
elif hasattr(relation, 'get_queryset') and relation.get_queryset() is not None:
relation_model = relation.get_queryset().model
elif (
getattr(relation, 'many', False) and
hasattr(relation.child, 'Meta') and
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if these hasattr checks are a bit too much. A serializer doesn't have to be a ModelSerializer so there is no requirement for Meta to exist, but isn't model a required attribute of Meta for a ModelSerializer? It feels slightly too defensive to me to check that model exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand. If the serializer may not be a ModelSerializer, then shouldn't we not assume that Meta has a model?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you're right. I thought a vanilla serializer made no use of Meta, but I can see from the source that that is not correct. By all means, please keep model. 😊

hasattr(relation.child.Meta, 'model')):
# For ManyToMany relationships, get the model from the child
# serializer of the list serializer
relation_model = relation.child.Meta.model
else:
parent_serializer = relation.parent
parent_model = None
Expand Down Expand Up @@ -266,10 +273,10 @@ def get_included_resources(request, serializer=None):


def get_default_included_resources_from_serializer(serializer):
try:
return list(serializer.JSONAPIMeta.included_resources)
except AttributeError:
return []
meta = getattr(serializer, 'JSONAPIMeta', None)
if meta is None and getattr(serializer, 'many', False):
meta = getattr(serializer.child, 'JSONAPIMeta', None)
return list(getattr(meta, 'included_resources', []))


def get_included_serializers(serializer):
Expand Down