Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Side loading support #105

Open
dustinfarris opened this issue Jul 7, 2015 · 13 comments
Open

Side loading support #105

dustinfarris opened this issue Jul 7, 2015 · 13 comments
Assignees

Comments

@dustinfarris
Copy link
Owner

An EDA user reached out recently and inquired about side-loaded data support. I think we should consider this. After brain-storming how to do this in the least DRF-invasive way, I think we can specify a special key to represent the data. Since JSON API uses included, I recommend we use a similar name-spaced key like _included_modelnames.

Something like this:

# models

class Person(models.Model):

    name = models.CharField(max_length=80)


class Animal(models.Model):

    owner = models.ForeignKey(Person, related_name='pets')    
    name = models.CharField(max_length=80)


# serializers

class AnimalSerializer(serializers.ModelSerializer):

    class Meta:
        model = Animal


class PersonSerializer(serializers.ModelSerializer):

    _included_animals = AnimalSerializer(many=True, read_only=True, source='pets')

    class Meta:
        model = Person
        fields = ('name', 'pets', '_included_animals')

Here, the pets field will use the default PrimaryKeyRelatedField, but there is an added _included_animals field that will piggy back the nested serializer. EDA would automagically intercept any _included keys and proxy to ED side loading. Thoughts?

/cc @benkonrath @holandes22

@benkonrath
Copy link
Collaborator

It feels more natural to me to support the EmbeddedRecordsMixin instead of adding support for side loading.

http://emberjs.com/api/data/classes/DS.EmbeddedRecordsMixin.html

DRF supports nested serializers (as you demonstrate in your example) so I think a user would just need to extend the RESTSerializer to add the EmbeddedRecordsMixin. We should actually consider adding the EmbeddedRecordsMixin to the RESTSerializer by default considering how easy it is to add nested or embedded records with DRF. I haven't looked into the details so I could be missing something. What do you think?

@dustinfarris
Copy link
Owner Author

EmbeddedRecordsMixin is already working well in EDA—but writable nested serializers are not supported by DRF. Side-loading would be a nice hybrid solution to have the writable pk field, but still get the embedded data in one request.

@benkonrath
Copy link
Collaborator

Thanks for the insight. Ok yeah, I agree it's useful to have the side loading. Your proposal seems good to me.

@g-cassie
Copy link

g-cassie commented Jul 8, 2015

I am using embedded records and to support the writable pk field I use a custom field.

Edit: not sure if I have ever tried this with a hasMany relationship but it definitely works with a belongsTo.

from __future__ import unicode_literals
from django.core.exceptions import ObjectDoesNotExist
from rest_framework.relations import RelatedField


class EmbeddedPrimaryKeyRelatedField(RelatedField):
    default_error_messages = {
        'required': 'This field is required.',
        'does_not_exist': "Invalid pk '{pk_value}' - object does not exist.",
        'incorrect_type': 'Incorrect type. Expected pk value, received '
                          '{data_type}.',
    }

    def __init__(self, *args, **kwargs):
        self._serializer = kwargs.pop('serializer', None)()
        assert self._serializer is not None, \
            '`serializer` is a required argument.'
        super(EmbeddedPrimaryKeyRelatedField, self).__init__(*args, **kwargs)

    def to_internal_value(self, data):
        try:
            return self.get_queryset().get(pk=data)
        except ObjectDoesNotExist:
            self.fail('does_not_exist', pk_value=data)
        except (TypeError, ValueError):
            self.fail('incorrect_type', data_type=type(data).__name__)

    @property
    def fields(self):
        return self._serializer.fields

    def __iter__(self):
        for field in self._serializer:
            yield field

    def __getitem__(self, attr_name):
        return self._serializer[attr_name]

    def to_representation(self, data):
        return self._serializer.to_representation(data)

Then I can do this in my serializer:

class PersonSerializer(serializers.ModelSerializer):

    pets = EmbeddedPrimaryKeyRelatedField(many=True, queryset=Animal.objects.all(), 
                                                                        serializer=AnimalSerializer)

    class Meta:
        model = Person
        fields = ('name', 'pets')

In Ember you need to do this:

