Repository navigation
Conversation
charettes
left a comment
There was a problem hiding this comment.
I like the non-invasive approach you have taken but I wonder if we could avoid coupling QuerySet with .queries and setting.DEBUG by adding a new last_query property
diff --git a/django/db/backends/base/base.py b/django/db/backends/base/base.py
index 9a4c27f0f3..6d62d30e86 100644
--- a/django/db/backends/base/base.py
+++ b/django/db/backends/base/base.py
@@ -158,6 +158,10 @@ def queries(self):
"will be returned.".format(self.queries_log.maxlen))
return list(self.queries_log)
+ @property
+ def last_query(self):
+ return queries[-1] if (queries := self.queries_log) else None
+
# ##### Backend-specific methods for creating connections and cursors #####
def get_connection_params(self):This will also require tests and docs.
I agree on checking whether there are queries in the list instead of checking django/django/db/backends/base/base.py Lines 173 to 180 in bc9b625 I guess it's not relevant in this case since we're picking always the last one, but in the future there could be other checks or warnings. Wouldn't it make sense to use the more public queries property?
100% agree, but I wanted to wait until I was sure the approach was acceptable. I'll start work on them soon. |
|
Just as a quick update, I'm still slowly working on this, but life keeps happening. |
019ef53 to
c6a3a11
Compare
86874da to
d8e604a
Compare
|
Implementation is done and I added a test. The only missing thing is the documentation. I'm not sure if there's any other case that should be tested. I tried an async test, but in async connection.queries always seems to be |
34a5b4f to
1e1befd
Compare
rtpg
left a comment
There was a problem hiding this comment.
Seems like this would only kick in with DEBUG = True (or a forced debug cursor) so the documentation should reflect that.
| columns. | ||
|
|
||
| * The new :attr:`QuerySet.executed_query <.QuerySet.executed_query>` | ||
| attribute returns the SQL query if the QuerySet has been evaluated. |
There was a problem hiding this comment.
This only happens if the connection is actually building up a query log right? And so even if a queryset is evaluated there are times where executed_query is None right? Or am I misreading something about Connection.queries?
There was a problem hiding this comment.
As far as I know, connection.queries is filled based on this condition
django/django/db/backends/base/base.py
Lines 169 to 171 in 661dfdd
force_debug_cursor is not documented and the only use I can find in the Django codebase is here:
Lines 714 to 716 in 661dfdd
Even a custom Database backend that inherits from the BaseDatabaseWrapper should implement these two methods:
django/django/db/backends/base/base.py
Lines 677 to 683 in 661dfdd
And which one is called is decided here:
django/django/db/backends/base/base.py
Lines 284 to 293 in 661dfdd
So the only case I realistically see in which you have access to the evaluated queryset after executing it (So methods like .all(), since .get() wouldn't return a queryset) and executed_query is None is if DEBUG is False, which is something I will add in the documentation changes.
|
With the API of I do recognize that this would mean we'd have to map the queries being run to the |
|
it could be interesting to insert the test related with |
| def _fetch_all(self): | ||
| if self._result_cache is None: | ||
| self._result_cache = list(self._iterable_class(self)) | ||
| self.executed_query = connections[self.db].last_query |
There was a problem hiding this comment.
Thanks for the PR!
As mentioned during the Djangonauts Review session, this could be a problem if the queryset does not execute a database query at all e.g. objects.none(), as it would return the last executed query which may come from a different queryset.
A potential workaround is to get a snapshot of connections[self.db].queries before the iterable is evaluated. Then, compare the snapshot with what the list of queries are now. If there are new queries, then those are likely to be related to the current queryset. This might also help with prefetch queries. So maybe instead of executed_query, we should make it executed_queries instead which contains the new queries. This means we return an empty list if no queries are executed – which may also avoid having to decide whether to return None or raise an exception.
Of course, this solution might not be safe for race conditions. Though I'm not sure if the current approach is safe either 🤔
There was a problem hiding this comment.
I wonder if Django would be better off if there was a signal that was raised when queries are executed.
There was a problem hiding this comment.
Sounds like it could be a performance hit though? Calling all the handlers for that signal (even if there aren't any) for every single query seems like it would add up. (Of course, we can't be sure without any benchmarks.)
There was a problem hiding this comment.
I think my solution would be to simply clear the .executed_query when you call .none().
That's the maximum I would complicate this case. If fixing the none() case required extra work I think it would be better to not fix it.
This a purely debugging helper field, so in that case, a situation in which someone reuses a queryset to call none() and then looks at the executed query expecting it to be None it's too rare to have such a complicated implementation.
There was a problem hiding this comment.
It might. I think I should take this discussion to the forum, it doesn't belong on this particular PR. I do like your idea Sage of having an executed_queries as that seems more future proof.
There was a problem hiding this comment.
Another issue with using a signal is that it is possible to perform a query during a signal receiver causing an infinite loop of queries.
There was a problem hiding this comment.
I like @laymonage's idea of snapshoting. We could snapshot the length of connection.queries list before and after evaluation and have executed_query (or executed_queries assuming we want to include prefetching as well) be a slice of it.
If the index remain the same because no queries were executed then executed_query is None (or executed_queries is []).
|
During Django on the Med @charettes and I agreed on the following, to move this forward and handle the feedback:
The values will be the following:
|
jacobtylerwalls
left a comment
There was a problem hiding this comment.
Thanks! Looked at this with @raffaellasuardini at DjangoCon EU 🇬🇷. This is very exciting!
(I know you are adding docs for this later.)
|
|
||
|
|
||
| class ExecutedQueriesTests(TestCase): | ||
| def test_no_debug_executed_queries(self): |
There was a problem hiding this comment.
Let's make the value of DEBUG explicit like the other test methods you added.
| required_db_features = {"supports_json_field"} | ||
|
|
||
|
|
||
| class ExecutedQueriesModel(models.Model): |
There was a problem hiding this comment.
We discussed this is a great candidate for reusing a simple test model, maybe one used often in other tests in this file.
| queries_executed = True | ||
| self._prefetch_related_objects() | ||
|
|
||
| if queries_executed: |
There was a problem hiding this comment.
Factoring this into a util will allow us to reuse this on RawQuerySet.
|
|
||
| self.assertEqual( | ||
| queryset.executed_queries, | ||
| [ |
There was a problem hiding this comment.
We'll need to relax this assertion.
| self._result_cache = list(self._iterable_class(self)) | ||
|
|
||
| if self._prefetch_related_lookups and not self._prefetch_done: | ||
| queries_executed = True |
There was a problem hiding this comment.
No tests fail when I remove this. Is there a test we can write to demonstrate this is needed?
There was a problem hiding this comment.
It was not needed indeed. The local tests also pass if the code is
if self._result_cache is None:
executed_queries_length = len(connections[self.db].queries)
self._result_cache = list(self.iterator())
if self._prefetch_related_lookups and not self._prefetch_done: # Nested instead
self._prefetch_related_objects()
So there may not be a need for it two be two separate ifs
5cae913 to
98ade1f
Compare
|
Thank you for your contribution to Django! This pull request has one or more items that need attention before it can be accepted for review. ❗ Error: Incomplete ChecklistThe items in the Checklist section must be all understood and checked accordingly before this PR can be accepted. These items confirm that you have read and followed the Django contributing guidelines. What to do:
If you have questions about these requirements, please review the contributing guidelines or ask for help on the Django Forum. |
edc2af3 to
c47187a
Compare
| columns. | ||
|
|
||
| * The new :attr:`QuerySet.executed_query <.QuerySet.executed_query>` | ||
| attribute returns the SQL query if the QuerySet has been evaluated. |
There was a problem hiding this comment.
As far as I know, connection.queries is filled based on this condition
django/django/db/backends/base/base.py
Lines 169 to 171 in 661dfdd
force_debug_cursor is not documented and the only use I can find in the Django codebase is here:
Lines 714 to 716 in 661dfdd
Even a custom Database backend that inherits from the BaseDatabaseWrapper should implement these two methods:
django/django/db/backends/base/base.py
Lines 677 to 683 in 661dfdd
And which one is called is decided here:
django/django/db/backends/base/base.py
Lines 284 to 293 in 661dfdd
So the only case I realistically see in which you have access to the evaluated queryset after executing it (So methods like .all(), since .get() wouldn't return a queryset) and executed_query is None is if DEBUG is False, which is something I will add in the documentation changes.
| def _fetch_all(self): | ||
| if self._result_cache is None: | ||
| self._result_cache = list(self._iterable_class(self)) | ||
| self.executed_query = connections[self.db].last_query |
There was a problem hiding this comment.
I think my solution would be to simply clear the .executed_query when you call .none().
That's the maximum I would complicate this case. If fixing the none() case required extra work I think it would be better to not fix it.
This a purely debugging helper field, so in that case, a situation in which someone reuses a queryset to call none() and then looks at the executed query expecting it to be None it's too rare to have such a complicated implementation.
| self._result_cache = list(self._iterable_class(self)) | ||
|
|
||
| if self._prefetch_related_lookups and not self._prefetch_done: | ||
| queries_executed = True |
There was a problem hiding this comment.
It was not needed indeed. The local tests also pass if the code is
if self._result_cache is None:
executed_queries_length = len(connections[self.db].queries)
self._result_cache = list(self.iterator())
if self._prefetch_related_lookups and not self._prefetch_done: # Nested instead
self._prefetch_related_objects()
So there may not be a need for it two be two separate ifs
| with self.settings(DATABASE_ROUTERS=[router]): | ||
| book.refresh_from_db() | ||
| router.db_for_read.assert_called_once_with(Book, instance=book) | ||
| router.db_for_read.assert_called_with(Book, instance=book) |
There was a problem hiding this comment.
Relaxed this assertion, as talked with Jacob in DjangoCon Athens.
Trac ticket number
ticket-25705
Branch description
Based on the discussion with @charettes it was agreed to follow the path of adding
Queryset.executed_queries. To solve the ticket.Since the DB connections are per-thread as far as I know, this approach should not have any race condition issues. And since it's done before the prefetch, it should pick the Queryset query. Also, since queries are only logged if DEBUG is enabled, this approach requires that too.
My discarded approach was to to get it somehow in the return of
self._result_cache = list(self._iterable_class(self)), or stored in the QuerySet.However, it's much more complicated, because as far as I see the flow is something like
QuerySet when evaluating calls the IterableClass which then executes the query which then gets the connection cursor, which is wrapped in the previous mentioned one that can log the query.
So the a way would be to make all 4
SqlCompiler.execute_sqlreturn the result and the SQL query. Then you have to adapt all 6Iterable.__iter__to also return the query. Then you can store it in the Queryset. Since these methods are part of the public API, this would be a breaking change in both methods. Also I'm not 100% sure how to get the query in the wrapper since theCursor.executeis wrapped in a context manager which is the one logging the query.AI Assistance Disclosure (REQUIRED)
Checklist
mainbranch.