Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Where queries with casted enum value throw Can't write CLR type #489

Closed
onimoger opened this issue Jun 27, 2018 · 32 comments
Closed

Where queries with casted enum value throw Can't write CLR type #489

onimoger opened this issue Jun 27, 2018 · 32 comments
Assignees

Comments

@onimoger
Copy link

onimoger commented Jun 27, 2018

After upgrading to version 2.1 started getting errors with such queries:
var t = VehicleTypes.Truck;
context.Vehicles.Where(x => x.TypeId == (int)t)
throwing exception:

System.InvalidCastException: Can't write CLR type VehicleTypes with handler type Int32Handler

If code is changed to:
int t = (int)VehicleTypes.Truck;
context.Vehicles.Where(x => x.TypeId == t)
it works.

I guess it's because of new strict parameter type handling, but is this really the place for it trigger error? The enum value is supplied with a cast to correct type.
Is this intended? It would require a lot of refactoring in our project.

@roji
Copy link
Member

roji commented Jun 27, 2018

This should definitely work, I'll try to take a look in the coming days.

@onimoger
Copy link
Author

My hasty example was wrong. Error only occurs when enum in Where condition comes from a variable.

I have created a console app with exact reproduction of this problem:
https://github.com/onimoger/npgsql4repro

Hopefully this helps

@austindrenski
Copy link
Contributor

@onimoger Thanks for providing that repro, I was able to replicate this issue in our test suite.

@roji I opened #490 to add this to the test suite (currently fails). If you want to tackle this one, I can set the test to skip for the merge. Otherwise, I can take a look this evening.

@austindrenski austindrenski added the bug Something isn't working label Jun 27, 2018
@austindrenski austindrenski added this to the 2.1.1 milestone Jun 27, 2018
@roji
Copy link
Member

roji commented Jun 27, 2018

Nope, go ahead and take this if you want! The idea is to release 2.1.1 (and Npgsql 4.0.1) this weekend (probably Sunday). If you get stuck feel free to ping me.

@austindrenski
Copy link
Contributor

I took a look and it gets interesting:

The type cast is recorded in the expression tree as expected, so it does go through the normal visitor process. But since PostgreSQL doesn't support casting enums to numeric values, the normal type cast handling generates SQL that PostgreSQL will reject.

I'm not sure if there is a good option from the EF Core side. I can't seem to find access to parameter values before NpgsqlQuerySqlGenerator, and by that point, changing the value or type of a hoisted variable sounds troublesome (e.g. is the original value used elsewhere? should we create a new parameter? embed a literal?).

If the parameter values were available in NpgsqlSqlTranslatingExpressionVisitor, then perhaps we could cast to the underlying type and pass back a ConstantExpression. But so far I haven't found a way to access them from inside this visitor.

Another idea that comes to mind would be to make changes at the Npgsql level and relax NpgsqlSimpleTypeHandler<T> to support enums when their underlying type matches the generic parameter. This sounds like it could be useful, but I'm not sure how it would impact the larger type handler/resolution system.

@roji Any thoughts?

@roji
Copy link
Member

roji commented Jun 28, 2018

@austindrenski can you please confirm that this error only occurs when the enum is mapped to a PostgreSQL enum? In other words, that NpgsqlConnection.GlobalTypeMapper.MapEnum() is being called? Unmapped enums are automatically mapped by EF Core to database int columns, and in this case the above code should be working correctly - it would be good if you could confirm.

If this is the situation, then I think the error is expected behavior. On disk, PostgreSQL stores enums values as an int, but the wire representation of enums (when sending/receiving values) is actually the string value - this is why we are able to handle unmapped enums as strings. This means that there is no way for us to send an integer value to PostgreSQL for storage as an enum: PostgreSQL simply does not support that. That rules out changing the Npgsql ADO.NET handler for enums to support integers.

It's also unclear for me why one would map a CLR enum to a PostgreSQL enum, and then want to interact with it as an integer. The whole point of mapping CLR enums to PostgreSQL enums is that you don't deal with the underlying integral value.

