zio / zio-json

Fast, secure JSON library with tight ZIO integration.
https://zio.dev/zio-json
Apache License 2.0
401 stars 144 forks source link

Default case class values are memoized #1055

Open jwcarvana opened 6 months ago

jwcarvana commented 6 months ago

zio-json is memoizing a variable's default in case classes even when the default is a method.

Here is a Scala 2 example. I'd expect each deserialization to create a unique UUID.

import zio.json._

import java.util.UUID

case class MyCaseClass(id: UUID = UUID.randomUUID, name: String)

object MyCaseClass {
  implicit val decoder: JsonDecoder[MyCaseClass] = DeriveJsonDecoder.gen[MyCaseClass]
  implicit val encoder: JsonEncoder[MyCaseClass] = DeriveJsonEncoder.gen[MyCaseClass]
}

val str = """{"name": "david"}"""

val a = str.fromJson[MyCaseClass].toOption.get
val b = str.fromJson[MyCaseClass].toOption.get
val c = str.fromJson[MyCaseClass].toOption.get

println(a) // MyCaseClass(36fd6b6a-5d9b-4939-a98a-7924d4ed259c,david)
println(b) // MyCaseClass(36fd6b6a-5d9b-4939-a98a-7924d4ed259c,david)
println(c) // MyCaseClass(36fd6b6a-5d9b-4939-a98a-7924d4ed259c,david)

val d = MyCaseClass(name = "david")
val e = MyCaseClass(name = "david")
val f = MyCaseClass(name = "david")

println(d) // MyCaseClass(9157e22f-80af-44ee-81e9-b6d0acd397e0,david)
println(e) // MyCaseClass(9a6ecb29-b02b-41d7-822d-9d4ad3865838,david)
println(f) // MyCaseClass(1027c46c-329c-44d4-b59a-a783076fd872,david)
plokhotnyuk commented 6 months ago

@jwcarvana Thanks for reporting!

I've just added a missing check for default value memorization in jsoniter-scala and it passes for both Scala 2 and Scala 3 versions.

Probably similar macro implementations could be used to fix the issue in zio-json, if it is not designed intentionally to support only constant values for defaults.

jdegoes commented 2 months ago

/bounty $100

algora-pbc[bot] commented 2 months ago

💎 $100 bounty • ZIO

Steps to solve:

  1. Start working: Comment /attempt #1055 with your implementation plan
  2. Submit work: Create a pull request including /claim #1055 in the PR body to claim the bounty
  3. Receive payment: 100% of the bounty is received 2-5 days post-reward. Make sure you are eligible for payouts

Thank you for contributing to zio/zio-json!

Add a bountyShare on socials

Attempt Started (GMT+0) Solution
🔴 @varshith257 Apr 25, 2024, 6:26:55 PM WIP
🔴 @Andrapyre May 2, 2024, 8:30:15 AM #1119
varshith257 commented 2 months ago

/attempt #1055

Andrapyre commented 2 months ago

/attempt #1055

Andrapyre commented 2 months ago

Side note: default parameters are not working in Scala 3 at all, unless users add the -Yretain-trees scalac option.

Given the following case class:

final case class RandomClass(
  str: String,
  uuid: UUID = UUID.randomUUID
)

And the following code in zio.json.macros.DeriveJsonDecoder.join:

def getDefaults: Array[Option[Any]] = {
  println(s"Random id: ${UUID.randomUUID().toString}")
  println(ctx.parameters.map(_.default).toArray.mkString("Array(", ", ", ")"))
  ctx.parameters.map(_.default).toArray
}

The first logged id is different every time a string is decoded into a RandomClass, while the second id is the same. E.g. this issue comes from Magnolia. It can be fixed in several ways:

  1. A PR into Magnolia. I'm not sure that this fits with their design and would likely involve a change breaking binary compat.
  2. A change in zio-json involving an annotation such as fieldDefaultValue, like there is in zio-schema. Currently all annotations in zio-json relate to field names, not to values. Is this by design?
  3. No code change - add documentation explaining how to work with default parameters generally (including how to deal with defaults in Scala 3), as well as a workaround if users want to have dynamic default values.

I would tend towards 3 - serialization with random generators isn't referentially transparent and I would lean towards keeping the two separate and doing random generation as part of an effect, serializing the case class with a default null value, and then copying the randomly generated value over. If there are performance considerations, then the following should suffice:

final case class RandomClass(
  str: String,
  @jsonField("id")
  private val initialId: UUID = null
) {
  val id: UUID = if (initialId == null) {
    UUID.randomUUID()
  } else {
    initialId
  }
}

@jdegoes , any preferences here?

algora-pbc[bot] commented 2 months ago

@Andrapyre: Reminder that in 7 days the bounty will become up for grabs, so please submit a pull request before then 🙏

algora-pbc[bot] commented 2 months ago

The bounty is up for grabs! Everyone is welcome to /attempt #1055 🙌

algora-pbc[bot] commented 1 month ago

💡 @Andrapyre submitted a pull request that claims the bounty. You can visit your bounty board to reward.