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

Consider disabling the derivation of nested data types during JsonCodeMaker.make as a configuration option #1226

Open
mrooneytrimble opened this issue Dec 5, 2024 · 4 comments
Labels

Comments

@mrooneytrimble
Copy link

mrooneytrimble commented Dec 5, 2024

There are some use cases where the default scheme of generating the JsonValueCodec can cause some issues when migrating from another library (such as play-json) where custom formats have been defined.

One good feature (IMO) about play-json is that all of the nested objects must have json encoder/decoder (Reads/Writes) in scope for the macro to succeed.

In my case we have a custom serialisation format written in play-json which I would like to move to jsoniter for speed. These encoder/decoder(s) are not defined on the companion objects for the classes in question because they come from an in house geometry library, which has no awareness of JSON at all. And unfortunately there are next to no unit tests for this json serialisation (although no doubt I will have to find some payloads in the wild and add some).

This would help in the migration for this use case, as it would force me to think about the serialisation of each data type

Proposal

Add a new configuration option to CodecMakerConfig something like val deriveNestedDataTypes: Boolean which will default to true.

Naively I think that it could be placed here, but no doubt it will be more complex than that

def inferImplicitValue(typeTree: Tree): Tree = c.inferImplicitValue(c.typecheck(typeTree, c.TYPEmode).tpe, silent = config.deriveNestedDataTypes)
@plokhotnyuk
Copy link
Owner

plokhotnyuk commented Dec 5, 2024

@mrooneytrimble Thanks for trying jsoniter-scala and sending your feedback!

Before thinking about the proposed solution I need to understand the root cause of your challenges.

Could you please give some simplified example to clearly see any possible contradictions with the recommended approach for using of jsoniter-scala macros and other approach that you have used with play-json?

Please be concrete in your code samples, so that they could be immediately run by scala-cli or within jsoniter-scala-macros tests.

Please, also, look into sources of geo-scala which provides both circe and jsoniter-scala codecs. Probably the similar approach could be used for your geometry library. BTW, have you considered share it for publicity?

@mrooneytrimble
Copy link
Author

Thanks for your response @plokhotnyuk

I've tried to minimise it as best I can

import Geometry.Triangle
import Geometry.Vector3
import com.github.plokhotnyuk.jsoniter_scala.core.JsonValueCodec
import com.github.plokhotnyuk.jsoniter_scala.core.readFromString
import com.github.plokhotnyuk.jsoniter_scala.macros.JsonCodecMaker
import play.api.libs.json.Format
import play.api.libs.json.JsError
import play.api.libs.json.JsSuccess
import play.api.libs.json.Json
import play.api.libs.json.Reads
import play.api.libs.json.Writes

object Geometry {
  // represents objects in a library that aren't aware of serialization
  case class Vector3(x: Double, y: Double, z: Double)

  case class Triangle(a: Vector3, b: Vector3, c: Vector3)
}

object Application {
  // represents an application that needs to JSON serialize stuff

  case class FloorPlate(mesh: List[Triangle])

  object FloorPlate {

    // play-json only compiles if you uncomment these
//    implicit val vectorFormat: Format[Vector3] = Json.format[Vector3]
//    implicit val triangleFormat: Format[Triangle] = Format(
//      Reads[Triangle](js => {
//        js.validate[Array[Vector3]]
//          .flatMap(in =>
//            if (in.length != 3) JsError("Triangles need 3 vertices")
//            else JsSuccess(Triangle(in(0), in(1), in(2))))
//      }),
//      Writes[Triangle](in => Json.arr(in.a, in.b, in.c)))

    implicit val floorPlateFormat: Format[FloorPlate] = Json.format[FloorPlate]

    implicit val floorPlateCodec: JsonValueCodec[FloorPlate] = JsonCodecMaker.make[FloorPlate]
  }

  val validJson: String =
    """
      |{
      |  "mesh": [
      |    [{"x":0, "y": 0, "z": 0}, {"x":1, "y": 0, "z": 0}, {"x":1, "y": 1, "z": 0}],
      |    [{"x":0, "y": 0, "z": 0}, {"x":1, "y": 1, "z": 0}, {"x":0, "y": 1, "z": 0}]
      |  ]
      |}
      |""".stripMargin
  Json.parse(validJson).as[FloorPlate]

  // this throws an error because the macro has generated definitions for Triangle
  readFromString[FloorPlate](validJson)
}

