Opened 7 years ago

Closed 6 years ago

#28431 closed Bug (fixed)

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

Reported by: James Owned by: Hasan Ramezani
Component: Core (System checks) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes 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 (8)

comment:1 by James, 7 years ago

Description: modified (diff)

comment:2 by Tim Graham, 7 years ago

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 by Windson yang, 7 years ago

Owner: changed from nobody to Windson yang
Status: newassigned

comment:4 by Windson yang, 7 years ago

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

comment:5 by Claude Paroz, 7 years ago

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']

comment:6 by Hasan Ramezani, 6 years ago

Has patch: set
Owner: changed from Windson yang to Hasan Ramezani

System check for preventing default string value for BinaryField added based on @Claude Paroz suggestion PR

Last edited 6 years ago by Tim Graham (previous) (diff)

comment:7 by Mariusz Felisiak, 6 years ago

Component: MigrationsCore (System checks)
Triage Stage: AcceptedReady for checkin
Version: 1.10master

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 6 years ago

Resolution: fixed
Status: assignedclosed

In 981dd6dd:

Fixed #28431 -- Added a system check for BinaryField to prevent strings defaults.

Thanks Claude Paroz for the initial patch.

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