[Django] #30380: Support mysql query objects as strings in addition to bytes, for PyMySQL support
Django
support
-----------------------------------------+------------------------
Reporter: Nathan Klug | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 2.2
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 |
-----------------------------------------+------------------------
Per https://github.com/PyMySQL/PyMySQL/issues/790, Django 2.2 changed the
expected type of mysql queries; it's be great if we could support both
mysqlclient and PyMySQL here. More details from that issue:
Django 2.1.x used to cast every query into "str" like below
https://github.com/django/django/blob/stable/2.1.x/django/db/backends/mysql/operations.py#L134
Django 2.2.x they changed the code by using query.decode instead of
force_text method.
https://github.com/django/django/blob/stable/2.2.x/django/db/backends/mysql/operations.py#L140
Could you also help open a ticket on Django's issue tracking system ?
It's relatively easy to fix from Django side.
e.g.
{{{
from django.utils.encoding import force_text
def last_executed_query(self, cursor, sql, params):
# With MySQLdb, cursor objects have an (undocumented) "_executed"
# attribute where the exact query sent to the database is saved.
# See MySQLdb/cursors.py in the source distribution.
query = getattr(cursor, '_executed', None)
if query is not None:
if type(query) == bytes:
query = query.decode(errors='replace') # mysqlclient
elif type(query) == str:
query = query.encode(errors='replace') # PyMySQL
else:
query = force_text(query, errors='replace') # fallback
compatibility ?
return query
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30380>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Nathan Klug | Owner: nobody
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* status: new => closed
* resolution: => wontfix
* type: Uncategorized => New feature
* component: Uncategorized => Database layer (models, ORM)
Comment:
Thanks for this report, however Django doesn't support `PyMySQL` (see
#12500 and #22391).
--
Ticket URL: <https://code.djangoproject.com/ticket/30380#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Nathan Klug | Owner: nobody
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution: wontfix
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz):
Even if Django does not officially support PyMySQL, I find a bit
unfortunate that when a simple fix is available (reverting to `force_text`
usage), we don't even consider it. I think that if at some point PyMySQL
support is reachable with minimal efforts, it might be a good thing.
--
Ticket URL: <https://code.djangoproject.com/ticket/30380#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Nathan Klug | Owner: nobody
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution: wontfix
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by felixxm):
IMO it is just hard to tell in how many places we have broken `PyMySQL`
support because we don't have a CI build. Moreover even if we'll fix this
issue we cannot guarantee that we will not brake anything in the future :|
I'm torn.
--
Ticket URL: <https://code.djangoproject.com/ticket/30380#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Nathan Klug | Owner: nobody
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution: wontfix
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz):
Sure I understand, but the fact that this bug was reported may be a sign
that we are close to support PyMySQL. It may be time to reevaluate this
support.
--
Ticket URL: <https://code.djangoproject.com/ticket/30380#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Nathan Klug | Owner: nobody
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution: wontfix
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
I agree with Claude here, even if we don't have explicit support confirmed
by CI we should try to stay as compatible as possible with PyMySQL.
It already does as a pretty good job at exposing a compatible interface
through `pymysql.install_as_MySQLdb()` and in this particular we're using
an undocumented API and expecting it to behave in a certain weird way.
FWIW we [https://docs.djangoproject.com/en/2.2/ref/databases/#mysql-db-
api-drivers document we support Oracle's connector] but we don't run CI
against it and last time I checked support was completely broken and we
had to switch to `PyMySQL` for a project where `mysqlclient` could not be
used.
--
Ticket URL: <https://code.djangoproject.com/ticket/30380#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Nathan Klug | Owner: nobody
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* status: closed => new
* type: New feature => Bug
* version: 2.2 => master
* resolution: wontfix =>
* stage: Unreviewed => Accepted
Comment:
ok, agreed. I will partly revert efd8a82e268a82b3ad0be77bd5b4548c30bcb4d7
tomorrow. I can also check if any other test is broken on PyMySQL.
--
Ticket URL: <https://code.djangoproject.com/ticket/30380#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Nathan Klug | Owner: felixxm
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* status: new => assigned
* owner: nobody => felixxm
--
Ticket URL: <https://code.djangoproject.com/ticket/30380#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Nathan Klug | Owner: felixxm
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/11255 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/30380#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Nathan Klug | Owner: felixxm
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by GitHub <noreply@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"a41b09266dcdd01036d59d76fe926fe0386aaade" a41b092]:
{{{
#!CommitTicketReference repository=""
revision="a41b09266dcdd01036d59d76fe926fe0386aaade"
Fixed #30380 -- Handled bytes in MySQL backend for PyMySQL support.
This commit partly reverts efd8a82e268a82b3ad0be77bd5b4548c30bcb4d7.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30380#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Nathan Klug | Owner: felixxm
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tobias Krönke):
Is the partial revert correct? The comment now reads
{{{
cursor objects have an (undocumented) "_executed"
}}}
but the actual code gets
{{{
'_last_executed'
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30380#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Nathan Klug | Owner: felixxm
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by felixxm):
Thanks Tobias, good catch! I added
[https://github.com/django/django/pull/11259 PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/30380#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Nathan Klug | Owner: felixxm
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by GitHub <noreply@…>):
In [changeset:"994a00eb70969e4fd8f7a30a95122e2f0411ff48" 994a00e]:
{{{
#!CommitTicketReference repository=""
revision="994a00eb70969e4fd8f7a30a95122e2f0411ff48"
Refs #30380 -- Used cursor._executed in
DatabaseOperations.last_executed_query() on MySQL.
Regression in a41b09266dcdd01036d59d76fe926fe0386aaade.
Thanks Tobias Krönke for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30380#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Nathan Klug | Owner: felixxm
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by quantum5):
Is there a plan to backport this change into the 2.2.x branch? It would be
nice if this fix could be released in Django 2.2.1.
--
Ticket URL: <https://code.djangoproject.com/ticket/30380#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Nathan Klug | Owner: felixxm
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by eavictor):
Should we open another ticket with Django version 2.2.x and reference to
this ticket number for backport ?
--
Ticket URL: <https://code.djangoproject.com/ticket/30380#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Nathan Klug | Owner: felixxm
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by felixxm):
This patch doesn't qualify for a backport.
--
Ticket URL: <https://code.djangoproject.com/ticket/30380#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: Nathan Klug | Owner: felixxm
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Liam):
Hi, bit confused by the release process, when will this fix be available?
I can see it wasn't included in 2.2.5, is that correct?
Thanks
--
Ticket URL: <https://code.djangoproject.com/ticket/30380#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Nathan Klug | Owner: felixxm
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz):
Correct, it will be in Django 3.0, see the
[https://code.djangoproject.com/wiki/Version3.0Roadmap Roadmap].
--
Ticket URL: <https://code.djangoproject.com/ticket/30380#comment:17>