PositiveIntegerfield does not handle empty values well

[anonymous]

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.

[Baptiste Mispelon]

The original report is not very clear, so here's a way to reproduce the error that the OP is refering to:

# 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.

[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.

[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.

[Amine Zyad]

foo.full_clean()

return None, instead it should raise a ValidationError exception.

[Areski Belaid]

Patch for ticket 20205 fix for empty values allowed on IntegerField

[Areski Belaid]

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

[rach]

Could you add a regression test in the pull request / patch submitted as bmispelon illustrated in his example.

[Areski Belaid]

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.

[Sasha Romijn]

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.

[Tim Graham]

Updated pull request without unrelated changes and tweaked test.

​https://github.com/django/django/pull/1213

[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?

[Tim Graham]

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).

[Aymeric Augustin]

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.

[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.

[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.

[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

[Simon Charette]

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.

[Anssi Kääriäinen]

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?

[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.

[Jacob Walls]

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.

[Jacob Walls]

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.