From ce98c3d92179f65868f7c900867bc24140501f8f Mon Sep 17 00:00:00 2001 From: bphillips Date: Wed, 2 Dec 2015 13:37:23 -0500 Subject: [PATCH 1/3] 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 f151c6f36..c2afd9691 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/3] 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 99d36a8a5..8ed812099 100644 --- a/rest_framework/serializers.py +++ b/rest_framework/serializers.py @@ -1131,9 +1131,14 @@ class ModelSerializer(Serializer): 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 1d8cb22f2..8642ccf55 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 c2afd9691..c496d2711 100644 --- a/rest_framework/utils/model_meta.py +++ b/rest_framework/utils/model_meta.py @@ -29,9 +29,11 @@ FieldInfo = namedtuple('FieldResult', [ 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/3] 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 8ed812099..70ec08a20 100644 --- a/rest_framework/serializers.py +++ b/rest_framework/serializers.py @@ -1132,7 +1132,7 @@ class ModelSerializer(Serializer): 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 57e540e7a..9154a28bb 100644 --- a/tests/test_model_serializer.py +++ b/tests/test_model_serializer.py @@ -899,3 +899,38 @@ class TestDecimalFieldMappings(TestCase): 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)