Strange behavior -=. combinator for negative values. #195

Closed
Matthiasvanderhallen opened this Issue Jan 10, 2014 · 14 comments

Comments

Projects
None yet
5 participants
@Matthiasvanderhallen

When using the -=. combinator to update a field, no update is performed when the value on the right hand side is negative. +=. has no such problems. There is no compilation or runtime error, simply the database not updating.

I provided a mini test that shows the erratic behavior.

http://lpaste.net/98283

Version information:

  • persistent-postgresql
    Installed versions: 1.2.1
  • persistent
    Installed versions: 1.2.3.0
  • Postgres93.app provides the postgresql server, Postgresql 9.3.1.
  • Mac OS X 10.9.1

If I've forgotten any information you need, I'm sorry and I will try to provide it as quickly as possible.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Jan 11, 2014

Member

@lpsmith If I run the following:

{-# LANGUAGE OverloadedStrings #-}
import Database.PostgreSQL.Simple

main :: IO ()
main = do
    conn <- connectPostgreSQL "host=localhost dbname=test user=test password=test port=5432"

    formatQuery
        conn
        "update account set balance=balance-? where id=?"
        (-3 :: Int, 5 :: Int) >>= print

The result is:

"update account set balance=balance--3 where id=5"

Shouldn't the result instead be:

"update account set balance=balance-(-3) where id=5"
Member

snoyberg commented Jan 11, 2014

@lpsmith If I run the following:

{-# LANGUAGE OverloadedStrings #-}
import Database.PostgreSQL.Simple

main :: IO ()
main = do
    conn <- connectPostgreSQL "host=localhost dbname=test user=test password=test port=5432"

    formatQuery
        conn
        "update account set balance=balance-? where id=?"
        (-3 :: Int, 5 :: Int) >>= print

The result is:

"update account set balance=balance--3 where id=5"

Shouldn't the result instead be:

"update account set balance=balance-(-3) where id=5"
@Matthiasvanderhallen

This comment has been minimized.

Show comment
Hide comment
@Matthiasvanderhallen

Matthiasvanderhallen Jan 11, 2014

Since -- is the line comment token in postgresql, doesn't this imply that any other fields updated in the same update list before the

balance -=. value

are updated without the where clause present? (I.e. on the entire table?)

I'm lucky I never did that in my main application :).

Since -- is the line comment token in postgresql, doesn't this imply that any other fields updated in the same update list before the

balance -=. value

are updated without the where clause present? (I.e. on the entire table?)

I'm lucky I never did that in my main application :).

@lpsmith

This comment has been minimized.

Show comment
Hide comment
@lpsmith

lpsmith Jan 11, 2014

Contributor

Well, this wouldn't be an issue if postgresql-simple used protocol-level parameters :-/

As for what it should be, I dunno. I'm sure you'll get the same behavior in mysql-simple, and thus probably the same with persistent-mysql. You could fix it by inserting a space in between the - and the ?, or parens, or whatever you like. It wouldn't necessarily involve modifying the ToField instance for Int.

Contributor

lpsmith commented Jan 11, 2014

Well, this wouldn't be an issue if postgresql-simple used protocol-level parameters :-/

As for what it should be, I dunno. I'm sure you'll get the same behavior in mysql-simple, and thus probably the same with persistent-mysql. You could fix it by inserting a space in between the - and the ?, or parens, or whatever you like. It wouldn't necessarily involve modifying the ToField instance for Int.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Jan 12, 2014

Member

@lpsmith Maybe postgresql-simple should wrap all numeric types (or perhaps all types?) in parentheses. I don't think there would be any downside to this.

I suppose this can be done at the persistent level as well if you prefer, I'm just surprised by this behavior, as I'm used to the prepared statement API provided by SQLite, which handles this case properly.

Member

snoyberg commented Jan 12, 2014

@lpsmith Maybe postgresql-simple should wrap all numeric types (or perhaps all types?) in parentheses. I don't think there would be any downside to this.

I suppose this can be done at the persistent level as well if you prefer, I'm just surprised by this behavior, as I'm used to the prepared statement API provided by SQLite, which handles this case properly.

@lpsmith

This comment has been minimized.

Show comment
Hide comment
@lpsmith

lpsmith Jan 12, 2014

Contributor

Have you tried rerunning this test on persistent-mysql? My money is that you'll get the same result.

Contributor

lpsmith commented Jan 12, 2014

Have you tried rerunning this test on persistent-mysql? My money is that you'll get the same result.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Jan 12, 2014

Member

That may be, but does that at all detract from the question of whether or not this is buggy behavior in postgresql-simple?

Member

snoyberg commented Jan 12, 2014

That may be, but does that at all detract from the question of whether or not this is buggy behavior in postgresql-simple?

@lpsmith

This comment has been minimized.

Show comment
Hide comment
@lpsmith

lpsmith Jan 13, 2014

Contributor

No, my point is since this is a judgement call, looking at what other packages do can help inform that judgement call. Personally, I always put a space around ?.

And from a pragmatic perspective, if this is postgresql-simple's bug, and it also manifests in persistent-mysql, I don't know how easy or difficult it might be to get it fixed in mysql-simple as well given that bos seems to have pretty much stopped working on that project.

There are other issues with the current parameterization interface that needs to be fixed. Parenthesizing integer literals should fix some cases, but not others.

And I tried this case out in psycopg2:

$ python
Python 2.7.3 (default, Sep 26 2013, 20:03:06) 
[GCC 4.6.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import psycopg2
>>> conn = psycopg2.connect("")
>>> cur = conn.cursor()
>>> cur.mogrify("update account set balance=balance-%s where id=%s", (-3,5))
'update account set balance=balance- -3 where id=5'
>>> cur.mogrify("update account set balance=balance-%s where id=%s", (3,5))
'update account set balance=balance-3 where id=5'

Yeah, I'm not going to take that approach, but it does handle this as the programmer intended.

Contributor

lpsmith commented Jan 13, 2014

No, my point is since this is a judgement call, looking at what other packages do can help inform that judgement call. Personally, I always put a space around ?.

And from a pragmatic perspective, if this is postgresql-simple's bug, and it also manifests in persistent-mysql, I don't know how easy or difficult it might be to get it fixed in mysql-simple as well given that bos seems to have pretty much stopped working on that project.

There are other issues with the current parameterization interface that needs to be fixed. Parenthesizing integer literals should fix some cases, but not others.

And I tried this case out in psycopg2:

$ python
Python 2.7.3 (default, Sep 26 2013, 20:03:06) 
[GCC 4.6.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import psycopg2
>>> conn = psycopg2.connect("")
>>> cur = conn.cursor()
>>> cur.mogrify("update account set balance=balance-%s where id=%s", (-3,5))
'update account set balance=balance- -3 where id=5'
>>> cur.mogrify("update account set balance=balance-%s where id=%s", (3,5))
'update account set balance=balance-3 where id=5'

Yeah, I'm not going to take that approach, but it does handle this as the programmer intended.

@lpsmith

This comment has been minimized.

Show comment
Hide comment
@lpsmith

lpsmith Jan 13, 2014

Contributor

Ahh, well, psycopg2 puts a space before negative integer literals, so maybe that would be ok.

>>> cur.mogrify("update account set balance=balance- %s where id=%s", (3,5))
'update account set balance=balance- 3 where id=5'
>>> cur.mogrify("update account set balance=balance- %s where id=%s", (-3,5))
'update account set balance=balance-  -3 where id=5'
Contributor

lpsmith commented Jan 13, 2014

Ahh, well, psycopg2 puts a space before negative integer literals, so maybe that would be ok.

>>> cur.mogrify("update account set balance=balance- %s where id=%s", (3,5))
'update account set balance=balance- 3 where id=5'
>>> cur.mogrify("update account set balance=balance- %s where id=%s", (-3,5))
'update account set balance=balance-  -3 where id=5'

@gregwebs gregwebs added the Postgres label Aug 4, 2014

@MaxGabriel

This comment has been minimized.

Show comment
Hide comment
@MaxGabriel

MaxGabriel Jun 7, 2015

Member

@lpsmith Based on a quick test, mysql-simple does not have this behavior.

Excuse the lack of a real script; I just threw this code into a Handler of a mysql yesod app I already had setup:

Bing
    number Int
    deriving Show
bingId <- runDB $ insert $ Bing { bingNumber = 5 }
runDB $ update bingId [BingNumber -=. (-1)]
updatedBing <- runDB $ get bingId
traceShowM updatedBing

Using negative 1 for the update gives 6 and using positive 1 for the update gives 4.

Member

MaxGabriel commented Jun 7, 2015

@lpsmith Based on a quick test, mysql-simple does not have this behavior.

Excuse the lack of a real script; I just threw this code into a Handler of a mysql yesod app I already had setup:

Bing
    number Int
    deriving Show
bingId <- runDB $ insert $ Bing { bingNumber = 5 }
runDB $ update bingId [BingNumber -=. (-1)]
updatedBing <- runDB $ get bingId
traceShowM updatedBing

Using negative 1 for the update gives 6 and using positive 1 for the update gives 4.

@lpsmith

This comment has been minimized.

Show comment
Hide comment
@lpsmith

lpsmith Jun 8, 2015

Contributor

Yes, mysql-simple most certainly does exhibit this formatting behavior as well. I believe that this issue has since been fixed in Persistent, which is what your sample code is using, not mysql-simple.

Contributor

lpsmith commented Jun 8, 2015

Yes, mysql-simple most certainly does exhibit this formatting behavior as well. I believe that this issue has since been fixed in Persistent, which is what your sample code is using, not mysql-simple.

@lpsmith

This comment has been minimized.

Show comment
Hide comment
@lpsmith

lpsmith Jun 8, 2015

Contributor
$ ghci
GHCi, version 7.6.3: http://www.haskell.org/ghc/  :? for help
> import Database.MySQL.Simple
> c <- connect defaultConnectInfo { ... }
> formatQuery c "update account set balance=balance-? where id=?" (-3 :: Int, 5 :: Int)
"update account set balance=balance--3 where id=5"
Contributor

lpsmith commented Jun 8, 2015

$ ghci
GHCi, version 7.6.3: http://www.haskell.org/ghc/  :? for help
> import Database.MySQL.Simple
> c <- connect defaultConnectInfo { ... }
> formatQuery c "update account set balance=balance-? where id=?" (-3 :: Int, 5 :: Int)
"update account set balance=balance--3 where id=5"
@MaxGabriel

This comment has been minimized.

Show comment
Hide comment
@MaxGabriel

MaxGabriel Jun 8, 2015

Member

My mistake @lpsmith.

Member

MaxGabriel commented Jun 8, 2015

My mistake @lpsmith.

@lpsmith

This comment has been minimized.

Show comment
Hide comment
@lpsmith

lpsmith Jun 8, 2015

Contributor

No problem.

Contributor

lpsmith commented Jun 8, 2015

No problem.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Jul 19, 2015

Member

Closing out old issues, please reopen if still relevant

Member

snoyberg commented Jul 19, 2015

Closing out old issues, please reopen if still relevant

@snoyberg snoyberg closed this Jul 19, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment