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

Potential bug in CommandBatchPreparer.BatchCommands related to parameterNameGenerator #23281

Closed
MaxG117 opened this issue Nov 12, 2020 · 2 comments · Fixed by #28721
Closed
Assignees
Labels
area-save-changes closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported punted-for-6.0 type-bug
Milestone

Comments

@MaxG117
Copy link

MaxG117 commented Nov 12, 2020

I suspect there's a bug in CommandBatchPreparer.BatchCommands or maybe an incorrect assumption about database providers. When a modificationCommand cannot be added to the current batch, this method yield returns the current batch, then places this modificationCommand into a new batch, which resets the parameterNameGenerator:
batch = StartNewBatch(parameterNameGenerator, modificationCommand);

But this modificationCommand already has parameter names that were generated by the parameterNameGenerator before it was reset to 0. For example, if the current batch has 300 parameters and a command with 301st parameter could not be added, this command, with a parameter named "p300", will become the first command of the new batch. If this new batch can fit more parameters than the previous batch, then parameterNameGenerator will generate "p300" again, and ReaderModificationCommandBatch.CreateStoreCommand() will fail with an ArgumentException because of a parameter name collision in the parameterValues dictionary.

Perhaps other database providers are not as flexible, and a subsequent batch can never have more parameters than a previous batch and this problem never manifests. Our database provider dynamically calculates the number of statements and parameters that can fit in a batch, so it is possible for batches (even those made up of identical statements) to have a different number of statements and parameters.

Ideally the parameter names in the modificationCommand that didn't fit should be regenerated starting with "p0". I don't see an easy way to do that. Another solution would be to not reset the parameterNameGenerator when a new batch is created, but that's a bit wasteful.

Provider and version information

EF Core version: 3.1.x
Database provider: the one I'm working on

@AndriySvyryd
Copy link
Member

@MaxG117 For other providers AddCommand implementation doesn't always get the parameter names from the ColumnModifications for performance reasons, so it's just very unlikely that this situation arises.

I'll add API to reset the generated parameter names in the ModificationCommand that wasn't added.

If this is blocking you can use reflection to reset _parameterName or _originalParameterName as a workaround for now.

@roji
Copy link
Member

roji commented Aug 15, 2022

This has already been fixed in 7.0: when adding a command fails, we roll back the batch, and that now includes resetting the parameter name on the column modifications (code). Submitting a PR with a regression test.

@roji roji added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Aug 15, 2022
roji added a commit to roji/efcore that referenced this issue Aug 15, 2022
roji added a commit to roji/efcore that referenced this issue Aug 15, 2022
@ajcvickers ajcvickers modified the milestones: 7.0.0, 7.0.0-rc1 Aug 16, 2022
@ajcvickers ajcvickers modified the milestones: 7.0.0-rc1, 7.0.0 Nov 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-save-changes closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported punted-for-6.0 type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants