Skip to content
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

Add CreateParameter() method (and CanCreateParameter) to DbBatchCommand #82326

Closed
SoftStoneDevelop opened this issue Feb 18, 2023 · 13 comments · Fixed by #89503
Closed

Add CreateParameter() method (and CanCreateParameter) to DbBatchCommand #82326

SoftStoneDevelop opened this issue Feb 18, 2023 · 13 comments · Fixed by #89503
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Data
Milestone

Comments

@SoftStoneDevelop
Copy link

SoftStoneDevelop commented Feb 18, 2023

Thanks to @SoftStoneDevelop who posted the original issue; I've moved the original proposal to the bottom - it's very similar.

The DbBatch abstraction was added to System.Data in .NET 6.0 (issue). In a nutshell, it allows sending multiple SQL statements to the database in a single roundtrip through the use of multiple DbBatchCommands, each representing a statement. This is superior in various ways to the classical way of implementing batching in ADO.NET, which is to insert multiple semicolon-separated SQL statements into DbCommand.CommandText.

Unfortunately, due to an oversight, an API is missing on this abstraction for creating a a DbParameters, which are needed for parameterizing SQL.

  • Code that targets a specific database can simply instantiate a parameter directly (e.g. SqlParameter), but database-agnostic code which doesn't target a specific database provider requires factory methods to construct System.Data objects.
  • The main place for such factory methods is the DbProviderFactory abstraction, which exposes e.g. CreateParameter(). However, in various scenarios, a library or method is only passed a DbConnection directly, and does not have access to the driver's DbProviderFactory. This is notably the case for the popular Dapper micro-ORM (which is the main motivation behind this proposal).
  • To support these scenarios, DbCommand also exposes a CreateParameter() factory method, allowing anyone with a connection (or even just a command) to create parameters.
  • However, neither DbBatch nor its contains DbBatchCommand expose a similar method.

API proposal

class DbBatchCommand
{
    public virtual DbParameter CreateParameter()
        => throw new NotSupportedException();

    public virtual bool CanCreateParameter
        => false;
}

Notes

  • The proposal adds the CreateParameter method to DbBatchCommand, not to DbBatch; the former is where the parameters actually live, and is the analog of DbCommand in that sense. It's also conceivable that a single DbBatchCommand be passed around, and that a method accepting it would want to add parameters to it.
  • This method should have been abstract, but introducing it at this point would break all DbBatchCommand implementations. Instead, the proposal adds a method which throws by default; all providers implementing DbBatch will be expected to override it.
  • Unfortunately, DbBatchCommand doesn't reference DbConnection or similar, so there's no way to provide a default implementation which would access the CreateParameter() method e.g. on DbCommand. If this API were placed on DbBatch, it could access the DbConnection instance for that batch, create a command and call its CreateProvider method; however, this would have been of limited value since DbBatch can exist without an assigned connection, at which point this API would have to throw anyway. So it seems preferable to have the new API on DbBatchCommand where it belongs.
  • CanCreateParameter can be used to discover whether calling CreateParameter will throw or not; providers are expected to override it to return true when implementing CreateParameter. While it's not expected that programs can continue in a useful way if they require CreateParameter() and it isn't supported, at the very least this would allow the consuming program to throw a meaningful exception.
  • CanCreateParameter is consistent with other CanCreate* capability properties in System.Data, specifically in DbProviderFactory (e.g. CanCreateBatch, CanCreateDataAdapter...).
  • We ideally want to get this into .NET 8.0, as it's blocking Dapper (/cc @mgravell) and other scenarios. Also, DbBatch is slowly being implemented (SqlClient support is underway, PR), so it's important to get this done earlier rather than later. Two mainstream System.Data providers have already implemented DbBatch (Npgsql and MySqlConnector).

Provider issues:

Original proposal by @SoftStoneDevelop

Background and motivation

It is not possible to create a parameter for a DbBatchCommand without knowing the specific parameter of a specific provider (for example, NpgsqlParameter). This makes it difficult to use Batch abstracted from the provider.

With the non batch query, we are all right:

