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

Default case class values are memoized #1055

Open
jwcarvana opened this issue Jan 4, 2024 · 9 comments · May be fixed by #1119
Open

Default case class values are memoized #1055

jwcarvana opened this issue Jan 4, 2024 · 9 comments · May be fixed by #1119

Comments

@jwcarvana
Copy link

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
Copy link
Contributor

plokhotnyuk commented Jan 5, 2024

@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
Copy link
Member

jdegoes commented Apr 25, 2024

/bounty $100

Copy link

algora-pbc bot commented Apr 25, 2024

💎 $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
Copy link

varshith257 commented Apr 25, 2024

/attempt #1055

1 similar comment
@Andrapyre
Copy link

Andrapyre commented May 2, 2024

/attempt #1055

@Andrapyre
Copy link

Andrapyre commented May 3, 2024

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?

Copy link

algora-pbc bot commented May 9, 2024

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

Copy link

algora-pbc bot commented May 16, 2024

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

@Andrapyre Andrapyre linked a pull request May 20, 2024 that will close this issue
Copy link

algora-pbc bot commented May 20, 2024

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

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

Successfully merging a pull request may close this issue.

5 participants