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

conv option should override defaults and convertions changed by use_unicode #683

Closed
wants to merge 3 commits into from

Conversation

tltx
Copy link

@tltx tltx commented Jan 3, 2024

Right now you can't customise conversions for:
FIELD_TYPE.STRING,
FIELD_TYPE.VAR_STRING,
FIELD_TYPE.VARCHAR,
FIELD_TYPE.TINY_BLOB,
FIELD_TYPE.MEDIUM_BLOB,
FIELD_TYPE.LONG_BLOB,
FIELD_TYPE.BLOB,
FIELD_TYPE.JSON

at the same time as having 'use_unicode' set to True. That option overwrites the conversions you have provided with the 'conv' option. I have tried to fix this and also set make the the unicode conversions the default as I think that makes most sense and is has the least risk of changing the behaviour of users options

Copy link

codecov bot commented Jan 3, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (a2212b3) 88.86% compared to head (d0b1185) 88.34%.

Files Patch % Lines
src/MySQLdb/connections.py 62.50% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #683      +/-   ##
==========================================
- Coverage   88.86%   88.34%   -0.53%     
==========================================
  Files           7        7              
  Lines         548      549       +1     
==========================================
- Hits          487      485       -2     
- Misses         61       64       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tltx tltx changed the title try to fix broken conv conv option should override defaults and convertions changed by use_unicode Jan 3, 2024
@methane
Copy link
Member

methane commented Jan 4, 2024

I don't like support overriding them. It is too low level and user won't know how to handle them.
ORM or higher level tool should convert from bytes/str to types users want.

@methane methane closed this Jan 4, 2024
@tltx
Copy link
Author

tltx commented Jan 4, 2024

I agree that conversions should preferably be done in a higher level tool but the conv option already supports overriding for most field types but not all because of how use_unicode was implemented. Django uses the conv option to change the FIELD_TYPE.TIME conversion for example, so it is used. I have some legacy code that need to override FIELD_TYPE.STRING, FIELD_TYPE.VAR_STRING and FIELD_TYPE.VARCHAR to convert the MySQL set type to a Python set. A workaround is to set use_unicode to False so that it wont interfere and add the unicode conversions back with conv option. The problem is that you need to misuse the use_unicode option to get it working and that feels a bit fragile. I would like to move the set conversion out of mysqlclient and into the application but it is hard to do when a lot of code has been written with the assumption that a set and not a string will be returned. One option could be to extend the default conversion instead of replacing them but that would be a bigger change to the API

@methane
Copy link
Member

methane commented Jan 5, 2024

Django uses the conv option to change the FIELD_TYPE.TIME conversion for example,

I dislike conv but I didn't mean it. When I said "them", I meant FIELD_TYPE.STRING ~ FIELD_TYPE.JSON. So TIME is unrelating to this issue.

I have some legacy code that need to override FIELD_TYPE.STRING, FIELD_TYPE.VAR_STRING and FIELD_TYPE.VARCHAR to convert the MySQL set type to a Python set.

Please fix your code, not this library.
There are very ugly hacks in both of mysqlclient and PyMySQL to support conv parameter.
And supporting these types in conv parameter makes these library unmaintainable to me.

conv was designed very old time. There was no Python 3. There was no use_unicode option and every string are byte string.
That makes porting MySQLdb to Python3 very hard. That was the most difficult part.

Converter functions should accept both of bytes and unicode. But as you know, there are many converters defined outside of this library, like Django's. I can not fix all converters in the world. That is why I need to ugly hack in mysqlclient/PyMySQL side.

And converter function won't accept encoding parameter. That is one of reasons why I drop supporting overriding string field types.

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 this pull request may close these issues.

2 participants