Use structured JSON for interfacing with 'ClientRequest' / 'ClientResponse' #29

KtorZ · 2021-06-24T16:27:26Z

📍 Use structured JSON for interfacing with 'ClientRequest'
📍 Remove use of aeson-casing
Instead, favor hand-written JSON instances over generic with options.
The rationale being that generic deriving with options and other
meta-programming are often hard to read and easy to shoot oneself in
the foot.

Instead, using JSON builder makes for transparent definitions at the
cost of a small maintainance burden when changing data-types.
📍 Replace use of aesonQQ with builders
QuasiQuoters don't make unanimity, but builders do despite being
perhaps a bit too polymorphic.
📍 Write JSON roundtrip properties for ClientRequest
This was slightly more annoying than I initially thought because of
the shrinkers. I thought I could get away with genericShrink and a
Generic instance on ClientRequest, but this requires the subtypes
to have Arbitrary instances, which is pretty annoying since we're
trying to precisely avoid those.

Ended up writing the shrinker by hand, not sure if it's worth it given
that the model is small enough in practice anyway...
📍 Write JSON instances for 'ClientResponse'
📍 Add {From,To}JSON / {From,To}CBOR / Arbitrary instances to hydra-prelude
All used pervasively across the codebase. So good candidates to be factored out in the prelude.
📍 Define Arbitrary instances for base data-types in lib code
After discussion, we concluded that it is pretty useful to have
generic Arbitrary instances readily available for base data-types, and
only provide hand-written generators on a case-by-case. This leverages
the generic-random package for generating arbitrary instances, and
quickcheck for shrinkers when possible.
📍 Refactor End-to-End to use smart-constructors for building JSON requests.
📍 Reply with structured JSON instead of mere 'show'
📍 Rework 'waitForResponse' expectation failure message
Enhance readability, as a big unformatted show is like looking for a needle in a haystack. Not good for quick debugging.
📍 Represent 'Party' as JSON integers
The 'real' implementation should likely be more like it was before
this commit, a base16 or bech32 encoding of the key. But with the mock
cryptos, keys are basically just numbers and it's easier to read than
long bytestring in end-to-end tests. Arguably, the end-to-end
interface should not change when we change the crypto, so this may be
only midly correct...
📍 Rename 'ClientRequest' -> 'ClientInput', 'ClientResponse' -> 'ServerOutput'

ch1bo · 2021-06-24T16:29:56Z

local-cluster/test/Test/EndToEndSpec.hs

-                sendRequest n3 "Commit (fromList [3])"
+                sendRequest n1 [aesonQQ|{"req": "commit", "args": [1] }|]
+                sendRequest n2 [aesonQQ|{"req": "commit", "args": [2] }|]
+                sendRequest n3 [aesonQQ|{"req": "commit", "args": [3] }|]


Why do we need quasi quotes here?

Because it's handy and readable for constructing JSON ?

(if you have a better proposal, I am glad to hear it 😶)

I have used "builders" for an Aeson.Value in the past, but have also seem full string quasi-quotation. This seems to be something in between from the looks of it. Personally I prefer to use the former as the compiler helps in finding bugs when creating the json AST.

The following should be equal, but I would be interested in seeing how the two approaches compare in a more complicated example:

Suggested change

sendRequest n3 [aesonQQ|{"req": "commit", "args": [3] }|]

sendRequest n3 $ object [ "req" .= "commit", "args" .= [3] ]

I would also favor builders, I don't see what QQuoters add here, it adds a dependency and compillation time for little value.

And as the name implies, Aeson builders will lead us into test builders which will simplify the tests.

I don't see what QQuoters add here, it adds a dependency and compillation time for little value.

Well, actually, the quoter comes from the aeson library itself so it's not like it is adding much dependency. Plus, it's not a mere string QQ, it does parse the JSON underneath and provides compile-time errors for invalid JSON. The added value to me really being in the "what you see is what you get" kind of spirit. With builders and implicit conversions it's sometimes easy to miss something.

ch1bo · 2021-06-25T05:58:49Z

hydra-node/src/Hydra/Ledger/Simple.hs

@@ -27,6 +35,12 @@ data SimpleTx = SimpleTx
  }
  deriving stock (Eq, Ord, Generic, Read, Show)

+instance ToJSON SimpleTx where
+  toJSON = genericToJSON (aesonPrefix camelCase)


So this is creating a JSON object with fields id, inputs and outputs? IMO this "drop all lowercase letters until first uppercase" convention of this aesonPrefix is only sligthly better than drop 2 and would like to avoid such things as much as possible. Can we stick to just use field names as is and make then non-overlapping or non-conflicting on a case by case basis?

Can we stick to just use field names as is and make then non-overlapping or non-conflicting on a case by case basis?

I am fine with that ^.^

ch1bo · 2021-06-25T06:06:04Z

hydra-node/src/Hydra/HeadLogic.hs

