dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.75k stars 3.18k forks source link

Existing migration containing "geography" col type breaks after upgrading to 5.0 #23812

Closed joaopgrassi closed 2 years ago

joaopgrassi commented 3 years ago

We have an app that uses the Point type from NetTopologySuite (https://docs.microsoft.com/en-us/ef/core/modeling/spatial). We use SQL Server as our "prod" db. During our integration tests we use SQLite in memory.

The migrations are generated targeting SQL Server (UseSqlServer). The migration was created with a type geography for the Point column. Here is one example of a migration:

protected override void Up(MigrationBuilder migrationBuilder)
{
    migrationBuilder.CreateTable(
        name: "Resources",
        columns: table => new
        {
            Id = table.Column<Guid>(type: "uniqueidentifier", nullable: false),
            Location = table.Column<Point>(type: "geography", nullable: true) // -> geography type for sql server
        },
        constraints: table =>
        {
            table.PrimaryKey("PK_Resources", x => x.Id);
        });
}

During our integration tests we do stuff like this:

var originalDbContext = services.Single(
    d => d.ServiceType == typeof(DbContextOptions<AppContext>));
services.Remove(originalDbContext);

services.AddDbContext<ResourceDbContext>(options =>
{
    options.UseSqlite(_connection, x =>
    {
        x.MigrationsHistoryTable().UseNetTopologySuite();
    });
});

using (var scope = services.BuildServiceProvider().CreateScope())
{
    var scopedServices = scope.ServiceProvider;
    var db = scopedServices.GetRequiredService<ResourceDbContext>();

    db.Database.Migrate();
}

to get our db ready for the tests. All worked well using the NuGet packages for 3.1.x.

We started recently updating to EF Core 5 and we are facing this now: (No actual code/model changes)

Unhandled exception. System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
 ---> System.ArgumentException: Invalid geometry type: geography (Parameter 'storeType')
   at Microsoft.EntityFrameworkCore.Sqlite.Storage.Internal.SqliteGeometryTypeMapping`1.CreateWriter(String storeType)
   at Microsoft.EntityFrameworkCore.Sqlite.Storage.Internal.SqliteGeometryTypeMapping`1..ctor(NtsGeometryServices geometryServices, String storeType)

Then I started to look at the source code, and found where the exception is thrown: SqliteGeometryTypeMapping. There's no geography type in the switch case.

Repro

I created a sample app to reproduce the issue. The app uses SQLite in memory by default. https://github.com/joaopgrassi/efcore-sqlite-geometry-migration.

1 - This commit creates the initial migration, setting the column type manually to geography to mimic our case of it being created targeting SQL Server https://github.com/joaopgrassi/efcore-sqlite-geometry-migration/commit/6869b6080a60e76f9fee2d6b5d09d6f65e371046

you can run the app and it works. It inserts and queries the entity.

2 - The second commit I upgrade the packages to 5.0.0, https://github.com/joaopgrassi/efcore-sqlite-geometry-migration/commit/e982a47bd500ce521ec107be2efeeeb3e16a6286

Then when running the app you can see the exception.

Do you know what we could do to work around it? On EF 3.1 when querying the table it seems to translate to Point type of SQLite, so maybe just adding geometry to the switch case should work? This kind of blocks us from migrating to EF Core 5 atm =/.

Note: If I use EnsureCreated in the tests and change the model configuration to this: modelBuilder.Entity<Resource>().Property(x => x.Location).HasSrid(4326); it works (as it will use the Point type on SQLite). The problem is that we have views on migrations and they are not created so the tests fail.

ajcvickers commented 3 years ago

@joaopgrassi Migrations are provider-specific. That is, migrations created for SQL Server will not reliably work for creating a SQLite database. See #16406 for discussion of provider-specific migrations.

joaopgrassi commented 3 years ago

I see, thanks for the info @ajcvickers. I failed to see that there was this option: Migrations with Multiple Providers. Probably should have used it from the start since it looks pretty cool and fixes my problem.

I'm all in for provider specific migrations and simplification like discussed in #16404. I think it's the right path.

But: Since we already have several migrations that are more or like SQL Server specific, is there a way I could generate a new migration for Sqlite provider that would contain the current snapshot? Or I'm out of luck, and the only way is start all over? (which probably is not an option..)

For example:

// existing migrations for sql server
Migrations
  Init.cs
  AddField.cs
  AnotherField.cs

SqliteMigrations
  Init.cs => contains the three migrations above

After setting up the projects to be able to read the "provider" config and adding the appropriate provider, if I run this:

dotnet ef migrations add Test --project ../Data.Search.Ef --output-dir Migrations/SqliteMigrations -- --provider Sqlite

It fails as above, I guess since the existing migrations have the geography type which sqlite lacks.

System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
 ---> System.ArgumentException: Invalid geometry type: geography (Parameter 'storeType')
   at Microsoft.EntityFrameworkCore.Sqlite.Storage.Internal.SqliteGeometryTypeMapping`1.CreateWriter(String storeType)
   at Microsoft.EntityFrameworkCore.Sqlite.Storage.Internal.SqliteGeometryTypeMapping`1..ctor(NtsGeometryServices geometryServices, String storeType)
ajcvickers commented 3 years ago

@bricelam Is there an equivalent of the geography type when using SpatiaLite? (The code is currently doing .HasColumnType("geography") which fails.)

bricelam commented 3 years ago

There's only one set of types in SpatiaLite. Just set the column SRID instead: .HasSrid(4326)

joaopgrassi commented 3 years ago

Hi @bricelam , I saw this in another issue here. Adding .HasSrid(4326) to the entity's configuration doesn't change the error when running MigrateAsync during our tests. You can see it in the repro I mentioned initially.

Unless I need to do something else, apart from adding .HasSrid(4326)?

modelBuilder.Entity<Resource>()
    .Property(x => x.Location).HasColumnName("GpsMapLocation").HasSrid(4326);
using var db = new AppContext();
db.Database.Migrate(); // => fails
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor, Boolean wrapExceptions)
   at System.Reflection.RuntimeConstructorInfo.Invoke(BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.RuntimeType.CreateInstanceImpl(BindingFlags bindingAttr, Binder binder, Object[] args, CultureInfo culture)
   at System.Activator.CreateInstance(Type type, BindingFlags bindingAttr, Binder binder, Object[] args, CultureInfo culture, Object[] activationAttributes)
   at System.Activator.CreateInstance(Type type, Object[] args)
   at Microsoft.EntityFrameworkCore.Sqlite.Storage.Internal.SqliteNetTopologySuiteTypeMappingSourcePlugin.FindMapping(RelationalTypeMappingInfo& mappingInfo)
   at Microsoft.EntityFrameworkCore.Storage.RelationalTypeMappingSource.FindMapping(RelationalTypeMappingInfo& mappingInfo)
   at Microsoft.EntityFrameworkCore.Sqlite.Storage.Internal.SqliteTypeMappingSource.FindRawMapping(RelationalTypeMappingInfo mappingInfo)
   at Microsoft.EntityFrameworkCore.Sqlite.Storage.Internal.SqliteTypeMappingSource.FindMapping(RelationalTypeMappingInfo& mappingInfo)
   at Microsoft.EntityFrameworkCore.Storage.RelationalTypeMappingSource.<FindMappingWithConversion>b__7_0(ValueTuple`3 k)
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
   at Microsoft.EntityFrameworkCore.Storage.RelationalTypeMappingSource.FindMappingWithConversion(RelationalTypeMappingInfo& mappingInfo, IReadOnlyList`1 principals)
   at Microsoft.EntityFrameworkCore.Storage.RelationalTypeMappingSource.FindMapping(IProperty property)
   at Microsoft.EntityFrameworkCore.Metadata.Conventions.TypeMappingConvention.ProcessModelFinalizing(IConventionModelBuilder modelBuilder, IConventionContext`1 context)
   at Microsoft.EntityFrameworkCore.Migrations.Internal.Migrator.FinalizeModel(IModel model)
   at Microsoft.EntityFrameworkCore.Migrations.Internal.Migrator.GenerateUpSql(Migration migration, MigrationsSqlGenerationOptions options)
   at Microsoft.EntityFrameworkCore.Migrations.Internal.Migrator.<>c__DisplayClass16_2.<GetMigrationCommandLists>b__2()
   at Microsoft.EntityFrameworkCore.Migrations.Internal.Migrator.Migrate(String targetMigration)
   at Microsoft.EntityFrameworkCore.RelationalDatabaseFacadeExtensions.Migrate(DatabaseFacade databaseFacade)
   at App.Program.Main(String[] args) in C:\github\efcore-sqlite-geometry-migration\src\App\Program.cs:line 17

Inner exception:

Invalid geometry type: geography (Parameter 'storeType')
   at Microsoft.EntityFrameworkCore.Sqlite.Storage.Internal.SqliteGeometryTypeMapping`1.CreateWriter(String storeType)
   at Microsoft.EntityFrameworkCore.Sqlite.Storage.Internal.SqliteGeometryTypeMapping`1..ctor(NtsGeometryServices geometryServices, String storeType)
ajcvickers commented 3 years ago

@joaopgrassi geogrphy is a SQL Server-specific type. Your model for SQLite must use geometry since this is the database type name expected by SpatiaLite which handles spatial types when using SQLite.

joaopgrassi commented 3 years ago

Yes, that is clear now @ajcvickers . But what I asked before is if there's a way to generate a new migration for Sqlite that contain all the changes. Something like: it would see that I don't have any migrations for Sqlite and it would create a new one. I tried following here: https://docs.microsoft.com/en-us/ef/core/managing-schemas/migrations/providers?tabs=dotnet-core-cli, (the part where one can specify the provider as a custom config param)

Because my situation now is that I have several migrations for SQL Server, and I can't get my integration tests to work. Is the only way out is deleting all the migrations and starting from scratch, adding the migrations for each provider?

ajcvickers commented 3 years ago

@joaopgrassi You should be able to add a new migration for SQLite that will migrate the database from empty to where the model is now. You shouldn't try to use any of the existing migrations for this, since they have all been created for SQL Server. Also, you will need to conditionally build the model to not use SQL-Server specific features in the model, including the geography type.

bricelam commented 3 years ago

See also Migrations with Multiple Providers.

svha1977 commented 3 years ago

joaopgrassi

Ran into the same issue. Actually there excist a workaround like you asked for - wheather its acceptable I leave up to you. Strictly speaking its not ideal since EF team lists one of the involved classes as "intended for internal use" so of course they might change its since its considered an internal API.

But this works: Extract from a TextFixture

public DataContext Context
        {
            get
            {
                return new DataContext(new DbContextOptionsBuilder()
                    .UseSqlite(Connection, x => { x.UseNetTopologySuite(); })
                    // This will replace the SQllite type mapping plugin for geometries with the inherited and marginally modified on below.
                    .ReplaceService<IRelationalTypeMappingSourcePlugin, MyMigrationsAnnotationProvider>().Options);
            }

        }

        public InMemoryDbContextFixture()
        {
            Connection = new SqliteConnection("Filename=:memory:");
            Connection.Open();
            Context.Database.EnsureCreated();
            // ... initialize data in the test database ...
        }

        public void Dispose()
        {
            // ... clean up test data from the database ...
            Connection.Close();
        }

public class MyMigrationsAnnotationProvider : SqliteNetTopologySuiteTypeMappingSourcePlugin
    {
        public MyMigrationsAnnotationProvider(NtsGeometryServices geometryServices) : base(geometryServices)
        {
        }

        public override RelationalTypeMapping FindMapping(in RelationalTypeMappingInfo mappingInfo)
        {
            if (!String.Equals(mappingInfo.StoreTypeName, "geography", StringComparison.InvariantCultureIgnoreCase))
            {
                return base.FindMapping(in mappingInfo);
            }

            var geographyChangedToGeometry = new RelationalTypeMappingInfo(mappingInfo.ClrType, storeTypeName: "geometry", storeTypeNameBase: "geometry");
            return base.FindMapping(geographyChangedToGeometry);
        }
    }

Of course its hacky due to the inner API modification but its for test not production. Furthermore you can argue in case it might break in the future to a point where its unrecoverable then you have ways out by e.g. 1) Moving on to use physical SQLSErver for tests 2) Follow the advice from above by creating a provider specific migration.

We have decided to postpone 2) since we´re in development where migrations changes all the time so in worst case we basicly postpone issues with possible breaking changes in the API to a point where the migrations in the project is more stable - in case its ever required.

Probably much more complicated in reality but could some of this be baked into EF like having provider spceific attributes e.g. something like

[Column(TypeName = "geography", Provider="SqlServer")] 
[Column(TypeName = "geometry", Provider="Sqlite")]
public Geometry Geometry { get; set; }

Of course there is still potential raw executed Sql in the migration that can never work, but in case not it might be possible to avoid multiple providers? Just a thought anyway...

joaopgrassi commented 3 years ago

Hey @svha1977 thanks for sharing this approach, looks interesting :).

Since I posted this more things showed up, like you said views and other things so it started to become really a problem. Plus we had also no real confidence that the code would work in production since well, we didn't use the same database during the tests.

So, I ended up moving all our tests to use a real SQL Server instance during integration tests with the help of containers. I wrote a blog post series about it, in case you want to check: https://blog.joaograssi.com/series/integration-tests-in-asp.net-core/

It has really paid off for our team because we were able to find bugs way faster now. Specially since we use EF with OData and that is... let's say a bit complicated :sweat_smile:

svha1977 commented 3 years ago

I know. Litteraly 5 min after sharing what we did a month ago a team member ran into doing bulk insert utilizing the IncludeGraph=true to Add a whole graph of related entities in one simple statement. Then of course this is apparently not a feature supported in SQLite but only in more advanced dbs and of course its not ideal having to chance the implementation just because it cannot function in the test DB.

philipag commented 2 years ago

I found this to resolve the issue in this thread:

modelBuilder.Entity<GpsPoint>().Property(p => p.Location).HasColumnType("geography");

var converter = new Microsoft.EntityFrameworkCore.Sqlite.Storage.ValueConversion.Internal.GeometryValueConverter<NetTopologySuite.Geometries.Point>(new GaiaGeoReader(), new GaiaGeoWriter());
modelBuilder.Entity<GpsPoint>().Property(p => p.Location).HasConversion(converter);

The "Location" property is of type "Point"