Fix mapping issues around arrays and byte arrays #1196
Conversation
Caching is already done by TypeMappingSource in EF
| internal static bool IsArrayOrGenericList(this Type type) | ||
| => type.IsArray || type.IsGenericList(); | ||
|
|
||
| internal static bool TryGetElementType(this Type type, out Type? elementType) |
There was a problem hiding this comment.
It's better to return an element type as is or null.
There was a problem hiding this comment.
I'm not sure... This means that instead of:
if (!operand.Type.TryGetElementType(out var operandElementType))we need to write:
if (!(operand.Type.GetElementType() is Type operandElementType))which seems considerably more verbose/ugly. Another small advantage of TryGetElementType is that you can put an existing variable.
Both patterns seem to have advantages and disadvantages...
| { | ||
| // PostgreSQL array type names are the element plus [] | ||
| var clrType = mappingInfo.ClrType; |
There was a problem hiding this comment.
When could clrType be equal to null?
There was a problem hiding this comment.
When reverse engineering an existing database - all you have is the store type from PostgreSQL, and you're creating the C# code model.
src/EFCore.PG/Storage/Internal/Mapping/NpgsqlArrayTypeMapping.cs
Outdated
Show resolved
Hide resolved
src/EFCore.PG/Storage/Internal/Mapping/NpgsqlArrayTypeMapping.cs
Outdated
Show resolved
Hide resolved
| if (clrType == null || !ClrTypeMappings.TryGetValue(clrType, out var mapping)) | ||
| if (clrType == null || | ||
| !ClrTypeMappings.TryGetValue(clrType, out var mapping) || | ||
| storeTypeName != null && mapping.StoreType != storeTypeName) |
There was a problem hiding this comment.
Should ClrTypeMappings be checked last?
| storeTypeName != null && mapping.StoreType != storeTypeName) | |
| storeTypeName != null && storeTypeName != mapping.StoreType) |
There was a problem hiding this comment.
Should ClrTypeMappings be checked last?
Can you explain what you mean? Assuming there's a mapping that corresponds to the given clrType, we want to verify that the storeTypeName corresponds to it too (but only if it's given).
There was a problem hiding this comment.
I mean that you can change the order of execution and do lookup as the last step.
There was a problem hiding this comment.
But the lookup provides mapping, which is used afterwards? Or am I missing something?
There was a problem hiding this comment.
Yes, but you know I am a bit maniacal (:
Co-Authored-By: Yoh Deadfall <yoh.deadfall@hotmail.com>
| ? elementNpgsqlTypeMapping.NpgsqlDbType | ||
| : elementMapping.DbType.HasValue | ||
| ? new NpgsqlParameter { DbType = elementMapping.DbType.Value }.NpgsqlDbType | ||
| : default(NpgsqlDbType?)); |
There was a problem hiding this comment.
To large indentation, but it's a nit.
There was a problem hiding this comment.
This is how Rider likes it... I've stopped fighting :)
There was a problem hiding this comment.
As I know, it's adjustable. For me it doesn't recommend such things, am using a cleaned up editor config from dotnet/runtime.
There was a problem hiding this comment.
Yeah. If you really feel like it you can work on our .editorconfig :)
Fixes #1189