-
Notifications
You must be signed in to change notification settings - Fork 877
Fix type resolution logic #4382
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 type resolution logic #4382
Conversation
vonzshik
left a comment
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.
Could you please add a test for this fix? It should be in CompositeTests file and look something like this:
[Test, IssueLink("https://github.com/npgsql/npgsql/issues/4365")]
public async Task In_different_schemas_same_type_with_child()
{
var csb = new NpgsqlConnectionStringBuilder(ConnectionString)
{
ApplicationName = nameof(In_different_schemas_same_type_with_child), // Prevent backend type caching in TypeHandlerRegistry
Pooling = false
};
await using var conn = await OpenConnectionAsync(csb);
await using var _ = await CreateTempSchema(conn, out var firstSchemaName);
await using var __ = await CreateTempSchema(conn, out var secondSchemaName);
await conn.ExecuteNonQueryAsync($"CREATE TYPE {firstSchemaName}.child_composite AS (name text)");
await conn.ExecuteNonQueryAsync($"CREATE TYPE {firstSchemaName}.composite AS (id int, child {firstSchemaName}.child_composite)");
await conn.ExecuteNonQueryAsync($"CREATE TYPE {secondSchemaName}.child_composite AS (name text)");
await conn.ExecuteNonQueryAsync($"CREATE TYPE {secondSchemaName}.composite AS (id int, child {secondSchemaName}.child_composite)");
await conn.ExecuteNonQueryAsync(
@$"
CREATE TABLE {firstSchemaName}.composite_table (mycomposite {firstSchemaName}.composite);
INSERT INTO {firstSchemaName}.composite_table VALUES(ROW(1, ROW('1')));
CREATE TABLE {secondSchemaName}.composite_table (mycomposite {secondSchemaName}.composite);
INSERT INTO {secondSchemaName}.composite_table VALUES(ROW(2, ROW('2')));
");
conn.ReloadTypes();
conn.TypeMapper
.MapComposite<ChildComposite>($"{firstSchemaName}.child_composite")
.MapComposite<ParentComposite>($"{firstSchemaName}.composite")
.MapComposite<ChildComposite>($"{secondSchemaName}.child_composite")
.MapComposite<ParentComposite>($"{secondSchemaName}.composite");
await using var cmd = conn.CreateCommand();
cmd.CommandText = $"SELECT * FROM {firstSchemaName}.composite_table; SELECT * FROM {secondSchemaName}.composite_table";
await using var reader = await cmd.ExecuteReaderAsync();
Assert.IsTrue(await reader.ReadAsync());
Assert.That(reader.GetDataTypeName(0), Is.EqualTo($"{firstSchemaName}.composite"));
var firstParentComposite = reader.GetFieldValue<ParentComposite>(0);
Assert.That(firstParentComposite.ID, Is.EqualTo(1));
Assert.NotNull(firstParentComposite.Child);
Assert.That(firstParentComposite.Child!.Name, Is.EqualTo("1"));
Assert.IsTrue(await reader.NextResultAsync());
Assert.IsTrue(await reader.ReadAsync());
Assert.That(reader.GetDataTypeName(0), Is.EqualTo($"{secondSchemaName}.composite"));
var secondParentComposite = reader.GetFieldValue<ParentComposite>(0);
Assert.That(secondParentComposite.ID, Is.EqualTo(2));
Assert.NotNull(secondParentComposite.Child);
Assert.That(secondParentComposite.Child!.Name, Is.EqualTo("2"));
}
class ParentComposite
{
public int ID { get; set; }
public ChildComposite? Child { get; set; }
}
class ChildComposite
{
public string? Name { get; set; }
}ce38652 to
560b89c
Compare
roji
left a comment
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.
This looks OK to me as a specific fix here.
The idea behind DisplayName was only to be used for displaying the type name to the user; ideally, in internal lookups we'd always use the fully-quailfied name rather than sometimes the fully-qualified and sometimes not.
On that note, having the fully-qualified name in a single string raises some questions - I suspect we don't handle escaping correctly, and things would probably break for a type with a dot in its name.
So ideally, our key would be a struct record with the name and schema separate (and non-nullable) - but that's probably a more extensive change than we'd like to patch... So @vonzshik if you're OK with it, we can merge this for 6.0 and open another issue to track cleaning up more properly for 7.0.
|
@davidhunt135 the PR is failing various tests related to extension-defined types, can you please take a look? |
I had this problem a long time ago when working on DeriveParameters for functions and found the elegant fix of simply casting the string to I don't know whether this applies here but sometimes a cast to |
|
@roji @vonzshik Looks like the problem is that DisplayName includes the 'public.' prefix for types in the public schema and it seems these types use just the type name without the schema for the key in the internal dictionary Without a big change I think the most straight forward solution would be to add a new property to use rather than DisplayName or FullName and that property would remove both pg_catalog and public schema prefix. Alternatively, DisplayName could also remove the public schema prefix but I would suggest that is harder to verify. What are your thoughts? |
|
I have pushed one solution to verify it does resolve the issue. |
a4002d3 to
560b89c
Compare
|
@davidhunt135 done, I'll try to reply to #4382 (comment) soon. |
|
@roji No problem. Managed to miss a change in a merge conflict if you could run again. |
vonzshik
left a comment
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.
All in all, looks good to me, except a few things. Maybe @roji has something different in mind...
roji
left a comment
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.
Thanks @davidhunt135 and sorry for all the reviewing delays...
As I wrote above, I think we need a more serious cleanup for 7.0 - opened #4481 to track that.
| /// A schema qualified name for this backend type, includes the namespace unless it is pg_catalog (the namespace | ||
| /// for all built-in types) or public. | ||
| /// </summary> | ||
| public string SchemaQualifiedName => QualifiedNameExcludedSchemas.Contains(Namespace, StringComparer.Ordinal) ? Name : FullName; |
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 don't think this should be exposed publicly... Even if we need it for our own internal purposes, we already have DisplayName for not showing pg_catalog... Users probably don't need another SchemaQualifiedName which show public (not to mention the naming, which doesn't really make any of this clear).
| /// A schema qualified name for this backend type, includes the namespace unless it is pg_catalog (the namespace | ||
| /// for all built-in types) or public. | ||
| /// </summary> | ||
| public string SchemaQualifiedName => QualifiedNameExcludedSchemas.Contains(Namespace, StringComparer.Ordinal) ? Name : FullName; |
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.
| public string SchemaQualifiedName => QualifiedNameExcludedSchemas.Contains(Namespace, StringComparer.Ordinal) ? Name : FullName; | |
| public string SchemaQualifiedName => Namespace is "pg_catalog" or "public" ? Name : FullName; |
|
Backported to 6.0.5 via bccbec6 |
Fixes #4365
This resolved the issues for me and I have been working with that version quite extensively so I think the fix covers all issues.
My initial approach was to use
FullNamerather thanDisplayNamebut that includedpg_catalogwhen looking up those types which would cause an exception because they are stored in the lookup without the schema.