unit test and fix for null values in rendered JSON in UserProfile #1292
Conversation
While omitting a json field, or setting the field to null parses the same for Haskell, that is not the case for other programming languages, which can easily trip up over a change in behaviour. The problem was accidentally introduced in #1281, resulting in null values instead of omitting fields for UserProfile json, e.g. { "deleted" : null, "picture" : [], "email" : null, "accent_id" : 1, "team" : null, "handle" : "test1", "expires_at" : null, "locale" : "en-US", "qualified_id" : { "id" : "d93f2555-e4b7-4051-8d84-2bcf59ea6066", "domain" : "staging.zinfra.io" }, "service" : null, "name" : "test", "id" : "d93f2555-e4b7-4051-8d84-2bcf59ea6066", "assets" : [ { "type" : "image", "size" : "preview", "key" : "3-1-43978d07-040c-4f49-be7e-06e2560701fd" }, { "type" : "image", "key" : "3-1-f3039a69-a0b8-45a5-bc6b-3da087b680c7", "size" : "complete" } ] } This PR reverts the offending change, and adds one manual unit test for this particular data type. We may wish to add more tests for other data types as well in future PRs to catch some backwards compatibility changes.
| # "assets" .= profileAssets u | ||
| # "accent_id" .= profileAccentId u | ||
| # "deleted" .= (if profileDeleted u then Just True else Nothing) | ||
| # "service" .= profileService u |
| let userProfile = UserProfile (Qualified uid domain) (Name "name") (Pict []) [] colour False Nothing Nothing Nothing Nothing Nothing Nothing | ||
| let profileJSONAsText = Text.pack $ show $ Aeson.encode userProfile | ||
| let msg = Text.unpack $ "toJSON encoding must not convert Nothing to null, but instead omit those json fields for backwards compatibility. UserProfileJSON:" <> profileJSONAsText | ||
| assertBool msg (not $ "null" `Text.isInfixOf` profileJSONAsText) |
There was a problem hiding this comment.
I would have just pasted the offending json into a QQ-block, fixed it, and make a round-trip test from json to haskell and back out of it. But this works, too.
There was a problem hiding this comment.
We already have JSON roundtrip tests. They don't catch this problem, so they are somewhat useless (for this purpose of checking there are no null values), as they don't make a difference between fields being omitted and fields being set to null, since Aeson accepts either and interprets them the same.
I'm not sure how you can write a roundtrip test to fail when there are null values. Do you have an example?
There was a problem hiding this comment.
Yes, I read your PR :) i meant:
sample = [json|{"field": 3}|] -- omitting the null fields
test = encode (decode sample) `shouldBe` sample
There was a problem hiding this comment.
But I'm not saying you should change the test you have, which works fine. Just thinking out loud.
There was a problem hiding this comment.
the more i think about it, the better i like my approach. it's very easy to read, and it potentially catches other mistakes we haven't even thought about here. on the other hand, it gives you more room for errors when crafting the samples. donno. but i want to have 2 or 3 samples for every type now, everywhere. :)
jschaul
force-pushed
the
fix-null-json
branch
from
a404f54 to
9e4ec4b
Compare
In most cases we want optional fields to not appear at all in the serialised object (see for example #1292). Using the `opt` combinator achieves this behaviour.
In most cases we want optional fields to not appear at all in the serialised object (see for example #1292). Using the `opt` combinator achieves this behaviour.
In most cases we want optional fields to not appear at all in the serialised object (see for example #1292). Using the `opt` combinator achieves this behaviour.
While omitting a json field, or setting the field to null parses the
same for Haskell, that is not the case for other programming languages,
which can easily trip up over a change in behaviour.
The problem was accidentally introduced in #1281, resulting in null
values instead of omitting fields for unset values in UserProfile json, e.g.
{ "deleted" : null, "picture" : [], "email" : null, "accent_id" : 1, "team" : null, "handle" : "test1", "expires_at" : null, "locale" : "en-US", "qualified_id" : { "id" : "d93f2555-e4b7-4051-8d84-2bcf59ea6066", "domain" : "staging.zinfra.io" }, "service" : null, "name" : "test", "id" : "d93f2555-e4b7-4051-8d84-2bcf59ea6066", "assets" : [ { "type" : "image", "size" : "preview", "key" : "3-1-43978d07-040c-4f49-be7e-06e2560701fd" }, { "type" : "image", "key" : "3-1-f3039a69-a0b8-45a5-bc6b-3da087b680c7", "size" : "complete" } ] }This caused a problem as described in https://wearezeta.atlassian.net/browse/SQCORE-351
This PR reverts the offending change, and adds one manual unit test for
this particular data type. We may wish to add more tests for other data
types as well in future PRs to catch some backwards compatibility changes.