Support polymorphic models#211
Conversation
@leo-naeka your code looks nice, I don't currently have a use for this in my own apps so I'd welcome usability feedback from the community. Would it be better to set the polymorphic model in SETTINGS rather than detecting it in |
Well, this is a pertinent question. The arguments you raised seems not legitimate since The main advantage of the auto-detection approach is to support these polymorphic modules without requiring extra configuration. |
If I see this correctly the whole 'POLYMORPHIC_ANCESTORS' is for autodetection right? If one would explictly define a ResourceRelatedField relation, with is_polymorphic=True this would work too right? I believe this is a classic case of implicit vs explicit behaviour. Since we explicitly have to declare serializers for each model, I think it's natural practice to explicitly declare a relation if it has different behaviour. This also enables different behaviour when dealing with list vs detail api calls. I personally can imagine situations where you do not want to access entities in a polymorphic manner when performing list calls, only when retrieving the details for an entity. The is_polymorphic flag can even be renamed to something like resource_type_from_instance. |
Ok, ofcourse what i mentioned only applies to polymorphic relations, I see that 'POLYMORPHIC_ANCESTORS' is ofcourse also used by the renderer. I believe this can be made explicit as well. As the method build_json_resource_obj is called at line: 294, 312, 443, 458 and all those lines have access to a serializer instance. Thus it should be possible to access a 'resource_type_from_instance' attribute defined in the serializer. I'm willing to make a pull request if you guys want. |
stianpr commented Apr 15, 2016
Any progress on this? We need to use use relations with polymorphism our selves, but after a bit of testing this branch we noticed that it does not handle incoming data? It seems that the method I'm willing to put some effort into getting this landed. |
👍 on tests for the create(POST) cases. Definitely want those to get those in. In our app we use I do think the autodetection would break our current API, because some relations would be serialized to their real type and not their base type. That may be our issue, not an issue with this feature. It just really depends how people are building their API. |
stianpr commented Sep 8, 2016
We recently made a simple field for our code to make it work with django-polymorphic. I'm a big fan of a more explicit approach with this field than doing lots of magic in The fundamental reason for this to work is that we need to get the model name based on the object ID with the queryset. That will make the type verification work. fromrest_framework_json_api.exceptionsimportConflictfromrest_framework_json_api.relationsimportResourceRelatedFieldfromrest_framework_json_api.utilsimportget_resource_type_from_instanceclassPolymorphicRelatedField(ResourceRelatedField): """Polymorphic Related Field. A field to ignore failed type verification in `ResourceRelatedField.to_internal_value`. Rest Framework JSON API does not support Django-polymorphic yet. Its type verification is based on retrieving resource name from `queryset.model` on the field. In our case that will always return the base model name, which will fail for sub models. """defto_internal_value(self, data): """Override to ignore `Conflict` exception. Since the resource name is retrieved from `queryset.model` on the field, we make sure it's ignored. """try: returnsuper().to_internal_value(data) exceptConflict: obj=self.queryset.filter(id=data['id']).first() ifdata['type'] !=get_resource_type_from_instance(obj): raise# Luckily the Conflict exception is raised at the end so we can# explicitly call super on `ResourceRelatedField`, which is the# `PrimaryKeyRelatedField`.returnsuper(ResourceRelatedField, self).to_internal_value( data['id'])
|
Update documentation
- support for post and patch request on polymorphic model endpoints. - makes polymorphic serializers give child fields instead of its own.
Update gitignore
pytest-factoryboy does not support pytest3.0+
Codecov Report
@@ Coverage Diff @@## develop #211 +/- ## =========================================== + Coverage 77.06% 77.76% +0.69% =========================================== Files 50 52 +2 Lines 6027 6252 +225 =========================================== + Hits 4645 4862 +217 - Misses 1382 1390 +8
Continue to review full report at Codecov.
|
@stianpr I'm back on this PR. |
I use polymorphic and as stated in my previous comment I like opt-in. |
stianpr commented Sep 9, 2016
Hmm, I'm still not convinced of why we need the |
rest_framework_json_api/relations.py Outdated
| model = model or getattr(self.get_queryset(), 'model', None) | ||
| if model and issubclass(model, POLYMORPHIC_ANCESTORS): | ||
| self.is_polymorphic = True | ||
There was a problem hiding this comment.
Actually that block is useless since fddb06b
ntadej commented Jan 11, 2017
I tried rebasing this on develop to see if it is usable in a project I'm working on currently. There are some issues with fields selection for both list and item views. |
scottkidder commented Feb 10, 2017
Any update on where this is at? |
I was able to rebase this against develop without too much hassle. All drf-json-api tests are passing but I am seeing the same issue as ntadej in my tests.
|
The bulk of this conversation seems to have died off back in September of last year. Given that was 8 months ago and there are conflicts on this branch, I'd like to consider closing this PR unless it finds a champion who can bring it to closure. Is that ok? What needs to be done to bring this to a conclusion? |
| class Meta: | ||
| model = Company | ||
| if version.parse(rest_framework.VERSION) >= version.parse('3.3'): |
There was a problem hiding this comment.
Interesting... I had no idea it was possible to use an if statement in a class definition. I learned something new about Python today. Thanks!
| @@ -224,6 +225,43 @@ def get_choices(self, cutoff=None): | |||
| ]) | |||
| class PolymorphicResourceRelatedField(ResourceRelatedField): | |||
There was a problem hiding this comment.
Since this is one of the primary APIs that is exposed to support polymorphic models, I think it should have a docstring describing its function/purpose and any other things that a potential user needs to know.
| type_to_serializer = { | ||
| get_resource_type_from_serializer(serializer): serializer for | ||
| serializer in polymorphic_serializers} | ||
| setattr(new_class, '_poly_serializer_model_map', serializer_to_model) |
There was a problem hiding this comment.
Why is this (and the following lines) using setattr instead of new_class._poly_serializer_model_map = serializer_to_model?
There was a problem hiding this comment.
No reasons actually: first I was considering formatting the name depending on the serializer but it would have been pointless and overkill. Later, I used setattr for consistency...
| 'received {received_type}.'.format( | ||
| expected_types=', '.join(expected_types), received_type=received_type)) | ||
| serializer_class = self.get_polymorphic_serializer_for_type(received_type) | ||
| self.__class__ = serializer_class |
There was a problem hiding this comment.
This line seems odd to me. Why is the code changing its __class__ attribute?
There was a problem hiding this comment.
That's a trick to replace the current instance class (the polymorphic "parent") by the correct subclass without altering the dynamic data that may have been associated with the instance. It is necessary to get the correct defaults (such as subclass Meta, custom validators) for the rest of the process.
I'm not very proud of that too, I'll search how this case may have been handled by other drf libraries and I'm open to any suggestions.
rest_framework_json_api/utils.py Outdated
| @@ -76,7 +79,11 @@ def get_resource_name(context): | |||
| except AttributeError: | |||
| try: | |||
| serializer = view.get_serializer_class() | |||
| return get_resource_type_from_serializer(serializer) | |||
| if issubclass(serializer, PolymorphicModelSerializer) and \ | |||
There was a problem hiding this comment.
This is minor, but could we switch the order of these checks? It would be slightly more performant to check the boolean expand_polymorphic_types first and only do the subclass check if necessary since and will short circuit if the boolean is False.
rest_framework_json_api/utils.py Outdated
| @@ -236,15 +243,11 @@ def get_related_resource_type(relation): | |||
| relation_model = parent_model_relation.related.related_model | |||
| else: | |||
| relation_model = parent_model_relation.related.model | |||
| elif parent_model_relation_type is ManyToManyDescriptor: | |||
There was a problem hiding this comment.
What was the driving logic for removing these elif clauses? I recall reviewing a PR fairly recently that added some of these cases and I would like to make sure we're not regressing here.
There was a problem hiding this comment.
Dunno, I suppose it's a merging issue from 0ddf5ca. Any clue @astronouth7303?
There was a problem hiding this comment.
My general strategy was to apply the changes this made to the new codebase (ie, a manual rebase).
I don't recall what my logic is here, but it does look like a merge failure.
The code looks pretty good to me. My primary concern is that addition of polymorphic support does not disrupt package users who have no need for polymorphic models. As far as I can tell, you've achieved that. For the actually functionality, there are tests (hurray!), and I presume that you're scratching your own itch and have made this work for polymorphic models. I think if we can do a bit of cleanup and extend some documentation at spots, this will be ready to merge. Thanks @leo-naeka and @astronouth7303! |
Thank you for the review @mblayman, I'll work on it today. |
@leo-naeka Is there another way you'd like your name spelled in @mblayman I think that's all your code comments. However, the documentation still needs improvement. |
…work-json-api into polymorphism
AUTHORS Outdated
| @@ -3,6 +3,6 @@ Christian Zosel <https://zosel.ch> | |||
| Greg Aker <greg@gregaker.net> | |||
| Jamie Bliss <astronouth7303@gmail.com> | |||
| Jerel Unruh <mail@unruhdesigns.com> | |||
| Léo S. <leo@naeka.fr> | |||
CHANGELOG.md Outdated
| @@ -1,5 +1,6 @@ | |||
| v2.3.0 | |||
| * Added support for polymorphic models via `django-polymorphic` | |||
There was a problem hiding this comment.
Well, so actually this is only tested against django-polymorphic, which is the most popular and maintained polymorphic library for Django. But the implemented polymorphic support is library-agnostic and should work with libraries like django-polymodels, django-typed-models or even Django's contrib.GenericRelation...
I'm not sure how to deal with it, maybe an explicit warning, I'm not convinced to add more tests with more libraries... Any thoughts?
There was a problem hiding this comment.
My sense is that we should make the documentation clear that django-polymorphic is the only tested polymorphic library (thereby implying supported library). As far as the CHANGELOG is concerned, "via django-polymorphic" can be deleted.
docs/usage.md Outdated
| ### Working with polymorphic resources | ||
| Polymorphic resources (as backed by [django-polymorphic](https://django-polymorphic.readthedocs.io/en/stable/)) | ||
| allow you to easily use specialized subclasses without have to have special |
There was a problem hiding this comment.
I think you can drop "easily" and replace "have to have" with "requiring."
docs/usage.md Outdated
| endpoints to expose the specialized versions. For example, if you had a | ||
| `Project` that could be either an `ArtProject` or a `ResearchProject`, you can | ||
| have both kinds at the same URL. | ||
There was a problem hiding this comment.
I think we could put a new paragraph here that includes comments about polymorphic libraries (and remove the "as backed by" parenthetical in the previous paragraph). How does the following sound?
DJA tests its polymorphic support against django-polymorphic. The polymorphic feature should also work with other popular libraries like
django-polymodelsordjango-typed-models.
Thanks for the feedback! |
Is there anything else that needs to happen? |
I thought the generic relation support oddness was still in here and I was waiting on that, but I see that was cleaned up in 05cdb51. Thanks so much to the both of you! I really like how this all came together. I'll see if I can get the publish permissions so I can cut a new release to get this feature out there. |
Is there a reason that polymorphic "list" endpoints aren't supported (at least with the capability to display all of the fields of the parent model)? There is an exception raised by the get_fields method of PolymorphicModelSerializer that just says "("Cannot get fields from a polymorphic serializer given a queryset")" Also in the tests there is a "test_polymorphism_on_polymorphic_model_list_post" where you can POST to a polymorphic endpoint, but there is no "test_polymorphism_on_polymorphic_model_list" (ie. no GET project-list) I've patched this by removing the line that throws the exception in get_fields and then using the following (I changed the models to fit the Project example) This returns a list where the individual items are listed with the proper "type", unlike before where they were all listed with the parent type. I haven't tested this very thoroughly but thought I'd submit here for some feedback before digging any deeper. |
For me this merge is breaking some of my existing code. The problem is that at the end of the parser you add the type to the parsed data: ... # Construct the return dataparsed_data= {'id': data.get('id')} if'id'indataelse {} parsed_data['type'] =data.get('type') parsed_data.update(self.parse_attributes(data)) ...In case you have a model that has a |
@leo-naeka As you were the initial initiator of the polymorphic support within DJA I wonder whether you are still using it nowadays? Would you mind joining our discussion on #1194 so we can get a better picture on how the polymorphic support is used nowadays? Thanks. Anyone getting notified here and is still using polymorphic would also be great if you could join our discussion at #1194. |
Handles polymorphic resources with the correct models types.
This is transparent: no extra configuration is needed.
Should be tested a little bit more (typically on inverse relations, coming in the few next days), but I opened the PR to let anybody play with this and eventually get some feedback.