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

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

Merged
merged 3 commits into from Oct 30, 2017

Conversation

rpkilby
Copy link
Member

@rpkilby rpkilby commented Oct 20, 2017

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

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

The question is whether or not allow_null=True should imply default=None.

Thanks to @eschercode for pointing this out here.

cc @xordoquy

@xordoquy
Copy link
Collaborator

xordoquy commented Oct 20, 2017

@rpkilby 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 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
@shangxiao
Copy link
Contributor

shangxiao commented Oct 23, 2017

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?

@carltongibson
Copy link
Collaborator

carltongibson commented Oct 25, 2017

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

@rpkilby
Copy link
Member Author

rpkilby commented Oct 25, 2017

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

allow_null=True should not imply default=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.

@rpkilby
Copy link
Member Author

rpkilby commented Oct 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 with required=True, a user must supply a value during deserialization.

@carltongibson 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
Copy link
Collaborator

@carltongibson carltongibson left a comment

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; adding default=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 carltongibson merged commit 5009aef into encode:master Oct 30, 2017
1 check passed
@rpkilby rpkilby deleted the allow-null-default branch Nov 11, 2017
@rpkilby
Copy link
Member Author

rpkilby commented Dec 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 the Field.required docs:

Setting this to False 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.

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

Successfully merging this pull request may close these issues.

None yet

4 participants