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

[Postgres] RowNotFound when mapping enum to varchar column #237

Closed
jamwaffles opened this issue Apr 7, 2020 · 10 comments
Closed

[Postgres] RowNotFound when mapping enum to varchar column #237

jamwaffles opened this issue Apr 7, 2020 · 10 comments
Labels
bug db:postgres Related to PostgreSQL

Comments

@jamwaffles
Copy link
Contributor

jamwaffles commented Apr 7, 2020

I'm trying to read/write enums to/from a varchar not null column in Postgres. This issue may also exhibit in other databases, but I haven't tested them. As far as I can see from the docs, creating an enum like the following should allow me to use it in a query:

#[derive(
    serde_derive::Deserialize, serde_derive::Serialize, sqlx::Type,
)]
#[sqlx(rename = "TEXT")]
pub enum UserType {
    #[sqlx(rename = "users_normal")]
    Normal,

    #[sqlx(rename = "users_admin")]
    Admin,
}

My insert statement looks like this:

let user: User = sqlx::query_as(
        "insert into users
            (id, name, user_type)
        values
            ($1, $2, $3)
        on conflict (id) do update set
            name = excluded.name,
            user_type = excluded.user_type
        returning *
        ",
    )
    .bind(user_id)
    .bind("Bobby Beans".to_string())
    .bind(UserType::Normal)
    .fetch_one(&postgres)
    .await
    .unwrap();

The record is never created in the database, and the insert panics with this message:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: RowNotFound'

Removing the UserType enum from the insert lets it work as normal, and inserts the record into the database.

I've tried changing #[sqlx(rename = "TEXT")] to #[sqlx(rename = "varchar not null")] to mirror the column type to no avail (see case below).

Is this a bug, or am I not following the docs properly? I found some stuff in CHANGELOG.md about this but it's not very clear.


This is the reduced (as much as possible) test case.

Note that removing all serde stuff doesn't fix the issue, so it's not serde messing with things.

use sqlx::{postgres::PgQueryAs, PgPool};
use uuid::Uuid;

#[derive(
    serde_derive::Deserialize, serde_derive::Serialize, Debug, Copy, Clone, PartialEq, sqlx::Type,
)]
#[sqlx(rename = "TEXT")]
pub enum UserType {
    #[serde(rename = "users_normal")]
    #[sqlx(rename = "users_normal")]
    Normal,

    #[serde(rename = "users_admin")]
    #[sqlx(rename = "users_admin")]
    Admin,
}

#[derive(
    serde_derive::Deserialize, serde_derive::Serialize, Debug, Clone, PartialEq, sqlx::FromRow,
)]
pub struct User {
    id: Uuid,
    name: String,
    user_type: UserType,
}

#[async_std::test]
async fn enum_text_type() {
    let postgres = PgPool::new(&std::env::var("DATABASE_URL").unwrap())
        .await
        .expect("Error creating postgres pool");

    sqlx::query(
        r#"
            create table if not exists users (
                id uuid primary key,
                name varchar not null,
                user_type varchar not null
            );
        "#,
    )
    .execute(&postgres)
    .await
    .expect("Failed to create users table");

    let user_id = Uuid::new_v4();

    let user: User = sqlx::query_as(
        "insert into users
            (id, name, user_type)
        values
            ($1, $2, $3)
        on conflict (id) do update set
            name = excluded.name,
            user_type = excluded.user_type
        returning *
        ",
    )
    .bind(user_id)
    .bind("Bobby Beans".to_string())
    .bind(UserType::Normal)
    .fetch_one(&postgres)
    .await
    .unwrap();

    assert_eq!(
        user,
        User {
            id: user_id,
            name: "Bobby Beans".to_string(),
            user_type: UserType::Normal
        }
    )
}

Cc @Istar-Eldritch who's been helping me fight this issue.

@mehcode
Copy link
Member

mehcode commented Apr 7, 2020

Forgive me but, I'm a bit confused. Your SQL as written in the issue will never return rows in postgres. You would need a RETURNING id, name, user_type clause.

Can you double check that you copied over the right query?

@jamwaffles
Copy link
Contributor Author

Argh you're right! My bad. The real queries do indeed have returning * at the end of them and I've edited my original post to reflect that. The error also occurs with returning (id, name, user_type) (and returning id, name, user_type for completeness' sake) so that unfortunately doesn't help.

@qtbeee
Copy link
Contributor

qtbeee commented Apr 8, 2020

It turns out that when resolving the type name in postgres, the executor uses = instead of ILIKE, and postgres uses lowercase internally. Additionally since the column is a varchar you need to use #[sqlx(rename = "varchar")]

PR incoming to make this more flexible!

@mehcode
Copy link
Member

mehcode commented Apr 8, 2020

Thank you @qtbeee for investigating and pushing a fix.

@mehcode mehcode closed this as completed Apr 8, 2020
@jamwaffles
Copy link
Contributor Author

Thanks for the fix! It works for my code as well. It would be very useful if the list of types was either documented, or better, validated in the macro.

@mehcode
Copy link
Member

mehcode commented Apr 8, 2020

@jamwaffles Can you help me understand what you mean with:

It would be very useful if the list of types was either documented, or better, validated in the macro.

We have basic type documentation here: https://docs.rs/sqlx/0.3.3/sqlx/types/index.html and https://docs.rs/sqlx/0.3.3/sqlx/postgres/types/index.html

I think the primary issue here ( aside from the obvious mistake in the implementation ), is the error message you received.

thread 'main' panicked at 'called Result::unwrap() on an Err value: RowNotFound'

That's awful as it has nothing to do with what actually happened. The RowNotFound is coming from:

let (oid,): (u32,) = query_as(
"
SELECT oid FROM pg_catalog.pg_type WHERE typname ILIKE $1
",
)
.bind(name)
.fetch_one(&mut *self)
.await?;

This should definitely be tweaked to be a .fetch_optional and emitting a much better error in the vein of:

user-defined type named "TEXT" not found

That would have made the bug in the implementation much more obvious.

@mehcode
Copy link
Member

mehcode commented Apr 8, 2020

Added an issue to track improving that error message

@jamwaffles
Copy link
Contributor Author

Can you help me understand what you mean with:

In the proc macro code, it could hold a list of known valid values for the type in #[sqlx(rename = "type")], although the doc pages you linked are a really good resource.

We have basic type documentation here: https://docs.rs/sqlx/0.3.3/sqlx/types/index.html and https://docs.rs/sqlx/0.3.3/sqlx/postgres/types/index.html

I didn't know those pages existed to be honest. The docs for the Type derive aren't written yet which left me reading the changelog, stabbing in the dark and opening this issue haha. A short paragraph on the proc macro linking to the types that should be used would be really useful, particularly with #239 merged.

I think the primary issue here ( aside from the obvious mistake in the implementation ), is the error message you received.

Yeah, agreed! We followed quite a few false starts tracking the root cause down :)

@mehcode
Copy link
Member

mehcode commented Apr 8, 2020

In the proc macro code, it could hold a list of known valid values for the type in #[sqlx(rename = "type")], although the doc pages you linked are a really good resource.

I believe the normal idea is you're supposed to just use the type of the SQL column.

Note that the original intention of the feature is more to allow this:

CREATE TYPE mood AS ENUM ('sad', 'ok', 'happy');
#[derive(sqlx::Type)]
#[sqlx(rename = "mood", rename_all = "lowercase")]
enum Mood { Sad, Ok, Happy }

So there really isn't a list of "known valid values". Supporting simple string matching
was more of an after thought of "hey we could do this? why not."


The docs for the Type derive [...]

Yeah.. that's definitely on us. We're trying to build up the documentation as we go but it's definitely not in an amazing place right now.

That's a good point on linking to the types module from the Type documentation.


All in all, I'm glad the fix is working for you and sorry for the troubles. We'll get a patch release out shortly.

@jamwaffles
Copy link
Contributor Author

Thank you for explaining the intent of this feature. It makes a lot more sense when used in the context of custom types for sure.

As a fellow open source maintainer I appreciate that docs are time consuming and sometimes annoying lol. It looks like this crate is also moving pretty quickly so keeping docs up to date would be even more of a burden I imagine.

All in all, I'm glad the fix is working for you and sorry for the troubles.

No apology required, and thank you for your time working through this with me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug db:postgres Related to PostgreSQL
Projects
None yet
Development

No branches or pull requests

3 participants