Allow hstore columns to be represented as ImmutableDictionary<string, string>#1184
Allow hstore columns to be represented as ImmutableDictionary<string, string>#1184Quogu wants to merge 6 commits intonpgsql:devfrom Quogu:IReadOnlyDictionaryHstore
Conversation
… string>. The motivation for this is that most other types on an EF model class supported by Npgsql offer only one type of mutability - replacing the instance of the property. Dictionaries offer an additional type of mutability, as not only can the reference be swapped but the same reference can have its internal state changed, which makes reasoning about the correctness of code harder. Supporting IReadOnlyDictionary ensures there's only one point of mutability (or none, if EF is configured to use private field access and the public contract only offers a getter). It would be nice to offer full support for ImmutableDictionary<string, string> too but from looking at the code, I think this would require further changes to Npgsql rather than just the EF Core part, which unfortunately I don't have time to look into. Still, I think this is a net win even without that.
Quogu
requested review from
YohDeadfall,
austindrenski and
roji
as code owners
roji
left a comment
roji
left a comment
There was a problem hiding this comment.
It would be nice to offer full support for ImmutableDictionary<string, string> too but from looking at the code, I think this would require further changes to Npgsql rather than just the EF Core part, which unfortunately I don't have time to look into. Still, I think this is a net win even without that.
I've done this in npgsql/npgsql#2776. We can take a dependency on a CI build of Npgsql from the EFCore.PG provider (once it's merged) and improve this PR to also support ImmutableDictionary.
As well as running the existing tests locally, I put together a simple application to verify that I could add, retrieve, compare using equality and call sql functions with IReadOnlyDictionary<string, string> as the type on the model. Happy to stick that code in a Gist if you'd like to see it.
This PR definitely needs more tests - the tests added NpgsqlTypeMapppingTest are very restricted unit tests which check only the value comparer. We should at the very least add this case to BuiltInDataTypesNpgsqlTests,
- Rename Immutable to ReadOnly to better represent what the code is actually doing. - Change comparison type for the ReadOnly hstore comparer to just check for references, with a comment explaining the reasoning for this.
|
Thanks very much for the quick review! I've updated the PR to address the comments you left on the existing code, and will look to add a test in |
…dded support for them to the base library.
|
When adding tests in |
|
@roji, ping. |
There was a problem hiding this comment.
Thanks for the continued work on this, @Quogu - it's pretty close to merging. We still need to remove the IReadOnlyDictionary mapping as per npgsql/npgsql#2784 (comment).
Unless I'm mistaken, in order for the ImmutableDictionary mapping to work, this needs to take a dependency on a CI version of Npgsql which contains npgsql/npgsql#2775. I can do this once you've finished with the other work and amend it into your commit. If you'd like to do this yourself, you'll need to add a NuGet.Config referencing our unstable feed, and take the latest 5.0.0 package from there. However, you may run into difficulties - it's fine to leave that to me in that case.
| /// <remarks> | ||
| /// See: https://www.postgresql.org/docs/current/static/hstore.html | ||
| /// </remarks> | ||
| public class NpgsqlReadOnlyHstoreTypeMapping : NpgsqlHstoreTypeMapping |
There was a problem hiding this comment.
As discussed, while we want to add a mapping for ImmutableDictionary, we shouldn't add mappings to interfaces... So we need to remove this (and everything related elsewhere).
|
|
||
| var comparer = Mapper.FindMapping(typeof(ImmutableDictionary<string, string>), "hstore").Comparer; | ||
| var snapshot = comparer.Snapshot(source); | ||
| Assert.Equal(source, snapshot); |
There was a problem hiding this comment.
Might as well assert that they're the same...
|
Looking back at old PRs... @Quogu, is this something you still want to pursue? |
|
Hi @roji, unfortunately I've moved jobs since raising this PR and don't have time to continue with it. I still think it's a good idea, but as I'm unable to commit any time to help, I understand if you just want to close it. |
|
Thanks @Quogu, no problem. I'll take a look at it and will probably merge after some fixups. |
roji
changed the title
IReadOnlyDictionary<string, string>.ImmutableDictionary<string, string>.
roji
changed the title
ImmutableDictionary<string, string>.
|
Cleaned up and replaced by #1492 |
3 participants
The motivation for this is that most other types on an EF model class supported by Npgsql offer only one type of mutability - replacing the instance of the property. Dictionaries offer an additional type of mutability, as not only can the reference be swapped but the same reference can have its internal state changed, which makes reasoning about the correctness of code harder. Supporting IReadOnlyDictionary ensures there's only one point of mutability (or none, if EF is configured to use private field access and the public contract only offers a getter).
It would be nice to offer full support for ImmutableDictionary<string, string> too but from looking at the code, I think this would require further changes to Npgsql rather than just the EF Core part, which unfortunately I don't have time to look into. Still, I think this is a net win even without that.
As well as running the existing tests locally, I put together a simple application to verify that I could add, retrieve, compare using equality and call sql functions with
IReadOnlyDictionary<string, string>as the type on the model. Happy to stick that code in a Gist if you'd like to see it.