paul-rouse / mysql-simple

A mid-level client library for the MySQL database, intended to be fast and easy to use.
Other
91 stars 35 forks source link

JSON support #55

Closed Daniel-Diaz closed 2 years ago

Daniel-Diaz commented 2 years ago

I added JSON support for both parameters and results. I followed the documentation here. This adds aeson as a dependency.

I haven't throughly tested this yet (will do so soon), but I thought I would post the PR early to see if there's interest in merging something like this in mysql-simple or if I should move this functionality to my code instead.

Thanks.

Daniel-Diaz commented 2 years ago

I tested this against a running mysql server and it worked as expected.

paul-rouse commented 2 years ago

Thank you for suggesting this change! However, I am a little worried about including it in this library because aeson brings in about 30 additional dependencies, almost doubling the total number! I would be wary of imposing that on applications which might not themselves require aeson. Do you think the benefit of including the JSON decoding at this level is worth that cost?

Daniel-Diaz commented 2 years ago

Thank you for suggesting this change! However, I am a little worried about including it in this library because aeson brings in about 30 additional dependencies, almost doubling the total number! I would be wary of imposing that on applications which might not themselves require aeson. Do you think the benefit of including the JSON decoding at this level is worth that cost?

Yeah, this is actually a good point. What I could do is to put the feature behind a flag, so only if the flag is enabled would all those dependencies be needed. I would have to update the documentation if we go this way so that the feature can still be discovered from the haddocks even if the flag is off. Is this a good plan?

Daniel-Diaz commented 2 years ago

I think it's worth noting that most web applications probably use JSON somewhere, which means they are probably depending on aeson already (it's at least the most commonly used JSON implementation in Haskell). But this is speculation.

Daniel-Diaz commented 2 years ago

There's also the fact that MySQL has a JSON column type that we are not really supporting at the moment. It can only be read as Text, which seems wrong to me.

paul-rouse commented 2 years ago

Those are important points, of course, although not all database applications are web applications.

I have been experimenting with providing the hooks you need in a more general way, avoiding building in the actual encoding and decoding in this library. Give me a few days - it's not big, but I am a little pressed for time at the moment!

Daniel-Diaz commented 2 years ago

Those are important points, of course, although not all database applications are web applications.

I have been experimenting with providing the hooks you need in a more general way, avoiding building in the actual encoding and decoding in this library. Give me a few days - it's not big, but I am a little pressed for time at the moment!

Thanks! That sounds neat.

paul-rouse commented 2 years ago

@Daniel-Diaz - would you mind testing the current state of master to see if it is OK for you? The idea is that you just need to define the ByteString encoding and decoding in new classes ToField and FromField, and use a default implementation for Param and Result. There is haddock markup for the documentation in Result.hs, Param.hs and the top-level Simple.hs.

At the end of Simple.hs, the documentation shows an example of JSON encoding a single type. If you wanted a newtype wrapper to handle a bunch of types, as in this PR, you can do that, too, of course, and if you feel strongly enough, you could even release it as a tiny, separate library! It would just put the code you wrote for this PR into a slightly different context:

newtype AsJson a = AsJson { fromAsJson :: a }
    deriving Typeable

instance FromJSON a => FromField (AsJson a) where
    fromField = ([Json], (AsJson <$>) . JSON.eitherDecodeStrict')
instance (FromJSON a, Typeable a) => Result (AsJson a)

instance ToJSON a => ToField (AsJson a) where
    toField = BL.toStrict . JSON.encode . fromAsJson
instance ToJSON a => Param (AsJson a)

If you are happy to go with this, I'll release it and close this PR.

Daniel-Diaz commented 2 years ago

@paul-rouse Thank you!

Yeah, it works for me. It feels to me like ToField/FromField are very similar to Param/Result, just a bit simpler to implement. Did I get that wrong? In any case, it does mean I would still have to add extra instances for all the types that I want to use as JSON, and that all those instances will look exactly the same. I might follow your suggestion and release a tiny mysql-simple-json package.

Thanks again!

paul-rouse commented 2 years ago

Thanks for the reply! The main thing was providing a way to use the low-level machinery in Result.hs (mkCompats, conversionFailed and things they depend on) without exposing them in the exports. ToField is pretty much for symmetry with FromField, but still I thought it best to avoid exporting Action at the top level (a small point, though).

It's released now on Hackage, so I'll close this. Thanks for the proposal and the discussion!