Fix executemany() raising from the SQL panel - #2437
Conversation
|
@alliasgher please be more concise when using LLMs to generate messages. They are overly verbose and some of it is unhelpful. |
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| times = len(params) | ||
| except TypeError: | ||
| times = "?" | ||
| display_sql = f"{times} times: {sql}" |
There was a problem hiding this comment.
I think this should probably be internationalized.
There was a problem hiding this comment.
Done in aead842d, using ngettext so the count pluralizes, with a separate string for the ? case when params has no len(). Added a singular test.
| * Added support for Django 6.1. | ||
| * Fixed ``cursor.executemany()`` raising from the SQL panel instead of being | ||
| recorded. The statement is now shown as ``N times: <sql>``, matching | ||
| Django's own debug cursor. |
There was a problem hiding this comment.
This has to be moved up to the section covering the next release now.
There was a problem hiding this comment.
Moved to Pending in aead842d. I rebased on main and it had landed inside the released 7.1.0 section.
|
|
||
| with connection.cursor() as cursor: | ||
| cursor.executemany( | ||
| "INSERT INTO tests_binary (field) VALUES (%s)", |
There was a problem hiding this comment.
Just asking: Is there a specific reason you chose tests_binary for this?
There was a problem hiding this comment.
It is the only backend-agnostic model in tests/models.py. The other two are Postgres-only (PostgresJSON) and GeoDjango. Binary also has a single field, so the INSERT stays one column. Happy to switch if you prefer another.
_record() passed the param list straight to _last_executed_query(), but the backend's last_executed_query() expects a flat sequence of scalars. On sqlite it re-quotes them with QUOTE(?) placeholders, so a sequence of param sequences raises ProgrammingError. It happens in a finally: block after the statement has already run, so the write lands and the request still 500s. Thread a many flag through _record() and render the statement as 'N times: <sql>' in that case, which is what Django's own CursorDebugWrapper does. The select/explain/profile buttons re-execute raw_sql with params through plain cursor.execute, which fails the same way, so they are hidden for executemany queries.
Use ngettext for the repeat count and move the changelog entry into Pending, which a release moved out from under it.
80560cf to
aead842
Compare
Description
With the SQL panel instrumented,
cursor.executemany(sql, param_list)raises instead of being recorded.executemany()hands the list of param sequences to the same_record()thatexecute()uses, and_record()'sfinally:block callsself._last_executed_query(sql, params). The backend'slast_executed_query()expects a flat sequence of scalars — sqlite re-quotes them withcursor.execute("SELECT " + ", ".join(["QUOTE(?)"] * len(params)), params)— so binding a tuple fails:Two things make it worse than a display glitch: only the
self._decode(params)call is wrapped incontextlib.suppress(TypeError), and it is infinally:after the statement has run. So the write commits and the request still 500s, with nothing recorded in the panel.I re-reproduced this on current
main(49f4ef3) with Django 6.1 / Python 3.12 / sqlite rather than relying on the 2018 report — the traceback shape has changed since (it used to surface asTypeError: not enough arguments for format string), but the cause is the same one @matthiask identified in the issue thread.Postgres happens to escape it because its
last_executed_query()reads the driver's already-interpolated query instead of re-quoting, so this is not sqlite-specific by design — it just depends on which backend you use. The fix is in the toolbar, not per-vendor.Approach
Mirror Django core rather than invent behaviour.
django/db/backends/utils.pyalready has this exact concept:CursorDebugWrapper.executemanypassesmany=True, anddebug_sqlthen logs"%s times: %s" % (times, sql)instead of callinglast_executed_query. The toolbar's wrapper had nomanyconcept at all, so this threads one through_record()and does the same.I also hid the Sel/Expl/Prof buttons for these queries.
SQLSelectForm.select/explain/profilere-runquery["raw_sql"]withquery["params"]through a plaincursor.execute, which fails the same way — so clicking them on an executemany row would just reproduce the original error. I realise #2393 deliberately made those buttons unconditional; this is a narrower exclusion for the one case where the params are structurally not re-executable, not a rollback of that.Scope
Worth being precise about the blast radius: modern Django does not route
bulk_createthroughexecutemany(it builds a single multi-row INSERT), so this affects code callingcursor.executemany()on raw SQL. That is real but not universal — this is not a "fixes bulk_create" change.Fixes #1069
Testing
tests/panels/test_sql.pyhad noexecutemanycoverage at all. Added three cases: the main one, an empty param list, and a guard that ordinaryexecute()is not marked asmany. The first fails on main with theProgrammingErrorabove.tests/panels/test_sql.py→ 35 passed, 7 skipped on sqlite.For the rest of the suite I should be upfront: I get 61 failures locally both with and without this change — I diffed the two failure sets and they are byte-identical, and the pass count goes 267 → 270 (my three tests). They are environmental, e.g.
test_checks.pyfails withDatabaseOperationForbidden: Database queries to 'default' are not allowed in SimpleTestCase subclasses. So: nothing here is a regression, but I have not run a fully green suite and did not want to imply otherwise.pre-commit run --files ...passes on all four changed files.Checklist:
docs/changes.rst.AI/LLM Usage