Opened 14 years ago

Closed 14 years ago

Last modified 13 years ago

#12612 closed (fixed)

SQLite3 executemany() exception handling is too broad

Reported by: Niels <niels@…> Owned by: Gabriel Hurley
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: no UI/UX: no

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 Gabriel Hurley 14 years ago.
Removes error suppression from sqlite executemany parameter handling and adds tests.

Download all attachments as: .zip

Change History (7)

comment:1 by Niels <niels@…>, 14 years ago

Summary: SQLite3 executemany() method catches too much exceptionsSQLite3 executemany() exception handling is too broad

comment:2 by Russell Keith-Magee, 14 years ago

milestone: 1.2
Triage Stage: UnreviewedAccepted

comment:3 by Gabriel Hurley, 14 years ago

Has patch: set
Owner: changed from nobody to Gabriel Hurley
Status: newassigned

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.

by Gabriel Hurley, 14 years ago

Attachment: 12612_patch.diff added

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

comment:4 by Russell Keith-Magee, 14 years ago

Resolution: fixed
Status: assignedclosed

(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 by Russell Keith-Magee, 14 years ago

(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 by Jacob, 13 years ago

milestone: 1.2

Milestone 1.2 deleted

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