Allow opting out of RETURNING/OUTPUT clauses in SaveChanges

[roji]

Fixes #29916

[roji]

@AndriySvyryd I've pushed some updates; this is still a draft to get some feedback though. Can you please confirm you're OK with the below? FWIW I think this really is complex for what we're doing here.

• I've added ITriggerAddedConvention and ITriggerRemovedConvention, and SqlServerOutputClauseConvention is now a reactive convention.
• In a TPH hierarchy, the UseSqlOutputClause is stored on the root entity type, and the triggers convention keeps it in sync based on triggers.
• If this is a good approach, then I think we also need a (separate) convention to manage the setting when it's set directly by the user; e.g. if the user sets it on some entity in a TPH hierarchy, we should by-convention also set it on the root, so that the entire hierarchy is affected, right? The convention would need to also exist for SQLite (where we don't have a triggers convention).
• Finally, the triggers convention should also detect triggers defined on entity types sharing the same table - can you point to an example of how to do this?

(any other review remarks welcome as well, of course).

[AndriySvyryd]

I think that by not using the same granularity as triggers the convention logic is more complex than it needs to be.

If this is a good approach, then I think we also need a (separate) convention to manage the setting when it's set directly by the user; e.g. if the user sets it on some entity in a TPH hierarchy, we should by-convention also set it on the root, so that the entire hierarchy is affected, right? The convention would need to also exist for SQLite (where we don't have a triggers convention).
Finally, the triggers convention should also detect triggers defined on entity types sharing the same table - can you point to an example of how to do this?

It's generally messy to propagate configuration using conventions. A more robust approach would be to throw if this is configured on a non-root entity type and add IsSqlOutputClauseUsed to ITable and calculate the value when creating the Table object.

[roji]

A more robust approach would be to throw if this is configured on a non-root entity type and add IsSqlOutputClauseUsed to ITable and calculate the value when creating the Table object.

Can we do that given that IsSqlOutputClauseUsed is SQL Server-specific and ITable is relational? If I understand things correctly, the creation of the Table (in RelationalModel) also doesn't allow for provider-specific customizations.

Maybe we should have a quick call to discuss this.

[AndriySvyryd]

Can we do that given that IsSqlOutputClauseUsed is SQL Server-specific and ITable is relational? If I understand things correctly, the creation of the Table (in RelationalModel) also doesn't allow for provider-specific customizations.

It does. You can add an annotation in SqlServerAnnotationProvider

[roji]

@AndriySvyryd OK, I pushed a change... UseSqSqlOutputClause is now recorded (only) in a mapping fragment (I've removed the API to set/get it on entity type without a StoreObjectIdentifier). I ended up not using SqlServerAnnotationProvider, just having the IsSqlOutputClauseUsed extension accessor on ITable access the appropriate mapping fragments on all entity types mapped to that table. I agree this is simpler/better, and also supports better granularity. I wanted to get an OK on the approach before continuing.

One last thing I'm not sure about:

It's generally messy to propagate configuration using conventions. A more robust approach would be to throw if this is configured on a non-root entity type and add IsSqlOutputClauseUsed to ITable and calculate the value when creating the Table object.

Sounds good - but how do we handle a case where an entity type is made part of a TPH hierarchy after UseSqlOutputClause(false) is called on it (or after a trigger is added to it)? Is there a better solution than detecting that in the convention and moving setting to the root? And if we do this specific propagation, should we just support it when the setting is done on the non-root entity type after it's added to the hierarchy?

[AndriySvyryd]

I wanted to get an OK on the approach before continuing.

You still need to have the method on IMutableEntityType for the implicit/main mapping fragment (i.e. when ToTable is called without a name)

Sounds good - but how do we handle a case where an entity type is made part of a TPH hierarchy after UseSqlOutputClause(false) is called on it (or after a trigger is added to it)? Is there a better solution than detecting that in the convention and moving setting to the root? And if we do this specific propagation, should we just support it when the setting is done on the non-root entity type after it's added to the hierarchy?

I am saying that if that happens we should just throw and make the user configure it on the root themselves, so we don't need to propagate anything

[roji]

Thanks, I think I understand better how mapping fragments are supposed to work.

I've pushed another commit handling the implicit/main mapping fragment and disallowing UseSqlOutputClause for non-root TPH entity types; hopefully this is OK. Take a look, if so I'll do the changes for SQLite (where I think we'd need a convention just to throw for the non-root TPH entity type thing).

[roji]

Thanks @AndriySvyryd, I think this should be ready for a final review.

[AndriySvyryd]

Consider caching this and IsSqlReturningClauseUsed in an annotation.

[roji]

@AndriySvyryd fixed it all, if you want to take a final look.

[ghost]

Hello @roji!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[ghost]

Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:

1. Azure Pipelines

These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check.

Give feedback on this

From the bot dev team

We've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments.

Please reach out to us at [email protected] to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin.