Fix BEGIN leaking into SQL panel when using DatabaseCache store - #2397
Fix BEGIN leaking into SQL panel when using DatabaseCache store#2397marcosalvesdev wants to merge 2 commits into
Conversation
|
👋🏼 Hey you! Guys, the tox tests for SQLite are failing. I investigated it and found that the behavior expected in tests/panels/test_sql.py, which assumed that boolean parameters for non-PostgreSQL databases would be introduced in Django 6.2, was already introduced in 6.1. So I opened issue #2398. I was thinking about fixing it in this PR, but I would like to hear your thoughts first to make sure my findings are correct and the fix suggested in the issue is the right one. |
…it-leak-sql-panel
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||
Fixed by #2403. Thanks @matthiask 👍🏼 |
| should_record = not skip_toolbar_queries and any( | ||
| table in sql for table in DDT_MODELS | ||
| ) |
There was a problem hiding this comment.
I don't believe we want to hide the table-less commands here.
|
|
||
|
|
||
| @contextlib.contextmanager | ||
| def no_sql_recording(): |
There was a problem hiding this comment.
I think the name here may be better as suppress_sql_recording as it's a bit clearer on its purpose.
| def save_panel(cls, request_id: str, panel_id: str, data: Any = None): | ||
| """Save the panel data for the given request_id""" | ||
| with transaction.atomic(): | ||
| from debug_toolbar.panels.sql.tracking import no_sql_recording |
There was a problem hiding this comment.
I think this creates a circular dependency. It may be a good idea to move to a different place.
There was a problem hiding this comment.
Sure. I used this aproach to avoid the circular import of tracking module. Some suggestion from where I can move the function?
There was a problem hiding this comment.
@tim-schilling is it fine to move the sql_recording and the suppress_sql_recording (old no_sql_recording) to debug_toolbar.utils? Or may I create a new module for this? Because of the import in panels.sql.__init__.py any module inside of panels.sql will cause the circular import. So the best solution I tested was create a new module outside of panels.sql or use the debug_toolbar.utils. What do you think?
There was a problem hiding this comment.
I think creating a new module would be fine, though perhaps we should rename it slightly so it's not tailored to the sql panel.
tim-schilling
left a comment
There was a problem hiding this comment.
Thank you for working on this. I think if we adopt the ContextVar approach, we can remove the SQL inspection logic altogether and solely rely on the ContextVar. Did you consider removing that?
I did, but I'd like to get the reviews first and have a best pic of what to do next. I wasn't sure if could be other applications for the SQL inspection before I remove it. What do you think? |
@marcosalvesdev that SQL inspection was included for this use-case. I think if we need it again in the future, we can re-add it. |
Got it, I'll remove it so. |
Description
Fixes #2338
When using CacheStore with Django's DatabaseCache backend, the transaction.atomic() calls used internally by the toolbar to persist its own data were emitting BEGIN/COMMIT (or SAVEPOINT/RELEASE SAVEPOINT inside a
test transaction) into the SQL panel. These commands carry no table name, so the existing
SKIP_TOOLBAR_QUERIES filter — which works by matching known toolbar table names in the SQL string — could
never suppress them.
The fix introduces a ContextVar (sql_recording, default True) in tracking.py and a context manager
no_sql_recording() that sets it to False for the duration of the toolbar's own persistence operations. The
decision logic in NormalCursorMixin._record() now consults this flag: when sql_recording=False, only
queries that explicitly reference a known toolbar table (and where SKIP_TOOLBAR_QUERIES=False) are recorded — preserving the existing opt-in behavior while suppressing table-less control commands unconditionally.
no_sql_recording() is applied in two places:
by DatabaseStore
Checklist:
AI/LLM Usage