Skip to content

Added nested included serializer support for remapped relations #347

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 2 commits into from
Jun 5, 2017
Merged

Added nested included serializer support for remapped relations #347

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2d0303b
Added nested included serializer support for remapped relations
May 11, 2017
5023383
Added docstring to extract_relation_instance
sliverc May 30, 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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ v2.3.0
* Fix for apps that don't use `django.contrib.contenttypes`.
* Fix `resource_name` support for POST requests and nested serializers
* Enforcing flake8 linting
* Added nested included serializer support for remapped relations

v2.2.0

Expand Down
17 changes: 16 additions & 1 deletion example/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,25 @@ class Meta:
fields = ('name', 'email', 'bio')


class WriterSerializer(serializers.ModelSerializer):
included_serializers = {
'bio': AuthorBioSerializer
}

class Meta:
model = Author
fields = ('name', 'email', 'bio')
resource_name = 'writers'


class CommentSerializer(serializers.ModelSerializer):
# testing remapping of related name
writer = relations.ResourceRelatedField(source='author', read_only=True)

included_serializers = {
'entry': EntrySerializer,
'author': AuthorSerializer
'author': AuthorSerializer,
'writer': WriterSerializer
}

class Meta:
Expand Down
12 changes: 10 additions & 2 deletions example/tests/integration/test_includes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,14 +82,15 @@ def test_missing_field_not_included(author_bio_factory, author_factory, client):

def test_deep_included_data_on_list(multiple_entries, client):
response = client.get(reverse("entry-list") + '?include=comments,comments.author,'
'comments.author.bio&page_size=5')
'comments.author.bio,comments.writer&page_size=5')
included = load_json(response.content).get('included')

assert len(load_json(response.content)['data']) == len(multiple_entries), (
'Incorrect entry count'
)
assert [x.get('type') for x in included] == [
'authorBios', 'authorBios', 'authors', 'authors', 'comments', 'comments'
'authorBios', 'authorBios', 'authors', 'authors',
'comments', 'comments', 'writers', 'writers'
], 'List included types are incorrect'

comment_count = len([resource for resource in included if resource["type"] == "comments"])
Expand All @@ -106,6 +107,13 @@ def test_deep_included_data_on_list(multiple_entries, client):
author__bio__isnull=False).count() for entry in multiple_entries])
assert author_bio_count == expected_author_bio_count, 'List author bio count is incorrect'

writer_count = len(
[resource for resource in included if resource["type"] == "writers"]
)
expected_writer_count = sum(
[entry.comments.filter(author__isnull=False).count() for entry in multiple_entries])
assert writer_count == expected_writer_count, 'List writer count is incorrect'

# Also include entry authors
response = client.get(reverse("entry-list") + '?include=authors,comments,comments.author,'
'comments.author.bio&page_size=5')
Expand Down
6 changes: 6 additions & 0 deletions example/tests/test_serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,12 @@ def test_model_serializer_with_implicit_fields(self, comment, client):
"id": str(comment.author.pk)
}
},
"writer": {
"data": {
"type": "writers",
"id": str(comment.author.pk)
}
},
}
}
}
Expand Down
40 changes: 27 additions & 13 deletions rest_framework_json_api/renderers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,30 @@ def extract_relationships(cls, fields, resource, resource_instance):

return utils.format_keys(data)

@classmethod
def extract_relation_instance(cls, field_name, field, resource_instance, serializer):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please add a docstring which provides a conceptual overview of what's going on here? I'm finding the four nested levels to be really intense. If you could use PEP 257 style, that would be great.

"""Extract ...

<relevant details that might help understand what's going on here>
"""

This many levels of exceptions makes me wonder if there might performance challenges here (because of the cost of going through the exception stack over and over). I don't think it's fair to ask you for a performance test so I'm mostly wondering "out loud" if this is too much exception handling as a way to do control flow. It does feel like a bit of a code smell. Maybe that's the cost of working with a spec that's so dynamic ¯\(ツ)/¯

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added a docstring.

Considering the levels of exceptions I also think this is certainly not ideal but I cannot think of an alternative way to do it though.

For example calling getattr with default argument None wouldn't work, as an attribute could actually be None. Calling hasattr wouldn't make things better either as internally it also does a try-except (https://docs.python.org/3/library/functions.html#hasattr).
It could be argued whether the serializer method case is really needed but otherwise I guess we will need to live with it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for thinking about it. The only alternative that I can think of is to have multiple returns. What do you think of something like the following:

         try:
             return getattr(resource_instance, field_name)
         except AttributeError:
             pass

         try:
             # For ManyRelatedFields if `related_name` is not set
             # we need to access `foo_set` from `source`
             return getattr(resource_instance, field.child_relation.source)
         except AttributeError:
             pass

        <and so on...>

Is that any "better?" Or is it worse? It would remove the deep nesting even if the logic is functionally the same.

relation_instance = None

try:
relation_instance = getattr(resource_instance, field_name)
except AttributeError:
try:
# For ManyRelatedFields if `related_name` is not set
# we need to access `foo_set` from `source`
relation_instance = getattr(resource_instance, field.child_relation.source)
except AttributeError:
if hasattr(serializer, field.source):
serializer_method = getattr(serializer, field.source)
relation_instance = serializer_method(resource_instance)
else:
# case when source is a simple remap on resource_instance
try:
relation_instance = getattr(resource_instance, field.source)
except AttributeError:
pass

return relation_instance

@classmethod
def extract_included(cls, fields, resource, resource_instance, included_resources):
# this function may be called with an empty record (example: Browsable Interface)
Expand Down Expand Up @@ -304,19 +328,9 @@ def extract_included(cls, fields, resource, resource_instance, included_resource
if field_name not in [node.split('.')[0] for node in included_resources]:
continue

try:
relation_instance = getattr(resource_instance, field_name)
except AttributeError:
try:
# For ManyRelatedFields if `related_name` is not set we need to access `foo_set`
# from `source`
relation_instance = getattr(resource_instance, field.child_relation.source)
except AttributeError:
if not hasattr(current_serializer, field.source):
continue
serializer_method = getattr(current_serializer, field.source)
relation_instance = serializer_method(resource_instance)

relation_instance = cls.extract_relation_instance(
field_name, field, resource_instance, current_serializer
)
if isinstance(relation_instance, Manager):
relation_instance = relation_instance.all()

Expand Down