Code

Opened 15 months ago

Last modified 10 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 14 months ago.
Patch for ticket 20205 fix for empty values allowed on IntegerField
patch_ticket_20205_b.txt (11.2 KB) - added by areski 14 months ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 15 months ago by anonymous

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

comment:2 Changed 15 months 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 15 months 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 15 months 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 15 months ago by AmiZya

foo.full_clean()

returns None, instead it should raise a ValidationError exception.

Last edited 15 months ago by AmiZya (previous) (diff)

comment:6 Changed 14 months ago by AmiZya

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

Changed 14 months ago by areski

Patch for ticket 20205 fix for empty values allowed on IntegerField

comment:7 Changed 14 months 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 14 months ago by areski (previous) (diff)

comment:8 Changed 14 months ago by rach

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

Changed 14 months ago by areski

comment:9 Changed 14 months 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 14 months 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 14 months ago by erikr

  • Cc eromijn@… added

comment:12 Changed 14 months 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 14 months 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 14 months 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 14 months 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 14 months ago by timo

  • Patch needs improvement set

comment:17 Changed 13 months 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 13 months 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 10 months 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 10 months 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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as assigned
The owner will be changed from AmiZya to anonymous. Next status will be 'assigned'
The ticket will be disowned. Next status will be 'new'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.