Opened 7 years ago

Last modified 2 years ago

#20205 assigned Bug

PositiveIntegerfield does not handle empty values well

Reported by: anonymous Owned by: Amine Zyad
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: bmispelon@…, amizya@…, eromijn@…, timograham@…, Zach Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no


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/ in get_prep_value, line 991

It seems an update to IntegerField's get_prep_value or to_python may help.

Attachments (2)

patch_ticket20205.txt (436 bytes) - added by Areski Belaid 7 years ago.
Patch for ticket 20205 fix for empty values allowed on IntegerField
patch_ticket_20205_b.txt (11.2 KB) - added by Areski Belaid 7 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 7 years ago by anonymous

Component: UncategorizedForms

comment:2 Changed 7 years ago by Baptiste Mispelon

Cc: bmispelon@… added
Component: FormsDatabase layer (models, ORM)

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

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='')

except ValidationError as error:

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.

comment:3 Changed 7 years ago 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.

comment:4 Changed 7 years ago 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.

comment:5 Changed 7 years ago by Amine Zyad


returns None, instead it should raise a ValidationError exception.

Last edited 7 years ago by Amine Zyad (previous) (diff)

comment:6 Changed 7 years ago by Amine Zyad

Cc: amizya@… added
Owner: changed from nobody to Amine Zyad
Status: newassigned

Changed 7 years ago by Areski Belaid

Attachment: patch_ticket20205.txt added

Patch for ticket 20205 fix for empty values allowed on IntegerField

comment:7 Changed 7 years ago by 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

Last edited 7 years ago by Areski Belaid (previous) (diff)

comment:8 Changed 7 years ago by rach

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

Changed 7 years ago by Areski Belaid

Attachment: patch_ticket_20205_b.txt added

comment:9 Changed 7 years ago by Areski Belaid

thanks I added new patch and updated the pull request:
I added some cleaning of the test file.

comment:10 Changed 7 years ago by Sasha Romijn

Has patch: set
Patch needs improvement: set
Summary: positiveintegerfield null=True, blank=TruePositiveIntegerfield does not handle empty values well
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug
Version: 1.5master

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 Changed 7 years ago by Sasha Romijn

Cc: eromijn@… added

comment:12 Changed 7 years ago by Tim Graham

Cc: timograham@… added
Patch needs improvement: unset

Updated pull request without unrelated changes and tweaked test.

comment:13 Changed 7 years ago 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?

comment:14 Changed 7 years ago by 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).

comment:15 Changed 7 years ago by 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.

comment:16 Changed 7 years ago by Tim Graham

Patch needs improvement: set

comment:17 Changed 7 years ago 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.

comment:18 Changed 7 years ago 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.

comment:19 Changed 7 years ago 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):
class SmallIntegerField(FixedIntegerMixin, SmallIntegerField_django):

class PositiveIntegerField(FixedIntegerMixin, PositiveIntegerField_django):

class PositiveSmallIntegerField(FixedIntegerMixin, PositiveSmallIntegerField_django):

comment:20 Changed 7 years ago by 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.

comment:21 Changed 6 years ago by 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?

comment:22 Changed 3 years ago by Zach

Cc: Zach added

comment:23 Changed 2 years ago 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.

Note: See TracTickets for help on using tickets.
Back to Top