typelevel / doobie

Functional JDBC layer for Scala.
MIT License
2.16k stars 356 forks source link

BUG: Instances of Meta for Timestamp and Instant. #633

Open diesalbla opened 6 years ago

diesalbla commented 6 years ago

The current implementation of Meta for java.time.Instant, as contributed in #360 by @fiadliel, is based on the conversion to and from java.sql.Timestamp available in the Standard Java Libraries.

This implementation, however, hits into some existing bugs in the Standard Java Libraries, some of them already known, for transforming between Instant and Timestamp, as can be seen in this example.

Welcome to the Ammonite Repl 1.0.2
(Scala 2.12.3 Java 1.8.0_151)
@ import java.sql.Timestamp
@ import java.time.Instant

@Timestamp.from(Instant.parse("+230210361-06-28T16:01:21Z") ) 
 res42: Timestamp = 230210361-06-28 17:01:21.0

@ Instant.parse("-230210361-06-28T16:01:21Z") 
res17: Instant = -230210361-06-28T16:01:21Z

@ Timestamp.from(Instant.parse("-230210361-06-28T16:01:21Z") ) 
 res43: Timestamp = 230205635-08-10 16:01:21.0

As you can see, the conversion of dates long into the past fails.

Even though the problem comes from the Java Standard Library, the doobie library could defensively prevent users from silently falling into this trap and carrying out data corruption. To that end, I would propose to add into the conversion some boundary check, to exclude dates for which the conversion fails.

m4dc4p commented 5 years ago

Just ran into this one myself, where I was passing Instant values to a query of 2017-01-01T00:00:00Z and they got converted to 2016-12-31T00:16:00 because my local computer is 8 hours behind GMT.

natzei commented 5 years ago

A workaround to let doobie store the Instant in UTC would be:

implicit protected val instantMeta: Meta[Instant] =  Meta[Timestamp].imap
  (t => t.toLocalDateTime.atZone(ZoneOffset.UTC).toInstant)
  (i => Timestamp.valueOf(LocalDateTime.ofInstant(i, ZoneOffset.UTC)))
loicknuchel commented 4 years ago

Hi, just ran into the problem. Thanks @natzei for the workaround!

guizmaii commented 4 years ago

Is it fixed now with the last changes done in v0.8.(7|8)?

Not for Postgres apparently because the v0.8.8 changelog says to use the legacy instances: https://github.com/tpolecat/doobie/blob/master/modules/core/src/main/scala/doobie/util/meta/legacymeta.scala#L35

jatcwang commented 3 years ago

Regarding the timezone issues, there are two variables at play that I'm aware of:

If you're using java.time.Instant, TIMESTAMPTZ (TIMESTAMP WITH TIME ZONE) should do the right thing regardless of your JVM timezone, because the JDBC driver should do the correct conversion depending on your DB session's timezone.

If however you're inserting/querying TIMESTAMP into with a (i.e. TIMESTAMP WITHOUT TIMEZONE) you will get the wrong data back.

https://justatheory.com/2012/04/postgres-use-timestamptz/

Here's a test programme using doobie 0.9.2.

import java.time.{Instant, OffsetDateTime, ZoneId}
import java.util.TimeZone

import cats.effect.{Blocker, ExitCode, IO, IOApp}
import doobie.implicits._
import doobie._

object Main extends IOApp {
  override def run(args: List[String]): IO[ExitCode] = {
    val xa = Transactor.fromDriverManager[IO](
      "org.postgresql.Driver", // driver classname
      "jdbc:postgresql:postgres", // connect URL (driver-specific)
      "postgres", // user
      "postgres", // password
      Blocker
        .liftExecutionContext(ExecutionContexts.synchronous), // just for testing
    )

    import doobie.implicits.legacy.instant._
    import doobie.implicits.javatime.JavaOffsetDateTimeMeta
    import doobie.hi._

    val i = Instant.parse("2018-01-01T00:00:00.000Z")

    val zone = ZoneId.systemDefault()
    println(s"JVM timezone: $zone")

    val offsetDt = OffsetDateTime.ofInstant(i, zone)

    (for {
      _ <- fr"""show timezone"""
        .query[String]
        .unique
        .map(z => println(s"DB Connection timezone: $z"))
      _ <- fr"""
          CREATE TEMPORARY TABLE ttt (
            idx INTEGER PRIMARY KEY,
            ts TIMESTAMP NOT NULL,
            tsz TIMESTAMPTZ NOT NULL
          )
          """.update.run
      _ <- fr"""
          INSERT INTO ttt (idx, ts, tsz) VALUES (0, $i, $i)
          """.update.run
      _ <- fr"""
          INSERT INTO ttt (idx, ts, tsz) VALUES (1, $offsetDt, $offsetDt)
             """.update.run
      _ = println("=== values inserted into DB ===")

      // Set the JVM to a different timezone, simulating e.g. a different machine or DST
      _ = {
        println("Setting JVM timezone to EST")
        TimeZone.setDefault(TimeZone.getTimeZone("EST"))
      }

      _ <- fr"""SELECT ts, tsz FROM ttt where idx = 0"""
        .query[(Instant, Instant)]
        .unique
        .map {
          case (ts, tsz) =>
            println("== Instant ==")
            println(s"Original: $i")
            println(s"ts: $ts")
            println(s"tsz: $tsz")
        }

      _ <- fr"""SELECT ts, tsz FROM ttt where idx = 1"""
        .query[(OffsetDateTime, OffsetDateTime)]
        .unique
        .map {
          case (ts, tsz) =>
            println("== OffsetDateTime ==")
            println(s"Original: $offsetDt")
            println(s"ts: $ts") // wrong instant
            println(s"tsz: $tsz") // Note that the "instant" is still correct, but the offset gets set to +0. This is because TIMEZONETZ doesn't actually store timezones
        }

    } yield {
      ExitCode.Success
    }).transact(xa)

  }
}

and the output on my computer:

JVM timezone: Pacific/Auckland
DB Connection timezone: Pacific/Auckland
=== values inserted ===
Setting JVM timezone to EST
== Instant ==
Original: 2018-01-01T00:00:00Z
ts: 2018-01-01T18:00:00Z
tsz: 2018-01-01T00:00:00Z
== OffsetDateTime ==
Original: 2018-01-01T13:00+13:00
ts: 2018-01-01T13:00Z
tsz: 2018-01-01T00:00Z

As you can see, the value of TIMESTAMP is wrong in both cases.

Basically, use TIMESTAMP WITH TIMEZONE if you want to store an actual instant in time.

If you got an example where TIMESTAMP WITH TIMEZONE still breaks, please provide the database you're using and the JDBC driver version