Missing SQL action buttons for comments and CTEs

[roskakori]

The SQL panel does not show the buttons for "Sel" and "Expl" for certain SQL queries:

1. Queries starting with an SQL line comment, for example:

   -- Some comment
   select * from table

2. Common table expressions (CTEs), for example:

   with 
       active_user in (
           select * from auth_user where is_active
       )
   select * from active_user where ...

While these kind of constructs are typically not added by the standard Django ORM, raw queries or queries created by extensions like django-cte can utilize them.

The culprit seems to be panels.utils.is_select_query(), which currently is very limited:

def is_select_query(sql):
    # UNION queries can start with "(".
    return sql.lower().lstrip(" (").startswith("select")

Bellow you can find an improved version with some tests attached.

This still does not cover /* ... */ style comments which some SQL dialects support. If requested, I can add that, but this would require a slightly more elaborate lexer then the current simple line parsing.

Let me know if you want this version (or an extended one with /* ... */ comments) wrapped in a PR.

Code

from django.test import TestCase

# Drop-in replacement for debug_toolbar.panels.sql.utils.is_select_query
def is_select_query(sql: str) -> bool:
    for line in sql.splitlines():
        stripped_line = line.lstrip(" \t")
        if stripped_line == "":
            pass # Skip empty lines
        elif stripped_line.startswith("--"):
            pass # Skip SQL line comment
        else:
            # UNION queries can start with "(".
            cleaned_line = stripped_line.lstrip("(").lower()
            # Common table expressions start with "with".
            return cleaned_line.startswith("select") or cleaned_line.startswith("with")
    return False  # The query is empty (for whatever curious reason)

class IsSelectQueryTest(TestCase):
    def test_can_accept_plain_query(self):
        self.assertTrue(is_select_query('SELECT * FROM table'))
        self.assertTrue(is_select_query('select * from table'))

    def test_can_reject_non_query(self):
        self.assertFalse(is_select_query('update table set foo = 123'))

    def test_can_accept_query_with_leading_whitespace(self):
        self.assertTrue(is_select_query(' \t select * from table'))

    def test_can_accept_query_with_leading_empty_lines(self):
        self.assertTrue(is_select_query('\n\r\n select * from table'))

    def test_can_accept_query_with_leading_line_comments(self):
        self.assertTrue(is_select_query('''-- A comment
              -- A comment with leading whitespace
            select * from table
        '''))

    def test_can_accept_common_table_expression(self):
        self.assertTrue(is_select_query('''
            (select * from table where foo = 123)
            union
            (select * from table where bar = 234)
        '''))

    def test_can_accept_common_table_expression(self):
        self.assertTrue(is_select_query('''
            with some_date as (
                select * from table where foo = 123
            )
            select * from some_date
        '''))

[tim-schilling]

Here's a different question altogether.

Should the toolbar allow running non-select queries? I could see a benefit to that.

[roskakori]

@tim-schilling

Should the toolbar allow running non-select queries? I could see a benefit to that.

Frankly, when skimming the code and eventually encountering is_select_query I could not understand the reasoning for limiting it to queries.

Most non-queries are probably trivial performance wise because they are simple insert or update statements for single rows. For those, the toolbar would provide little value. But then, one would rarely bring up the toolbar to optimize simple form/edit views.

And for more elaborate statements like update with complex joins, the toolbar would be very useful.

So in my opinion, removing is_select_query altogether would be even better.

[matthiask]

I think it has to do with the fact that SELECT was deemed safe when the functionality was initially introduced in b7d7e9f?

Since we're only supporting executing SQL we signed ourselves I think supporting all sorts of queries is safe, at least in environments where the toolbar should be used at all.

[roskakori]

Is there anything I can do to move this forward?

Like making a PR that removes is_select_query or replaces with a more accurate one as suggested?

[tim-schilling]

@roskakori yes, PRs are always welcome here 😁