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

Fix PostgreSQL enum arrays and case-sensitive types #108

Merged
merged 11 commits into from
Jul 21, 2023

Conversation

niklaskorz
Copy link
Contributor

@niklaskorz niklaskorz commented May 30, 2023

PR Info

Bug Fixes

  • udt_name::regtype panics if udt_name contains uppercase letters. This is fixed by wrapping the value of udt_name in double quotes: CONCAT('"', udt_name, '"')::regtype
  • The column type parser always returned unknown for enum types. This is now fixed by creating a correctly filled EnumDef if an enum for that type name is found and then returning a Type::Enum.

Copy link
Member

@billy1624 billy1624 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!! @niklaskorz Great stuffs!

@billy1624 billy1624 requested a review from tyt2y3 June 1, 2023 06:43
@niklaskorz
Copy link
Contributor Author

@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
Copy link
Member

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

@niklaskorz
Copy link
Contributor Author

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

@billy1624
Copy link
Member

Yes, please!

@niklaskorz
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@niklaskorz niklaskorz changed the title Fix PostgreSQL enum arrays Fix PostgreSQL enum arrays and case-sensitive types Jun 2, 2023
@billy1624
Copy link
Member

Thanks!! @niklaskorz

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

@niklaskorz
Copy link
Contributor Author

niklaskorz commented Jun 6, 2023

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
Copy link
Member

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

@niklaskorz
Copy link
Contributor Author

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

@tyt2y3
Copy link
Member

tyt2y3 commented Jul 20, 2023

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
Copy link
Member

tyt2y3 commented Jul 21, 2023

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

@billy1624
Copy link
Member

Cleaning up :P

@billy1624
Copy link
Member

billy1624 commented Jul 21, 2023

@tyt2y3 done

@tyt2y3 tyt2y3 merged commit 50f2383 into SeaQL:master Jul 21, 2023
@github-actions
Copy link

🎉 Released In 0.14.0 🎉

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 ⭐.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants