-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ADO.NET batching API #54467
ADO.NET batching API #54467
Changes from all commits
27681c3
ca73eb2
4e8bfe7
64f1133
dc741dd
3b6a3ba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Threading; | ||
using System.Threading.Tasks; | ||
|
||
namespace System.Data.Common | ||
{ | ||
public abstract class DbBatch : IDisposable, IAsyncDisposable | ||
{ | ||
public DbBatchCommandCollection BatchCommands => DbBatchCommands; | ||
|
||
protected abstract DbBatchCommandCollection DbBatchCommands { get; } | ||
|
||
public abstract int Timeout { get; set; } | ||
|
||
public DbConnection? Connection | ||
{ | ||
get => DbConnection; | ||
set => DbConnection = value; | ||
} | ||
|
||
protected abstract DbConnection? DbConnection { get; set; } | ||
|
||
public DbTransaction? Transaction | ||
{ | ||
get => DbTransaction; | ||
set => DbTransaction = value; | ||
} | ||
|
||
protected abstract DbTransaction? DbTransaction { get; set; } | ||
|
||
public DbDataReader ExecuteReader(CommandBehavior behavior = CommandBehavior.Default) | ||
=> ExecuteDbDataReader(behavior); | ||
|
||
protected abstract DbDataReader ExecuteDbDataReader(CommandBehavior behavior); | ||
|
||
public Task<DbDataReader> ExecuteReaderAsync(CancellationToken cancellationToken = default) | ||
=> ExecuteDbDataReaderAsync(CommandBehavior.Default, cancellationToken); | ||
|
||
public Task<DbDataReader> ExecuteReaderAsync( | ||
CommandBehavior behavior, | ||
CancellationToken cancellationToken = default) | ||
=> ExecuteDbDataReaderAsync(behavior, cancellationToken); | ||
|
||
protected abstract Task<DbDataReader> ExecuteDbDataReaderAsync( | ||
CommandBehavior behavior, | ||
CancellationToken cancellationToken); | ||
|
||
public abstract int ExecuteNonQuery(); | ||
|
||
public abstract Task<int> ExecuteNonQueryAsync(CancellationToken cancellationToken = default); | ||
|
||
public abstract object? ExecuteScalar(); | ||
|
||
public abstract Task<object?> ExecuteScalarAsync(CancellationToken cancellationToken = default); | ||
|
||
public abstract void Prepare(); | ||
|
||
public abstract Task PrepareAsync(CancellationToken cancellationToken = default); | ||
|
||
public abstract void Cancel(); | ||
|
||
public DbBatchCommand CreateBatchCommand() => CreateDbBatchCommand(); | ||
|
||
protected abstract DbBatchCommand CreateDbBatchCommand(); | ||
|
||
public virtual void Dispose() {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. |
||
|
||
public virtual ValueTask DisposeAsync() | ||
{ | ||
Dispose(); | ||
return default; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.ComponentModel; | ||
using System.Diagnostics.CodeAnalysis; | ||
|
||
namespace System.Data.Common | ||
{ | ||
public abstract class DbBatchCommand | ||
{ | ||
public abstract string CommandText { get; set; } | ||
|
||
public abstract CommandType CommandType { get; set; } | ||
|
||
public abstract int RecordsAffected { get; } | ||
|
||
public DbParameterCollection Parameters => DbParameterCollection; | ||
|
||
protected abstract DbParameterCollection DbParameterCollection { get; } | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Collections; | ||
using System.Collections.Generic; | ||
|
||
namespace System.Data.Common | ||
{ | ||
public abstract class DbBatchCommandCollection : IList<DbBatchCommand> | ||
{ | ||
public abstract IEnumerator<DbBatchCommand> GetEnumerator(); | ||
|
||
IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); | ||
|
||
public abstract void Add(DbBatchCommand item); | ||
|
||
public abstract void Clear(); | ||
|
||
public abstract bool Contains(DbBatchCommand item); | ||
|
||
public abstract void CopyTo(DbBatchCommand[] array, int arrayIndex); | ||
|
||
public abstract bool Remove(DbBatchCommand item); | ||
|
||
public abstract int Count { get; } | ||
|
||
public abstract bool IsReadOnly { get; } | ||
|
||
public abstract int IndexOf(DbBatchCommand item); | ||
|
||
public abstract void Insert(int index, DbBatchCommand item); | ||
|
||
public abstract void RemoveAt(int index); | ||
|
||
public abstract DbBatchCommand this[int index] { get; set; } | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).