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.

Sign up for GitHub

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

Jump to bottom

Fix unicode error when trying to escape binary data #1107

Merged
merged 7 commits into from
Nov 28, 2018
Merged

Fix unicode error when trying to escape binary data #1107

merged 7 commits into from
Nov 28, 2018

Conversation

dbowd
Copy link
Contributor

@dbowd dbowd commented Oct 18, 2018

No description provided.

@matthiask
Copy link
Member

matthiask commented Oct 25, 2018 •
edited
Loading

Thanks for your contribution!

isinstance(element, six.binary_type) and handling binary data explicitly would be nice. Also, can you please add a test for this so that it doesn't break in the future?

@matthiask
Copy link
Member

Hmm, reading this again it seems to have some relation to #1064

@jdufresne Care to chime in? Maybe you some advice to offer when it comes to handling binary data in django-debug-toolbar?

@dbowd
Copy link
Contributor Author

dbowd commented Oct 26, 2018

@matthiask I encountered this bug when creating an object with a models.BinaryField() field.

The test is using the username field because there is no BinaryField in the django User model. I am making a raw query using cursor.execute() because django does complain about binary data as mentioned by @jdufresne.

The exact same exception is encountered in both cases.

jdufresne
jdufresne reviewed Oct 26, 2018
View reviewed changes
Copy link
Contributor

@jdufresne jdufresne left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The BinaryField use case makes sense to me. I hadn't considered that. How does that look in a query? Can we use the same representation?

debug_toolbar/panels/sql/tracking.py Outdated Show resolved Hide resolved
tests/panels/test_sql.py Outdated Show resolved Hide resolved
tests/panels/test_sql.py Outdated Show resolved Hide resolved
@dbowd dbowd force-pushed the patch-1 branch 2 times, most recently from 6862752 to d67ef3d Compare November 8, 2018 18:19
jdufresne
jdufresne reviewed Nov 19, 2018
View reviewed changes
debug_toolbar/panels/sql/tracking.py Outdated Show resolved Hide resolved
tests/panels/test_sql.py Outdated Show resolved Hide resolved
@dbowd
Copy link
Contributor Author

dbowd commented Nov 27, 2018

@matthiask

@jdufresne
Copy link
Contributor

Should we add MySQL/MariaDB to Travis so this gets covered? We could use the PostgreSQL target as a template.

@matthiask matthiask merged commit e78ac8c into django-commons:master Nov 28, 2018
@matthiask
Copy link
Member

matthiask commented Nov 28, 2018 •
edited
Loading

@dbowd Thanks!

@jdufresne That's a good idea. I added a basic PR here: #1121

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdufresne jdufresne jdufresne left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dbowd @matthiask @jdufresne