Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pymssql 2.x does not support "%(foo)d" parameter substitution style; pymssql 1.x did #155

Closed
kunxi opened this issue Jan 7, 2014 · 10 comments

Comments

@kunxi
Copy link

kunxi commented Jan 7, 2014

The PEP 249 support various params binding.

qmark is not supported:

cur.execute('select @@version where 1=?', (1,))

numeric is not supported:

cur.execute('select @@version where 1=:1', (1,))

named is not supported:

cur.execute('select @@version where 1=:name', dict(name=1))

format is partially supported:

cur.execute('select @@version where 1=%d', (1,)) # OK
cur.execute('select @@version where 1=%1d', (1,)) # fail

pyformat does not support %(foo)d:

cur.execute('select @@version where 1=%(name)s', dict(name='1')) # OK
cur.execute('select @@version where 1=%(name)d', dict(name=1)) # fail
@msabramo
Copy link
Contributor

msabramo commented Jan 8, 2014

pymssql advertises that it supports only the pyformat paramstyle. In pymssql.pyx, it has this line:

paramstyle = 'pyformat'

But perhaps this should be mentioned in the new docs from @ramiro if it's not already there.

It sounds like you also might've found a problem with the pyformat stuff relating to '%(foo)d"? Is it the d (integer) specifically? The two lines you posted look identical; maybe a copy/paste error? Maybe the failing one should be d instead of s?

@msabramo
Copy link
Contributor

msabramo commented Jan 8, 2014

Looking at the code some more, I can see that pymssql advertises that it supports pyformat as I mentioned. However it also supports format (looking at the code for the substitute_params function in _mssql.pyx).

The tricky thing about pyformat is that if you look at PEP 249, the only example they show for pyformat is %(foo)s -- e.g.: with the s. It's not clear whether they meant that other things like d, f, or i are supported.

I found this thread where someone asked about this and it seemed the general consensus was that only the s form was required. This includes Marc-André Lemburg, who authored PEP 249.

It seems that pymssql 1.X supported those extra forms though, whereas pymssql 2.X does not.

I see 3 options:

  1. Convert your code to use the pyformat style but only using the s.
  2. Convert your code to use the format style with %s, %d, etc.
  3. Modify pymssql 2.X to support stuff like %(foo)d, which may not be strictly required by PEP-249, but it was supported by pymssql 1.X so maybe it's nice.

Let me know if you have other thoughts.

@rsyring
Copy link
Contributor

rsyring commented Jan 8, 2014

On 1/8/14, 1:38 AM, Marc Abramowitz wrote:

  1. Modify pymssql 2.X to support stuff like %(foo)d, which may not be
    strictly required by PEP-249, but it was supported by pymssql 1.X so
    maybe it's nice.
    I think we should avoid this option and instead note in the docs as a BC
    issue from 1.x to 2.x. Supporting what the PEP requires and what the
    PEP author meant to intend seems adequate IMO.

Randy Syring
Husband | Father | Redeemed Sinner

/"For what does it profit a man to gain the whole world
and forfeit his soul?" (Mark 8:36 ESV)/

@kunxi
Copy link
Author

kunxi commented Jan 8, 2014

Just fixed the typo in the comment 1.

cur.execute('select @@version where 1=%(name)d', dict(name=1)) # fail

@kunxi
Copy link
Author

kunxi commented Jan 8, 2014

I prefer 3, we keep the backward compatibility as pymssql 1.x, that would make the migration much easier.

@msabramo
Copy link
Contributor

msabramo commented Jan 9, 2014

Just changed the title from:

The pyformat params binding is not supported

to:

pymssql 2.x does not support "%(foo)d" parameter substitution style; pymssql 1.x did

@rsyring
Copy link
Contributor

rsyring commented Jan 9, 2014

@kunxi but a major release is the place to break BC if we are going to break it.

Marc, I think this one is up to you since you are currently working on the code. From an idealist perspective, supporting the differing characters "%s, %d" implies some kind of different handling based on type, IMO. It seems best to avoid that. At the same time, it's not likely to cause much headache in practice, so....shrugs :)

@msabramo
Copy link
Contributor

msabramo commented Jan 9, 2014

@rsyring: @kunxi and I work together and we spoke about this yesterday and he seems OK with changing his code to use a different paramstyle, as the %(foo)s form should work in both 1.x and 2.x

So I'm closing this. @kunxi: If you run into problems and you have to have this, let me know and we can reopen.

@msabramo msabramo closed this as completed Jan 9, 2014
@msabramo
Copy link
Contributor

msabramo commented Jan 9, 2014

Oh, I forgot to mention, that I added info about this to the docs:

http://pymssql.org/migrate_1_x_to_2_x.html#parameter-substitution

@bra-fsn
Copy link

bra-fsn commented Oct 22, 2019

Just for the record: the above URL now is:
http://pymssql.org/en/stable/migrate_1_x_to_2_x.html#parameter-substitution

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

No branches or pull requests

4 participants