Skip to content

Error when recording SQL queries that are byte-strings #987

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

Closed
ahardjasa opened this issue Aug 24, 2017 · 11 comments · Fixed by #1812
Closed

Error when recording SQL queries that are byte-strings #987

ahardjasa opened this issue Aug 24, 2017 · 11 comments · Fixed by #1812

Comments

@ahardjasa
Copy link

When a byte-string SQL query is run, e.g. those produced by psycopg2.execute_values, I get the error TypeError: startswith first arg must be bytes or a tuple of bytes, not str.

This seems to be a result of line 142 in django-debug-toolbar/debug_toolbar/panels/sql/tracking.py: 'is_select': sql.lower().strip().startswith('select')
since 'select' is unicode.

Flask-debugtoolbar addressed this with prefix = b'select' if isinstance(statement, bytes) else 'select' - would that work in this case?

@matthiask
Copy link
Member

The DB API 2.0 PEP (https://www.python.org/dev/peps/pep-0249/) does not mandate a type for the SQL operation -- whether it's bytes or str isn't defined (which isn't surprising at all since the PEP was added at least 17 years ago).

Still, I'm a bit reluctant because I fear breakage throughout; is this really all that's required to support byte string queries? Should we even support byte string queries at all?

@aaugustin
Copy link
Contributor

I'm OK with that change.

@matthiask
Copy link
Member

Great. @ahardjasa do you want to submit a pull request?

@thaxy
Copy link

thaxy commented Nov 3, 2017

Same problem for Django raw SQL queries such as:

with connection.cursor() as cursor:
            cursor.execute(
                sql.SQL(
                    '''
                    SELECT json_build_object(
                        'data', array_agg(r)
                    )
                    FROM (
                        SELECT
                        a,
                        {}
                        FROM b
                        WHERE c=%s
                        ORDER BY a DESC
                        LIMIT %s
                    ) r
                    '''
                ).format(var),
                [x, 5]
            )
            result = cursor.fetchall()

@bschollnick
Copy link

This appears to be a problem for me as well. I'm not sure if this should be broken off into a different issue ticket?

I'm storing binary image thumbnails, and it works fine when I am not running django debug toolbar, but as soon as I enable DDT, a bit of chaos occurs.

With it enabled, Django can not store, read, or manipulate the binary fields. Everything else works fine, but the binary fields do not work.

@Caiofcas
Copy link

Is this still a problem? Looking around I found this test (in tests/panels/test_sql.py) and it seems to catch this use case:

def test_non_ascii_query(self):
        ...
        # non-ASCII bytes parameters
        list(Binary.objects.filter(field__in=["café".encode()]))
        self.assertEqual(len(self.panel._queries), 3)
        ...

@matthiask
Copy link
Member

@Caiofcas Thank you! If I'm reading this issue correctly it is about the whole SQL string being bytes which is different from the test where (only) a parameter is bytes.

@Lucidiot
Copy link
Contributor

Lucidiot commented Jul 6, 2023

I just got this with the latest version, 4.1.0. This line in psycopg2.extras.execute_values causes it to execute a query of type bytes.

utting this in a view alone will raise the issue when the toolbar is active:

from django.db import connection

with connection.cursor() as cursor:
    cursor.execute(b'SELECT 1')

@matthiask
Copy link
Member

@Lucidiot It seems nobody is actively working on this right now. Does the proposed fix in the initial report make everything work as it should? It would be very helpful if you could check this and maybe submit a pull request with a small test case. Thanks in advance!

@living180
Copy link
Contributor

@Lucidiot I could also potentially take a look in the next couple of days if you don't have time to get a PR together.

@Lucidiot
Copy link
Contributor

Lucidiot commented Jul 7, 2023

I found that I was hitting an error in another place, before the code originally mentioned in this issue could even be called, so I fixed that instead and now the query seems to always be a string by the time it reaches the is_select of the initial report.

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.

8 participants