[Django] #30371: sqlmigrate fails with string defaults on mysql

Django

unread,

Apr 15, 2019, 11:01:52 AM4/15/19

to django-...@googlegroups.com

#30371: sqlmigrate fails with string defaults on mysql
-------------------------------------+-------------------------------------
Reporter: Melvyn | Owner: nobody
Sopacua |
Type: Bug | Status: new
Component: Database | Version: 2.1
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
When we have a migration that is adding strings defaults to a field,
sqlmigrate will fail:

{{{
File "/home/melvyn/.local/py-env/button3d-7LYQbQGk/lib/python3.6/site-
packages/django/db/migrations/migration.py", line 124, in apply
operation.database_forwards(self.app_label, schema_editor, old_state,
project_state)
File "/home/melvyn/.local/py-env/button3d-7LYQbQGk/lib/python3.6/site-
packages/django/db/migrations/operations/fields.py", line 84, in
database_forwards
field,
File "/home/melvyn/.local/py-env/button3d-7LYQbQGk/lib/python3.6/site-
packages/django/db/backends/mysql/schema.py", line 42, in add_field
super().add_field(model, field)
File "/home/melvyn/.local/py-env/button3d-7LYQbQGk/lib/python3.6/site-
packages/django/db/backends/base/schema.py", line 435, in add_field
self.execute(sql, params)
File "/home/melvyn/.local/py-env/button3d-7LYQbQGk/lib/python3.6/site-
packages/django/db/backends/base/schema.py", line 128, in execute
self.collected_sql.append((sql % tuple(map(self.quote_value, params)))
+ ending)
File "/home/melvyn/.local/py-env/button3d-7LYQbQGk/lib/python3.6/site-
packages/django/db/backends/mysql/schema.py", line 31, in quote_value
quoted = quoted.decode()
AttributeError: 'str' object has no attribute 'decode'
}}}

This is because of the quote_value method calling `decode()` on a string
object (it is a method of a byte object). This doesn't happen when the
migration is being applied, presumably, because then we're creating byte
objects and the code doesn't trigger, but this is a guess.

--
Ticket URL: <https://code.djangoproject.com/ticket/30371>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

Django

unread,

Apr 16, 2019, 2:29:53 AM4/16/19

to django-...@googlegroups.com

#30371: sqlmigrate fails with string defaults on mysql
-------------------------------------+-------------------------------------

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Claude Paroz):

* keywords: => regression
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted

Comment:

Regression in [3c4ff2176323dd20507e35658599da220fbe1741].

--
Ticket URL: <https://code.djangoproject.com/ticket/30371#comment:1>

Django

unread,

Apr 16, 2019, 2:39:59 AM4/16/19

to django-...@googlegroups.com

#30371: sqlmigrate fails with string defaults on mysql
-------------------------------------+-------------------------------------

Reporter: Melvyn Sopacua | Owner: nobody
Type: Bug | Status: new

Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: regression | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Claude Paroz):

Melvyn, could you tell us what's the type of your default value? Please
also tell us your MySQL and mysqlclient version.

--
Ticket URL: <https://code.djangoproject.com/ticket/30371#comment:2>

Django

unread,

Apr 17, 2019, 2:30:13 AM4/17/19

to django-...@googlegroups.com

#30371: sqlmigrate fails with string defaults on mysql
-------------------------------------+-------------------------------------

Reporter: Melvyn Sopacua | Owner: nobody

Type: Bug | Status: closed

Component: Database layer | Version: 2.1
(models, ORM) |

Severity: Release blocker | Resolution: needsinfo

Keywords: regression | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Claude Paroz):

* status: new => closed
* resolution: => needsinfo

Comment:

I was unable to reproduce until now, we need more information, please.

--
Ticket URL: <https://code.djangoproject.com/ticket/30371#comment:3>

Django

unread,

Apr 17, 2019, 4:46:54 AM4/17/19

to django-...@googlegroups.com

#30371: sqlmigrate fails with string defaults on mysql
-------------------------------------+-------------------------------------

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Melvyn Sopacua):

If you can't reproduce it, you have dead code, cause str.decode() simply
isn't a thing.

However, it may be caused by an extension. I will have time on Friday to
investigate.

--
Ticket URL: <https://code.djangoproject.com/ticket/30371#comment:4>

Django

unread,

Apr 17, 2019, 12:36:23 PM4/17/19

to django-...@googlegroups.com

#30371: sqlmigrate fails with string defaults on mysql
-------------------------------------+-------------------------------------

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Claude Paroz):

The code counts on the fact that `connection.escape()` will return a
bytestring for a `str` value. Note the `isinstance` test is done on
`value` but the `decode()` is called on `quoted`.

--
Ticket URL: <https://code.djangoproject.com/ticket/30371#comment:5>

Django

unread,

Apr 18, 2019, 12:48:01 PM4/18/19

to django-...@googlegroups.com

#30371: sqlmigrate fails with string defaults on mysql
-------------------------------------+-------------------------------------

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Melvyn Sopacua):

Thanks!
Note to self: test 2 cases:
- migrations without any extensions and/or foreign migration operations
- compare mysql driver versions: pymysql 0.9.3

Also check connection.escape().

--
Ticket URL: <https://code.djangoproject.com/ticket/30371#comment:6>

Django

unread,

Apr 19, 2019, 3:16:47 AM4/19/19

to django-...@googlegroups.com

#30371: sqlmigrate fails with string defaults on mysql
-------------------------------------+-------------------------------------

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Claude Paroz):

Replying to [comment:6 Melvyn Sopacua]:

> - compare mysql driver versions: pymysql 0.9.3

Ah, ah... that is probably the explanation. It might be that pymysql
doesn't return a bytestring from `quote()` which would explain the issue
(to be confirmed). Django doesn't officially support pymysql (read also
#30380). It's another issue where we might call `force_text` instead of
`decode()`. However this section is a bit more performance-sensitive.

--
Ticket URL: <https://code.djangoproject.com/ticket/30371#comment:7>

Django

unread,

Apr 21, 2019, 5:31:46 AM4/21/19

to django-...@googlegroups.com

#30371: sqlmigrate fails with string defaults on mysql
-------------------------------------+-------------------------------------

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Melvyn Sopacua):

I think the proper fix here is to confirm both str on value and 'decode'
attribute on quoted.

I've tried to add a test for it, but the code never gets called anymore in
the scope of migrations, because Django no longer transmits default values
to the backend. So the issue is with [https://github.com/3YOURMIND/django-
add-default-value our extension] that ensures we can do blue/green
deployments. Looking at
[https://github.com/PyMySQL/PyMySQL/blob/master/pymysql/converters.py this
code] confirms your suspicions that pymysql returns strings.

Still - when I read the code the first few times until you highlighted the
fact here, I thought the if clause and the modification are using the same
value, cause it's such a common pattern you don't read the variable name
anymore.
And secondly, it assumes that quoted has the decode() method based on a
not related test, so I still suggest to fix it like this:

{{{
if isinstance(value, str) and callable(getattr(quoted, 'decode', None)):
quoted = quoted.decode()
}}}

This doesn't require the performance hit of force_text and it even
supports exotic quoted values that implement a decode() method.

--
Ticket URL: <https://code.djangoproject.com/ticket/30371#comment:8>

Django

unread,

Apr 21, 2019, 5:38:23 AM4/21/19

to django-...@googlegroups.com

#30371: sqlmigrate fails with string defaults on mysql
-------------------------------------+-------------------------------------

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------