Repository: django-json-api/django-rest-framework-json-api

Issue:
Allow overwriting of get_serializer_class with related urls
In https://github.com/django-json-api/django-rest-framework-json-api/blob/a67a521327c9ec62a18a875abf13504a72d8eaf0/rest_framework_json_api/views.py#L182
the serializer class defined in the view is used directly. This is a problem if one wants to override `get_serializer_class()`; e.g., to provide a different serializer depending on the view action. Is there a reason to prevent dynamic serializer selection here via `get_serializer_class()`?

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/django-json-api/django-rest-framework-json-api/pull/860?src=pr&el=h1) Report
> Merging [#860](https://codecov.io/gh/django-json-api/django-rest-framework-json-api/pull/860?src=pr&el=desc) (67d79ff) into [master](https://codecov.io/gh/django-json-api/django-rest-framework-json-api/commit/c392a439510ef05e6b9f9554b59cb3577fd86610?el=desc) (c392a43) will **decrease** coverage by `0.02%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/django-json-api/django-rest-framework-json-api/pull/860/graphs/tree.svg?width=650&height=150&src=pr&token=kJ4T4hcVh8)](https://codecov.io/gh/django-json-api/django-rest-framework-json-api/pull/860?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #860      +/-   ##
==========================================
- Coverage   97.74%   97.71%   -0.03%     
==========================================
  Files          60       60              
  Lines        3319     3276      -43     
==========================================
- Hits         3244     3201      -43     
  Misses         75       75              
```


| [Impacted Files](https://codecov.io/gh/django-json-api/django-rest-framework-json-api/pull/860?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [example/tests/snapshots/snap\_test\_openapi.py](https://codecov.io/gh/django-json-api/django-rest-framework-json-api/pull/860/diff?src=pr&el=tree#diff-ZXhhbXBsZS90ZXN0cy9zbmFwc2hvdHMvc25hcF90ZXN0X29wZW5hcGkucHk=) | `100.00% <ø> (ø)` | |
| [example/tests/test\_views.py](https://codecov.io/gh/django-json-api/django-rest-framework-json-api/pull/860/diff?src=pr&el=tree#diff-ZXhhbXBsZS90ZXN0cy90ZXN0X3ZpZXdzLnB5) | `100.00% <100.00%> (ø)` | |
| [rest\_framework\_json\_api/renderers.py](https://codecov.io/gh/django-json-api/django-rest-framework-json-api/pull/860/diff?src=pr&el=tree#diff-cmVzdF9mcmFtZXdvcmtfanNvbl9hcGkvcmVuZGVyZXJzLnB5) | `90.00% <0.00%> (-0.19%)` | :arrow_down: |
| [rest\_framework\_json\_api/relations.py](https://codecov.io/gh/django-json-api/django-rest-framework-json-api/pull/860/diff?src=pr&el=tree#diff-cmVzdF9mcmFtZXdvcmtfanNvbl9hcGkvcmVsYXRpb25zLnB5) | `88.29% <0.00%> (-0.12%)` | :arrow_down: |
| [rest\_framework\_json\_api/parsers.py](https://codecov.io/gh/django-json-api/django-rest-framework-json-api/pull/860/diff?src=pr&el=tree#diff-cmVzdF9mcmFtZXdvcmtfanNvbl9hcGkvcGFyc2Vycy5weQ==) | `97.33% <0.00%> (-0.11%)` | :arrow_down: |
| [tests/conftest.py](https://codecov.io/gh/django-json-api/django-rest-framework-json-api/pull/860/diff?src=pr&el=tree#diff-dGVzdHMvY29uZnRlc3QucHk=) | `100.00% <0.00%> (ø)` | |
| [example/factories.py](https://codecov.io/gh/django-json-api/django-rest-framework-json-api/pull/860/diff?src=pr&el=tree#diff-ZXhhbXBsZS9mYWN0b3JpZXMucHk=) | `100.00% <0.00%> (ø)` | |
| [example/tests/test\_errors.py](https://codecov.io/gh/django-json-api/django-rest-framework-json-api/pull/860/diff?src=pr&el=tree#diff-ZXhhbXBsZS90ZXN0cy90ZXN0X2Vycm9ycy5weQ==) | `100.00% <0.00%> (ø)` | |
| [example/tests/test\_openapi.py](https://codecov.io/gh/django-json-api/django-rest-framework-json-api/pull/860/diff?src=pr&el=tree#diff-ZXhhbXBsZS90ZXN0cy90ZXN0X29wZW5hcGkucHk=) | `100.00% <0.00%> (ø)` | |
| ... and [10 more](https://codecov.io/gh/django-json-api/django-rest-framework-json-api/pull/860/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/django-json-api/django-rest-framework-json-api/pull/860?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/django-json-api/django-rest-framework-json-api/pull/860?src=pr&el=footer). Last update [c392a43...67d79ff](https://codecov.io/gh/django-json-api/django-rest-framework-json-api/pull/860?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

uliSchuster: I wasn't quite sure which linter you are using and didn't want to break other things (I'm running black and prospector on my project. But I'm happy to fix all remaining linting issues.

About the tests: As I understood from the source, the tests rely on a single example application. The data model, views and serializers of this application effectively are the entities under test. The test cases present already do cover the methods where the issue arises if the entities under test would have included the special case at hand. Writing additional tests alone would not have shown the issue without additional serializers, so I added these serializers.

The alternative would be to add a model with its own views and serializers, and then write tests that use these new classes. I think this would habe been lots of additional code without that much additional benefit.
n2ygk: @uliSchuster:
> I wasn't quite sure which linter you are using and didn't want to break other things (I'm running black and prospector on my project. But I'm happy to fix all remaining linting issues.

Prior to pushing your commits, you can use [tox -e lint](https://github.com/django-json-api/django-rest-framework-json-api/blob/master/tox.ini#L23-L29) locally to perform a lint test. See also #857 regarding converting the codebase to use black, adding pre-commit hooks, etc. 

Also use `tox -e docs` to locally generate the documentation as html in `docs/_build/html/index.html`.

> About the tests: As I understood from the source, the tests rely on a single example application. The data model, views and serializers of this application effectively are the entities under test. The test cases present already do cover the methods where the issue arises if the entities under test would have included the special case at hand. Writing additional tests alone would not have shown the issue without additional serializers, so I added these serializers.

Per #855 and #856 @sliverc has recently begun converting tests away from using the example app and to consistently use pytest. So, for now, doing them in the example is the right way -- which is why it is so ugly.

`tox` will run all the tests against the matrix of python, Django, DRF and DJA versions.

(Hmm, this should all get added to [CONTRIBUTING.md](https://github.com/django-json-api/django-rest-framework-json-api/blob/master/docs/CONTRIBUTING.md) I suppose: #861)

n2ygk: Thanks for your contribution @uliSchuster and apologies for not understanding it. Thanks as usual @sliverc for explaining.

Patch:
diff --git a/AUTHORS b/AUTHORS
index b440df8..d4c03b2 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -33,4 +33,5 @@ Sergey Kolomenkin <https://kolomenkin.com>
 Stas S. <stas@nerd.ro>
 Tim Selman <timcbaoth@gmail.com>
 Tom Glowka <glowka.tom@gmail.com>
+Ulrich Schuster <ulrich.schuster@mailworks.org>
 Yaniv Peer <yanivpeer@gmail.com>
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 12b3e80..4613aa1 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -14,6 +14,10 @@ any parts of the framework not mentioned in the documentation should generally b
 
 * Ability for the user to select `included_serializers` to apply when using `BrowsableAPI`, based on available `included_serializers` defined for the current endpoint.
 
+### Fixed
+
+* Allow users to overwrite a view's `get_serializer_class()` method when using [related urls](https://django-rest-framework-json-api.readthedocs.io/en/stable/usage.html#related-urls)
+
 
 ## [4.0.0] - 2020-10-31
 
diff --git a/example/serializers.py b/example/serializers.py
index 9dc84a4..566f39d 100644
--- a/example/serializers.py
+++ b/example/serializers.py
@@ -261,6 +261,14 @@ class AuthorSerializer(serializers.ModelSerializer):
         return obj.entries.first()
 
 
+class AuthorListSerializer(AuthorSerializer):
+    pass
+
+
+class AuthorDetailSerializer(AuthorSerializer):
+    pass
+
+
 class WriterSerializer(serializers.ModelSerializer):
     included_serializers = {
         'bio': AuthorBioSerializer
diff --git a/example/views.py b/example/views.py
index 8c80d14..99a5419 100644
--- a/example/views.py
+++ b/example/views.py
@@ -15,6 +15,8 @@ from rest_framework_json_api.views import ModelViewSet, RelationshipView
 
 from example.models import Author, Blog, Comment, Company, Entry, Project, ProjectType
 from example.serializers import (
+    AuthorDetailSerializer,
+    AuthorListSerializer,
     AuthorSerializer,
     BlogDRFSerializer,
     BlogSerializer,
@@ -185,7 +187,16 @@ class NoFiltersetEntryViewSet(EntryViewSet):
 
 class AuthorViewSet(ModelViewSet):
     queryset = Author.objects.all()
-    serializer_class = AuthorSerializer
+    serializer_classes = {
+        "list": AuthorListSerializer,
+        "retrieve": AuthorDetailSerializer}
+    serializer_class = AuthorSerializer  # fallback
+
+    def get_serializer_class(self):
+        try:
+            return self.serializer_classes.get(self.action, self.serializer_class)
+        except AttributeError:
+            return self.serializer_class
 
 
 class CommentViewSet(ModelViewSet):
diff --git a/rest_framework_json_api/utils.py b/rest_framework_json_api/utils.py
index 2443575..b99de91 100644
--- a/rest_framework_json_api/utils.py
+++ b/rest_framework_json_api/utils.py
@@ -52,7 +52,10 @@ def get_resource_name(context, expand_polymorphic_types=False):
         resource_name = getattr(view, 'resource_name')
     except AttributeError:
         try:
-            serializer = view.get_serializer_class()
+            if 'kwargs' in context and 'related_field' in context['kwargs']:
+                serializer = view.get_related_serializer_class()
+            else:
+                serializer = view.get_serializer_class()
             if expand_polymorphic_types and issubclass(serializer, PolymorphicModelSerializer):
                 return serializer.get_polymorphic_types()
             else:
diff --git a/rest_framework_json_api/views.py b/rest_framework_json_api/views.py
index 2e0b22e..7c874e7 100644
--- a/rest_framework_json_api/views.py
+++ b/rest_framework_json_api/views.py
@@ -144,10 +144,15 @@ class RelatedMixin(object):
         if isinstance(instance, Iterable):
             serializer_kwargs['many'] = True
 
-        serializer = self.get_serializer(instance, **serializer_kwargs)
+        serializer = self.get_related_serializer(instance, **serializer_kwargs)
         return Response(serializer.data)
 
-    def get_serializer_class(self):
+    def get_related_serializer(self, instance, **kwargs):
+        serializer_class = self.get_related_serializer_class()
+        kwargs.setdefault('context', self.get_serializer_context())
+        return serializer_class(instance, **kwargs)
+
+    def get_related_serializer_class(self):
         parent_serializer_class = super(RelatedMixin, self).get_serializer_class()
 
         if 'related_field' in self.kwargs:
@@ -179,7 +184,8 @@ class RelatedMixin(object):
 
     def get_related_instance(self):
         parent_obj = self.get_object()
-        parent_serializer = self.serializer_class(parent_obj)
+        parent_serializer_class = self.get_serializer_class()
+        parent_serializer = parent_serializer_class(parent_obj)
         field_name = self.get_related_field_name()
         field = parent_serializer.fields.get(field_name, None)
 


Test patch:
diff --git a/example/tests/snapshots/snap_test_openapi.py b/example/tests/snapshots/snap_test_openapi.py
index ec8da38..aca41d7 100644
--- a/example/tests/snapshots/snap_test_openapi.py
+++ b/example/tests/snapshots/snap_test_openapi.py
@@ -65,7 +65,7 @@ snapshots['test_path_without_parameters 1'] = '''{
             "properties": {
               "data": {
                 "items": {
-                  "$ref": "#/components/schemas/Author"
+                  "$ref": "#/components/schemas/AuthorList"
                 },
                 "type": "array"
               },
@@ -171,7 +171,7 @@ snapshots['test_path_with_id_parameter 1'] = '''{
           "schema": {
             "properties": {
               "data": {
-                "$ref": "#/components/schemas/Author"
+                "$ref": "#/components/schemas/AuthorDetail"
               },
               "included": {
                 "items": {
diff --git a/example/tests/test_views.py b/example/tests/test_views.py
index 9cd493f..25eeca8 100644
--- a/example/tests/test_views.py
+++ b/example/tests/test_views.py
@@ -367,16 +367,16 @@ class TestRelatedMixin(APITestCase):
         got = view.get_related_instance()
         self.assertEqual(got, self.author.id)
 
-    def test_get_serializer_class(self):
+    def test_get_related_serializer_class(self):
         kwargs = {'pk': self.author.id, 'related_field': 'bio'}
         view = self._get_view(kwargs)
-        got = view.get_serializer_class()
+        got = view.get_related_serializer_class()
         self.assertEqual(got, AuthorBioSerializer)
 
-    def test_get_serializer_class_many(self):
+    def test_get_related_serializer_class_many(self):
         kwargs = {'pk': self.author.id, 'related_field': 'entries'}
         view = self._get_view(kwargs)
-        got = view.get_serializer_class()
+        got = view.get_related_serializer_class()
         self.assertEqual(got, EntrySerializer)
 
     def test_get_serializer_comes_from_included_serializers(self):
@@ -384,15 +384,15 @@ class TestRelatedMixin(APITestCase):
         view = self._get_view(kwargs)
         related_serializers = view.serializer_class.related_serializers
         delattr(view.serializer_class, 'related_serializers')
-        got = view.get_serializer_class()
+        got = view.get_related_serializer_class()
         self.assertEqual(got, AuthorTypeSerializer)
 
         view.serializer_class.related_serializers = related_serializers
 
-    def test_get_serializer_class_raises_error(self):
+    def test_get_related_serializer_class_raises_error(self):
         kwargs = {'pk': self.author.id, 'related_field': 'unknown'}
         view = self._get_view(kwargs)
-        self.assertRaises(NotFound, view.get_serializer_class)
+        self.assertRaises(NotFound, view.get_related_serializer_class)
 
     def test_retrieve_related_single_reverse_lookup(self):
         url = reverse('author-related', kwargs={'pk': self.author.pk, 'related_field': 'bio'})