# /serializers/person.js
import ApplicationSerializer from './application';
import DS from 'ember-data';

export default ApplicationSerializer.extend(DS.EmbeddedRecordsMixin, {
  attrs: {
    pets: { serialize: 'id', deserialize: 'records' },
  }
});

@dustinfarris
Copy link
Owner Author

@g-cassie that's an interesting approach. So it's basically embedded records on read, pk on write?

@g-cassie
Copy link

g-cassie commented Jul 8, 2015

@dustinfarris that's correct. This approach is documented in the ED api docs as well so support should continue for it.

http://emberjs.com/api/data/classes/DS.EmbeddedRecordsMixin.html

@dustinfarris
Copy link
Owner Author

From the section on hasMany:

{ embedded: 'always' } is shorthand for: { serialize: 'records', deserialize: 'records' }

So you are just flipping the switch on serialize—I think that should work on hasMany as well if you were to do:

attrs: {
  pets: { serialize: 'ids', deserialize: 'records' }
}

Would it be easier to maintain documentation for two separate serializers for read/write instead of a custom field?

Like:

# serializers

class PersonSerializer(serializers.ModelSerializer):

    pets = AnimalSerializer(many=True, read_only=True)

    class Meta:
        model = Person


class WritablePersonSerializer(serializers.ModelSerializer):

    # PrimaryKeyRelatedField used by default for `pets`

    class Meta:
        model = Person


# viewsets

class PersonViewSet(viewsets.ModelViewSet):

    model = Person

    def get_serializer_class(self):
        if self.action in ['create', 'update']:
            return WritablePersonSerializer
        return PersonSerializer

Not saying this is any better, just wondering if there are any long-term pros/cons either way?

Does the embedded solution negate the need for side loading support altogether?

@g-cassie
Copy link

g-cassie commented Jul 8, 2015

Two serializers could work and your code example is simpler to document for sure. Personally I like that the custom field consolidates all the logic in serializers.py. If my custom field were available in the DRF core I think it would definitely be the way to go, but with no widely supported package available with my custom field your solution is probably easier to recommend.

I think with the implementation you are proposing you will not get the other benefits of side-loading - namely eliminating duplicate data in embedded payloads. ActiveModelSerializer and JSON API both do something like this:

data: [{<person 1 attrs>}, {<person 2 attrs}],
included: [{pet 1 attrs}, {pet 2 attrs}]

Without modifying DRF you will be stuck with something like this:

[
{<person 1 attrs>, included: [{pet 1 attrs}, {pet 2 attrs}]}, 
{<person 2 attrs>, included: [{pet 1 attrs}, {pet 2 attrs}]}, 
]

@dustinfarris
Copy link
Owner Author

@g-cassie: Good point, none of the solutions here address duplicate data in a ListView. To do that would require some hacking at the response/renderer level which is definitely out of scope for this project.

I think I am going to document the multiple serializer solution for now and forego formal side-loading support. Do you have a blog post on the custom field I can link to as an alternate solution?

@g-cassie
Copy link

g-cassie commented Jul 8, 2015

@dustinfarris No blog post and my bandwidth is limited right now. I threw it into a gist you can link to:
https://gist.github.com/g-cassie/8b4c62bf2da7d791bc36

I don't think I have ever tested this field with hasMany relationships but theoretically it should work.

@dustinfarris
Copy link
Owner Author

👍

@rinti
Copy link

rinti commented Jul 16, 2015

@g-cassie Thanks man! That works flawlessly for my needs at the moment 👍

@rinti
Copy link

rinti commented Sep 1, 2015

This duplicate issue - is it that I'm running into maybe?

I have an article with embedded events and send json like this
{title: 'Hello', events:[{start: '2014-01-01', end: '2014-02-02'}]}

and when I save this object my model has two events attached until i refresh the page. Any pointers as to how I should solve that? Calling .reload() on the model doesn't seem to help.

I can solve it by doing

        model.get('events').forEach(event => {
          if (!event.get('object_id')) {
            model.get('events').removeObject(event);
          }
        });

but that doesn't really seem optimal.

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

No branches or pull requests

4 participants