Opened 12 years ago
Closed 3 years ago
#20205 closed Bug (wontfix)
PositiveIntegerfield does not handle empty values well
Reported by: | anonymous | Owned by: | Jacob Walls |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | bmispelon@…, amizya@…, eromijn@…, timograham@…, Zach | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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.
Attachments (2)
Change History (27)
comment:1 by , 12 years ago
Component: | Uncategorized → Forms |
---|
comment:2 by , 12 years ago
Cc: | added |
---|---|
Component: | Forms → Database layer (models, ORM) |
comment:3 by , 12 years ago
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.
comment:4 by , 12 years ago
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.
comment:5 by , 12 years ago
foo.full_clean()
returns None, instead it should raise a ValidationError exception.
comment:6 by , 12 years ago
Cc: | added |
---|---|
Owner: | changed from | to
Status: | new → assigned |
by , 11 years ago
Attachment: | patch_ticket20205.txt added |
---|
Patch for ticket 20205 fix for empty values allowed on IntegerField
comment:7 by , 11 years ago
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. See patch in attachment. I will appreciate feedbacks
comment:8 by , 11 years ago
Could you add a regression test in the pull request / patch submitted as bmispelon illustrated in his example.
by , 11 years ago
Attachment: | patch_ticket_20205_b.txt added |
---|
comment:9 by , 11 years ago
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.
comment:10 by , 11 years ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
Summary: | positiveintegerfield null=True, blank=True → PositiveIntegerfield does not handle empty values well |
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Bug |
Version: | 1.5 → master |
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.
comment:11 by , 11 years ago
Cc: | added |
---|
comment:12 by , 11 years ago
Cc: | added |
---|---|
Patch needs improvement: | unset |
Updated pull request without unrelated changes and tweaked test.
comment:13 by , 11 years ago
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?
comment:14 by , 11 years ago
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).
comment:15 by , 11 years ago
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.
comment:16 by , 11 years ago
Patch needs improvement: | set |
---|
comment:17 by , 11 years ago
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.
comment:18 by , 11 years ago
[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.
comment:19 by , 11 years ago
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
comment:20 by , 11 years ago
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.
comment:21 by , 10 years ago
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?
comment:22 by , 7 years ago
Cc: | added |
---|
comment:23 by , 6 years ago
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.
comment:24 by , 3 years ago
Owner: | changed from | to
---|---|
Patch needs improvement: | unset |
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
). 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 for folks without a solution for MyModel.clean() or just wontfix.
Happy to hear advice or to be nudged toward the mailing list.
comment:25 by , 3 years ago
Resolution: | → wontfix |
---|---|
Status: | assigned → closed |
Per 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.
The original report is not very clear, so here's a way to reproduce the error that the OP is refering to:
Doing this, the model validation passes but the save throws an error:
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: