Add API to NpgsqlDataSource to resolve type mappings#6084
Add API to NpgsqlDataSource to resolve type mappings#6084
Conversation
| /// <summary> | ||
| /// Attempts to return a mapping for a specific type. | ||
| /// </summary> | ||
| public PgTypeInfo? TryGetMapping(Type? type = null, string? dataTypeName = null) |
There was a problem hiding this comment.
Note that PgTypeInfo is [Experimental], and also in the Internal namespace - I think that up to now the idea was that this was only for converter writers; we'd be exposing this to users for the first time. This also shows in the public API surface of this type - most of the stuff there probably shouldn't be shown to users.
I think that if we want to add an introspection API like this, we need another type. We do have the PostgresType hierarchy, but that really models the PG type only (no CLR type there, for example).
(just noting that there's no real urgent need for this API from my side)
/cc @NinoFloris
There was a problem hiding this comment.
I think that if we want to add an introspection API like this, we need another type. We do have the PostgresType hierarchy, but that really models the PG type only (no CLR type there, for example).
Makes sense. We just have to agree on what exactly that type has to have. Probably, CLR type, the name of postgres's type + whether it supports writing?
There was a problem hiding this comment.
NpgsqlMappingInfo or something? Maybe @NinoFloris has a good idea here.
It could actually reference the PostgresType rather than just a string name (for full introspection capabilities). Besides that, just having the CLR type seems sufficient to me as a start, writability seems like an extra thing we don't necessarily need to do...
vonzshik
force-pushed
the
data-source-support-type-mapping-resolve
branch
from
c6130c4 to
d5314a8
Compare
| /// <summary> | ||
| /// Attempts to return a mapping for a specific type. | ||
| /// </summary> | ||
| public async ValueTask<PgTypeInfo?> TryGetMappingAsync(Type? type = null, string? dataTypeName = null, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
Yeah, ideally we'd be able to return mapping info to the user without ever connecting to the database, purely based on how the data source was configured - in which case we wouldn't need an async API here. This is a bit related to us returning a separate type (not PgTypeInfo), which would presumably return restricted info that's purely client-side mapping config.
Or am I missing something and this mapping information API really doesn't make sense without first connecting to the database and getting actual type OIDs etc.?
| /// <summary> | ||
| /// Attempts to return a mapping for a specific type. | ||
| /// </summary> | ||
| public ValueTask<PgTypeInfo?> TryGetMappingAsync<T>(string? dataTypeName = null, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
I think that for cases where we don't actually need a generic type parameter, i.e. a Type parameter is enough (and will likely always be enough), there's no reason to introduce the additional generic API.
| /// <summary> | ||
| /// Attempts to return a mapping for a specific type. | ||
| /// </summary> | ||
| public PgTypeInfo? TryGetMapping(Type? type = null, string? dataTypeName = null) |
There was a problem hiding this comment.
API design note: rather than a single method with two nullable parameters - which throws when both are null - we can have two methods, so that the annotations prevent the call at compile-time (internally they'd both just immediately call into a single method).
Yep, I'm fine with that. After all, you're the one who requested this feature (and probably the only one who's actually going to use it). |
FWIW I do think there are other usages for it - I've seen some cases over the years where dynamic layers/users want to know what Npgsql will map as; that's why the "existing introspection API" exists on NpgsqlParameter. But these are indeed quite rare/advanced indeed. |
2 participants
No description provided.