Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connection.escape fails to escape byte strings and raises TypeError: bytes() argument 2 must be str, not dict. #489

Closed
rsiemens opened this issue Mar 26, 2021 · 9 comments · Fixed by #490

Comments

@rsiemens
Copy link
Contributor

rsiemens commented Mar 26, 2021

Describe the bug

MySQL 8.0.23
Python 3.8.5
mysqlclient 2.0.3

Connection.escape fails to escape byte strings and raises TypeError: bytes() argument 2 must be str, not dict.

At first I assumed this was a Django issue and raised it there (https://code.djangoproject.com/ticket/32595), but was pointed back here. I believe this is the offending code (https://github.com/PyMySQL/mysqlclient/blob/v2.0.1/MySQLdb/connections.py#L189-L194). Looking at the comment I think it can simply be removed as mysqlclient is on 2.x now and Django 1.11 is EOL.

To Reproduce

Simplest reproduction -

  • Have a MySQL server running and connect to it
import MySQLdb
conn = MySQLdb.connect(user='root', host='localhost', port=33060)
conn.escape(b'foo', conn.encoders)
>> TypeError: bytes() argument 2 must be str, not dict

Environment

MySQL Server

  • Server OS (e.g. Windows 10, Ubuntu 20.04): MacOS 10.15.7
  • Server Version (e.g. MariaDB 10.3.16): MySQL 8.0.23

MySQL Client

  • OS (e.g. Windows 10, Ubuntu 20.04): MacOS 10.15.7
  • Python (e.g. Homebrew Python 3.7.5): 3.8.5
  • mysqlclient 2.0.3
@methane
Copy link
Member

methane commented Mar 27, 2021

conn.escape(b'foo', conn.encoders)

This is not intended usage. conn.escape(b'foo') should be used.
And self.encoders[bytes] = bytes was added because Django used it in unexpected way.

It is so frustlating. Why Django use this library so complex way...

@felixxm
Copy link

felixxm commented Mar 27, 2021

@methane Thanks for an advice. I tried (django/django#14196) calling escape() without encoders but it crashes with unicode chars 🤔 , e.g.

>>> conn.escape('μg/mL')
UnicodeEncodeError: 'ascii' codec can't encode character '\u03bc' in position 0: ordinal not in range(128)

@felixxm
Copy link

felixxm commented Mar 27, 2021

We can manually encode strings, but should we? 🤔

@methane
Copy link
Member

methane commented Mar 28, 2021

  • UnicodeEncodeError is not relating to encoders parameter. You should use ‘charset=utf8MB4‘ in ‘connect()‘.
  • DB-API doesn't provide API to escape. You should use cur.execute(sql_with_placeholder, params).

@felixxm
Copy link

felixxm commented Mar 28, 2021

  • UnicodeEncodeError is not relating to encoders parameter. You should use ‘charset=utf8MB4‘ in ‘connect()‘.

We use charset='utf8' but it also crashes with 'utf8mb4'. Moreover it works with encoders 🤔 :

>>> conn.encoding
'utf8'
>>> conn.escape('μg/mL')
UnicodeEncodeError: 'ascii' codec can't encode character '\u03bc' in position 0: ordinal not in range(128)
>>> conn.escape('μg/mL', conn.encoders)
b"'\xce\xbcg/mL'"
  • DB-API doesn't provide API to escape. You should use cur.execute(sql_with_placeholder, params).

Docs are confusing IMO: "escape any special characters in object obj using mapping dict to provide quoting functions for each type." We want to quote values for DDL statements. We would be grateful for any advice. Thanks.

\cc @claudep

@methane
Copy link
Member

methane commented Mar 28, 2021

You are right. Connection.escape() needs conn.encoders to support Unicode.

I am not original author of this library, and Connection.escape() is broken from first. It is implemented in C. But C library don't support Unicode. Connection.encoding is supported by Python.

I added ugly hack to workaround it. That's why Connection.escape(obj) can not support Unicode and you need to pass Connection.encoders.

So there is no simple right way to do it for now.

@felixxm
Copy link

felixxm commented Mar 29, 2021

@methane Thanks 👍 I've proposed adding a "temporary" workaround to Django, see django/django#14196.

@felixxm
Copy link

felixxm commented Mar 31, 2021

IMO, this can be closed.

@rsiemens
Copy link
Contributor Author

Closing - issue #472 captures removing self.encoders[bytes] = bytes in 2.1.

Thanks @methane and @felixxm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants