Add Support for prepared statements - #44
Conversation
| , ((#const MYSQL_TYPE_VAR_STRING), VarString) | ||
| , ((#const MYSQL_TYPE_STRING), String) | ||
| , ((#const MYSQL_TYPE_GEOMETRY), Geometry) | ||
| #if defined(MYSQL_TYPE_JSON) |
There was a problem hiding this comment.
This seems to actually be causing some issues and is why i ended up on this journey in the first place.
I think it is safe to remove since MYSQL_TYPE_JSON has been supported since 2015
| withStatement statement $ \mysqlStmt -> do | ||
| throwErrnoIf_ (> 0) "mysql_stmt_execute" $ mysql_stmt_execute mysqlStmt | ||
|
|
||
| storeResult :: Statement -> IO () |
There was a problem hiding this comment.
should this name overlap with the Base.storeResult or should it be storeResultStatement
| connectPort = 33306, | ||
| connectUser = "travis", | ||
| connectPassword = "esqutest", | ||
| connectDatabase = "esqutest", | ||
| connectOptions = [ |
There was a problem hiding this comment.
need to revert this. Was reusing my test db from esqueleto
| it "seems to be connected" $ do | ||
| query conn "select 1 + 1" | ||
| result <- useResult conn | ||
| result <- storeResult conn |
There was a problem hiding this comment.
when using useResult the connection would be left in an unusable state and prepare would fail. By switching to storeResult this issue went away.
| | Blob ByteString | ||
| deriving (Eq, Show) | ||
|
|
||
| withValue :: Value -> (MYSQL_BIND -> IO a) -> IO a |
There was a problem hiding this comment.
Is there a better way to implement this or is this kind of boilerplate inevitable?
| error err | ||
| pure $ Statement mysql stmt | ||
|
|
||
| withConn :: Statement -> (Ptr MYSQL -> IO a) -> IO a |
There was a problem hiding this comment.
the pointer to the underlying MYSQL is only needed for a mysql_error call which is only relevant for mysql_stmt_close all other functions will call mysql_stmt_error
Should we stop holding on to the ref for the duration of the Statement? Is it possible for the Connection to get dropped if Statement doesn't hold it?
| } | ||
|
|
||
| Decimal _ -> | ||
| error "Unsupported input type: Decimal" |
There was a problem hiding this comment.
Should we just treat this like a blob on input instead of throwing a runtime error?
|
One thing which makes me nervous about this package is the lack of detailed tests. This hasn't been too serious up to now, since when I took over as maintainer, I reckoned that the only changes would be minor bug fixes and updated dependencies. However, if we're going to make changes involving several hundred lines of code, I think we would need to add full tests, both for the existing functionality (to make sure it isn't disturbed by the change) and for the new. Can I ask whether you have looked at mysql-haskell? It supports prepared statements, and doesn't have the thread-safety problems of the C library. It's probably a better way to go in the long run, though I haven't yet had an opportunity to check out |
|
I Am definitely on board with the needing more tests. I was a bad and didn't save all the different things I tested as I was writing the code. I have not had a look at |
|
On the threading issues, these articles are both a little old, and I haven't checked whether more recent versions of the standard C client library change anything: https://ro-che.info/articles/2015-04-17-safe-concurrent-mysql-haskell and https://www.yesodweb.com/blog/2016/11/use-mysql-safely-in-yesod I get the point about re-implementations vs. the "standard" library. There do seem to be a number of re-implementations, though (eg the node mysql module just to choose one at random). |
2 participants
This is definitely a first pass. I am not sure if this is the correct module structure or if the
Valuetype is too "high level"Some of the mysql_stmt_* functions don't have wrappers implemented yet but figured it was better to get something out there rather than just leaving it hiding on my computer.