Sitelet https://github.com/npgsql/npgsql/pull/6011
Skip to content

Compare normalized type names - #6011

Merged
NinoFloris merged 1 commit into
npgsql:mainfrom
0MG-DEN:fix6010
Jul 4, 2025
Merged

NinoFloris merged 1 commit into
npgsql:mainfrom
0MG-DEN:fix6010

Conversation

@0MG-DEN

@0MG-DEN 0MG-DEN commented Feb 2, 2025

Copy link
Copy Markdown
Contributor

Fixes #6010

@0MG-DEN
0MG-DEN requested review from roji and vonzshik as code owners February 2, 2025 19:20
@roji

roji commented Feb 2, 2025

Copy link
Copy Markdown
Member

/cc @NinoFloris

@NinoFloris NinoFloris left a comment

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.

Thanks for the PR.

I agree we should fix it, though it's not critical as it can easily be worked around.

Comment thread src/Npgsql/Internal/TypeInfoMapping.cs Outdated
public bool TypeEquals(Type type) => TypeMatchPredicate?.Invoke(type) ?? Type == type;
public bool DataTypeNameEquals(string dataTypeName)
{
var norm = Postgres.DataTypeName.NormalizeName(dataTypeName);

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.

Suggested change
var norm = Postgres.DataTypeName.NormalizeName(dataTypeName);
var normalized = Postgres.DataTypeName.NormalizeName(dataTypeName);

@NinoFloris NinoFloris Feb 4, 2025 •

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.

We can't do the normalization here, as it would very negatively affect startup time and in general mapping resolution time. But it does make sense for the collection lookup to do this.

We should define 2 new methods: private DataTypeNameEqualsCore essentially a rename of the original implementation. Then the current public DataTypeNameEquals(string) can do a normalize and call into core for the collection lookups. Finally we'd also add internal DataTypeNameEquals(DataTypeName) to be used in Find, also calling into core by taking the value of the DataTypeName.

@0MG-DEN 0MG-DEN Feb 14, 2025 •

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.

Thanks for the guidance! I initially added some tracing to check normalization results and indeed there were a lot of calls to it (coming from data source builders' default ConfigureDefaultFactories implementations, IIUC).

@0MG-DEN
0MG-DEN requested a review from NinoFloris February 14, 2025 20:59

@NinoFloris NinoFloris left a comment

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.

Apologies for missing this @0MG-DEN, it all looks good. Thanks!

@NinoFloris
NinoFloris enabled auto-merge (squash) July 3, 2025 16:53
@NinoFloris
NinoFloris merged commit 016ae35 into npgsql:main Jul 4, 2025
NinoFloris pushed a commit that referenced this pull request Jul 4, 2025
Fixes #6010

(cherry picked from commit 016ae35)
@NinoFloris

Copy link
Copy Markdown
Member

Backported to 9.0.4 via 8aa2ce8

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.

Adding numeric converter via alias name throws exception

3 participants