Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

execute & mogrify don't fail with no format placeholder but with arguments given

#189

Hi,

I realized that cursor.{execute,mogrify} do accept not-empty arguments (either dict or tuple) while the query string has no format placeholder :

>>> sys.version
'3.3.1 (default, Sep 25 2013, 19:29:01) \n[GCC 4.7.3]'

>>> psycopg2.__version__
'2.5.1 (dt dec pq3 ext)'

>>> conn = psycopg2.connect(database='greg')
>>> cursor = conn.cursor()

Problem 1:

>>> cursor.execute('SELECT 1', dict(a=1, b=2) )
>>> cursor.mogrify('SELECT 1', dict(a=1, b=2) )
b'SELECT 1'

>>> cursor.execute('SELECT 1', (1,2) )
>>> cursor.mogrify('SELECT 1', (1,2) )
b'SELECT 1'
>>>

I'm not sure if this is a bug or a feature.
I would say the first one when compared with this :

1) too many (un-named) arguments given:

>>> cursor.mogrify('SELECT %s', (1,2) )
Traceback (most recent call last):
  File "<pyshell#215>", line 1, in <module>
    cursor.mogrify('SELECT %s', (1,2) )
TypeError: not all arguments converted during string formatting
>>>

2) or not enough arguments given:

>>> cursor.mogrify('SELECT %s, %s', (1,))
Traceback (most recent call last):
  File "<pyshell#217>", line 1, in <module>
    cursor.mogrify('SELECT %s, %s', (1,))
IndexError: tuple index out of range
>>>

or the same with dict argument:

>>> cursor.mogrify('SELECT %(a)s, %(b)s', dict(a=1))
Traceback (most recent call last):
  File "<pyshell#218>", line 1, in <module>
    cursor.mogrify('SELECT %(a)s, %(b)s', dict(a=1))
KeyError: 'b'
>>>

IMHO: The only "valid" & "special" case is with the use of a query with named arguments and a dict where there are more keys than in the query :

>>> cursor.mogrify('SELECT %(a)s', dict(a=1, b=1))
b'SELECT 1'
>>>

Now there is a last special case : query with (named or not) arguments but with no (or None) argument :

Problem 2:

>>> cursor.mogrify('SELECT %(a)s')
b'SELECT %(a)s'
>>> cursor.mogrify('SELECT %s')
b'SELECT %s'
>>>

where I would also have expected a TypeError because afaik such query will inevitably fail if executed.

wdyt of such thoughts ?

would it be correct to so declare the 2 problems (1 & 2, probably related) as bugs ?

as far as I know problem 2 will always trigger a database error so it should'nt be a problem to make it raise a TypeError.
But I can see the problem with making a typeerror on problem-1 there could have some people which are using this form of query.. then maybe in a multi-steps way : first put a warning then on some later release change it to a typeerror ?

regards,

Reported by GrĂ©gory Starck · December 30th, 2013 @ 10:16 AM

State: new
Milestone: psycopg3
Assigned to: nobody

Activity

  1. Daniele Varrazzo
    Daniele Varrazzo
    • Milestone set to psycopg3

    About problem 1: if the query contains no placeholder I'd argue it should also allow an empty list as argument (it is a sequence with the same number of item as the number of the placeholder in the query). It should also allow any dictionary because the set of keys in the query string is always a subset (it's empty) of the keys in the args dictionary. Hence the only thing psycopg should reject is a non-empty sequence with queries with no placeholder. I agree it's a defect but I don't know if it's worth adding an explicit check to fix it.

    About problem 2, I don't think it is related, no. I think it is related to a different problem which actually bothers me more: psycopg doesn't parse the %-escaping at all if no parameter is specified. This becomes evident as soon as the % operator is used: when no parameter is specified % works ok; if a parameter is specified it must be escaped as %%. Arguably the %-escaping should be uniform. Unfortunately now it's too late to fix it: every query using an operator including % and no param should be rewritten (probably there aren't many in the wild, but a few are definitely there). I would fix this problem only in a non-100%-backward-compatible psycopg3 (or was 100%%?).

    Note that the workaround for the not uniform %% escaping is exactly to pass an empty parameters list to the query.

    >>> cur.execute("select '%%s'")
    >>> cur.fetchone()[0]
    '%%s'
    
    >>> cur.execute("select '%%s'", [])
    >>> cur.fetchone()[0]
    '%s'
    

    I will leave this bug open as an idea to fix in psycopg3, but probably won't do anything in psycopg2 for backward compatibility reason.

    January 7th, 2014 @ 05:02 PM