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

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

Merged
merged 1 commit into from Dec 1, 2017

Conversation

RomuloOliveira
Copy link
Contributor

@RomuloOliveira RomuloOliveira commented Nov 30, 2017

Prior to the #5518, all fields with required=False and without a explicit default value were not included in the output.

After the patch, even non-required fields now had a default value, which is counter-intuitive if you look at the required documentation, which says Set to false if this field is not required to be present during deserialization.

This PR restore the original required=False behaviour. Now, any required=False without a explicit default value should not be returned in the output.
However, it keeps the implicit default=None for required fields introduced in #5518.

Question I: Did I wrote the test in the correct class (TestNotRequiredOutput) or should it be in the TestDefaultOutput class?

Question II: Is there a reason that the implicit default None value for allow_null is not in the Field.get_default function? Looking at the except handling code, at first it did not sound right.

Question III: Should I add to the documentation that allow_null implicitly set a default=None for required fields?

That issue is already closed and it is not my intention to reopen it, but it's a special case that I did not fully understand why not forcing a explicit default value. E.g, what about allow_blank? And many=True? Shouldn't they have a implicit default value too, using the same logic?

@carltongibson carltongibson requested a review from rpkilby Dec 1, 2017
@rpkilby
Copy link
Member

rpkilby commented Dec 1, 2017

Hi @RomuloOliveira, thanks for raising this. The docs do say that required fields may be omitted:

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.

To address a few things...

However, it keeps the implicit default=None for required fields introduced in #5518.

To clarify, the PR description is not completely correct, and this was not the final behavior. default values affect both serialization and deserialization, and allow_null implying default=None was not the correct behavior (you can see this in the commit/build history for #5518). Instead, allow_null only implies a default value for serialization output.

Did I wrote the test in the correct class (TestNotRequiredOutput) or should it be in the TestDefaultOutput class?

TestNotRequiredOutput is correct, as this isn't testing the default argument.

Is there a reason that the implicit default None value for allow_null is not in the Field.get_default function?

Yes, this is related to allow_null implying default=None not being the correct behavior.

Should I add to the documentation that allow_null implicitly set a default=None for required fields?

I don't think it's necessary for this PR. I'll address it separately.

rpkilby
rpkilby approved these changes Dec 1, 2017
Copy link
Member

@rpkilby rpkilby left a comment

👍

@carltongibson carltongibson added this to the v3.7.4 milestone Dec 1, 2017
@carltongibson carltongibson merged commit 905a557 into encode:master Dec 1, 2017
1 check passed
ticosax added a commit to ticosax/django-rest-framework that referenced this pull request Dec 23, 2017
Since encode#5639 we are unable to render fields that were rendered previously
with `allow_null=True`.
Backward compatibility is not preserved,
because `required=True` and `read_only=True` are conflicting, so in our
case, some fields just disappeared and it seems there is not way to get
them back.

I'm not sure about the fix, but the regression test is probably right.
@rpkilby
Copy link
Member

rpkilby commented Dec 23, 2017

In hindsight, while consistent with the required docs, I don't think this is the correct behavior. I'm thinking that allow_null should always imply a default serialization value, regardless of whether a field is required.

@RomuloOliveira, was there a specific reason why these fields needed to be omitted from the serialized output? It's not clear to me what behavior this is enabling.

@RomuloOliveira
Copy link
Contributor Author

RomuloOliveira commented Dec 26, 2017

@rpkilby I've made this to made required=False output consistent and simple (not required -> omitted from output, period) and restore this behaviour, which existed before 3.7.2.

In my opinion, it keeps things explicit and do not depend on any other flags combinations to work as expected. In this way, an user do not need to know all caveats and special behaviour that occurs when flags are combined.

I noticed this because after upgrading to 3.7.2 some fields were not consistent with others in an API that I was developing and it took a lot of time to figure out what was causing it. I've thought at first it was a bug before seeing the discussion #5518.

I thought it's a bit weird that allow_null=True implies a default serialization output. It is not consistent with python/DRF "philosophy" of keeping things simple and explicit.

This discussion was closed and so was #5518, but as an DRF user, I'm asked myself why use implicit values (which can interfere with another flags) and, why use it only in allow_null. I think it made inconsistent, since allow_blank=True do not imply a blank default output and I really don't think it should.

ticosax added a commit to lock8/django-rest-framework that referenced this pull request Jan 5, 2018
Since encode#5639 we are unable to render fields that were rendered previously
with `allow_null=True`.
Backward compatibility is not preserved,
because `required=True` and `read_only=True` are conflicting, so in our
case, some fields just disappeared and it seems there is not way to get
them back.

I'm not sure about the fix, but the regression test is probably right.
carltongibson added a commit to carltongibson/django-rest-framework that referenced this pull request Mar 20, 2018
carltongibson added a commit to carltongibson/django-rest-framework that referenced this pull request Mar 20, 2018
carltongibson added a commit that referenced this pull request Mar 20, 2018
* Revert "Non-required fields with 'allow_null=True' should not imply a default value (#5639)"
    This reverts commit 905a557.
    Closes #5708

* Add test for allow_null + required=False
    Ref #5708: allow_null should imply default=None, even for non-required fields.

* Re-order allow_null and default in field docs
    default is prior to allow_null. allow_null implies an outgoing default=None.

* Adjust allow_null note.
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
* Revert "Non-required fields with 'allow_null=True' should not imply a default value (encode#5639)"
    This reverts commit 905a557.
    Closes encode#5708

* Add test for allow_null + required=False
    Ref encode#5708: allow_null should imply default=None, even for non-required fields.

* Re-order allow_null and default in field docs
    default is prior to allow_null. allow_null implies an outgoing default=None.

* Adjust allow_null note.
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

Successfully merging this pull request may close these issues.

None yet

3 participants