From 720a37d3dedc501968bebaca3a339c72392b9c81 Mon Sep 17 00:00:00 2001 From: Tom Christie Date: Tue, 9 Dec 2014 17:28:56 +0000 Subject: [PATCH 1/4] Hyperlinked PK optimization. Closes #1872. --- rest_framework/relations.py | 64 ++++++++++++++++++------------- tests/test_relations_hyperlink.py | 12 ++++-- 2 files changed, 45 insertions(+), 31 deletions(-) diff --git a/rest_framework/relations.py b/rest_framework/relations.py index 75d68204b..ae4086ecf 100644 --- a/rest_framework/relations.py +++ b/rest_framework/relations.py @@ -84,9 +84,34 @@ class RelatedField(Field): queryset = queryset.all() return queryset + def use_pk_only_optimization(self): + return False + + def get_attribute(self, instance): + if self.use_pk_only_optimization(): + try: + # Optimized case, return a mock object only containing the pk attribute. + instance = get_attribute(instance, self.source_attrs[:-1]) + return PKOnlyObject(pk=instance.serializable_value(self.source_attrs[-1])) + except AttributeError: + pass + + # Standard case, return the object instance. + return get_attribute(instance, self.source_attrs) + def get_iterable(self, instance, source_attrs): relationship = get_attribute(instance, source_attrs) - return relationship.all() if (hasattr(relationship, 'all')) else relationship + relationship = relationship.all() if (hasattr(relationship, 'all')) else relationship + + if self.use_pk_only_optimization(): + # Optimized case, return mock objects only containing the pk attribute. + return [ + PKOnlyObject(pk=pk) + for pk in relationship.values_list('pk', flat=True) + ] + + # Standard case, return the object instances. + return relationship @property def choices(self): @@ -120,6 +145,9 @@ class PrimaryKeyRelatedField(RelatedField): 'incorrect_type': _('Incorrect type. Expected pk value, received {data_type}.'), } + def use_pk_only_optimization(self): + return True + def to_internal_value(self, data): try: return self.get_queryset().get(pk=data) @@ -128,32 +156,6 @@ class PrimaryKeyRelatedField(RelatedField): except (TypeError, ValueError): self.fail('incorrect_type', data_type=type(data).__name__) - def get_attribute(self, instance): - # We customize `get_attribute` here for performance reasons. - # For relationships the instance will already have the pk of - # the related object. We return this directly instead of returning the - # object itself, which would require a database lookup. - try: - instance = get_attribute(instance, self.source_attrs[:-1]) - return PKOnlyObject(pk=instance.serializable_value(self.source_attrs[-1])) - except AttributeError: - return get_attribute(instance, self.source_attrs) - - def get_iterable(self, instance, source_attrs): - # For consistency with `get_attribute` we're using `serializable_value()` - # here. Typically there won't be any difference, but some custom field - # types might return a non-primitive value for the pk otherwise. - # - # We could try to get smart with `values_list('pk', flat=True)`, which - # would be better in some case, but would actually end up with *more* - # queries if the developer is using `prefetch_related` across the - # relationship. - relationship = super(PrimaryKeyRelatedField, self).get_iterable(instance, source_attrs) - return [ - PKOnlyObject(pk=item.serializable_value('pk')) - for item in relationship - ] - def to_representation(self, value): return value.pk @@ -184,6 +186,9 @@ class HyperlinkedRelatedField(RelatedField): super(HyperlinkedRelatedField, self).__init__(**kwargs) + def use_pk_only_optimization(self): + return self.lookup_field == 'pk' + def get_object(self, view_name, view_args, view_kwargs): """ Return the object corresponding to a matched URL. @@ -285,6 +290,11 @@ class HyperlinkedIdentityField(HyperlinkedRelatedField): kwargs['source'] = '*' super(HyperlinkedIdentityField, self).__init__(view_name, **kwargs) + def use_pk_only_optimization(self): + # We have the complete object instance already. We don't need + # to run the 'only get the pk for this relationship' code. + return False + class SlugRelatedField(RelatedField): """ diff --git a/tests/test_relations_hyperlink.py b/tests/test_relations_hyperlink.py index b938e3857..e741e99be 100644 --- a/tests/test_relations_hyperlink.py +++ b/tests/test_relations_hyperlink.py @@ -89,7 +89,8 @@ class HyperlinkedManyToManyTests(TestCase): {'url': 'http://testserver/manytomanysource/2/', 'name': 'source-2', 'targets': ['http://testserver/manytomanytarget/1/', 'http://testserver/manytomanytarget/2/']}, {'url': 'http://testserver/manytomanysource/3/', 'name': 'source-3', 'targets': ['http://testserver/manytomanytarget/1/', 'http://testserver/manytomanytarget/2/', 'http://testserver/manytomanytarget/3/']} ] - self.assertEqual(serializer.data, expected) + with self.assertNumQueries(4): + self.assertEqual(serializer.data, expected) def test_reverse_many_to_many_retrieve(self): queryset = ManyToManyTarget.objects.all() @@ -99,7 +100,8 @@ class HyperlinkedManyToManyTests(TestCase): {'url': 'http://testserver/manytomanytarget/2/', 'name': 'target-2', 'sources': ['http://testserver/manytomanysource/2/', 'http://testserver/manytomanysource/3/']}, {'url': 'http://testserver/manytomanytarget/3/', 'name': 'target-3', 'sources': ['http://testserver/manytomanysource/3/']} ] - self.assertEqual(serializer.data, expected) + with self.assertNumQueries(4): + self.assertEqual(serializer.data, expected) def test_many_to_many_update(self): data = {'url': 'http://testserver/manytomanysource/1/', 'name': 'source-1', 'targets': ['http://testserver/manytomanytarget/1/', 'http://testserver/manytomanytarget/2/', 'http://testserver/manytomanytarget/3/']} @@ -197,7 +199,8 @@ class HyperlinkedForeignKeyTests(TestCase): {'url': 'http://testserver/foreignkeysource/2/', 'name': 'source-2', 'target': 'http://testserver/foreignkeytarget/1/'}, {'url': 'http://testserver/foreignkeysource/3/', 'name': 'source-3', 'target': 'http://testserver/foreignkeytarget/1/'} ] - self.assertEqual(serializer.data, expected) + with self.assertNumQueries(1): + self.assertEqual(serializer.data, expected) def test_reverse_foreign_key_retrieve(self): queryset = ForeignKeyTarget.objects.all() @@ -206,7 +209,8 @@ class HyperlinkedForeignKeyTests(TestCase): {'url': 'http://testserver/foreignkeytarget/1/', 'name': 'target-1', 'sources': ['http://testserver/foreignkeysource/1/', 'http://testserver/foreignkeysource/2/', 'http://testserver/foreignkeysource/3/']}, {'url': 'http://testserver/foreignkeytarget/2/', 'name': 'target-2', 'sources': []}, ] - self.assertEqual(serializer.data, expected) + with self.assertNumQueries(3): + self.assertEqual(serializer.data, expected) def test_foreign_key_update(self): data = {'url': 'http://testserver/foreignkeysource/1/', 'name': 'source-1', 'target': 'http://testserver/foreignkeytarget/2/'} From ca7b1f6d5189398be8a0d24b1e01577281b1b187 Mon Sep 17 00:00:00 2001 From: Tom Christie Date: Wed, 10 Dec 2014 21:09:45 +0000 Subject: [PATCH 2/4] Optimizations play nicely with select_related, prefetch_related --- rest_framework/relations.py | 12 +----------- tests/test_relations_hyperlink.py | 6 ++++++ tests/test_relations_pk.py | 12 ++++++++++++ tests/test_relations_slug.py | 15 ++++++++++++++- 4 files changed, 33 insertions(+), 12 deletions(-) diff --git a/rest_framework/relations.py b/rest_framework/relations.py index ae4086ecf..6b693e4b2 100644 --- a/rest_framework/relations.py +++ b/rest_framework/relations.py @@ -101,17 +101,7 @@ class RelatedField(Field): def get_iterable(self, instance, source_attrs): relationship = get_attribute(instance, source_attrs) - relationship = relationship.all() if (hasattr(relationship, 'all')) else relationship - - if self.use_pk_only_optimization(): - # Optimized case, return mock objects only containing the pk attribute. - return [ - PKOnlyObject(pk=pk) - for pk in relationship.values_list('pk', flat=True) - ] - - # Standard case, return the object instances. - return relationship + return relationship.all() if (hasattr(relationship, 'all')) else relationship @property def choices(self): diff --git a/tests/test_relations_hyperlink.py b/tests/test_relations_hyperlink.py index e741e99be..f1b882edf 100644 --- a/tests/test_relations_hyperlink.py +++ b/tests/test_relations_hyperlink.py @@ -92,6 +92,12 @@ class HyperlinkedManyToManyTests(TestCase): with self.assertNumQueries(4): self.assertEqual(serializer.data, expected) + def test_many_to_many_retrieve_prefetch_related(self): + queryset = ManyToManySource.objects.all().prefetch_related('targets') + serializer = ManyToManySourceSerializer(queryset, many=True, context={'request': request}) + with self.assertNumQueries(2): + serializer.data + def test_reverse_many_to_many_retrieve(self): queryset = ManyToManyTarget.objects.all() serializer = ManyToManyTargetSerializer(queryset, many=True, context={'request': request}) diff --git a/tests/test_relations_pk.py b/tests/test_relations_pk.py index e95a877e1..f872a8dc5 100644 --- a/tests/test_relations_pk.py +++ b/tests/test_relations_pk.py @@ -71,6 +71,12 @@ class PKManyToManyTests(TestCase): with self.assertNumQueries(4): self.assertEqual(serializer.data, expected) + def test_many_to_many_retrieve_prefetch_related(self): + queryset = ManyToManySource.objects.all().prefetch_related('targets') + serializer = ManyToManySourceSerializer(queryset, many=True) + with self.assertNumQueries(2): + serializer.data + def test_reverse_many_to_many_retrieve(self): queryset = ManyToManyTarget.objects.all() serializer = ManyToManyTargetSerializer(queryset, many=True) @@ -188,6 +194,12 @@ class PKForeignKeyTests(TestCase): with self.assertNumQueries(3): self.assertEqual(serializer.data, expected) + def test_reverse_foreign_key_retrieve_prefetch_related(self): + queryset = ForeignKeyTarget.objects.all().prefetch_related('sources') + serializer = ForeignKeyTargetSerializer(queryset, many=True) + with self.assertNumQueries(2): + serializer.data + def test_foreign_key_update(self): data = {'id': 1, 'name': 'source-1', 'target': 2} instance = ForeignKeySource.objects.get(pk=1) diff --git a/tests/test_relations_slug.py b/tests/test_relations_slug.py index 7bac90460..cd2cb1ed6 100644 --- a/tests/test_relations_slug.py +++ b/tests/test_relations_slug.py @@ -54,7 +54,14 @@ class SlugForeignKeyTests(TestCase): {'id': 2, 'name': 'source-2', 'target': 'target-1'}, {'id': 3, 'name': 'source-3', 'target': 'target-1'} ] - self.assertEqual(serializer.data, expected) + with self.assertNumQueries(4): + self.assertEqual(serializer.data, expected) + + def test_foreign_key_retrieve_select_related(self): + queryset = ForeignKeySource.objects.all().select_related('target') + serializer = ForeignKeySourceSerializer(queryset, many=True) + with self.assertNumQueries(1): + serializer.data def test_reverse_foreign_key_retrieve(self): queryset = ForeignKeyTarget.objects.all() @@ -65,6 +72,12 @@ class SlugForeignKeyTests(TestCase): ] self.assertEqual(serializer.data, expected) + def test_reverse_foreign_key_retrieve_prefetch_related(self): + queryset = ForeignKeyTarget.objects.all().prefetch_related('sources') + serializer = ForeignKeyTargetSerializer(queryset, many=True) + with self.assertNumQueries(2): + serializer.data + def test_foreign_key_update(self): data = {'id': 1, 'name': 'source-1', 'target': 'target-2'} instance = ForeignKeySource.objects.get(pk=1) From 9d3810f313123ce48dea48f3c3f0f2cae5816bde Mon Sep 17 00:00:00 2001 From: Tom Christie Date: Wed, 10 Dec 2014 22:09:24 +0000 Subject: [PATCH 3/4] Drop get_iterable() --- rest_framework/relations.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/rest_framework/relations.py b/rest_framework/relations.py index 6b693e4b2..a5f975967 100644 --- a/rest_framework/relations.py +++ b/rest_framework/relations.py @@ -88,7 +88,7 @@ class RelatedField(Field): return False def get_attribute(self, instance): - if self.use_pk_only_optimization(): + if self.use_pk_only_optimization() and self.source_attrs: try: # Optimized case, return a mock object only containing the pk attribute. instance = get_attribute(instance, self.source_attrs[:-1]) @@ -99,10 +99,6 @@ class RelatedField(Field): # Standard case, return the object instance. return get_attribute(instance, self.source_attrs) - def get_iterable(self, instance, source_attrs): - relationship = get_attribute(instance, source_attrs) - return relationship.all() if (hasattr(relationship, 'all')) else relationship - @property def choices(self): return dict([ @@ -349,7 +345,8 @@ class ManyRelatedField(Field): ] def get_attribute(self, instance): - return self.child_relation.get_iterable(instance, self.source_attrs) + relationship = get_attribute(instance, self.source_attrs) + return relationship.all() if (hasattr(relationship, 'all')) else relationship def to_representation(self, iterable): return [ From 1e336ef30da9f7366fc68185016c4011c25e1199 Mon Sep 17 00:00:00 2001 From: Tom Christie Date: Wed, 10 Dec 2014 22:10:45 +0000 Subject: [PATCH 4/4] Move comment --- rest_framework/relations.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rest_framework/relations.py b/rest_framework/relations.py index a5f975967..892ce6c19 100644 --- a/rest_framework/relations.py +++ b/rest_framework/relations.py @@ -89,8 +89,8 @@ class RelatedField(Field): def get_attribute(self, instance): if self.use_pk_only_optimization() and self.source_attrs: + # Optimized case, return a mock object only containing the pk attribute. try: - # Optimized case, return a mock object only containing the pk attribute. instance = get_attribute(instance, self.source_attrs[:-1]) return PKOnlyObject(pk=instance.serializable_value(self.source_attrs[-1])) except AttributeError: