-
Notifications
You must be signed in to change notification settings - Fork 228
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
Comments
This should definitely work, I'll try to take a look in the coming days. |
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: Hopefully this helps |
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. |
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 If the parameter values were available in Another idea that comes to mind would be to make changes at the Npgsql level and relax @roji Any thoughts? |
@austindrenski can you please confirm that this error only occurs when the enum is mapped to a PostgreSQL enum? In other words, that 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 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. |
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. |
@roji wrote:
The test was using mapped enums, so I updated #490 to use an unmapped enum for
I'm working on a change in /// <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 -- 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:
I was surprised by this, since the test cases do not cast to [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:
I think a proper solution that handles @onimoger example, and doesn't break mapped enums will need to address this from @roji wrote:
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:
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:
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:
Thank you for additional clarification. We are indeed talking about a couple of different use cases:
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:
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:
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 wrote:
#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:
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. |
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:
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.
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. |
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 |
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. |
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. |
Sure, I can take a look at this today. |
@roji wrote:
I think that's very sensible, and a great deal simpler than supporting both at once! @roji wrote:
(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, When EF Core finds 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 -- 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");
}
} |
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. |
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. |
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. |
@onimoger Changing the model property from Can you post a snippet showing the configuration of your |
@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? |
@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): |
OK, that's good to know and it makes some 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.
Although it's a bit confusing, it does make some sense: when you specify a literal/constant (e.g. Let's wait for @austindrenski's confirmation that the issue exists also in Sqlite/SQL Server and see where to go from there. |
I was unable to reproduce the issue in SQLite or LocalDB... So it must be something on our end. |
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:
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 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 wrote:
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 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:
@roji wrote:
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). |
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) this is the command generated by EFE while inserting two objects to DB:
Should the problem be solved on the side of EFE or Npgsql? Npgsql 3.2.7 works fine. |
@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? |
https://github.com/purple-technology/EFE-issue
|
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? |
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.... |
@quicoli Could you open a new issue with that description and some version info? |
@austindrenski done :) |
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:
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.
The text was updated successfully, but these errors were encountered: