You signed in with another tab or window.
Reload
to refresh your session.
You signed out in another tab or window.
Reload
to refresh your session.
You switched accounts on another tab or window.
Reload
to refresh your session.
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
Serializer.save() doesn't respect commit argument and has different behavior if many=True
#3166
Serializer.save() doesn't respect commit argument and has different behavior if many=True
Miserlou
opened this issue
Jul 16, 2015
· 9 comments
Hey all!
Easy but extremely important one here!
By default, ModelSerializers do not respect
commit
on
save()
. This defies Django conventions - and had me pulling my proverbial hair out for an hour!
Imagine a situation where I want to consume API from a distributed application, and I want to use Django Rest Framework to deserialize data from another API endpoint. I just want to consume it for a view, but not write it to a database. Ex:
serializer = KittenSerializer(data=data, many=True)
kittens = serializer.save(commit=False)
print kittens
[<Kitten 1>, <Kitten 2>] # Not written to database
kitten[0].purr()
purrrrrr
But instead, you fill get:
kittens = serializer.save(commit=False)
'commit' is an invalid keyword argument for this function
Now, an even more confusing thing is that it behaves differently with many=False. So, instead of the desired behavior, you will find:
serializer = KittenSerializer(data=data, many=False)
kitten = serializer.save(commit=False)
print kitten
<Kitten 1> # Written to database! :(
I did ultimately find a simply solution by overriding create in my Serializer like so (although it doesn't ever actually save, and I don't see a way to pass data to this function to make it conditional):
def create(self, validated_data):
return Kitten(**validated_data)
However, this should really work out of the box! If you want to user Django-Rest-Framework as an API
consumer
(which I think is a larger use case than the documentation accounts for, especially as microservices become more popular ) - this is really important information that should be more readily available!
This has already come up here, with no official answer:
#2958
but I think needs to be a) handed without overriding create and b) documented. I am willing to write the changes to the code and documentation if you will accept the feature in a PR.
Thoughts?
def test_kittens(self):
class KittenModel(models.Model):
name = models.CharField(max_length=30)
class KittenSerializer(serializers.ModelSerializer):
class Meta:
model = KittenModel
fields = ('name',)
serializer = KittenSerializer(data={'name': 'abc'})
serializer.is_valid()
serializer.save(commit=False)
Results in:
TypeError: Got a `TypeError` when calling `KittenModel.objects.create()`. This may be because you have a writable field on the serializer class that is not a valid argument to `KittenModel.objects.create()`. You may need to make the field read-only, or override the KittenSerializer.create() method to handle this correctly.
Original exception text was: 'commit' is an invalid keyword argument for this function.
Changing that to instead...
serializer = KittenSerializer(data=[{'name': 'abc'}], many=True)
Results in the same error.
I'd be happy to reconsider the 'inconsistent' behavior but I'd need a demonstrably replicable example or test case first, as it appear consistent to me.
Secondly:
ModelSerializers do not respect commit on save(). This defies Django conventions
You are correct that ModelSerializers do not behave in exactly the same way as ModelForm, however there are intentional design decisions behind these differences. commit is not (and won't ever be) a keyword argument to save().
I believe that the usage of .save() is pretty much adequately documented here:
http://www.django-rest-framework.org/api-guide/serializers/#saving-instances
But I'd accept suggestions on improvements there.
So, why?
commit=False would require the ModelSerializer to return an unsaved model instance. In the case where there are relationships as part of that instance then those relationships cannot be resolved until the instance has a primary key for the relationships to point at. This means we need to return a kind of proxy object that additionally saves information about any relationships, and that will defer instantiation of those instances until .save() is called. In the case of nested representations this is even more complex because you end up with a tree of dependancies that all need to be resolved. Note that this is slightly less of an issue in ModelForm, which can't (doesn't need to) deal with nested cases.
Not only would this behavior introduce inscrutable complexity it would also significantly alter the behavior of ModelSerializer from Serializer, adding in a whole lot of custom logic. As it stands there's a simple step to move between those two different levels of abstraction.
If you need this kind of behavior then rather than expecting the serializer to return an unsaved model instance, you should inspect the validated data, and then act on that as you wish in the view code or elsewhere.
kitten_kwargs = serializer.validated_data
kitten = Kitten(**kitten_kwargs)
Yes, the behavior is a break with the ModelForm style, but I believe it represents a cleaner more comprehensible implementation and a better separation of concerns.
For more context on this see also:
http://www.dabapps.com/blog/django-models-and-encapsulation/
https://www.youtube.com/watch?v=3cSsbe-tA0E
Thanks for taking the time to address this issue!
I think you're right about the first part, the inconsistent behavior was likely a result of me modifying the source while trying to figure out what was going on.
About the second part..
commit=False would require the ModelSerializer to return an unsaved model instance.
This is still a feature that I think is necessary for state-less API consumers. It isn't unreasonable to want to use DRF to use to map incoming data from an API to a Django model.
Still, I respect your point about complexity. So, I suggest that this issue actually be addressed from a documentation standpoint, to give a better understanding of the product as consumer by explicitly showing how to solve this problem with an example.
Sorry to come here and interrupt. However, I think the point of save(commit=False) is to be consistent with normal Django, and I would expect it to do the same as a ModelForm. Isn't it the point of ModelSerializer to replace the convenience of ModelForm? Otherwise you would use both serializers and normal Django forms in DRF code.
The following is the behavior in normal Django:
This save() method accepts an optional commit keyword argument, which accepts either True or False. If you call save() with commit=False, then it will return an object that hasn’t yet been saved to the database. In this case, it’s up to you to call save() on the resulting model instance. This is useful if you want to do custom processing on the object before saving it, or if you want to use one of the specialized model saving options. commit is True by default.
Another side effect of using commit=False is seen when your model has a many-to-many relation with another model. If your model has a many-to-many relation and you specify commit=False when you save a form, Django cannot immediately save the form data for the many-to-many relation. This is because it isn’t possible to save many-to-many data for an instance until the instance exists in the database.
To work around this problem, every time you save a form using commit=False, Django adds a save_m2m() method to your ModelForm subclass. After you’ve manually saved the instance produced by the form, you can invoke save_m2m() to save the many-to-many form data. For example:
# Create a form instance with POST data.
>> f = AuthorForm(request.POST)
# Create, but don't save the new author instance.
>> new_author = f.save(commit=False)
# Modify the author in some way.
>> new_author.some_field = 'some_value'
# Save the new instance.
>> new_author.save()
# Now, save the many-to-many data for the form.
>> f.save_m2m()
Calling save_m2m() is only required if you use save(commit=False). When you use a simple save() on a form, all data – including many-to-many data – is saved without the need for any additional method calls. For example:
@rserrano ModelSerializer performs a different job from ModelForm and as such have differences.
ModelSerializer makes it super easy to override the creation / update therefore there's little point in having a commit=False.
The two possible use cases of commit set to False are:
You want to add extra data to the model or have related instances to process first: in which case DRF has solid solutions by providing extra keywords arguments to save and create / update overriding respectively.
You want to use stateless data: in which case you shouldn't be using a Model are all.
In my case I want to send the model to a celery process that when finished saves it. I don't see a problem with using the validated_data and creating a model like Model(**validated_data), but when coming from normal Django you remember that it exists, (you could also do the same in ModelForm).
The result of ModelSerializer save method is the same as the result of ModelForm save method, there are not "models for DRF" and "models for Django", so I don't see how they would act differently.