Allow users to overwrite get_serializer_class while using related urls
#860
Conversation
An attempt to fix django-json-api#859, which unfortunately breaks two tests in test_views.
Tha basic method was copied over from the GenericAPIView. The comment here does not fit, though.
Ensure that the correct resource is returned during rendering, which in turn requires to use the correct serializer, depending on if it is a related resource or the parent resource. sliverc pointed out the issue here.
As pointed out by n2ygk, the schema names here must reflect the Serializer class names, which I changed in the two affected test cases.
Codecov Report
@@ Coverage Diff @@
## master #860 +/- ##
=======================================
Coverage 97.74% 97.74%
=======================================
Files 60 60
Lines 3319 3319
=======================================
Hits 3244 3244
Misses 75 75
Continue to review full report at Codecov.
|
Clarified changelog entry
sliverc
changed the title
get_serializer_class while using related urls
There was a problem hiding this comment.
@uliSchuster Thanks for working on this. There are some linting issue which need to be fixed (run flake8 or see CI). I have adjusted the changelog to clarify when this issue is encountered.
I am not too happy about changing an existing test for this but I know tests are currently messy. So I think this is fine for now and can be cleaned up with #468
@n2ygk As you have already started reviewing it I leave you with this to give the final call in terms of approval.
|
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. |
Prior to pushing your commits, you can use tox -e lint locally to perform a lint test. See also #857 regarding converting the codebase to use black, adding pre-commit hooks, etc. Also use
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.
(Hmm, this should all get added to CONTRIBUTING.md I suppose: #861) |
| return Response(serializer.data) | ||
|
|
||
| def get_serializer_class(self): |
There was a problem hiding this comment.
removing this exposes the upstream DRF get_serializer_class(). It seems this needs to remain for compatibility. @sliverc Do you agree?
There was a problem hiding this comment.
My whole point in making the change here is to do exactly this: expose the upstream method, so that it can be overwritten. get_serializer_class() is a public method on the base class. As such, it is part of DRF's public API - the DRF documentation explicitly mentions that the method can be overwritten. In my opinion, an extension library should preserve the upstream API.
The current version of DJA does break the path to the upstream method. There might be cases where DJA users rely on this bug in some way. The change here would break their code. I'm just a first-time user of DJA without any idea about how it is being used in other projects, what the effects might be, and who might be affected.
There was a problem hiding this comment.
Forgive me for being dense but how is get_serializer_class() in DJA not able to be overridden by you? You've completely removed the function from DJA; not just added get_related_serializer().
This extension library routinely overrides the upstream DRF because it is extending it.
There was a problem hiding this comment.
I do Full understand that you don't want to run the risk of breaking someone's application. And given that I am fairly new to DJA, I might not fully understand its inner workings, either.
I came across the issue the present PR is trying to solve, when I had to overwrite get_serializer_class() on a custom viewset, in exactly the same way I now added as part of the AuthorViewSet in the test example. I investigated the issue and discovered that when processing a GET request for a related instance, RelatedMixin.get_related_instance() directly accessed the self.serializer_class field instead of getting the class via its accessor method. Consequently, the upstream get_serializer_class() would not be called, either, which would be my overwritten version. Hence my remark that DJA effectively prevents get_serializer_class() to be overwritten on a custom viewset, which I consider to be a bug.
My first attempt to fix the issue was to have RelatedMixin.get_related_instance call the upstream method instead of accessing the field directly. But this breaks things badly. During two exchanges with @sliverc (see the comments on the issue here) showed that the functionality of RelatedMixin.get_serializer_class is actually not about getting the serializer class, but getting the class of related serializers. I followed the hint of @sliverc to rename the method from get_serializer_class() to get_related_serializer_class(), which better captures its functionality. This way, RelatedMixin.get_related_instance can call get_serializer_class() of the parent class (potentially overwritten), while all functionality that has to do with getting serializers for related views is now handled by aptly named get_related_* methods.
To make this work, I need to do some other adjustments along the call chain. In particular, retrieve_related(), which is the entry point for a GET request on a related resource, now calls the renamed get_related_serializer_class(). One more change, also pointed out by @sliverc, was in utils.get_resource_name(): Here, I had to make the distinction if the serializer of the present viewset is needed, or if the serializer of a related viewset should be returned. The distinction between the two cases depends on the related_field kwarg, similar to the condition in get_serializer_class()(not get_related_serializer_class().
I hope this helps to provide some understanding of what I am trying to accomplish. I was very much focussed on the functionality of getting related resources; because of my limited understanding of DJA, I am not able to tell if other parts of its functionality are affected by the changes introduced in RelatedMixin. Here, I relied on the tests, which are all passing. If there is something else I can do to clear things up or to provide better explanation, please let me know. I am also available for a quick online call, if that can help to better assess the changes I introduced.
There was a problem hiding this comment.
Let me try to shed some light on this... RelatedMixin mistakenly shadowed get_serializer_class to get related serializer. Shadowing happened as RelatedMixin only works when used with a DRF generic view which provides a get_serializer_class method
For RelatedMixin to be able to get parent serializer it then simply used self.serializer_class which is not DRF conform. However this only happened when related urls have been configured as otherwise this if is always false and there is no difference to the method not being overwritten in RelatedMixin.
If someone overwrote get_serializer_class in its view without using related urls it would just work as RelatedMixin did not do anything anyway in such a case then passing it on to super. If someone tried to overwrite get_serializer_class with related urls this user would run into exactly the same problem as @uliSchuster has.
Therefore I don't see that this change affects existing users.
There was a problem hiding this comment.
This looks good now and is in my opinion safe to be merged.
It actually follows our second goal to be compatible with DRF as much as possible.
@n2ygk I hope my inline explanations make sense and I suggest we merge this. Otherwise let me know.
| return Response(serializer.data) | ||
|
|
||
| def get_serializer_class(self): |
There was a problem hiding this comment.
Let me try to shed some light on this... RelatedMixin mistakenly shadowed get_serializer_class to get related serializer. Shadowing happened as RelatedMixin only works when used with a DRF generic view which provides a get_serializer_class method
For RelatedMixin to be able to get parent serializer it then simply used self.serializer_class which is not DRF conform. However this only happened when related urls have been configured as otherwise this if is always false and there is no difference to the method not being overwritten in RelatedMixin.
If someone overwrote get_serializer_class in its view without using related urls it would just work as RelatedMixin did not do anything anyway in such a case then passing it on to super. If someone tried to overwrite get_serializer_class with related urls this user would run into exactly the same problem as @uliSchuster has.
Therefore I don't see that this change affects existing users.
|
Thanks for your contribution @uliSchuster and apologies for not understanding it. Thanks as usual @sliverc for explaining. |
|
Thanks very much! Are you planning on a release that includes this change anytime soon? |
Fixes #859
Description of the Change
Allows users to overwrite the
get_serializer()andget_serializer_class()methods in subclassed views that have related or included serializers.This change seems to interfere with OpenAPI Schema generation. As it stands, the tests
test_path_with_id_parameterandtest_path_without_parametersare failing, because the result no longer matches the expected snapshot. I suspect that the snapshots were incorrect in the first place, but I am not able to tell for sure.Checklist
CHANGELOG.mdupdated (only for user relevant changes)AUTHORS