Efcore.pg: Nested (through navigation property) Value Conversion (enum to string) causes exception.

[smitpatel]

_From @JakenVeina on July 28, 2018 5:52_

When selecting an enum value from a related record, conversions defined for that property are not evaluated, and cause an InvalidCastException (wrapped by an InvalidOperationException).

1. Problem Query: https://hastebin.com/ugufozituy.cs
2. ModerationActionSummary.FromEntityProjection: https://hastebin.com/ukudanudoc.cs
3. ModerationActionEntity: https://hastebin.com/lemedabofe.cs
4. InfractionEntity: https://hastebin.com/wihotayoyu.cs
5. ModixContext.OnModelCreating(): https://hastebin.com/iqayacecox.cs
6. InfractionSummary: https://hastebin.com/padakicese.cs
7. Full Codebase: https://github.com/JakenVeina/MODiX/tree/efcore-issue-12834

Specifically, the Type = entity.Infraction.Type, line in snippet 2 above is causing the following exception: https://hastebin.com/equfozunaj.

When the .Type property is being selected from the root element in an expression, as in snippet 6 above, the value conversion works fine.

Furthermore, I can work around the error by changing
Type = entity.Infraction.Type
in snippet 2, to the following lovely expression.
Type = Enum.Parse<InfractionType>(entity.Infraction.Type.ToString())

This seems to work great. The whole query still evaluates to one SQL query. But surely, there ought to be a way to support the more sensible version of the expression.

Using:

1. netcoreapp2.1
2. Microsoft.EntityFrameworkCore (2.1.1)
3. Mpgsql.EntityFrameworkCore.PostgreSQL (2.1.1)
4. Windows 8.1 Pro
5. Visual Studio 2017 15.7.5.

_Copied from original issue: aspnet/EntityFrameworkCore#12834_

[smitpatel]

_From @JakenVeina on July 29, 2018 3:17_

Built a minimal test case project, to reproduce the issue.

https://github.com/JakenVeina/EFCoreIssue12834

Running it should be as simple as setting up a postgreSQL instance for it and setting the appropriate connection string in Program.cs.

[smitpatel]

Since I did not have PostgreSQL installed, I tried it with SqlServer and it worked fine, moving it here. If this repros in other providers then feel free to open an issue in EF Core repo.

[austindrenski]

@smipatel Thanks for forwarding, I'll take a look.

[austindrenski]

Still investigating, but I've narrowed the repro here: austindrenski/EFCore.PG-556

[austindrenski]

Okay—I can reproduce and eliminate the exception in both projects by removing the calls to HasConversion<string>() on the enums.

In this case, it looks like the value converters are redundant, but still cause trouble during materialization. (Running these samples with empty tables doesn't throw.)

My suspicion is that this wasn't reproducible by @smitpatel because CLR enums are just ~~text integers in SqlServer. But PostgreSQL has a real enum data type, so this provider has some special handling for CLR enums (both when explicitly mapped or unmapped).~~

That probably makes this a bug on our end, since I don't think that we _want_ redundant value converters to cause exceptions.

@JakenVeina Could you please test removing the value converters from your entity configurations, and report back if that resolves your immediate issue?

/cc @roji

_edit: see below_

[roji]

I don't have time to look at this in depth, but now that the default enum behavior for any provider should be to map it to database int, so I don't think it should matter if SQL Server treats enums as strings. Take a look at this, I'll poke my nose in as well this weekend.

[austindrenski]

@roji That's a typo on my part. When I wrote:

because CLR enums are just text in SqlServer.

I meant to wite:

because CLR enums are just integers in SqlServer.

[roji]

Right, so in fact enums are supposed be just ints in PostgreSQL too - just like SQL Server - unless they are mapped via the Npgsql-specific API. This is why there shouldn't be any behavioral difference between the two providers unless the user is mapping explicitly to database enums.

[austindrenski]

Some additional info:

• HasConversion<string>() adds an annotation for ProviderClrType to the metadata.

• During materialization, the FieldDescription is set as SomeEnum(integer).

  • Without HasConversion<string>(), the FieldDescription is SomeEnum(text).
• This leads to the exception being thrown from this call:
  c# NpgsqlDbDataReader.GetInt32(int ordinal) => NpgsqlDbDataReader.GetFieldValue<int>(ordinal);

[austindrenski]

Right, so in fact enums are supposed be just ints in PostgreSQL too - just like SQL Server - unless they are mapped via the Npgsql-specific API. This is why there shouldn't be any behavioral difference between the two providers unless the user is mapping explicitly to database enums.

Just double checked that the database generation is working as expected:

```c#
public class SomeEntity
{
public long Id { get; set; }
public SomeEnum SomeEnum { get; set; }
public long SomeOtherEntityId { get; set; }
public OtherEntity OtherEntity { get; set; }
}

Here is the database generated when ___not using___ `HasConversion<string>()`:
```sql
testing=# \d "SomeEntities"
                                   Table "public.SomeEntities"
      Column       |  Type   | Collation | Nullable |                  Default
-------------------+---------+-----------+----------+--------------------------------------------
 Id                | bigint  |           | not null | nextval('"SomeEntities_Id_seq"'::regclass)
 SomeEnum          | integer |           | not null |
 SomeOtherEntityId | bigint  |           | not null |

And here is the database generated when using HasConversion<string>():

testing=# \d "SomeEntities"
                                   Table "public.SomeEntities"
      Column       |  Type  | Collation | Nullable |                  Default
-------------------+--------+-----------+----------+--------------------------------------------
 Id                | bigint |           | not null | nextval('"SomeEntities_Id_seq"'::regclass)
 SomeEnum          | text   |           | not null |
 SomeOtherEntityId | bigint |           | not null |

[roji]

@austindrenski right, so migrations are working well, and when you specify the value conversion you appropriately get a text column for your enum instead of an integer. The question now is why you're getting exceptions when reading/writing...

[austindrenski]

Here's the underlying throw:

```c#
// this: Npgsql.TypeHandlers.TextHandler
// TAny: int
protected internal override ValueTask Read(NpgsqlReadBuffer buf, int len, bool async, FieldDescription fieldDescription = null)
{
var asTypedHandler = this as INpgsqlTypeHandler;
if (asTypedHandler == null)
{
buf.Skip(len); // Perform this in sync for performance
throw new NpgsqlSafeReadException(new InvalidCastException(fieldDescription == null
? $"Can't cast database type to {typeof(TAny).Name}"
: $"Can't cast database type {fieldDescription.Handler.PgDisplayName} to {typeof(TAny).Name}"
));
}

return asTypedHandler.Read(buf, len, async, fieldDescription);

}
```

_edit: removed unrelated info_

[austindrenski]

Just another bit of info: this only occurs when the relationship results in a LEFT OUTER JOIN (i.e. when SomeEntityId is long?).

When an INNER JOIN is generated, no exception occurs.

[austindrenski]

SelectExpression.GetMappedProjectionTypes() uses RelationalExpressionExtensions.FindProperty(...) to construct an instance of TypeMaterializationInfo for each item in the projection:

```c#
foreach (var typeMaterializationInfo in _projection.Select(
e =>
{
var queryType = e.NodeType == ExpressionType.Convert
&& e.Type == typeof(object)
? ((UnaryExpression)e).Operand.Type
: e.Type;

    return new TypeMaterializationInfo(
        queryType,
        e.FindProperty(queryType),
        Dependencies.TypeMappingSource);
}))

{
yield return typeMaterializationInfo;
}

In the repro of this issue, `e` is a `UnaryExpression ` wrapping a `NullableExpression` wrapping a `UnaryExpression ` wrapping a `ColumnExpression`.

Unfortunately, it seems that `RelationalExpressionExtensions.FindProperty(...)` doesn't unwrap the `NullableExpression`, so it never gets down to the `ColumnExpression`, and thus returns `null`:

```c#
public static IProperty FindProperty(
    [NotNull] this Expression expression, 
    [NotNull] Type targetType)
{
    targetType = targetType.UnwrapNullableType();

    switch (expression)
    {
        case ColumnExpression columnExpression:
            return columnExpression.Property;
        case ColumnReferenceExpression columnReferenceExpression:
            return columnReferenceExpression.Expression.FindProperty(targetType);
        case AliasExpression aliasExpression:
            return aliasExpression.Expression.FindProperty(targetType);
        case UnaryExpression unaryExpression:
            return unaryExpression.Operand.FindProperty(targetType);
        case SqlFunctionExpression functionExpression:
            var properties = functionExpression.Arguments
                .Select(e => e.FindProperty(targetType))
                .Where(p => p != null && p.ClrType.UnwrapNullableType() == targetType)
                .ToList();

            var property = properties.FirstOrDefault();
            if (properties.Count > 1)
            {
                var mapping = property.FindRelationalMapping();
                foreach (var otherProperty in properties)
                {
                    if (otherProperty.FindRelationalMapping() != mapping)
                    {
                        // Issue #10006
                        return null;
                    }
                }
            }

            return property;
    }

    return null;
}

If I manually unwrap e, everything seems to work again:

```c#
foreach (var typeMaterializationInfo in _projection.Select(
e =>
{
var queryType = e.NodeType == ExpressionType.Convert
&& e.Type == typeof(object)
? ((UnaryExpression)e).Operand.Type
: e.Type;

    if (e is UnaryExpression u1 &&
        u1.Operand is NullableExpression n1 &&
        n1.Operand is UnaryExpression u2 &&
        u2.Operand is ColumnExpression c)
        e = c;

    return new TypeMaterializationInfo(
        queryType,
        e.FindProperty(queryType),
        Dependencies.TypeMappingSource);
}))

{
yield return typeMaterializationInfo;
}
```

It actually looks like RelationalExpressionExtensions.UnwrapNullableExpression(...) does just that, but it isn't used in RelationalExpressionExtensions.FindProperty(...).

The NullableExpression is (presumably) introduced as part of the LEFT JOIN, which would explain why this doesn't occur with INNER JOIN relationships.

I put together a branch to reproduce this within the test suite: https://github.com/austindrenski/Npgsql.EntityFrameworkCore.PostgreSQL/tree/efcore-12834.

[smitpatel]

Can close this as duplicate of https://github.com/aspnet/EntityFrameworkCore/issues/13025
I am adding code to unwrap nullableExpression

[austindrenski]

@smitpatel Much appreciated!

[JakenVeina]

Confirming this is fixed in 2.1.4.