Do not lazy load forward relations #461

timcbaoth · 2018-08-21T11:43:14Z

If the forward relations are included, the renderer will break, so this optimization only works if it is not included.

Description of the Change

Fixes a copy paste error, which resulted in an optimization not being applied.

Checklist

PR only contains one change (considered splitting up PR)
unit-test added
documentation updated
changelog entry added to CHANGELOG.md
author name in AUTHORS

codecov-io · 2018-08-21T13:13:26Z

Codecov Report

Merging #461 into master will increase coverage by 0.29%.
The diff coverage is 98.11%.

@@            Coverage Diff             @@
##           master     #461      +/-   ##
==========================================
+ Coverage   93.44%   93.73%   +0.29%     
==========================================
  Files          56       56              
  Lines        3217     3255      +38     
==========================================
+ Hits         3006     3051      +45     
+ Misses        211      204       -7

Impacted Files	Coverage Δ
example/tests/test_serializers.py	`100% <100%> (ø)`	⬆️
rest_framework_json_api/serializers.py	`85.88% <94.11%> (+4.63%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a320536...91bb86d. Read the comment docs.

sliverc · 2018-08-21T20:57:39Z

Could you elaborate what you mean with the rendererer will break? I think best would be to reproduce this in a test case then I think things will be clearer and we make sure this error won't happen again in the future.

timcbaoth · 2018-08-22T05:58:55Z

The quoted part is only an explanation for why I added the check
is_included = field.source in get_included_resources(request)
The meat of the fix was changing
and hasattr(field, field.source + "_id")
to
and hasattr(instance, field.source + "_id")
in the if statement, which was probably a typo in the first place.

I will try to submit a test case to cover these changes.

timcbaoth · 2018-08-29T07:54:52Z

A test case was added.

sliverc

Implementation looks good. Just a few comments on the testing front.
Thanks for working on this.

sliverc · 2018-08-29T07:56:33Z

example/tests/test_serializers.py

@@ -30,6 +32,32 @@ def setUp(self):
                Author.objects.create(name=name, email='{}@example.org'.format(name))
            )

+    def test_forward_relationship_not_loaded_when_not_included(self):
+        MockRequest = namedtuple('Request', ['query_params'])


Maybe better to use from rest_framework.test import APIRequestFactory. There is an example in unit/test_pagination.py how to use it.

sliverc · 2018-08-29T08:03:19Z

example/tests/test_serializers.py

+                fields = '__all__'
+
+            def to_representation(self, instance):
+                raise Exception('to_representation of BlogSerializer was called')


the disadvantage of this way of testing is that this code will actually never be covered. A bit odd when test code is not covered and the long term goal of DJA would be to have 100% coverage.

So I would prefer if the already existing serializers in example/serializers.py are used for this. to_representation would need to be mocked and assert_not_called could be used to check that to_representation has not been called.

I didn't think about it in this way, that you also want full coverage on test code itself. Interesting.

Anyway it is nicer to have a proper assert, so I changed it according to your suggestion.

sliverc · 2018-08-29T08:03:41Z

example/tests/test_serializers.py

+            }
+
+        serializer = EntrySerializer(context={'request': request_without_includes})
+        serializer.to_representation(self.entry)


I think it would also be good to assert the result.

I agree. I added another test to validate the result.

sliverc · 2018-08-29T08:05:45Z

rest_framework_json_api/serializers.py

        return ret

+    def _get_field_representation(self, field, instance):
+        request = self.context.get('request', None)


None is the default return value when using get. So seems to be a bit cleaner to use self.context.get('request').

👍 to extract this logic into its own method.

Changed it. Good suggestion!

sliverc

Looks good.

One small comment + rebase to master + add yourself to AUTHROS + changelog entry and I think this is ready for merging.

sliverc · 2018-08-29T15:14:41Z

example/tests/test_serializers.py

+        result.pop('created_at')
+        result.pop('modified_at')
+
+        expected = OrderedDict(


ordering is not relevant in an assertion so I think the code would be more readable using normal dict.

Makes sense. I will also change it to assertDictEqual.

Sync

If the forward relations are included, the renderer will break, so this optimization only works if it is not included. fixup! Do not lazy load forward relations fixup! Do not lazy load forward relations fixup! Do not lazy load forward relations fixup! Do not lazy load forward relations fixup! Do not lazy load forward relations Include test case Small refactoring Make compatible with Python 2.X fixup! Make compatible with Python 2.X Fix travis Use isort Implement suggested changes Make import backward compatible Make travis happy

timcbaoth force-pushed the include-forward-relation branch from a7c0766 to bba4f6e Compare August 28, 2018 14:38

sliverc requested changes Aug 29, 2018

View reviewed changes

timcbaoth force-pushed the include-forward-relation branch from 4239178 to 64f8ae3 Compare August 29, 2018 13:46

sliverc requested changes Aug 29, 2018

View reviewed changes

Merge pull request #1 from django-json-api/master

a12de94

Sync

timcbaoth force-pushed the include-forward-relation branch from 64f8ae3 to 7186b0c Compare August 31, 2018 08:39

timcbaoth force-pushed the include-forward-relation branch from 7186b0c to 91bb86d Compare August 31, 2018 08:48

sliverc approved these changes Sep 4, 2018

View reviewed changes

sliverc added 2 commits September 4, 2018 09:42

Clarify changelog entry

c794f0b

Update CHANGELOG.md

b5b5ce7

sliverc merged commit 22c4587 into django-json-api:master Sep 4, 2018

n2ygk added this to the 2.6.0 milestone Sep 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not lazy load forward relations #461

Do not lazy load forward relations #461

timcbaoth commented Aug 21, 2018 •

edited by sliverc

Loading

codecov-io commented Aug 21, 2018 •

edited

Loading

sliverc commented Aug 21, 2018

timcbaoth commented Aug 22, 2018

timcbaoth commented Aug 29, 2018

sliverc left a comment

sliverc Aug 29, 2018

timcbaoth Aug 29, 2018

sliverc Aug 29, 2018

timcbaoth Aug 29, 2018

sliverc Aug 29, 2018

timcbaoth Aug 29, 2018

sliverc Aug 29, 2018

timcbaoth Aug 29, 2018

sliverc left a comment

sliverc Aug 29, 2018 •

edited

Loading

timcbaoth Aug 31, 2018

Do not lazy load forward relations #461

Do not lazy load forward relations #461

Conversation

timcbaoth commented Aug 21, 2018 • edited by sliverc Loading

Description of the Change

Checklist

codecov-io commented Aug 21, 2018 • edited Loading

Codecov Report

sliverc commented Aug 21, 2018

timcbaoth commented Aug 22, 2018

timcbaoth commented Aug 29, 2018

sliverc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sliverc left a comment

Choose a reason for hiding this comment

sliverc Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timcbaoth commented Aug 21, 2018 •

edited by sliverc

Loading

codecov-io commented Aug 21, 2018 •

edited

Loading

sliverc Aug 29, 2018 •

edited

Loading