public void Method(DbConnection connection)
{
    DbCommand command = connection.CreateCommand();
    DbParameter commandParametr = command.CreateParameter();
    //...
    command.Parameters.Add(commandParametr);
    //...
}

API Proposal

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; }

    public abstract DbParameter CreateParameter();
}

API Usage

public void Method(DbConnection connection)
{
    DbBatch batch = connection.CreateBatch();
    DbBatchCommand batchCommand = batch.CreateBatchCommand();
    //set batchCommand properties
    //...
    DbParametr batchParametr = batchCommand.CreateParameter();//!New Method!
    //set batchParametr properties
    //...
    batchCommand.Parameters.Add(batchParametr);
    batch.BatchCommands.Add(batchCommand);
    //execute batch
    //...
}
@SoftStoneDevelop SoftStoneDevelop added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Feb 18, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Feb 18, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Feb 18, 2023
@SoftStoneDevelop SoftStoneDevelop changed the title [API Proposal]: CreateParameter() method for DbBatchCommand class [API Proposal]: CreateParameter() method for DbBatchCommand Feb 18, 2023
@SoftStoneDevelop
Copy link
Author

SoftStoneDevelop commented Feb 18, 2023

For now to get around this is with a fake command:

public void Method(DbConnection connection)
{
    var fakeCommand = connection.CreateCommand();
    DbBatch batch = connection.CreateBatch();
    DbBatchCommand batchCommand = batch.CreateBatchCommand();
    //set batchCommand properties
    //...
    DbParametr batchParametr = fakeCommand.CreateParameter();
    //set batchParametr properties
    //...
    batchCommand.Parameters.Add(batchParametr);
    batch.BatchCommands.Add(batchCommand);
    fakeCommand.Dispose();
    //execute batch
    //...
}

@vcsjones vcsjones added area-System.Data and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Feb 18, 2023
@ghost
Copy link

ghost commented Feb 18, 2023

Tagging subscribers to this area: @roji, @ajcvickers
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

It is not possible to create a parameter for a DbBatchCommand without knowing the specific parameter of a specific provider (for example, NpgsqlParameter). This makes it difficult to use Batch abstracted from the provider.

With the non batch query, we are all right:

public void Method(DbConnection connection)
{
    DbCommand command = connection.CreateCommand();
    DbParameter commandParametr = command.CreateParameter();
    //...
    command.Parameters.Add(commandParametr);
    //...
}

API Proposal

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; }

    public abstract DbParameter CreateParameter();
}

API Usage

public void Method(DbConnection connection)
{
    DbBatch batch = connection.CreateBatch();
    DbBatchCommand batchCommand = batch.CreateBatchCommand();
    //set batchCommand properties
    //...
    DbParametr batchParametr = batchCommand.CreateParameter();//!New Method!
    //set batchParametr properties
    //...
    batchCommand.Parameters.Add(batchParametr);
    batch.BatchCommands.Add(batchCommand);
    //execute batch
    //...
}
Author: SoftStoneDevelop
Assignees: -
Labels:

api-suggestion, area-System.Data, untriaged

Milestone: -

@roji
Copy link
Member

roji commented Feb 19, 2023

Thanks, I'll look into this soon. Note that you can also create a parameter with your driver's DbProviderFactory.

@SoftStoneDevelop
Copy link
Author

SoftStoneDevelop commented Feb 20, 2023

@roji
Yes, I can, but for this need to know which provider it is, but I don’t know that. Or do I need to pass the "DbProviderFactory" as a method parameter, which I don't want to do.

@mgravell
Copy link
Member

Echoing this need from Dapper; we don't usually have the provider-factory - just the connection and/or transaction; right now, the only way we'd be able to advance the DbBatch scenario would be to request an otherwise unused DbCommand for the purpose

mgravell added a commit to DapperLib/DapperAOT that referenced this issue Jul 17, 2023
@mgravell
Copy link
Member

Here's the best I can come up with for Dapper right now: https://github.com/DapperLib/DapperAOT/blob/abde6537c91959c7431883bdfce23870bc43f74a/test/Dapper.AOT.Test/Interceptors/ExecuteBatch.output.cs#L188-L215 - short version:

DbCommand? paramFactory = null;
DbParameter p;
// then for each parameter
p = (paramFactory ??= conn.CreateCommand()).CreateParameter();
// not shown: set name, value, etc of p
cmd.Parameters.Add(p);

@roji roji removed the untriaged New issue has not been triaged by the area owner label Jul 18, 2023
@roji roji self-assigned this Jul 18, 2023
@roji roji changed the title [API Proposal]: CreateParameter() method for DbBatchCommand Add CreateParameter() method (and CanCreateParameter) to DbBatchCommand Jul 24, 2023
@roji roji added api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work labels Jul 24, 2023
@roji roji added this to the 8.0.0 milestone Jul 24, 2023
@roji
Copy link
Member

roji commented Jul 24, 2023

Full proposal written above, /cc @mgravell @bgrainger @Wraith2 @ErikEJ @bricelam @ajcvickers @AndriySvyryd

@terrajobst
Copy link
Member

terrajobst commented Jul 25, 2023

Video

  • Looks good as proposed
namespace System.Data.Common;

public partial class DbBatchCommand
{
    public virtual DbParameter CreateParameter();
    public virtual bool CanCreateParameter { get; }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jul 25, 2023
@roji
Copy link
Member

roji commented Jul 26, 2023

One small additional note... In ADO.NET there's a "covariant" pattern that allows provider implementations to return more specific types. For example, the CreateParameter() implementation on DbCommand looks as follows:

public DbParameter CreateParameter() => CreateDbParameter();
protected abstract DbParameter CreateDbParameter();

A provider overrides the protected CreateDbParameter, but also hides the public non-virtual CreateParameter with a new CreateParameter that returns XyzParameter. Users using a specific provider (the majority) therefore get their provider's specific XyzParameter implementation rather than DbParameter.

In this API, I'm proposing simply using covariant return types, so a provider would be able to more simply do the following:

public partial class DbBatchCommand
{
    public virtual DbParameter CreateParameter();
}

// In provider code:
public partial class XyzBatchCommand
{
    public override XyzParameter CreateParameter()
        => new XyzParameter();
}

Let me know if anyone sees an issue with this.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 26, 2023
@roji roji added blocking Marks issues that we want to fast track in order to unblock other important work and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work labels Jul 26, 2023
@mgravell
Copy link
Member

mgravell commented Jul 26, 2023

re covariant: sounds fine (100% support this) as long as there's no intent to ever backport (which I think is a hard "no, this API is net8+ only"); IIRC this demands net6.0 and c# 9 (you can still do it without those manually via method hiding, but it is scrappier and you need an additional method - the point being: you need a different approach)

@roji
Copy link
Member

roji commented Jul 26, 2023

Yeah, I don't think backporting a new API to a patch release is on the table... Even if it were, we'd most probably only possibly consider backporting to 6.0 (since earlier is out of support now). A year from now net6.0 will also be out of support, and then this silliness will hopefully mostly be gone (assuming I badger the providers well enough that the virtual method is overridden across the board 😉 )

@Wraith2
Copy link
Contributor

Wraith2 commented Jul 26, 2023

If we do manage to expose batches to netfx in sqlclient it could be troublesome but I imagine we could just omit the variance since we'd be carrying the implementation of DbBatch ourselves. Still waiting on feedback on which way the sqliclient team want to go with that.

@roji
Copy link
Member

roji commented Jul 26, 2023

@Wraith2 yeah, in netfx you won't have a base class coming from the framework at all, so it shouldn't be a problem. The way I dealt with this in Npgsql is to copy in the runtime base classes under #if, so that your SqlBatch implementation code works either way. You'd be able to simply modify the copied base class to return SqlParameter instead (since no covariant return types there).

roji added a commit that referenced this issue Aug 1, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 1, 2023
@roji roji removed the blocking Marks issues that we want to fast track in order to unblock other important work label Aug 1, 2023
@dotnet dotnet locked as resolved and limited conversation to collaborators Aug 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Data
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants