Coerce pg types to lowercase untils wrapped in quotes

[zephraph]

Background

I lost a lot of time to this today. This might not be the best way to go about solving it, but some mechanism to inform users what's going on would be good. Essentially I was implementing a custom enum. This is roughly the code I have:

#[derive(SqlType, Debug)]
#[postgres(type_name = "MyEnum")]
pub struct MyEnumType;

#[derive(Clone, Debug, AsExpression, FromSqlRow)]
#[sql_type = "MyEnumType"]
enum MyEnum { ... }

Notice that type_name is PascalCase. Something I didn't recognize (that I should've) is that pg coerces all identifiers to lowercase unless they're explicitly wrapped in quotes. When I created the enum in the database it was via a statement like

CREATE TYPE MyEnum AS ENUM (
    'state1',
    'state2'
);

Notice again that MyEnum is pascal case. Then, when I was trying to make a call to serialize something in the DB I got this error:

Unknown diesel error: SerializationError(FailedToLookupTypeError(PgMetadataCacheKey { schema: None, type_name: \"MyEnum\" }))

Everything looked right! I could not for the life of me figure out what was up. This FailedToLookupTypeError is happening because the key that the PgMetadata struct actually has is all lowercase.

Solution

I've went for the simplest possible solution here. Assuming the type isn't wrapped in quotes (which would cause postgres to preserve the casing) then we to_lowercase() the type name such that it matches the PgMetadataCacheKey. There may be more sophisticated solutions here, but it's a bit beyond the time I have to invest in it. Also, I've obviously only made this change for pg given I'm somewhat familiar with its behavior.

If you're good w/ this level of change I can push it forward further. Perhaps, if we know the casing behavior for MySql and sqlite, we can put fixes in place for those as well.

[weiznich]

Thanks for opening this PR.

Instead of adding hidden behaviour to the derive, I would prefer to add either documentation to this error type stating what could be possibly wrong or add a improved debug/display implementation that points out a potential problem. The problem I see with doing the lowercasing just all the time is that it can lead to really hard to spot bugs for people using quoted identifiers.

[zephraph]

That's fair. I'll think about it a bit and see if I can update the error message. Perhaps on the error path we can do a second lookup for the lowercase version and specifically mention that may be the error if it's found.

[weiznich]

Perhaps on the error path we can do a second lookup for the lowercase version and specifically mention that may be the error if it's found.

That sounds like a good idea. The corresponding place to perform this check is here: https://github.com/diesel-rs/diesel/blob/b2c58897c39c519a946314bd5b63765d3af56204/diesel/src/pg/metadata_lookup.rs#L50 Basically we only need to call lookup_type again with the lower cased type name and see if that query succeeds. Additionally we likely need to adjust the error type here: https://github.com/diesel-rs/diesel/blob/b2c58897c39c519a946314bd5b63765d3af56204/diesel/src/pg/backend.rs#L32 to include this additional information? (And maybe switch that Debug impl to a manual one to have a better control about the emitted error message. )

[zephraph]

Okay @weiznich, it may fail CI, but I made a first pass at updating it to just be an error message. I'm still really new to the rust ecosystem so forgive me if I made any obvious errors. Thanks for the hint about where to make updates, that was helpful.

[zephraph]

I think https://github.com/diesel-rs/diesel/pull/2993 may have ultimately fixed this. Regardless it's been a long time and I don't have the capacity to pick this back up so I'm just going to close. Thanks for your help back then @weiznich! Time flies, ha.