npgsql / EntityFramework6.Npgsql

Entity Framework 6 provider for PostgreSQL
PostgreSQL License
66 stars 53 forks source link

Difficulty extending NpgsqlMigrationSqlGenerator #167

Closed Bouke closed 3 years ago

Bouke commented 3 years ago

The SQL generation for AlterColumn cannot easily be extended. In order to add USING "Id"::uuid I'd have to re-implement all the logic in Convert(AlterColumnOperation alterColumnOperation) and all the internal members called from that method: https://github.com/npgsql/EntityFramework6.Npgsql/blob/55c19004838451d62e364fa7da38b6cf3b471640/EF6.PG/NpgsqlMigrationSqlGenerator.cs#L244-L247. Ideally those methods would be open to extension / some other hooks to easily add a USING statement.

E.g. I'd need to change:

ALTER TABLE "public"."Entity" ALTER COLUMN "Id" TYPE uuid

into this:

ALTER TABLE "public"."Entity" ALTER COLUMN "Id" TYPE uuid USING "Id"::uuid

I've now worked around this by overriding the generated sql:

public override IEnumerable<MigrationStatement> Generate(IEnumerable<MigrationOperation> migrationOperations, string providerManifestToken)
{
    foreach (var statement in base.Generate(migrationOperations, providerManifestToken))
    {
        var match = Regex.Match(statement.Sql, "ALTER TABLE .*? ALTER COLUMN (?<column>.+?) TYPE uuid");
        if (match.Success)
            statement.Sql += $" USING {match.Groups["column"]}::uuid";
        yield return statement;
    }
}
Emill commented 3 years ago

Hmm. Is this something a EF6 provider should be able to to handle? Shouldn't the EF itself have support for this first? I mean can this be fixed? Note that the new EF Core is far more flexible than the ancient EF6.

roji commented 3 years ago

Yeah, I doubt the EF6 provider model gives a lot of flexibility here. As @Emill mentions, EF Core was designed from the start with this kind of extensibility in mind. In any case, this provider is pretty much archived at this point - although we'd accept well-written PRs for bug fixes, realistically speaking it's not going to evolve, e.g. by introducing new hooks. So I'm going to go ahead and close this for now (but feel free to post back here of course).

Bouke commented 3 years ago

I do believe the provider model gives that flexibility, but the implementation of NpgsqlMigrationSqlGenerator is too closed. I can, for example override protected virtual void Convert(AlterColumnOperation alterColumnOperation), but I cannot override any of the methods it's calling. Also this method doesn't have a return value that I can replace in a subclass, and the result aggregation also happens through a method I cannot extend:void AddStatment(string sql, bool suppressTransacion = false).

I understand the EOL decision on this package, and as I have workaround my immediate need is taken care of.