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

Support for DbBatch #3461

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Support for DbBatch #3461

wants to merge 8 commits into from

Conversation

gliljas
Copy link
Member

@gliljas gliljas commented Dec 14, 2023

Observations

A DbBatch looks like a DbCommand, but just on the surface.
Unlike batching which uses an underlying DbCommand, DbBatch will require special handling for things such as Prepare, Enlist etc. While the DbBatch could theoretically be wrapped in a DbCommand "adapter" or even a NH specific "DbCommandOrBatch", since DbBatch is now a part of the core .NET 6+ API, I think the route forward is to be explicit about it, e.g. having both Enlist(DbCommand) and Enlist(DbBatch).

This means that a transparent move is harder, since custom derivatives of e.g MicrosoftDataSqlClientDriver may have vital implementations of e.g AdjustCommand that will have to be duplicated for DbBatch.

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 DbBatch creators, especially since they already use "CanDoXxxx" properties.
All the parameters in a DbCommand will have to be recreated for the DbBatchCommand, since they can't be attached to two DbParameterCollections at the same time. The good ol' SqlCommandSet uses a bit of trickery for that, but I just opted to make sure that the DbParameter is ICloneable, and used that. Perhaps the value should be explicitly cloned too, if it's ICloneable. 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.?

@gliljas gliljas changed the title WIP: Support for DbBatch Support for DbBatch Mar 14, 2024
@hazzik
Copy link
Member

hazzik commented Mar 15, 2024

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.

@gliljas
Copy link
Member Author

gliljas commented Mar 15, 2024

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.

@gliljas
Copy link
Member Author

gliljas commented Mar 15, 2024

We could make it select the DbBatchBatcher automatically when possible, but perhaps that is something we should/could wait with.

@gliljas
Copy link
Member Author

gliljas commented Apr 23, 2024

@hazzik Thanks for helping out!

A couple of ideas.

  1. Maybe we should add the commands to a List instead of directly to the batch. It would make reuse of e.g existing AdjustCommand implementations easier (we'd call both AdjustCommand and AdjustBatch), and if the batch only contains one command, there no need to add the overhead of creating a batch and cloning the command. Perhaps the IDriver method should be CreateDbBatchFromDbCommands, and it will be called just before executing the batch.
  2. The CanCreateBatch naming is maybe a bit off, since it answers the question "Can you create batches and can you create batch commands from dbcommands"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants