Skip to content

Conversation

@austindrenski
Copy link
Contributor

@austindrenski austindrenski commented Jul 29, 2018

Moved from #541, follow-up to #542.

Related

#542 (comment)
#542 (comment)

var list = (IList)value;

if (list.GetType().GenericTypeArguments[0] != ElementMapping.ClrType)
var genericType = ElementMapping.ClrType;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@roji roji mentioned this pull request Jul 29, 2018
@austindrenski austindrenski merged commit b3582c2 into npgsql:dev Jul 29, 2018
@austindrenski austindrenski deleted the list-type-mapping branch July 29, 2018 22:29
@roji roji added cleanup and removed refactor labels May 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants