Support for inserting/updating tables with triggers (not use OUTPUT when batching off)

[rmja]

Let there be a table Table1 in SQL Server with an AFTER INSERT trigger. The produced SQL is:

exec sp_executesql N'SET NOCOUNT OFF;
INSERT INTO [Table1] ([Column1])
OUTPUT INSERTED.[Id], INSERTED.[ComputedColumn]
VALUES (@p0);
',N'@p0 int',@p0=1

The problem is that SQL reports the following error for such a statement if a trigger exists on the table:
The target table 'Table1' of the DML statement cannot have any enabled triggers if the statement contains an OUTPUT clause without INTO clause.

I have been trying a possible fix where the following code is changed for AppendBulkInsertOperation() in SqlServerSqlGenerator.cs:

            var operations = modificationCommands[i].ColumnModifications;
            var writeOperations = operations.Where(o => o.IsWrite).ToArray();
            var readOperations = operations.Where(o => o.IsRead).ToArray();

            if (readOperations.Length > 0)
            {
                commandStringBuilder
                    .Append("DECLARE @inserted TABLE (")
                    .AppendJoin(readOperations.Select(c => string.Format("{0} {1}",
                            DelimitIdentifier(c.ColumnName),
                            typeMapper.GetTypeMapping(c.Property.SqlServer().ColumnType, c.Property.SqlServer().Column, c.Property.UnderlyingType, false, false).StoreTypeName
                        )))
                    .Append(")")
                    .Append(BatchCommandSeparator)
                    .AppendLine();
            }

            AppendInsertCommandHeader(commandStringBuilder, schemaQualifiedName, writeOperations);
            if (readOperations.Length > 0)
            {
                AppendOutputClause(commandStringBuilder, readOperations);
                commandStringBuilder
                    .AppendLine()
                    .Append("INTO @inserted");
            }
            AppendValuesHeader(commandStringBuilder, writeOperations);
            AppendValues(commandStringBuilder, writeOperations);
            for (var j = 1; j < valueSetCount; j++)
            {
                commandStringBuilder.Append(",").AppendLine();
                AppendValues(commandStringBuilder, modificationCommands[j].ColumnModifications.Where(o => o.IsWrite).ToArray());
            }
            commandStringBuilder.Append(BatchCommandSeparator).AppendLine();

            if (readOperations.Length > 0)
            {
                commandStringBuilder
                    .Append("SELECT ")
                    .AppendJoin(readOperations.Select(c => DelimitIdentifier(c.ColumnName)))
                    .Append(" FROM @inserted")
                    .Append(BatchCommandSeparator)
                    .AppendLine();
            }
            else
            {
                AppendSelectAffectedCountCommand(commandStringBuilder, schemaQualifiedName);
            }

The following query is then produced, and it seems to work as expected. I am not sure about the GetTypeMapping() on the typeMapper.

exec sp_executesql N'SET NOCOUNT OFF;
DECLARE @inserted table (Id int, ComputedColumn int)
INSERT INTO [Table1] ([Column1])
OUTPUT INSERTED.[Id], INSERTED.[ComputedColumn]
INTO @inserted
VALUES (@p0)
SELECT * FROM @inserted;
',N'@p0 int',@p0=1

[rowanmiller]

We should look at allowing batching to be switched off and then not use OUTPUT

[rmja]

@rowanmiller, any news on this? - or do you have a hint on what I can do to mitigate the problem? It must be a very common problem, that records are inserted/updated on tables that have insert/update triggers associated.

[rowanmiller]

Hey - we aren't planning to tackle this right now. I think if you disable batching for the moment it should work...

        protected override void OnConfiguring(DbContextOptions builder)
        {
            builder
                .UseSqlServer(...)
                .MaxBatchSize(1);
        }

[rmja]

Hi @rowanmiller, thanks for getting back. Unfortunately it doesn't work. The OUTPUT statement is generated even for batch sizes of 1. Do you have any other suggestion?

[rowanmiller]

@rmja - nothing off the top of my head. I'm clearing this issue up for us to re-triage since when we put it on the backlog we thought disabling batching would be a feasible workaround.

[toddtsic]

Any update on this issue, with the lack of sp support at this time, triggers can be very helpful...

[louislewis2]

Please excuse me for opening a new issue, when one exists already.
#3292

I only found this issue after filing the above one.

Is there any update regarding this issue? Without this issue resolved we would be unable to use EF 7 in our application, which we are currently porting from Asp.Net 4 with EF 6 to Asp.Net 5 with EF 7. Where not being able to use EF 7 would partially invalidate the reasons behind our complete re-write / port.

[rowanmiller]

Bumping from Backlog to 7.0.0 for us to look at before RTM

[louislewis2]

Hi @rowanmiller I can understand that the team is extremely busy, but could you possibly let us know more or less when this feature may be looked at? At this point this is seriously preventing our implementation as the systems we work with have triggers present which affect us with this specific issue. Any estimate would be greatly appreciated.

[rowanmiller]

We are going to look at it after RC1 (next month) but before RTM.

[louislewis2]

@rowanmiller We are hoping to complete the setup of our internal preview environment for our customers in about 2 weeks. Could we possibly get involved with making this feature happen? If we can perhaps we can just hear the rough ideas that you had in mind, and the best starting points in the repo's. Without this feature we are totally unable to proceed, as the databases we work with have audit triggers on them that are out of our control. Currently on local dev, we simply remove the offenders, but this was only doable up till now.

[divega]

In case it helps, this is what I had in mind:

1. Try and see if we can make the approach that @rmja originally suggested work by adding a table variable and an INTO clause. Ideally this could work even with batching enabled.
2. If for some reason that does not work, look into enabling an alternate SQL generation without an OUTPUT clause when batching isn't enabled.
3. Whatever option we choose, test to see if there is any functional or performance impact. If the solution works but has gotchas, add a SQL Server extension for DbContextOptions to allow opting in the behavior.

[hwassermann]

@rowanmiller, @louislewis2 we are experimenting with a minimal solution in /src/EntityFramework.SqlServer/Update/SqlServerUpdateSqlGenerator.cs, assuming .MaxBatchSize(1):

                AppendInsertCommandHeader(commandStringBuilder, name, schema, writeOperations);
-               if (readOperations.Length > 0)
-               {
-                   AppendOutputClause(commandStringBuilder, readOperations);
-               }
                AppendValuesHeader(commandStringBuilder, writeOperations);
                AppendValues(commandStringBuilder, writeOperations);
                for (var j = 1; j < valueSetCount; j++)
                {
                    commandStringBuilder.Append(",").AppendLine();
                    AppendValues(commandStringBuilder, modificationCommands[j].ColumnModifications.Where(o => o.IsWrite).ToArray());
                }
                commandStringBuilder.Append(BatchCommandSeparator).AppendLine();

+               if (readOperations.Length > 0)
+               {
+                   var keyOperations = operations.Where(o => o.IsKey).ToArray();
+                   AppendSelectAffectedCommand(commandStringBuilder, name, schema, readOperations, keyOperations);
+               }

Perhaps this will be of some use to those more intimately familiar with the codebase, in devising a more comprehensive solution.

[louislewis2]

@divega @rowanmiller Hi guys, do you have any comments regarding the above 'fix'?
@hwassermann is my colleague and created that fix for us, it is working great for us so far. While there may still be other kinds of fixes, at least this one does get the MaxBatchSize(1) working as expected. We are currently running on a local build and as you can imagine this can become problematic and we would like to be able to jump on your packages as quick as we can. Any feedback would be greatly appreciated.

[rowanmiller]

Clearing the milestone so that we can re-discuss this in triage

[rowanmiller]

@AndriySvyryd for RTM we should do the simplest thing needed to support triggers. Swapping to a batch size of 1 would be an ok requirement to make it work.

[trailheadtechnology]

So I just ran into this problem too. I see a fix was checked in. What do you recommend I do to get it? Is there an imminent release, or do I need to pull and build this myself?

[AndriySvyryd]

The next release is more than a month away. You could try out the nightly build from https://www.myget.org/F/aspnetvnext

[KerolosMalak]

@rmja
where i find this file "sqlserversqlgenerator.cs" in my project
project with EF 7 , .Net FrameWork 4.6 and Asp.Net 5 (RC1)
thnx in advance