mtesseract / nakadi-client

Haskell Client Library for the Nakadi Event Broker
Other
13 stars 9 forks source link

Add BusinessEvent type support #58

Closed vitold closed 6 years ago

vitold commented 6 years ago

Hi @mtesseract, I'm trying to add BusinessEvent and got a question. For DataChangeEvent type we have an abstract parameter "a" for payload. Trying to do the same for BusinessEvent I found some difficulties. From the definition of business events https://zalando.github.io/nakadi/manual.html#definition_BusinessEvent we need to mixin "metadata" field into our payload. Trying to write aeson serializer I got something like this:

data BusinessEvent a = BusinessEvent
  { _payload  :: a
  , _metadata :: Metadata
  } deriving (Eq, Show, Generic)

instance FromJSON a => FromJSON (BusinessEvent a) where
  parseJSON = withObject "BusinessEvent" $ \obj -> BusinessEvent
    <$> parseJSON (Object obj)
    <*> obj .: "metadata"

instance ToJSON a => ToJSON (BusinessEvent a) where
  toJSON (BusinessEvent payload metadata) = case toJSON payload of
    Object obj -> Object $ obj `union` fromList ["metadata" .= toJSON metadata]
    other -> other

The problem is in ToJSON instance. Type parameter "a" doesn't mean that it will be serialized into aeson Object. It could also be the other Value which makes impossible to mixin metadata into it. How do you think, what the better way to define BusinessEvent serializer?

mtesseract commented 6 years ago

Hi!

I guess there was a reason why I did not implement support for BusinessEvent yet. :-) This mixin-weirdness makes it tricky. I wonder why they did specified it like this...

Anyway. I am not completely sure what the best approach here is.

I guess, one could use type classes for this. For example:

class ToJSONObject a where
  toJSONObject :: a -> Object

And then use it like this:

instance ToJSONObject a => ToJSON (BusinessEvent a) where
  toJSON (BusinessEvent payload metadata) = Object $
    toJSONObject payload `HashMap.union` HashMap.fromList ["metadata" .= toJSON metadata]

What do you think?

mtesseract commented 6 years ago

PS: This, of course requires appropriate type class instances to be in scope for the payload datatypes, that should be used as payload for BusinessEvent. For example:

data Foo = Foo { _foo :: Int
               , _bar :: Text
               }

instance ToJSONObject Foo where
  toJSONObject Foo { .. } = HashMap.fromList $
    [ ("foo", Number (fromIntegral _foo))
    , ("bar", String _bar)
    ]

Somebody, who is better with generic programming in Haskell, then I am could probably make these kind of lines derivable. But as a start, one could probably live with writing them by hand.

mtesseract commented 6 years ago

@vitold Hi! Did you decide for one way or the other? If so, is it PR-ready?

vitold commented 6 years ago

@mtesseract hi, on my personal opinion, type class is more preferable solution. I’m right now on vacation, and coming back next week, so then I’ll be able to create pull request.

mtesseract commented 6 years ago

Cool, enjoy your vacation!

mtesseract commented 6 years ago

@vitold Do you have anything in the pipeline regarding this? If not, I might just implement this.