Conversation
0fb90c0 to
7982f05
Compare
|
Rebased onto main |
There was a problem hiding this comment.
Pull request overview
This PR refactors Npgsql’s type info/converter resolution model as prework for simplifying read/write call sites by making PgTypeInfo abstract and introducing PgConcreteTypeInfo as the unified, authoritative carrier used during reads/writes. It also renames the “resolver” concepts to “provider” concepts (PgConverterResolver → PgConcreteTypeInfoProvider, PgResolverTypeInfo → PgProviderTypeInfo) to better reflect responsibilities and reduce duplicated logic around delayed resolution.
Changes:
- Introduce
PgConcreteTypeInfoand makePgTypeInfoabstract, shifting resolution APIs to return concrete type infos. - Replace converter resolvers with concrete type info providers and update call sites/mapping composition accordingly.
- Update tests and internal factories/converters to use the new provider + concrete type info model.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Npgsql.Tests/TypeMapperTests.cs | Update test resolver to return PgConcreteTypeInfo. |
| test/Npgsql.Tests/ReaderTests.cs | Update test resolver to return PgConcreteTypeInfo. |
| test/Npgsql.Tests/CopyTests.cs | Rename test to reflect provider terminology. |
| src/Npgsql/TypeMapping/GlobalTypeMapper.cs | Adjust data type name inference to use provider/concrete type info APIs. |
| src/Npgsql/NpgsqlParameter`.cs | Update generic parameter implementation to request concrete type info. |
| src/Npgsql/NpgsqlParameter.cs | Replace converter resolution with concrete type info selection during binding. |
| src/Npgsql/NpgsqlBinaryExporter.cs | Update binary export binding to use provider default concrete type info. |
| src/Npgsql/Internal/TypeInfoMapping.cs | Refactor mapping composition to operate on providers and PgConcreteTypeInfo. |
| src/Npgsql/Internal/ResolverFactories/UnmappedTypeInfoResolverFactory.cs | Update dynamic range/multirange mapping composition for concrete type infos. |
| src/Npgsql/Internal/ResolverFactories/NetworkTypeInfoResolverFactory.cs | Use PgConcreteTypeInfo for IPAddress mapping. |
| src/Npgsql/Internal/ResolverFactories/AdoTypeInfoResolverFactory.cs | Update baseline mappings to use providers/concrete type infos (incl. temporal/bitstring/arrays). |
| src/Npgsql/Internal/ResolverFactories/AdoTypeInfoResolverFactory.Range.cs | Update range mappings to use DateTime type info providers. |
| src/Npgsql/Internal/ResolverFactories/AdoTypeInfoResolverFactory.Multirange.cs | Update multirange mappings to use DateTime type info providers. |
| src/Npgsql/Internal/PgTypeInfo.cs | Core refactor: make PgTypeInfo abstract, add PgProviderTypeInfo + PgConcreteTypeInfo, replace resolution APIs. |
| src/Npgsql/Internal/PgSerializerOptions.cs | Update UnspecifiedDBNullTypeInfo to be a PgConcreteTypeInfo. |
| src/Npgsql/Internal/PgConverterResolver.cs | Removed in favor of PgConcreteTypeInfoProvider. |
| src/Npgsql/Internal/PgConcreteTypeInfoProvider.cs | New public experimental provider abstraction with validation and internal plumbing. |
| src/Npgsql/Internal/PgComposingTypeInfoProvider.cs | New composing provider base to cache composed PgConcreteTypeInfo instances. |
| src/Npgsql/Internal/PgComposingConverterResolver.cs | Removed in favor of composing provider base. |
| src/Npgsql/Internal/DynamicTypeInfoResolver.cs | Update reflection-based mapping registration to call provider APIs. |
| src/Npgsql/Internal/Converters/Temporal/DateTimeTypeInfoProvider.cs | New DateTime provider implementation replacing the old resolver. |
| src/Npgsql/Internal/Converters/Temporal/DateTimeConverterResolver.cs | Removed in favor of DateTime type info provider. |
| src/Npgsql/Internal/Converters/PolymorphicConverterResolver.cs | Removed in favor of polymorphic array type info provider(s). |
| src/Npgsql/Internal/Converters/PolymorphicArrayTypeInfoProvider.cs | New provider for polymorphic array converter composition (read-time). |
| src/Npgsql/Internal/Converters/ObjectConverter.cs | Switch object converter binding to new concrete type info APIs. |
| src/Npgsql/Internal/Converters/NullableConverter.cs | Replace nullable composing resolver with nullable composing provider. |
| src/Npgsql/Internal/Converters/CastingConverter.cs | Replace casting composing resolver with casting composing provider and update ToNonBoxing. |
| src/Npgsql/Internal/Converters/BitStringConverters.cs | Replace polymorphic bitstring resolver with provider producing cached concrete infos. |
| src/Npgsql/Internal/Converters/ArrayConverter.cs | Refactor array converter composition to accept PgConcreteTypeInfo and use providers for polymorphic cases. |
| src/Npgsql/Internal/Composites/Metadata/CompositeFieldInfo.cs | Update composite field binding to use concrete type infos and provider terminology. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
roji
left a comment
There was a problem hiding this comment.
OK, I gave this a somewhat impressionistic; it generally makes sense to me, but my grasp of the details in this area isn't very strong, and this is also the first step in a direction where I don't have the final picture. But it all does seem to make sense.
@NinoFloris if you have some specific points/thoughts/questions you want to discuss around this don't hesitate to let me know.
(as always, I can, however, generate lots of uninteresting nits ;))
d2f0833 to
19a2538
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
# Conflicts: # src/Npgsql/Internal/Converters/ObjectConverter.cs # src/Npgsql/NpgsqlParameter`.cs
# Conflicts: # src/Npgsql/Internal/TypeInfoMapping.cs
19a2538 to
f67caf6
Compare
|
@roji please take a look at this portion, you might find this interesting to think about.
For the completed picture, this was mostly completed in #6317, going commit by commit should give a decent overview. Essentially the finished rework will remove a lot of the lifetime paper cuts, easy mistakes, and duplication. Having most of it be handled centrally by PgTypeInfo. Once users are able to use the high level pieces I'm much more confident in helping them author their own (non-toy) converters/providers for composing over other type infos. |
As it stands we support two kinds of PgTypeInfos: one directly wrapping a converter and one to facilitate delayed converter resolution (PgConverterResolver). Unfortunately both return the same information from different places today, and it must generally be cached for different lengths of time as well. This causes us to have a bunch of accidental complexity (additional types and state) and duplicated logic to work on 'these things that must not be forgotten or uncommonly used things go wrong' for the case where converter resolver type infos are passed in.
For example, the public PgConverterResolution, the internal
PgConverterResolverInfoand even to some extent the internal ColumnInfo and ColumnInfo for the nested reader exist because we need to handle that split (where an info could either be concrete or a delayed resolution) all the way down at the actual read and write call sites. Just to paint a picture, just Npgsql alone is expected to implement this split correctly in all the following places: NpgsqlParameter, NpgsqlBinaryImporter/NpgsqlBinaryExporter, NpgsqlDataReader, NpgsqlNestedDataReader, ReplicationValue, CompositeConverter, and ObjectConverter. But also any plugin that wants to compose type infos to write some larger data type has to deal with this.Unsurprisingly it would be much better if all of this could be centralized, and I recently got inspired to finally tackle this after reviewing #6005. The feature that the linked PR intends to enable deals with all the same complexity around value dependent writing to provide general DataTable to pg (table) composite support, which they would like to release as a plugin. Today it's practically impossible to do that correctly, due to the knowledge required to replicate that split correctly for the composed, field level, type info handling needed for composites.
To improve the status quo this PR starts by making PgTypeInfo abstract and adds a new derived type to the hierarchy: PgConcreteTypeInfo, this will be the authoritative info source that gets used during reads and writes, no more split at all those sites. We also rename
PgConverterResolvertoPgConcreteTypeInfoProviderandPgResolverTypeInfotoPgProviderTypeInfo. These renames align the types with what they operate on and gets rid of the overloaded resolver/resolution moniker, which will now just relate toIPgTypeInfoResolver. This has been a change I've been meaning to make ever since we released 8.0, at the time it was just too late in the cycle to rework and think all of this through properly.Shifting the PgTypeInfo hierarchy into this direction allows us to merge the concrete/resolver split back into one source after field and value binding. For most cases nothing really changes on the mapping authoring side except the type identity: mappings that don't rely on a resolver will now return PgConcreteTypeInfo. As most mappings rely on
mapping.CreateInfoto create their PgTypeInfo we are able to change this in a binary compatible way (and if the PgTypeInfo constructor does happen to be used today, rare even in Npgsql, a small change to new up PgConcreteTypeInfo instead is sufficient).The PgResolverTypeInfo and PgConverterResolver changes are more breaking, though also easily adapted to. Regardless, no users of this api surface have been found after a thorough github search (searching for any direct references to these types); it's also quite unlikely private implementations exist. Additionally all of this api surface is marked experimental, and while I don't advocate for doing impactful breaking changes just because we can, it is intended to give us that kind of wiggle room.
Finally, where current mappings do use the PgProviderTypeInfo functionality (like bitstring fields or datetime values) the provider implementation (previously converter resolver) will become responsible for returning *cached* PgConcreteTypeInfo instances. The unwillingness to demand this caching was the original sin that lead to the split, and why the PgConverterResolution return type was a struct.
At the time it was unclear to me whether we could always cache the returned results (which otherwise would lead to an unavoidable allocation on every bind) as the providers may act on ever changing values. In practice all implementations we authored end up caching their produced converter instances already. And at this point I'm also convinced that any useful implementation classifies fields and values into a few well known categories it has converters for, and for which it is never a problem to cache the required variations.
The total work is split across PRs to make reviewing more practical. This PR mostly contains the nominal and type level restructuring, while the follow up PR will use these changes to actually deliver the simplification of the call sites. A final PR should provide the public surface that we can recommend to plugin authors interested in working with POCO or dynamic (e.g. datatable) conversions that interact correctly with nested field and value binding.