#25417 closed New feature (fixed)
Add a system check for an invalid default on a model field
Reported by: | André Avorio | Owned by: | Simon Charette |
---|---|---|---|
Component: | Core (System checks) | Version: | dev |
Severity: | Normal | Keywords: | decimal, InvalidOperation, migrations |
Cc: | Simon Charette | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | yes |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
Hello,
I'm using Django version 1.8.4 and Python 3.4.3.
I created this field:
tax_rate = models.DecimalField('tax rate (%)', max_digits=3, decimal_places=2, help_text='VAT rate', blank=True, null=True, default=20.00)
I then ran python manage.py migrate
and got the database table created. All good.
I then decided I wanted more digits, so I changed that field to:
tax_rate = models.DecimalField('tax rate (%)', max_digits=5, decimal_places=2, help_text='VAT rate', blank=True, null=True, default=20.00)
Now, when I run python manage.py migrate
I get the following error:
File "manage.py", line 10, in <module> execute_from_command_line(sys.argv) File "django-project/.venv/lib/python3.4/site-packages/django/core/management/__init__.py", line 338, in execute_from_command_line utility.execute() File "django-project/.venv/lib/python3.4/site-packages/django/core/management/__init__.py", line 330, in execute self.fetch_command(subcommand).run_from_argv(self.argv) File "django-project/.venv/lib/python3.4/site-packages/django/core/management/base.py", line 393, in run_from_argv self.execute(*args, **cmd_options) File "django-project/.venv/lib/python3.4/site-packages/django/core/management/base.py", line 444, in execute output = self.handle(*args, **options) File "django-project/.venv/lib/python3.4/site-packages/django/core/management/commands/migrate.py", line 222, in handle executor.migrate(targets, plan, fake=fake, fake_initial=fake_initial) File "django-project/.venv/lib/python3.4/site-packages/django/db/migrations/executor.py", line 110, in migrate self.apply_migration(states[migration], migration, fake=fake, fake_initial=fake_initial) File "django-project/.venv/lib/python3.4/site-packages/django/db/migrations/executor.py", line 148, in apply_migration state = migration.apply(state, schema_editor) File "django-project/.venv/lib/python3.4/site-packages/django/db/migrations/migration.py", line 115, in apply operation.database_forwards(self.app_label, schema_editor, old_state, project_state) File "django-project/.venv/lib/python3.4/site-packages/django/db/migrations/operations/fields.py", line 201, in database_forwards schema_editor.alter_field(from_model, from_field, to_field) File "django-project/.venv/lib/python3.4/site-packages/django/db/backends/base/schema.py", line 484, in alter_field old_db_params, new_db_params, strict) File "django-project/.venv/lib/python3.4/site-packages/django/db/backends/base/schema.py", line 566, in _alter_field old_default = self.effective_default(old_field) File "django-project/.venv/lib/python3.4/site-packages/django/db/backends/base/schema.py", line 211, in effective_default default = field.get_db_prep_save(default, self.connection) File "django-project/.venv/lib/python3.4/site-packages/django/db/models/fields/__init__.py", line 1627, in get_db_prep_save self.max_digits, self.decimal_places) File "django-project/.venv/lib/python3.4/site-packages/django/db/backends/base/operations.py", line 477, in value_to_db_decimal return utils.format_number(value, max_digits, decimal_places) File "django-project/.venv/lib/python3.4/site-packages/django/db/backends/utils.py", line 200, in format_number value = value.quantize(decimal.Decimal(".1") ** decimal_places, context=context) decimal.InvalidOperation: [<class 'decimal.InvalidOperation'>]
Is this an actual bug, or am I doing something I shouldn't be?
Change History (27)
comment:1 by , 9 years ago
comment:2 by , 9 years ago
I guess an extra check wouldn't hurt here.
I thought adding Field.check_default
method running field.validate(field.get_default(), None)
and making sure no ValidationError
is raised would make sense but in this case the float is silently converted to a decimal.Decimal
.
>>> from django.db.models import DecimalField >>> DecimalField().validate(20.5, None) >>> DecimalField().to_python(20.5) Decimal('20.5')
comment:3 by , 9 years ago
Thank you for your contributions, @collinanderson @charettes.
How do I fix this? I can't move ahead with this app as Migrations is crashing.
I tried editing the migrations file and replaced default=20.0
with default=Decimal('20.0')
, as in:
operations = [ migrations.AlterField( model_name='quoteitem', name='tax_rate', field=models.DecimalField(blank=True, decimal_places=2, max_digits=5, default=Decimal('20.0'), help_text='VAT rate', verbose_name='tax rate (%)', null=True), ), ]
But the error persists, even after adding from decimal import *
to it.
Any ideas on how I could fix this so my Migrations file will be processed properly?
Many thanks!
comment:4 by , 9 years ago
Resolution: | → needsinfo |
---|---|
Status: | new → closed |
I couldn't reproduce against master and 1.8.4 on Python 3.4 with SQLite with the following steps:
- Create an app with a single model with a decimal like specified above;
- Create the initial migration for the app and run it;
- Change the model field like specified above;
- Create the new migration and run it.
Which database backend are you using?
comment:5 by , 9 years ago
I just reproduced this error again on a fresh new Django project using Python 3.4.3 and Django 1.8.4. I followed exactly the same steps using django.db.backends.postgresql_psycopg2
as the database backend instead.
Just to clarify, the only change in the field is from max_digits=3
to max_digits=5
. The default value continues the same default=20.00
(which, by the way, would not be compatible with the first max_digits
value).
Here's the first migration:
# -*- coding: utf-8 -*- from __future__ import unicode_literals from django.db import models, migrations class Migration(migrations.Migration): dependencies = [ ] operations = [ migrations.CreateModel( name='Item', fields=[ ('id', models.AutoField(auto_created=True, primary_key=True, verbose_name='ID', serialize=False)), ('tax_rate', models.DecimalField(help_text='VAT rate', decimal_places=2, blank=True, null=True, max_digits=3, default=20.0, verbose_name='tax rate (%)')), ], ), ]
And here's the second:
# -*- coding: utf-8 -*- from __future__ import unicode_literals from django.db import models, migrations class Migration(migrations.Migration): dependencies = [ ('testapp', '0001_initial'), ] operations = [ migrations.AlterField( model_name='item', name='tax_rate', field=models.DecimalField(decimal_places=2, help_text='VAT rate', verbose_name='tax rate (%)', default=20.0, max_digits=5, blank=True, null=True), ), ]
And here's the output of migrations:
(.venv)➜ mysite python manage.py makemigrations Migrations for 'testapp': 0001_initial.py: - Create model Item (.venv)➜ mysite python manage.py migrate Operations to perform: Synchronize unmigrated apps: messages, staticfiles Apply all migrations: apython manage.py migratein, auth, contenttypes, sessions, testapp Synchronizing apps without migrations: Creating tables... Running deferred SQL... Installing custom SQL... Running migrations: Rendering model states... DONE Applying testapp.0001_initial... OK (.venv)➜ mysite python manage.py makemigrations Migrations for 'testapp': 0002_auto_20150917_0951.py: - Alter field tax_rate on item (.venv)➜ mysite python manage.py migrate Operations to perform: Synchronize unmigrated apps: messages, staticfiles Apply all migrations: testapp, contenttypes, auth, sessions, apython manage.py migratein Synchronizing apps without migrations: Creating tables... Running deferred SQL... Installing custom SQL... Running migrations: Rendering model states... DONE Applying testapp.0002_auto_20150917_0951...Traceback (most recent call last): File "manage.py", line 10, in <module> execute_from_command_line(sys.argv) File "/new-django-project/.venv/lib/python3.4/site-packages/django/core/management/__init__.py", line 338, in execute_from_command_line utility.execute() File "/new-django-project/.venv/lib/python3.4/site-packages/django/core/management/__init__.py", line 330, in execute self.fetch_command(subcommand).run_from_argv(self.argv) File "/new-django-project/.venv/lib/python3.4/site-packages/django/core/management/base.py", line 393, in run_from_argv self.execute(*args, **cmd_options) File "/new-django-project/.venv/lib/python3.4/site-packages/django/core/management/base.py", line 444, in execute output = self.handle(*args, **options) File "/new-django-project/.venv/lib/python3.4/site-packages/django/core/management/commands/migrate.py", line 222, in handle executor.migrate(targets, plan, fake=fake, fake_initial=fake_initial) File "/new-django-project/.venv/lib/python3.4/site-packages/django/db/migrations/executor.py", line 110, in migrate self.apply_migration(states[migration], migration, fake=fake, fake_initial=fake_initial) File "/new-django-project/.venv/lib/python3.4/site-packages/django/db/migrations/executor.py", line 148, in apply_migration state = migration.apply(state, schema_editor) File "/new-django-project/.venv/lib/python3.4/site-packages/django/db/migrations/migration.py", line 115, in apply operation.database_forwards(self.app_label, schema_editor, old_state, project_state) File "/new-django-project/.venv/lib/python3.4/site-packages/django/db/migrations/operations/fields.py", line 201, in database_forwards schema_editor.alter_field(from_model, from_field, to_field) File "/new-django-project/.venv/lib/python3.4/site-packages/django/db/backends/base/schema.py", line 484, in alter_field old_db_params, new_db_params, strict) File "/new-django-project/.venv/lib/python3.4/site-packages/django/db/backends/base/schema.py", line 566, in _alter_field old_default = self.effective_default(old_field) File "/new-django-project/.venv/lib/python3.4/site-packages/django/db/backends/base/schema.py", line 211, in effective_default default = field.get_db_prep_save(default, self.connection) File "/new-django-project/.venv/lib/python3.4/site-packages/django/db/models/fields/__init__.py", line 1627, in get_db_prep_save self.max_digits, self.decimal_places) File "/new-django-project/.venv/lib/python3.4/site-packages/django/db/backends/base/operations.py", line 477, in value_to_db_decimal return utils.format_number(value, max_digits, decimal_places) File "/new-django-project/.venv/lib/python3.4/site-packages/django/db/backends/utils.py", line 200, in format_number value = value.quantize(decimal.Decimal(".1") ** decimal_places, context=context) decimal.InvalidOperation: [<class 'decimal.InvalidOperation'>]
comment:6 by , 9 years ago
Resolution: | needsinfo |
---|---|
Status: | closed → new |
comment:7 by , 9 years ago
Severity: | Release blocker → Normal |
---|
I believe you should edit your models.py
to specify default=Decimal('20.0')
as well.
comment:8 by , 9 years ago
Cc: | added |
---|---|
Summary: | decimal.InvalidOperation caused by Migrations and DecimalField() → No warning raised when an invalid default is specified for a field |
Triage Stage: | Unreviewed → Accepted |
I also managed to reproduce against 1.8.4 with Python 3.4 and PostgreSQL.
The issue is the first migration having an invalid field, 20.00
can't be stored with three digits.
The reason an exception was not raised during the initial migration generation and executing is that the default value is not used at table creation. It would have also raised an exception if the field has been added instead of altered.
I suggest we fix this by adding a field check that makes sure the default value of a field is valid. We should also make sure that DecimalField
validation takes max_digits
and decimal_places
into account.
In the meanwhile you can work around this issue by editing the migration containing the invalid field (max_digits=3, decimal_places=2, default=20.00
) to have a valid default value e.g. decimal.Decimal('2.00')
.
comment:9 by , 9 years ago
Component: | Migrations → Core (System checks) |
---|---|
Type: | Bug → New feature |
#24636 is about adding the validation you described. Moving this to a "System checks" issue to take care of the rest.
comment:10 by , 9 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Version: | 1.8 → master |
I've got a PR based on #24636 changes coming.
comment:12 by , 9 years ago
Patch needs improvement: | unset |
---|
comment:13 by , 9 years ago
Summary: | No warning raised when an invalid default is specified for a field → Add a system check for an invalid default on a model field |
---|---|
Triage Stage: | Accepted → Ready for checkin |
comment:15 by , 9 years ago
Needs documentation: | set |
---|---|
Patch needs improvement: | set |
Resolution: | fixed |
Status: | closed → new |
This change is not backwards compatible and isn't a valid fix for the original issue. Validation in Django only happens through forms, and is bypassed when manipulating a model directly. The precedent in Django is to allow creation of models that don't actually validate.
I use fields with blank=False, default=''
to create a model with empty content that's only validated when a user edits it through a form.
I don't understand how this is a Django bug at all. The opener of the bug is doing something wrong with Decimals, and got a decimal.InvalidOperation error. That seems like what should happen.
comment:16 by , 9 years ago
Thanks for the raising the issue. I figured there would probably be some use case where this check would cause problems, however, I don't think it's very common to intentionally create models that don't validate. Do you have any problem adding this check to SILENCED_SYSTEM_CHECKS
? We could downgrade the check from "error" to "warning" or discuss it further on the DevelopersMailingList.
comment:17 by , 9 years ago
Validation in Django only happens through forms, and is bypassed when manipulating a model directly.
That's not true, there's a whole documented layer of model validation.
The precedent in Django is to allow creation of models that don't actually validate.
You can still create model instances that don't validate and even save them if you'd like. The only thing that changed here is that you cannot tell Django to assign an invalid default value at instantiation time.
I use fields with
blank=False, default=''
to create a model with empty content that's only validated when a user edits it through a form.
I'm not entirely sure I understand your use case but wouldn't this special handling belong at the form level anyway? I think the whole point of the separation of concerns between the model and the form layer is to be able to have enough flexibility at this level.
I don't understand how this is a Django bug at all. The opener of the bug is doing something wrong with Decimals, and got a decimal.InvalidOperation error. That seems like what should happen.
The whole point of model field checks is to prevent you from doing the wrong thing by mistake and confirm you know what you are doing by silencing false positives.
#25480 is an example where a simple configuration mistake was caught by this check.
Like Tim said you can silence this check if you want to rely on having a blank=False, default=''
field but don't be surprised if something breaks in Django further down the road.
comment:18 by , 9 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:19 by , 9 years ago
Do you have any problem adding this check to SILENCED_SYSTEM_CHECKS?
Yes. This is in a reusable app. I can't make my users silence the check in order to use it.
That's not true, there's a whole documented layer of model validation.
This validation is only used when called explicitly or when using a ModelForm. I expect a validation error from an initial value only to be raised if I ask if the model is valid.
I'm not entirely sure I understand your use case but wouldn't this special handling belong at the form level anyway?
I need to be able to create the model instances without a form. A form is only used later, when the user is ready to edit the object. The initial object is created without user interaction.
I'll also note that this isn't even the first time I've had to work around a buggy, overzealous, or plain wrong system check.
comment:20 by , 9 years ago
I'll also note that this isn't even the first time I've had to work around a buggy, overzealous, or plain wrong system check.
The system check is a still a new component of Django and from the feedback we've had so far I think the prevention of misconfiguration and footguns outweigh the nuisance caused by some initial bugs and required adjustments.
While I could be convinced we should remove or downgrade this check to a warning your actual use case doesn't convince me it's worth it. I still think this check is not buggy, overzealous nor plain wrong but that it's your use case that is bug prone.
As Tim suggested I think gathering feedback from the developpers mailing list could help your case here.
In the meantime if want to work around this check you can override the check()
method of your reusable field to filter out 'fields.E008'
errors.
comment:24 by , 9 years ago
Just FTR, I'm not totally sure if this is a valid use case, but for me this check (as published in Django 1.9 Beta 1) resulted in an error when running "manage.py check" or "manage.py migrate" on an empty database, when a model A defines a ForeignKey field to model B where the default is a certain value from the model B table. As in Django 1.9b1 the check tried to fetch the model B object from the still empty database, resulting in a "table does not exist" error.
comment:25 by , 9 years ago
Yeah 71ebcb8 seems naive. Trying to validate the default value causes exceptions under certain scenarios, which means more than just a warning, it prevents the initial migrations from being run. The default value might only be valid after migrations are finished, e.g.:
a) dynamic default values that rely on the database, e.g.:
ref = models.CharField(unique=True, max_length=200, default=rectification_sequence_current, blank=True)
rectification_sequence_current is a method that checks the sequence in the database. This throws as uncaught exception that the table doesn't exist.
b) defaults that rely on a migration creating a row, e.g:
pricetier = models.ForeignKey('PriceTier', default=0)
the initial pricetier is created by the first migration, but that won't run, because the check does a database query against a table that doesn't exist.
comment:27 by , 8 years ago
I closed #27684 as a duplicate. It's a similar problem where a string (rather than date
) was incorrectly assigned as the default for DateField
.
It needs to be default=Decimal('20.00'), otherwise 20.00 is a float and is incompatible with decimal. I wonder if it would make sense to have a warning when the default is specified as a float.