Skip to content

Fix BEGIN leaking into SQL panel when using DatabaseCache store#2397

Open
marcosalvesdev wants to merge 2 commits into
django-commons:mainfrom
marcosalvesdev:fix/2338-begin-commit-leak-sql-panel
Open

Fix BEGIN leaking into SQL panel when using DatabaseCache store#2397
marcosalvesdev wants to merge 2 commits into
django-commons:mainfrom
marcosalvesdev:fix/2338-begin-commit-leak-sql-panel

Conversation

@marcosalvesdev

Copy link
Copy Markdown

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:

  • _UntrackedCache.untracked() — covers all cache operations made by CacheStore
  • DatabaseStore.set() and DatabaseStore.save_panel() — covers the transaction.atomic() calls made directly
    by DatabaseStore

Checklist:

  • I have added the relevant tests for this change.
  • I have added an item to the Pending section of docs/changes.rst.

AI/LLM Usage

  • This PR includes code generated with the help of an AI/LLM

@marcosalvesdev

marcosalvesdev commented Jun 27, 2026

Copy link
Copy Markdown
Author

👋🏼 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.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  debug_toolbar
  store.py
  debug_toolbar/panels/sql
  tracking.py 273-274
Project Total  

This report was generated by python-coverage-comment-action

@marcosalvesdev

Copy link
Copy Markdown
Author

👋🏼 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.

Fixed by #2403. Thanks @matthiask 👍🏼

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 this pull request may close these issues.

Using CacheStore with DatabaseCache backend shows BEGIN sql queries

1 participant