[haskell-http-client] compare rendered form in PropMime

[guoshimin]

PR checklist

• Read the contribution guidelines.
• Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh, ./bin/security/{LANG}-petstore.sh and ./bin/openapi3/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
• Filed the PR against the correct branch: master, 3.4.x, 4.0.x. Default: master.
• Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

It is known that fromJson and toJson for Maybe are not the inverse of each other: haskell/aeson#376. It causes failures in the PropMime tests.

> encode (Just Null)
"null"
> encode (Nothing :: Maybe Value)
"null"
> > decode "null" :: Maybe Value
Just Null

Changing the PropMime tests to compared the rendered form instead.

@jonschoning, what do you think?

[auto-labeler]

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

[jonschoning]

It does change what's being tested — a renderer that drops fields wouldn't be caught.

But since it avoids the inverse issue, what I'd accept is simply leaving the existing propMime as is, adding a propMimeRendered & proprMimeRenderedEq in the same file to compare the rendered form, and changing test.mustache to use proprMimeRenderedEq instead of propMimeEq

also, to avoid escaping the output in actual rendered output for failMsg, a failMsg that now avoids show in propMimeRendered like this looks a little better:

         failMsg =
           "ACTUAL " <> either ("(L): " ++) ("(R): " ++) (fmap BL8.unpack actual) <> "\nRENDERED:   " <> BL8.unpack rendered

[guoshimin]

Good point about dropping fields. Now I don't feel this is a good change. I'm thinking maybe we can define another type class Eq1 which has the same instances as Eq but equalizes Just Null and Nothing; or alternatively modify the Arbitrary instance for Maybe Value to never generate Nothing. What do you think?

[jonschoning]

I'm in favor of another solution too.

define another type class Eq1 which has the same instances as Eq but equalizes Just Null and Nothing

I was going down this road with ApproxEq which I used in a different library in order to compare lists while ignoring the order of the elements, but I wasn't able to fully generalize this. I'm not sure how to "get the same instances as Eq" exactly, and couldn't get my attempts using OVERLAPPING to work; I don't understand how to use overlapping effectively at all.

modify the Arbitrary instance for Maybe Value to never generate Nothing

since instance Arbitrary a => Arbitrary (Maybe a) is already defined defined in Test.QuickCheck.Arbitrary, I think you get into overlapping territory again here - i don't know if it's possible, i just don't know how to use it.

One option I know how to do is simply modifying the instance we do have defined (instanace Arbitrary A.Value) directly to not generate A.Null at all, at a slight loss of coverage.

But if you have any ideas how to achieve the options you mentioned, i'd be for them.

[jonschoning]

playing around with Aeson a bit, .:! might be useful

-- | Animal
data Animal2 = Animal2
  { animal2ClassName :: !(Text) -- ^ /Required/ "className"
  , animal2Color :: !(Maybe A.Value) -- ^ "color"
  } deriving (P.Show, P.Eq, P.Typeable)

-- | FromJSON Animal
instance A.FromJSON Animal2 where
  parseJSON = A.withObject "Animal" $ \o ->
    Animal2
      <$> (o .:  "className")
      <*> (o .:! "color")

>>> decode "{\"className\": \"foo\"}" :: Maybe Animal2
Just (Animal2 {animal2ClassName = "foo", animal2Color = Nothing})

>>> decode "{\"className\": \"foo\", \"color\":null}" :: Maybe Animal2
Just (Animal2 {animal2ClassName = "foo", animal2Color = Just Null})

but .:! will cause parse errors more often for values that aren't Maybe A.Value, because it forces the parser to handle the Null, so maybe I can just generate a .:! only for the case of Maybe A.Value, and also use the generation switch -DallowToJsonNulls=true which will avoid stripping out nulls, on condition that kubernetes can handle the nulls (in strict OAS it isn't allowed).

I'll play around with it and see if it roundtrips successfully.
...
(unfortunately, -DallowToJsonNulls=true would emit "key":null for all Maybes that are Nothing as well)

if it's possible to just exclude the scenario in the test module that would be ideal imho

[guoshimin]

Generating "key": null for Nothing would be a big minus. I came up with the following for not generating Just x where x is the value that represents null for some type a. One downside it that it relies on decode which is what we are trying to test. We can avoid it if we only do it for Value as opposed to every type that has a value for null.

arbitraryReducedMaybe :: forall a . (A.FromJSON a, Arbitrary a, Eq a) => Int -> Gen (Maybe a)
arbitraryReducedMaybe 0 = elements [Nothing]
arbitraryReducedMaybe n = do
  let decodedNull = A.decode "null" :: Maybe a  -- does `a` have a value that represents null?
  generated <- arbitraryReduced n
  if generated == decodedNull
    then return Nothing
    else return generated

[jonschoning]

Nice. I'll look into "only doing it for Value" tomorrow

[jonschoning]

@guoshimin I updated my PR #2343 in include c00321c which adds arbitraryReducedMaybeValue for only Maybe Value.

It seems to work as you described, and tests passed with the kubernetes spec (https://gist.github.com/jonschoning/45b7223ed72fd8cf6b6a2b0e5de246e1). if it looks good to you i'll merge PR #2343 in with the change

[guoshimin]

Works perfectly. Thanks!