Replace unnecesary calls to SaveChangesAsync by FlushAsync

[sebastienros]

See sebastienros/yessql#601

[sebastienros]

@kevinchalet just because we talked about these years ago and you might remember something that I don't.

[kevinchalet]

I'm not familiar enough with YesSql to know the differences between SaveChangeAsync() and FlushAsync(), but assuming concurrency checks and unique indexes checks are performed by FlushAsync(), that should work.

I guess one risk of making this change is that some exceptions might surface elsewhere, when the session is committed higher in the stack, rather than in the stores themselves?

[sebastienros]

FlushAsync executes the pending SQL statements without committing the transaction, while SaveChangesAsync will call FlushAsync and commit the transaction, and clear the identity map ... The last part has the effect that the following SELECTS will return fresh data instead of local caches (identity map, aka first level cache, aka reference cache). But in most cases that is fine for a session that is linked to a request, because you get the same data during the whole request this way instead of dealing with different objects representing the same record, but with different state eventually. Concurrency checks would still work since they are doing pessimistic concurrency using the version field.

Calling SaveChangesAsync makes sense if you use a long running session for multiple work units, where you want to refresh your entities on the next SELECTS. But then you need to be careful not to call "Save" on a previously loaded item or YesSql could throw an exception.

[sebastienros]

Functional tests are still passing on this branch. https://github.com/OrchardCMS/OrchardCore/actions/runs/12042510905

Doesn't mean that everything was tested. How awesome would it be if we have functional tests for these operations, including concurrency checks.

[MikeAlhayek]

looks good to me

[kevinchalet]

@sebastienros makes sense, thanks the detailed explanation 👍🏻

(but just in case, that might be worth testing a complete authorization code flow using Postman, to ensure nothing regressed).

[deanmarcussen]

Yeah, I think this is fine.

@kevinchalet largely had me calling SaveChangesAsync to ensure that IdentityResult.Failed was the result of an actual sql failure during the session, rather than an exception later in the chain, but when running the transaction through the lifetime of the request, it makes more sense to me to flush it, but leave the transaction still open.