Play JSON codecs #37

vil1 · 2019-01-13T18:34:09Z

This PR adds interpreters for (en|de)coders to/from the Play! JSON AST in a playJson module, and is therefore part of #32.

More specifically, it introduces interpreters for play.api.libs.json.Reads and play.api.libs.json.Writes.

JSON having no standard support for mere products (aka tuples), I had to make a decision about how we represent such mere products. At the same time, I had to devise a way to handle differently products that are part of a Record (which must be represented as flat JsObjects) and "mere products" (represented as I arbitrarily decided).

Solving that problem made me introduce HCoalgebra, hyloNT and HEnvT.

HCoalgebra is the dual of HAlgebra, ie. a natural transformation F ~> S[F, ?]
hyloNT is a recursion scheme that uses both an HCoalgebra and HAlgebra. It builds up a tree (here, some schema flavour) using the HCoalgebra and then collapses if (into a target functor F) using the HAlgebra.
HEnvT is a mean for labelling nodes of a tree (here a schema) with values of some given type, while retaining the structure of that tree (schema).

These three constructs are used to derive Reads and Writes from a schema.

First I use an HCoalgebra to "push-down" information (using HEnvT) so that every :*: node "knows" if it's part of a Record or not, and then I use this information in the HAlgebra to decide how to render this particular :*: node.

The :*: nodes that are under a Record node are rendered as a single JsObject: we know that these nodes contain only ProductTerms which are rendered as JsObject with a single field, these JsObjects are then merged to form the record's representation.

Other :*: node are considered as tuples, which are (debatably) rendered as { "_1": "foo", "_2": { "_1": "bar", "_2": ...}}.

Tests ensure that the Reads and Writes derived from a given schema are symmetrical, ie. reads(writes(data)) == data.

GrafBlutwurst

I don't quite follow the annotation logic you employ during the coalgebra. you start out with a false, a record causes any further nodes to carry along a true, a :*: passes false to left and carries along on the right and any other just carries the flag along.

but shouldn't a record pass a true to all immediate children and in any other case pass a false?

GrafBlutwurst · 2019-01-14T15:13:11Z

modules/play-json/src/main/scala/PlayJsonModule.scala

+            HEnvT(
+              x,
+              :*:(
+                ascribeWith(false)(left),


Why are you ascribing left always with false?

GrafBlutwurst · 2019-01-14T15:20:05Z

modules/play-json/src/main/scala/PlayJsonModule.scala

+              Writes(
+                pair =>
+                  (left.writes(pair._1), right.writes(pair._2)) match {
+                    case (l @ JsObject(_), r @ JsObject(_)) => l ++ r


could this collide if both terms have the same productTermID ?

Absolutely, and we currently don't ensure that such "absurd" schema (having multiple fields with the same name) cannot be constructed.

vil1 · 2019-01-14T15:42:58Z

I don't quite follow the annotation logic you employ during the coalgebra. you start out with a false, a record causes any further nodes to carry along a true, a :*: passes false to left and carries along on the right and any other just carries the flag along.

but shouldn't a record pass a true to all immediate children and in any other case pass a false?

This needs clarification indeed.

What I want is to label with true all the "chain" of :*: that are under a Record, but not their children.

So I schematically (no pun intended) end up with something like:

false -> Record(
  true -> :*:(
    false -> x // <- is guarantied to be a ProductTerm,
    true -> :*:(
       false -> y,
       true -> z  // also guarantied to be a ProductTerm
    )
  )
)

The "last child" (here z) ends up labelled with true but it doesn't matter (it is guarantied to be a ProductTerm and I don't use the label for rendering those).

Does that make more sense?

julienrf · 2019-01-14T19:16:57Z

.travis.yml

@@ -7,5 +7,5 @@ jdk:
 before_install:
  - export PATH=${PATH}:./vendor/bundle
 script:
-  - sbt ++$TRAVIS_SCALA_VERSION scalafmtCheck test:scalafmtCheck scalafmtSbtCheck "project core" test:run "project scalacheck" test:run
+  - sbt ++$TRAVIS_SCALA_VERSION scalafmtCheck test:scalafmtCheck scalafmtSbtCheck "project core" test:run "project scalacheck" test:run "project playJson" test:run


What do you think of defining a root module aggregating all the modules?

I think I should have thought about it earlier, (I mean, we do have a root module aggregating all other modules already).
Anyway, this will be indirectly fixed when I update this PR to accommodate the changes introduced by #39.

julienrf · 2019-01-14T20:16:40Z

modules/play-json/src/main/scala/PlayJsonModule.scala

+      val alg = new (HEnvT[Boolean, RSchema, Writes, ?] ~> Writes) {
+
+        def apply[A](env: HEnvT[Boolean, RSchema, Writes, A]): Writes[A] = env.fa match {
+          case One()            => Writes(_ => JsNull)


Do you think it would also be possible to not emit optional fields that have the None value?

I'm not completely sure it would be impossible, but I'm unable to devise a way to do so (for now).

That's because of the encoding of options as a mere Iso on top of A \/ Unit. Not emitting fields valued as None would therefore require to find a way to process x :+: y differently wether y is One() or something else.

So I guess your remark makes another case for adding an Optional member to the Schema GADT.

So I guess your remark makes another case for adding an Optional member to the Schema GADT.

That’s what I also think.

This is what I was saying in my PR regarding A \/ Unit can lead to an ugly representation. The Optional member for the GADT should be trivial but we should take care we don't pollute the GADT too much with such special cases. This brings us full circle back to the discussion in gitter about effects in schemas 😁

julienrf · 2019-01-14T20:20:48Z

modules/play-json/src/main/scala/PlayJsonModule.scala

+                      },
+                    a => JsSuccess(-\/(a))
+                  )
+            )


Would it be possible to directly choose the correct (left or right) decoder based on its label?

Well, at that exact point it wouldn't, because a :+: node has no clue about what its children are "looking for" (and in the case of a Union, the label is handled by the :+:'s children).
But that's a good lead for future optimisation.

julienrf · 2019-01-14T20:32:19Z

modules/play-json/src/main/scala/PlayJsonModule.scala

+                  .fold(
+                    el =>
+                      right.reads(json).map(\/-.apply) match {
+                        case JsError(er) => JsError(JsError.merge(el, er))


Why merge the errors of both alternatives?

Given the Schema[Role] defined in the tests, I would expect the following behavior from decoders:

{ "user": {} } would be decoded as a failure with the following (single) error: “Missing field ’active’” ;

{ "moderator": {} } would be decoded as a failure with the following error: “Unknown alternative label: 'moderator'. Valid labels are: 'user', 'admin'.” ;

{} would be decoded as a failure with the following error: “Missing alternative label”.

I agree that error management isn't completely satisfactory.

This is due to:

the fact that (as you pointed out in another comment) we don't leverage our encoding for unions, which wraps branches in an object with a single field whose name is the branch's label (mostly because we're unable to do so ATM)

the semantics of JsError. Getting a JsError(Seq(x, y, z)) means "My unfulfilled expectations are x and y and z". But in the case of a union, we would need a "or" semantic.

But know that I've written it down, I realise that 2. is a consequence of 1. If we find a way to properly leverage our encoding for branches, we should be able to get better error messages.

I'm not sure we can/want to satisfy your second point though (detecting "unknown branches")

I'm not sure we can/want to satisfy your second point though (detecting "unknown branches")

At least, just reporting “unknown alternative: 'moderator'”, would be fine.

That might be doable.

julienrf · 2019-01-14T20:34:21Z

modules/play-json/src/main/scala/PlayJsonModule.scala

+          case SeqSchema(elem) =>
+            Reads {
+              case JsArray(elems) =>
+                elems.toList.traverse(elem.reads _)


Why don’t we need to wrap with undefinedAsNull here?

Anyway, I guess that if we add Optional as an AST node we won’t need undefinedAsNull anymore :)

I kept thinking about it, and I had to sadly remove that GIF, because I think you're wrong after all.

Once you "get" to a SeqSchema, you're necessarily "inside" a field's value, so the udefinedAsNull part will be taken care of by the ProductTerm above.

julienrf · 2019-01-14T20:36:12Z

modules/core/src/main/scala/Json.scala

-          case One()             => (_ => "null")
-        }
+  ): RInterpreter[Encoder] =
+    new Interpreter[R.Prim, R.SumTermId, R.ProductTermId, Encoder] {


It feels to me a bit heavy to always carry these type parameters everywhere. Why not use abstract type members instead? I see below that you defined an RInterpreter type alias to alleviate the syntactic noise but I think it would be simpler to use type members everywhere instead of type parameters.

I intend to get #50 merged before this PR, that should enable me to make type signatures much more concise in this PR.

julienrf · 2019-01-14T20:38:44Z

modules/core/src/main/scala/Json.scala

+              encloseInBraces.compose(u.choices).compose(u.iso.reverseGet)
+            case SumTerm(id, base) => makeField(branchLabel(id)).compose(base)
+            case One()             => (_ => "null")
+          }


This is unrelated to this PR but what is the difference between :*:, Record and ProductTerm?

:*:(a, b) represents a (A, B), ProductTerm(id, a) represents a "field" with name id and schema a (it doesn't make much sense outside of a record).
A Record contains a schema that is guarantied to be composed of a "tree of :*:" whose all left children (as well as the right child of the deepest :*:) are ProductTerms.

A Record contains a schema that is guarantied to be composed of a "tree of :*:" whose all left children (as well as the right child of the deepest :*:) are ProductTerms.

Could we see this guarantee in the types? Currently the type of a record’s fields is F[_].

I don't think it's doable. We need fields to be a F[_] to use recursion schemes (during a cataNT for example, F is bound to the "target functor" the schema is folded to).

GrafBlutwurst reviewed Jan 14, 2019

View reviewed changes

julienrf reviewed Jan 14, 2019

View reviewed changes

vil1 force-pushed the play-json branch 2 times, most recently from eb4ce54 to 892e86f Compare January 14, 2019 22:44

vil1 added 8 commits February 10, 2019 15:46

First (almost) working draft for an interpreter to play Json Reads

e1db276

Add naive interperter for play.api.libs.json.Writes

fa65933

Add hyloNT and use it to make Reads/Writes interpreters symmetric

e082ad0

Handle errors slightly more correctly

168c362

Add tests

83b2225

Remove unused CataInterpreter and HyloInterpreter

de4d1be

Moved tests according to changes introduced by spartanz#39

8c24080

Fix and refactor after rebasing on prototyping

de00e7f

vil1 force-pushed the play-json branch from 892e86f to de00e7f Compare February 10, 2019 16:08

Make it easier to create interpreters

2d53a97

vil1 force-pushed the play-json branch from 2f703fe to 2d53a97 Compare February 10, 2019 17:13

Move all recursion-related code to a separate file/package

7348f60

vil1 force-pushed the play-json branch from 2194b7d to 7348f60 Compare February 10, 2019 17:33

Rename interpreters

90249e2

vil1 merged commit 9b2eef8 into spartanz:prototyping Feb 11, 2019

vil1 deleted the play-json branch February 11, 2019 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Play JSON codecs #37

Play JSON codecs #37

vil1 commented Jan 13, 2019

GrafBlutwurst left a comment

GrafBlutwurst Jan 14, 2019

GrafBlutwurst Jan 14, 2019

vil1 Jan 14, 2019

vil1 commented Jan 14, 2019

julienrf Jan 14, 2019

vil1 Jan 14, 2019

julienrf Jan 14, 2019

vil1 Jan 14, 2019

julienrf Jan 14, 2019

GrafBlutwurst Jan 15, 2019

julienrf Jan 14, 2019

vil1 Jan 14, 2019

julienrf Jan 14, 2019 •

edited

Loading

vil1 Jan 14, 2019

julienrf Jan 14, 2019

vil1 Jan 14, 2019

julienrf Jan 14, 2019

julienrf Jan 14, 2019

vil1 Jan 22, 2019

julienrf Jan 14, 2019

vil1 Jan 22, 2019

julienrf Jan 14, 2019 •

edited

Loading

vil1 Jan 14, 2019

julienrf Jan 15, 2019

vil1 Jan 22, 2019

Play JSON codecs #37

Play JSON codecs #37

Conversation

vil1 commented Jan 13, 2019

GrafBlutwurst 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

vil1 commented Jan 14, 2019

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

julienrf Jan 14, 2019 • 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

julienrf Jan 14, 2019 • 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

julienrf Jan 14, 2019 •

edited

Loading

julienrf Jan 14, 2019 •

edited

Loading