Opened 5 months ago

Last modified 7 weeks ago

#28431 assigned Bug

default='' (non-bytestring) on BinaryField crashes some migration operations

Reported by: James Owned by: Windson yang
Component: Migrations Version: 1.10
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by James)

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 = '')
  1. makemigrations firstapp
  2. migrate firstapp 0001
  3. Modify firstapp/models.py
class TableOne(models.Model):
    field1 = models.BinaryField(default = b'')
  1. migrate firstapp 0002
  2. 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

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)

Change History (5)

comment:1 Changed 5 months ago by James

Description: modified (diff)

comment:2 Changed 5 months ago by Tim Graham

Summary: Default value for BinaryField in reverse migrationdefault='' (non-bytestring) on BinaryField crashes some migration operations
Triage Stage: UnreviewedAccepted

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.

comment:3 Changed 4 months ago by Windson yang

Owner: changed from nobody to Windson yang
Status: newassigned

comment:4 Changed 4 months ago by Windson yang

It didn't crash when I using sqlite3, maybe related to Postgres, right?

comment:5 Changed 7 weeks ago 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']
Note: See TracTickets for help on using tickets.
Back to Top