jiff-sqlx: add integration crate for SQLx

[BurntSushi]

This PR adds a new jiff-sqlx crate. It defines wrapper types for
Timestamp, DateTime, Date, Time and Span. For each wrapper
type, the SQLx encoding traits are implemented. (Except, with Span,
only the decoding trait is implemented.)

This is similar to #141, but organizes things a bit differently. This
also comes with SQLite support. MySQL support is missing since it seems,
at present, to require exposing APIs in SQLx for a correct
implementation.

This initial implementation also omits Zoned entirely. I've left a
comment in the source code explaining why. The quick summary is that, at
least for PostgreSQL, I don't see a way to provide support for it
without either silently losing data (the time zone) or just storing it
as an RFC 9557 timestamp in a TEXT field. The downside of the latter
is that it doesn't use PostgreSQL native datetime types. (Becuase we
can't. Because PostgreSQL doesn't support storing anything other than
civil time and timestamps with respect to its datetime types.) I do
personally lean toward just using RFC 9557 as a TEXT type, but I'd
like to collect real use cases first to make sure that's the right way
to go.

Ref #50, Closes #141
Ref launchbadge/sqlx#3487

[BurntSushi]

I'm going to bring this in, but if anyone wants to offer feedback post-merge, I'm happy to follow-up!

[bionicles]

Just got this working, it did / does work well enough to get it rolling, I had to convert a Zoned to a Timestamp which was a bummer, one thing i've done in the past is use a separate "tz" column to hold the time zone names

one area of friction was the sqlx::FromRow derive macro for a big struct, in order for structs to lend references to Timestamps, the field values can't be jiff_sqlx::Timestamp, they need to be
jiff::Timestamp

so one writes the jiff type as a struct field type, however the jiff::Timestamp type isn't sqlx::Type / sqlx::Decode, so one winds up needing a custom impl<'r, R: Row> sqlx::FromRow<'r, R> for the struct, can't derive it, which is more convenient; perhaps if the main jiff::Timestamp could conditionally derive or implement sqlx::Type, then derive(FromRow) could work

not sure it's worth making jiff types into sqlx types directly, maybe worth a test to demonstrate the proper technique, though?

Thank you for making jiff! I love it!

/// an example struct for jiff_sqlx integration
#[derive(Debug)] // FromRow fails here
pub struct Tournament {
    // ... other fields ...
    /// timestamp at which the tournament was recorded
    finished_at: Timestamp,
}

impl Tournament {
    /// goal: borrow timestamp at which this tournament was recorded
    pub fn finished_at(&self) -> &Timestamp {
        &self.timestamp
    }
}

impl<'r> sqlx::FromRow<'r, PgRow> for Tournament
{
    fn from_row(row: &'r PgRow) -> Result<Self, sqlx::Error> {
        Ok(Tournament {
            // ... other fields ...
            finished_at: row
            .try_get::<jiff_sqlx::Timestamp, &'static str>("finished_at")?
                    // ^^^^^^^^^^^^^^^^^^^^ get the column value as a jiff_sqlx type
            .to_jiff(),
          // ^^^^^^^ convert the jiff_sqlx type into a jiff type
        })
    }
}

[BurntSushi]

Thank you for the feedback! I have some follow-ups if you don't mind. :-)

Just got this working, it did / does work well enough to get it rolling, I had to convert a Zoned to a Timestamp which was a bummer, one thing i've done in the past is use a separate "tz" column to hold the time zone names

Yeah I figured folks would run into this. So now that I have you here, what would you like jiff_sqlx to do? If it uses PostgreSQL's TIMESTAMP WITH TIME ZONE type, then the time zone on the Zoned value will get silently dropped. If it instead uses TEXT and encodes it as an RFC 9557 timestamp, then you retain all your data, but you don't use a PostgreSQL native time data type.

one area of friction was the sqlx::FromRow derive macro for a big struct, in order for structs to lend references to Timestamps, the field values can't be jiff_sqlx::Timestamp, they need to be
jiff::Timestamp

Can this be fixed by providing a method on jiff_sqlx::Timestamp that returns &jiff::Timestamp? Maybe implementing AsRef?

perhaps if the main jiff::Timestamp could conditionally derive or implement sqlx::Type, then derive(FromRow) could work

This defeats the purpose of having wrapper types. :-) Is isn't really appropriate for Jiff to depend on SQLx. The reverse is more plausible, but that's up to the SQLx maintainers.

Why doesn't this work with the wrapper types? Can we add impls to jiff-sqlx to make this work?

Note that I am a SQLx newb. What I did in jiff-sqlx was just strictly based on what is in SQLx itself for chrono and time. But maybe I missed something.

[bionicles]

it makes sense to have jiff not depend on sqlx, that's a great point

I'm not an expert on AsRef, but that sounds reasonable, maybe an "as_jiff" borrowing method would work, then we could put sqlx_jiff::Timestamp on structs directly and borrow the inner value without needing to make a full conversion, good idea

maybe for zoned (since postgres apparently doesn't know what a time zone is 🤣) there would be two options

0. Could the crate have a jiff_sqlx::Zoned wrapper type which implements sqlx::Type and wraps a string type? https://docs.rs/sqlx/latest/sqlx/trait.Type.html

1. Could the crate have a helper method to use CREATE TYPE to make a custom zoned datetime postgres type? (more research, work, finagling, not for sqlite, but might be able to store the inner jiff::Zoned more natively)

2. Could we contact postgresql maintainers and request a timestamp with time zone update for Temporal / RFC 9557? (depends on externals, might take indefinitely long, but could be satisfying and help a wider audience)

Seems like 0 would work for now, 1 is too annoying, and 2 could be nice to have but requires other people to make a big change to a big software system, I'm not gonna hold my breath for that!

[BurntSushi]

I think I still don't understand why you need to be able to get a &jiff::Timestamp. Is there an example program using SQLx and Jiff that you can share with me that doesn't work today? Basically, think about it from my perspective: I want to write a regression test ensuring that whatever thing you're trying to do works and continues to work.

Could the crate have a jiff_sqlx::Zoned wrapper type which implements sqlx::Type and wraps a string type?

That's what I meant by using TEXT above. The question is not really whether we could do this but whether we should. It might be very surprising or undesirable to folks.

Could the crate have a helper method to use CREATE TYPE to make a custom zoned datetime postgres type? (more research, work, finagling, not for sqlite, but might be able to store the inner jiff::Zoned more natively)

I think this is definitely outside of the scope of jiff_sqlx. It's really just meant to be a bridge that makes using Jiff with SQLx easier. But getting into database specific shenanigans is pretty well outside its scope and is entering design territory whose burden I don't really want to invite.

Could we contact postgresql maintainers and request a timestamp with time zone update for Temporal / RFC 9557? (depends on externals, might take indefinitely long, but could be satisfying and help a wider audience)

I imagine you could. But that's definitely not something I have the time or patience for. Someone else will need to lead that charge I think.

[tonyfinn]

I attempted to migrate one of my projects from chrono to jiff using the jiff-sqlx crate, but it appears that jiff-sqlx is not compatible with the query! and query_as! macros which are used for static type checking of SQL queries (not to be confused with the query and query_as functions).

I've created a minimal reproducer here: https://github.com/tonyfinn/jiff-sqlx-poc/tree/main

but the gist is that if you don't enable any of the other time crates then a crate using sqlx fails to build as it there is no From<()> implementation for the jiff_sqlx types, and tells you you need to enable the time feature to use the TIMESTAMPTZ type (example error output) and if you do enable the other time crates then it complains that e.g. time::OffsetDateTime and jiff_sqlx::Timestamp are not the same type.

(Let me know if you want me to file this as a seperate issue, just keeping it with the existing sqlx discussion as its newly implemented)

I suspect this will require changes inside sqlx to make it work

[BurntSushi]

Hi @tonyfinn, it's much better to open new issues for stuff like that. I've created one for you at #272

[abonander]

@BurntSushi I just noticed this while working on my own example for launchbadge/sqlx. These crates are semver-exempt, it's not recommended to depend on them directly without pinning the version: https://docs.rs/sqlx-core/latest/sqlx_core/#note-semver-exempt-api

Is there a particular reason you didn't just use the sqlx crate? I don't see any relevant discussion in #50 or #141 (which actually did use sqlx). I don't see any private types being used here.

[BurntSushi]

Hmmm I honestly don't remember. I think I was just trying to use the minimal number of dependencies as I could and this "worked" as far as I could tell.

Honestly the whole process of adding an integration crate was quite difficult from the perspective of someone who doesn't actually use SQLx. There was a lot of guesswork involved and "okay the chrono integration does things this way so I'll do it that way too" style of reasoning.

I did not see the note about semver being an exempt. I created #322 to track fixing this. Thank you for flagging this!

[abonander]

Honestly the whole process of adding an integration crate was quite difficult from the perspective of someone who doesn't actually use SQLx. There was a lot of guesswork involved and "okay the chrono integration does things this way so I'll do it that way too" style of reasoning.

Well honestly, no one's ever really bothered before. They usually just insist on upstreaming it into SQLx, even when, in retrospect, it doesn't make a whole lot of sense (supporting the whole git2 crate for just one type was a huge headache because of the native code dependency).

This wouldn't be a problem if jiff wasn't so young still. time and chrono have been on their current major versions for years now. When jiff is at that point, or you're ready to release a 1.0, I'd be happy to talk about upstreaming.

[BurntSushi]

Yeah I plan to release 1.0 this summer and commit to it indefinitely.

And yeah I added the ecosystem crates because I perceived it as one the main reasons why folks were bouncing of Jiff.

It's a nasty chicken and egg problem.