From ce98c3d92179f65868f7c900867bc24140501f8f Mon Sep 17 00:00:00 2001 From: bphillips Date: Wed, 2 Dec 2015 13:37:23 -0500 Subject: [PATCH 1/5] Refactored _get_reverse_relationships() to use related object's pk field instead of current object's --- rest_framework/utils/model_meta.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rest_framework/utils/model_meta.py b/rest_framework/utils/model_meta.py index f151c6f36e..c2afd9691e 100644 --- a/rest_framework/utils/model_meta.py +++ b/rest_framework/utils/model_meta.py @@ -145,7 +145,7 @@ def _get_reverse_relationships(opts): model_field=None, related_model=related, to_many=relation.field.rel.multiple, - to_field=_get_to_field(relation.field), + to_field=_get_to_field(relation.field.model._meta.pk), has_through_model=False ) From 21839e45c6da35ea8c2155b7840d0d0cd863f0b0 Mon Sep 17 00:00:00 2001 From: bphillips Date: Thu, 3 Dec 2015 14:08:59 -0500 Subject: [PATCH 2/5] Modified build_relational_field() to change its behaviour if relationship is reverse vs forward to ensure correct field checking --- rest_framework/serializers.py | 11 ++++++++--- rest_framework/utils/field_mapping.py | 2 +- rest_framework/utils/model_meta.py | 22 ++++++++++++++++------ 3 files changed, 25 insertions(+), 10 deletions(-) diff --git a/rest_framework/serializers.py b/rest_framework/serializers.py index 99d36a8a54..8ed8120996 100644 --- a/rest_framework/serializers.py +++ b/rest_framework/serializers.py @@ -1131,9 +1131,14 @@ def build_relational_field(self, field_name, relation_info): field_kwargs = get_relation_kwargs(field_name, relation_info) to_field = field_kwargs.pop('to_field', None) - if to_field and not relation_info.related_model._meta.get_field(to_field).primary_key: - field_kwargs['slug_field'] = to_field - field_class = self.serializer_related_to_field + if relation_info.reverse: + if to_field and not relation_info.related_model_field.related_field.primary_key: + field_kwargs['slug_field'] = to_field + field_class = self.serializer_related_to_field + else: + if to_field and not relation_info.related_model._meta.get_field(to_field).primary_key: + field_kwargs['slug_field'] = to_field + field_class = self.serializer_related_to_field # `view_name` is only valid for hyperlinked relationships. if not issubclass(field_class, HyperlinkedRelatedField): diff --git a/rest_framework/utils/field_mapping.py b/rest_framework/utils/field_mapping.py index 1d8cb22f2f..8642ccf55c 100644 --- a/rest_framework/utils/field_mapping.py +++ b/rest_framework/utils/field_mapping.py @@ -238,7 +238,7 @@ def get_relation_kwargs(field_name, relation_info): """ Creates a default instance of a flat relational field. """ - model_field, related_model, to_many, to_field, has_through_model = relation_info + model_field, related_model, related_model_field, to_many, to_field, has_through_model, reverse = relation_info kwargs = { 'queryset': related_model._default_manager, 'view_name': get_detail_view_name(related_model) diff --git a/rest_framework/utils/model_meta.py b/rest_framework/utils/model_meta.py index c2afd9691e..c496d27113 100644 --- a/rest_framework/utils/model_meta.py +++ b/rest_framework/utils/model_meta.py @@ -29,9 +29,11 @@ RelationInfo = namedtuple('RelationInfo', [ 'model_field', 'related_model', + 'related_model_field', 'to_many', 'to_field', - 'has_through_model' + 'has_through_model', + 'reverse' ]) @@ -108,9 +110,11 @@ def _get_forward_relationships(opts): forward_relations[field.name] = RelationInfo( model_field=field, related_model=_resolve_model(field.rel.to), + related_model_field=None, to_many=False, to_field=_get_to_field(field), - has_through_model=False + has_through_model=False, + reverse=False ) # Deal with forward many-to-many relationships. @@ -118,12 +122,14 @@ def _get_forward_relationships(opts): forward_relations[field.name] = RelationInfo( model_field=field, related_model=_resolve_model(field.rel.to), + related_model_field=None, to_many=True, # manytomany do not have to_fields to_field=None, has_through_model=( not field.rel.through._meta.auto_created - ) + ), + reverse=False ) return forward_relations @@ -144,9 +150,11 @@ def _get_reverse_relationships(opts): reverse_relations[accessor_name] = RelationInfo( model_field=None, related_model=related, + related_model_field=relation.field, to_many=relation.field.rel.multiple, - to_field=_get_to_field(relation.field.model._meta.pk), - has_through_model=False + to_field=_get_to_field(relation.field), + has_through_model=False, + reverse=True ) # Deal with reverse many-to-many relationships. @@ -156,13 +164,15 @@ def _get_reverse_relationships(opts): reverse_relations[accessor_name] = RelationInfo( model_field=None, related_model=related, + related_model_field=relation.field, to_many=True, # manytomany do not have to_fields to_field=None, has_through_model=( (getattr(relation.field.rel, 'through', None) is not None) and not relation.field.rel.through._meta.auto_created - ) + ), + reverse=True ) return reverse_relations From dae5a5ce43f63e96c10dce2ad1e9c080db172a22 Mon Sep 17 00:00:00 2001 From: bphillips Date: Mon, 7 Dec 2015 11:13:04 -0500 Subject: [PATCH 3/5] Fixed Django1.9 compatibility and added test. --- rest_framework/serializers.py | 2 +- tests/test_model_serializer.py | 35 ++++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/rest_framework/serializers.py b/rest_framework/serializers.py index 8ed8120996..70ec08a204 100644 --- a/rest_framework/serializers.py +++ b/rest_framework/serializers.py @@ -1132,7 +1132,7 @@ def build_relational_field(self, field_name, relation_info): to_field = field_kwargs.pop('to_field', None) if relation_info.reverse: - if to_field and not relation_info.related_model_field.related_field.primary_key: + if to_field and not relation_info.related_model_field.related_fields[0][1].primary_key: field_kwargs['slug_field'] = to_field field_class = self.serializer_related_to_field else: diff --git a/tests/test_model_serializer.py b/tests/test_model_serializer.py index 57e540e7a5..9154a28bbd 100644 --- a/tests/test_model_serializer.py +++ b/tests/test_model_serializer.py @@ -899,3 +899,38 @@ class Meta: serializer = TestSerializer() assert len(serializer.fields['decimal_field'].validators) == 2 + + +class Issue3674Test(TestCase): + def test_nonPK_foreignkey_model_serializer(self): + class TestParentModel(models.Model): + title = models.CharField(max_length=64) + + class TestChildModel(models.Model): + parent = models.ForeignKey(TestParentModel, related_name='children') + value = models.CharField(primary_key=True, max_length=64) + + class TestChildModelSerializer(serializers.ModelSerializer): + class Meta: + model = TestChildModel + fields = ('value', 'parent') + + class TestParentModelSerializer(serializers.ModelSerializer): + class Meta: + model = TestParentModel + fields = ('id', 'title', 'children') + + parent_expected = dedent(""" + TestParentModelSerializer(): + id = IntegerField(label='ID', read_only=True) + title = CharField(max_length=64) + children = PrimaryKeyRelatedField(many=True, queryset=TestChildModel.objects.all()) + """) + self.assertEqual(unicode_repr(TestParentModelSerializer()), parent_expected) + + child_expected = dedent(""" + TestChildModelSerializer(): + value = CharField(max_length=64, validators=[]) + parent = PrimaryKeyRelatedField(queryset=TestParentModel.objects.all()) + """) + self.assertEqual(unicode_repr(TestChildModelSerializer()), child_expected) From 15ec87ce7be19b206138f1080c0fa76cf050e5d8 Mon Sep 17 00:00:00 2001 From: Xavier Ordoquy Date: Wed, 20 Jan 2016 18:45:38 +0100 Subject: [PATCH 4/5] Add a non ID PK test (thanks to @benred42) --- tests/test_model_serializer.py | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/tests/test_model_serializer.py b/tests/test_model_serializer.py index f58ba12a15..1b920bfa52 100644 --- a/tests/test_model_serializer.py +++ b/tests/test_model_serializer.py @@ -91,6 +91,15 @@ class ChoicesModel(models.Model): choices_field_with_nonstandard_args = models.DecimalField(max_digits=3, decimal_places=1, choices=DECIMAL_CHOICES, verbose_name='A label') +class ParentModel(models.Model): + title = models.CharField(max_length=64) + + +class ChildModel(models.Model): + parent = models.ForeignKey(ParentModel, related_name='children') + value = models.CharField(primary_key=True, max_length=64) + + class TestModelSerializer(TestCase): def test_create_method(self): class TestSerializer(serializers.ModelSerializer): @@ -975,3 +984,27 @@ class Meta: parent = PrimaryKeyRelatedField(queryset=TestParentModel.objects.all()) """) self.assertEqual(unicode_repr(TestChildModelSerializer()), child_expected) + + def test_nonID_PK_foreignkey_model_serializer(self): + + class TestChildModelSerializer(serializers.ModelSerializer): + class Meta: + model = ChildModel + fields = ('value', 'parent') + + class TestParentModelSerializer(serializers.ModelSerializer): + class Meta: + model = ParentModel + fields = ('id', 'title', 'children') + + parent = ParentModel.objects.create(title='abc') + child = ChildModel.objects.create(value='def', parent=parent) + + parent_serializer = TestParentModelSerializer(parent) + child_serializer = TestChildModelSerializer(child) + + parent_expected = {u'children': [u'def'], u'id': 1, u'title': u'abc'} + self.assertEqual(parent_serializer.data, parent_expected) + + child_expected = {u'parent': 1, u'value': u'def'} + self.assertEqual(child_serializer.data, child_expected) From e670284f07302631de52c0ce5bc379d087d8ea71 Mon Sep 17 00:00:00 2001 From: Xavier Ordoquy Date: Thu, 21 Jan 2016 12:49:10 +0100 Subject: [PATCH 5/5] Add an explicit name to the models. Otherwise we'll get a conflict between tests.test_model_serializer.ParentModel and tests.test_multitable_inheritance.ParentModel. --- tests/test_model_serializer.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/test_model_serializer.py b/tests/test_model_serializer.py index 1b920bfa52..dbefe0636f 100644 --- a/tests/test_model_serializer.py +++ b/tests/test_model_serializer.py @@ -91,12 +91,12 @@ class ChoicesModel(models.Model): choices_field_with_nonstandard_args = models.DecimalField(max_digits=3, decimal_places=1, choices=DECIMAL_CHOICES, verbose_name='A label') -class ParentModel(models.Model): +class Issue3674ParentModel(models.Model): title = models.CharField(max_length=64) -class ChildModel(models.Model): - parent = models.ForeignKey(ParentModel, related_name='children') +class Issue3674ChildModel(models.Model): + parent = models.ForeignKey(Issue3674ParentModel, related_name='children') value = models.CharField(primary_key=True, max_length=64) @@ -989,22 +989,22 @@ def test_nonID_PK_foreignkey_model_serializer(self): class TestChildModelSerializer(serializers.ModelSerializer): class Meta: - model = ChildModel + model = Issue3674ChildModel fields = ('value', 'parent') class TestParentModelSerializer(serializers.ModelSerializer): class Meta: - model = ParentModel + model = Issue3674ParentModel fields = ('id', 'title', 'children') - parent = ParentModel.objects.create(title='abc') - child = ChildModel.objects.create(value='def', parent=parent) + parent = Issue3674ParentModel.objects.create(title='abc') + child = Issue3674ChildModel.objects.create(value='def', parent=parent) parent_serializer = TestParentModelSerializer(parent) child_serializer = TestChildModelSerializer(child) - parent_expected = {u'children': [u'def'], u'id': 1, u'title': u'abc'} + parent_expected = {'children': ['def'], 'id': 1, 'title': 'abc'} self.assertEqual(parent_serializer.data, parent_expected) - child_expected = {u'parent': 1, u'value': u'def'} + child_expected = {'parent': 1, 'value': 'def'} self.assertEqual(child_serializer.data, child_expected)