Add type-casts for SQL literals in list mapping#552
Conversation
| var list = (IList)value; | ||
|
|
||
| if (list.GetType().GenericTypeArguments[0] != ElementMapping.ClrType) | ||
| var genericType = ElementMapping.ClrType; |
There was a problem hiding this comment.
Some comments on this...
First, the error message is imprecise - this is detecting arrays of arrays ("jagged arrays") rather than multi-dimensional arrays. We're conflating two different issues:
- Jagged arrays simply aren't supported by PostgreSQL.
- Multidimensional arrays are supported by PostgreSQL, but we just didn't write the code for generating a literal representation of them. This is only because multidimensional arrays are a rarely-used feature and I didn't have time to work on it - full support is tracked by Fully support multi-dimensional arrays #314 (you may want to work on this in a separate PR).
At the very least we should fix the exception message(s), to make sure the issue is clear.
To be clear, this isn't an issue with your PR, the problems were there before as well! Let's leave all this out of this PR (sticking only to the missing type-cast) and clean this up in a separate PR - are you interested?
Second, I'd rather check for invalid mappings at construction time, rather than at literal generation in time. In other words, if a mapping is not supported (e.g. jagged arrays), then it should never be instantiated. The current code successfully creates the mapping instance and even stores it in NpgsqlTypeMappingSource.ClrTypeMappings, but only bombs once literal generation is attempted. I'd rather move the check and the exception to the constructor. This is valid for NpgsqlListTypeMapping as well as for NpgsqlArrayTypeMapping.
Third, I'd rename genericType to elementClrType to make the code more clear (although this change is probably getting reverted anyway in this PR).
There was a problem hiding this comment.
Okay, I'm partially reverting to keep just the SQL cast generation (and the is null/== null revert).
I'll open a new PR to track these issues in more detail.
ef300d2 to
3d7451e
Compare
Moved from #541, follow-up to #542.
Related
#542 (comment)
#542 (comment)