getFieldOptional possibly broken in nested structures?

[vagifverdi]

Here's the minimal example to reproduce:

newtype Foo = Foo
  {fooId :: Int
  ,bazList :: List Baz
  }

instance decodeJsonFoo :: DecodeJson Foo where
  decodeJson json = do
    obj <- decodeJson json
    id <- obj .? "fooId"
    bs <- obj .? "bazList"
    pure $ Foo { fooId: id
               , bazList : bs}


newtype Baz = Baz
  {bazId :: Int
  ,bazOpt :: Maybe Int
  }

instance decodeJsonBaz :: DecodeJson Baz where
  decodeJson json = do
    obj <- decodeJson json
    bId   <- obj .? "bazId"
    bad   <- obj .?? "bazOpt"
    good <- foldJsonNumber Nothing (Just <<< floor) <$> obj .? "bazOpt"
    pure $ Baz { bazId: bId
               , bazOpt: bad
               }

When each Baz in the list has Int value in bazOpt then "bad" works fine.
When some Bazes in the list has null in bazOpt field then "bad" give this error:
Couldn't decode List Value is not a Number.

"good" works fine even with nulls.

is getFieldOptional broken?

[dgendill]

I think what's going on here is getFieldOptional only cares about whether or not a key exists in an object. If the key isn't there, the return value is Right Nothing, otherwise, it's Just a, regardless of the value. In your case, the return value is Just null, since the key is there but the value is null. Since your data type requires a Maybe Int you're getting the default error.

If you want to treat the null value the same as the case where the key is missing, I would define a new function that treats the null case the same as the case where the key is missing.

foreign import isNull :: forall a. a -> Boolean

getFieldOptional' :: forall a. DecodeJson a => JObject -> String -> Either String (Maybe a)
getFieldOptional' o s = (case _ of
    Just v -> if isNull v then Nothing else v
    Nothing -> Nothing
  ) <$> (getFieldOptional o s)

Where isNull strictly checks if the value is null.

exports.isNull = function(v) {
  return v === null;
}

[vagifverdi]

The law of the least surprise dictates that nulls propagate. While I'm sure a function that recognizes the distinction between null and absent keys is useful and needed, I do not think it should replace the most anticipated outcome.

I do not know whether fixing getFieldOptional would break too much of existing code. And if that's the case perhaps it would be possible to create a new similar function with propagating nulls?

Or make getFieldOptional to have the most expected behavior, and move current functionality in a different function.

[cdepillabout]

I ran into this same error as well.

I was under the impression that argonaut's (.??) operator was the same as the Aeson library's (.:?) operator, but it appears that this is not the case.

I wrote a small test program to show the difference between Aeson's (.:!) and (.:?) operators:

#!/usr/bin/env stack
-- stack --resolver lts-8.14 script --package aeson

{-# LANGUAGE InstanceSigs #-}
{-# LANGUAGE OverloadedStrings #-}

import Data.Aeson
       (FromJSON(parseJSON), Value, (.:), (.:?), (.:!), eitherDecode,
        withObject)
import Data.Aeson.Types (Parser)

data Foo = Foo
  { fooInt :: Int
  , fooBar :: Maybe String
  , fooBaz :: Maybe String
  } deriving Show

instance FromJSON Foo where
  parseJSON :: Value -> Parser Foo
  parseJSON = withObject "Foo" $ \o ->
    Foo
      <$> o .: "int"
      <*> o .:? "bar"
      <*> o .:! "baz"

main :: IO ()
main = do
  print myEitherFooNoBarBaz
  print myEitherFooNullBarBaz

myEitherFooNoBarBaz :: Either String Foo
myEitherFooNoBarBaz = eitherDecode "{ \"int\": 1 }"

myEitherFooNullBarBaz :: Either String Foo
myEitherFooNullBarBaz =
  eitherDecode "{ \"int\": 1, \"bar\": null, \"baz\": null }"

Here is the output of running this:

Right (Foo {fooInt = 1, fooBar = Nothing, fooBaz = Nothing})
Left "Error in $.baz: expected String, encountered Null"

You can see that the (.:?) allows either null values or missing values. However, (.:!) doesn't allow null values. (That is to say, it passes the null to the underlying parser.)

I assumed that argonaut's (.??) was the same as aeson's (.:?).

I agree with @vagifverdi that argonaut's (.??) should be made to work the same as aeson's (.:?) operator. However, I imagine this could break some existing code, so maybe it would just be better to add a new operator. Adding documentation showing the differences between the two operators would be nice.

I do feel it is somewhat unfortunate that the suffix character ! and ? are being used differently in aeson and argonaut.

[garyb]

I don't really have a strong preference about how this behaves, but due to the current poor encode/decode instances for Maybe, it would perhaps have another knock-on effect, as it would be impossible to get a Just Nothing out of this when using a o .?? "maybeField" and there's a Nothing for the field, since currently Nothing is encoded as null.

I don't really think that's something people are likely depending on, but you never know. Just thought I'd point it out as another consideration if we do change the behaviour of the operator.

[mengu]

i'm definitely voting for the change. the current behavior is not intuitive.

[thomashoneyman]

@mengu @garyb @cdepillabout @vagifverdi @dgendill

I agree that this should be improved, and I would like to take action on this and merge an update as soon as I have some idea of consensus from those of you involved so far. If there's no set opinion, then I'll most likely go with adding a new operator as in #32.

I don't want to linger too long -- it's been long enough already -- so I will most likely make an update in 1 week unless there are objections.

---

I'm not sure whether it would be better for .?? to be revised to work the same as .:? in Aeson, or if the better alternative would be to introduce a new operator with the desired behavior, .?!.

The main questions for me comes down to whether there's ever a case in which you'd want the current behavior of .??, and if not, whether it's worth breaking existing code (with the idea being deprecation of a not-useful operator).

My current preference is probably to remove the .?? operator altogether and replace it with .?!. This avoids people shooting themselves in the foot with .?? (as I and my team have done in the past) but doesn't mean an update will start silently changing the behavior of code using the old operator.

As I know this has been an issue for @foresttoney @crcornwell and @davezuch in the past, I'd appreciate hearing your suggestions as well.

[davezuch]

I wouldn't assume there won't ever be a use case for the current behavior, I've encountered codebases and API's where null and undefined were distinct values. I think the best option is to introduce another combinator and support both behaviors like Aeson does.

There's the problem of what to name each combinator though. Following Aeson's naming convention, we would update .?? to also check for null, and introduce .?! that functions like .?? currently does. However, this would change the behavior for existing codebases without any way to let users know, so I don't think that's a good idea. However, not changing the name will continue to cause confusion for anyone expecting .?? to behave like Aeson's .:?.

Perhaps the solution is to break the API completely and switch to Aeson's naming for these combinators, renaming .?? to .:! and introducing .:? that also checks for null. This would clear any confusion for anyone expecting the same behavior as Aeson, and anyone already using Argonaut and then updating would be forced to check the documentation and find the correct combinator for them.

[thomashoneyman]

I'd be OK with switching to the Aeson naming scheme given how similar these interfaces already are and that the slight differences have caused confusion in the past. From what I can tell, these operators are more influenced by Aeson than by Scala's Argonaut.

[mengu]

thank you very much for the work @thomashoneyman.

[thomashoneyman]

Closed by #47