Fixed #24638 -- Added support for SQL comments #18258

viciu · 2024-06-08T16:01:20Z

This PR reactivates work done in #4495 and #15711 - useful addition to Django for adding traceability for queries.

Trac ticket number

ticket-24638

Branch description

Add queryset comment() method

Checklist

This PR targets the main branch.
The commit message is written in past tense, mentions the ticket number, and ends with a period.
I have checked the "Has patch" ticket flag in the Trac system.
I have added or updated relevant tests.
I have added or updated relevant docs, including release notes if applicable.
I have attached screenshots in both light and dark modes for any UI changes.

adamchainz

Great start, looking forward to your iteration tomorrow.

The PR description formatting needs a little fixing.

django/db/models/sql/compiler.py

adamchainz · 2024-06-09T10:27:06Z

docs/ref/models/querysets.txt

+.. method:: comment(message)
+
+Adds an SQL comment to be inserted into the resultant SQL statement, after
+``SELECT``/``UPDATE``, and if applicable, ``DISTINCT``.. For example::


Is it not possible to make it work for INSERT, at least when using Model.objects.comment(...).create(...) ?

docs/ref/models/querysets.txt

docs/releases/5.2.txt

django/db/models/query.py

django/db/models/sql/compiler.py

adamchainz · 2024-06-09T17:46:43Z

tests/queries/tests.py

+        with CaptureQueriesContext(connection) as captured_queries:
+            list(NamedCategory.objects.comment("! STRAIGHT_JOIN"))
+            list(NamedCategory.objects.comment("").comment("some comment"))
+        self.assertIn("SELECT /* ! STRAIGHT_JOIN */ ", captured_queries[0]["sql"])


Use startswith in the tests for more accuracy

Suggested change

self.assertIn("SELECT /* ! STRAIGHT_JOIN */ ", captured_queries[0]["sql"])

self.assertTrue(captured_queries[0]["sql"].startswith("SELECT /* ! STRAIGHT_JOIN */ "))

Also, I see you're testing with an example of a MySQL-specific ! comment. But that would require no space before ! to work, so it would be /*! STRAIGHT JOIN */. See the docs: https://dev.mysql.com/doc/refman/8.0/en/comments.html .

