Skip to content

Fix type mapping for non-public types #4586

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.

Sign up for GitHub

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

Merged
vonzshik merged 5 commits into from
Aug 3, 2022
Merged

Fix type mapping for non-public types #4586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5cc8813
Fix type mapping for non-public types
vonzshik Aug 2, 2022
e2236fb
Fix resolving by name
vonzshik Aug 3, 2022
590a049
Add a test
vonzshik Aug 3, 2022
e86eb54
A bit of review cleanup
vonzshik Aug 3, 2022
eb8d5ca
Drop extension at the end of the test
vonzshik Aug 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/Npgsql/PostgresTypes/PostgresType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,6 @@ protected PostgresType(string ns, string name, string internalName, uint oid)

#endregion

internal string SchemaQualifiedName => Namespace is "pg_catalog" or "public" ? Name : FullName;

internal virtual string GetPartialNameWithFacets(int typeModifier) => Name;

/// <summary>
Expand Down
38 changes: 33 additions & 5 deletions src/Npgsql/TypeMapping/ConnectorTypeMapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,19 @@ internal bool TryResolveByOID(uint oid, [NotNullWhen(true)] out NpgsqlTypeHandle

lock (_writeLock)
{
if ((handler = ResolveByDataTypeName(pgType.SchemaQualifiedName, throwOnError: false)) is not null)
if ((handler = ResolveByDataTypeNameCore(pgType.FullName)) is not null)

@roji roji Aug 3, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure we really need this - will a resolver ever know about a specific type in a specific schema? Maybe only some special user-written one, tailored for a specific scenario or something...

@vonzshik vonzshik Aug 3, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We probably don't need this, but it's definitely a nice thing to support just in case (if we do go through with resolver cleanup in 7.0, we'll essentially going to support this anyway, just at resolver level, and not at TypeMapper).

@roji roji Aug 3, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, and there's no perf impact either, so no reason to remove it.

{
_handlersByOID[oid] = handler;
return true;
}

if ((handler = ResolveByDataTypeNameCore(pgType.Name)) is not null)
{
_handlersByOID[oid] = handler;
return true;
}

if ((handler = ResolveComplexTypeByDataTypeName(pgType.FullName, throwOnError: false)) is not null)
{
_handlersByOID[oid] = handler;
return true;
Expand Down Expand Up @@ -169,9 +181,9 @@ internal NpgsqlTypeHandler ResolveByNpgsqlDbType(NpgsqlDbType npgsqlDbType)
}

internal NpgsqlTypeHandler ResolveByDataTypeName(string typeName)
=> ResolveByDataTypeName(typeName, throwOnError: true)!;
=> ResolveByDataTypeNameCore(typeName) ?? ResolveComplexTypeByDataTypeName(typeName, throwOnError: true)!;

NpgsqlTypeHandler? ResolveByDataTypeName(string typeName, bool throwOnError)
NpgsqlTypeHandler? ResolveByDataTypeNameCore(string typeName)
{
if (_handlersByDataTypeName.TryGetValue(typeName, out var handler))
return handler;
Expand All @@ -191,6 +203,14 @@ internal NpgsqlTypeHandler ResolveByDataTypeName(string typeName)
}
}

return null;
}
}

NpgsqlTypeHandler? ResolveComplexTypeByDataTypeName(string typeName, bool throwOnError)
{
lock (_writeLock)
{
if (DatabaseInfo.GetPostgresTypeByName(typeName) is not { } pgType)
throw new NotSupportedException("Could not find PostgreSQL type " + typeName);

Expand Down Expand Up @@ -409,7 +429,11 @@ Type GetUnderlyingType(Type t)
internal bool TryGetMapping(PostgresType pgType, [NotNullWhen(true)] out TypeMappingInfo? mapping)
{
foreach (var resolver in _resolvers)
if ((mapping = resolver.GetMappingByDataTypeName(pgType.SchemaQualifiedName)) is not null)
if ((mapping = resolver.GetMappingByDataTypeName(pgType.FullName)) is not null)
return true;

foreach (var resolver in _resolvers)
if ((mapping = resolver.GetMappingByDataTypeName(pgType.Name)) is not null)
return true;

switch (pgType)
Expand Down Expand Up @@ -663,7 +687,11 @@ public override void Reset()
throw new InvalidOperationException($"Couldn't find PostgreSQL type with OID {oid}");

foreach (var resolver in _resolvers)
if (resolver.GetMappingByDataTypeName(pgType.SchemaQualifiedName) is { } mapping)
if (resolver.GetMappingByDataTypeName(pgType.FullName) is { } mapping)
return (mapping.NpgsqlDbType, pgType);

foreach (var resolver in _resolvers)
if (resolver.GetMappingByDataTypeName(pgType.Name) is { } mapping)
return (mapping.NpgsqlDbType, pgType);

switch (pgType)
Expand Down
26 changes: 26 additions & 0 deletions test/Npgsql.Tests/TypeMapperTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,32 @@ public async Task String_to_citext()
}
}

[Test, IssueLink("https://github.com/npgsql/npgsql/issues/4582")]
[NonParallelizable] // Drops extension
public async Task Type_in_non_default_schema()
{
await using var conn = await OpenConnectionAsync();
try
{
await using var _ = await CreateTempSchema(conn, out var schemaName);

await conn.ExecuteNonQueryAsync($"DROP EXTENSION IF EXISTS citext; CREATE EXTENSION citext SCHEMA \"{schemaName}\"");
conn.ReloadTypes();

await using var __ = await CreateTempTable(conn, $"created_by {schemaName}.citext NOT NULL", out var tableName);

const string expected = "SomeValue";
await conn.ExecuteNonQueryAsync($"INSERT INTO \"{tableName}\" VALUES('{expected}')");

var value = (string?)await conn.ExecuteScalarAsync($"SELECT created_by FROM \"{tableName}\" LIMIT 1");
Assert.That(value, Is.EqualTo(expected));
}
finally
{
await conn.ExecuteNonQueryAsync($"DROP EXTENSION IF EXISTS citext");
}
}

#region Support

class MyInt32TypeHandlerResolverFactory : TypeHandlerResolverFactory
Expand Down