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

ModelSerializer fields don't obey default from the model #4703

Closed
gcbirzan opened this issue Nov 24, 2016 · 1 comment
Closed

ModelSerializer fields don't obey default from the model #4703

gcbirzan opened this issue Nov 24, 2016 · 1 comment

Comments

@gcbirzan
Copy link
Contributor

Currently, in django, even if a field doesn't have an explicit default=None, the implicit default is going to be None (technically, it's django.db.models.fields.NOT_PROVIDED). However, even if an explicit default is set on a field on the model, the serializer will ignore it.

This comes up in the context of PUT/PATCH, where one will (I know I did) expect PUT to put the default value for fields not passed that, at the very least, have an explicit default.

To give an example, a model with a field that is:

foo = models.PositiveSmallIntegerField(null=True, blank=True, default=42)

will generate the following field in the serializer:

foo = IntegerField(allow_null=True, max_value=32767, min_value=0, required=False)

The default value will be used when creating the instance, but not on PUT updates.

So, two things need to be discussed/decided:

  • Should fields that have an explicit default move that on to the serializer so that a PUT will populate the default value? - I would say a definite yes here, the current behaviour is surprising and undocumented (Or, I couldn't find it in the docs)
  • Should fields with implicit default (django.db.models.fields.NOT_PROVIDED) behave similarly and null out fields not provided for a PUT? - I would say no here, this is too big a change in behaviour and also might be surprising to other people.
@tomchristie
Copy link
Member

I'll narrow this down to one Q...

  • "Should model fields that have a default move that on to the serializer"

No. If there's a model default then the serializer field is marked as not required.


Pasting from IRC to save myself some time here...

[10:11:49] <tomchristie>	But the short is "default on a Model" =/= "default on Serializer"
[10:12:05] <tomchristie>	If the model has a default then the serializer field should be "not required". That's all.
[10:12:09] <tomchristie>	Nothing more nothing less.
[10:12:24] <tomchristie>	There's two reasons I wouldn't be changing that
[10:12:46] <tomchristie>	1. Because I think the "reset fields using the model default on update" is less obvious behavior, not more.
[10:13:00] <tomchristie>	2. Because it'd be a huge breaking change.
[10:13:07] <tomchristie>	But let's stick with (1)
[10:13:31] <gcbirzan>	Hm. Most APIs I interacted with behaved like this
[10:14:51] <tomchristie>	gcbirzan: Eg?...
[10:15:10] <tomchristie>	You *can* set a serializer default.
[10:15:36] <gcbirzan>	True.
[10:15:44] <tomchristie>	created = models.DateTimeField(default=...)
[10:15:44] <gcbirzan>	that's what I'm doing :)
[10:16:09] <gcbirzan>	The other reason I don't care very much is that we have our own base serializer class, so it's easy to change this
[10:16:15] <tomchristie>	is v different from `updated = serializers.DateTimeField(default=...)

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

No branches or pull requests

2 participants