.Net: Change UpsertAsync return type to IReadOnlyList<TKey>

[roji]

Note: this PR is based on top of #11392, review 2nd commit only

Closes #10692

[adamsitnik]

@roji are you sure that we should return IReadOnlyList<TKey> rather than Task (void). I just want to make sure we want to support the key generation.

[roji]

are you sure that we should return IReadOnlyList rather than Task (void). I just want to make sure we want to support the key generation.

I'm honestly not completely sure. From our last design discussion on this, I got the impression that @westey-m and @dmytrostruk felt strongly that this is something we should keep. I'm on the fence - it is a capability that's shared across at least our relational providers (PG, SQL Server, SQLite), and I think @westey-m said at least one other non-relational database supports it (we should confirm, though even if not, that might not be a sufficient reason not to support it).

I do think that once we remove the IAsyncEnumerable in this PR, there's much less objection to supporting this - it no longer really gets in the way, since users can just ignore the return value. So I think there's at least consensus that this is the minimum we want to do.

But I'm totally fine with discussing this again and seeing how we feel about removing the feature altogether. It would be good to have the definitive list of database which do support this, though.

[adamsitnik]

My understanding was that we do the research first to find out whether any non-relational DB supports it. If not, we make it return Task, if yes, we make it return a list.

[roji]

OK, no problem - let's wait with this PR then until the research is done and we rediscuss (@westey-m I think this one's on you - I'll assign the issue back to you). I'll turn this into a draft in the meantime.

[roji]

@westey-m @adamsitnik I've rebased this PR and updated the docs to make it extra clear that batching UpsertAsync isn't idempotent when store-generated keys are in use, and atomicity isn't guaranteed.

[adamsitnik]

LGTM, thank you @roji !

[adamsitnik]

is the bang still required?

[roji]

The one on r isn't (because we now have a notnull constraint on TRecord), the other unfortunately still is... We need to replace at least some of the calls to GetValueAsObject (and Set) with generic getters/setters - that would allow #11183, but would also obviate the bang (since TKey is constrained to notnull. Later :)

[adamsitnik]

it looks much better now 💯

[roji]

I actually now seem to remember that a .NET design guideline is to not return collection interfaces, but rather prefer concrete types (so List<T>), because users may end up downcasting them anyway, and a change of the concrete type would end up breaking people in any case. I'm not sure if that's still a guideline - I'll go ahead and merge as-is with IReadOnlyList for now, and raise in API review (it's trivial to change later anyway).