disneystreaming / smithy4s

https://disneystreaming.github.io/smithy4s/
Other
351 stars 72 forks source link

Timestamp inconsistent JSON representation when using Document #1622

Open msosnicki opened 5 days ago

msosnicki commented 5 days ago

Timestamp with epoch-seconds is inconsistently represented currently when encoding directly to JSON vs using a Document as an intermediate layer.

Reproduction

//> using scala "3.3"

//> using dep "com.disneystreaming.smithy4s::smithy4s-json:0.18.25"
import smithy4s.json.Json
import smithy4s.Document
import smithy4s.ShapeId
import smithy4s.Timestamp
import smithy4s.schema.Schema

case class Foo(seconds: Timestamp)

object Foo {
  given Schema[Foo] = Schema.struct(
    Schema.timestamp.required[Foo]("seconds", _.seconds).addHints(smithy.api.TimestampFormat.EPOCH_SECONDS.widen),
    )(Foo.apply).withId(ShapeId("document", "Foo"))
}

val encoder = Document.Encoder.fromSchema(Schema[Foo])

val foo = Foo(Timestamp(1, 0))

println(Json.writeDocumentAsBlob(encoder.encode(foo)).toUTF8String)

println(Json.writeBlob(foo).toUTF8String)

prints

{"seconds":1.000000000}
{"seconds":1}

The fix should be rather simple, I can create a PR once this issue is acked

kubukoz commented 5 days ago

is this specific to timestamps or just #1579?

msosnicki commented 5 days ago

These two are connected, I think the issue you posted is more like a long term fix and having more ADT members instead of just DNumber will help. And depending on the implementation, the fix here might either stay or be obsolete.

The example which I pasted above does not apply to Long or Int for example, as for those BigDecimal is always created using a different constructor (taking integers) - they go through different codepath, for example Long here

Jsoniter has the issue, where two BigDecimal might be considered equal, but have a different representation in JSON. This issue could be also solved there, and maybe that's where it belongs?

msosnicki commented 5 days ago

To expand on the Jsoniter issue:

//> using scala "3.3"

//> using dep "com.github.plokhotnyuk.jsoniter-scala::jsoniter-scala-core::2.31.3"
//> using compileOnly.dep "com.github.plokhotnyuk.jsoniter-scala::jsoniter-scala-macros::2.31.3"

import com.github.plokhotnyuk.jsoniter_scala.macros._
import com.github.plokhotnyuk.jsoniter_scala.core._
case class User(bigDecimal: BigDecimal)
given userCodec: JsonValueCodec[User] = JsonCodecMaker.make

val one = BigDecimal(1) + (BigDecimal(0.0))
val two = BigDecimal(1)
println(one == two)
println(one)
println(two)

prints

true
1.0
1
plokhotnyuk commented 5 days ago

I don't understand yet how it connects to jsoniter-scala.

Here is the same output for last 5 lines, but without using jsoniter-scala:

$ scala-cli
Welcome to Scala 3.5.2 (21.0.5, Java Java HotSpot(TM) 64-Bit Server VM).
Type in expressions for evaluation. Or try :help.

scala> val one = BigDecimal(1) + (BigDecimal(0.0))
     | val two = BigDecimal(1)
     | println(one == two)
     | println(one)
     | println(two)
true
1.0
1
val one: BigDecimal = 1.0
val two: BigDecimal = 1
msosnicki commented 5 days ago

Depending on if we encode timestamp value to JSON directly or via Document:

The problem can as well be fixed in smithy4s, if I were to make a decision.

But coming back to to jsoniter, it could also be fixed there and encode big decimals consistently (question is, would that be 1.0 or just 1 in the above?). I just find the above example surprising, and while I understand that the toString has the same issue, JSON library and how it encodes data is likely to be more crucial for most cases than weird behavior of toString.

It could as well be that the above is perfectly valid with the JSON specs and common "problem" among JSON libs.

plokhotnyuk commented 4 days ago

The jsoniter-scala serialize BigDecimal to the same textual representation as BigDecimal.toString.

Currently you have an issue of using different scales:

scala> val one = (BigDecimal(1) + (BigDecimal(0.0)))
     | val two = BigDecimal(1)
     | println(s"one.scale=${one.scale}, two.scale=${two.scale}")
one.scale=1, two.scale=0
val one: BigDecimal = 1.0
val two: BigDecimal = 1

To mitigate difference in text representation just round them to the same scale.

msosnicki commented 4 days ago

Makes sense, thank you. If that's the case, similar fix could be applied in the Document codepath: if nano==0, just leverage the scale correctly