From d892953eccd6499fee7a3bdf4a12ec443c859aa0 Mon Sep 17 00:00:00 2001 From: Joseba Mendivil Date: Wed, 22 Jan 2020 10:03:02 +0000 Subject: [PATCH 1/7] Pass on instance when using polymorphic serializers fixes #759 --- rest_framework_json_api/serializers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rest_framework_json_api/serializers.py b/rest_framework_json_api/serializers.py index 5f7a31e8..56688bd9 100644 --- a/rest_framework_json_api/serializers.py +++ b/rest_framework_json_api/serializers.py @@ -352,5 +352,5 @@ def to_internal_value(self, data): expected_types=', '.join(expected_types), received_type=received_type)) serializer_class = self.get_polymorphic_serializer_for_type(received_type) self.__class__ = serializer_class - return serializer_class(data, context=self.context, + return serializer_class(self.instance, data, context=self.context, partial=self.partial).to_internal_value(data) From f90a34c2315a3b8a1517f852ad0d43f05f92c271 Mon Sep 17 00:00:00 2001 From: Joseba Mendivil Date: Thu, 23 Jan 2020 17:23:34 +0000 Subject: [PATCH 2/7] Add test to ensure that `PolymorphicModelSerializer` is passing the instance to the child serializer --- example/tests/test_serializers.py | 45 ++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/example/tests/test_serializers.py b/example/tests/test_serializers.py index e1296e2f..02ed4d8a 100644 --- a/example/tests/test_serializers.py +++ b/example/tests/test_serializers.py @@ -7,6 +7,7 @@ from rest_framework.request import Request from rest_framework.test import APIRequestFactory +from example.factories import ArtProjectFactory from rest_framework_json_api.serializers import ( DateField, ModelSerializer, @@ -15,7 +16,11 @@ from rest_framework_json_api.utils import format_resource_type from example.models import Author, Blog, Entry -from example.serializers import BlogSerializer +from example.serializers import ( + BlogSerializer, + ProjectSerializer, + ArtProjectSerializer, +) request_factory = APIRequestFactory() pytestmark = pytest.mark.django_db @@ -193,3 +198,41 @@ def test_model_serializer_with_implicit_fields(self, comment, client): assert response.status_code == 200 assert expected == response.json() + + +class TestPolymorphicModelSerializer(TestCase): + def setUp(self): + self.project = ArtProjectFactory.create() + + def test_polymorphic_model_serializer_passes_instance_to_child(self): + """ + Ensure that `PolymorphicModelSerializer` is passing the instance to the + child serializer when initializing it in `to_internal_value` + """ + # Arrange + def to_internal_value(serializer_self, data): + """ + Override `ArtProjectSerializer.to_internal_value` to get the + instance serializer, which is later used assertion + """ + self.serializer_instance = serializer_self.instance + return super(ArtProjectSerializer, + serializer_self).to_internal_value(data) + + # Override `to_internal_value` with our own method + ArtProjectSerializer.to_internal_value = to_internal_value + + # Initialize a serializer that would partially update a model instance + data = {"artist": "Mark Bishop", "type": "artProjects"} + serializer = ProjectSerializer( + instance=self.project, data=data, partial=True + ) + serializer.is_valid(raise_exception=True) + + self.serializer_instance = None + + # Act + serializer.save() + + # Assert + assert self.serializer_instance is not None From a5f956a3e46f749e79333c8dc83eb55f1d3ab28b Mon Sep 17 00:00:00 2001 From: Joseba Mendivil Date: Thu, 23 Jan 2020 17:33:01 +0000 Subject: [PATCH 3/7] Override `save` instead of `to_internal_value` Previously it was relying on the fact that `save` was later calling `to_internal_value`, which was then passing the instance to the children. Now if in the future `PolymorphicModelSerializer` initialises the children serializers at a different point, this test will remain relevant --- example/tests/test_serializers.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/example/tests/test_serializers.py b/example/tests/test_serializers.py index 02ed4d8a..e7b4522c 100644 --- a/example/tests/test_serializers.py +++ b/example/tests/test_serializers.py @@ -207,20 +207,21 @@ def setUp(self): def test_polymorphic_model_serializer_passes_instance_to_child(self): """ Ensure that `PolymorphicModelSerializer` is passing the instance to the - child serializer when initializing it in `to_internal_value` + child serializer when initializing them """ # Arrange - def to_internal_value(serializer_self, data): + self.serializer_instance = None + + def save(serializer_self): """ - Override `ArtProjectSerializer.to_internal_value` to get the - instance serializer, which is later used assertion + Override `ArtProjectSerializer.save` to get the instance serializer, + which is later used assertion """ self.serializer_instance = serializer_self.instance - return super(ArtProjectSerializer, - serializer_self).to_internal_value(data) + return super(ArtProjectSerializer, serializer_self).save() - # Override `to_internal_value` with our own method - ArtProjectSerializer.to_internal_value = to_internal_value + # Override `save` with our own method + ArtProjectSerializer.save = save # Initialize a serializer that would partially update a model instance data = {"artist": "Mark Bishop", "type": "artProjects"} @@ -229,8 +230,6 @@ def to_internal_value(serializer_self, data): ) serializer.is_valid(raise_exception=True) - self.serializer_instance = None - # Act serializer.save() From 3532bbb05477be2d03c43efbedea4892ff39ddc7 Mon Sep 17 00:00:00 2001 From: Joseba Mendivil Date: Mon, 27 Jan 2020 09:41:13 +0000 Subject: [PATCH 4/7] Assert that the init receives the expected arguments Make the test more independent for the current DRF Serializer implementation by overriding the `__init__` and asserting that it receives the expected arguments. Also move the asserts to overridden init to avoid using extra unneeded variables --- example/tests/test_serializers.py | 45 +++++++++++++++++-------------- 1 file changed, 25 insertions(+), 20 deletions(-) diff --git a/example/tests/test_serializers.py b/example/tests/test_serializers.py index e7b4522c..9d37af39 100644 --- a/example/tests/test_serializers.py +++ b/example/tests/test_serializers.py @@ -11,7 +11,8 @@ from rest_framework_json_api.serializers import ( DateField, ModelSerializer, - ResourceIdentifierObjectSerializer + ResourceIdentifierObjectSerializer, + empty, ) from rest_framework_json_api.utils import format_resource_type @@ -209,29 +210,33 @@ def test_polymorphic_model_serializer_passes_instance_to_child(self): Ensure that `PolymorphicModelSerializer` is passing the instance to the child serializer when initializing them """ - # Arrange - self.serializer_instance = None + # Initialize a serializer that would partially update a model instance + initial_data = {"artist": "Mark Bishop", "type": "artProjects"} + parent_serializer = ProjectSerializer( + instance=self.project, data=initial_data, partial=True + ) - def save(serializer_self): + # Override `__init__` with our own method + def init_with_asserts(child_self, instance=None, data=empty, **kwargs): """ - Override `ArtProjectSerializer.save` to get the instance serializer, - which is later used assertion + Override `ArtProjectSerializer.__init__` with the same signature that + `BaseSerializer.__init__` has to assert that it receives the parameters + that `BaseSerializer` expects """ - self.serializer_instance = serializer_self.instance - return super(ArtProjectSerializer, serializer_self).save() + assert instance == self.project + assert data == initial_data + assert kwargs.get("partial", False) == parent_serializer.partial + assert kwargs.get("context", {}) == parent_serializer.context + + return super(ArtProjectSerializer, child_self).__init__( + instance, data, **kwargs + ) - # Override `save` with our own method - ArtProjectSerializer.save = save + ArtProjectSerializer.__init__ = init_with_asserts - # Initialize a serializer that would partially update a model instance - data = {"artist": "Mark Bishop", "type": "artProjects"} - serializer = ProjectSerializer( - instance=self.project, data=data, partial=True - ) - serializer.is_valid(raise_exception=True) + parent_serializer.is_valid(raise_exception=True) - # Act - serializer.save() + # Run save to force `ProjectSerializer` to init `ArtProjectSerializer` + parent_serializer.save() - # Assert - assert self.serializer_instance is not None + # Asserts in the overridden `__init__` method declared above From 0ef5688c45c2580d8b34d1b97403d1da5bfb9fbd Mon Sep 17 00:00:00 2001 From: Joseba Mendivil Date: Mon, 27 Jan 2020 09:46:53 +0000 Subject: [PATCH 5/7] Add polymorphic child serializer fix to CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e9b41f3b..1318b9ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ any parts of the framework not mentioned in the documentation should generally b * Ensure that `409 Conflict` is returned when processing a `PATCH` request in which the resource object’s type and id do not match the server’s endpoint properly as outlined in [JSON:API](https://jsonapi.org/format/#crud-updating-responses-409) spec. * Properly return parser error when primary data is of invalid type +* Pass instance to child serializer when `PolymorphicModelSerializer` inits it in `to_internal_value` ## [3.0.0] - 2019-10-14 From 9b14c604358a8c9a3b674dc537b598759dd08793 Mon Sep 17 00:00:00 2001 From: Joseba Mendivil Date: Mon, 27 Jan 2020 09:56:50 +0000 Subject: [PATCH 6/7] Restore original init at the end of test --- example/tests/test_serializers.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/example/tests/test_serializers.py b/example/tests/test_serializers.py index 9d37af39..7c5e0ce8 100644 --- a/example/tests/test_serializers.py +++ b/example/tests/test_serializers.py @@ -232,6 +232,7 @@ def init_with_asserts(child_self, instance=None, data=empty, **kwargs): instance, data, **kwargs ) + original_init = ArtProjectSerializer.__init__ ArtProjectSerializer.__init__ = init_with_asserts parent_serializer.is_valid(raise_exception=True) @@ -240,3 +241,6 @@ def init_with_asserts(child_self, instance=None, data=empty, **kwargs): parent_serializer.save() # Asserts in the overridden `__init__` method declared above + + # Restore original init to avoid affecting other tests + ArtProjectSerializer.__init__ = original_init From e3265dee17df23377be67d709c9e2fc420a37f0c Mon Sep 17 00:00:00 2001 From: Joseba Mendivil Date: Thu, 30 Jan 2020 10:55:02 +0000 Subject: [PATCH 7/7] Override the init in setUp and tearDown The previous approach to restore the overridden init was not running if the test failed, so it was only restoring the init when the test passed. By setting it and restoring it during the setUp and tearDown of the test we ensure that it will always be restored. Also because the overridden init is now declared in the setUp, I have moved the asserts back to the test for readability. --- example/tests/test_serializers.py | 47 ++++++++++++++++--------------- 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/example/tests/test_serializers.py b/example/tests/test_serializers.py index 7c5e0ce8..50a84f4d 100644 --- a/example/tests/test_serializers.py +++ b/example/tests/test_serializers.py @@ -204,43 +204,46 @@ def test_model_serializer_with_implicit_fields(self, comment, client): class TestPolymorphicModelSerializer(TestCase): def setUp(self): self.project = ArtProjectFactory.create() - - def test_polymorphic_model_serializer_passes_instance_to_child(self): - """ - Ensure that `PolymorphicModelSerializer` is passing the instance to the - child serializer when initializing them - """ - # Initialize a serializer that would partially update a model instance - initial_data = {"artist": "Mark Bishop", "type": "artProjects"} - parent_serializer = ProjectSerializer( - instance=self.project, data=initial_data, partial=True - ) + self.child_init_args = {} # Override `__init__` with our own method - def init_with_asserts(child_self, instance=None, data=empty, **kwargs): + def overridden_init(child_self, instance=None, data=empty, **kwargs): """ Override `ArtProjectSerializer.__init__` with the same signature that `BaseSerializer.__init__` has to assert that it receives the parameters that `BaseSerializer` expects """ - assert instance == self.project - assert data == initial_data - assert kwargs.get("partial", False) == parent_serializer.partial - assert kwargs.get("context", {}) == parent_serializer.context + self.child_init_args = dict(instance=instance, data=data, **kwargs) return super(ArtProjectSerializer, child_self).__init__( instance, data, **kwargs ) - original_init = ArtProjectSerializer.__init__ - ArtProjectSerializer.__init__ = init_with_asserts + self.child_serializer_init = ArtProjectSerializer.__init__ + ArtProjectSerializer.__init__ = overridden_init + + def tearDown(self): + # Restore original init to avoid affecting other tests + ArtProjectSerializer.__init__ = self.child_serializer_init + + def test_polymorphic_model_serializer_passes_instance_to_child(self): + """ + Ensure that `PolymorphicModelSerializer` is passing the instance to the + child serializer when initializing them + """ + # Initialize a serializer that would partially update a model instance + initial_data = {"artist": "Mark Bishop", "type": "artProjects"} + parent_serializer = ProjectSerializer( + instance=self.project, data=initial_data, partial=True + ) parent_serializer.is_valid(raise_exception=True) # Run save to force `ProjectSerializer` to init `ArtProjectSerializer` parent_serializer.save() - # Asserts in the overridden `__init__` method declared above - - # Restore original init to avoid affecting other tests - ArtProjectSerializer.__init__ = original_init + # Assert that child init received the expected arguments + assert self.child_init_args["instance"] == self.project + assert self.child_init_args["data"] == initial_data + assert self.child_init_args["partial"] == parent_serializer.partial + assert self.child_init_args["context"] == parent_serializer.context