-
-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[draft] Implement To/FromSql for chrono::DateTime with SQLite #2351
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working in this. I've noticed that I should probably a bit clearer in #2331 how I envisioned this feature. I've added the information as comment at the corresponding locations in the diff.
} | ||
|
||
// #[test] | ||
// /// Test that the sql select...where ordering functionality interops properly with rfc3339 datetimes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.