More generally, when you put code inside an IQueryable, that means that in principle you're asking for the expression to be evaluated at the server. So in the code fragment context.Vehicles.Where(x => x.TypeId == (int)t), assuming t is an enum type, you're literally asking PostgreSQL to cast an enum to an int, which simply isn't supported. So it actually makes sense for me that this throws. The exception itself is a little strange/unexpected: I would have preferred EF Core to translate the cast to a server-side cast, and for that to generate a PostgreSQL error. I suspect this has to do with some specific enum handling inside EF Core. But that isn't important enough IMHO.

A final note on complex fragment translations... it's probably theoretically possible to perform complex expression tree fragment translation, detect the case of a mapped enum being cast to an integral type, resolve that back to the enum value for that integral type and send that. However, as a matter of principle I prefer we avoid doing that unless there's real value in it - it makes the provider much more complex, usually involves interacting with internal EF Core APIs which may change, etc. etc. This is also part of the reason why I'm a bit reluctant to continue in the direction of #444/#431... It's definitely not a an absolute no or anything, just that we need to have a clear demonstration of value (and #431 has the null behavior discrepancy issue which seems to stand in the way).

Let me know what you think.

@onimoger
Copy link
Author

If i understand what your are talking about, my 2 cents: this does not involve Postgres enums in any way. The DB column is a simple int column. The enum exsists only in C#, that is why it is casted to int when passed into expression. There are no enums in the DB.

@austindrenski
Copy link
Contributor

austindrenski commented Jun 28, 2018

@roji wrote:

@austindrenski can you please confirm that this error only occurs when the enum is mapped to a PostgreSQL enum? In other words, that NpgsqlConnection.GlobalTypeMapper.MapEnum() is being called? Unmapped enums are automatically mapped by EF Core to database int columns, and in this case the above code should be working correctly - it would be good if you could confirm.

The test was using mapped enums, so I updated #490 to use an unmapped enum for Where_with_parameter_downcast(). Unfortunately, it still draws the exception:

System.InvalidCastException : 
  Can't write CLR type Npgsql.EntityFrameworkCore.PostgreSQL.Query.EnumQueryTest+UnmappedEnum
  with handler type Int32Handler

I'm working on a change in NpgsqlSqlTranslatingExpressionVisitor to force the SQL generation for enums that are down-cast to their underlying system type:

/// <inheritdoc />
protected override Expression VisitUnary(UnaryExpression expression)
{
    if (expression.NodeType is ExpressionType.Convert &&
        expression.Operand.Type.IsEnum &&
        Enum.GetUnderlyingType(expression.Operand.Type) == expression.Type &&
        Visit(expression.Operand) is Expression e)
        return new ExplicitCastExpression(e, expression.Type);

    return base.VisitUnary(expression);
}

This works for unmapped enums, generating the following SQL which is accepted by PostgreSQL. In this case, the cast expression isn't actually needed, since we sent the parameter as Int32. But it seems to disrupt a later stage, avoiding the InvalidCastException. (Basically, addressing the symptoms, not the cause...)

-- The parameter type is right, so handlers take it
(DbType = Int32)

-- The SQL syntax is right, though redundant
WHERE e."EnumValue" = CAST(@__sad_0 AS integer)

Unfortunately, that causes mapped enums to draw the following exception when used in binary comparisons:

Npgsql.PostgresException : 42846: cannot cast type mapped_enum to integer

I was surprised by this, since the test cases do not cast to int....at least in the source code:

[Fact]
public void Where_with_parameter()
{
    using (var ctx = CreateContext())
    {
        var sad = MappedEnum.Sad;
        var x = ctx.SomeEntities.Single(e => e.MappedEnum == sad);
        Assert.Equal(MappedEnum.Sad, x.MappedEnum);
        AssertContainsInSql("(DbType = Object)");
        AssertContainsInSql("WHERE e.\"MappedEnum\" = @__sad_0");
    }
}

However, in the expression tree, we can see that enums are cast to their underlying system types before comparison:

(Convert([e].MappedEnum, Int32) == Convert(__sad_0, Int32))

I think a proper solution that handles @onimoger example, and doesn't break mapped enums will need to address this from NpgsqlSqlTranslatingExpressionVisitor.VisitBinary(...), rather than in NpgsqlSqlTranslatingExpressionVisitor.VisitUnary(...) where we can't tell if the cast is user-specified or system-specified.


@roji wrote:

If this is the situation, then I think the error is expected behavior. On disk, PostgreSQL stores enums values as an int, but the wire representation of enums (when sending/receiving values) is actually the string value - this is why we are able to handle unmapped enums as strings. This means that there is no way for us to send an integer value to PostgreSQL for storage as an enum: PostgreSQL simply does not support that. That rules out changing the Npgsql ADO.NET handler for enums to support integers.

I agree with all of this, but I think we might be talking about different issues.

I didn't mean to suggest that we modify the enum handlers, but rather that the integral type handlers be relaxed to support an enum whose underlying system type is supported. (Though, this could be untenable from an implementation or maintenance perspective.)

But it does sound reasonable from a type system perspective. While down-casting enums is never supported in PostgreSQL, it is—setting aside opinions of whether one ought to do so—a common-enough operation in C#.

@roji wrote:

It's also unclear for me why one would map a CLR enum to a PostgreSQL enum, and then want to interact with it as an integer. The whole point of mapping CLR enums to PostgreSQL enums is that you don't deal with the underlying integral value.

I could imagine a few scenario where the CLR enum is mapped to a database enum, but the CLR enum has explicit values assigned that need to occasionally be accessed (cast on the client) and sent as parameter of a query. For example, the underlying values could:

  • map to an external data store,
  • provide compatibility to a legacy database, or
  • be used during migration from an RDBMS that didn't support enums to PostgreSQL.

None of this is to say that Npgsql needs to support this behavior, just that there could be cases in the wild where this behavior is both intentional and useful.

@onimoger wrote:

If i understand what your are talking about, my 2 cents: this does not involve Postgres enums in any way. The DB column is a simple int column. The enum exsists only in C#, that is why it is casted to int when passed into expression. There are no enums in the DB.

Thank you for additional clarification. We are indeed talking about a couple of different use cases:

  1. CLR enums mapped to PostgreSQL enums for use as enums
  2. CLR enums not mapped to PostgreSQL for use as integral types
  3. CLR enums mapped to PostgreSQL for use as both enums and integral types (separate from the mapped enum in the database)

It sounds like you are in the second case. We expect both the first and the second cases to work, with the third case being nice to have.

The root of the exception you're seeing seems to be related to the way in which the CLR handles binary comparisons on enums (i.e. casting to the underlying system type). I think the minimal outcome of this issue and #490 should be to ensure that unmapped enums are supported in binary comparisons. (Currently, it seems that this is not the case.)


@roji wrote:

More generally, when you put code inside an IQueryable, that means that in principle you're asking for the expression to be evaluated at the server. So in the code fragment context.Vehicles.Where(x => x.TypeId == (int)t), assuming t is an enum type, you're literally asking PostgreSQL to cast an enum to an int, which simply isn't supported. So it actually makes sense for me that this throws. The exception itself is a little strange/unexpected: I would have preferred EF Core to translate the cast to a server-side cast, and for that to generate a PostgreSQL error. I suspect this has to do with some specific enum handling inside EF Core. But that isn't important enough IMHO.

I agree that it would be desirable to force this translation and draw the error from PostgreSQL. This would be the best case result for #490 (once the casting issues are dealt with).


@roji wrote:

A final note on complex fragment translations... it's probably theoretically possible to perform complex expression tree fragment translation, detect the case of a mapped enum being cast to an integral type, resolve that back to the enum value for that integral type and send that. However, as a matter of principle I prefer we avoid doing that unless there's real value in it - it makes the provider much more complex, usually involves interacting with internal EF Core APIs which may change, etc. etc. This is also part of the reason why I'm a bit reluctant to continue in the direction of #444/#431... It's definitely not a an absolute no or anything, just that we need to have a clear demonstration of value (and #431 has the null behavior discrepancy issue which seems to stand in the way).

I think we're on the same page with this. When I have some time to circle back to #431, I'll try to address the concerns raised in #460 by reducing/eliminating the fragment translations. I still think they're a useful approach, but as you say, they do make things more complex. Perhaps they'll provide a better value proposition in future versions of EF Core.

@austindrenski austindrenski self-assigned this Jun 28, 2018
@austindrenski
Copy link
Contributor

@austindrenski wrote:

I think a proper solution that handles @onimoger example, and doesn't break mapped enums will need to address this from NpgsqlSqlTranslatingExpressionVisitor.VisitBinary(...), rather than in NpgsqlSqlTranslatingExpressionVisitor.VisitUnary(...) where we can't tell if the cast is user-specified or system-specified.

#490 now passes both the previous enum tests and the repro for this issue. It does still need some additional tests to make sure the translation is strictly limited to unmapped enums cast to their exact underlying system type and used in an equality comparison and nothing else.

@roji wrote:

A final note on complex fragment translations... it's probably theoretically possible to perform complex expression tree fragment translation, detect the case of a mapped enum being cast to an integral type, resolve that back to the enum value for that integral type and send that. However, as a matter of principle I prefer we avoid doing that unless there's real value in it

I've tried to keep this in mind while working on #490, keeping the translation as narrow as possible to limit the complexity (and specifically limiting it to unmapped enums).

That said, this was a fair bit more complicated than I anticipated, so I'm not sure how I would rate the value/risk of introducing this feature.

If you think it's worthwhile to support this type of casting on unmapped enums, then #490 is close to what we need. But if not, we could always shelve it and reconsider it at a later release. I'm good either way.

@roji
Copy link
Member

roji commented Jun 29, 2018

Apologies, I was under the impression the issues where with mapped enums. It's definitely more serious/interesting if this happens to unmapped enums, which EF Core maps to int columns. I agree we need to make sure that binary comparison of unmapped ints works.

But before going into fragment translation, I have a much simpler question - how is this working on other providers (e.g. SQL Server)? After all, they're handling unmapped enums as ints, since they don't support anything else. So either SQL Server also experiences this issue (in which case it's better raised to the EF Core team for handling in the core), or it doesn't and there's something we're doing in Npgsql to trigger this bug (in which case we should investigate that and find the root cause rather than work around it via fragment translation). Any chance you can look into this @austindrenski? Note that you don't need a full-fledged SQL Server installation, you can either use LocalDB (which is an SQL Server-compatible lightweight local db that comes pre-installed) or possibly even SQLite.

Some responses to the comments above:

I didn't mean to suggest that we modify the enum handlers, but rather that the integral type handlers be relaxed to support an enum whose underlying system type is supported. (Though, this could be untenable from an implementation or maintenance perspective.)

To me this doesn't make any more sense than modifying the enum handlers. Once again, PostgreSQL does not transferring enums over the wire as ints (even if it does store them internally as ints on disk). So as far as I can tell the Npgsql ADO.NET is fine and doesn't need any modification, any issue here is at the EF Core layer.

It's also unclear for me why one would map a CLR enum to a PostgreSQL enum, and then want to interact with it as an integer. The whole point of mapping CLR enums to PostgreSQL enums is that you don't deal with the underlying integral value.

I could imagine a few scenario where the CLR enum is mapped to a database enum, but the CLR enum has explicit values assigned that need to occasionally be accessed (cast on the client) and sent as parameter of a query. For example, the underlying values could:

map to an external data store,
provide compatibility to a legacy database, or
be used during migration from an RDBMS that didn't support enums to PostgreSQL.

I'm still not seeing why the enum would need to be sent as an integer to PostgreSQL (as opposed to some other EF Core provider or whatever). But in any case we don't have a request to support this, so as you suggest above we can drop it at least for now.

@onimoger
Copy link
Author

I'm still not seeing why the enum would need to be sent as an integer to PostgreSQL

Because it's CLR enum, and in DB it's an int column referencing another table. Granted, this practice comes from using EF with Sql Server. Postgres enums are not used because of decision to keep with generic sql as much as possible, in case we need to change DB. If this would not have worked from the start, i would say ok, we can work around it, but it works fine with Npgsql 3.2.7

We have a system running with lots of such code, and this is a blocker for us, preventing upgrade to .net core 2.1
Refactoring is complicated, since, unfortunately, we do not have 100% test coverage, and this error only manifests during runtime.

@roji
Copy link
Member

roji commented Jun 29, 2018

I'm still not seeing why the enum would need to be sent as an integer to PostgreSQL

Because it's CLR enum, and in DB it's an int column referencing another table. Granted, this practice comes from using EF with Sql Server. Postgres enums are not used because of decision to keep with generic sql as much as possible, in case we need to change DB. If this would not have worked from the start, i would say ok, we can work around it, but it works fine with Npgsql 3.2.7

I think there may be some confusion...

I completely agree that Npgsql needs to fully support unmapped enums, i.e. mapping CLR enums to database integer columns. This is the default mode in EF Core (since PostgreSQL is the only DB AFAIK that supports database enums), and we need to resolve this issue which is preventing comparison.

My comment above was that if an enum is mapped to a PostgreSQL enum, then it's expected for it to always be sent as a PostgreSQL enum and not as an integer. In other words, I don't think it should be a goal to support both modes at the same time, mapping to PostgreSQL enums and to ints in the same model/application. This is what @austindrenski described in his comment above as option 3.

So rest assured that we will do what is needed to support unmapped enums (enums as ints) for 4.0.1 or 4.0.2 at the latest. As I wrote above, the best way forward in my opinion is to understand where the root issue is coming from, i.e. is it an EF Core issue (unlikely) or something we do in the Npgsql provider which breaks comparisons for unmapped enums.

@roji
Copy link
Member

roji commented Jun 29, 2018

One last note regarding cross-database portability... It should be totally fine to map CLR enum to PostgreSQL enums on PostgreSQL, but to integers on other database (e.g. SQL Server); EF Core was built with this idea in mind. You can have the same model map in one way on one database, and in another way on another database. The same LINQ query should work just fine on PostgreSQL and on SQL Server.

So the fact that you need to support enums in SQL Server doesn't mean you can't use PostgreSQL enums.

@austindrenski
Copy link
Contributor

But before going into fragment translation, I have a much simpler question - how is this working on other providers (e.g. SQL Server)? After all, they're handling unmapped enums as ints, since they don't support anything else. So either SQL Server also experiences this issue (in which case it's better raised to the EF Core team for handling in the core), or it doesn't and there's something we're doing in Npgsql to trigger this bug (in which case we should investigate that and find the root cause rather than work around it via fragment translation). Any chance you can look into this @austindrenski? Note that you don't need a full-fledged SQL Server installation, you can either use LocalDB (which is an SQL Server-compatible lightweight local db that comes pre-installed) or possibly even SQLite.

Sure, I can take a look at this today.

@austindrenski
Copy link
Contributor

austindrenski commented Jun 29, 2018

@roji wrote:

In other words, I don't think it should be a goal to support both modes at the same time, mapping to PostgreSQL enums and to ints in the same model/application.

I think that's very sensible, and a great deal simpler than supporting both at once!

@roji wrote:

As I wrote above, the best way forward in my opinion is to understand where the root issue is coming from, i.e. is it an EF Core issue (unlikely) or something we do in the Npgsql provider which breaks comparisons for unmapped enums.

More generally, when you put code inside an IQueryable, that means that in principle you're asking for the expression to be evaluated at the server. So in the code fragment context.Vehicles.Where(x => x.TypeId == (int)t), assuming t is an enum type, you're literally asking PostgreSQL to cast an enum to an int, which simply isn't supported. So it actually makes sense for me that this throws.

(emphasis added)

I think this just clicked for me, and I think you've been right that this is all expected behavior. I'll test with SQL Server to confirm and report back.

EF Core lifts enums out of the cast expressions on purpose. Since the default behavior is to send unmapped enums as integers, this makes sense. The default handlers must write enums to the wire as integers, so the expression tree casting isn't necessary.

We use this default behavior for unmapped enums too. So if @onimoger modeled his entity property, TypeId, with his CLR enum, VehicleType, he wouldn't need the cast at all. EF Core would simply understand the unmapped enum to be an abstraction over an integer.

When EF Core finds VehicleType cast to an enum, it can't differentiate between the normal expression tree representation and his explicit cast. So it lifts the enum out of the cast entirely. But since VehicleType is unmapped, the normal EF Core behavior uses the Int32Handler, which complains about the enum.

If my understanding here is correct, then #490 should be closed and we don't need to make any changes. (Except perhaps to explain this in the documentation for enum mappings.)

@roji Does that sound right to you?

@onimoger Is there any reason that you can't model your type_id integer column with the unmapped CLR enum?

-- no modifications here
CREATE TABLE (
    -- other columns
    type_id INTEGER
);
// leave VehicleType unmapped, but change this:
public class OldEntity
{
    // other properties
    
    public int TypeId { get; set; }
}

// to this:
public class NewEntity
{
    // other properties
    
    public VehicleType TypeId { get; set; }
}

// before 
VehicleType t = VehicleType.Suv;
context.Vehicles.Where(x => x.TypeId == (int)t);

// after
VehicleType t = VehicleType.Suv;
context.Vehicles.Where(x => x.TypeId == t);

For clarity, these tests pass without any modifications to the provider:

[Fact]
public void Where_with_unmapped_enum_parameter_downcasts_are_implicit()
{
    using (var ctx = CreateContext())
    {
        var sad = UnmappedEnum.Sad;
        var _ = ctx.SomeEntities.Single(e => e.UnmappedEnum == sad);

        AssertContainsInSql("@__sad_0='?' (DbType = Int32)");
        AssertContainsInSql("WHERE e.\"UnmappedEnum\" = @__sad_0");
    }
}

[Fact]
public void Where_with_unmapped_enum_parameter_downcasts_do_not_matter()
{
    using (var ctx = CreateContext())
    {
        var sad = UnmappedEnum.Sad;
        var _ = ctx.SomeEntities.Single(e => (int)e.UnmappedEnum == (int)sad);

        AssertContainsInSql("@__sad_0='?' (DbType = Int32)");
        AssertContainsInSql("WHERE e.\"UnmappedEnum\" = @__sad_0");
    }
}

[Fact]
public void Where_with_mapped_enum_parameter_downcasts_do_not_matter()
{
    using (var ctx = CreateContext())
    {
        var sad = MappedEnum.Sad;
        var _ = ctx.SomeEntities.Single(e => (int)e.MappedEnum == (int)sad);

        AssertContainsInSql("@__sad_0='?' (DbType = Object)");
        AssertContainsInSql("WHERE e.\"MappedEnum\" = @__sad_0");
    }
}

@onimoger
Copy link
Author

@onimoger Is there any reason that you can't model your type_id integer column with the unmapped CLR enum?

I guess it was because db was modeled before the enum was introduced to make C# code clearer when "hardcoded" type id's became required for business logic.

I will have to try it out then.

@roji
Copy link
Member

roji commented Jun 29, 2018

I think this just clicked for me, and I think you've been right that this is all expected behavior. I'll test with SQL Server to confirm and report back.

EF Core lifts enums out of the cast expressions on purpose. Since the default behavior is to send unmapped enums as integers, this makes sense. The default handlers must write enums to the wire as integers, so the expression tree casting isn't necessary.

I'm actually not 100% sure yet, but yeah, I think your description is more or less correct. I'm still curious to see what will happen on SQL Server. If it works there, that means we've done something special in Npgsql to make it fail and probably want to fix it. Otherwise if it fails there as well it may be worth raising to the EF Core team, but I do think it's reasonable for this not to work.

More generally, there may be other types of casts that are valid in C# but invalid in your particular database. We may have types A and B which have an explicit cast operator in C#, but which PostgreSQL (or some other database) refuses to cast; in this case the C# expression will be valid and get compiled, but throw in runtime just like the attempt to instruct PostgreSQL to cast an enum to an int.

@onimoger
Copy link
Author

Unfortunately changing model property from int to enum causes problems. EF looses track that this property is a foreign key and auto creates new property "VehicleTypeId1". Guess for that to work VehicleType Id column should also be changed into enum. Basically a chain reaction of refactoring.

Looks like i can't escape from the fact that enums will have to be removed from all queries.

@austindrenski
Copy link
Contributor

austindrenski commented Jun 30, 2018

@onimoger Changing the model property from int to enum is just CLR modeling, there shouldn't have any effect on the database... It sounds like you may be using convention-based foreign keys, and may have other model properties that need to be updated to enum as well.

Can you post a snippet showing the configuration of your DbContext?

@roji
Copy link
Member

roji commented Jun 30, 2018

@onimoger it does make sense that if you change a column's type from int to enum, and that column participates in a foreign key relationship, then you need to change the column type at the other end as well... Can you confirm that doing that fixes the issue?

@onimoger
Copy link
Author

@austindrenski Yes, it's convention based foreign keys.

@roji Yes, it fixes the issue.

So it's either changing to enums everywhere in the model or removing enums from queries.

I understand that this works as intended, but personally I can't think of a reason when passing this cast operation to Postgres would make sense. Also, casting the enum directly (without variable):
(int)VehicleTypes.Suv
does not throw. This is confusing in my opinion.

@roji
Copy link
Member

roji commented Jun 30, 2018

So it's either changing to enums everywhere in the model or removing enums from queries.

OK, that's good to know and it makes some sense.

I understand that this works as intended, but personally I can't think of a reason when passing this cast operation to Postgres would make sense.

I actually think it could make sense to specifically apply this type of cast client side, as a sort of special hard-coded exception in EF Core. However, this would happen in EF Core rather than in the Npgsql provider. Once @austindrenski verifies what the behavior in other providers is, we can consider submitting an issue to EF Core for this.

Also, casting the enum directly (without variable): (int)VehicleTypes.Suv does not throw. This is confusing in my opinion.

Although it's a bit confusing, it does make some sense: when you specify a literal/constant (e.g. VehicleTypes.Suv) EF Core evaluates it once at the client-side when generating the query model. However, when a variable is used, EF Core must generate a model with a parameter which will only be assigned when the query is actually executed. This is why the cast is currently performed server-side.

Let's wait for @austindrenski's confirmation that the issue exists also in Sqlite/SQL Server and see where to go from there.

@austindrenski
Copy link
Contributor

austindrenski commented Jul 1, 2018

I was unable to reproduce the issue in SQLite or LocalDB... So it must be something on our end.

@roji
Copy link
Member

roji commented Jul 1, 2018

I looked into this, and the Microsoft.Data.Sqlite ADO.NET provider accepts CLR enums and unwraps them into ints (I assume the SQL Server provider does the same). So this has nothing to do with EF Core, which simply hands off the enum to the underlying ADO.NET provider; Npgsql just happens not to support this. Unless I'm mistaken that means the cast to int is actually sent to the server (haven't inspected the SQL), and ends up doing nothing because the value is already an int.

We could think about making Npgsql (ADO.NET) do the same (as @austindrenski proposed above). This would mean the following behavior:

  1. If you map the enum (recommended), the CLR enum type always gets sent as the PostgreSQL enum.
  2. If you don't map the enum but specify DataTypeName, Npgsql still sends the PostgreSQL enum.
  3. If you don't map the enum and don't specify DataTypeName, Npgsql sends an integer (or smallint or bigint). This would be the new behavior.

To be honest, this seems pretty confusing, and doesn't fit with the strict, strongly-typed behavior that both PostgreSQL and Npgsql implement. We're currently asking users to perform the cast (unwrapping the enum) outside the driver, rather than doing it for them inside the driver, just like we avoid doing other types of casts and conversions. It would also confuse first-time users who want to use PostgreSQL enums but getting integers (or integer-related errors) because they haven't properly mapped or set DataTypeName).

I understand that this gets a bit more complicated with EF Core specifically, because unwrapping outside the driver is a bit less nice. But this still seems reasonable to me, and is unfortunately a small database portability issue.

I'm going to go ahead and close this for now, but am of course open to hearing opinions to the contrary.

@roji roji closed this as completed Jul 1, 2018
@roji roji removed this from the 2.1.1 milestone Jul 1, 2018
@roji roji removed the bug Something isn't working label Jul 1, 2018
@austindrenski
Copy link
Contributor

@roji wrote:

We could think about making Npgsql (ADO.NET) do the same (as @austindrenski proposed above)

To be honest, this seems pretty confusing, and doesn't fit with the strict, strongly-typed behavior that both PostgreSQL and Npgsql implement.

I'm still up in the air on this. I have a strong preference for the existing strict, strongly-typed behavior. (Even if I tend to propose otherwise while brainstorming...)

If there's a change to make, I lean towards encapsulating it in the provider (EF Core). It doesn't sound like users of the driver (ADO.NET) have asked for a change, and that any such change could impact them.

@roji wrote:

I understand that this gets a bit more complicated with EF Core specifically, because unwrapping outside the driver is a bit less nice. But this still seems reasonable to me, and is unfortunately a small database portability issue.

I might even suggest that users should take needing to unwrap outside the driver as a design hint.

This would seem to align with the EF Core Guiding Principles, specifically:

  • EF Core does not hide the type of database you are targeting. We are not building a magical abstraction where the same model can transparently target different types of providers.
    ...
  • EF Core favors an opinionated "this is how you do things" approach over "we support any mapping pattern".
    ...
  • EF Core allows you to easily circumvent O/RM features when you need to get close to the metal (e.g. drop down to SQL when LINQ doesn't work).
    ...
  • EF Core is pay-per-play (i.e. if you don't use SQL Server/commands/etc. then you don't download, reference, deploy, or load the binaries)

@roji wrote:

I'm going to go ahead and close this for now, but am of course open to hearing opinions to the contrary.

We could apply the client-side cast, but it would complicate the story of translation and transmission. At the same time, it is also a complication to have differences in the behavior of literals and parameters.

Is this the type of issue where the EF Core team might have an opinion? As you mentioned above, this type of issue could arise in the future for other providers, so it might make sense to see if they have a preference on this issue.

Overall, I can get on board with any of these options. If you decide that this should be addressed in the provider, #490 is a possible solution (though, by no means a complete one at this point).

@jbrezina
Copy link

I use Npgsql 4.0.2 and Entity Framework Extensions and I still get the error while trying to write to DB any data containing enums mapped to DB as ints (C# classes use enums in model classes and DB use INTEGER)
Can't write CLR type ReportServerORM.Model.TransactionCodeState with handler type Int32Handler

this is the command generated by EFE while inserting two objects to DB:

INSERT  INTO "public"."TransactionCode"
("TransactionCodeID", "Name", "State")
SELECT  "TransactionCodeID", "Name", "State"
FROM    (
SELECT @0_0 AS "TransactionCodeID", @0_1 AS "Name", @0_2 AS "State", @0_3 AS ZZZ_Index
UNION ALL SELECT @1_0 AS "TransactionCodeID", @1_1 AS "Name", @1_2 AS "State", @1_3 AS ZZZ_Index
) AS StagingTable;

-- @0_0: -1 (Type = Int32, Size = -1)
-- @0_1: testing (Type = String, Size = 64)
-- @0_2: Invalid (Type = Int32, Size = -1)
-- @0_3: 0 (Type = Int32, Size = 0)
-- @1_0: 10 (Type = Int32, Size = -1)
-- @1_1: 10 testing (Type = String, Size = 64)
-- @1_2: Invalid (Type = Int32, Size = -1)
-- @1_3: 1 (Type = Int32, Size = 0)

-- Failed with error: Can't write CLR type ReportServerORM.Model.TransactionCodeState with handler type Int32Handler

Should the problem be solved on the side of EFE or Npgsql? Npgsql 3.2.7 works fine.

@austindrenski
Copy link
Contributor

@jbrezina Can you provide us with a minimal reproduction (in the form of a repo we can clone) showing your configuration and the code that produces the problematic command?

@jbrezina
Copy link

https://github.com/purple-technology/EFE-issue

  • update the connection string in TestingORM/DAL/TestingContext.cs
  • generate DB using the following command in Package Manager Console in VS
    EntityFramework\update-database -ProjectName TestingORM -StartUpProjectName TestingORM -Verbose
  • run - exception is thrown

@austindrenski
Copy link
Contributor

It looks like your project is using EF6, not EF Core.

<?xml version="1.0" encoding="utf-8"?>
<packages>
  <package id="EntityFramework" version="6.2.0" targetFramework="net462" />
  <package id="Npgsql" version="4.0.2" targetFramework="net461" />
  <package id="System.Runtime.CompilerServices.Unsafe" version="4.5.1" targetFramework="net461" />
  <package id="System.Threading.Tasks.Extensions" version="4.5.1" targetFramework="net461" />
  <package id="System.ValueTuple" version="4.5.0" targetFramework="net461" />
  <package id="Z.EntityFramework.Extensions" version="3.15.5" targetFramework="net461" />
</packages>

Would you mind opening this issue with the repo for the Npgsql EF6 provider?

@quicoli
Copy link

quicoli commented Jul 31, 2018

I'm using NHibernate in a .net core webapi and I get the same error. Before changing to postgresql I was using SQL Server and I hadn't this issue.

I have a table with a int column, and in my model this column is a Enum value. I can retrieve values now because of this error...

thanks guys....

@austindrenski
Copy link
Contributor

@quicoli Could you open a new issue with that description and some version info?

@quicoli
Copy link

quicoli commented Aug 1, 2018

@austindrenski done :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants