plokhotnyuk / jsoniter-scala

Scala macros for compile-time generation of safe and ultra-fast JSON codecs + circe booster
MIT License
750 stars 99 forks source link

Adt discriminator in non-first position #1196

Closed codesurf42 closed 2 months ago

codesurf42 commented 2 months ago

Snippet from output of sbt clean jsoniter-scala-macrosJVM/test:

[info] - should serialize and deserialize case class ADTs using discriminator (1 millisecond)
[info] - should serialize and deserialize case class ADTs using discriminator in different positions *** FAILED *** (4 milliseconds)
[info]   "...ype":"AAA","a":1},{"[type":"BBB","a":1},{"type":"CCC","a":1],"b":"VVV"},{"type":..." was not equal to "...ype":"AAA","a":1},{"[a":1,"type":"BBB"},{"a":1,"type":"CCC"],"b":"VVV"},{"type":..." (VerifyingSpec.scala:68)
[info]   org.scalatest.exceptions.TestFailedException:
[info]   at org.scalatest.matchers.MatchersHelper$.indicateFailure(MatchersHelper.scala:392)
[info]   at org.scalatest.matchers.should.Matchers$AnyShouldWrapper.shouldBe(Matchers.scala:7539)
plokhotnyuk commented 2 months ago

@codesurf42 Thanks for your PR!

Please resubmit it with only your changes.

codesurf42 commented 2 months ago

refreshed @plokhotnyuk

plokhotnyuk commented 2 months ago

The correct version of your test would be:

    "serialize and deserialize case class ADTs using discriminator in different positions" in {
      verifyDeser(make[List[AdtBase]](CodecMakerConfig.withRequireDiscriminatorFirst(false)),
        List(AAA(1), BBB(BigInt(1)), CCC(1, "VVV"), DDD),
        """[{"type":"AAA","a":1},{"a":1,"type":"BBB"},{"a":1,"type":"CCC","b":"VVV"},{"type":"DDD"}]""")
      verifyDeser(make[List[AdtBase]](CodecMakerConfig
        .withRequireDiscriminatorFirst(false)
        .withDiscriminatorFieldName(_root_.scala.Some("t"))),
        List(CCC(2, "WWW"), CCC(1, "VVV")), """[{"a":2,"b":"WWW","t":"CCC"},{"a":1,"t":"CCC","b":"VVV"}]""")
    }

The CodecMakerConfig.withRequireDiscriminatorFirst(false) option changes only decoding part of the generated codec, and the encoding part always write the discriminator field in the first position.

plokhotnyuk commented 2 months ago

It could be great degradation of performance for deeply nested ADTs in parsers that do not use intermediate AST representation (like zio-json, borer, etc.) when the discriminator field appears in the last position. So for security reasons the discriminator field allowed only in the first position by default. If you have only trusted input in your system or only non-nested ADTs then you can use CodecMakerConfig.withRequireDiscriminatorFirst(false) without worrying about possible DoS/DoW attacks.

codesurf42 commented 2 months ago

Thanks for quick reactions.

It could be great degradation of performance

It is an understandable reason. How about adding something in readme - about the chosen default for the first position and how someone can change it? I can add it in this PR, somewhere around - An optional name of the discriminator field for ADTs - wdyt?