Question marks in string literals cause substitution errors#54
Question marks in string literals cause substitution errors#54paul-rouse merged 8 commits intopaul-rouse:masterfrom
Conversation
| -- break a fragment if the question mark is in a string literal. | ||
| splitQuery :: ByteString -> [Builder] | ||
| splitQuery s = | ||
| reverse $ fmap (fromByteString . BS.pack . reverse) $ |
There was a problem hiding this comment.
TODO: dlist this
test/main.hs
Outdated
| describe "Database.MySQL.Simple.unitSpec" unitSpec | ||
| describe "Database.MySQL.Simple.integrationSpec" $ integrationSpec conn |
There was a problem hiding this comment.
changed up the hspec formatting a bit
There was a problem hiding this comment.
Sorry, I've just introduced a conflict here - maybe rebase, if you would.
| '\'' -> | ||
| normal ret (c : acc) cs | ||
| _ -> | ||
| quotes ret (c : acc) cs |
There was a problem hiding this comment.
I thought about breaking out attoparsec for this. Not sure what any potential performance improvements would be.
I'll likely need to factor this out into a library, anyway, since Postgres also needs it. Then that lib can be tested/benchmarked and work well.
There was a problem hiding this comment.
attoparsec is already a dependency, so absolutely fine if you want to use it!
| describe "splitQuery" $ do | ||
| it "works for a single question mark" $ do | ||
| splitQuery "select * from foo where name = ?" | ||
| `shouldBe` | ||
| ["select * from foo where name = ", ""] | ||
| it "works with a question mark in a string literal" $ do | ||
| splitQuery "select 'hello?'" | ||
| `shouldBe` | ||
| ["select 'hello?'"] | ||
| it "works with many question marks" $ do | ||
| splitQuery "select ? + ? + what from foo where bar = ?" | ||
| `shouldBe` | ||
| ["select ", " + ", " + what from foo where bar = ", ""] |
There was a problem hiding this comment.
These tests ensure that it behaves appropriately when formatting for other functions
| it "can have question marks in string literals" $ do | ||
| result <- query_ conn "select 'hello?'" | ||
| result `shouldBe` [Only ("hello?" :: Text)] | ||
| describe "query" $ do | ||
| it "can have question marks in string literals" $ do | ||
| result <- query conn "select 'hello?'" () | ||
| result `shouldBe` [Only ("hello?" :: Text)] | ||
| describe "with too many query params" $ do | ||
| it "should have the right message" $ do | ||
| (query conn "select 'hello?'" (Only ['a']) :: IO [Only Text]) | ||
| `shouldThrow` | ||
| (\e -> fmtMessage e == "0 '?' characters, but 1 parameters") | ||
| describe "with too few query params" $ do | ||
| it "should have the right message" $ do | ||
| (query conn "select 'hello?' = ?" () :: IO [Only Text]) | ||
| `shouldThrow` | ||
| (\e -> fmtMessage e == "1 '?' characters, but 0 parameters") | ||
| describe "formatQuery" $ do | ||
| it "should not blow up on a question mark in string literal" $ do | ||
| formatQuery conn "select 'hello?'" () | ||
| `shouldReturn` | ||
| "select 'hello?'" |
There was a problem hiding this comment.
and then these ensure it works in query etc
| describe "with too many query params" $ do | ||
| it "should have the right message" $ do | ||
| (query conn "select 'hello?'" (Only ['a']) :: IO [Only Text]) | ||
| `shouldThrow` | ||
| (\e -> fmtMessage e == "0 '?' characters, but 1 parameters") | ||
| describe "with too few query params" $ do | ||
| it "should have the right message" $ do | ||
| (query conn "select 'hello?' = ?" () :: IO [Only Text]) | ||
| `shouldThrow` | ||
| (\e -> fmtMessage e == "1 '?' characters, but 0 parameters") |
There was a problem hiding this comment.
love too hspec's shouldThrow
|
Thanks for spotting and fixing this! There's a minor conflict now, so would you mind rebasing - it would be slightly cleaner. I belatedly got round to Github workflow CI, and needed to make a small change in If you want to use |
|
Great - thanks! |
2 participants
Currently just a failing test but I hope to have a fix up soon
Cf yesodweb/persistent#1375