[Django] #28431: Default value for BinaryField in reverse migration
Django
--------------------------------------+------------------------
Reporter: sg-james | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
--------------------------------------+------------------------
== Description ==
Initial migration has a default value {{{''}}} for BinaryField.
Later, change default value to {{{b''}}} and migrate.
Trying to undo this migration fails. It seems like {{{''}}} is allowed
during migration, but not in reverse migration.
== Related issue ==
#22851 Default value for BinaryField
== Reproduce ==
Python 3.6.0, Django 1.10.6, Postgres 9.5.4
1. startproject djangoproject
2. startapp firstapp
3. firstapp/models.py:
{{{
class TableOne(models.Model):
field1 = models.BinaryField(default = '')
}}}
4. makemigrations firstapp
5. migrate firstapp 0001
5. Modify firstapp/models.py
{{{
class TableOne(models.Model):
field1 = models.BinaryField(default = b'')
}}}
6. migrate firstapp 0002
7. migrate firstapp 0001
Error: TypeError: can't escape str to binary
{{{
Traceback (most recent call last):
File "manage.py", line 22, in <module>
execute_from_command_line(sys.argv)
File "C:\Py\py3_64\lib\site-
packages\django\core\management\__init__.py", line 367, in
execute_from_command_line
utility.execute()
File "C:\Py\py3_64\lib\site-
packages\django\core\management\__init__.py", line 359, in execute
self.fetch_command(subcommand).run_from_argv(self.argv)
File "C:\Py\py3_64\lib\site-packages\django\core\management\base.py",
line 294, in run_from_argv
self.execute(*args, **cmd_options)
File "C:\Py\py3_64\lib\site-packages\django\core\management\base.py",
line 345, in execute
output = self.handle(*args, **options)
File "C:\Py\py3_64\lib\site-
packages\django\core\management\commands\migrate.py", line 204, in handle
fake_initial=fake_initial,
File "C:\Py\py3_64\lib\site-packages\django\db\migrations\executor.py",
line 119, in migrate
state = self._migrate_all_backwards(plan, full_plan, fake=fake)
File "C:\Py\py3_64\lib\site-packages\django\db\migrations\executor.py",
line 194, in _migrate_all_backwards
self.unapply_migration(states[migration], migration, fake=fake)
File "C:\Py\py3_64\lib\site-packages\django\db\migrations\executor.py",
line 264, in unapply_migration
state = migration.unapply(state, schema_editor)
File "C:\Py\py3_64\lib\site-packages\django\db\migrations\migration.py",
line 178, in unapply
operation.database_backwards(self.app_label, schema_editor,
from_state, to_state)
File "C:\Py\py3_64\lib\site-
packages\django\db\migrations\operations\fields.py", line 210, in
database_backwards
self.database_forwards(app_label, schema_editor, from_state, to_state)
File "C:\Py\py3_64\lib\site-
packages\django\db\migrations\operations\fields.py", line 205, in
database_forwards
schema_editor.alter_field(from_model, from_field, to_field)
File "C:\Py\py3_64\lib\site-packages\django\db\backends\base\schema.py",
line 506, in alter_field
old_db_params, new_db_params, strict)
File "C:\Py\py3_64\lib\site-
packages\django\db\backends\postgresql\schema.py", line 118, in
_alter_field
new_db_params, strict,
File "C:\Py\py3_64\lib\site-packages\django\db\backends\base\schema.py",
line 660, in _alter_field
params,
File "C:\Py\py3_64\lib\site-packages\django\db\backends\base\schema.py",
line 112, in execute
cursor.execute(sql, params)
File "C:\Py\py3_64\lib\site-packages\django\db\backends\utils.py", line
80, in execute
return super(CursorDebugWrapper, self).execute(sql, params)
File "C:\Py\py3_64\lib\site-packages\django\db\backends\utils.py", line
65, in execute
return self.cursor.execute(sql, params)
TypeError: can't escape str to binary
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28431>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: sg-james | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by sg-james:
Old description:
> File "C:\Py\py3_64\lib\site-
> packages\django\db\migrations\migration.py", line 178, in unapply
> operation.database_backwards(self.app_label, schema_editor,
> from_state, to_state)
> File "C:\Py\py3_64\lib\site-
> packages\django\db\migrations\operations\fields.py", line 210, in
> database_backwards
> self.database_forwards(app_label, schema_editor, from_state,
> to_state)
> File "C:\Py\py3_64\lib\site-
> packages\django\db\migrations\operations\fields.py", line 205, in
> database_forwards
> schema_editor.alter_field(from_model, from_field, to_field)
> File "C:\Py\py3_64\lib\site-
> packages\django\db\backends\base\schema.py", line 506, in alter_field
> old_db_params, new_db_params, strict)
> File "C:\Py\py3_64\lib\site-
> packages\django\db\backends\postgresql\schema.py", line 118, in
> _alter_field
> new_db_params, strict,
> File "C:\Py\py3_64\lib\site-
> packages\django\db\backends\base\schema.py", line 660, in _alter_field
> params,
> File "C:\Py\py3_64\lib\site-
> packages\django\db\backends\base\schema.py", line 112, in execute
> cursor.execute(sql, params)
> File "C:\Py\py3_64\lib\site-packages\django\db\backends\utils.py", line
> 80, in execute
> return super(CursorDebugWrapper, self).execute(sql, params)
> File "C:\Py\py3_64\lib\site-packages\django\db\backends\utils.py", line
> 65, in execute
> return self.cursor.execute(sql, params)
> TypeError: can't escape str to binary
> }}}
New description:
== Notes ==
site-packages\django\db\backends\base\shema.py {{{ def
effective_default(self, field): }}}
determines default as an empty <class 'str'>, when {{{(default = '')}}}
== Possible Fix? ==
site-packages\django\db\backends\base\shema.py ~line 197
{{{
def effective_default(self, field):
if field.has_default():
default = field.get_default()
if field.get_internal_type() == "BinaryField" and not default:
default = six.binary_type()
elif not field.null and field.blank and
field.empty_strings_allowed:
if field.get_internal_type() == "BinaryField":
default = six.binary_type()
else:
default = six.text_type()
elif getattr(field, 'auto_now', False)
}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/28431#comment:1>
Django
operations
----------------------------+------------------------------------
Reporter: James | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Unreviewed => Accepted
Comment:
While doing a quick test, I noticed that a non-bytestring also crashes
with `AddField` (forward). I'm not sure about the proper resolution
(perhaps a system check error for an invalid default?) but the
inconsistency is certainly unexpected.
--
Ticket URL: <https://code.djangoproject.com/ticket/28431#comment:2>
Django
operations
Reporter: James | Owner: Windson yang
Type: Bug | Status: assigned
Component: Migrations | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Windson yang):
* owner: nobody => Windson yang
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/28431#comment:3>
Django
operations
----------------------------+----------------------------------------
Reporter: James | Owner: Windson yang
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Windson yang):
It didn't crash when I using sqlite3, maybe related to Postgres, right?
--
Ticket URL: <https://code.djangoproject.com/ticket/28431#comment:4>
Django
operations
----------------------------+----------------------------------------
Reporter: James | Owner: Windson yang
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz):
I would also suggest a system check to prevent default strings in the
first place. Something like:
{{{
diff --git a/django/db/models/fields/__init__.py
b/django/db/models/fields/__init__.py
index b2e9b18351..af4671c0f6 100644
--- a/django/db/models/fields/__init__.py
+++ b/django/db/models/fields/__init__.py
@@ -2292,6 +2292,22 @@ class BinaryField(Field):
if self.max_length is not None:
self.validators.append(validators.MaxLengthValidator(self.max_length))
+ def check(self, **kwargs):
+ errors = super().check(**kwargs)
+ errors.extend(self._check_default_is_not_str(**kwargs))
+ return errors
+
+ def _check_default_is_not_str(self, **kwargs):
+ if self.has_default() and isinstance(self.default, str):
+ return [
+ checks.Error(
+ "BinaryField 'default' cannot be a string, use bytes
content instead.",
+ obj=self,
+ id='fields.E170',
+ )
+ ]
+ return []
+
def deconstruct(self):
name, path, args, kwargs = super().deconstruct()
del kwargs['editable']
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28431#comment:5>
Django
operations
Reporter: James | Owner: Hasan Ramezani
Type: Bug | Status: assigned
Component: Migrations | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Hasan Ramezani):
* owner: Windson yang => Hasan Ramezani
* has_patch: 0 => 1
Comment:
System check for preventing default string value for BinaryField added
based on @Claude Paroz suggestion
--
Ticket URL: <https://code.djangoproject.com/ticket/28431#comment:6>
Django
operations
Reporter: James | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* version: 1.10 => master
* component: Migrations => Core (System checks)
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/28431#comment:7>
Django
operations
-------------------------------------+-------------------------------------
Reporter: James | Owner: Hasan
| Ramezani
Component: Core (System | Version: master
checks) |
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"981dd6dd71ea80e5149c2eff564622e96c12b5be" 981dd6dd]:
{{{
#!CommitTicketReference repository=""
revision="981dd6dd71ea80e5149c2eff564622e96c12b5be"
Fixed #28431 -- Added a system check for BinaryField to prevent strings
defaults.
Thanks Claude Paroz for the initial patch.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28431#comment:8>