We agreed not to support any such comments in the old PR ( #15711 (comment) ), so let's drop any references to them in the tests. Let’s instead have test data like the intended use case, like blog/views.py:123. (This applies to all the tests. I find the COUNT / DISTINCT tests a bit weird to read, using the keywords.)

adamchainz · 2024-06-09T18:16:25Z

tests/queries/tests.py

+        for comment in ["foo=/a/b/c*/", "/*foo=/a/b/c", "**//SELECT nothing;//**"]:
+            msg = (
+                "Cannot pass strings containing /* or */ to comment(). "
+                "Escape or strip these delimiters before calling comment()."
+            )
+
+            with self.subTest(comment), self.assertRaisesMessage(ValueError, msg):
+                NamedCategory.objects.comment(comment)


We can hoist msg and split the subtests one per line, to make it easier to read and add any more in the future.

Suggested change

for comment in ["foo=/a/b/c*/", "/*foo=/a/b/c", "**//SELECT nothing;//**"]:

msg = (

"Cannot pass strings containing /* or */ to comment(). "

"Escape or strip these delimiters before calling comment()."

)

with self.subTest(comment), self.assertRaisesMessage(ValueError, msg):

NamedCategory.objects.comment(comment)

msg = (

"Cannot pass strings containing /* or */ to comment(). "

"Escape or strip these delimiters before calling comment()."

)

for comment in [

"foo=/a/b/c*/",

"/*foo=/a/b/c",

"**//SELECT nothing;//**",

]:

with self.subTest(comment), self.assertRaisesMessage(ValueError, msg):

NamedCategory.objects.comment(comment)

adamchainz · 2024-06-09T18:18:25Z

tests/queries/tests.py

+            NamedCategory.objects.create(name="test")
+            NamedCategory.objects.comment("DELETE 1").all().delete()
+
+            NamedCategory.objects.create(name="test")
+            NamedCategory.objects.comment("DELETE 2").all().delete()
+        self.assertIn("DELETE FROM /* DELETE 1 */", captured_queries[1]["sql"])


We don't need to create any data in order to run DELETE

Only one delete query needed

Use startswith and more realistic example data

Suggested change

NamedCategory.objects.create(name="test")

NamedCategory.objects.comment("DELETE 1").all().delete()

NamedCategory.objects.create(name="test")

NamedCategory.objects.comment("DELETE 2").all().delete()

self.assertIn("DELETE FROM /* DELETE 1 */", captured_queries[1]["sql"])

NamedCategory.objects.comment("blog/views.py:50").all().delete()

self.assertTrue(

captured_queries[0]["sql"].startswith(

"DELETE /* blog/views.py:50 */ FROM",

)

)

The problem here is that delete does not generate delete query, but select:

This

NamedCategory.objects.comment("DELETE 2").delete()

will fail:

FAIL: test_delete (queries.tests.SqlCommentsInQueriesTests.test_delete) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/viciu/prog/django/tests/queries/tests.py", line 4670, in test_delete self.assertIn("DELETE FROM /* DELETE 1 */", [q['sql'] for q in captured_queries]) AssertionError: 'DELETE FROM /* DELETE 1 */' not found in ['SELECT /* DELETE 2 */ "queries_dumbcategory"."id", "queries_namedcategory"."dumbcategory_ptr_id", "queries_namedcategory"."name" FROM "queries_namedcategory" INNER JOIN "queries_dumbcategory" ON ("queries_namedcategory"."dumbcategory_ptr_id" = "queries_dumbcategory"."id")']

therefore I was experimenting with adding some records first and then deleting them. If I understand correctly it looks like django fetches records to delete first, then updates related records with nulls, then finally deletes them one by one (including related records):

FAIL: test_delete (queries.tests.SqlCommentsInQueriesTests.test_delete) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/viciu/prog/django/tests/queries/tests.py", line 4670, in test_delete self.assertIn("DELETE FROM /* DELETE 1 */", [q['sql'] for q in captured_queries]) AssertionError: 'DELETE FROM /* DELETE 1 */' not found in [ 'INSERT INTO "queries_dumbcategory" ("id") VALUES (NULL)', 'INSERT INTO "queries_namedcategory" ("dumbcategory_ptr_id", "name") VALUES (1, \'test\')', 'SELECT /* DELETE 1 */ "queries_dumbcategory"."id", "queries_namedcategory"."dumbcategory_ptr_id", "queries_namedcategory"."name" FROM "queries_namedcategory" INNER JOIN "queries_dumbcategory" ON ("queries_namedcategory"."dumbcategory_ptr_id" = "queries_dumbcategory"."id")', 'UPDATE "queries_tag" SET "category_id" = NULL WHERE "queries_tag"."category_id" IN (1)', 'DELETE FROM "queries_namedcategory" WHERE "queries_namedcategory"."dumbcategory_ptr_id" IN (1)', 'DELETE FROM "queries_dumbcategory" WHERE "queries_dumbcategory"."id" IN (1)', 'INSERT INTO "queries_dumbcategory" ("id") VALUES (NULL)', 'INSERT INTO "queries_namedcategory" ("dumbcategory_ptr_id", "name") VALUES (2, \'test\')', 'SELECT /* DELETE 2 */ "queries_dumbcategory"."id", "queries_namedcategory"."dumbcategory_ptr_id", "queries_namedcategory"."name" FROM "queries_namedcategory" INNER JOIN "queries_dumbcategory" ON ("queries_namedcategory"."dumbcategory_ptr_id" = "queries_dumbcategory"."id")', 'UPDATE "queries_tag" SET "category_id" = NULL WHERE "queries_tag"."category_id" IN (2)', 'DELETE FROM "queries_namedcategory" WHERE "queries_namedcategory"."dumbcategory_ptr_id" IN (2)', 'DELETE FROM "queries_dumbcategory" WHERE "queries_dumbcategory"."id" IN (2)']

adamchainz · 2024-06-09T18:19:12Z

Thanks for putting sprint time in on this! I found a bit more time here to review, hope this helps.

…ueries. Co-authored-by: Adam Johnson <[email protected]> Co-authored-by: Wiktor Kolodziej <[email protected]>

- Moved wrapping to comments in QuerySet.comment() - Removed comments_sql() - Added re.search instead of use if */ - WIP INSERT and DELETE

charettes

I understand the desire to help workaround MySQL'ism but I find it strange that we default to injecting comments where it can be useful for the latter on all backends given the main purpose of these is likely to be traceability which is harder to parse when it's right in the middle SQL.

Could we have some backends level option that at least configure where they should live and default to prepending / postpending them on non-MySQL/Oracle backends?

charettes · 2024-06-10T01:35:41Z

django/db/models/sql/compiler.py

-        delete = "DELETE FROM %s" % self.quote_name_unless_alias(query.base_table)
+        result = [
+            "DELETE",
+            *self.query.comments,


What's the rationale for injecting comments here instead of at the end of the query?

Thanks for your comment. Queries can be long and truncated - please refer to the comment #15711 (comment)

Yeah, the idea is that in a running query list, you can see the type of query first, then any tracing info. I saw this recommended by MySQL experts years ago, hence my suggestion for it in the previous PR. This is not for supporting any MySQL specific options.

I'm not so sure now that we should try to support inserting comments at any of the positions used for query hints. That would require a very complex API and might still not cover all cases. Also, database instrumentation exists to allow rewriting queries when needed.

I saw this recommended by MySQL experts years ago.

Do you have references for that? Isn't it fair to assume that an approach that works well for MySQL and recommended by these experts might not work so well for other backends (e.g. Postgres). I've seen a few solutions default prepending the traces to queries to ensure they are never truncated and APMs requiring that the comment be pre or postpended.

At work we built a solution that postpends commented traces specifically for MySQL and it appears to be working well hence my curiosity here.

From an internal discussion with DBREs

Some tooling doesn’t support parsing in the middle of a query

Keeping queries starting with their respective verb makes it easier to filter

Putting the trace at the end of query might cause it to be truncated off on some systems

This makes me think that a proper solution would need to be configurable (PREPEND, POSTVERB, POSTPEND)?

django/db/models/sql/compiler.py

pgramaca · 2024-06-18T16:54:17Z

Let me know if there is something i can help with.
I would love to help with this feature.

viciu changed the title ~~Fixed #24638 -- Add support for SQL comments in SELECT and UPDATE que…~~ Fixed #24638 -- Add support for SQL comments Jun 8, 2024

adamchainz suggested changes Jun 8, 2024

View reviewed changes

django/db/models/sql/compiler.py Outdated Show resolved Hide resolved

django/db/models/sql/compiler.py Outdated Show resolved Hide resolved

nessita added the DjangoCon Europe 🏰 label Jun 8, 2024

viciu force-pushed the 24638-query-comments branch 2 times, most recently from 19932f2 to 3d75c28 Compare June 9, 2024 09:58

viciu changed the title ~~Fixed #24638 -- Add support for SQL comments~~ Fixed #24638 -- Added support for SQL comments Jun 9, 2024

viciu force-pushed the 24638-query-comments branch from 3d75c28 to 9f1cd3a Compare June 9, 2024 10:10

adamchainz reviewed Jun 9, 2024

View reviewed changes

django/db/models/sql/compiler.py Outdated Show resolved Hide resolved

viciu force-pushed the 24638-query-comments branch from 9f1cd3a to 4e1bc80 Compare June 9, 2024 10:30

adamchainz reviewed Jun 9, 2024

View reviewed changes

viciu force-pushed the 24638-query-comments branch 4 times, most recently from fd32c25 to 076612e Compare June 9, 2024 14:51

adamchainz reviewed Jun 9, 2024

View reviewed changes

django/db/models/query.py Outdated Show resolved Hide resolved

adamchainz suggested changes Jun 9, 2024

View reviewed changes

viciu force-pushed the 24638-query-comments branch from 076612e to 61c76f5 Compare June 9, 2024 22:47

hannseman and others added 2 commits June 10, 2024 00:50

Fixed #24638 -- Added support for SQL comments in SELECT and UPDATE q…

e8e4193

…ueries. Co-authored-by: Adam Johnson <[email protected]> Co-authored-by: Wiktor Kolodziej <[email protected]>

Fixed #24638 -- Added support for SQL comments

19cff6f

- Moved wrapping to comments in QuerySet.comment() - Removed comments_sql() - Added re.search instead of use if */ - WIP INSERT and DELETE

viciu force-pushed the 24638-query-comments branch from 61c76f5 to 19cff6f Compare June 9, 2024 22:50

charettes reviewed Jun 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #24638 -- Added support for SQL comments #18258

Fixed #24638 -- Added support for SQL comments #18258

viciu commented Jun 8, 2024 •

edited

Loading

adamchainz left a comment

adamchainz Jun 9, 2024

adamchainz Jun 9, 2024

adamchainz Jun 9, 2024

adamchainz Jun 9, 2024

viciu Jun 9, 2024

adamchainz commented Jun 9, 2024 •

edited

Loading

charettes left a comment •

edited

Loading

charettes Jun 10, 2024

viciu Jun 10, 2024

adamchainz Jun 10, 2024

charettes Jun 10, 2024 •

edited

Loading

charettes Jun 10, 2024 •

edited

Loading

pgramaca commented Jun 18, 2024

	self.assertIn("SELECT /* ! STRAIGHT_JOIN */ ", captured_queries[0]["sql"])
	self.assertTrue(captured_queries[0]["sql"].startswith("SELECT /* ! STRAIGHT_JOIN */ "))

Fixed #24638 -- Added support for SQL comments #18258

Are you sure you want to change the base?

Fixed #24638 -- Added support for SQL comments #18258

Conversation

viciu commented Jun 8, 2024 • edited Loading

Trac ticket number

Branch description

Checklist

adamchainz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamchainz commented Jun 9, 2024 • edited Loading

charettes left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charettes Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

charettes Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

pgramaca commented Jun 18, 2024

viciu commented Jun 8, 2024 •

edited

Loading

adamchainz commented Jun 9, 2024 •

edited

Loading

charettes left a comment •

edited

Loading

charettes Jun 10, 2024 •

edited

Loading

charettes Jun 10, 2024 •

edited

Loading