Split TypeMapperResolver from TypeHandlerResolver #4973

vonzshik · 2023-03-04T12:45:53Z

The main idea is that now we have TypeMapperResolver, which can be created from TypeHandlerResolverFactory. Now, whenever we initialize a TypeMapper (on datasource bootstrapping) we create a local TypeMapperResolver, which we add to the TypeMapper and a global TypeMapperResolver, which we try to add to GlobalTypeMapper. This allows us to add global mappings for types user opts into automatically.
While most of the time both local and global TypeMapperResolver are the same, we have to differentiate between them because some mapper resolvers can have state (e.g. json serialization settings), which make them unfit to automatically add to GlobalTypeMapper. But there is an exception to that rule, that is if a user adds TypeHandlerResolverFactory to GlobalTypeMapper explicitly, in which case we have to create local TypeMapperResolver and use it in GlobalTypeMapper.

~~This change allows us to move range type mapping from BuiltInResolver, which saves 120kb on linux (and about 300kb on windows, no idea why such a difference).~~
upd: saves on linux about 340kb

NinoFloris

Split looks good and will help me with refactoring the TypeHandlerResolver/TypeHandlers.

As discussed offline there is still quite a bit of unnecessary complication and duplication in the mapping api's and its code but that's not directly the goal to fix in this PR either. (Same goes for the beautiful GetDataTypeNameByValueDependentValue)

This reverts commit c52730d.

roji · 2023-03-20T10:04:47Z

src/Npgsql/Internal/TypeMapping/TypeMapper.cs

+
+            var globalMapperResolver = resolverFactories[i].CreateGlobalMapperResolver();
+            if (globalMapperResolver is not null)
+                GlobalTypeMapper.Instance.TryAddMapperResolver(globalMapperResolver);


Doesn't this mean that if I add some plugin to my specific data source, we change the global type mapper? If so, this is new behavior that I'm not sure we want - the global type mapper config is supposed to flow to the data source, but not vice versa, no? At least up to now, the idea was that you only see mappings on the global type mapper for plugins which you add to it.

See general review comment on where we want to go with NpgsqlParameter type inference.

Doesn't this mean that if I add some plugin to my specific data source, we change the global type mapper? If so, this is new behavior that I'm not sure we want - the global type mapper config is supposed to flow to the data source, but not vice versa, no? At least up to now, the idea was that you only see mappings on the global type mapper for plugins which you add to it.

Yesn't
The main problem are ranges (again). In 7.0 we were able to map on them via GlobalTypeMapper automatically (as it was done in BuiltInTypeResolver and TypeMapper). Currently, that's not going to work as (almost) all of that logic was moved to a separate resolver. And we don't really want to add mapping resolver to GlobalTypeMapper because it references NpgsqlRange, which brings a lot of code + trimming warning.

I think that if we need to, it's not the end of the world to not have ranges in the global type mapper. We already don't take into account data source mappings, so it's not like it's a full-featured thing.

Sure, though the more things we'll move in separate resolvers, the less we'll be able to map from global type mapper.

Good point, though at least in some other cases having a type mapping resolver rooted globally maybe isn't so bad, since the types it references aren't problematic...

So you think it's better to just have the data source mappings impact the global type mapper, with the assumption that there's only one data source in the typical app anyway?

So you think it's better to just have the data source mappings impact the global type mapper, with the assumption that there's only one data source in the typical app anyway?

That pretty much depends on whether we're going to support global type mapper. I personally don't really like this design (having to differentiate between a mapper for local and a mapper for global type mappers, adding a specific mapper to global automatically), but if we're going to support global type mapper (at least for the foreseeable future), we might as well merge this and refactor later (removing the code shouldn't be that hard).
And my assumption was that users aren't going to use multiple data sources with different mappings (e.g. one has ranges, one doesn't, that's probably only going to be true for custom types, which we don't map anyway).

src/Npgsql/Internal/TypeMapping/TypeMapper.cs

src/Npgsql/Internal/TypeMapping/TypeMapperResolver.cs

roji · 2023-03-20T10:30:55Z

Assuming I haven't missed anything, the global-level mapper (mapping) resolvers are there to support NpgsqlParameter type inference, whereas the TypeMapper ones are there to supportNpgsqlSchema.GetDataTypes.

The global support - for NpgsqlParmeter type inference - is becoming somewhat less relevant since we have data source; doing type mapping on the global type mapper is now discouraged/deprecated. NpgsqlParameter type inference is also problematic because it doesn't take into account data source-level mappings. So maybe we should thinkg about where we want to go with all this...

This PR makes data source mappings visible also on the global type mapper. While this allows NpgsqlParameter inference to reflect global type mapping changes, if you have two data sources with differing configurations, they interfere.
We could "give up" on NpgsqlParameter's type inference, and just expose some basic static type mappings that don't take into account anything. This may seem bad, but remember that users are expected to switch to data source, where plugins aren't (currently) taken into account anyway. In the future we may introduce a better type inference API on the data source (briefly discussed this possibility with @NinoFloris).
Or we can improve NpgsqlParameter type inference, and have it be aware of its data source (via references...) so that it can return accurate results based on the data source's mapping (I don't really want to do that).
Or we could keep the current model, where plugins register on global type mapper (deprecated API!) are taken into account on NpgsqlParameter and that's it. It may be needless complexity for supporting an API which we've obsoleted.

/cc @NinoFloris

vonzshik · 2023-03-20T10:54:51Z

This PR makes data source mappings visible also on the global type mapper. While this allows NpgsqlParameter inference to reflect global type mapping changes, if you have two data sources with differing configurations, they interfere.

That's the reason for having both CreateMapperResolver and CreateGlobalMapperResolver in TypeHandlerResolverFactory separately: if there are settings (like json serialization), we only implement CreateMapperResolver, and CreateGlobalMapperResolver will always return null.

roji · 2023-03-20T14:19:24Z

Sure, though the point is that we "leak" mappings from the data source to the global. For example, say i have two data sources, one with ranges and one without; parameters will always report that range is supported, even though using them on one of the data sources will actually fail.

Maybe this isn't so bad, I dunno...

roji

I thought about it some more and I guess I'm OK with it... The global type mapper will mean something like "some data source in the application has this mapping", without any guarantee that this mapping is actually valid everywhere. To be completely accurate, there wasn't even that guarantee before, i.e. a data source created before a global plugin registration wouldn't have it, of course.

Maybe just take a look at the naming suggestion...

vonzshik marked this pull request as ready for review March 4, 2023 13:19

vonzshik requested a review from roji as a code owner March 4, 2023 13:19

vonzshik force-pushed the 4972-split-TypeMapperResolver-from-TypeHandlerResolver branch 3 times, most recently from 1b4c6a2 to 428875f Compare March 10, 2023 14:18

Split TypeMapperResolver from TypeHandlerResolver

311566a

vonzshik force-pushed the 4972-split-TypeMapperResolver-from-TypeHandlerResolver branch from 428875f to 311566a Compare March 15, 2023 11:47

Make TypeMappingInfo.NpgsqlDbType non-nullable

c52730d

NinoFloris approved these changes Mar 15, 2023

View reviewed changes

vonzshik added 3 commits March 15, 2023 23:57

Revert "Make TypeMappingInfo.NpgsqlDbType non-nullable"

24b3b71

This reverts commit c52730d.

Fix a few usages of GetPostgresTypeByName

ba9aba6

Fix test

36f9d85

vonzshik force-pushed the 4972-split-TypeMapperResolver-from-TypeHandlerResolver branch from 0f3a7f2 to 36f9d85 Compare March 15, 2023 21:37

roji reviewed Mar 20, 2023

View reviewed changes

vonzshik added 2 commits March 20, 2023 18:58

Rename resolvers

069c365

Remove TypeMapperResolverExtensions

f1402a8

roji approved these changes Mar 20, 2023

View reviewed changes

vonzshik added 2 commits March 20, 2023 19:10

Rename MapperResolver

0fb3230

Remove more ranges from BuiltInTypeMappingResolver

12f61d1

vonzshik enabled auto-merge (squash) March 20, 2023 16:23

vonzshik merged commit c9b8bce into main Mar 20, 2023

vonzshik deleted the 4972-split-TypeMapperResolver-from-TypeHandlerResolver branch March 20, 2023 16:30

roji mentioned this pull request Mar 20, 2023

Reducing Npgsql size under NativeAOT #4799

Closed

16 tasks

Split TypeMapperResolver from TypeHandlerResolver #4973

Split TypeMapperResolver from TypeHandlerResolver #4973

Uh oh!

Conversation

vonzshik commented Mar 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NinoFloris left a comment

Choose a reason for hiding this comment

Uh oh!

roji Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

vonzshik Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

roji Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

vonzshik Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

roji Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

vonzshik Mar 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

roji commented Mar 20, 2023

Uh oh!

vonzshik commented Mar 20, 2023

Uh oh!

roji commented Mar 20, 2023

Uh oh!

roji left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vonzshik commented Mar 4, 2023 •

edited

Loading

vonzshik Mar 20, 2023 •

edited

Loading