brendanhay / gogol

A comprehensive Google Services SDK for Haskell.
Other
282 stars 105 forks source link

Datastore: Not exporting internal To/FromJSON instances #48

Closed runeksvendsen closed 8 years ago

runeksvendsen commented 8 years ago

Many Network.Google.Datastore.Value types -- and types contained within these -- have sensible Data.Aeson.ToJSON/FromJSON instances, because their purpose is to map to JSON types.

This goes for types like ValueNullValue (toJSON = const Data.Aeson.Null), ArrayValue (toJSON = ^. avValues) and Entity:

instance ToJSON Entity where
    toJSON e = Object $
        fmap toJSON . view epAddtional .
        fromMaybe (entityProperties Map.empty) $
        (e ^. eProperties)

However, these types also need to be serialized, and sent over the API, hence this library's ToJSON and FromJSON instances, which are used for this internal purpose.

I would like to ask if it's possible to contain these instances internally in the library, such that I can create an elegant wrapper around gogol-datastore which has the right Aeson instances for the various types, thus allowing users to convert seamlessly between eg. a value & vStringValue and a Data.Aeson.String.

I have created JSON conversion functions for all the useful Network.Google.Datastore.Value types, as well as eg. ArrayValue and ValueNullValue. However, these implementations really belong as ToJSON/FromJSON instances of Network.Google.Datastore.Value, ArrayValue and ValueNullValue, such that toJSON and fromJSON knows which lenses to use for conversion, for example vIntegerValue or vDoubleValue to convert to/from a JSON Number.

brendanhay commented 8 years ago

Generally it's not possible to make typeclass instances 'private' due to the instance coherence problem.

Does a ToValue or similarly named class which serialises via ToValue a => a -> Value make more sense? This would allow you to serialise any trivial or complex type to a Value directly in place of using value & .... It avoids having to fool around with ToJSON instances, orphans, or newtype wrappers, and a default instance could be provided for types that already have ToJSON instances - meaning you only need to declare an empty ToValue instance for those types to also be supported.

Since this is a similar problem to what amazonka-dynamodb has (with the equivalent to Value being called AttributeValue), there is an experimental feature addition which allows the serialisation of any sensible value to an AttributeValue: https://github.com/brendanhay/amazonka/blob/feature/dynamodb-mapper/amazonka-dynamodb-extras/src/Amazonka/DynamoDB/Item/Value.hs#L254-L255 - along with some additional type-level constraints to ensure certain invariants are held.

brendanhay commented 8 years ago

An example of above:

class ToValue a where
    toValue :: a -> Value

    default toValue :: ToJSON a => a -> Value
    toValue = undefined

instance ToValue Int where ...
instance ToValue String where ...
instance ToValue (HashMap Text a) where ...

data MyFoo = MyFoo deriving (Show)

instance ToJSON MyFoo where
    toJSON = toJSON . show

instance ToValue MyFoo
brendanhay commented 8 years ago

In fact - reviewing the type of Value - it appears that a very similar implementation of the DynamoDB approach would be preferrable. It would ensure the invariant that only a single attribute of the greater Value type is ever set.

runeksvendsen commented 8 years ago

I think you are right, I should just define my own class for the JSON stuff, even though it's the same as ToJSON/FromJSON. In the end, that's what I went ahead and did.

So far I have a class for wrapping native Datastore values inside a Datastore.Value, and two for converting back and forth between Aeson.Value and Datastore.Value.

I'm also trying to add the ability to encode arbitrary values inside a Datastore.Value, because eg. an Aeson.Number (Scientific) does not fit in Datastore's native types. So I would like to be able to encode this in a Datastore.Key, as a string, such that I'm not restricted to the native types, and Datastore.Key can be isomorphic to Aeson.Value. I think I need to add a new class for this, but I haven't looked at it yet.

Looks like I can use some tricks from your implementation of the same in amazonka-dynamodb, and probably learn some new stuff as well. I wasn't familiar with the coerce function, for example.

EDIT: I've got it working now. I encode an Aeson.Number as either Int64, Double or as a string inside a Datastore.Key as a last resort: https://github.com/runeksvendsen/paychan-datastore/blob/master/src/DB/Model/NativeConvert.hs#L12 The conversions tests now all pass. Before there were occasional errors due to floating point arithmetic.