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

NpgsqlMigrationsSqlGenerator issue after upgrading to EF Core 5.0.1 #1603

Closed
suadev opened this issue Dec 10, 2020 · 17 comments
Closed

NpgsqlMigrationsSqlGenerator issue after upgrading to EF Core 5.0.1 #1603

suadev opened this issue Dec 10, 2020 · 17 comments

Comments

@suadev
Copy link

suadev commented Dec 10, 2020

I've just upgraded to 5.0.1 and my custom migration SQL generator failed.

I replace it like below

  .ReplaceService<IMigrationsSqlGenerator, MyMigrationSqlGenerator>()

All tables and constraints are created successfully but, while starting to seeding the cities table for a specific schema, I got the exception;

"There is no entity type mapped to the table 'sample_tenant.cities' used in a data operation. Either add the corresponding entity type to the model or specify the column types in the data operation."

I got an exception on the line below of Generate( ) method for an InsertDataOperation

  base.Generate(operation, model, builder);

Whole MyMigrationSqlGenerator class;

    public class PmsMigrationSqlGenerator : NpgsqlMigrationsSqlGenerator
    {
        private readonly string _schema;

        public PmsMigrationSqlGenerator(MigrationsSqlGeneratorDependencies dependencies, INpgsqlOptions npgsqlOptions)
            : base(dependencies, npgsqlOptions)
        {
            _schema = UserLocalContext.Instance.GetTenantCode();
        }
        protected override void Generate(MigrationOperation operation, IModel model, MigrationCommandListBuilder builder)
        {
            Action action;
            switch (operation)
            {
                case EnsureSchemaOperation op:
                    action = ProcessEnsureSchemaOperation(op);
                    break;

                case CreateTableOperation op:
                    action = ProcessCreateTableOperation(op);
                    break;

                case SqlOperation op:
                    action = ProcessQueryOperation(op);
                    break;

                default:
                    action = ProcessBaseMigrationOperation(operation);
                    break;
            }

            if (action == Action.Skip)
                return;

            base.Generate(operation, model, builder);
        }

        private Action ProcessQueryOperation(SqlOperation op)
        {
            op.Sql = op.Sql.Replace("#schema", _schema);
            return Action.Apply;
        }

        private Action ProcessEnsureSchemaOperation(EnsureSchemaOperation op)
        {
            op.Name = _schema;
            return Action.Apply;
        }

        private Action ProcessCreateTableOperation(CreateTableOperation op)
        {
            if (ProcessBaseMigrationOperation(op) == Action.Skip)
                return Action.Skip;

            foreach (var item in op.UniqueConstraints)
                item.Schema = _schema;

            foreach (var foreignKey in op.ForeignKeys)
            {
                foreignKey.Schema = _schema;
                foreignKey.PrincipalSchema = _schema;
            }

            return Action.Apply;
        }

        private Action ProcessBaseMigrationOperation(MigrationOperation op)
        {
            var schemaProperty = op.GetType().GetProperties().FirstOrDefault(p => p.Name == "Schema");

            if (schemaProperty != null)
                schemaProperty.SetValue(op, _schema);

            var principleSchemaProperty = op.GetType().GetProperties().FirstOrDefault(p => p.Name == "PrincipalSchema");

            if (principleSchemaProperty == null)
                return Action.Apply;

            principleSchemaProperty.SetValue(op, _schema);

            return Action.Apply;
        }

        private enum Action
        {
            Apply,
            Skip
        }
    }
@roji
Copy link
Member

roji commented Dec 10, 2020

@suadev what were you using before upgrading to 5.0.1, 5.0.0 or 3.1.x?

Regardless, I'm going to need a full, runnable code sample in order to investigate...

@suadev
Copy link
Author

suadev commented Dec 10, 2020

@roji I know you need a runnable sample to reproduce it but it looks kinda tough. I'll try to do my best.

Previous versions;

"Npgsql.EntityFrameworkCore.PostgreSQL" Version="3.1.4"
"Microsoft.EntityFrameworkCore" Version="3.1.7"

@suadev
Copy link
Author

suadev commented Dec 11, 2020

Hi @roji, I checked out dotnet/efcore repo. According to the stack trace below, the root cause is GetPropertyMapping method.

I found this commit: dotnet/efcore@ae17675

Maybe you or @AndriySvyryd have an idea? ( I am working on the sample to give you for reproducing)

Another useful info; The schema in 'model' is null, so I guess my exception is following InvalidOperationException.

How can I set schema value in RelationalModel/Tables object?

           var table = model.GetRelationalModel().FindTable(tableName, schema);

            if (table == null)
            {
                throw new InvalidOperationException(RelationalStrings.DataOperationNoTable(
                    FormatTable(tableName, schema)));
            }

image

" at Microsoft.EntityFrameworkCore.Migrations.MigrationsSqlGenerator.GetPropertyMappings(String[] names, String tableName, String schema, IModel model)\r\n at Microsoft.EntityFrameworkCore.Migrations.MigrationsSqlGenerator.GenerateModificationCommands(InsertDataOperation operation, IModel model)+MoveNext()\r\n at Npgsql.EntityFrameworkCore.PostgreSQL.Migrations.NpgsqlMigrationsSqlGenerator.Generate(InsertDataOperation operation, IModel model, MigrationCommandListBuilder builder, Boolean terminate)\r\n at Microsoft.EntityFrameworkCore.Migrations.MigrationsSqlGenerator.<>c.<.cctor>b__83_28(MigrationsSqlGenerator g, MigrationOperation o, IModel m, MigrationCommandListBuilder b)\r\n at Microsoft.EntityFrameworkCore.Migrations.MigrationsSqlGenerator.Generate(MigrationOperation operation, IModel model, MigrationCommandListBuilder builder)\r\n at Npgsql.EntityFrameworkCore.PostgreSQL.Migrations.NpgsqlMigrationsSqlGenerator.Generate(MigrationOperation operation, IModel model, MigrationCommandListBuilder builder)\r\n at Pms.Infrastructure.EfCore.ReplaceServices.PmsMigrationSqlGenerator.Generate(MigrationOperation operation, IModel model, MigrationCommandListBuilder builder) in C:\source\Pms.Backend\src\Pms.Infrastructure\EfCore\ReplaceServices\PmsMigrationSqlGenerator.cs:line 51\r\n at Microsoft.EntityFrameworkCore.Migrations.MigrationsSqlGenerator.Generate(IReadOnlyList1 operations, IModel model, MigrationsSqlGenerationOptions options)\r\n at Npgsql.EntityFrameworkCore.PostgreSQL.Migrations.NpgsqlMigrationsSqlGenerator.Generate(IReadOnlyList1 operations, IModel model, MigrationsSqlGenerationOptions options)\r\n at Microsoft.EntityFrameworkCore.Migrations.Internal.Migrator.GenerateUpSql(Migration migration, MigrationsSqlGenerationOptions options)\r\n at Microsoft.EntityFrameworkCore.Migrations.Internal.Migrator.<>c__DisplayClass16_2.b__2()\r\n at Microsoft.EntityFrameworkCore.Migrations.Internal.Migrator.MigrateAsync(String targetMigration, CancellationToken cancellationToken)"

@roji
Copy link
Member

roji commented Dec 11, 2020

@suadev you seem to be doing some pretty serious customizations of the migration process to implement multi-tenant support, I'm not sure that's going to be fully supported...

The schema is usually configured on your model by calling ToTable in your model's OnModelCreating. Since you're changing the schema in various ways in the MigrationsSqlGenerator, you're going to have to make sure that that GetPropertyMappings can find your table in the relational model, possibly by tweaking the Schema property on the InsertDataOperation before GenerateModificationCommands is called.

Beyond that we're going to have to see some code to dive deeper.

@suadev
Copy link
Author

suadev commented Dec 11, 2020

@roji thank you. Yes, this is for the 'schema per tenant' approach.

By the way, we set default schema like below in OnModelCreating. But it seems this doesn't enough to catch schema value in the 'model' of the Generate method.

   builder.HasDefaultSchema("sample_tenant");

I also tried this but doesn't works;

builder.Entity<City>().Metadata.Model.SetDefaultSchema("sample_tenant")

I can't modify the RelationalModel because it's null in OnModelCreating method.

I'm working on a runnable sample.

@suadev
Copy link
Author

suadev commented Dec 11, 2020

I wonder why 'operation.Schema' is used in CreteTableOperation like below, but for InsertDataOperation , operation.Schema is not enough, also check model.GetRelationalModel().FindTable().

That means I can create a table with only using 'operation' object, but I can not insert any data into it. I'm really confused.

I would like to know why this behavior changed after 3.1.x.

cc:/ @smitpatel @ajcvickers

      protected virtual void Generate(
            [NotNull] CreateTableOperation operation,
            [CanBeNull] IModel model,
            [NotNull] MigrationCommandListBuilder builder,
            bool terminate = true)
        {
            Check.NotNull(operation, nameof(operation));
            Check.NotNull(builder, nameof(builder));

            builder
                .Append("CREATE TABLE ")
                .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(operation.Name, operation.Schema))
                .AppendLine(" (");

            using (builder.Indent())
            {
                CreateTableColumns(operation, model, builder);
                CreateTableConstraints(operation, model, builder);
                builder.AppendLine();
            }

            builder.Append(")");

            if (terminate)
            {
                builder.AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator);
                EndStatement(builder);
            }
        }

@smitpatel
Copy link

The issue is mismatch between the model and the migration operations.
Each migration file has a Designer.cs file associated with it which provides the model against which the migration was created for. This model is used when using the update pipeline to insert data through seeding.
In your code above you are changing schema for your Migration operation but you are not updating the associated model hence when it tries to find right column based on schema on InsertDataOperation in relational model it fails since relational model was created using the model which has dummy schema. If you also update the model for migration in designer file then it works correctly. (Verified by making edits in file manually).

@suadev
Copy link
Author

suadev commented Dec 11, 2020

@smitpatel thank you. But, since this is a multi-tenant DB that separates tenants by schema, I can't set a schema value in the designer.

For instance, creating and seeding the city table in the designer like below. I can not define the schema name here..

No problem with creating the city table as I mentioned above. I set schema in the migration SQL generator.

But, how I am supposed to do the same thing for inserting data?

Between, I added hard-coded schema: 'sample_tenant' in migrationBuilder.InsertData below, but doesn't work, still null schema in migration SQL generator.

  migrationBuilder.CreateTable(
                name: "cities",
                columns: table => new
                {
                    id = table.Column<int>(nullable: false)
                        .Annotation("Npgsql:ValueGenerationStrategy", NpgsqlValueGenerationStrategy.IdentityByDefaultColumn),
                    is_deleted = table.Column<bool>(nullable: false),
                    name = table.Column<string>(maxLength: 256, nullable: true),
                    country_id = table.Column<string>(maxLength: 256, nullable: true),
                    state_id = table.Column<int>(nullable: true)
                },
                constraints: table =>
                {
                    table.PrimaryKey("PK_cities", x => x.id);
                    table.ForeignKey(
                        name: "FK_cities_states_state_id",
                        column: x => x.state_id,
                        principalTable: "states",
                        principalColumn: "id",
                        onDelete: ReferentialAction.Restrict);
                });
   migrationBuilder.InsertData(
                table: "cities",
                columns: new[] { "id", "country_id", "is_deleted", "name", "state_id" },
                values: new object[,]
                {
                    { 1, "afg", false, "Badakhshān", null },
                    { 2666, "sen", false, "Kaolack", null },
                    { 2667, "sen", false, "Kolda", null },
                     .....

@smitpatel
Copy link

As @roji wrote above

you seem to be doing some pretty serious customizations of the migration process to implement multi-tenant support, I'm not sure that's going to be fully supported...

Couple of options I can think of,

  • Intercept the generated SQL after it is generated and then replace the placeholder schema with actual schema. So SQL will be generated correctly from dummy schema without causing mismatch between model and migration operations
  • If you want to intercept migration operations like above then you need to figure out how to intercept the model being built form designer file and also modify that model appropriately to set your tenant specific schema.

At this point, all I can say is, what you are doing is not a support scenario. It worked in 3.1 by accident. We did not intend to support it. Hence if you want to make it work this way in 5.0, you would need to figure out a way to do it.

@AndriySvyryd
Copy link

AndriySvyryd commented Dec 12, 2020

@suadev As the exception message states "Either add the corresponding entity type to the model or specify the column types in the data operation."
So if you can't alter the model appropriately then specify the column types in the InsertDataOperation. Though there's currently an issue when it doesn't actually flow through, so be sure to vote for it to be fixed soon: dotnet/efcore#23503

@suadev
Copy link
Author

suadev commented Dec 14, 2020

@AndriySvyryd i see. So, I will need to add column types manually?

I mean, there are many entities like this and new ones are coming. Will I need to add 'columnTypes' into InsertData method manually for each new migration file?

@AndriySvyryd
Copy link

@suadev Since you are already overriding other methods you can override Generate for *DataOperations, find the column types from the model (see GenerateModificationCommands) and then pass the updated operation to base.

@roji
Copy link
Member

roji commented Dec 14, 2020

Am closing this as there's nothing actionable on the Npgsql - but @suadev don't hesitate to continue posting if needed.

@roji roji closed this as completed Dec 14, 2020
@suadev
Copy link
Author

suadev commented Dec 14, 2020

@AndriySvyryd hi, I am not sure this is the best way but it works.

Overridde insert/update data operations, set ColumnTypes and KeyColumnTypes. That's it.

One last thing that is not clear for me; When you fix dotnet/efcore#23503, am I gonna need to refactor here again?

Any feedback will be appreciated ! Thank you.

 public class PmsMigrationSqlGenerator : NpgsqlMigrationsSqlGenerator
    {
        private readonly string _schema;

        public PmsMigrationSqlGenerator(MigrationsSqlGeneratorDependencies dependencies, INpgsqlOptions npgsqlOptions)
            : base(dependencies, npgsqlOptions)
        {
            _schema = UserLocalContext.Instance.GetTenantCode();
        }

        protected override void Generate(MigrationOperation operation, IModel model, MigrationCommandListBuilder builder)
        {
            switch (operation)
            {
                case SqlOperation op:

                    op.Sql = op.Sql.Replace("#schema", _schema);
                    break;
                case CreateTableOperation op:

                    SetSchemaForCreateTableOperation(op);
                    break;
                case InsertDataOperation op:

                    SetSchemaForMigrationOperation(op);

                    var table = GetRelationalTable(model, op.Table);
                    op.ColumnTypes = table.Columns.Select(s => s.Value.StoreType).ToArray();

                    break;
                case UpdateDataOperation op:

                    SetSchemaForMigrationOperation(op);

                    table = GetRelationalTable(model, op.Table);

                    var opColumnNames = op.Columns.ToList();
                    var opColumns = table.Columns.Where(s => opColumnNames.Contains(s.Value.Name));

                    op.ColumnTypes = opColumns.Select(s => s.Value.StoreType).ToArray();
                    op.KeyColumnTypes = table.PrimaryKey.Columns.Select(s => s.Name).ToArray();

                    break;
                default:
                    SetSchemaForMigrationOperation(operation);
                    break;
            }

            base.Generate(operation, model, builder);
        }

        private static Table GetRelationalTable(IModel model, string tableName)
        {
            var relationalModel = model.FindAnnotation("Relational:RelationalModel").Value as RelationalModel;
            return relationalModel.Tables.FirstOrDefault(s => s.Key.Item1 == tableName).Value;
        }

        private void SetSchemaForCreateTableOperation(CreateTableOperation op)
        {
            SetSchemaForMigrationOperation(op);

            foreach (var item in op.UniqueConstraints)
                item.Schema = _schema;

            foreach (var foreignKey in op.ForeignKeys)
            {
                foreignKey.Schema = _schema;
                foreignKey.PrincipalSchema = _schema;
            }
        }

        private void SetSchemaForMigrationOperation(MigrationOperation op)
        {
            var schemaProperty = op.GetType().GetProperties().FirstOrDefault(p => p.Name == "Schema");

            if (schemaProperty != null)
                schemaProperty.SetValue(op, _schema);

            var principleSchemaProperty = op.GetType().GetProperties().FirstOrDefault(p => p.Name == "PrincipalSchema");

            if (principleSchemaProperty == null)
                return;

            principleSchemaProperty.SetValue(op, _schema);
        }
    }

@AndriySvyryd
Copy link

@suadev You should handle DeleteDataOperation as well

One last thing that is not clear for me; When you fix dotnet/efcore#23503, am I gonna need to refactor here again?

No, just don't set them if ColumnTypes or KeyColumnTypes are already not null

@AndriySvyryd
Copy link

op.KeyColumnTypes = table.PrimaryKey.Columns.Select(s => s.Name).ToArray();

I think you meant op.KeyColumnTypes = table.PrimaryKey.Columns.Select(s => s.StoreType).ToArray();

@suadev
Copy link
Author

suadev commented Dec 14, 2020

Thank you Andriy, you really helped a lot.

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

4 participants