ADO.NET batching API #54467
ADO.NET batching API #54467
Conversation
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
|
Tagging subscribers to this area: @roji, @ajcvickers Issue DetailsCloses #28633 /cc @bgrainger @Wraith2 @bgribaudo @FransBouma @GSPP @bricelam @ajcvickers
|
|
|
||
| protected virtual DbBatchCommand CreateDbBatchCommand() => throw new NotSupportedException(); | ||
|
|
||
| public virtual void Dispose() {} |
There was a problem hiding this comment.
@stephentoub can you please confirm that this (and the below DisposeAsync) is acceptable in an abstract base class, given there are no resources to be disposed on it, and that the dispose pattern could simply be implemented by implementations if needed?
Also, I've made this non-abstract since most implementations aren't expected to override.
There was a problem hiding this comment.
I'm a little surprised to not see the standard dispose pattern implemented here, although I do expect the inheritance hierarchy to be very shallow (a single derived class per provider, and it should be sealed), thus I can't imagine it being a problem in practice.
There was a problem hiding this comment.
@bgrainger missed this originally.
I think the standard dispose pattern makes sense when the base class already have some disposable resource; DbCommand/DbConnection extend Component (DbBatch doesn't), which itself is disposable (mainly for event management, AFAIK), which is why the pattern is followed there. For an empty base class with no disposable resources, I'm not sure what the pattern would bring - it can always be added in implementing classes, if those classes have a disposable resource and expect to be themselves inherited...
Let me know if you see any issue with this logic.
|
See some discussion in #28633 (comment) |
src/libraries/System.Data.Common/src/System/Data/Common/DbBatch.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Data.Common/src/System/Data/Common/DbBatch.cs
Outdated
Show resolved
Hide resolved
| public System.Threading.Tasks.Task<System.Data.Common.DbDataReader> ExecuteReaderAsync(System.Threading.CancellationToken cancellationToken = default) { throw null; } | ||
| public System.Threading.Tasks.Task<System.Data.Common.DbDataReader> ExecuteReaderAsync(System.Data.CommandBehavior behavior, System.Threading.CancellationToken cancellationToken = default) { throw null; } |
There was a problem hiding this comment.
Why two overloads instead of two defaulted parameters?
(I thought I remembered a discussion about a different new ADO.NET API where it was decided that new code didn't have to follow the existing pattern of multiple overloads. Or maybe I'm remembering the first version of the async schema API and it was eventually changed to follow the pattern and use multiple overloads?)
There was a problem hiding this comment.
You're right that we could do just one overload with two optional parameters. My logic here was that explicitly specifying the behavior is a relatively rare thing, so it would be good to allow invoking with a cancellation token without having to either specify the behavior or use a named parameter for the token. The same applies for consistency with DbCommand, where you can invoke ExecuteReaderAsync this way.
We did have a discussion about the GetSchemaAsync overloads where we decided to leave them all virtual, but I think that's different (here all the public methods are non-virtual, and call a single abstract protected method without optional parameters).
| { | ||
| public abstract string CommandText { get; set; } | ||
| public abstract CommandType CommandType { get; set; } | ||
| public abstract int RecordsAffected { get; set; } |
There was a problem hiding this comment.
Public setter seems odd; should this be protected set?
There was a problem hiding this comment.
Good catch, there definitely shouldn't be a public setter. Though rather than making the setter protected I've removed it entirely, to not impose things on implementations (this is also how DbDataReader.RecordsAffected is defined.
| public abstract System.Threading.Tasks.Task PrepareAsync(System.Threading.CancellationToken cancellationToken = default); | ||
| public abstract void Cancel(); | ||
| public System.Data.Common.DbBatchCommand CreateBatchCommand() { throw null; } | ||
| protected virtual System.Data.Common.DbBatchCommand CreateDbBatchCommand() { throw null; } |
There was a problem hiding this comment.
| protected virtual System.Data.Common.DbBatchCommand CreateDbBatchCommand() { throw null; } | |
| protected abstract System.Data.Common.DbBatchCommand CreateDbBatchCommand() { throw null; } |
Shouldn't it be required for derived classes to implement this?
There was a problem hiding this comment.
Good catch here too. CreateDbBatch on DbConnection and DbProviderFactory are virtual (and throw NotSupportedException by default) since we can't add abstract methods to existing types without breaking. But DbBatch is a new type, and it makes no sense to define an implementation of it which doesn't support creating a DbBatchCommand.
ghost
locked as resolved and limited conversation to collaborators
Closes #28633
/cc @bgrainger @Wraith2 @bgribaudo @FransBouma @GSPP @bricelam @ajcvickers