[draft] Implement To/FromSql for chrono::DateTime with SQLite #2351
Conversation
| } | ||
|
|
||
| // #[test] | ||
| // /// Test that the sql select...where ordering functionality interops properly with rfc3339 datetimes. |
There was a problem hiding this comment.
Those two tests should just "work" if we use Timestamptz as sql type. Making them work with Text as sql type is basically impossible.
| @@ -103,12 +103,31 @@ impl ToSql<Timestamp, Sqlite> for NaiveDateTime { | |||
| } | |||
| } | |||
|
|
|||
| // chrono::DateTime<FixedOffset> impls | |||
|
|
|||
| impl FromSql<Text, Sqlite> for DateTime<FixedOffset> { | |||
There was a problem hiding this comment.
I should have made that probably clearer in #2331, but we definitively don't want to have literally impl FromSql<Text, Sqlite> DateTime<FixedOffset> here, because that would:
- Remove any type safety
- Interact poorly with the strongly typed query builder
So to clarify that a bit more, I think a better approach to handle this is to use a distinct SQL type for this. I'm not 100% sold to reusing postgres Timestamptz type here as I'm not sure if the semantics are similar, but that could be a solution. Alternatively we could introduce a new sql type. So the strategy should be basically similar to that one above used to implement support for NaiveDateTime.
Other things I've noticed while thinking about this:
- We are using here some guessing for the actual timestamp format. It's probably a good idea to do that here as well with all formats that support timezone information. So we can be compatible with all sqlite supported formats and not only rfc3339 style timestamps.
- We probably want to support
DateTime<TZ: Timezone>instead of onlyDateTime<FixedOffset>, similar to the postgres variant.
This PR is not complete, see discussion from #2331 regarding SelectDsl trait implementation.