-
Notifications
You must be signed in to change notification settings - Fork 930
Support for DbBatch #3461
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
Support for DbBatch #3461
Conversation
|
Just a thought for the discussion (do not need to implement straight away). The .NET Core since 3 supports default interface members. With it we can add methods to interfaces without introducing breaking changes (it is similar to adding a virtual method for the class in prior versions). Since the DbBatch is .NET 6 and above we could use default methods to extend interfaces instead of doing our usual shimming. This should simplify code a little bit. |
Yes! I wanted to use that but didn't realize the compiler directives actually meant that I could. Fixed. |
|
We could make it select the DbBatchBatcher automatically when possible, but perhaps that is something we should/could wait with. |
|
@hazzik Thanks for helping out! A couple of ideas.
|
fredericDelaporte
left a comment
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.
This looks good to me excepted a few commented details.
hazzik
left a comment
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.
Can you please change not implemented exceptions to not supported?
|
I intend merging this in the coming days (up to next Sunday). |
Co-authored-by: Frédéric Delaporte <12201973+fredericDelaporte@users.noreply.github.com>
Co-authored-by: Frédéric Delaporte <12201973+fredericDelaporte@users.noreply.github.com>
92cc9fd to
ac7d1e3
Compare
fredericDelaporte
left a comment
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.
Rebased for solving a conflict.
Observations
A DbBatch looks like a DbCommand, but just on the surface.
Unlike batching which uses an underlying
DbCommand,DbBatchwill require special handling for things such asPrepare,Enlistetc. While theDbBatchcould theoretically be wrapped in aDbCommand"adapter" or even a NH specific "DbCommandOrBatch", sinceDbBatchis now a part of the core .NET 6+ API, I think the route forward is to be explicit about it, e.g. having bothEnlist(DbCommand)andEnlist(DbBatch).This means that a transparent move is harder, since custom derivatives of e.g MicrosoftDataSqlClientDriver may have vital implementations of e.g
AdjustCommandthat will have to be duplicated forDbBatch.No really good way to create a DbBatchCommand from a DbCommand
If I'm not missing something, I think this is a bit of an oversight on the part of the
DbBatchcreators, especially since they already use "CanDoXxxx" properties.All the parameters in a
DbCommandwill have to be recreated for theDbBatchCommand, since they can't be attached to twoDbParameterCollectionsat the same time. The good ol'SqlCommandSetuses a bit of trickery for that, but I just opted to make sure that theDbParameterisICloneable, and used that. Perhaps the value should be explicitly cloned too, if it'sICloneable. Making it possible to override this logic is probably a good idea.ReflectionBasedDriver doesn't use DbProviderFactoryDriveConnectionCommandProvider on .NET 5 and above
Any reason for this, or just an effect of NETSTANDARD2_1_OR_GREATER not including NET5 etc.?