encode/django-rest-frameworkPublic

NotificationsYou must be signed in to change notification settings
Fork7k
Star29.7k

Fields with 'allow_null=True' should imply a default serialization value#5518

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

carltongibson merged 3 commits intoencode:masterfromrpkilby:allow-null-default

Oct 30, 2017

Merged

Fields with 'allow_null=True' should imply a default serialization value#5518

carltongibson merged 3 commits intoencode:masterfromrpkilby:allow-null-default

Oct 30, 2017

Conversation

Copy link

Contributor

rpkilby commentedOct 20, 2017

The following currently fails as it's missing adefault value for serialization.

classSerializer(serializers.Serializer):bar=serializers.CharField(source='foo.bar',allow_null=True)

The question is whether or notallow_null=True should implydefault=None.

Thanks to@eschercode for pointing this outhere.

cc@xordoquy

Add test for dotted source + allow_null

b949629

Copy link

Contributor

xordoquy commentedOct 20, 2017

Yeah, this is a follow up forhttps://stackoverflow.com/questions/46849343/drf-3-7-0-removed-handling-none-in-fields-and-broke-my-foreign-key-source-fields

xordoquy referenced this pull request

Oct 20, 2017

Remove None handling from fields.get_attribute()

07258ca

carltongibson added this to the3.7.2 milestone

Oct 20, 2017

carltongibson assignedxordoquy andrpkilby

Oct 20, 2017

Ryan P Kilby added2 commits

October 20, 2017 10:33

Field 'allow_null' implies 'default=None'

ddeecde

Field 'allow_null' provides serialization default

c6bc7ff

rpkilby changed the title~~Fields with 'allow_null=True' should imply 'default=None'~~Fields with 'allow_null=True' should imply a default serialization value

Oct 20, 2017

rpkilby changed the title~~Fields with 'allow_null=True' should imply a default serialization value~~[don't merge] Fields with 'allow_null=True' should imply a default serialization value

Oct 20, 2017

Copy link

shangxiao commentedOct 23, 2017•
edited
Loading

Aside from discussions about whether flags should imply default values, I'd just would like to point out that if a serializer declares a field then a key for that field should unconditionally exist in the serialized output. The referenced change broke this contract for the first time.

Although: I'd agree if this was rejected due to explicit vs implicit. Perhaps requiring a default to be declared in these cases?

Copy link

Collaborator

carltongibson commentedOct 25, 2017

@rpkilby What's the status here? I'm beginning to look at a 3.7.2 (say beginning of next week). Ta!

Copy link

ContributorAuthor

rpkilby commentedOct 25, 2017

@carltongibson Basically, trying to figure out what the correct behaviorshould be here.

allow_null=True shouldnot implydefault=None. This was verified by test failures, where you may want to allow null values as input, but require this explicitly during deserialization.

That all said:

Aside from discussions about whether flags should imply default values, I'd just would like to point out that if a serializer declares a field then a key for that field should unconditionally exist in the serialized output. The referenced change broke this contract for the first time.

Is this contract actually true, or is it an assumption made as a result of the previous behavior? Pinging@tomchristie for input here.

Copy link

ContributorAuthor

rpkilby commentedOct 25, 2017

At this point, I'm kind of leaning towards accepting the PR. It allows correct serialization of fields when no object relation exists. However, when coupled withrequired=True, a usermust supply a value during deserialization.

carltongibson changed the title~~[don't merge] Fields with 'allow_null=True' should imply a default serialization value~~Fields with 'allow_null=True' should imply a default serialization value

Oct 30, 2017

carltongibson approved these changes

Oct 30, 2017

View reviewed changes

Copy link

Collaborator

carltongibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

OK. We're going to take this. Ultimately it seems entailed by the decisions we've already made, and we seemed to agree at each step.

The behaviour now seems consistent. There is (already) a slight change from earlier behaviour; addingdefault=None is recommended.

Maybe there's some weird combination of flags for which the behaviour is unexpected, but we need that pinned down in an actual code example if we are to proceed on it.

Even with such an example the answer might be, "use a custom field in that case" — we're not necessarily committed to covering every possible scenario.

For now, at least, we've spent long enough thinking about this one.

Thanks all.

carltongibson merged commit5009aef intoencode:master

Oct 30, 2017

carltongibson added the Cleanup label

Oct 30, 2017

rpkilby deleted the allow-null-default branch

November 11, 2017 05:27

RomuloOliveira mentioned this pull request

Nov 30, 2017

Non-required fields with 'allow_null=True' should not imply a default value#5639

Merged

Copy link

ContributorAuthor

rpkilby commentedDec 1, 2017

Just as a followup from#5639..

I'd just would like to point out that if a serializer declares a field then a key for that field should unconditionally exist in the serialized output.
--@RapiLabs

This statement is true,except for non-required fields. Quoting from theField.required docs:

Setting this toFalse also allows the object attribute or dictionary key to be omitted from output when serializing the instance. If the key is not present it will simply not be included in the output representation.