Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Breaking changes on 0.10 regarding Null field parsing on Maybe types #287

Closed
dmjio opened this issue Sep 19, 2015 · 14 comments
Closed

Breaking changes on 0.10 regarding Null field parsing on Maybe types #287

dmjio opened this issue Sep 19, 2015 · 14 comments

Comments

@dmjio
Copy link
Contributor

dmjio commented Sep 19, 2015

-- 0.9
λ: newtype A = A { val :: Maybe String } deriving Show
(0.01 secs, 3,077,712 bytes)
λ: instance FromJSON A where parseJSON (Object o) = A <$> o .:? "val"
(0.00 secs, 1,032,488 bytes)
λ: eitherDecode "{ \"val\" : null }" :: Either String A
Right (A {val = Nothing})
(0.00 secs, 1,552,616 bytes)
-- 0.10
λ> newtype A = A { val :: Maybe String } deriving Show                                                                                                                            
(0.01 secs, 3,086,560 bytes)                                                                                                                                                      
λ>  instance FromJSON A where parseJSON (Object o) = A <$> o .:? "val"                                                                                                                                                                                                                                                                       
(0.01 secs, 2,069,808 bytes)                                                                                                                                                      
λ> eitherDecode "{ \"val\" : null }" :: Either String A                                                                                                                           
Left "Error in $.val: failed to parse field val: expected String, encountered Null"                                                                                               
(0.01 secs, 2,622,128 bytes)        

Is this intended behavior? Normally Maybe types would return Nothing in the presence of a Null field (Under 0.9).

This change causes the stripe-haskell project to fail 95% of tests. 😭

@dmjio
Copy link
Contributor Author

dmjio commented Sep 19, 2015

Seems like this is the culprit, d0414be

Can we keep the existing operator (.:?) with 0.9.0 behavior, and as @gregwebs recommends, add a (.:??) operator for this new behavior ? ( as proposed by @lykahb in #83 )

@lykahb
Copy link

lykahb commented Sep 19, 2015

You use a wrong operator. Field val has type Maybe String. In the return value of the (.:?) Maybe is related only to the presence of the key. The operator has never not handled nulls. The key exists so it tries to convert null into String.
If the key val is mandatory, you can use (.:). If it is optional, fmap join (o .:? "val") will use the right FromJSON instance.

@dmjio
Copy link
Contributor Author

dmjio commented Sep 19, 2015

@lykahb I see, so previously fmap join (o .:? "val") was the default behavior (Null would be coerced into Nothing). Null and key existence were conflated. Now it's separate. Cool.

Maybe is related only to the presence of the key

In 0.10 yes, in 0.9 it could be related to the value too, right? Example below.

newtype A = A { val :: Maybe String } deriving Show
λ: instance FromJSON A where parseJSON (Object o) = A <$> o .: "val"
λ: eitherDecode "{ \"val\" : null }" :: Either String A
Right (A {val = Nothing})

@dmjio
Copy link
Contributor Author

dmjio commented Sep 19, 2015

Here's a list of the old and new differences. http://lpaste.net/141286

@MaxGabriel
Copy link

This is a major breaking change; I'd echo @gregwebs request that this be in the changelog or maybe use a different operator.

@tolysz
Copy link
Contributor

tolysz commented Oct 11, 2015

Hi,
Just to add my 5 cents; (.??) is a part of the original proposal to add one extra constructor to the Value type (eg. https://github.com/tolysz/aeson/blob/MissingDefault2/examples/Possible.hs)
And really there is no other way around.
I've been keeping quiet but types Maybe and Maybe Maybe have different universal properties and sure you can hack around adding extra information on creation of From/ToJSON to cater for a difference but it will never be generic i.e. encodable in the type system by storing all information needed to present this data.

My Original proposal was:

toJson (Nothing :: Maybe a) =  Null
toJson (Just a :: Maybe a) =  toJson a

toJson (Nothing :: Maybe (Maybe a)) =  Missing -- extra constructor
toJson (Just Nothing :: Maybe (Maybe a)) =  Null 
toJson (Just (Just a) :: Maybe (Maybe a)) =  toJson a

-- or Equivalent
toJson (MissingData :: Possible a) =  Missing -- extra constructor
toJson (HaveHull :: Possible a) =  Null 
toJson ((HaveData a) :: Possible a) =  toJson a

This gives you some very nice properties: eg. the toJson a allows type a to decide on how it should present itself and thus easily build APIs eg. https://github.com/tolysz/google-api/blob/master/src/Network/Google/Api/Kinds.hs
where I do not need to specialise ListResponse to include HACK to show/hide fields it at times.

@sapek
Copy link

sapek commented Nov 11, 2015

@lykahb, this also seems to work in 0.10:

instance FromJSON A where parseJSON (Object o) = A <$> o .:? "val" .!= Nothing

Is this a valid option to get old behavior or do I need to use fmap join (o .:? "val")?

@lykahb
Copy link

lykahb commented Nov 12, 2015

@sapek Yes, they should have the same behavior.

@nikomi
Copy link

nikomi commented Dec 14, 2015

I would +1 pull #288 - is anything happening with this thread?

@tolysz
Copy link
Contributor

tolysz commented Dec 15, 2015

This is sorted by: #326
I.e. the original question I mean. Simply one pattern got lost in the upgrade; Api does not change.

@nikomi
Copy link

nikomi commented Dec 15, 2015

thanx for info!

Pull request #326 does not seem to be merged into master yet - any idea when this might be released?

@tolysz
Copy link
Contributor

tolysz commented Dec 15, 2015

I use stack; but I agree it prevents people from using 0.10 in the wild; and the sooner the better.
With this patch it simply builds all packages :)

@hvr
Copy link
Member

hvr commented Dec 20, 2015

@bos bump

@bos
Copy link
Collaborator

bos commented Jan 20, 2016

I believe this should now be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants