Skip to content

Implementation of sqlx::Decode is not general enough #1031

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

Closed
ajosecueto opened this issue Feb 4, 2021 · 27 comments
Closed

Implementation of sqlx::Decode is not general enough #1031

ajosecueto opened this issue Feb 4, 2021 · 27 comments

Comments

@ajosecueto
Copy link

ajosecueto commented Feb 4, 2021

The error is generated when i'm using a struct with sqlx::Type and some field is an Option. i tried to decode manually my struct but the error is still there.

Sample sql, structs and query

`sql
create table demo_seller
(
id int not null constraint demo_seller_pk primary key
username varchar(30) not null,
first_name varchar(30) not null,
last_name varchar(30) not null,
photo varchar(150)
);

create table demo_shop
(
shop_id int not null
constraint demo_shop_pk
primary key,
created_at timestamptz default now() not null,
price numeric(15,3) not null,
seller_id int not null
);
`

`rust
#[derive(Serialize, Deserialize, FromRow, sqlx::Type)]
pub struct Seller {
id: i32,
username: String,
first_name: String,
last_name: String,
photo: Option< String >,
}

#[derive(Serialize, Deserialize, FromRow)]
pub struct Shop{
shop_id: i32,
price: Decimal,
seller: Seller
}

let shops: Vec = sqlx::query_as!(Shop, r#"
SELECT
s.shop_id,
s.price,
(sl.id, sl.username, sl.first_name, sl.last_name, sl.photo) as "seller!:Seller"
FROM demo_shop s inner join demo_seller sl on s.seller_id = sl.id
"#).fetch_all(&*pool).await?;
`

//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
Implementation of sqlx::Decode is not general enough

0 |
/ pub trait Decode<'r, DB: Database>: Sized {
61 | | /// Decode a new value of this type using a raw value from the database.
62 | | fn decode(value: <DB as HasValueRef<'r>>::ValueRef) -> Result<Self, BoxDynError>;
63 | | }
| |_- trait sqlx::Decode defined here
|
= note: std::option::Option<std::string::String> must implement sqlx::Decode<'0, Postgres>, for any lifetime '0...
= note: ...but std::option::Option<std::string::String> actually implements sqlx::Decode<'1, Postgres>, for some specific lifetime '1

@ajosecueto
Copy link
Author

struct with options fields cant use #[derive(sqlx::Type)]

Implementation of sqlx::Decode is not general enough

@jplatte
Copy link
Contributor

jplatte commented Feb 4, 2021

This might be fixed on master and/or a newer version of Rust (which one are you using?) as I can't reproduce it, though there's another problem relating to the sqlx::Type on master, I'll create a PR for that shortly..

@jplatte jplatte mentioned this issue Feb 4, 2021
@ajosecueto
Copy link
Author

This might be fixed on master and/or a newer version of Rust (which one are you using?) as I can't reproduce it, though there's another problem relating to the sqlx::Type on master, I'll create a PR for that shortly..

rustc 1.49.0 (e1884a8e3 2020-12-29)

@jplatte
Copy link
Contributor

jplatte commented Feb 4, 2021

Can you try whether this still happens with 0.5.1?

@Kruptein
Copy link

Kruptein commented Feb 5, 2021

I have the same issue with Option on 0.5.1 (on rustc 1.51.0-nightly (d1aed50ab 2021-01-26))

@jplatte
Copy link
Contributor

jplatte commented Feb 5, 2021

For some really weird reason, this happens with a String field and an Option<String> field, but not with an i32 field and an Option<String> field. Minimal repro:

#[derive(sqlx::Type)]
pub struct Seller {
    username: String,
    photo: Option<String>,
}

@jplatte
Copy link
Contributor

jplatte commented Feb 5, 2021

Cleaned up the macro output to reduce this to

use std::error::Error;

use sqlx::{decode::Decode, postgres::PgValueRef, types::Type, Postgres};

pub struct Seller {
    //username: String,
    photo: Option<String>,
}

impl<'r> Decode<'r, Postgres> for Seller
where
    String: Decode<'r, Postgres>,
    String: Type<Postgres>,
    Option<String>: Decode<'r, Postgres>,
    Option<String>: Type<Postgres>,
{
    fn decode(value: PgValueRef<'r>) -> Result<Self, Box<dyn Error + 'static + Send + Sync>> {
        let mut decoder = sqlx::postgres::types::PgRecordDecoder::new(value)?;
        //let username = decoder.try_decode::<String>()?;
        let photo = decoder.try_decode::<Option<String>>()?;
        Ok(Seller {
            // username
            photo,
        })
    }
}

Removing the String: Decode<'r, Postgres> bound fixed the issue.

I'm guessing this is probably a bug in rustc.

@jplatte
Copy link
Contributor

jplatte commented Feb 5, 2021

It also doesn't make sense to me that SQLx would emit these bounds though. Generally, these should be emitted for generic parameters, not for field types. That theoretically limits use cases but works around this issue, is what other derives do (to avoid making internal field types part of possibly public API) and should not be an issue in practice.

EDIT: Actually it does make sense for Decode impls that are generic over the database. That's not the case here, but often it is.

@Kruptein
Copy link

Kruptein commented Feb 5, 2021

should I as a workaround write my own Decode impl based on what you have here above (removing the String: Decode stuff) or is there some easier workaround ?

@jplatte
Copy link
Contributor

jplatte commented Feb 5, 2021

@Kruptein You would have to write your own Decode, Encode and Type implementations. You can use cargo-expand to obtain the output of the macro invocation on your type and customize that. For now yes, that's the only workaround. I'll probably also create a PR working around this in the coming days (and also create an issue on rustc).

@Kruptein
Copy link

Kruptein commented Feb 5, 2021

Ok cool, I'll see if I can figure it out and otherwise I'll have to be patient :)

@Kruptein
Copy link

Kruptein commented Feb 5, 2021

cargo-expand is really a great help indeed, the first two structs I implemented manually indeed work after removing the String: Decode<'r, Postgres> line.
I do also have a struct with an Option that has a similar issue, where removing the Decode for the i32 fixes it.

@jplatte
Copy link
Contributor

jplatte commented Feb 5, 2021

In general you shouldn't need any of these bounds on concrete types. That's only going to be necessary for generic parameters (when deriving / implementing Decode for a generic type).

@Kruptein
Copy link

Kruptein commented Feb 5, 2021

Yeah I just wanted to let you know because you couldn't reproduce it with i32's (#1031 (comment))

@jplatte
Copy link
Contributor

jplatte commented Feb 8, 2021

Okay, actually I don't think I'm gonna write the PR for this. My suggested fix is making these two loops: [encode, decode] work over the type's generic type parameter rather than the field types. That's at least theoretically a breaking change, but only for generic structs that derive sqlx::Type, of which there are probably very few, if any, out there. A similar change should probably be done for the other cases where split_for_impl is used, simply because currently the derives leak the types of potentially private fields into the public API (this is why derive(Clone) for a generic type bounds on Clone for all generic type params which results in overly strict bounds sometimes, e.g. when a T is only used as Arc<T>).

EDIT: I'll open a PR for #[automatically_derived] though, as I've already done that.

@jplatte
Copy link
Contributor

jplatte commented Feb 17, 2021

Finally was able to reduce this further and posted a rust issue: rust-lang/rust#82219

@victorteokw
Copy link

Hi,
With decimal feature included, I get this

the trait `sqlx::Decode<'_, sqlx::Any>` is not implemented for `rust_decimal::Decimal`

bendoerry added a commit to bendoerry/zero2prod that referenced this issue Nov 30, 2022
We are using a postgres composite type for our headers.
We can't do the same for the response as a whole due to a bug in sqlx.
See launchbadge/sqlx#1031
benluelo added a commit to benluelo/sqlx that referenced this issue Feb 1, 2023
benluelo added a commit to benluelo/sqlx that referenced this issue Feb 1, 2023
benluelo added a commit to benluelo/sqlx that referenced this issue Mar 10, 2023
@acomanescu
Copy link

Any news on this matter? Should I use SeaORM instead of manually trying to decode the value into a struct?

@guilhermewerner
Copy link

In my case I need a generic trait to use with sea-query, and I get this error:

error[E0277]: the trait bound `u64: sqlx::Decode<'_, sqlx::Any>` is not satisfied
  --> src\models\oauth2.rs:9:16
   |
9  | impl Model for OAuth2Client {
   |                ^^^^^^^^^^^^ the trait `sqlx::Decode<'_, sqlx::Any>` is not implemented for `u64`
   |
   = help: the trait `sqlx::Decode<'_, sqlx::MySql>` is implemented for `u64`
note: required for `OAuth2Client` to implement `for<'r> FromRow<'r, AnyRow>`
  --> src\models\oauth2.rs:13:48
   |
13 | #[derive(Clone, Debug, Serialize, Deserialize, FromRow)]
   |                                                ^^^^^^^ unsatisfied trait bound introduced in this `derive` macro
14 | pub struct OAuth2Client {
   |            ^^^^^^^^^^^^
note: required by a bound in `Model`
  --> C:\Projects\Services\src\core\model.rs:11:5
   |
10 | pub trait Model:
   |           ----- required by a bound in this trait
11 |     for<'r> FromRow<'r, AnyRow> + Send + Sync + Unpin + Serialize + DeserializeOwned
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `Model`
   = note: this error originates in the derive macro `FromRow` (in Nightly builds, run with -Z macro-backtrace for more info)
pub trait Model:
    for<'r> FromRow<'r, AnyRow> + Send + Sync + Unpin + Serialize + DeserializeOwned
{
    const TABLE: &'static str;

    fn table_ref() -> TableRef {
        TableRef::Table(StaticIden(Self::TABLE).into_iden())
    }
}

impl Model for OAuth2Client {
    const TABLE: &'static str = "oauth_client";
}

#[derive(Clone, Debug, Serialize, Deserialize, FromRow)]
pub struct OAuth2Client {
    pub id: u64,
    pub name: String,
    #[serde(rename = "type")]
    #[sqlx(rename = "type")]
    pub kind: OAuth2ClientType,
    pub organization_id: u32,
    pub trusted: bool,
    #[serde(skip_serializing_if = "Option::is_none")]
    pub package_id: Option<u32>,
    pub website_url: String,
    pub photo_url: String,
    pub background_url: Option<String>,
    #[serde(skip)]
    pub secret: String,
    pub custom_schema: Option<String>,
    pub redirects: Option<Value>,
    pub scopes: Option<String>,
    pub created: NaiveDateTime,
    pub updated: Option<NaiveDateTime>,
}

#[repr(u8)]
#[derive(Debug, Copy, Clone, PartialEq, Eq, Serialize, Deserialize, Type)]
#[serde(rename_all = "snake_case")]
pub enum OAuth2ClientType {
    Web = 0,
    Native = 1,
}

@frantisek-heca
Copy link

frantisek-heca commented Mar 19, 2024

Please, what is the conclusion about the "Implementation of sqlx::Decode is not general enough" problem?

Is the conclusion: "one cannot derive sqlx::Type over a struct with an Option field"?

@victorteokw
Copy link

Hi @frantisek-heca, finally we don't use sqlx anyway. Have a look at Teo. This web framework has builtin ORM support. Write CRUD and aggregates are very fast with it. The framework handles the encoding and decoding for you.

@acomanescu
Copy link

Hi @frantisek-heca, finally we don't use sqlx anyway. Have a look at Teo. This web framework has builtin ORM support. Write CRUD and aggregates are very fast with it. The framework handles the encoding and decoding for you.

How is this relevant?

benluelo added a commit to benluelo/sqlx that referenced this issue Mar 22, 2024
@uonr
Copy link

uonr commented Apr 28, 2024

When I used the stable version, I ran into this problem with the Option type, but it worked fine once I workaround it.

However when testing with the nightly version of CI, I found this error on all #[derive(sqlx::Type)].

@henningcullin
Copy link

When I used the stable version, I ran into this problem with the Option type, but it worked fine once I workaround it.

However when testing with the nightly version of CI, I found this error on all #[derive(sqlx::Type)].

What was your workaround for it?

@uonr
Copy link

uonr commented Apr 28, 2024

When I used the stable version, I ran into this problem with the Option type, but it worked fine once I workaround it.
However when testing with the nightly version of CI, I found this error on all #[derive(sqlx::Type)].

What was your workaround for it?

@henningcullin

Just macro expansion and delete the where clause.

@henningcullin
Copy link

@uonr I got my query_as! implementation working with nested structs that use the derived Type macro. It said the error was on an Option enum which was the same error as the op had. But when I used the query! macro to view the resulting record it became clear that issue was not the Type macro and the Option enum. The issue was that I didn't use Option on everything that could be nullable in the query.

@abonander
Copy link
Collaborator

Closed by #2940

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants