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

Timestamp inconsistent JSON representation when using Document #1622

Open
msosnicki opened this issue Nov 25, 2024 · 7 comments · May be fixed by #1623
Open

Timestamp inconsistent JSON representation when using Document #1622

msosnicki opened this issue Nov 25, 2024 · 7 comments · May be fixed by #1623

Comments

@msosnicki
Copy link
Contributor

msosnicki commented Nov 25, 2024

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

kubukoz commented Nov 25, 2024

is this specific to timestamps or just #1579?

@msosnicki
Copy link
Contributor Author

msosnicki commented Nov 25, 2024

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

msosnicki commented Nov 26, 2024

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

plokhotnyuk commented Nov 26, 2024

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

msosnicki commented Nov 26, 2024

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

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

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

@msosnicki msosnicki linked a pull request Dec 2, 2024 that will close this issue
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants