From a1a5a8d115577424b4e2e2aa20485ea837ca096b Mon Sep 17 00:00:00 2001 From: jschaul Date: Wed, 16 Dec 2020 20:22:17 +0100 Subject: [PATCH 1/2] unit test and fix for null values in rendered JSON in UserProfile 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. --- libs/wire-api/package.yaml | 2 ++ libs/wire-api/src/Wire/API/User.hs | 30 +++++++++---------- libs/wire-api/test/unit/Test/Wire/API/User.hs | 27 +++++++++++++++-- libs/wire-api/wire-api.cabal | 4 ++- 4 files changed, 44 insertions(+), 19 deletions(-) diff --git a/libs/wire-api/package.yaml b/libs/wire-api/package.yaml index 31b34c0b480..5595adc74c1 100644 --- a/libs/wire-api/package.yaml +++ b/libs/wire-api/package.yaml @@ -68,6 +68,8 @@ tests: - base - bytestring-conversion - wire-api + - uuid + - text - aeson-qq - lens - swagger2 diff --git a/libs/wire-api/src/Wire/API/User.hs b/libs/wire-api/src/Wire/API/User.hs index c9e19373b5c..7f8efb3cc9b 100644 --- a/libs/wire-api/src/Wire/API/User.hs +++ b/libs/wire-api/src/Wire/API/User.hs @@ -239,21 +239,21 @@ modelUser = Doc.defineModel "User" $ do instance ToJSON UserProfile where toJSON u = - object - [ "id" .= qUnqualified (profileQualifiedId u), - "qualified_id" .= profileQualifiedId u, - "name" .= profileName u, - "picture" .= profilePict u, - "assets" .= profileAssets u, - "accent_id" .= profileAccentId u, - "deleted" .= (if profileDeleted u then Just True else Nothing), - "service" .= profileService u, - "handle" .= profileHandle u, - "locale" .= profileLocale u, - "expires_at" .= profileExpire u, - "team" .= profileTeam u, - "email" .= profileEmail u - ] + object $ + "id" .= qUnqualified (profileQualifiedId u) + # "qualified_id" .= profileQualifiedId u + # "name" .= profileName u + # "picture" .= profilePict u + # "assets" .= profileAssets u + # "accent_id" .= profileAccentId u + # "deleted" .= (if profileDeleted u then Just True else Nothing) + # "service" .= profileService u + # "handle" .= profileHandle u + # "locale" .= profileLocale u + # "expires_at" .= profileExpire u + # "team" .= profileTeam u + # "email" .= profileEmail u + # [] instance FromJSON UserProfile where parseJSON = withObject "UserProfile" $ \o -> diff --git a/libs/wire-api/test/unit/Test/Wire/API/User.hs b/libs/wire-api/test/unit/Test/Wire/API/User.hs index c99668a7aa7..65f5d209690 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/User.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/User.hs @@ -21,18 +21,39 @@ module Test.Wire.API.User where import Data.Aeson +import qualified Data.Aeson as Aeson import Data.Aeson.Types as Aeson +import Data.Domain +import Data.Id +import Data.Qualified +import qualified Data.Text as Text +import qualified Data.UUID.V4 as UUID import Imports import Test.Tasty import Test.Tasty.HUnit -import Wire.API.User (parseIdentity) -import Wire.API.User.Identity +import Wire.API.User tests :: TestTree tests = testGroup "User (types vs. aeson)" $ unitTests unitTests :: [TestTree] -unitTests = +unitTests = parseIdentityTests ++ jsonNullTests + +jsonNullTests :: [TestTree] +jsonNullTests = [testGroup "JSON null" [testCase "userProfile" $ testUserProfile]] + +testUserProfile :: Assertion +testUserProfile = do + uid <- Id <$> UUID.nextRandom + let domain = Domain "example.com" + let colour = ColourId 0 + 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) + +parseIdentityTests :: [TestTree] +parseIdentityTests = [ let (=#=) :: Either String (Maybe UserIdentity) -> (Maybe UserSSOId, [Pair]) -> Assertion (=#=) uid (mssoid, object -> Object obj) = assertEqual "=#=" uid (parseEither (parseIdentity mssoid) obj) (=#=) _ bad = error $ "=#=: impossible: " <> show bad diff --git a/libs/wire-api/wire-api.cabal b/libs/wire-api/wire-api.cabal index 70313fa8b03..6958751feda 100644 --- a/libs/wire-api/wire-api.cabal +++ b/libs/wire-api/wire-api.cabal @@ -4,7 +4,7 @@ cabal-version: 1.12 -- -- see: https://github.com/sol/hpack -- --- hash: 4e9f1f53fb43a39da1366fe060fe0ef8e2b0185847abdc61552ca3262bf25322 +-- hash: 68fccd06d4ecadf4d0580f3aa528d1ad832002a42e73ae395948ff2f8eb38420 name: wire-api version: 0.1.0 @@ -151,7 +151,9 @@ test-suite wire-api-tests , tasty-expected-failure , tasty-hunit , tasty-quickcheck + , text , types-common >=0.16 , unordered-containers + , uuid , wire-api default-language: Haskell2010 From 9e4ec4be5b090a163215b24ab9055e63dd3e564d Mon Sep 17 00:00:00 2001 From: jschaul Date: Wed, 16 Dec 2020 22:01:31 +0100 Subject: [PATCH 2/2] PR suggestion: no need for Text --- libs/wire-api/package.yaml | 1 - libs/wire-api/test/unit/Test/Wire/API/User.hs | 7 +++---- libs/wire-api/wire-api.cabal | 3 +-- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/libs/wire-api/package.yaml b/libs/wire-api/package.yaml index 5595adc74c1..b6dfc8b978f 100644 --- a/libs/wire-api/package.yaml +++ b/libs/wire-api/package.yaml @@ -69,7 +69,6 @@ tests: - bytestring-conversion - wire-api - uuid - - text - aeson-qq - lens - swagger2 diff --git a/libs/wire-api/test/unit/Test/Wire/API/User.hs b/libs/wire-api/test/unit/Test/Wire/API/User.hs index 65f5d209690..8d33146ba61 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/User.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/User.hs @@ -26,7 +26,6 @@ import Data.Aeson.Types as Aeson import Data.Domain import Data.Id import Data.Qualified -import qualified Data.Text as Text import qualified Data.UUID.V4 as UUID import Imports import Test.Tasty @@ -48,9 +47,9 @@ testUserProfile = do let domain = Domain "example.com" let colour = ColourId 0 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) + let profileJSONAsText = show $ Aeson.encode userProfile + let msg = "toJSON encoding must not convert Nothing to null, but instead omit those json fields for backwards compatibility. UserProfileJSON:" <> profileJSONAsText + assertBool msg (not $ "null" `isInfixOf` profileJSONAsText) parseIdentityTests :: [TestTree] parseIdentityTests = diff --git a/libs/wire-api/wire-api.cabal b/libs/wire-api/wire-api.cabal index 6958751feda..0b0c3b30b4a 100644 --- a/libs/wire-api/wire-api.cabal +++ b/libs/wire-api/wire-api.cabal @@ -4,7 +4,7 @@ cabal-version: 1.12 -- -- see: https://github.com/sol/hpack -- --- hash: 68fccd06d4ecadf4d0580f3aa528d1ad832002a42e73ae395948ff2f8eb38420 +-- hash: 46c27a7dd27a8d410e856a84b3d86983e03db46567976decb2856d18e0d2068f name: wire-api version: 0.1.0 @@ -151,7 +151,6 @@ test-suite wire-api-tests , tasty-expected-failure , tasty-hunit , tasty-quickcheck - , text , types-common >=0.16 , unordered-containers , uuid