[Django] #20205: positiveintegerfield null=True, blank=True
Django
-------------------------------+--------------------
Reporter: anonymous | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.5
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
When using a model PositiveIntegerField with blank=True and null=True, ''
(empty string) is not allowed as a blank response value. An extra manual
step seems needlessly necessary to convert an empty string (say, received
from a form) to convert to python None value. Shouldn't this be fixed in
to_get_prep_value()? This is usually not the case for CharFields,
TextFields or some of the other model fields. Here is the exception:
{{{
Exception Value:
invalid literal for int() with base 10: ''
Exception Location: .../django/db/models/fields/__init__.py in
get_prep_value, line 991
}}}
It seems an update to IntegerField's get_prep_value or to_python may help.
--
Ticket URL: <https://code.djangoproject.com/ticket/20205>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------+--------------------------------------
Reporter: anonymous | Owner: nobody
Type: Uncategorized | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by anonymous):
* needs_better_patch: => 0
* component: Uncategorized => Forms
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/20205#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: nobody
Type: Uncategorized | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bmispelon):
* cc: bmispelon@… (added)
* component: Forms => Database layer (models, ORM)
Comment:
The original report is not very clear, so here's a way to reproduce the
error that the OP is refering to:
{{{
#!python
# models.py
from django.db import models
class Foo(models.Model):
bar = models.PositiveIntegerField(blank=True, null=True)
# to reproduce:
from django.core.exceptions import ValidationError
foo = Foo(bar='')
try:
foo.full_clean()
except ValidationError as error:
print(error)
else:
foo.save()
}}}
Doing this, the model validation passes but the save throws an error:
{{{
ValueError: invalid literal for int() with base 10: ''
}}}
I would be inclined to mark this ticket as invalid, since it's incorrect
to be passing an empty string as the value of an integer field, but
there's some issues in this report that might still be relevant:
* The model validation passes, which I find surprising (and might be
another bug on its own)
* The documentation (as far as I could find) does not mention what happens
when you give incorrect data to models.
--
Ticket URL: <https://code.djangoproject.com/ticket/20205#comment:2>
Django
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by anonymous):
I think there needs to be more control around what values the model should
allow for blank, Since PositveInteger inherits from Integer->Field it just
uses the Field clean which uses blank=''.
I think here the ORM abstraction leaks a bit. The Database could define
one set of values it will treats as blank for a given column while the
django ORM has a different another for the field.
--
Ticket URL: <https://code.djangoproject.com/ticket/20205#comment:3>
Django
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by anonymous):
I just came across the same problem. Also, I found that validation for
PositiveIntegerField is broken; doing a full_clean() does not detect
negative values.
--
Ticket URL: <https://code.djangoproject.com/ticket/20205#comment:4>
Django
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by AmiZya):
{{{
foo.full_clean()
}}}
return None, instead it should raise a ValidationError exception.
--
Ticket URL: <https://code.djangoproject.com/ticket/20205#comment:5>
Django
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: assigned
Component: Database layer | Version: 1.5
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by AmiZya):
* owner: nobody => AmiZya
* cc: amizya@… (added)
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/20205#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: AmiZya
Type: Uncategorized | Status: assigned
Component: Database layer | Version: 1.5
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by areski):
I think the problem is in the empty values allowed on IntegerField, this
makes the full_clean to not detect that empty string '' is not valid. I
will appreciate feedbacks
--
Ticket URL: <https://code.djangoproject.com/ticket/20205#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: AmiZya
Type: Uncategorized | Status: assigned
Component: Database layer | Version: 1.5
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by rach):
Could you add a regression test in the pull request / patch submitted as
bmispelon illustrated in his example.
--
Ticket URL: <https://code.djangoproject.com/ticket/20205#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: AmiZya
Type: Uncategorized | Status: assigned
Component: Database layer | Version: 1.5
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by areski):
thanks I added new patch and updated the pull request:
https://github.com/django/django/pull/1067
I added some cleaning of the test file.
--
Ticket URL: <https://code.djangoproject.com/ticket/20205#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: AmiZya
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by erikr):
* needs_better_patch: 0 => 1
* version: 1.5 => master
* has_patch: 0 => 1
* type: Uncategorized => Bug
* stage: Unreviewed => Accepted
Comment:
I looked at the patch, but the amount of style fixes made in the patch
makes it too hard for me to review the changes.
We typically do not do code style cleanup, unless it's very extreme or the
code is being modified already.
--
Ticket URL: <https://code.djangoproject.com/ticket/20205#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: AmiZya
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by erikr):
* cc: eromijn@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/20205#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: AmiZya
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: timograham@… (added)
* needs_better_patch: 1 => 0
Comment:
Updated pull request without unrelated changes and tweaked test.
https://github.com/django/django/pull/1213
--
Ticket URL: <https://code.djangoproject.com/ticket/20205#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: AmiZya
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by anonymous):
Thanks Timo for following up on this.
Quick question, I understand it s not good practice to change things that
aren't related.
I know Django code is no PEP8 but nevertheless some part of the code
changed in the patch were quite dirty, specially the 3 chars indent, what
the best practice to fix those?
--
Ticket URL: <https://code.djangoproject.com/ticket/20205#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: AmiZya
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by timo):
Separate ticket(s) for those issues would be great. I'd like to see each
type of cleanup in a separate patch (for example, all indentation fixes
together).
--
Ticket URL: <https://code.djangoproject.com/ticket/20205#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: AmiZya
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
A quick look at the code shows that DecimalField and FloatField probably
suffer from the same issue. I'd like to fix it for all numeric fields.
Since [4cccb85e] it's possible to customize `empty_values` per field.
However, I'm not sure that feature was introduced for this use case. It
appears to be targeted at custom fields (judging by the tests introduced
in that commit). Maybe Claude could confirm.
Besides, `IntegerField` already has `empty_strings_allowed = False`.
Removing the empty string from `empty_values` appears to duplicate this
information. So I really doubt it's the right fix.
If we step back, the root cause it's Django's historical lack of model-
level validation. Unfortunately I don't know the entire story. I'm just
mentioning it in case it helps finding a patch consistent with the
existing code.
--
Ticket URL: <https://code.djangoproject.com/ticket/20205#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: AmiZya
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/20205#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: AmiZya
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Niko Mäkelä <niko.j.makela@…>):
If I add some text or leave a PositiveIntegerField empty, it says "type
object 'unicode' has no attribute '_meta'". I'm using model forms and
Django 1.5.0 final.
--
Ticket URL: <https://code.djangoproject.com/ticket/20205#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: AmiZya
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by loic84):
[4cccb85e] was indeed designed with custom fields in mind, but using it is
the right way of addressing this issue.
Ideally each field would define exactly what values it considers empty
rather than using the historical blanket `EMPTY_VALUES = (None, '', [],
(), {})`.
After all `{}` would hardly make sense for a numerical field, yet it would
pass validation.
For the record, there is a tricky issue where a value of `None` when
`is_null=False` would pass validation only to burst upon saving. I've
tried to address that but it turned out to be almost impossible without
making significant incompatible changes. Also some people on IRC voiced
their concerns that None/null is a database concept and that they don't
want validation to mess with it, a few core dev however agreed that it
should be fixed if possible.
--
Ticket URL: <https://code.djangoproject.com/ticket/20205#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: AmiZya
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Adam Easterling <easterling.adam@…>):
After thinking about this issue at some length myself, I agree with
loic84: The real problem is that Model doesn't call a field's clean method
in the case that the value is "empty." That logic doesn't seem to belong
to the Model.
To me, it seems that this logic should be moved to the field class, so
each field could decide what's a valid "empty value" and what isn't.
Furthermore, there's a comment near the aformentioned Model logic that
says, "Skip validation for empty fields with blank=True. The developer is
responsible for making sure they have a valid value." This is odd to me --
the developer is responsible for making sure that fields have a valid
value, but where would that appropriately be done? You can't add a
validator to the field, as clean() isn't run for that field. Any remaining
solutions seem rather hacky.
My own solution was to just override all number fields and change how they
work, but I'm not super proud of it. Here's an example for Integers:
{{{
class FixedIntegerMixin(object):
'''
If you attempt to save an int field with a blank string, Django
doesn't
convert that to a None object, which is what the db expects.
Also, if the value is "blank" according to Django (see EMPTY_VALUES
found
at django.core.validators) it won't call to_python for that field (see
full_clean at django.db.models.base).
This means that we have to override get_prep_value itself.
'''
def get_prep_value(self, value):
if value is None:
return None
if value == "":
return None
return int(value)
class IntegerField(FixedIntegerMixin, IntegerField_django):
pass
class SmallIntegerField(FixedIntegerMixin, SmallIntegerField_django):
pass
class PositiveIntegerField(FixedIntegerMixin,
PositiveIntegerField_django):
pass
class PositiveSmallIntegerField(FixedIntegerMixin,
PositiveSmallIntegerField_django):
pass
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20205#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: AmiZya
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by charettes):
> The developer is responsible for making sure they have a valid value."
This is odd to me -- the developer is responsible for making sure that
fields have a valid value, but where would that appropriately be done?
I think a valid use case is to use the `clean()` method of the model
associated with the field to provide such a value.
--
Ticket URL: <https://code.djangoproject.com/ticket/20205#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: AmiZya
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
The problem here likely comes from HTML form submissions: there "" is the
natural None value for all fields that do not accept "" directly as a
value. My interpretation is that empty_values should be those values that
should be converted to None in validation. For IntegerField this should be
None and "", for CharField just None (as "" is a valid value directly).
I am not sure if we can change the logic so that all empty_values are
converted to None without breaking backwards compatibility badly.
I am not sure why we skip field validation for empty_values. Backwards
compatibility?
--
Ticket URL: <https://code.djangoproject.com/ticket/20205#comment:21>
Django
-------------------------------------+-------------------------------------
| Zyad
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Zach):
* cc: Zach (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/20205#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: Amine
| Zyad
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Brenton Partridge):
For others who might come across this error, check to make sure that you
(and your colleagues' code) are using a forms.IntegerField and not a
forms.CharField (or similar). forms.IntegerField, if left blank by the
submitter, will send None to the model field, but forms.CharField will
send an empty string which triggers OP's error.
That said, this is a way for someone to shoot themselves in the foot, and
the error message could be cleaner. One possible way forward would be for
`Field.get_prep_value` to check if the field has `empty_strings_allowed =
False`, and if so and if the value is an empty string, to throw some type
of error with a more meaningful error message. That said, it's not
strictly necessary, as someone will likely find this very thread from the
current error message, and realize what the problem is.
--
Ticket URL: <https://code.djangoproject.com/ticket/20205#comment:23>
Django
-------------------------------------+-------------------------------------
| Walls
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* owner: Amine Zyad => Jacob Walls
* needs_better_patch: 1 => 0
Comment:
It seems curious that under the status quo, where `empty_strings_allowed`
is False, that we would skip model validation even at the moment we are
holding an empty string (as opposed to None or any of the other values in
`empty_values`). [https://github.com/django/django/pull/14666 PR to run
model validation in this case.]
However, we could also treat as a duplicate of #22224 and add either
[Simon's friendly guardrail
https://code.djangoproject.com/ticket/22224#comment:5] for folks without a
solution for MyModel.clean() or just wontfix.
Happy to hear advice or to be nudged toward the mailing list.
--
Ticket URL: <https://code.djangoproject.com/ticket/20205#comment:24>
Django
-------------------------------------+-------------------------------------
Reporter: anonymous | Owner: Jacob
| Walls
Component: Database layer | Version: dev
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* status: assigned => closed
* resolution: => wontfix
Comment:
Per [https://groups.google.com/g/django-developers/c/GlYM25fdRnA mailing
list discussion] we cannot envision making behavior changes here because
we neither want to remove the blankable-and-not-nullable-and-inject-data-
on-save idiom nor silently cast data in ways a developer might not expect.
I collected some comments about the blank=True, null=False idiom on #22224
and will reframe it as a documentation task.
> That said, this is a way for someone to shoot themselves in the foot,
and the error message could be cleaner. One possible way forward would be
for Field.get_prep_value to check if the field has empty_strings_allowed =
False, and if so and if the value is an empty string, to throw some type
of error with a more meaningful error message. That said, it's not
strictly necessary, as someone will likely find this very thread from the
current error message, and realize what the problem is.
I think the current error (assuming it's in a similar form today to the
original `invalid literal for int(): ''`) points out that `''` is not an
int, so in my opinion, I wouldn't add a particularized exception for folks
using any single particular poor combination of form and model fields.
--
Ticket URL: <https://code.djangoproject.com/ticket/20205#comment:25>