Add support for java.time.Instant

[zdanek]

Hi.
I run into this problem in my project. I can support you with relevant info and code, to get this done.
We need this codec as java.time.Instant is modern type for storing datetime when using machine-to-machine communication.

The problem is to choose which store type to use.
Use datetimeoffset in this case is tempting but I'd not use it. The reason is that we're aiming to store Instant as date time in UTC timezone only. So we have fixed TZ. Using datetimeoffset allows to define or put data in different timezone than UTC.

Using datetime2 is the best approach. It does not declare TZ explicitly but here we have only one possible TZ and it's UTC.
The way datetime2 works is taking datetime as it is, with trimming Z that denotes TZ. It's simply omitted. And this is what we need taking java.time.Instant as an input.

@mp911de
I can provide working Codec for this but I need your approval and agreement on this.

[mp911de]

DATETIME2 is a local type. Looking at the TDS Date and Time types, none of the types seems appropriate.

While you could read DATETIMEOFFSET into Instant, writing such a value would lose the offset information. Since there's no temporal type storing a point in time (UTC-based), I do not think that adding an Instant codec for writing is a good idea.

[zdanek]

DATETIME2 is a local type. Looking at the TDS Date and Time types, none of the types seems appropriate.

Sorry but I don't agree with that interpretation. DATETIME2 is a type of Date and Time without TZ explicitly set. Period.
So interpretation is left to the developer. Since Instant has a fixed TZ to UTC that means we could use DATETIME2 and explicitly say (in particular project that uses R2DBC) that it's always fixed to UTC TZ and we interpret this field as such. And it often happens with modern designes.

In fact, using any other mssql datetime type that allows us to set TZ could be a real problem as this can be configured wrong, to to use TZ order thatn UTC. Or we'd have to make a conversion from TZ set in data into UTC, which again is not a driver's responsibiliy. This makes me even more sure that a datetime without TZ storage has to be used.

DATETIME2 is not a local datetime type. It says nothing about TZ. If we assume it local this would be wrong since TZ of DB server can be different than application server that makes SQL call or worst, a few application servers from different TZs if we have a distributed system that uses same DB.

R2DBC nor other drivers are not to decide the designation of the data stored in the columns. Driver is to give an access and a possibility to map DB columns into datatypes present in particular language (in this case Java and Instant). And this gives a freedom for developers to use it.
Driver's responsibility is of course to map it clearly and idempotent so mapping should be done correctly in both ways (in and out).

JDBC allows this so there's some kind of a convention or possiblity. Then moving from JDBC to reactive access would be (almost) like switching some libraries. But not if suddenly R2DBC is not supporting Instant. Other problem arises that it supports Instant depending on DB used, here I refer H2. We've implemented all with Instant in Entities, worked well in test and failed on deploy on real MSSQL DB. The worst thing is that reading data as Instant actually worked. Saving it did not and this was so confusing for me.

Again, I encourage you to discuss it and be open for my reasoning. I propose my input on this and a ready PR. Just making sure that is worth spending time on this.

[mp911de]

I should have been more precise. The discussion here is about whether to represent a moment vs. not-a-moment. By “moment” I mean a specific point on the timeline, that isn't necessarily expressed by a date, but rather the duration since measuring time.

By “not-a-moment” I mean a date with a time-of-day, but lacking the context of a time zone or offset from UTC.

JDBC doesn't specify a mapping for Instant, yet some drivers provide additional mapping strategies to summon an Instant from a temporal type that is associated with a timezone or offset. If you refer to the JDBC 4.3 spec, then you will find mappings for Local[Date|Time] and Offset[Date]Time, Instant isn't part of the JDBC spec.

Driver's responsibility is of course to map it clearly and idempotent so mapping should be done correctly in both ways (in and out).

And that is what we're doing already. We've documented the type mapping. If you want to convert a temporal type to a different one, then your application (or a converter within a Framework) is the right place to manifest your assumptions, not the driver.

That being said, I'm fine adding a reading codec that turns DATETIMEOFFSET values into Instant. We have already a codec for the TDS TIMESTAMP ("rowversion") type.

[zdanek]

Ok. Since seems that I will not convince you - your remarks are also good and valid.

In my current project, as a solution, my colleagues introduced LocalDateTime in place of Instant. That is - in first place I moved to Instant all our Entities, this worked great with H2 and then, after deployment on TEST, it failed with real MSSQL DB. I went on vacations and in meantime, my colleagues reverted to LocalDateTime as a only solution here :-/ Seems like a reasonable (in this case).

But I see lots of damage that can happen if using LocalDateTime in place of Instant that is - whenever we have UTC represented datetime that arrives or being created in higher layers and then suddenly we put it in DB as LocalDateTime.
Interesting is that, they left DATETIME2 type for this. It works. But this involves a middle layer that translates LocalDateTime <-> Instant that we use in Domain and higer (up to DTO and API).

This is my story. Let's wrap it up. MSSQL has no good storate type to "natively" support Instant.
Period.

This is good ground for my second proposal, that I had in my pocket from the start - allow to add custom project Codecs.
Currently there's only DefaultCodecs collection that is fixed and set into Options of r2dbc connection.
My proposal is to add a feature to easily register new codecs. This would include easy way to add them into existing, created by Spring, DB connection. This would be like adding single implementation and/or adding a Codecs impl. This should be easily done by extending / composing / copying already created in DefaultCodecs.
Currently I see lots of limitations put in the code like package-private classes (for example ConnectionOptions), final classes and fixed instantiation of DefaultCodecs in ConnectionOptions.

For me, this is a natural direction of R2DBC library, since you've already introduced nice interfaces like Codec and Codecs. One piece is missing to add an ability for developers to enhance their codecs list. This would be a win-win since R2DBC is a clean solution and a weight of responsibility for vague codecs would be put on developers and their decisions in their project.

@mp911de WDYT? I could add some nice code for this. Daj szansę ;-)

[mp911de]

As of now, our codecs support isn't complete. We're missing support for more complex types such as SQL variant that will likely require changes to our codec registry. Once that is in place, we can evaluate opening up our codec registry.