Refactor UpdateSqlGenerator to separate select logic

[roji]

This introduces a new SelectingUpdateSqlGenerator abstract class, which uses SELECT after the INSERT/UPDATE/DELETE to retrieve database-generated values or to perform concurrency checks. This moves all SELECT logic out of UpdateSqlGenerator, for providers which don't require it. SqlServerUpdateSqlGenerator inherits from SelectingUpdateSqlGenerator and uses the SELECT logic in various cases where triggers are defined (and OUTPUT cannot be used).

The design isn't amazing: SelectingUpdateSqlGenerator extends UpdateSqlGenerator, so that SqlServerUpdateSqlGenerator can call methods from both (it's a combination of both modes, depending on whether there are triggers or not). But it seems reasonable (am open for other ideas).

Note that most of SelectingUpdateSqlGenerator is covered for SQL Server tests which involve triggers.

Closes #10431

[AndriySvyryd]

Though I don't think it's a significant issue of having the Select logic in UpdateSqlGenerator

[roji]

The main design smell here is that the select logic requires abstract methods in UpdateSqlGenerator, which are never called in the default implementation of that class (this is new in 7.0, since the default implementation now uses RETURNING/OUTPUT). Also, many (most?) implementations don't actually need selecting, and so these methods need to be overridden for nothing.

I agree it's not critical, more of a design smell. I'm OK either way - if you're against it we can leave it and close #10431.

[AndriySvyryd]

If @cincuranet is happy with these changes then I'm ok.

[roji]

@cincuranet could you take a look at this PR please, and let us know what you think?

[ErikEJ]

@roji maybe relevant:

SqL Compact only supports a single statement per SqlCeCommand so to get identity values back, as following command execution on the same open connection is needed, something like Select @@identity

[roji]

@ErikEJ you mean that there's not INSERT ... OUTPUT on SqlCe, right?

I think this change shouldn't cause any problems with this. Basically, before EF Core 7.0, the default UpdateSqlGenerator would never use the OUTPUT clause (or RETURNING in most databases), and would use INSERT/UPDATE/DELETE+SELECT by default to get any database-generated values (or concurrency checking, for UPDATE/DELETE).

In EF Core 7.0, the default behavior of UpdateSqlGenerator has been changed to do OUTPUT/RETURNING since that's better in various ways; but the INSERT/UPDATE/DELETE+SELECT pattern is still supported (SQL Server notably still requires it for when triggers are defined). All this PR does is factor out the latter logic into a separate subclass, so that providers which use the new, default OUTPUT/RETURNING logic won't be concerned with it.

Specifically about the single-statement-per-command aspect:

SqL Compact only supports a single statement per SqlCeCommand so to get identity values back, as following command execution on the same open connection is needed, something like Select @@identity

So you're saying it's not possible to have a single SqlCeCommand that has both the INSERT and the related SELECT? If so, that's indeed not supported at the moment (a single ModificationCommand that gets executed in two batches), but I also don't think it was supported before either... Looking at the 2.2.0 version of UpdateSqlGenerator and at the latest version of your provider's SqlCeUpdateSqlGenerator, it seems it was appending both statements inside a single command's CommandText - can you check that I'm not mistaken?

Splitting a single ModificationCommand into two batches would require an architectural change, and we haven't heard complaints from any provider which doesn't support it yet... /cc @AndriySvyryd

[ErikEJ]

The split is implemented here, so probably external to this: https://github.com/ErikEJ/EntityFramework.SqlServerCompact/blob/master/src/Provider40/Update/Internal/SqlCeModificationCommandBatch.cs

[roji]

Ah yes, I see... That's definitely not ideal, but indeed EF Core 7.0 doesn't change anything around this, so that workaround should still be possible. If another provider pops up with this problem, we can look into a better way to support that.

[ErikEJ]

Cool (no plans for an EF Core 7 SQLCE provider anyway)

[cincuranet]

@roji @AndriySvyryd LGTM