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

Can't decode PostgreSQL arrays of custom types #1672

Open
Thomasdezeeuw opened this issue Feb 3, 2022 · 13 comments · May be fixed by #3603
Open

Can't decode PostgreSQL arrays of custom types #1672

Thomasdezeeuw opened this issue Feb 3, 2022 · 13 comments · May be fixed by #3603

Comments

@Thomasdezeeuw
Copy link
Contributor

Thomasdezeeuw commented Feb 3, 2022

I'm trying to decode an array of custom (PosgreSQL) types inside a record, using PgRecordDecoder. It hits the panic found here:

unreachable!("(bug) use of unresolved type declaration [oid={}]", oid);

It has an oid which matches the array version of my custom enum (e.g. _mytype for the code below).

I think the following should reproduce it.

CREATE TYPE MyType AS ENUM ('val1', 'val2');

SELECT 'abc', ARRAY['val1', 'val1']::MyType[];
struct S {
    text: String,
    array: Vec<E>,
}

#[derive(Copy, Clone, Debug, Eq, PartialEq, sqlx::Type)]
#[sqlx(rename_all = "snake_case", type_name = "mytype")]
enum E {
    Val1,
    Val2,
}

impl sqlx::postgres::PgHasArrayType for ValueBinding {
    fn array_type_info() -> sqlx::postgres::PgTypeInfo {
        // Array type name is the name of the element type prefixed with `_`.
        sqlx::postgres::PgTypeInfo::with_name("_mytype")
    }
}


impl S {
    fn try_decode<'r>(decoder: &mut PgRecordDecoder<'r>) -> Result<S, Box<dyn std::error::Error + Send + Sync + 'static>> {
        let text = decoder.try_decode::<String>()?;
        let array = decoder.try_decode::<Vec<ValueBinding>>()?;
        Ok(S { text, array })
    }
}
@Thomasdezeeuw
Copy link
Contributor Author

Any update on this?

@Thomasdezeeuw
Copy link
Contributor Author

Thomasdezeeuw commented Feb 16, 2022

Anyone who could point into the correct direction on how to fix this? We really need this feature.

@abonander
Copy link
Collaborator

Try adding type_name = "MyType" to the #[sqlx] attribute on enum E

@Thomasdezeeuw
Copy link
Contributor Author

Thank you for helping @abonander. I added the attribute, but it didn't help unfortunately. I looked up the oid and it's _mytype, so I implemented sqlx::postgres::PgHasArrayType for the Rust enum E, but that didn't work. I also tried a custom type instead of the Vec<E>, like

#[derive(sqlx::Type)]
#[sqlx(type_name = "_pair")]
struct Pairs(Vec<Pair>);
, but no luck either.

@Thomasdezeeuw Thomasdezeeuw changed the title can't decode PostgreSQL arrays of custom types Can't decode PostgreSQL arrays of custom types Feb 17, 2022
@teenjuna
Copy link

teenjuna commented Mar 19, 2022

@abonander I guess I have the same issue. I described it on the forum: https://users.rust-lang.org/t/how-should-one-use-sqlx-with-newtype-pattern/73172. Is it a bug or a feature?

@demurgos
Copy link
Contributor

Hi,
I submitted support for arrays of custom types a few months ago, this issue is definitely a bug. Sorry. Arrays of custom types should be supported.

I've hit this bug in my own code last week and started working on a patch. I believe that the best solution to fix it is to refactor how SQLx handles types. Even if refactoring type resolution does not land immediately, I think it should help locating the source of the bug.

I will do my best to provide a fix in a timely manner.

@demurgos
Copy link
Contributor

demurgos commented May 4, 2022

A quick update on this bug. Over the last weeks, I was working on refactoring Postgres types. The linked PR is pretty exploratory and will need a lot of work before it can be in a state where it could get merged, however it was pretty helpful: it turned this runtime panic into a compilation error.

The essence of the issue is that custom types must be resolved from the DB, and then decoders must get access to their metadata. However, decoders can't pass arbitrary state and context gets lost. When context gets lost, the decoders fallback to using PgTypeInfo::try_from_oid which only supports a handful of builtin types. This happens here in record.rs. (this also occurs in tuples, and a couple other decoders in text mode).

Using try_from_oid is always a bug inside a decoder: on custom types, this returns a type references (PgType::{DeclareByOid, DeclareByName}) and breaks the invariant that decoders only use resolved types (thus the panic).

A quick solution would be to maintain context in a thread local or other similar ambient store, similarly to how async runtime or logger sinks are passed down implicitly. I prefer serde's style of using explicit deserializers, but this a larger change.

I'll continue work on my experimental branch to fix the issue, but full support for the Postgres type system (including custom types) will need deeper changes to SQLx. This means that the fix may take a long time before it's merged. In the meantime, I can at least replace panics with regular errors.

demurgos added a commit to demurgos/sqlx that referenced this issue May 4, 2022
Postgres arrays and records do not fully support custom types. When encountering an unknown OID, they currently default to using `PgTypeInfo::with_oid`. This is invalid as it breaks the invariant that decoding only uses resolved types, leading to panics.

This commit returns an error instead of panicking. This is merely a mitigation: a proper fix would actually add full support for custom Postgres types. Full support involves more work, so it may still be useful to fix this immediate issue.

Related issues:
- launchbadge#1672
- launchbadge#1797
demurgos added a commit to demurgos/sqlx that referenced this issue May 5, 2022
Postgres arrays and records do not fully support custom types. When encountering an unknown OID, they currently default to using `PgTypeInfo::with_oid`. This is invalid as it breaks the invariant that decoding only uses resolved types, leading to panics.

This commit returns an error instead of panicking. This is merely a mitigation: a proper fix would actually add full support for custom Postgres types. Full support involves more work, so it may still be useful to fix this immediate issue.

Related issues:
- launchbadge#1672
- launchbadge#1797
@demurgos
Copy link
Contributor

demurgos commented May 16, 2022

I have a good news: I have a branch where I am able to decode anonymous records and arrays of custom types.

The bad news is that it requires quite a few changes. I'll write a message going more into details but here are the main points:

  • Anonymous records are hard to handle because they act as barriers to type resolution: you don't know the types they contain until you start decoding them.
  • Values need access to the cache of fetched types while we decode them, so we can resolve values in anonymous records synchronously.
  • There needs to be a way to declare that we want to prefetch some types, so they are always present before we access any anonymous record.
  • Splitting the type representation between unresolved and resolved types is really helpful, but it brings some changes to the API

My plan is to fix Clippy issues / make sure CI passes, and then discuss how to implement the various changes required to support this feature.

abonander pushed a commit that referenced this issue May 31, 2022
Postgres arrays and records do not fully support custom types. When encountering an unknown OID, they currently default to using `PgTypeInfo::with_oid`. This is invalid as it breaks the invariant that decoding only uses resolved types, leading to panics.

This commit returns an error instead of panicking. This is merely a mitigation: a proper fix would actually add full support for custom Postgres types. Full support involves more work, so it may still be useful to fix this immediate issue.

Related issues:
- #1672
- #1797
@djc
Copy link
Contributor

djc commented Jun 9, 2022

@demurgos is this fixed with #1483?

@rex-remind101
Copy link

@demurgos

Anonymous records are hard to handle because they act as barriers to type resolution: you don't know the types they contain until you start decoding them.

I could really use this. I don't care that the failure will occur at decode time, having some way have mapping anonymous records and arrays of anon records is better than no way.

My first attempt at this was similar to the following:

#[derive(Clone, Debug)]
pub struct Data {
    pub sub_data: Vec<SubData>,
    pub some_bool: bool,
}

// This just has scalar fields so derive `FromRow` works
// but this type _does not_ exist in the database, it's just for 
// data returned by a query I'm running.
#[derive(Clone, Debug, sqlx::Decode, sqlx::FromRow)]
pub struct SubData { ...stuff... }

impl<'r> sqlx::FromRow<'r, PgRow> for Data {
    fn from_row(row: &'r PgRow) -> Result<Self, sqlx::Error> {
        // The column with array of records in my returned data is named `sub_data`.
        let sub_data: Vec<SubData> = row
            .try_get::<Vec<PgRow>, _>("sub_data")?
            .into_iter()
            .map(|sub_data_tuple| SubData::from_row(&sub_data_tuple))
            .collect();
        Ok(Data {
            sub_data,
            some_bool: row.try_get("some_bool")?,
        })
    }
}

but I end up with

the trait bound `for<'a> PgRow: sqlx::Decode<'a, Postgres>` is not satisfied

It seems like if at least PgRow implemented sqlx::Decode then we'd have some flexible, albeit not-pretty, way around this. Is that change easy to make?

@frederikhors
Copy link

@rex-remind101 did you find a way?

@qrilka
Copy link

qrilka commented Jan 4, 2024

Looks like for arrays to work one needs a newtype and implement Type and Decode for it as seen in https://github.com/launchbadge/sqlx/pull/1483/files#diff-227edfb063a81fad99246f78753bb75d1d2262b95efc8c2115fe30081fd97c3bR1154-R1170

@nico-incubiq
Copy link

I just hit that error too with sqlx: v0.8.2, it was not obvious what to do as it's a runtime error with no details.

internal error: entered unreachable code: (bug) use of unresolved type declaration [oid]

This was my code

#[derive(Debug, sqlx::Type)]
#[sqlx(type_name = "http_response")]
struct HttpResponseRecord {
    headers: Vec<HeaderPairRecord>,
}

#[derive(Debug, sqlx::Type)]
#[sqlx(type_name = "header_pair")]
struct HeaderPairRecord {
    name: String,
    value: String,
}

[...]

sqlx::query!(
    r#"
    INSERT INTO responses (response, created_at)
    VALUES ($1, NOW())
    "#,
    &http_response_record as _,
)
.execute(connection)
.await?;

with this table definition:

CREATE TYPE header_pair AS (
    name TEXT,
    value TEXT
);

CREATE TYPE http_response AS (
    headers header_pair[],
);

CREATE TABLE responses (
    response http_response NOT NULL,
    created_at timestamptz NOT NULL
);

Using cargo-expand I noticed the PgHasArrayType implementation for HeaderPairRecord derived for Type is:

#[automatically_derived]
impl ::sqlx::postgres::PgHasArrayType for HeaderPairRecord {
    fn array_type_info() -> ::sqlx::postgres::PgTypeInfo {
        ::sqlx::postgres::PgTypeInfo::array_of("header_pair")
    }
}

which, according to
https://github.com/launchbadge/sqlx/blame/82d332f4b487440b4c2bd5d54a5f17dcc1abc92c/sqlx-postgres/src/type_info.rs#L274-L287
generates a type info prefixed with an underscore _.

But in my case, I was able to resolve the issue by disabling that derived PgHasArrayType implementation and implementing it myself to generate type info suffixed with square brackets []:

#[derive(Debug, sqlx::Type)]
#[sqlx(type_name = "header_pair", no_pg_array)]
struct HeaderPairRecord {
    name: String,
    value: String,
}

impl PgHasArrayType for HeaderPairRecord {
    fn array_type_info() -> PgTypeInfo {
        PgTypeInfo::with_name("header_pair[]")
    }
}

I was surprised this worked as there are tickets and comments on this repo pointing to Postgres automatically prefixing custom type arrays with _, eg. #3204 and #1004 (comment) and https://github.com/launchbadge/sqlx/blame/82d332f4b487440b4c2bd5d54a5f17dcc1abc92c/sqlx-postgres/src/type_info.rs#L274-L287, but still it seems sqlx wants a different semantic (suffix with []).

It seems implementing #3003 would work in my case, but this seems contrary to all those links above 🤔 .

t-edzuka added a commit to t-edzuka/zero2prod that referenced this issue Dec 6, 2024
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.

9 participants