hagsteel / swampdragon

swampdragon
Other
556 stars 73 forks source link

Leverage Django REST Framework #11

Open cancan101 opened 10 years ago

cancan101 commented 10 years ago

Given this description:

Model serializers Because SwampDragon will be communicating with the frontend via JavaScript, and JavaScript don't understand Django models, we need to tell SwampDragon how to serialize these models into JSON (short for JavaScript Object Notation).

why not use Django REST Framework (DRF)'s serializers rather than re-inventing the wheel? Code here and description here and here. Perhaps use the renderers? They seem to solve the exact goal that you outlined.

I know you mentioned that you did not use them here: https://news.ycombinator.com/item?id=8676967:

About the serializers: You do need separate serializers from DRF (they are not the same package after all).

but you don't address the reason why.

hagsteel commented 10 years ago

This is true, and it's something I have considered. However the serializers are not the same for DRF and SD (and Django have it's own serializers and they are also different from DRF etc.).

DRF just reached version 3.0 and the serializers have been worked over quite a bit, so if SD have a dependency on a specific version of DRF you wouldn't be able to upgrade to latest DRF.

The SD serializers are quite different from DRFs serializers as well. They have an entry point to the object map amongst other things, and there is no depth value as it's not required by SD. SD also don't bother with serializers for objects that aren't models (something I am considering adding though, for the sake of being able to apply validators and reformat values etc.) This is just mentioning a few reasons why this hasn't been done.

That's not to say that one could not extend the DRF serializers, but there would be functions in the serializer that would never be used or possible have a breaking effect. It wouldn't necessarily be an easy undertaking to do this.

This is not the first time this is mentioned however, so I see that it's something that would possibly be desired by the dev community, so I'm not knocking the idea, There is a lot more than just the serializers that have to be rewritten for this to work.

cancan101 commented 9 years ago

Closed?

hagsteel commented 9 years ago

The serializers are too different at the moment.

I might open a new issue to implement the DRF serializer but this wasn't really an issue, more of a question so I'm closing this for now

hagsteel commented 9 years ago

The DRF serializers does a lot that SD doesn't need or do. The way it handles foreign key relationships are way different etc.

There is no depth option in SD etc. etc. Too many differences at the moment.

LikeI said, I'm not knocking the idea of implementing them, but it becomes a dependency. What about version changes to DRF etc?

Most people would probably be okay with being tied to a version of tornado-redis but not DRF.

EDIT: If someone wants to create a pull request for this I will of course review it.

cancan101 commented 9 years ago

You should loop in @tomchristie but I think now that DRF is on v3, the API for Serializers should be relatively stable.

hagsteel commented 9 years ago

It would be nice if DRF serializers could be used.

I think a solution where the existing serializers and DRF serializers can both work would be ideal.

If I rewrote the way serializers are handled this could be a possibility (even go as far as making serializers swappable).

Whatever solution I (or someone else) come up with, should not create a direct dependency on DRF 3.x and it should be backwards compatible with the current version of SD.

I am considering this though. The SD serializers are not without their shortcomings and there are things I want to change with them so I might plan out an update and see if I can include a way to use the DRF serializers as well.

tomchristie commented 9 years ago

More than happy to help out with any questions if helpful.

I wouldn't be too concerned about tying yourself to API stability with REST framework - the 3.0 upgrade was something of an exception - for medium point releases (ie 3.x - which is what we'll be having for the foreseeable future) we have a strong deprecation policy that's essentially in line with what Django does.

http://www.django-rest-framework.org/topics/release-notes/#deprecation-policy

Also I can't see the serializers changing substantially at this point.

hagsteel commented 9 years ago

This might be doable with an adapter between the serializers and some work done to the current SD serializer to fit all this in.

I will ponder this over the weekend. I think file upload might be an issue, but not sure yet.

pcompassion commented 9 years ago

+1!

hagsteel commented 9 years ago

Quick update on this: TL;DR: It will be done but it's going to take some considerable time

I've been poking at this for a bit now and the serializers are very different indeed, and this is going to be quite an arduous task.

At this point I'm more inclined to rewrite both the serializers and route handlers which is not going to be a small task.

The DRF serializers are (of course) not aware of the data maps.

What this actually means:

The data map contains information on how objects should be mapped.

class Parent(models.Model):
    name = models.CharField(max_length=100)

class Child(models.Model):
    name = models.CharField(max_length=100)
    parent = models.ForeignKey(Parent)

If you have a serializer for Parent including children, when I make a change to a Child it will contain information for the data mapper to know which parent it belongs to (rather than serializing EVERYTHING every time, this means a small message can be sent to the client with just the updated fields and the mapping data)

This doesn't only affect the route handler but the self publishing models as well.

In short: I'm happy to do this, but the work it involves is not tiny so it's going to take quite some time to do this (fitting this around haivng a baby and my day job).

hagsteel commented 9 years ago

This is now available in the 0.4.3 branch (it's experimental at the moment)

silentninja commented 9 years ago

Was waiting for this update!!!!.Really excited to test it out

chogarcia commented 9 years ago

+1!

hagsteel commented 9 years ago

If anyone has tried the 0.4.3 branch I would love some feedback

sylflo commented 9 years ago

I'm trying to do the first tutorial using DRF. And after something a little more complex. In DRF you can use a model to create a serializer. But SwampDragon needs a serializer in its model.

I've got serializers.py

from rest_framework import serializers
from .models import TodoList, TodoItem

class TodoListSerializer(serializers.ModelSerializer):
    class Meta:
        model = TodoList
        fields = ('id', 'name', 'description')
        publish_fields = ('name', 'description')

class TodoItemSerializer(serializers.ModelSerializer):
    class Meta:
        model = TodoItem
        fields = ('id', 'done', 'text')
        publish_fields = ('done', 'text')
        update_fields = ('done', )

And models.py

class TodoList(SelfPublishModel, models.Model):
    serializer_class = TodoListSerializer
    name = models.CharField(max_length=100)
    description = models.TextField()

class TodoItem(SelfPublishModel, models.Model):
    serializer_class = TodoItemSerializer
    todo_list = models.ForeignKey(TodoList)
    done = models.BooleanField(default=False)
    text = models.CharField(max_length=100)

And I got

ImportError: cannot import name TodoList

I don't know how to resolve this issue. I want to use model = in the serializer file if possible.

silentninja commented 9 years ago

@sylflo Please create a new issue. Now answering the question, its a circular import error.

Solution:Use a string of the model name in the metaclass of the serializer instead of importing the actual model.
pcompassion commented 9 years ago

Would it be possible to define multiple serializers for one model class?

SwampDragon needs a serializer in its model. makes me ask the question.

hagsteel commented 9 years ago

You can create several serializers for one model but you can not have multiple serializers on a selfpublish model. However I am currently unsure of the future of the self publishing model. People don't seem to be able to use it properly and get upset when it's either creating too many queries or not enough information so at the time of writing this I would say don't use the self publish model and let the routers handle the publishing

Also, this doesn't really have anything to do with the DRF serializers so let's not continue this conversation here.

pcompassion commented 9 years ago

@jonashagstedt Thanks for the quick answer. I'll try the tutorial and will be back to you asap.

jsenecal commented 9 years ago

@jonashagstedt do you have any updates on this? I am myself a strong DRF user and just stumbled across your project (it looks awesome btw) but would like to avoid having to write another set of serializers.

hagsteel commented 9 years ago

@jsenecal You could try the 0.4.3 branch. I would love some help with feedback on this