SeaQL / sea-schema

🌿 SQL schema definition and discovery
https://docs.rs/sea-schema
Apache License 2.0
192 stars 40 forks source link

Fix PostgreSQL enum arrays and case-sensitive types #108

Closed niklaskorz closed 1 year ago

niklaskorz commented 1 year ago

PR Info

Bug Fixes

niklaskorz commented 1 year ago

@billy1624 udt_name::regtype unfortunately results in a runtime panic for arrays of enums and thus can't be added back. There is an alternative function that returns NULL instead of panicking, I'll have a look what it was called.

billy1624 commented 1 year ago

Thanks!! @niklaskorz Is there a way to add a test cases for it?

niklaskorz commented 1 year ago

I'll look into that, tests/discovery/postgres would be the right place for this?

billy1624 commented 1 year ago

Yes, please!

niklaskorz commented 1 year ago

Still trying to pinpoint what exactly causes the issue on PG's side... I do have a schema where I can reproduce the issue, but I'm not yet able to reproduce this in a smaller isolated schema.

niklaskorz commented 1 year ago

Interesting, the problem only occurs with types that have upper case letters in their names. For these, ::regtype panics but the workaround introduced in this PR works.

niklaskorz commented 1 year ago

Going down this rabbit hole, I found the actual cause of the issue and the JOIN-heavy workaround becomes unnecessary. :) Working on the according commit right now, but basically the value of udt_name has to be wrapped in double quotes to be able to deal with case-sensitive type names, i.e. CONCAT('"', udt_name, '"')::regtype.

billy1624 commented 1 year ago

Thanks!! @niklaskorz

One question. Why not Expr::cust(r#""udt_name"::regtype"#)?

niklaskorz commented 1 year ago

Thanks!! @niklaskorz

One question. Why not Expr::cust(r#""udt_name"::regtype"#)?

"udt_name"::regtype is for case sensitive access of udt_name (which has no effect here as it does not contain any uppercase letters). A value such as FooBar (an example of a custom user type / enum) will be returned unchanged.

CONCAT('"', udt_name, '"') gets the value inside udt_name and wraps it in quotation marks. A value such as FooBar will become "FooBar".

Now, if we run ::regtype on this value, regtype will first look up the type corresponding to the value it is invoked on. Here, Postgres treats the type name as case insensitive if it is not wrapped in quotation marks. The first case of "udt_name"::regtype will result in a lookup of foobar as the value is treated as case insensitive. The type foobar does not exist and the lookup will panic. In the second case, "FooBar" is treated as case sensitive by ::regtype and the type will be found.

billy1624 commented 1 year ago

Ahhhh... I see what you mean now! Thanks for the explanation!

niklaskorz commented 1 year ago

@billy1624 @tyt2y3 Just saw that 0.12 was released without this... any chance we can get this and https://github.com/SeaQL/sea-orm/pull/1678 in soon?

tyt2y3 commented 1 year ago

The current release is for SQLx 0.6 and the next SeaORM will depend on SQLx 0.7, so we need another SeaSchema release for that. I'll see whether we can get this in the next release.

tyt2y3 commented 1 year ago

@billy1624 as discussed, we will merge this after a slight cleanup.

billy1624 commented 1 year ago

Cleaning up :P

billy1624 commented 1 year ago

@tyt2y3 done

github-actions[bot] commented 1 year ago

:tada: Released In 0.14.0 :tada:

Thank you everyone for the contribution! This feature is now available in the latest release. Now is a good time to upgrade! Your participation is what makes us unique; your adoption is what drives us forward. You can support SeaQL 🌊 by starring our repos, sharing our libraries and becoming a sponsor ⭐.