Derived ToJSON1 instance does not respect omitNothingFields = True

[ejconlon]

For an instance

data C a = C { required :: a, optional :: Maybe a }
    deriving (Eq, Show, Generic1)
    deriving (ToJSON1) via (AesonWrapper1 C)

with a simple deriving via newtype wrapper AesonWrapper1 I am seeing the value C 1 Nothing serialize with null

        Example test case: FAIL
          Repro.hs:77:
          expected: "{\"required\":1}"
           but got: "{\"required\":1,\"optional\":null}"

See https://gist.github.com/ejconlon/30dc50b183a0f72501c3e49ce57cae52 for a self-contained repro.

[Lysxia]

Indeed, it seems that this option currently only works for Generic, not Generic1. One way to fix it would be to add incoherent instances for Rec1, like what is there for K1. But Generic1 is also quite flawed, I wonder whether such a fix is worth maintaining, as opposed to discouraging use of Generic1 altogether (to be clear, I'm not the maintainer, I'm just proposing the idea, with my strongly negative opinion of Generic1).

In particular, ToJSON1 F should be subsumed by forall a. ToJSON a => ToJSON (F a).

Would it work for you to use ToJSON in this way, instead of ToJSON1?

[ejconlon]

I'm really not sure how to do it in a way that's re-usable.

instance (Generic (f a), ToJSON a) => ToJSON (AesonWrapper1 f a) where
    toJSON = genericToJSON options . unAesonWrapper1
    toEncoding = genericToEncoding options . unAesonWrapper1

data C a = C { required :: a, optional :: Maybe a }
    deriving (Eq, Show, Generic)

deriving via (AesonWrapper1 C a) instance (Generic (C a), ToJSON a) => ToJSON (C a)

fails to compile with

Could not deduce (aeson-1.4.2.0:Data.Aeson.Types.ToJSON.GToJSON
                          Value Zero (Rep (f a)))

similarly for Encoding. I really have no idea how to direct derivation of GToJSON and the like.

[Lysxia]

Can you try

-- When going through Generic, maybe you don't need the ...1 variant.
newtype AesonWrapper a = AesonWrapper { unAesonWrapper :: a }

-- You can only solve `GToJSON` and `GToEncoding` constraints for concrete `a`, so as long as it's a type variable, the constraints have to be explicit like this.
instance (Generic a, GToJSON (Rep a), GToEncoding (Rep a)) => ToJSON (AesonWrapper a) where
  ...

-- same as before
data C a = ...

deriving via (AesonWrapper (C a)) instance ToJSON a => ToJSON (C a)
  -- does the "via" go here or at the end?

[ejconlon]

Ah thanks! Yes, you are right I can just use the basic wrapper. (My brain is a little fuzzy at the moment...)

This makes the test pass

data C a = C { required :: a, optional :: Maybe a }
    deriving (Eq, Show, Generic)

deriving via (AesonWrapper (C a)) instance ToJSON a => ToJSON (C a)

Now, I originally switched to ToJSON1 because for fixpoint types like newtype TFix = TFix { unTFix :: T TFix }) Aeson would hang during encoding. I also had some crazy code that could be simplified, though. Let me expand the example and see if that's still the case.

[ejconlon]

Ok, it even seems to work recursively! https://gist.github.com/ejconlon/a017e2dc7f0482c26b0d26b0efd9e22d

My final question: Should we put a warning in the ToJSON1 documentation? I'm not even sure what other options are not respected - null fields are tolerable, but differences in sum encoding are not!

[phadej]

Problems with Maybe fields are because we do tricks with overlapping instances, everything else (IIRC) is on more solid ground, so should work.

[ejconlon]

What do you think about this warning in the documentation for genericLiftToJSON and genericLiftToEncoding:

Please note that this does not obey the omitNothingFields option of the provided Options. If this is an issue, please derive ToJSON instead of ToJSON1.

[bergmark]

sounds good!

…

On Thu, 7 Mar 2019 at 01:37, Eric Conlon ***@***.***> wrote: What do you think about this warning in the documentation for genericLiftToJSON and genericLiftToEncoding: Please note that this does not obey the omitNothingFields option of the provided Options. If this is an issue, please derive ToJSON instead of ToJSON1. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#687 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGJSftOQdp3uUur8GhfTDv95H6b_iB2ks5vUF8vgaJpZM4bhUck> .

[phadej]

With #1039 we should be able to fix ToJSON1 deriving.

I'll add tests for this issue to verify they do.