+clientRequestJsonOptions :: Aeson.Options
+clientRequestJsonOptions =
+  Aeson.defaultOptions
+    { constructorTagModifier = camelCase


This camelCase function is basically mapHead toLower and I know now why you were suggesting to avoid UTxO as a name as it would turn out as uTxO using this constructor modifier 😂

Not sure though whether we want to have another dependency just for this very simple modifier

Instead of adding dependencies and doing fancy meta-programming I'd be even open to hand-written instances. Sometimes they are easier to read and maintain than genericXXX + various options and modifiers

I am with @ch1bo on this: Let's use Generic until we hit a snag and then let's write manual instances.

Not sure though whether we want to have another dependency just for this very simple modifier

I initially wrote them by hand, and ended up writing the same one in two places. Wanted to move in a separate module but it felt a bit too excessive / inappropriate. In the end, aeson-casing is a tiny library with almost no dependency (base and aeson) which kinda does just is needed.

Writing JSON by hand is always doable, just a bit annoying. I think my favorite approach remains generics with default options, and possibly DuplicateRecordFields when really necessary. With NamedFieldPuns it's usually not a big problem.

ch1bo

The aesonPrefix kinda rubs me the wrong way (have been bitten by drop 16 as a field modifier in the past) and would be interested in alternatives.

ghost

Like @ch1bo I would suggest using "standard" Generic deriving wherever we can without "fancy" names and JSON wrangling, and use Aeson builders for constructing Value instances.

ghost · 2021-06-25T06:54:33Z

hydra-node/src/Hydra/API/Server.hs

  deriving (Eq, Show)

 withAPIServer ::
-  Tx tx =>
+  (Tx tx, FromJSON tx, FromJSON (UTxO tx)) =>


We have already added Show and Eq instances directly in the declaration of class Tx tx, would it not make sense to do the same for FromJSON and ToJSON ?

Thought of it, and even considered maybe to pass codecs as a record object actually, to not necessarily tied to JSON all-the-way. Problem is that we'll likely have to have JSON instances anyway so.. the benefits of abstracting away the codec is maybe not that great.

Moving it to Tx is a possibility 👍

ghost · 2021-06-25T06:56:41Z

hydra-node/src/Hydra/HeadLogic.hs

+clientRequestJsonOptions :: Aeson.Options
+clientRequestJsonOptions =
+  Aeson.defaultOptions
+    { constructorTagModifier = camelCase


I am with @ch1bo on this: Let's use Generic until we hit a snag and then let's write manual instances.

ghost · 2021-06-25T06:58:35Z

hydra-node/src/Hydra/HeadLogic.hs

+          { tagFieldName = "req"
+          , contentsFieldName = "args"
+          }
+    }


Is this something related to JSON-RPC? Doesn't seem so: https://www.jsonrpc.org/specification

No. But the default names from aeson are tag and content which I very much dislike. So I went for something a bit more appropriate IMO.

ghost · 2021-06-25T07:00:35Z

local-cluster/test/Test/EndToEndSpec.hs

-                sendRequest n3 "Commit (fromList [3])"
+                sendRequest n1 [aesonQQ|{"req": "commit", "args": [1] }|]
+                sendRequest n2 [aesonQQ|{"req": "commit", "args": [2] }|]
+                sendRequest n3 [aesonQQ|{"req": "commit", "args": [3] }|]


I would also favor builders, I don't see what QQuoters add here, it adds a dependency and compillation time for little value.

ghost · 2021-06-25T07:01:11Z

local-cluster/test/Test/EndToEndSpec.hs

-                sendRequest n3 "Commit (fromList [3])"
+                sendRequest n1 [aesonQQ|{"req": "commit", "args": [1] }|]
+                sendRequest n2 [aesonQQ|{"req": "commit", "args": [2] }|]
+                sendRequest n3 [aesonQQ|{"req": "commit", "args": [3] }|]


And as the name implies, Aeson builders will lead us into test builders which will simplify the tests.

ghost

I think adding golden master tests using https://hackage.haskell.org/package/hspec-golden-aeson and basic "isomorphism" properties using https://hackage.haskell.org/package/quickcheck-classes-0.6.5.0/docs/Test-QuickCheck-Classes.html#jsonLaws would be a great addition once we start investing in JSON.

ch1bo · 2021-06-28T16:42:34Z

hydra-node/src/Hydra/HeadLogic.hs

+
+  -- NOTE: Somehow, can't use 'genericShrink' here as GHC is complaining about
+  -- Overlapping instances with 'UTxO tx' even though for a fixed `tx`, there
+  -- should be only one 'UTxO tx'


Maybe we're missing a functional dependency or so?

Thought so. But really, it shouldn't be necessary here. I didn't give it much thought.

Instead, favor hand-written JSON instances over generic with options. The rationale being that generic deriving with options and other meta-programming are often hard to read and easy to shoot oneself in the foot. Instead, using JSON builder makes for transparent definitions at the cost of a small maintainance burden when changing data-types.

QuasiQuoters don't make unanimity, but builders do despite being perhaps a bit _too polymorphic_.

This was slightly more annoying than I initially thought because of the shrinkers. I thought I could get away with `genericShrink` and a `Generic` instance on `ClientRequest`, but this requires the subtypes to have `Arbitrary` instances, which is pretty annoying since we're trying to precisely avoid those. Ended up writing the shrinker by hand, not sure if it's worth it given that the model is small enough in practice anyway...

All used pervasively across the codebase. So good candidates to be factored out in the prelude.

After discussion, we concluded that it is pretty useful to have generic Arbitrary instances readily available for base data-types, and only provide hand-written generators on a case-by-case. This leverages the generic-random package for generating arbitrary instances, and quickcheck for shrinkers when possible.

…sts.

Enhance readability, as a big unformatted show is like looking for a needle in a haystack. Not good for quick debugging.

The 'real' implementation should likely be more like it was before this commit, a base16 or bech32 encoding of the key. But with the mock cryptos, keys are basically just numbers and it's easier to read than long bytestring in end-to-end tests. Arguably, the end-to-end interface should not change when we change the crypto, so this may be only midly correct...

…utput'

KtorZ requested a review from a team June 24, 2021 16:27

KtorZ self-assigned this Jun 24, 2021

ch1bo reviewed Jun 24, 2021

View reviewed changes

ch1bo reviewed Jun 25, 2021

View reviewed changes

ch1bo requested changes Jun 25, 2021

View reviewed changes

ghost reviewed Jun 25, 2021

View reviewed changes

ghost reviewed Jun 28, 2021

View reviewed changes

KtorZ force-pushed the KtorZ/JSON-for-interface branch from 65ec25d to 4b5864d Compare June 28, 2021 10:04

ch1bo reviewed Jun 28, 2021

View reviewed changes

KtorZ force-pushed the KtorZ/JSON-for-interface branch from d42fd29 to a769cc2 Compare June 28, 2021 16:46

KtorZ added 12 commits June 28, 2021 18:48

Use structured JSON for interfacing with 'ClientRequest'

057d03f

Replace use of aesonQQ with builders

9821a3c

QuasiQuoters don't make unanimity, but builders do despite being perhaps a bit _too polymorphic_.

Write JSON instances for 'ClientResponse'

64e06b9

Add {From,To}JSON / {From,To}CBOR / Arbitrary instances to hydra-prelude

ca83826

All used pervasively across the codebase. So good candidates to be factored out in the prelude.

Refactor End-to-End to use smart-constructors for building JSON reque…

f7ef1cc

…sts.

Reply with structured JSON instead of mere 'show'

817f1c4

Rework 'waitForResponse' expectation failure message

a1831d8

Enhance readability, as a big unformatted show is like looking for a needle in a haystack. Not good for quick debugging.

Rename 'ClientRequest' -> 'ClientInput', 'ClientResponse' -> 'ServerO…

f957ff1

…utput'

KtorZ force-pushed the KtorZ/JSON-for-interface branch from a769cc2 to f957ff1 Compare June 28, 2021 17:02

ch1bo merged commit 72aeeb0 into master Jun 29, 2021

ch1bo deleted the KtorZ/JSON-for-interface branch June 29, 2021 08:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use structured JSON for interfacing with 'ClientRequest' / 'ClientResponse' #29

Use structured JSON for interfacing with 'ClientRequest' / 'ClientResponse' #29

KtorZ commented Jun 24, 2021 •

edited

Loading

ch1bo Jun 24, 2021

KtorZ Jun 24, 2021

KtorZ Jun 24, 2021

ch1bo Jun 25, 2021

ghost Jun 25, 2021

ghost Jun 25, 2021

KtorZ Jun 28, 2021

ch1bo Jun 25, 2021

KtorZ Jun 28, 2021

ch1bo Jun 25, 2021

ch1bo Jun 25, 2021

ghost Jun 25, 2021

KtorZ Jun 28, 2021

ch1bo left a comment

ghost left a comment

ghost Jun 25, 2021

KtorZ Jun 28, 2021

ghost Jun 25, 2021

ghost Jun 25, 2021

KtorZ Jun 28, 2021

ghost Jun 25, 2021

ghost Jun 25, 2021

ghost left a comment

ch1bo Jun 28, 2021

KtorZ Jun 28, 2021

	sendRequest n3 [aesonQQ\|{"req": "commit", "args": [3] }\|]
	sendRequest n3 $ object [ "req" .= "commit", "args" .= [3] ]

Use structured JSON for interfacing with 'ClientRequest' / 'ClientResponse' #29

Use structured JSON for interfacing with 'ClientRequest' / 'ClientResponse' #29

Conversation

KtorZ commented Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ch1bo left a comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ commented Jun 24, 2021 •

edited

Loading