Allow overwriting of get_serializer_class with related urls

[uliSchuster]

In

django-rest-framework-json-api/rest_framework_json_api/views.py

Line 182 in a67a521

parent_serializer = self.serializer_class(parent_obj)

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()?

[sliverc]

This issue only seems to relate to related urls which need to be specially configured to enable. Otherwise it should not be a problem to overwrite get_serializer_class in any other case.

I think the issue is that RelatedMixin already overwrites get_serializer_class so serializer_class needs to be accessed here. It should be possible to rename RelatedMixin.get_serializer_class method to get_related_serializer_class and it should work (emphasis on should... 😄).

PR is welcome.

[uliSchuster]

If I understand you correctly, the actual bug is that RelatedMixin overwrites get_serializer_class() in the first place - this might already lead to unintended consequences for upstream users of a view that uses the mixin. Instead, the method should be called get_related_serializer_class(), because it does exactly what the name implies. It then would not shadow the actual get_serializer_class() method, which would continue to return the serializer class of the view proper, instead of serializers for related views. Is this understanding correct?

Then, a fix might lead to some upstream user code to break that is already designed around the bug.
But yes, I will try to fix the issue and provide a PR.

[uliSchuster]

Here is my attempted fix along the lines outlined above. Unfortunately, it does not seem to work, as it breaks two assertions in test_views.py::TestRelatedMixin::test_retrieve_related_single and test_views.py::TestRelatedMixin::test_retrieve_related_single_reverse_lookup
To separate related serializers from parent serializers, I also created a get_related_serializer() method. But this does not seem to be sufficient. By now, I am running out of ideas...

[uliSchuster]

After some more debugging, I didn't get any further unfortunately. It seems that retrieve_related returns the correct response - at least, it includes the correct serialized data. But then during response rendering, something goes wrong and the resource type gets marked as being the parent resource instead of the related resource.

[sliverc]

this might already lead to unintended consequences for upstream users of a view that uses the mixin.

To clarify, this particular issue only affects users who use related urls. Such are not on by default but need to be activated through a specific url pattern as outlined in the documentation. Without those urls this if statement won't be true and the get_serializer_class method simply calls super, as it would if it were not there.

I haven't analyzed the issue fully but I assume the issue lies here where the resource name is determined. Most likely does it need a similar if as here to determine whether it needs to get serializer_class from get_serializer_class or get_related_serializer_class.

[uliSchuster]

Thanks for this additional hint. Your assumption seems to be right. I added an if-statement to determine if the resource name of the related resource is needed or the parent resource. I extended the Author view in the example to test the issue - made sure that it fails on master first, then add my changes and see that the tests pass.

This is where another issue now pops up, in OpenAPI generation: test_path_without_parameters and test_path_with_id_parameter fail. I am somewhat reluctant to dig into all the OpenAPI code - do you have another hint for me were to look?

[sliverc]

I do not quite see how such a change makes those tests fail. If you don't find the issue feel free to create a PR with what you got.

[uliSchuster]

The problem seems to be in the snapshot tests, where the expected response does not match anymore. Instead of

                "items": {
                  "$ref": "#/components/schemas/Author"
                },

we now get

                "items": {
                  "$ref": "#/components/schemas/AuthorList"
                },

I suppose the new expected result is correct, but I can't tell for sure because I am not familiar with the OpenAPI schema generation.

I will open a PR of my changes, so you can judge for yourself. Thanks for your help!

[n2ygk]

See my review comments.