Range Fields do not support blank values via ModelForm

[James Addison]

The filing of this issue is based on a discussion in IRC (see ​https://botbot.me/freenode/django/2018-08-09/?msg=103129716&page=12 and prior messages), followed up by creating a test project to reproduce the issue.

Please do correct me if I am misusing range fields.

---

Saving a modelform with a model's rangefield 2 inputs left empty triggers an DB integrity error. I think the culprit lies with empty_values not containing ['', ''] as a possible empty value. (see the code around ​https://github.com/django/django/blob/1.11.15/django/forms/fields.py#L1026).

self.compress([]) should return the result of self.range_type(None, None) instead of just None.

With a view like:

def home(request):
    if request.method == 'POST':
        form = RangeTestForm(request.POST)
        if form.is_valid():
            instance = form.save()
    else:
        form = RangeTestForm(request.POST)

    return render(request, 'rangefieldtest/home.html', {'form': form})

Form like:

class RangeTestForm(forms.ModelForm):
    class Meta:
        model = RangeTest
        fields = '__all__'

and Model like:

from django.contrib.postgres.fields import FloatRangeField
from psycopg2._range import NumericRange

class RangeTest(models.Model):
    name = models.CharField(max_length=50, blank=True, default='')
    age_range = FloatRangeField(blank=True, default=NumericRange)

I will attach a sample project demonstrating this (use runserver, load the home page, click save)

---

psql table definition:

                                  Table "public.rangefieldtest_rangetest"
  Column   |         Type          |                               Modifiers
-----------+-----------------------+-----------------------------------------------------------------------
 id        | integer               | not null default nextval('rangefieldtest_rangetest_id_seq'::regclass)
 name      | character varying(50) | not null
 age_range | numrange              | not null
Indexes:
    "rangefieldtest_rangetest_pkey" PRIMARY KEY, btree (id)

data successfully stored on save (when at least one of the rangefield's 2 inputs are filled in):

 id | name | age_range
----+------+-----------
  2 |      | [3.0,)
(1 row)

data successfully stored (see id=3 below) on RangeTest().save(), which should also be the result when saving from the modelform:

test_rangefield=> select * from rangefieldtest_rangetest;
 id | name | age_range
----+------+-----------
  2 |      | [3.0,)
  3 |      | (,)
(2 rows)

[James Addison]

sample project reproducing the issue

[Tim Graham]

It looks like empty range values are currently stored as null and you're proposing to save them as empty ranges instead. The argument for changing behavior isn't completely obvious to me and it may be backwards incompatible.

[James Addison]

Tim, thanks for responding. I'm (almost?) positive that empty values are not stored as null. They're stored as (,) (see my psql select output in the description above).

[James Addison]

A workaround in the interim; add in the following .save(...) override:

class RangeTest(models.Model):
    name = models.CharField(max_length=50, blank=True, default='')
    age_range = FloatRangeField(blank=True, default=NumericRange)

    def save(self, *args, **kwargs):
        if getattr(self, 'age_range') == None:
            setattr(self, 'age_range', self._meta.get_field('age_range').range_type())

        super(RangeTest, self).save(*args, **kwargs)

[Tim Graham]

What I mean is that without any workaround code, Django is trying to save the empty value as null but you haven't added null=True on your FloatRangeField, hence the IntegrityError.

[James Addison]

Very true, I haven't in this example. I did initially start with null=True, but at the time it caused other issues (see below). Just making sure we're on the same page: shouldn't there be some consistency between an instance saved via the model's save method (ie RangeTest().save()) and that coming from a modelform's save method?

Otherwise you would always need to check for None before accessing range-specific properties/attributes:

lval = instance.age_range.lower  # this will blow up
if instance.age_range.isempty:  # even this will blow up
   # do stuff

# these will not blow up, but makes code very verbose!
lval = instance.age_range.lower if instance.age_range is not None else None
if instance.age_range is not None and instance.age_range.isempty:
   # do stuff

After all, the whole point of not using null=True is to ensure a properly typed value always exists.

[Simon Charette]

As Tim mentioned I think the behaviour should be consistent with other non-string based blank=True fields.

For example, trying to save an IntegerField(blank=True) form field without providing a default value at save time will also cause an integrity error because of the non-null constraint. Just like IntegerField(blank=True, default=0) when the user empties the inputs defaulting to the initial value.

Maybe we could add a note in the Field.blank documention to complement the Field.blank mention in Field.null's ​documentation.

Something along the line of

Setting blank=True and null=False on non-string based fields will require the addition of logic to handle empty user submitted values.

[James Addison]

Thanks, I appreciate the reply, but I do think this is sufficiently different and more impactful to warrant a deeper dive. It's end of day for me, and my mind is hazy, but I'll give this a go:

• Note that I did not clear any fields, as mentioned in the simpler example provided (ie. IntegerField(blank=True, default=0)). Rangefields with a default value have blank inputs, so there is nothing to clear. It's not 'user error', really.
• Without a fix for this, using rangefields with modelforms would a) require either null=True all the time to 'work', or b) field validation to always have either upper or lower bound value set (so, unable to have unbounded rangefield, but only when using a modelform to save), or c) the inability to use modelforms with models containing rangefields
• Lastly, as I mentioned in my comment above, with null=True as a 'fix' it would require using if instance.age_range is not None everywhere you wanted to access PostgreSQL Range-based properties like .lower, etc. This would make code incredibly verbose.

I admittedly don't have the same in-depth knowledge of Django as a core dev, but I think I understand the IntegerField's comparison being made and if so, I can't exactly agree that it's a valid comparison. Apologies if this is somewhat rambling and/or blunt - just trying to explain a user's view.

[Carlton Gibson]

Ultimately this is not anything to do with RangeFields per se.

Rather it's just a feature of how ModelForms interact with empty input and model field defaults.

Adding an equivalent IntegerField to the model, just to show it's not a RangeField thing, all the following is expected behaviour:

# The model class — note both `number` and `age_range` have `default`s: 
class RangeTest(models.Model):
    name = models.CharField(max_length=50, blank=True, default='')
    number = models.IntegerField(blank=True, default=0)
    age_range = FloatRangeField(blank=True, default=NumericRange)

# Then in a shell: 


In [1]: from rangefieldtest.models import RangeTest

In [2]: from rangefieldtest.forms import RangeTestForm

In [3]: r = RangeTest(name="No Defaults")

In [4]: r.__dict__
Out[4]: 
{'_state': <django.db.models.base.ModelState at 0x108bf32e8>,
 'id': None,
 'name': 'No Defaults',
 'number': 0,
 'age_range': NumericRange(None, None, '[)')}

In [7]: r.save()

In [8]: r2 = RangeTest(name="None number", number=None)

In [9]: r2.__dict__
Out[9]: 
{'_state': <django.db.models.base.ModelState at 0x108c468d0>,
 'id': None,
 'name': 'None number',
 'number': None,
 'age_range': NumericRange(None, None, '[)')}

In [10]: r2.save()
---------------------------------------------------------------------------
IntegrityError                            Traceback (most recent call last)
~/ve/tmp-2bf292c1f8d05d6/lib/python3.6/site-packages/django/db/backends/utils.py in execute(self, sql, params)
     63             else:
---> 64                 return self.cursor.execute(sql, params)
     65 

IntegrityError: null value in column "number" violates not-null constraint
DETAIL:  Failing row contains (7, None number, (,), null).

In [11]: r3 = RangeTest(name="None Range")

In [12]: r3 = RangeTest(name="None Range", age_range=None)

In [13]: r3.__dict__
Out[13]: 
{'_state': <django.db.models.base.ModelState at 0x108d52278>,
 'id': None,
 'name': 'None Range',
 'number': 0,
 'age_range': None}

In [14]: r3.save()
---------------------------------------------------------------------------
IntegrityError                            Traceback (most recent call last)
~/ve/tmp-2bf292c1f8d05d6/lib/python3.6/site-packages/django/db/backends/utils.py in execute(self, sql, params)
     63             else:
---> 64                 return self.cursor.execute(sql, params)
     65 

IntegrityError: null value in column "age_range" violates not-null constraint
DETAIL:  Failing row contains (8, None Range, null, 0).


In [15]: d = {"name": "Form data"} 

In [16]: f = RangeTestForm(data=d)

In [17]: f.is_valid()
Out[17]: True

In [18]: f.cleaned_data
Out[18]: {'name': 'Form data', 'number': None, 'age_range': None}

In [19]: r4 = RangeTest(**f.cleaned_data)

In [20]: r4.__dict__
Out[20]: 
{'_state': <django.db.models.base.ModelState at 0x108d7c2b0>,
 'id': None,
 'name': 'Form data',
 'number': None,
 'age_range': None}

In [21]: r4.save()
---------------------------------------------------------------------------
IntegrityError                            Traceback (most recent call last)
~/ve/tmp-2bf292c1f8d05d6/lib/python3.6/site-packages/django/db/backends/utils.py in execute(self, sql, params)
     63             else:
---> 64                 return self.cursor.execute(sql, params)
     65 

Here, the None value causes the IntegrityError. The form gives us the None value. And the model field default is not used when any value (even None) is provided.

This is what we expect.

What's wanted is to use the model field's default value. That's the same as #27039, so closing as a duplicate.

I think putting the work-around in the model's save() is misplacing it. What we want is the form to give us the right data.

One solution is to override clean():

# In forms.RangeTestForm...

    def clean(self):
        cleaned_data = super().clean()

        if cleaned_data["number"] is None:
            del cleaned_data["number"]

        if cleaned_data["age_range"] is None:
            del cleaned_data["age_range"]

        return cleaned_data

# Then in a shell: 

In [1]: from rangefieldtest.models import RangeTest

In [2]: from rangefieldtest.forms import RangeTestForm

In [3]: d = {"name": "Form data"}

In [4]: f = RangeTestForm(data=d)

In [5]: f.is_valid()
Out[5]: True

In [6]: f.cleaned_data
Out[6]: {'name': 'Form data'}

In [7]: r5 = RangeTest(**f.cleaned_data)

In [8]: r5.save()

An alternative would be to pick out the model field default in a clean_<field> method.
(A custom field/widget combo would also be an option, I guess.)
(It's a matter of taste.)

There are many competing concerns here. The current behaviour has evolved over a long period. It is a good default. For a specific form, with known constraints, there's no reason not to override the behaviour where that suits (as here).

Wanting to use the model default is not an uncommon use-case. Maybe we could improve the documentation here, but I don't want to leave this open pending a hypothetical suggestion. (We could review a PR with a new ticket if/when one comes in.)

[James Addison]

Carlton,

I know it's discouraged to reopen "core-dev closed" tickets (so I won't!), but I think this ticket should be reopened. Let me explain, starting with an apology: my first attached 'sample' project had a bug where the the modelform instance was created using request.POST as the data input even when loading via GET. Originally:

    if request.method == 'POST':
        form = RangeTestForm(request.POST)
        if form.is_valid():
            instance = form.save()
    else:
        form = RangeTestForm(request.POST)

... it should have been:

    if request.method == 'POST':
        form = RangeTestForm(request.POST)
        if form.is_valid():
            instance = form.save()
    else:
        form = RangeTestForm()

I found this because I naturally followed your IntegerField test (good idea for comparison, by the way). My bug causes any default values that MIGHT have been used as form defaults to render blank (as request.POST was a empty dict) - coming nowhere close to demonstrating the original problem at all, really. Sorry for the confusion!

That being said, I understand your logic, but it doesn't follow the dev-user experience with ModelForms. They are typically initially loaded without data (again, this was my sample code bug), which fills in all relevant fields with their defaults. ModelForms are request-driven data entities; using them in a shell-like environment as demonstrated is an edge case at best - meaning, all fields will be filled in from request.POST data.

• If the user submits the form, field defaults are in request.POST, so the IntegerField would normally have a value
  • Yes, the user could clear the IntegerField and that should trigger validation/an exception, etc. However, this is the outlying use case; additional code should be expected
  • without user invervention, the IntegerField's default value is respected on form submit
• In the case of RangeFields, an unbounded value in request.POST comes in as ... 'age_range_0': [''], 'age_range_1': [''] ... (due to the split widget) which is correct (no bounds are desired as the default), and this results in ['', ''] for the RangeField itself
  • without user invervention, the RangeField's default value of ['', ''] is not respected on form submit; it makes little sense to treat a valid default value as an IntegrityError; how can this not be a bug?
  • unbounded ranges are perfectly desirable and are not an outlying case that should need additional code to allow
  • put differently, the current save logic for one possible variation of a valid RangeField value is to raise an exception

I think I get that from a Django core-dev perspective this is muddying the waters/codebase (even though I think that the fix is likely as simple as empty_values including ['', ''] for RangeFields). But from a Django dev-user perspective (ok, ok, just me at the moment), what should just work doesn't.

I will attach a fixed sample project, which includes the number field in the model.

[James Addison]

Updated sample project to fix a bug

[Tim Graham]

I don't see a way forward that wouldn't be backwards incompatible. I believe that with your proposed change, what are saving as null values would now be saved as empty ranges. Maybe that's what some people want, but maybe it's not. You could perhaps argue that the deficiency is the form widget that doesn't allow distinguishing between null and an empty range.

[James Addison]

Tim,

You could perhaps argue that the deficiency is the form widget that doesn't allow distinguishing between null and an empty range.

I think this is exactly what I'm saying. As I see it, anyone who has null=False (or has omitted it) on a RangeField has to have a similar saving/cleaning hack to mine in place OR they don't use unbounded range fields. Either way, a fix is appropriate for this scenario, no?

Maybe that's what some people want, but maybe it's not.

I'm not sure what else people in the "null=False with a default set" scenario could possibly want other than the default value to be used if the form fields were left untouched.

Where it does break... The only scenario where I think allowing ['', ''] as a possible item in empty_values could break is for those who have null=True, as my simplistic fix would replace the current None value for range([None, None]). (Just guessing, but I'd think that many of these would have used null=True simply to make range fields 'work' for them.)

If that is the case, I can think of two possibilities:

1. put fix in place with a deprecation policy:
   1. add ['', ''] into empty_values for RangeFields (addresses the problem)
   2. as a temporary deprecation measure: if null=True is used, add a warning and override the value to be None if unbounded
   3. in future, remove the deprecation measure as per Django policies
   4. maybe advise to create a data migration to move nulls to empty ranges as necessary
2. put fix in place for null=False, but leave null=True as is (ie. always use None only if null=True)

The latter doesn't feel right to me, but I'm just tossing ideas at the wall to see if anything sticks. Of course, I may have missed backward incompatibility considerations - I appreciate any insight!

Lastly... Tim and Carlton, thanks for putting up with my feedback here. If, after everything mentioned here, you tell me that this won't be fixed because it is the desired path forward for Django, then I'll keep my hack in place and keep quiet. After all, I don't think I can add anything else at this point.

[Carlton Gibson]

Hey James. It’s late for me now so I can’t address each point you make individually. I will though make time to go over this again just to be sure.

Right now I’ll just say that I think this issue is a symptom of the more general problem that HTML forms don’t distinguish properly between empty string submissions when you mean empty string and empty string submissions when you mean None. (Or with checkboxes, not submitted vs false.) As such Django forms have to do their best to behave sensibly in the widest number of cases. (It’s been many years getting to where it is now: not that it can’t be improved but it’s pretty battle tested.)

In your case you have a form with a different requirement, but I think you’ll need to add that logic yourself.

As I said, I will though, have another think about this one.

[Tim Graham]

I'm not sure if there's a model field in Django where there's a chance for two values that could be considered "empty" besides CharField and the range fields. The docs for Field.null say, "In most cases, it’s redundant to have two possible values for “no data;” the Django convention is to use the empty string, not NULL." I'm not sure if there was much consideration of null vs. empty range for range field but changing it as this point merits a discussion on the DevelopersMailingList.

[James Addison]

Tim, Carlton,

I have little choice but to keep a hack in place (marked with a TODO note to revisit in future, of course). I don't particularly have a problem with this, but would like to see consistency and a better user experience for this in Django.

I'll bring it up on the mailing list, as you recommend. For the most part, I'll leave it in your hands after that, unless people ask me for feedback.

Thanks again.

[Carlton Gibson]

self.compress([]) should return the result of self.range_type(None, None) instead of just None.

This would be the simplest change. (Just adding ['', ''] to empty_values doesn't quite work, since the comprehension in the lines you linked goes per-value...)

So in ​`postgres.forms.ranges.BaseRangeField`:

    def compress(self, values):
        if not values:
            return None  # Should we use return self.range_type(None, None) here? 

For now (for your case) I'd subclass FloatRangeField and use ​`field_classes` to apply it.