Explanation needed for BlankTriggerAddingConvention example

[pm7y]

There is an example on this page BlankTriggerAddingConvention which gives an example of how one should work around the breaking change. Please can you add an explanation about what this code is doing. It's not exactly clear. Seems to be checking for declared triggers on the table, but only triggers that don't have a database? seems a bit strange and warrants further explanation.

---

Document Details

⚠ Do not edit this section. It is required for learn.microsoft.com ➟ GitHub issue linking.

• ID: f1b683ae-c697-ebc8-d0aa-648d7573136e
• Version Independent ID: f1b683ae-c697-ebc8-d0aa-648d7573136e
• Content: Breaking changes in EF Core 7.0 (EF7) - EF Core
• Content Source: entity-framework/core/what-is-new/ef-core-7.0/breaking-changes.md
• Product: entity-framework
• Technology: entity-framework-core
• GitHub Login: @ajcvickers
• Microsoft Alias: avickers

[roji]

Here's the text immediately before that code sample:

You can let EF Core know that the target table has a trigger; doing so will revert to the previous, less efficient technique. This can be done by configuring the corresponding entity type as follows:
[ ...]
A model building convention can be used to configure all tables with triggers:

So what this convention does seems clear - it adds a trigger configuration to all tables which don't have one. IMHO the precise details about the code aren't important within the context of the breaking change note.

[pm7y]

It's very confusing because there is also a bug, the following snippet from the example always calls HasTrigger because even if entityType.GetDeclaredTriggers() returns no items the .All will still evaluate to true. Therefore HasTrigger is called for all entities.

if (table != null
    && entityType.GetDeclaredTriggers().All(t => t.GetDatabaseName(table.Value) == null))
{
    entityType.Builder.HasTrigger(table.Value.Name + "_Trigger");
}

And GetDeclaredTriggers seems to have some odd behaviour. It appears to be returning Functions (e.g. I have a default constraint on a table column that comes from a Function). But on other tables that actually do have triggers associated with them, GetDeclaredTriggers returns nothing.

Also it appears to be trying to get all triggers that have no database? wat?

Then there's this last bit involving mapping fragments...what are mapping fragments? and why do we have to do what looks like the same thing for these fragments? A little bit more context or explanation I think is reasonable if you expect people to use this as a workaround in production code.

foreach (var fragment in entityType.GetMappingFragments(StoreObjectType.Table))
{
    if (entityType.GetDeclaredTriggers().All(t => t.GetDatabaseName(fragment.StoreObject) == null))
    {
        entityType.Builder.HasTrigger(fragment.StoreObject.Name + "_Trigger");
    }
}

[roji]

It's very confusing because there is also a bug, the following snippet from the example always calls HasTrigger because even if entityType.GetDeclaredTriggers() returns no items the .All will still evaluate to true. Therefore HasTrigger is called for all entities.

That's not a bug - that's exactly what the convention is meant to do. If there are no trigger configurations, it does HasTrigger to add one.

Also it appears to be trying to get all triggers that have no database? wat?

Not no database - no database name. If a table already has a trigger and that trigger has a name, we don't want to create another one. Otherwise we do.

[pm7y]

How is that what the convention is supposed to do? Based on the description in the breaking change explanation - we only need to call HasTrigger on tables that have triggers in the Db. Correct? The example convention is calling HasTrigger on ALL entities regardless of whether they have triggers associated with them or not. Thus opting all of those tables unnecessarily out of the new EF7 strategy. But the convention should only be calling HasTrigger on the specific tables that have triggers.

[roji]

How is that what the convention is supposed to do? Based on the description in the breaking change explanation - we only need to call HasTrigger on tables that have triggers in the Db. Correct?

@pmcilreavy note this sentence right before the convention code:

A model building convention can be used to configure all tables with triggers:

So you can optionally use this convention to tell EF "all my tables have triggers on them", so that it always generates the slower, more compatible SQL.

The example convention is calling HasTrigger on ALL entities regardless of whether they have triggers associated with them or not.

That's not true. It only calls HasTrigger if the table has no triggers or configured, or if all configured triggers have no name.

Thus opting all of those tables unnecessarily out of the new EF7 strategy.

That is the point of this convention.

But the convention should only be calling HasTrigger on the specific tables that have triggers.

EF cannot know which tables in the database actually have triggers on them and which don't; you have tell it by configuring that in the model. You can do it on a table-by-table basis by calling HasTrigger yourself (as documented just above in the breaking change note), or if most/all your tables have triggers, you can save yourself the trouble by using this convention, which calls HasTrigger on all tables for you (except for tables where HasTrigger has already been called outside of the convention).

[pm7y]

I think the fact the two issues have been raised by different people about the same thing suggests that further clarification is needed on the article. The lengthy explanation about how we only need to call HasTrigger on specific tables we want to opt out, is then immediately followed by an example of the complete opposite giving the net result of opting out for all tables.

A model building convention can be used to configure all tables with triggers:

This reads as if, the convention can be used to call HasTrigger on all tables that have triggers. i.e. achieving the stated aim of the preceding paragraph.

But what it's actually saying is the convention can be used to ensure all tables are opted out.

[roji]

@pmcilreavy @dzins I've amended the text to hopefully make this all clearer, can you please take a look?

[pm7y]

👍 looks good.

Also, dunno if this should be logged as a separate issue, but I've found I also have to call HasTrigger on tables that don't have any triggers but do have a default constraint that specifies a function. This is not mentioned in the article. Perhaps it should?

e.g. a table that has a column with the following constraint...
ADD CONSTRAINT DF_dbo_SomeTable_SomeColumn DEFAULT dbo.SomeFunction() FOR SomeColumn

[roji]

Thanks @pmcilreavy

Also, dunno if this should be logged as a separate issue, but I've found I also have to call HasTrigger on tables that don't have any triggers but do have a default constraint that specifies a function. This is not mentioned in the article. Perhaps it should?

Yep - I've already done this in #4131, but we haven't published that live yet (that should happen very soon).