#29656 closed Bug (duplicate)
Range Fields do not support blank values via ModelForm
Reported by: | James Addison | Owned by: | nobody |
---|---|---|---|
Component: | contrib.postgres | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Unreviewed | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
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)
Attachments (2)
Change History (20)
by , 6 years ago
Attachment: | rangefield.tgz added |
---|
comment:1 by , 6 years ago
Description: | modified (diff) |
---|
comment:2 by , 6 years ago
Description: | modified (diff) |
---|
comment:3 by , 6 years ago
Component: | Forms → contrib.postgres |
---|---|
Triage Stage: | Unreviewed → Accepted |
Version: | 1.11 → master |
comment:4 by , 6 years ago
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.
comment:5 by , 6 years ago
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).
comment:6 by , 6 years ago
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)
Edit: I realize I could have made the code simpler (see below), but that sample code is from my project where I have several range fields on my model so I loop through a list of field names to test for None
. For clarity, the workaround is as simple as:
class RangeTest(models.Model): ... def save(self, *args, **kwargs): if self.age_range == None: self.age_range = self._meta.get_field('age_range').range_type() super(RangeTest, self).save(*args, **kwargs)
Apologies for adding confusion.
comment:7 by , 6 years ago
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
.
comment:8 by , 6 years ago
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.
comment:9 by , 6 years ago
Triage Stage: | Accepted → Unreviewed |
---|
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
andnull=False
on non-string based fields will require the addition of logic to handle empty user submitted values.
comment:10 by , 6 years ago
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 usingif instance.age_range is not None
everywhere you wanted to access PostgreSQLRange
-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.
comment:11 by , 6 years ago
Resolution: | → duplicate |
---|---|
Status: | new → closed |
Ultimately this is not anything to do with RangeField
s 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.)
comment:12 by , 6 years ago
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 theIntegerField
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
- Yes, the user could clear the
- In the case of
RangeField
s, an unbounded value inrequest.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 theRangeField
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 anIntegrityError
; 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
- without user invervention, the
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.
comment:13 by , 6 years ago
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.
comment:14 by , 6 years ago
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:
- put fix in place with a deprecation policy:
- add
['', '']
intoempty_values
forRangeField
s (addresses the problem) - as a temporary deprecation measure: if
null=True
is used, add a warning and override the value to beNone
if unbounded - in future, remove the deprecation measure as per Django policies
- maybe advise to create a data migration to move nulls to empty ranges as necessary
- add
- put fix in place for
null=False
, but leavenull=True
as is (ie. always useNone
only ifnull=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.
comment:15 by , 6 years ago
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.
comment:16 by , 6 years ago
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.
comment:17 by , 6 years ago
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.
comment:18 by , 6 years ago
self.compress([])
should return the result ofself.range_type(None, None)
instead of justNone
.
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.
sample project reproducing the issue