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

[PostgreSQL] citext is not compatable with text #295

Closed
izik1 opened this issue May 1, 2020 · 16 comments · Fixed by #2478
Closed

[PostgreSQL] citext is not compatable with text #295

izik1 opened this issue May 1, 2020 · 16 comments · Fixed by #2478

Comments

@izik1
Copy link
Contributor

izik1 commented May 1, 2020

[2020-05-01T19:29:25Z ERROR actix_http::response] Internal Server Error: mismatched types; Rust type `alloc::string::String` (as SQL type TEXT) is not compatible with SQL type 
    Caused by:
        mismatched types; Rust type `alloc::string::String` (as SQL type TEXT) is not compatible with SQL type 

query:

select email, password from "user" where id = $1

Where email is citext. As a workaround, the following cast is required to cause this not to error:

select email::text, password from "user" where id = $1

However, sqlx should be able to handle citext natively.

@abonander
Copy link
Collaborator

abonander commented May 1, 2020

String should support decoding from citext without much change here as it's the same binary and text encoding as regular strings. However, a challenge here is that citext does not have a fixed OID because it's an extension type, so we need to match on the type name instead. This might cause weird things if someone else defines a type with the same name but citext still implies some sort of text compatibility so it has a good chance of just doing the right thing anyway.

@mehcode mehcode mentioned this issue May 31, 2020
10 tasks
@bjeanes
Copy link
Contributor

bjeanes commented Jun 3, 2020

Other adaptors often to query the metadata on initial connection to discover these unstable OIDs.

This will also come up if supporting Postgres DOMAINs and similar. I think this might also be an issue with supporting hstore but my memory is a bit fuzzy on that one. I did work on another pg adapter a few years ago where I was wrestling with this a bit.

@bjeanes
Copy link
Contributor

bjeanes commented Jun 3, 2020

Even though this is for a different driver in a different language, the query and pg_type overview over at will/crystal-pg#88 may be useful.

@hi2u
Copy link

hi2u commented Nov 1, 2020

Is there a workaround that I can do in my code to get CITEXT (single) + CITEXT[] (arrays) columns working? I get an error like:

mismatched types; Rust type `alloc::string::String` (as SQL type `TEXT`) is not compatible with SQL type citext

I've been digging through some github issues... can I use something like PgTypeInfo::with_name("citext"); ? (I'll opt for doing it by name if possible, rather than OID)

Not sure what else I need to do though? How do I tell sqlx that the "citext" type is to be treated the same way as regular "text" ? I'm pretty new to Rust. :)

@izik1
Copy link
Contributor Author

izik1 commented Nov 1, 2020

For sql to Rust, you can cast to text: (<thing>::TEXT), for Rust to sql, you can help it figure out that it wants text and then cast it to citext: $1::TEXT::CITEXT

@hi2u
Copy link

hi2u commented Nov 1, 2020

Is there a way to tell sqlx how to deal with CITEXT globally, without needing to manually cast in all of the actual SQL queries individually?

The PgTypeInfo::with_name() thing seems to be relevant, but I haven't found any example of how to actually do it.

@abonander
Copy link
Collaborator

abonander commented Nov 5, 2020

Honestly I think we can just add a couple checks:

This technically isn't robust if someone adds a type named citext which isn't compatible with text but... does that really matter?

@hi2u
Copy link

hi2u commented Nov 5, 2020

@abonander ...seems sensible to me! :)

I'd be very surprised if there is even a single person out there anywhere who has not only created their own custom type named CITEXT... but also used it for something other than some kind of "text"... let alone also also happens using this package.

And even if they did exist... I don't see why everyone else should miss out on this simple fix due to them doing something pretty silly (and that they could change anyway, they could just use a different type name for example).

And wouldn't sqlx already be broken for them anyway?

But for everyone else using CITEXT in its standard & common way (which in reality I reckon is 100% of them)... this seems like a no brainer.

Otherwise you're dealing with having to look up the OID on every new connection... which is wasteful both in terms of the extra query... but more importantly all the extra effort writing more complex code to develop that feature... which will probably never happen (and really doesn't need to). But even then... wouldn't we just using the name "citext" to convert to an OID anyway? So what's the difference in the end given that sqlx can already go by name without the OID lookup query?

if the type name is present and equals citext[]

Depending on where this library is getting the postgres type name from, it might instead be named _citext for arrays... postgres seems to use an underscore prefix for array types in some places rather than the [] suffix.

#[derive(sqlx::Type, Debug, Serialize)]
#[sqlx(rename = "_citext")]
pub struct CiTextArray(Vec<String>);

...is what worked for me in terms of detecting CITEXT[] array columns, so if the code being changed also gets the type name in the same fashion, then yeah _citext might be the type name you need to detect on.

@abonander
Copy link
Collaborator

We still have to do a lookup because all we get in the responses from the protocol is the type OID, but the connection will automatically resolve and cache unknown type OIDs.

@hi2u
Copy link

hi2u commented Nov 14, 2020

Ah right, fair enough.

But yeah still sounds like a great solution to me. I'd really love to see this solved once and for all, as I'm having to do lots of workarounds and extra codegen right now. It's also less performant doing unnecessary casting in postgres for all inserts/updates/selects.

I'd help out myself, but I'm still very new to Rust.

Also I'm still a bit fuzzy on whether I can actually use PgTypeInfo::with_name("citext"); in my own project to control this or not, in terms of just getting regular String + Vec<String> types in Rust? Or can that only be used to map to custom Rust data types? I'd really like to not have to deal with special custom Rust types if possible, seeing they really just are regular strings getting copied in/out of other regular strings everywhere.

Can I somehow use PgTypeInfo::with_name to map to a standard Rust String? Or do I need to wait until the sqlx crate itself fixes this internally?

Thanks for all your efforts & consideration!

@blazzy
Copy link

blazzy commented Jan 31, 2021

For sql to Rust, you can cast to text: (<thing>::TEXT), for Rust to sql, you can help it figure out that it wants text and then cast it to citext: $1::TEXT::CITEXT

Thanks. That was helpful!

Is there a way to perform the cast without SQLx thinking a non nullable column is now nullable?

@SilentByte
Copy link

Is there a way to perform the cast without SQLx thinking a non nullable column is now nullable?

In case anyone else is looking for an answer, you can force a column to be treated as NOT NULL: https://docs.rs/sqlx/0.5.2/sqlx/macro.query.html#force-not-null

e.g. if column 'email' is of type CITEXT NOT NULL: r#"SELECT email::TEXT as "email!" FROM public.user"#

@autarch
Copy link

autarch commented Dec 4, 2021

Is there any way to fix this that doesn't involve copious hacks right now? I'd really like to just declare the type once and move on. Otherwise it makes using the query_as! and other macros incredibly painful.

Or alternatively, is there a PR I could submit to get this fixed in sqlx directly?

@mfreeborn
Copy link
Contributor

Just came across this myself, too. Is anyone working on this?

@chessai
Copy link

chessai commented Oct 17, 2022

Also interested in this.

@billy1624
Copy link
Contributor

billy1624 commented Dec 14, 2022

Honestly I think we can just add a couple checks:

This technically isn't robust if someone adds a type named citext which isn't compatible with text but... does that really matter?

This sounds like a good idea to me. Is there anyone working on this?

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