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

Change tracking throws InvalidCastException when enum is bound to tinyint in Sqlite #16111

Closed
ArrowCase opened this issue Jun 15, 2019 · 5 comments · Fixed by #16851
Closed
Assignees
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Milestone

Comments

@ArrowCase
Copy link

ArrowCase commented Jun 15, 2019

Versions

Windows 10
Visual Studio 16.2 Preview 1
.NET Core SDK 3.0.100-preview6-012264
NETCore.App 3.0.0-preview6-27804-01
Microsoft.EntityFrameworkCore.Sqlite 3.0.0-preview6.19304.10

Steps to reproduce

  • Create a project using EF Core with Sqlite.
  • Create an entity class with an enum property whose base type is anything other than long.
  • Use annotations or fluent calls to configure the enum to use a tinyint column.
  • Create a DbContext with that entity.
  • Add a new entity and call SaveChanges().

Full repro gist here.

Note: this bug reproduces if the base type of the enum is explicitly set to something other than long, or if it is left as the implicit default of int.

Expected result

The record is created with no errors.

Actual result:

The database is created successfully with a tinyint (INTEGER affinity) data type, but attempting to save changes after adding the entity fails:

System.InvalidCastException: 'Unable to cast object of type 'EnumRepro.State' to type 'System.Int64'.'
   at Microsoft.EntityFrameworkCore.ChangeTracking.ValueComparer`1.Equals(Object left, Object right)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.ChangeDetector.LocalDetectChanges(InternalEntityEntry entry)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.ChangeDetector.DetectChanges(IStateManager stateManager)
   at Microsoft.EntityFrameworkCore.ChangeTracking.ChangeTracker.DetectChanges()
   at Microsoft.EntityFrameworkCore.DbContext.TryDetectChanges()
   at Microsoft.EntityFrameworkCore.DbContext.SaveChanges(Boolean acceptAllChangesOnSuccess)
   at Microsoft.EntityFrameworkCore.DbContext.SaveChanges()

It seems strange that change tracking needs to cast the enum to Int64 to compare it, but it's even stranger that the cast can somehow fail.

@ajcvickers
Copy link
Member

@ArrowCase SQLite doesn't have a column type tinyint. SQLite will let you specify anything, but it doesn't mean anything unless it's one of the four types that it understands. Is there a reason you are specifying tinyint on SQLite?

Note for triage: this happens because when picking a value conversion we reject it if it would not result in the correct mapping for the type specified. But since in this case the type specified is never correct, we will never accept the conversion. I'm not sure there is a good way to deal with this, but maybe we can have one behavior for store types we know, and a different one for store types we don't know.

@ArrowCase
Copy link
Author

@ajcvickers

Is there a reason you are specifying tinyint on SQLite?

My code was originally written for LocalDB, and I switched providers to Sqlite thinking I shouldn't have to worry about the column annotations since Sqlite understands those data types even though it doesn't actually use them. In this case, the enum column is created just fine with INTEGER affinity.

It seems like a bad experience that the tinyint annotation is acceptable for creating and reading from the column, but not for updating it, due to this issue in change tracking.

@ajcvickers
Copy link
Member

@ArrowCase Thanks for the additional info.

Note from triage: we should discuss with @bricelam

@ajcvickers
Copy link
Member

@bricelam to explain the difference between "supported types" and "type affinity".

@ajcvickers ajcvickers added this to the 3.0.0 milestone Jun 24, 2019
@ajcvickers ajcvickers self-assigned this Jun 24, 2019
@bricelam
Copy link
Contributor

SQLite supports four types: INTEGER, REAL, TEXT & BLOB. It uses a type affinity to determine the preferred way the data for a column should be stored on disk. But it also supports dynamic typing, so if converting to that type affinity would result in data loss, it won't be used.

We use those same affinity rules here to determine a suitable CLR type when reverse engineering a model. (#8824 is about picking an even better CLR type by sampling the data)

@ajcvickers ajcvickers modified the milestones: 3.0.0, 3.0.0-preview7 Jul 2, 2019
ajcvickers added a commit that referenced this issue Jul 30, 2019
Fixes #16111

The issue was that when finding a type mapping based on type name affinity we were not checking that it was compatible with the CLR type. This resulted in short-circuiting of the conversion selection and hence the wrong mapping.
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jul 30, 2019
ajcvickers added a commit that referenced this issue Jul 30, 2019
Fixes #16111

The issue was that when finding a type mapping based on type name affinity we were not checking that it was compatible with the CLR type. This resulted in short-circuiting of the conversion selection and hence the wrong mapping.
ajcvickers added a commit that referenced this issue Jul 31, 2019
Fixes #16111

The issue was that when finding a type mapping based on type name affinity we were not checking that it was compatible with the CLR type. This resulted in short-circuiting of the conversion selection and hence the wrong mapping.
ajcvickers added a commit that referenced this issue Jul 31, 2019
Fixes #16111

The issue was that when finding a type mapping based on type name affinity we were not checking that it was compatible with the CLR type. This resulted in short-circuiting of the conversion selection and hence the wrong mapping.
@ajcvickers ajcvickers modified the milestones: 3.0.0, 3.0.0-preview9 Aug 21, 2019
@ajcvickers ajcvickers modified the milestones: 3.0.0-preview9, 3.0.0 Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants