Opened 2 years ago

Last modified 13 months ago

#20205 assigned Bug

PositiveIntegerfield does not handle empty values well

Reported by: anonymous Owned by: AmiZya
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: bmispelon@…, amizya@…, eromijn@…, timograham@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
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)

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

Download all attachments as: .zip

Change History (23)

comment:1 Changed 2 years ago by anonymous

  • Component changed from Uncategorized to Forms
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 2 years ago by bmispelon

  • Cc bmispelon@… added
  • Component changed from Forms to Database 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:

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

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

foo.full_clean()

returns None, instead it should raise a ValidationError exception.

Last edited 2 years ago by AmiZya (previous) (diff)

comment:6 Changed 2 years ago by AmiZya

  • Cc amizya@… added
  • Owner changed from nobody to AmiZya
  • Status changed from new to assigned

Changed 2 years ago by areski

Patch for ticket 20205 fix for empty values allowed on IntegerField

comment:7 Changed 2 years ago 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. See patch in attachment. I will appreciate feedbacks

Last edited 2 years ago by areski (previous) (diff)

comment:8 Changed 2 years ago by rach

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

Changed 2 years ago by areski

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

comment:10 Changed 2 years ago by erikr

  • Has patch set
  • Patch needs improvement set
  • Summary changed from positiveintegerfield null=True, blank=True to PositiveIntegerfield does not handle empty values well
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Bug
  • Version changed from 1.5 to 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 Changed 2 years ago by erikr

  • Cc eromijn@… added

comment:12 Changed 2 years ago by timo

  • Cc timograham@… added
  • Patch needs improvement unset

Updated pull request without unrelated changes and tweaked test.

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

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

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

comment:16 Changed 2 years ago by timo

  • Patch needs improvement set

comment:17 Changed 2 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 2 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 2 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):
    pass
    
    
class SmallIntegerField(FixedIntegerMixin, SmallIntegerField_django):
    pass

    
class PositiveIntegerField(FixedIntegerMixin, PositiveIntegerField_django):
    pass


class PositiveSmallIntegerField(FixedIntegerMixin, PositiveSmallIntegerField_django):
    pass

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

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

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