Allow hstore columns to be represented as IReadOnlyDictionary<string, string?>.#2784
Allow hstore columns to be represented as IReadOnlyDictionary<string, string?>.#2784Quogu wants to merge 3 commits intonpgsql:devfrom Quogu:IReadOnlyDictionaryHstore
Conversation
… string?>. This adds support for IReadOnlyDictionary<string, string> off the back of #2776 - it's required for the work in npgsql/efcore.pg#1184.
Quogu
requested review from
YohDeadfall,
austindrenski and
roji
as code owners
YohDeadfall
left a comment
YohDeadfall
left a comment
There was a problem hiding this comment.
There are some issues which should be resolved first. After that I will merge your PR, thanks (:
>> >> - Correct null-safety in test class. >> - Use a private method taking an enumerable and a count to ensure compatibility with all dictionary classes and avoid duplicating a method. >> - Single lines for method signatures.
- Include IReadOnlyDictionary interface in all frameworks. - Implement IReadOnlyDictionary support with Dictionary instead of ImmutableDictionary. - Method signatures on one line.
|
I think I've fixed everything you mentioned. :) |
| => Write(value, value.Count, buf, lengthCache, parameter, async); | ||
|
|
||
| ValueTask<IReadOnlyDictionary<string, string?>> INpgsqlTypeHandler<IReadOnlyDictionary<string, string?>>.Read(NpgsqlReadBuffer buf, int len, bool async, FieldDescription? fieldDescription) | ||
| => new ValueTask<IReadOnlyDictionary<string, string?>>(Read(buf, len, async, fieldDescription).Result); |
There was a problem hiding this comment.
Getting result of an incomplete ValueTask<T> may cause an exception if IValueTaskSource is used under the hood. In case of Task this method will block the current thread. Therefore, you need to rewrite this as:
async ValueTask<IReadOnlyDictionary<string, string?>> INpgsqlTypeHandler<IReadOnlyDictionary<string, string?>>.Read(NpgsqlReadBuffer buf, int len, bool async, FieldDescription? fieldDescription)
=> await Read(buf, len, async, fieldDescription);
|
I'd like to make sure this is a good idea, and understand exactly why we think this is needed... First, as a general rule Npgsql doesn't map to interfaces but to concrete types. Writing is pretty straightforward, but it's quite odd to allow users to read a value as an interface (i.e. do At the EF Core level things could be a bit different, but I'm not completely sure it makes sense there either - if a user defines an entity property of type IDictionary, is it right for the ORM to decide that data should be materialized as Dictionary? Sorry for not originally having raised this in npgsql/efcore.pg#1184, but I'd like to be sure we're doing the right thing. Aren't the goals of npgsql/efcore.pg#1184 fully met by allowing to map ImmutableDictionary, without IReadOnlyDictionary? Also, even without any special support, users can always use value converters to have a property of type IReadOnlyDictionary that's "converted" (cast) to a Dictionary. |
|
Another option, if one wants to avoid value converters in EF, is simply to have a private backing field of type Dictionary exposed by a public property of type IReadOnlyDictionary. |
|
Makes sense... But previously we had interfaces as supported types for NTS if my memory serves me right. |
|
That's true, and I remember having big doubts on it back then too... In any case those are gone now... |
|
If you think that all required stuff can be done on the EF side, then I'm fine with it and closing this PR. Having less responsibilities is a good way for the driver. |
Yeah, I think so too. Let's wait to hear from @Quogu if there are any specific reasons why the backing field strategy isn't sufficient, for example. |
From an ergonomic standpoint, it'd be nicer not to have to set up backing fields and configure EF to use them, auto-properties are generally nicer, but it's not hugely important. I think that given the work has already been done to support |
|
At some point I would like to serialize interfaces to minimize allocations and CPU usage in case when there's a custom type not inherited from a class. Yet it's not possible to read a such custom object for now, but it's definitely possible. |
|
@Quogu thanks for understanding - yeah, I'd prefer to not decide implementation types in this way. We should definitely keep the ImmutableDictionary support both here and in EF Core. @YohDeadfall I assume you're talking about composite types, where we map CLR type ISomething and want to be able to send any type implementing ISomething? What would be your expectation for reading in this case? |
|
Not composites, but collections which implement Internally a collection could have a custom validation logic. The collection has a parameterless constructor, so can be instantiated using the activator. A good way forward is to support them without forcing to build a new handler. What's your thoughts on this? |
|
What would be the reading behavior? |
|
The same as before, only object creation will be delegated to a factory. I'll do a simple working example of this and later let's decide is it useful or not. |
|
I think it's better to wait until this is requested by some actual users, with a good explanation of why it's needed... Array use seems rare enough, and this would really only be useful for very niche ultra-high-perf scenarios - we have a lot of work on much more day-to-day stuff. |
3 participants
This adds support for
IReadOnlyDictionary<string, string>based onImmutableDictionary<string, string?>off the back of #2776 - it's required for the work in npgsql/efcore.pg#1184.