SqlBatchCommand support CommandBehavior#4125
Conversation
campersau
force-pushed
the
GetColumnSchemaKeyInfo
branch
from
0629d0c to
19a768e
Compare
campersau
force-pushed
the
GetColumnSchemaKeyInfo
branch
from
19a768e to
fe1ee31
Compare
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4125 +/- ##
==========================================
- Coverage 65.94% 64.62% -1.32%
==========================================
Files 277 270 -7
Lines 42949 65812 +22863
==========================================
+ Hits 28321 42531 +14210
- Misses 14628 23281 +8653
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mdaigle
left a comment
mdaigle
left a comment
There was a problem hiding this comment.
Overall, I have some concerns about existing behavior that we have copied over without explanation and I would like to remove any unnecessary refactors.
github-project-automation
Bot
moved this from In review
to In progress
in SqlClient Board
|
Thank you for making those changes. I'll do another review pass on Monday. |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
mdaigle
left a comment
mdaigle
left a comment
There was a problem hiding this comment.
I'm trying to think through the best way to expose things on SqlBatchCommand. It already accepts a CommandBehavior, and this PR now pipes that through. But certain behaviors are not meaningful. SingleResult, SingleRow, SequentialAccess, and CloseConnection all only apply at the top level (SqlBatch). I'm thinking we should add some validation to SqlBatchCommand that limits the CommandBehavior values we can pass in, or add some explanation to the public docs that most options will be no-ops.
|
Throwing exceptions is a breaking change, but not sure if somebody used it. So I am fine either way (validation or doc or both). |
mdaigle
left a comment
mdaigle
left a comment
There was a problem hiding this comment.
Those changes look good. Let's note the restriction in a doc comment.
|
I updated the doc comments. |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
4 participants
Description
#1825 added the Batch APIs and also added a
CommandBehaviorproperty toSqlBatchCommandwhich was completly unused.Note:
SqlBatchCommand.CommandBehavioris a custom property in SqlClient which isn't in the DbBatchCommandAlso the
CommandBehaviorforExecuteReaderwas ignored.This PR enables the use of the
SqlBatchCommand.CommandBehaviorand also supports passingCommandBehaviortoExecuteReader.Issues
Fixes #4083
Testing
Added tests for passing
CommandBehavior.SchemaOnly | CommandBehavior.KeyInfotoSqlBatchCommand.CommandBehavior.Added tests for passing
CommandBehavior.CloseConnectiontoExecuteReader.