Opened 6 years ago

Closed 5 years ago

Last modified 4 years ago

#12612 closed (fixed)

SQLite3 executemany() exception handling is too broad

Reported by: Niels <niels@…> Owned by: gabrielhurley
Component: Database layer (models, ORM) Version: 1.2-alpha
Severity: Keywords: sqlite, sqlite3
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

In SQLiteCursorWrapper.executemany() (http://code.djangoproject.com/browser/django/trunk/django/db/backends/sqlite3/base.py#L191)
convert_query (line 199) is called, but if you have an extra (accidental) %s in your query the
replacement of '%s' by ? will raise an exception (TypeError: not enough arguments for format string) which is then
caught and ignored by except(IndexError,TypeError) at line 195.

None will be returned and whoever is calling executemany will be left wondering why nothing happened. This bug basically causes bad input to be ignored instead of raising an exception.

Attachments (1)

12612_patch.diff (2.4 KB) - added by gabrielhurley 5 years ago.
Removes error suppression from sqlite executemany parameter handling and adds tests.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 6 years ago by Niels <niels@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from SQLite3 executemany() method catches too much exceptions to SQLite3 executemany() exception handling is too broad

comment:2 Changed 6 years ago by russellm

  • milestone set to 1.2
  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 5 years ago by gabrielhurley

  • Has patch set
  • Owner changed from nobody to gabrielhurley
  • Status changed from new to assigned

This was introduced in @6218 as the solution to #4765. I agree with Niels that the solution in that patch (suppressing an error regarding a malformed query and returning None) is not the right direction.

I can't find any discussion of why that decision was made, and there are no tests for executemany() anywhere that I see.

I'm adding a patch which lets the error bubble up and adds tests to check for the behaviors noted in this ticket and #4896.

Changed 5 years ago by gabrielhurley

Removes error suppression from sqlite executemany parameter handling and adds tests.

comment:4 Changed 5 years ago by russellm

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [12836]) Fixed #12612 -- Corrected handling of parameter formatting in SQLite backend so that executemany raises exceptions when bad parameter counts are provided. Thanks to Niels <niels@…> for the report, and Gabriel Hurley for the help narrowing down the problem.

comment:5 Changed 5 years ago by russellm

(In [12837]) [1.1.X] Fixed #12612 -- Corrected handling of parameter formatting in SQLite backend so that executemany raises exceptions when bad parameter counts are provided. Thanks to Niels <niels@…> for the report, and Gabriel Hurley for the help narrowing down the problem.

Backport of r12836 from trunk.

comment:6 Changed 4 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

Note: See TracTickets for help on using tickets.
Back to Top