-
Notifications
You must be signed in to change notification settings - Fork 872
Expose API for datatable plugin #6005
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.
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
base: main
Are you sure you want to change the base?
Conversation
eeddc9b to
5cca138
Compare
|
/cc @NinoFloris |
|
And one more potential addition: Call of this method should be placed at the beginning of ResolveTypeInfo. This will allow:
What do you think? |
These methods are explicitly internal. They are not part of the publicly accessible api surface as direct calls into these methods will miss important edge cases. You can of course use reflection to access anything, but as always when you resort to that you should try to understand why things aren't already setup the way you believe it should be. GetSizeAsObject really is a very good example of this. It's already publicly accessible via the GetSizeOrDbNullAsObject extension method that takes into account many of the peculiarities that apply once values are boxed, and as such I wouldn't want to make the more error prone internal method public, that just leads to a pit of failure. WriteAsObject on the other hand could be something we can consider making public, but certainly not directly. The internal signature containing These methods on PgConverter would be better candidates to make public: internal void WriteAsObject(PgWriter writer, object value);
internal ValueTask WriteAsObjectAsync(PgWriter writer, object value, CancellationToken cancellationToken = default);Let me think a bit whether this could cause any problems down the road, as we'd be somewhat committing to keeping this working in this exact way.
Yes, I believe this has come up before as well. Internally we have access to the entire DatabaseInfo through the PgSerializerOptions but so far I've refrained from making it public to deter plugin writers from building resolvers that are dependent on specific database information, as it's often the incorrect approach. Most resolvers should refer to their supported set of pg types statically. For plugins enhancing functionality matching some predicate on pg types - where you're targetting all composite types, or any type containing a timestamp type, etc. - you absolutely need access to it as the DataTypeName alone won't be enough. @roji what do you think? Should we just make the DatabaseInfo getter public, or should we move this to some static class akin to CollectionMarshal/MemoryMarshal in the BCL? I'm not sure it's worth it but it does signal to callers they might need to think about their approach a bit more.
Regarding your last suggestion about I also noticed in https://github.com/0UserName/npgsql.tvp/blob/master/Npgsql.Tvp/Internal/Resolvers/DataTableTypeInfoResolverFactory.cs you added your DataTable mapping logic to the resolver returned by CreateResolver. Given all mappings only apply to pg array types you might want to move things to be under CreateArrayResolver. Then CreateResolver would just return an empty resolver. That makes sure your resolver ends up in the right location in the chain and properly gets excluded by NpgsqlSlimDataSourceBuilder until EnableArrays is called. What you're trying enable is fairly complicated and hits a lot of the nuances that make the entire type mapping business such an intricate topic, so don't let my feedback deter you. I fully believe that with some careful work it should all be possible within the current architecture, and I agree you need access to a few methods that are currently internal. We just have to make sure anything we add makes sense in the larger picture as well. My suggestion would be to leave the PR for now, focus on getting your plugin into a complete state while trying to use as few internals as you can get away with. Once you're happy (or really running into concrete missing features) we can revisit and see what we should open up. Sounds good? |
But
Make sense.. So, I will revert modifiers and make methods you mentioned as public, as well as
There can be dozens or even hundreds of composite types/table types. For this reason, the implementation of resolving is dynamic in nature.
Well, it's not really about the plugin. If In general, I was interested in the idea of implicit conversion of the type name. |
I'm not sure if I understand correctly. If you specify the type name, but do not use array notation, then an error will occur when sending data to the server, because the name is passed as a regular type, while the server expects an array. That is, firstly, I tried to replace one |
I'm afraid I just have no choice :) At the very least, I would like to have comments on the clarifications and a more or less acceptable PR (at least taking into account the above-mentioned comments) And yes, thank you for the comments. |
Ah I overlooked that, it may make sense to make those extensions public.
If you carefully read my reply you'd read I agree there are resolvers where you do need access to the runtime set of pg types. However, in general, resolvers provide a static set of mappings from CLR to postgres type and vice versa. A resolver describes all the mappings it is capable of, not just which types some specific database has available. You can take a look at our builtin resolvers, almost all of them are just a long list of concrete pg type names mapped to concrete CLR types. For plugins enabling something like a new postgres plugin (pgvector, ltree, postgis) the same applies, and you'll see that those resolvers just contain mappings for CLR type(s) to e.g. "pgvector". That's really the primary use for having this infrastructure pubternal at all. Again, I agree there is a place for predicate based resolvers (and we also have some inside Npgsql, to handle any pg enum type for instance) but that was not the original target audience.
There is no DataTable support planned as far as I'm aware? Any support would live entirely in your plugin. Even if we would add some DataTable support in Npgsql we'd do so through the PgConverterResolver mechanism. That mechanism really has nothing to do with requiring array notation or not. PgConverterResolver exists to defer postgres type resolution until a user value is presented. Specifically when no other information is given (so no NpgsqlDbType or DataTypeName). An IPgTypeInfoResolver for DataTable would see |
Thanks for detailed explanation. I solved this problem. Unfortunately, without access to
|
I'm curious how you did that, can you explain what you did?
Yes, you still need access to DatabaseInfo to be able to get the postgres type information. I haven't said anything to the contrary. |
yy. I just wrote why it's important. Maybe it's obvious, but still.
I implemented custom |
…nverterExtensions in PgConverter
5cca138 to
5eb664a
Compare
PR updated. |
|
Is there a chance that some of the changes will be accepted/merged? |
Continuing the discussion: #5415.
I changed the access modifiers for
WriteAsObjectandGetSizeAsObjectmethods in PgConverter, and addedGetCompositeTypetoPgSerializerOptions.This will avoid using
UsafeAccessorin the plugin.