Skip to content

Conversation

@davidhunt135
Copy link
Contributor

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 FullName rather than DisplayName but that included pg_catalog when looking up those types which would cause an exception because they are stored in the lookup without the schema.

Copy link
Contributor

@vonzshik vonzshik left a 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; }
}

@davidhunt135 davidhunt135 force-pushed the 4365-include-schema-in-type-lookup branch from ce38652 to 560b89c Compare April 8, 2022 09:13
@davidhunt135 davidhunt135 requested a review from vonzshik April 18, 2022 14:37
Copy link
Member

@roji roji left a 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.

@roji
Copy link
Member

roji commented Apr 19, 2022

@davidhunt135 the PR is failing various tests related to extension-defined types, can you please take a look?

@Brar
Copy link
Member

Brar commented Apr 19, 2022

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.

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 regproc which automagically solved all escaping and search_path issues.

I don't know whether this applies here but sometimes a cast to regclass is all you need to solve this kind of issue.
See: https://www.postgresql.org/docs/current/datatype-oid.html

@davidhunt135
Copy link
Contributor Author

@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 ConnectorTypeMapper._handlersByDataTypeName (in the same way as the pg_catalog schema, hence the use of DisplayName instead of FullName).

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?

@davidhunt135
Copy link
Contributor Author

I have pushed one solution to verify it does resolve the issue.

@davidhunt135 davidhunt135 force-pushed the 4365-include-schema-in-type-lookup branch 2 times, most recently from a4002d3 to 560b89c Compare April 19, 2022 15:25
@davidhunt135
Copy link
Contributor Author

@roji @vonzshik Could someone approve running of the workflows so I can verify tests have been fixed?

@roji
Copy link
Member

roji commented Apr 25, 2022

@davidhunt135 done, I'll try to reply to #4382 (comment) soon.

@davidhunt135
Copy link
Contributor Author

@roji No problem. Managed to miss a change in a merge conflict if you could run again.

@davidhunt135
Copy link
Contributor Author

@roji @vonzshik any update on this?

Copy link
Contributor

@vonzshik vonzshik left a 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...

@davidhunt135
Copy link
Contributor Author

@vonzshik @roji I've made the suggested changes

Copy link
Member

@roji roji left a 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;
Copy link
Member

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

Choose a reason for hiding this comment

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

Suggested change
public string SchemaQualifiedName => QualifiedNameExcludedSchemas.Contains(Namespace, StringComparer.Ordinal) ? Name : FullName;
public string SchemaQualifiedName => Namespace is "pg_catalog" or "public" ? Name : FullName;

@roji roji enabled auto-merge (squash) May 24, 2022 13:09
@roji roji changed the title Use DisplayName instead of Name when doing type lookup Fix type resolution logic May 24, 2022
@roji roji merged commit d3f14f5 into npgsql:main May 24, 2022
roji pushed a commit that referenced this pull request May 24, 2022
Fixes #4365

(cherry picked from commit d3f14f5)
@roji
Copy link
Member

roji commented May 24, 2022

Backported to 6.0.5 via bccbec6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Composite types in more than one schema

4 participants