Skip to content
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

sqlx uses time instead of chrono #3412

Open
karuna opened this issue Aug 7, 2024 · 14 comments · May be fixed by #3383
Open

sqlx uses time instead of chrono #3412

karuna opened this issue Aug 7, 2024 · 14 comments · May be fixed by #3383
Labels

Comments

@karuna
Copy link

karuna commented Aug 7, 2024

Bug Description

sqlx 0.8.0

After using 0.8.0, there's an opposite of this problem: #2689

Minimal Reproduction

sqlx = { version = "0.8", features = [
  "postgres",
  "runtime-tokio-rustls",
  "macros",
  "migrate",
  "json",
  "chrono",
  "uuid",
], default-features = false }
CREATE TABLE IF NOT EXISTS "users" (
  "id" uuid PRIMARY KEY DEFAULT uuid7(),
  "created_at" TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT CURRENT_TIMESTAMP
);
#[derive(Clone, Default, sqlx::FromRow)]
pub struct User {
    pub id: Uuid,
    pub created_at: DateTime<Utc>,
}

fn foo(conn: PgPool, user_id: Uuid) {
    let user = query_as!(
        User,
        r#"SELECT id, created_at FROM users WHERE users.id = $1"#,
        &user_id
    )
    .fetch_one(&conn)
    .await?;
}
153 |               let recover = query_as!(User,
    |  ___________________________^
154 | |                 r#"SELECT id, created_at FROM users WHERE users.id = $1"#,
158 | |                 &user_id
159 | |             )
    | |_____________^ the trait `From<OffsetDateTime>` is not implemented for `chrono::DateTime<Utc>`, which is required by `OffsetDateTime: Into<_>`

Info

  • SQLx version: 0.8.0
  • SQLx features enabled: "postgres", "runtime-tokio-rustls", "macros", "migrate", "json", "chrono", "uuid"
  • Database server and version: Postgres 16.3
  • Operating system: Linux
  • rustc --version: 1.80.0
@karuna karuna added the bug label Aug 7, 2024
@CommanderStorm
Copy link
Contributor

CommanderStorm commented Aug 7, 2024

Hi.
I tried to reproduce this, but cannot:
image
image

Could you try producing a minimal reproducible example?
Without this, helping you is basically impossible.

Here is my MRE, assuming that the typo in the sqlx veryion above is a typo (otherwise upgrate do 0.8 and be happy that your issue is fixed ^^)
bin.tar.gz

@CommanderStorm
Copy link
Contributor

Note that I also cannot reproduce this in 0.7...
Idk what you did..

@karuna
Copy link
Author

karuna commented Aug 7, 2024

I guess something else activating time

@karuna
Copy link
Author

karuna commented Aug 7, 2024

Here's my cargo tree output.
I see some depends on time

tree.txt

@karuna
Copy link
Author

karuna commented Aug 7, 2024

Yup, can confirm something is activating time. Here's my mre:
ajep.tar.gz

@abonander
Copy link
Collaborator

This was a breaking change in 0.8.0. It's the first entry in the CHANGELOG: https://github.com/launchbadge/sqlx/blob/main/CHANGELOG.md#breaking

The plan is to make it possible to specify an override in sqlx.toml (#3383, still WIP):

datetime_crate = "chrono"

@karuna
Copy link
Author

karuna commented Aug 8, 2024

@abonander I thought explicitly disabling time on features is enough.
I'll wait for #3383 then.

@abonander
Copy link
Collaborator

It's not just another crate depending on time. It's another crate that depends on sqlx and enables the time feature, due to Cargo's feature unification.

In your case, that appears to be tower-sessions-sqlx-store:

├── tower-sessions-sqlx-store v0.13.0
│   ├── async-trait v0.1.81 (proc-macro) (*)
│   ├── rmp-serde v1.3.0 (*)
│   ├── sqlx v0.8.0 (*)
│   ├── thiserror v1.0.62 (*)
│   ├── time v0.3.36 (*)
│   └── tower-sessions-core v0.12.2 (*)

We can confirm this by looking at their Cargo.toml: https://github.com/maxcountryman/tower-sessions-stores/blob/cecfba06e7ebc031d59d51a5926e19b3bc2c2d0d/sqlx-store/Cargo.toml#L23

I would consider this their bug due to them hardcoding the time feature instead of allowing you to choose.

@abonander
Copy link
Collaborator

In the meantime, you can use type overrides to work around this: https://docs.rs/sqlx/latest/sqlx/macro.query.html#force-a-differentcustom-type

@cemoktra
Copy link
Contributor

In the meantime, you can use type overrides to work around this: https://docs.rs/sqlx/latest/sqlx/macro.query.html#force-a-differentcustom-type

This is pretty annoying. We need to update to 0.8.x due to the advisory in 0.7.x but we have to change hundreds of queries due to that change. I sthere any easier approach?

Or can the advisory be fixed in a 0.7.5?

@abonander
Copy link
Collaborator

I included mitigation advice in the advisory for a reason. If you take reasonable precautions it's unlikely to be exploitable.

For that same reason, I'm not inclined to spend my limited bandwidth backporting the fix. We have no LTS policy for releases and I don't want people to get the impression that we do.

If the danger was more immediate, it'd be a different story, but no other database lib has even bothered to publish an advisory yet, and I know at least Diesel is likely affected as well because they've done their own extensive audit and they're considering backporting the changes.

Otherwise, you could instead find and deal with the dependency that's enabling the time feature unconditionally. There's unlikely to be many candidates.

@jdu
Copy link

jdu commented Aug 29, 2024

In the meantime, you can use type overrides to work around this: https://docs.rs/sqlx/latest/sqlx/macro.query.html#force-a-differentcustom-type

Is there a way to do this on an INSERT/UPDATE, I can't seem to see what the syntax would look like in that case.

@cemoktra
Copy link
Contributor

In the meantime, you can use type overrides to work around this: https://docs.rs/sqlx/latest/sqlx/macro.query.html#force-a-differentcustom-type

Is there a way to do this on an INSERT/UPDATE, I can't seem to see what the syntax would look like in that case.

Usually you just bind your value: value as _

@karuna
Copy link
Author

karuna commented Sep 3, 2024

let's wait for this maxcountryman/tower-sessions-stores#46 to be released

@abonander abonander linked a pull request Sep 20, 2024 that will close this issue
36 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants