Migrations script: wrap queries in sp_executesql when using --idempotent

[Sjlver]

What problem are you trying to solve?

When generating migration scripts with dotnet ef migrations script --idempotent, the tool generates invalid SQL in some cases. Consider the following migration:

public partial class CreateAView : Migration
{
    protected override void Up(MigrationBuilder migrationBuilder)
    {
        migrationBuilder.Sql("create view MyView as select 1;");

This results in the following migration script:

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20220301060232_CreateAView')
BEGIN

create view MyView as select 1;

END;
GO

This script is invalid because create view must be the only statement in an SQL query batch; it cannot appear inside IF.

Describe the solution you'd like

The easiest solution seems to me to wrap custom SQL inside sp_executesql. Something like this:

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20220301060232_CreateAView')
BEGIN

DECLARE @query nvarchar(max) = N'
  create view MyView as select 1;
';
EXECUTE sp_executesql @query;

END;
GO

This works for Microsoft SQL server -- I'm sure other dialects have their own equivalent of dynamic SQL.

[achikhv]

The same issue happens with functions.

[ajcvickers]

@Sjlver You should be able to put the call to sp_executesql in the raw SQL that you are including in the migration. Is there some reason why this does not work or is not appropriate?

[Sjlver]

@ajcvickers Yes, that's absolutely right.

I would prefer to not do this for two reasons:

• It makes the migration more complicated. For example, I'd have to escape certain characters in the raw SQL when putting it inside a string literal.
• It's unnecessary in most cases. When using dotnet ef database update, the migration works just fine. It even works just fine when used with dotnet ef migrations script. Only when adding the --idempotent switch does the problem occur.

The latter point makes me think that the problem is really with --idempotent, and not with the raw SQL.

[achikhv]

I would add that need for changing source migrations can potentially lead to confusing errors later. One may forget to "escape" procedure in migration, and everything will work fine until idempotent migration script has to be applied. It may be really confusing to figure up what is wrong.
So we decided to doctor script after it was generated as a part of CI/CD pipeline.

[bricelam]

The way this works for other operations is that the SQL generation knows whether an idempotent script is being generated or not. I wonder if we should provide a lambda overload to enable raw SQL operations to do the same...

migraionBuilder.Sql(
    options =>
    {
        var sql = "CREATE VIEW MyView AS SELECT 1;";

        return options.HasFlag(MigrationsSqlGenerationOptions.Idempotent)
            ? "sp_execute N'" + sql.Replace("'", "''") + "';"
            : sql;
    });

[roji]

From what I've seen, users seem to either always uses idempotent migrations, or to never use them - so I'm not sure that would be so useful... The user could also define an extension method which would add the wrapping (e.g. migrationBuilder.SqlWithSpExecute(...)), though there's still of course always the possibility of a dev forgetting to use it...

[Sjlver]

Hmm... What's so bad about simply automatically doing the right thing? Why are we discussing all these options? As in, there would seem to be no downside at all to my original suggestion. Dotnet ef migrations would generate correct SQL, and everyone would be happy. But maybe I'm missing something...

…

On Fri, 15 Apr 2022, 18:53 Shay Rojansky, ***@***.***> wrote: From what I've seen, users seem to either always uses idempotent migrations, or to never use them - so I'm not sure that would be so useful... The user could also define an extension method which would add the wrapping (e.g. migrationBuilder.SqlWithSpExecute(...)), though there's still of course always the possibility of a dev forgetting to use it... — Reply to this email directly, view it on GitHub <#27729 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAARS63A6TYTMB3K3QNHK2DVFGUKRANCNFSM5SCFAYUQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[roji]

There's a cross-provider aspect to be considered here: sp_execute is SQL Server-specific, and indeed the need to wrap/transform raw migration SQL hasn't popped up in other providers as far as I remember.

It's true we could expose a provider hook for transforming raw SQL, and add the sp_execute there for SQL Server. But it's not clear that as a user, I'd want all raw SQL to go through that transformation just because some constructs require it - at the very least it makes migration scripts much less readable etc. Just doing this manually for the few statements that require it may be a good-enough solution.

[Sjlver]

Isn't --idempotent already provider-specific? As in, it needs to know which dialect to use for the IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] statements. All I'm asking for, really, is to transform this IF ... statement into IF ... DECLARE @query ... sq_executesql ....

I think the advantages are stronger than the disadvantages here:

• advantage: generates correct SQL (!)
• advantage: does not require the user to work around implementation details of --idempotent
• disadvantage: "much less readable etc." -- really?

[roji]

--idempotent as a feature isn't provider-specific, but there are indeed hooks that provider to specify their IF NOT EXISTS ... variants. However, sp_execute is SQL Server-specific, as well as (AFAIK) the need to transform the user's raw SQL when it's in an idempotent migration.

At the end of the day, when specifying raw SQL, it's the user's responsibility to provide correct SQL. It's true that in this case the SQL is correct and there's unfortunate incompatibility between idempotent and certain statements (CREATE VIEW); so it's a matter of whether it makes sense to pass all SQL Server raw SQL, always, through sp_executesql, just because of this. It seems trivial enough for the user to work around this, although the discoverability of the workaround isn't great (I think we'd definitely add docs on this).

Note: I don't think there's a need for a variable as shown in the OP solution above - the following simpler version should work:

EXECUTE sp_executesql N'create view MyView as select 1 as foo';

[Sjlver]

@roji That all makes sense.

I'm just wondering why we would want to put the burden on the user (and write docs, and answer the occasional question if users don't find the docs, etc.). Is the increase in complexity of the raw SQL feature really so large as to outweigh the benefits for the users? It might well be; I don't know how the feature is implemented... but my hunch is that it would probably be less than 30 LOC to add a call to sp_executesql. Plus a unit test. Plus optionally another few lines and a regular expression to only wrap those SQL statements that need it.

Anyway, at this point I feel like I've been vocal enough ;-) Let me just use this opportunity to express my gratitude to EF. It's a great software, and I shudder when I think of previous projects that used stored procedures instead of an ORM... so: thank you!

[JiriZidek]

This is old known issue: #24045 really frustrating.

[Aaron-P]

This affects stored procedure create/alter scripts as well.

[samusaran]

In my case it impacted an UPDATE. Basically before the migration i had 1 field, while at the end i needed to split that field in 2 new fields. The migration worked the first time, but from that moment onwards the script would fail because the old column does not exists anymore

[jroney]

From what I've seen, users seem to either always uses idempotent migrations, or to never use them - so I'm not sure that would be so useful... The user could also define an extension method which would add the wrapping (e.g. migrationBuilder.SqlWithSpExecute(...)), though there's still of course always the possibility of a dev forgetting to use it...

FWIW, I've worked with multiple teams who wanted to begin using an idempotent script on existing projects. And this issue has consistently been a source of pain in those scenarios.

[samusaran]

Hi, I've added the following extension method to my project to easily wrap the statement with executesql:

    public static OperationBuilder<SqlOperation> SpExecuteSql(this MigrationBuilder builder, 
        string query, bool suppressTransaction = false)
    {
        return builder.Sql($"EXECUTE sp_executesql N'{query.Replace("'", "''")}'", suppressTransaction);
    }

usage:

migrationBuilder.SpExecuteSql("""
                              CREATE VIEW [schema].[viewname] AS
                              (
                                 SELECT column
                                 FROM table
                              )
                              """);

[simonegli8]

I use the following extension method SafeSql to avoid the problem:

public static class MigrationBuilderExtension
{
	public static OperationBuilder<SqlOperation> SafeSql(this MigrationBuilder builder,
		string query, bool suppressTransaction = false)
	{
		return builder.Sql(Regex.Replace(query, @"(?<=(?:^|(?:^|[ \t;,\n\r])(?<!--[^\n]*)GO(?=[ \t;,\n\r])[^\n]*\n))([^'""]*?('[^']*?')?(""[^""]*?"")?)*?(?=(?:(?:^|[ \t;,\n\r])(?<!--[^\n]*)GO(?:(?=[ \t;,\n\r]|$))|$))", match =>
		{
			if (Regex.IsMatch(match.Value, @"^(?:[^'""]*?('[^']*?')?(""[^""]*?"")?)*\bCREATE\s+(?:FUNCTION|VIEW)\b", RegexOptions.IgnoreCase))
			{
				return $"EXECUTE sp_executesql N'{match.Value.Replace("'", "''")}'";
			}
			else return match.Value;
		}, RegexOptions.Singleline | RegexOptions.IgnoreCase), suppressTransaction);
	}
}