Uh oh!
There was an error while loading. Please reload this page.
Fix range mapping - #698
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. Fixesnpgsql#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 |
roji
commented
Nov 14, 2018
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 |
austindrenski
commented
Nov 14, 2018
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. |
roji
commented
Nov 15, 2018
@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?
roji
commented
Nov 15, 2018
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".