Encode/DecodeJson Maybe instances can't handle nested Maybes

[hdgarrood]

For example:

roundtrip :: forall a. (EncodeJson a, DecodeJson a) => a -> Either String a
roundtrip x = decodeJson <<< encodeJson

Then roundtrip (Just Nothing :: Maybe (Maybe Unit)) is Right Nothing.

[garyb]

This sucks. It does seems worth fixing, but doing so will break everyone's existing codecs.

[hdgarrood]

Yeah. I guess people who do want nested maybes with the current could wrap them with the GJson newtype, but the normal instance should probably "just work".

I guess this code could be updated so that the DecodeJson a => DecodeJson (Maybe a) instance handles both the old and new encoding, at least for a short while?

[Deleted user]

This was added in aeson as a new operator .!=.

One use case is in a web service request if you want to allow the client to only send the fields they wish to change instead of the whole entity. You need a way to differentiate between the fields not included because the client didn't want to change them, and the fields sent as null because the client wants to clear them.

If my record is

data User = User { name :: Text, age :: Maybe Int }

I could have a patch request body like

data PatchUser = PatchUser { patchName :: Maybe Text, patchAge :: Maybe (Maybe Int) }

This allows me to accept a json like { age: null } that won't modify the user's name, and will set their age to Nothing. Except imagine entity has 15 fields and reducing the payload size is important.

[hdgarrood]

Unfortunately, when a type has instances for both EncodeJson and DecodeJson, there's a (reasonable!) expectation that my roundtrip above is equivalent to Right, so I don't think there's a backwards-compatible way we can fix this, we really just need to fix the instances.

It's possible to deal with fields of type Maybe (Maybe Int) using what's already exported from this library - one way is to use gEncodeJson and gDecodeJson for those fields instead of the default EncodeJson and DecodeJson instances.

Incidentally, aeson's .!= is defined as p .!= default = map (fromMaybe default) p so works for any Functor - you could define the same thing in your project (I don't have an opinion on whether this library should export .!=). But I don't see how .!= addresses the above issue; a parser defined as p .!= default is still going to break if p is decoding Maybe (Maybe Whatever) values incorrectly.

[Deleted user]

Sorry, my mistake, I meant to write .:! and not .!=. These operators are confusing sometimes.

.:! is similar to .:? but it parses the value differently. They are both FromJSON a => Object -> Text -> Parser (Maybe a) but instead of interpreting null as Nothing, it interprets it like any other json value and tries to parse null into an a. This combinator should give you Right (Just Nothing) in your roundtrip.

[Deleted user]

All I'm trying to say, is point out that they didn't modify the behaviour of .:? and break existing code, they just added a new combinator, .:!.

[hdgarrood]

This isn't like modifying the behaviour of a combinator, though; it's specifically because the brokenness is in an instance that makes this problem different from aeson's, and makes it hard to fix in a non-breaking way.

This is the best suggestion I can come up with:

instance (EncodeJson a) => EncodeJson (Maybe a) where
  encodeJson = encodeJson <<< maybeToArray
  where
  maybeToArray :: forall b. Maybe b -> Array b
  maybeToArray = Array.fromFoldable

instance (DecodeJson a) => DecodeJson (Maybe a) where
  decodeJson json = oldDecode json <|> newDecode json
  where
  oldDecode j
    | isNull j = pure Nothing
    | otherwise = Just <$> decodeJson j
  newDecode =
    decodeJson >=> fromArray
  fromArray arr =
    maybe (Left "Unable to decode Maybe") Right (arr !! 0)

[nwolverson]

Think a point on the release note about this breaking change would be good, this confused me when it caused things to fail at runtime

[garyb]

The fix we added for this shouldn't have been a breaking change 😕

Can you show an example case of where it is failing now?

[nwolverson]

Ah sorry, it was a case of EncodeJson being used to produce JSON for external consumption

[garyb]

Ah right, good point! I'll add the note.