Attempt of implementing a default `GenericForeignKey` representation. #755

Closed
wants to merge 13 commits into
from

Conversation

Projects
None yet
5 participants

Fixes #606. Attempt of implementing a default GenericForeignKey representation. Documentation in rest_framework.genericrelations.py. Tests included.

Owner

lukasbuenger commented on 67b891b Mar 26, 2013

There is an error in the inline documentation at Line 66: It should be as_hyperlink is False

Collaborator

tomchristie commented Mar 26, 2013

Hiya! Thanks for getting the ball rolling on this. :)

I think the broad idea of this PR is about right.

I'm not sure about the list of GenericRelatedOptions as the way of representing it, I'd previously been assuming that the dict style mentioned in #606 would cover things adequately.

tagged_item = serializers.GenericRelatedField({
    Note: serializers.HyperlinkedRelatedField(view_name='note-detail'),
    Bookmark: : serializers.HyperlinkedRelatedField(view_name='bookmark-detail')
})

The advantage of that is that we're just re-using existing relational field type in the definition, which feels simpler, and less extra stuff to document.

I didn't quite on first pass how generic field determines which serializer to use when deserializing the objects - I can see some bits that make sense for the hyperlinked case but I wasn't sure about the nested case, or about which bits of that would be exposed and documented as public API.

I guess next steps are getting your feedback on if you think the dict style representation would be suitable, and if not why not, and discuss the API & implementation for determining which serializer is used during deserialization.

@tomchristie tomchristie commented on an outdated diff Mar 26, 2013

rest_framework/genericrelations.py
+**`as_hyperlink`**
+Decides whether the output of the `GenericForeignKey` should be as end point or nested object. In the case of the
+latter a generic serializer will be used unless you pass a specific one.
+
+**`related_field`**
+A specific subclass of `HyperlinkedRelatedField` that should be used for resolving input data and resolving output data
+in case of `as_hyperlink` is `True`.
+
+**`serializer`**
+A specific subclass of `ModelSerializer` that should be used for resolving output data
+in case of `as_hyperlink` is `True`.
+
+"""
+from __future__ import unicode_literals
+
+import urlparse
@tomchristie

tomchristie Mar 26, 2013

Collaborator

from rest_framework.compat import urlparse should fix your travis Py3k issues.

I actually started implementing a dict style representation, I'll explain below why I didn't go with that. Mostly a matter of taste, I guess.

Feature requirements

Here's a list of features I'd expect from a GenericRelatedFieldimplementation:

  • A readable and well defined way to setup the possible relations and their components.
  • The possibility of creating a generic relation through the API by sending a resource URL as parameter (HyperlinkedRelatedField).
  • The possibility to output / deserialize a generic relation as resource URL (HyperlinkedRelatedField).
  • The possibility to output / deserialize a generic relation as nested object (ModelSerializer).
  • The possibility to configure each of the latter two according to my needs. For example slug_url_kwarg for the
    HyperlinkedRelatedField, exclude tuple for the ModelSerializer Meta and so on.
Some questions
  • How to decide whether a hyperlinked or nested output is wanted?
  • Is it desirable or even sensible to be able to do so for every possible relation or just for the GenericRelatedField in general?
  • What is the minimal configuration needed to setup a GenericRelatedField and how would we want to add and sanitize additional options?

Implementing a GenericRelatedField that only implements resource URL input and output would be pretty straightforward and the dict style fits in just perfectly.

tagged_item = serializers.GenericRelatedField({
    Note: serializers.HyperlinkedRelatedField(view_name='note-detail'),
    Bookmark: : serializers.HyperlinkedRelatedField(view_name='bookmark-detail')
})

The more complex part is the nested object thing. Let's assume that we accept another argument to decide whether the output should be nested in general or not:

tagged_item = serializers.GenericRelatedField({
    Note: serializers.HyperlinkedRelatedField(view_name='note-detail'),
    Bookmark: : serializers.HyperlinkedRelatedField(view_name='bookmark-detail')
}, output_as_hyperlink=False)

Now every possible relation has to be somehow connected to a ModelSerializer.
We could for example explicitly ask for a ModelSerializer instance if `output_as_hyperlink=False`` by a nested dict object (or a tuple of length=2 and so on):

tagged_item = serializers.GenericRelatedField({
        Note: {
            hyperlinked_field: serializers.HyperlinkedRelatedField(view_name='note-detail'),
            serializer: MyNoteSerializer()
            }
        Bookmark: {
            hyperlinked_field: serializers.HyperlinkedRelatedField(view_name='bookmark-detail')
            serializer: MyBookmarkSerializer()
        }
    }, output_as_hyperlink=False)

Even by now, we would have two different kind of data structures that would be required or be invalid based on a simple constructor argument. The evaluation of this dict could get quite messy and you'd have to raise errors like "Your dict is invalid because of parameter X is False, however, if you set this parameter to True it would be perfectly fine". While this is totally handleable I personally don't like it too much.

So I thought that it would be nice to have a few mandatory arguments (Model class, view_name) that imply some default behavior and the possibility to further customize by adding custom HyperlinkedRelatedFields, Serializers and so on.
That is basically why I abandoned the dict solution in favor of a class that with its argument and keyword argument evaluation does a lot of the heavy lifting you'd have to do manually by implementing a dict analysis.

I hope this explains my design decisions at least a little. But as I mentioned before, this is mostly a matter of taste. I think you as the project owner just have to decide which way to go and I'll do what I can.

Collaborator

tomchristie commented Mar 27, 2013

I don't quite understand the nested example, I'd have just been expecting something like this...

tagged_item = serializers.GenericRelatedField({
    Note: MyNoteSerializer(),
    Bookmark: MyBookmarkSerializer()
})

(Of course there's still a discussion to be had around how it determines which serializer to use for each incoming data item)

In your example what is the extra hyperlinked_field information being used for?

Fair point. In this case however you would not be able to write a GenericForeignKey. Is that right?
The hyperlinked_field handles the serialization of an incoming resource URL.

And it would allow something like this:

tagged_item = serializers.GenericRelatedField({
    Note: MyNoteSerializer(),
    Bookmark: serializers.HyperlinkedRelatedField('bookmark-detail')
})

This would imply, that you can write Bookmark objects but not Note objects.

"Of course there's still a discussion to be had around how it determines which serializer to use for each incoming data item"
Ah, now I get it. I assumed that it is simply not possible to map an incoming data object to a ContentType without exposing ContentType to the public domain. IMHO, the only sane way to identify incoming data is by resolving a resource detail endpoint.

Collaborator

tomchristie commented Mar 27, 2013

And it would allow something like this

Not a case you'd be likely to want to use, but yes that'd be valid :)

IMHO, the only sane way to identify incoming data is by resolving a resource detail endpoint.

There are other use-cases I can think of that would make sense, but yes I think it would be reasonable to only support the HyperlinkedRelatedField case as writable by default.

We just need to make sure that GenericRelatedField has a method that determines how the serializer is determined that can be overridden if the end-user developer really needs to support some other case.

Eg...

def determine_serializer_for_data(self, data):
      if this class is init'ed with a dict of HyperlinkedRelatedFields:
          # Handle the hyperlink case, since that's the only sensible default.
          get the view by reversing the incoming url
          return the corresponding serializer, or fail if the view doesn't match any of them.
       else:
          # Fail loudly in any other instance,
          # informing dev that the should set `read_only=True` on the GenericRelatedField

Alright, I summarize:

These are the two valid variants:

Nested output, read only:

tagged_item = serializers.GenericRelatedField({
    Note: MyNoteSerializer(),
    Bookmark: MyBookmarkSerializer()
}, read_only=True)

Hyperlinked output, read and write:

tagged_item = serializers.GenericRelatedField({
    Note: serializers.HyperlinkedRelatedField(view_name='note-detail'),
    Bookmark: serializers.HyperlinkedRelatedField(view_name='bookmark-detail')
}, read_only=False)

Additionally:

  • read_only=True fields are valid with mixed serializers.
  • read_only=False fields are valid with mixed serializers (or even ModelSerializer only) until you are trying to POST/PUT to the generic foreign key field.
  • GenericRelatedField has a method to determine the serializer. By default, it imposes aforementioned restrictions, but can be overridden in case of the end-user developer needing to support some other case.

Is that correct?

Collaborator

tomchristie commented Mar 30, 2013

From a quick first pass, that's looking about right I think.
If you could also update the 'generic FKs' section in the serializer relations part of the docs that'd be great (and would also make this easier to review)

Collaborator

tomchristie commented Apr 22, 2013

Apologies I've not gotten around to the latest review of this, been stacked up working on various stuff to get into a 2.3 release. Will try to make some time for it ASAP. Also feel free to give me a nudge if I've not gotten around to it sometime soon.

That's alright, I was quite busy too. As long as it is going to be included within the next few months, this is just fine with me.

Am 22.04.2013 um 21:32 schrieb Tom Christie notifications@github.com:

Apologies I've not gotten around to the latest review of this, been stacked up working on various stuff to get into a 2.3 release. Will try to make some time for it ASAP. Also feel free to give me a nudge if I've not gotten around to it sometime soon.


Reply to this email directly or view it on GitHub.

Contributor

thetylerhayes commented Aug 17, 2013

Any movement on this? I'm trying to do the same thing as in #896.

Contributor

thetylerhayes commented Aug 18, 2013

Update: I was able to accomplish our needs for #896 by overriding def list since those needs didn't explicitly include generic foreign keys. But we do still actually have generic foreign keys otherwise and this would be a killer feature for them.

Collaborator

tomchristie commented Sep 5, 2013

Marked for attention at DjangoCon. This'd be a really great ticket to progress. I need to put more detail against why it's currently in a stuck state, so contact me to discuss further if you'd like to work on it. Short answer is that a GenericField shouldn't be tied to any particular field type (eg should only support hyperlinked types), but we should discuss in more detail.

I'd love to push this further, but I think it's not been given enough thought yet. I opened a thread over at the groups to gather a little more feedback.

@lukasbuenger I read through your tests and didn't see an example of writing to a nested reverse relationship on a generic field. So , in this case creating a new Bookmark or Note and at the same time creating a new Tag.

JSON:

{
    'url': 'http://google.com/whatever',
    'tags': [
        'tag': 'search engines',
        'app_name': 'appname',
        'model': 'book'
    ]
}

I imagine I could then use ContentType.objects.get(app_label='appname', model='book') and create the correct serializer class or create a model instance manually, but I got lost trying to figure out how to create the bookmark first, get it's id attribute down to the nested serializer, and save that. Is this possible from either side of the relationship?

I'm not quite sure whether I understand your question. This GenericRelatedField implementation allows write operations on the model defining the generic relation only when a related model is hooked into the GenericRelatedField with a HyperlinkedRelatedField. Reverse generic relations "... expressed using the GenericRelation field, can be serialized using the regular relational field types, since the type of the target in the relationship is always known." Courtesy of the docs: http://www.django-rest-framework.org/api-guide/relations#generic-relationships.

@lukasbuenger It's OK, reading back through my question it doesn't make much sense to me now either. I think I was trying to use generic relations for something they are really not made for, so I ended up changing my models to something that seems more straightforward.

Contributor

Ian-Foote commented Apr 10, 2014

What still needs to be done for this to be merged? It looks like the functionality I need is implemented - creating instances of a model with a GenericForeignKey using GenericRelatedField with HyperlinkedRelatedField.

Collaborator

tomchristie commented Apr 10, 2014

@Ian-Foote Not sure, but at this point I'll be leaning pretty strongly towards the third-party-app camp. A year ago it seemed reasonable, but given the current constraints on maintaining the project I strongly feel like this would be much better handled with someone else as the owner of it as a separate project.

It's clearly one of those bits of functionality that'd result in additional maintenance / support questions, which I don't think we'd be in a position to adequately support.

Contributor

Ian-Foote commented Apr 10, 2014

Ok, thanks. I may look into releasing this as a third-party app (unless @lukasbuenger wants to do it).

Collaborator

tomchristie commented Apr 10, 2014

Great, closing this off now, but more than happy to continue to track any further progress against this ticket all the same. If and when there's an in-progress repo open a new issue for linking to it from the docs so it doesn't get forgotten.

@Ian-Foote Ian-Foote added a commit to Ian-Foote/rest-framework-generic-relations that referenced this pull request Apr 12, 2014

@lukasbuenger @Ian-Foote lukasbuenger + Ian-Foote refactored GenericRelatedField with dict argument as discussed here: e…
…ncode/django-rest-framework#755

Conflicts:
	generic_relations/tests/test_relations.py
337fae1
Contributor

Ian-Foote commented Apr 12, 2014

@tomchristie @lukasbuenger I've created a new repository with the code from this pull-request and started preparing to release it as a package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment