Correctly convert DateTimeOffset to/from database

[markhilb]

Problem

As described in the referenced issue, the way tiberius converts between DateTime<FixedOffset> and DateTimeOffset is incorrect. Currently, tiberius uses the DateTime<FixedOffset>'s local datetime + offset to create the DateTimeOffset. This means that what is stored in the database will be a local datetime and its offset.

However, as described in the docs:

"The data is stored in the database and processed, compared, sorted, and indexed in the server as in UTC. The time zone offset will be preserved in the database for retrieval."

the value should be stored in the database as a UTC datetime + offset.

The consequence of tiberius's current behavior is that retrieving DateTimeOffset values, that have been inserted by tiberius, using any other methods or languages will yield the wrong value. Likewise, using tiberius to retrieve values that have been inserted by other methods or languages will also yield the wrong value.

Changes

• Changes the conversion between DateTime<FixedOffset> and DateTimeOffset to use UTC + offset instead of local + offset.
• Adds a test that demonstrates the problem, which fails without these changes.

Closes https://github.com/prisma/tiberius/issues/260

[pimeys]

Acknowledged, we have an internal discussion about how we need to move forward, maybe meanwhile fix the tests :)

[markhilb]

Not sure what this error is:

---- connect_to_custom_cert_instance_jdbc stdout ----
Error: Tls("error:1416F086:SSL routines:tls_process_server_certificate:certificate verify failed:ssl/statem/statem_clnt.c:1919: (certificate has expired)")
thread 'connect_to_custom_cert_instance_jdbc' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `0`: the test returned a termination value with a non-zero status code (1) which indicates a failure', /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/test/src/lib.rs:187:5

Don't think I have done anything that should cause this

[markhilb]

Yes, this fixes the interop

[pimeys]

Ugh, that certifiacte error :( So the person who did that feature made the cert to expire. That's what we see there now.

[pimeys]

You might want to see if there's an easy way to create a new one. I think there could be some script for that?

[pimeys]

The last question I have here is what does this mean for our current users? If you upgrade with these changes, will the DateTimeOffset values you read from the database be wrong? What kind of upgrade path we'd have here?

[pimeys]

Oh, and is the time feature also broken similarly?

[markhilb]

Since this changes the way tiberius reads and writes DateTimeOffset from the database, any values that have been inserted with the current version will be read differently after these changes. Hence, this has to be a breaking change.

I have not tested whether the time feature has the same bug or not.

[markhilb]

Ok, I tested and the time feature had the same behaviour.

Added a test and fix for that as well.

[pimeys]

Cool. Nice catch. I'm going to communicate this to the management and what is the effect. Might be that we cut a new major releases from your changes, and continue maintaining the current major until we can release the whole Prisma ORM with breaking changes.

So, this means I'm not going to merge this yet, so for you it's better to just use your fork for a few days until I get my writeup done and this issue communicated. Thank you for the PR.

[pimeys]

Sorry to keep this pending. We'll be merging it before the end of January 2023. Luckily this issue doesn't affect Prisma that much due to us always writing in UTC. Still, it's a breaking change for a very small percentile of our operations, meaning we'll cut a major version and that's going to happen in the end of January.

[pimeys]

Closing this for the rebased version, going to merge today: https://github.com/prisma/tiberius/pull/269

[pimeys]

Released in v0.12.0