Conversation
We used to eagerly create mappings for user-defined ranges in the constructor of NpgsqlTypeMappingSource. This was too early, and did not have access to mappings coming from plugins, so creating a user-defined range over NodaTime types failed. We know create mappings for user-defined ranges late, when FindMapping() is called. Fixes npgsql#688
It was neither a MappingInfo nor a mapping in the narrow EF Core sense.
austindrenski
left a comment
There was a problem hiding this comment.
The implementation here looks good.
Would it make sense to add a test case for the pattern in #688 that triggered this?
Would it make sense to |
It does :) We haven't yet written any tests on NpgsqlTypeMappingSource, but now's a good a time as ever to start... I'll add some testing tomorrow.
I'm not sure... From the user perspective they really are mapping a PostgreSQL range type (the first parameter) to a CLR subtype (the second parameter). The only issue is that this isn't a mapping in the internal EF Core sense: it does not extend TypeMapping and isn't a TypeMappingInfo either. So it may be OK to leave |
Sounds great!
OK—keeping it as is sounds fine then. No need to churn the public API if we're really just talking about implementation details at this point. |
|
@austindrenski I pushed some tests, please take a look. |
austindrenski
left a comment
There was a problem hiding this comment.
This looks great. One tiny nit to fix if you have time, otherwise merge at will.
| public void By_StoreType_with_wrong_ClrType(string storeType, Type wrongClrType) | ||
| => Assert.Null(Source.FindMapping(wrongClrType, storeType)); | ||
|
|
||
| class UnknownType {} |
There was a problem hiding this comment.
nit: Could you move this down alongside DummyType?
a3359a7 to
2c81889
Compare
|
Fixed nit and merged, thanks for the reviewing! |
Fixes #688
Note the rename from RangeMappingInfo to UserRangeDefinition... While working on this it got confusing, as RangeMappingInfo wasn't a MappingInfo in the strict EF Core sense. I've left the public-facing
MapRange()method on NpgsqlDbContextOptionsBuilder, partially to avoid breaking existing code, and partially because it seems acceptable to call this "mapping" in user-facing APIs, while internally it produces a "definition".