In this case the snippet won't compile unless you uncomment the implicits for vectorFormat and triangleFormat

However Jsoniter happily compiles and generates the typeclasses for Triangle and Vector3. However the decoding isn't correct because the shape of the JSON does not match the shape of the case classes.

I can remedy that quite easily by providing the JsonValueCodec[Triangle] typeclass. But because Jsoniter is very convenient I am not made aware of this at compile time, only at runtime. For example in this case I would do.

    private val vectorArrayCodec: JsonValueCodec[Array[Vector3]] = JsonCodecMaker.make[Array[Vector3]]
    implicit val triangleCodec: JsonValueCodec[Triangle] = new JsonValueCodec[Triangle] {
      override def decodeValue(in: JsonReader, default: Triangle): Triangle = {
        val array = vectorArrayCodec.decodeValue(in, vectorArrayCodec.nullValue)
        if (array.length != 3) in.decodeError("Expected 3 vertices for a triangle")
        Triangle(array(0), array(1), array(2))
      }

      override def encodeValue(x: Triangle, out: JsonWriter): Unit = {
        vectorArrayCodec.encodeValue(Array(x.a, x.b, x.c), out)
      }

      override def nullValue: Triangle = null
    }
    implicit val floorPlateCodec: JsonValueCodec[FloorPlate] = JsonCodecMaker.make[FloorPlate]
  }

Let me know if I should provide any more info, or if this example isn't concrete enough. It might be that i'm the only person affected by this kind of issue 😆.

BTW, have you considered share it for publicity?

Sadly this isn't my call to make, but is something I would like to do.

@plokhotnyuk
Copy link
Owner

plokhotnyuk commented Dec 7, 2024

First of all here is a more efficient custom codec for Triangle:

implicit val codecOfTriangle: JsonValueCodec[Triangle] = new JsonValueCodec[Triangle] {
  private val codecOfVector3 = make[Vector3]

  override def decodeValue(in: JsonReader, default: Triangle): Triangle =
    if (in.isNextToken('[')) {
      val a = codecOfVector3.decodeValue(in, codecOfVector3.nullValue)
      val b =
        if (in.isNextToken(',')) codecOfVector3.decodeValue(in, codecOfVector3.nullValue)
        else in.commaError()
      val c =
        if (in.isNextToken(',')) codecOfVector3.decodeValue(in, codecOfVector3.nullValue)
        else in.commaError()
      if (in.isNextToken(']')) new Triangle(a, b, c)
      else in.arrayEndError()
    } else in.readNullOrTokenError(default, '[')

  override def encodeValue(x: Triangle, out: JsonWriter): _root_.scala.Unit = {
    out.writeArrayStart()
    codecOfVector3.encodeValue(x.a, out)
    codecOfVector3.encodeValue(x.b, out)
    codecOfVector3.encodeValue(x.c, out)
    out.writeArrayEnd()
  }

  override def nullValue: Triangle = null
}

It fails faster in parsing and does not instantiate intermediate arrays during parsing and serialization.

The similar codecs can be automatically generated for tuples.

Probably it worth to add a compile-time option to allow parsing/serialization of product types as JSON arrays (not maps).

But any new compile-time option will not save you from need to adding tests that covers parsing and serialization of all your data structures, including round-trip conversions.

So, I strongly recommend starting from adding good test coverage for play-json and then re-use those tests for jsoniter-scala codecs.

@mrooneytrimble
Copy link
Author

Thank you @plokhotnyuk that's really helpful :)

I wasn't really aware how best to write a decoder manually. That little trick with the Nothing type is really quite clever! I'll make sure and use that in the future if I need to.

But any new compile-time option will not save you from need to adding tests that covers parsing and serialization of all your data structures, including round-trip conversions.

So, I strongly recommend starting from adding good test coverage for play-json and then re-use those tests for jsoniter-scala codecs.

Yes true this makes perfect sense. I would need to do that regardless.

Feel free to close this issue as question answered, it would probably be quite a lot of work and based on the lack of activity on this issue I'm likely the only one who would like this typeclass resolving behaviour anyway 👍

Probably it worth to add a compile-time option to allow parsing/serialization of product types as JSON arrays (not maps).

That would be really nice actually, it would save quite a lot of boiler-plate. I've had to write similar encoders in the past to make JSON for other kinds of Geometry a bit terser to maximise bandwidth, in those cases I was just lazy and didn't implement the decoder (because I didn't need to)

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

No branches or pull requests

2 participants