-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Support schema in custom postgres type names, generate PgHasArrayType impls for enums #2791
Support schema in custom postgres type names, generate PgHasArrayType impls for enums #2791
Conversation
Adding a generated impl to the derive is unfortunately a breaking change as it conflicts with any manually added impl. We've learned this the hard way. |
should we add a separate derive then? and should we split off the first commit into a separate pr? what's the best path forward you think? |
This comment was marked as spam.
This comment was marked as spam.
It would be great to have this, maybe the |
@@ -1152,7 +1152,10 @@ impl PartialEq<PgType> for PgType { | |||
true | |||
} else { | |||
// Otherwise, perform a match on the name | |||
self.name().eq_ignore_ascii_case(other.name()) | |||
// If either name has no schema qualification, match only on the name part. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it is not possible with the current info (due to search_path
) to create a fully correct eq method using pg_ type names. How did you decide on this matching?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've previously had to make my own patches for this, and I remember the case-insensitive matching bit us, too.
@@ -84,7 +84,7 @@ async fn it_describes_composite() -> anyhow::Result<()> { | |||
|
|||
let ty = d.columns()[0].type_info(); | |||
|
|||
assert_eq!(ty.name(), "inventory_item"); | |||
assert_eq!(ty.name(), "public.inventory_item"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be tests involving cases where the database will forcibly escape names, like "my::schema"."TypeName"
.
// If either name has no schema qualification, match only on the name part. | ||
// If both names have a schema qualification, match both the schema and name parts. | ||
std::iter::zip(self.name().rsplitn(2, '.'), other.name().rsplitn(2, '.')) | ||
.all(|(part, other_part)| part.eq_ignore_ascii_case(other_part)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PostgreSQL doesn't use case-insensitive matching for escaped names. public."TypeName"
, for example, won't match public.typename
(but public.TYPENAME
, without escaping, will). I suggest we simply treat escaped and non-escaped names on an entirely separate branch, using case-insensitive ASCII comparison for non-quoted names, and using strict utf-8 equality otherwise.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
1 similar comment
This comment was marked as spam.
This comment was marked as spam.
Superceded by #3252 |
Fixes #2595, #2262, and #1576.
replaces #2641