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

NullBooleanField returns boolean value False for empty QueryDict. #7582

Closed
5 of 6 tasks
cosmosgenius opened this issue Oct 7, 2020 · 9 comments
Closed
5 of 6 tasks
Labels

Comments

@cosmosgenius
Copy link

cosmosgenius commented Oct 7, 2020

Checklist

  • I have verified that that issue exists against the master branch of Django REST framework.
  • I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
  • This is not a usage question. (Those should be directed to the discussion group instead.)
  • This cannot be dealt with as a third party library. (We prefer new functionality to be in the form of third party libraries where possible.)
  • I have reduced the issue to the simplest possible case.
  • I have included a failing test as a pull request. (If you are unable to do so we can still accept the issue.)

Steps to reproduce

Create a serializer as below with the test data

from django.http import QueryDict
from rest_framework import serializers

class TestSerializer(serializers.Serializer):
    is_active = serializers.NullBooleanField(required=False)

s_dict = TestSerializer(data={})
s_dict.is_valid()

s_qdict = TestSerializer(data=QueryDict())
s_qdict.is_valid()

Expected behavior

s_dict and s_qdict has the same value which is is_active: None

Actual behavior

s_dict returns is_active: None but s_qdict returns is_active: False

NullBooleanField was primarily used to avoid default_empty_html from BooleanField. Now that it is merged what is the alternative?

@amikrop
Copy link
Contributor

amikrop commented Oct 14, 2020

I observe that https://github.com/encode/django-rest-framework/blob/master/rest_framework/fields.py#L432 is False for the first case (data={}) which raises SkipField here https://github.com/encode/django-rest-framework/blob/master/rest_framework/serializers.py#L476 and True for the second case (data=QueryDict()) which lets set_value to be called here https://github.com/encode/django-rest-framework/blob/master/rest_framework/serializers.py#L486
That is the difference between those 2 cases, which causes this, but I don't know if this is intended behavior or not.

@xordoquy
Copy link
Collaborator

This is likely because NullBooleanField doesn't override the BooleanField's default_empty_html value which is False.

If you alter your field definition as is_active = serializers.NullBooleanField(required=False, default=None) you'll get a consistent behaviour.

A PR with the appropriate test could fix that too.

@amikrop
Copy link
Contributor

amikrop commented Oct 15, 2020

That's right, are we sure though that the developers' intentions are for NullBooleanField's default_empty_html to be None?
If yes, we can make that small PR, should be trivial.

@cosmosgenius
Copy link
Author

That's right, are we sure though that the developers' intentions are for NullBooleanField's default_empty_html to be None?
If yes, we can make that small PR, should be trivial.

Yes that is the expectation. But @xordoquy alternate field definition works well with BooleanField which is the right way to do since NullBooleanField is deprecated.

This does feel like a breaking change though in my opinion. If we didn't have proper CI/CD tests, this won't have been caught.

@beesnotincluded
Copy link

We too were caught out by this change. Our CI/CD test noticed the regression and we spent quite a while getting to the bottom of it.

@jaredmcdonald
Copy link

Just ran across this when upgrading our project to to 3.12.x (from 3.11.x) and also only caught it because of tests.

A BooleanField with default=None is still a bit of a departure from the previous behavior, where e.g. in @cosmosgenius' example above, is_active would not be present in validated_data if it wasn't passed in at all in the QueryDict. Is there some reason that the key should be present?

@Safrone
Copy link

Safrone commented May 4, 2021

I faced this as well where I was expecting the empty query dict to not insert the boolean

seeing that NullBooleanField is deprecated for DRF and django and I didn't actually need the third state (just using it to know whether true or false was explicitly given since default_empty_html otherwise obfuscates that), my solution is to override BooleanField:

from rest_framework import fields, serializers

class DefaultEmptyBooleanField(fields.BooleanField):
    default_empty_html = fields.empty

class MySerializer(serializers.Serializer):
    boolean_field = DefaultEmptyBooleanField(required=False)

use case:

from django.http import QueryDict

serializer = MySerializer(data=QueryDict(''))
serializer.is_valid()
serializer.validated_data # OrderedDict()

serializer = MySerializer(data=QueryDict('boolean_field=true'))
serializer.is_valid()
serializer.validated_data # OrderedDict([('boolean_field', True)])

You can add allow_null=True to DefaultEmptyBooleanField() for the added null option

@stale
Copy link

stale bot commented Apr 30, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@auvipy
Copy link
Member

auvipy commented Dec 4, 2022

can any of you try #8614 and report back please? seems related to this issue

@auvipy auvipy closed this as completed Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants