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

JavaDateTime parsers offset issue #694

Open
sirocchj opened this issue Jul 28, 2022 · 2 comments
Open

JavaDateTime parsers offset issue #694

sirocchj opened this issue Jul 28, 2022 · 2 comments

Comments

@sirocchj
Copy link

Hi,

according to ISO 8601 Time offset from UTC offsets should be expressible, for example, as "+03", "+03:00" and "+0300". However, the last case is not handled correctly, at least on the zio2 branch from which I believe 0.3.0[-RCX] releases are being cut.

I verified it for OffsetDateTime but it is applicable to other *Offset* cases too.

This way all tests pass (notice how the suite was missing an && as well):

diff --git a/zio-json/shared/src/test/scala/zio/json/JavaTimeSpec.scala b/zio-json/shared/src/test/scala/zio/json/JavaTimeSpec.scala
index c0d7ff7..060f2af 100644
--- a/zio-json/shared/src/test/scala/zio/json/JavaTimeSpec.scala
+++ b/zio-json/shared/src/test/scala/zio/json/JavaTimeSpec.scala
@@ -210,8 +210,10 @@ object JavaTimeSpec extends ZIOSpecDefault {
           val n = OffsetDateTime.now()
           val p = OffsetDateTime.of(2020, 1, 1, 12, 36, 12, 0, ZoneOffset.UTC)
           assert(stringify(n).fromJson[OffsetDateTime])(isRight(equalTo(n))) &&
-          assert(stringify("2020-01-01T12:36:12Z").fromJson[OffsetDateTime])(isRight(equalTo(p)))
-          assert(stringify("2020-01-01T12:36:12.Z").fromJson[OffsetDateTime])(isRight(equalTo(p)))
+          assert(stringify("2020-01-01T12:36:12Z").fromJson[OffsetDateTime])(isRight(equalTo(p))) &&
+          assert(stringify("2020-01-01T12:36:12.Z").fromJson[OffsetDateTime])(isRight(equalTo(p))) &&
+          assert(stringify("2020-01-01T12:36:12+00").fromJson[OffsetDateTime])(isRight(equalTo(p))) &&
+          assert(stringify("2020-01-01T12:36:12+00:00").fromJson[OffsetDateTime])(isRight(equalTo(p)))
         },
         test("OffsetTime") {
           val n = OffsetTime.now()

However, adding a positive test for the +0000 case, i.e.:

diff --git a/zio-json/shared/src/test/scala/zio/json/JavaTimeSpec.scala b/zio-json/shared/src/test/scala/zio/json/JavaTimeSpec.scala
index c0d7ff7..5610484 100644
--- a/zio-json/shared/src/test/scala/zio/json/JavaTimeSpec.scala
+++ b/zio-json/shared/src/test/scala/zio/json/JavaTimeSpec.scala
@@ -210,8 +210,11 @@ object JavaTimeSpec extends ZIOSpecDefault {
           val n = OffsetDateTime.now()
           val p = OffsetDateTime.of(2020, 1, 1, 12, 36, 12, 0, ZoneOffset.UTC)
           assert(stringify(n).fromJson[OffsetDateTime])(isRight(equalTo(n))) &&
-          assert(stringify("2020-01-01T12:36:12Z").fromJson[OffsetDateTime])(isRight(equalTo(p)))
-          assert(stringify("2020-01-01T12:36:12.Z").fromJson[OffsetDateTime])(isRight(equalTo(p)))
+          assert(stringify("2020-01-01T12:36:12Z").fromJson[OffsetDateTime])(isRight(equalTo(p))) &&
+          assert(stringify("2020-01-01T12:36:12.Z").fromJson[OffsetDateTime])(isRight(equalTo(p))) &&
+          assert(stringify("2020-01-01T12:36:12+00").fromJson[OffsetDateTime])(isRight(equalTo(p))) &&
+          assert(stringify("2020-01-01T12:36:12+00:00").fromJson[OffsetDateTime])(isRight(equalTo(p))) &&
+          assert(stringify("2020-01-01T12:36:12+0000").fromJson[OffsetDateTime])(isRight(equalTo(p)))
         },
         test("OffsetTime") {
           val n = OffsetTime.now()

fails with:

[info]     - java.time / Decoder / OffsetDateTime
[info]        Either was Left
[info]       stringify("2020-01-01T12:36:12+0000").fromJson[OffsetDateTime] did not satisfy isRight(equalTo(p))
[info]       stringify("2020-01-01T12:36:12+0000").fromJson[OffsetDateTime] = Left(value = "(2020-01-01T12:36:12+0000 is not a valid ISO-8601 format, illegal offset date time at index 23)")
[info]       at /Users/jsirocchi/code/zio-json/zio-json/shared/src/test/scala/zio/json/JavaTimeSpec.scala:217

The culprit looks to be this check, which seems to be commonly used elsewhere, that expect a : to be present to divide between hours and minutes.

By the way, ISO 8601 does not seem to allow second offsets but the parser can handle that too.. is this by design?

@plokhotnyuk
Copy link
Contributor

plokhotnyuk commented Jul 28, 2022

@sirocchj Hi, Julien! Thanks for the feedback!

I've proposed a PR with a fix for missing &&: https://github.com/zio/zio-json/pull/695/files

Currently, zio-json supports only those formats that are supported by OffsetDateTime.parse.

Main tests for them are in RoundTripSpec.

As a workaround you can use a custom codec with a different format:

$ scala
Welcome to Scala 3.1.0-RC1 (17.0.2, Java OpenJDK 64-Bit Server VM).
Type in expressions for evaluation. Or try :help.
                                                                                                                                                                                                                   
scala> import java.time._
                                                                                                                                                                                                                 
scala> import java.time.format._
                                                                                                                                                                                                                   
scala> val str = "2020-01-01T22:36:12+0300"
val str: String = 2020-01-01T22:36:12+0300
                                                                                                                                                                                                                   
scala> OffsetDateTime.parse(str, DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ssZ"))
val res0: java.time.OffsetDateTime = 2020-01-01T22:36:12+03:00

@sirocchj
Copy link
Author

Thanks for the quick turnaround! Yes I have been doing that workaround, I'm hoping we could do better than Java on this one :) (unless ofc the spec is not correct)

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

No branches or pull requests

2 participants