Merge | SqlCommand The Mop Up #3738
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…lbak, CancelIgnoreFailure
…cachedMetaData, _reconnectionCompletionSource, _intrnalEndExecuteInitiated, CachingQueryMetadataPostponed, IsProviderRetriable, ProcParamsColIndex, PreSql2008ProcParamsNames, Sql2008ProcParamsNames, MetaData
…d renamed synchronous isSynchronous)
…ctionClosed (rewritten as ?.), NotifyDependency (rewritten as ?.)
…ion with Span<string>)
…and GetParameterCount
…g), GetResetOptionsString (made static, renamed to GetOptionsResetString), GetCommandText
… remove boolean "found" flag and just return directly)
…Notification, and netfx-only SqlNotificationContext
benrr101
added
the
Common Project 🚮
There was a problem hiding this comment.
Pull Request Overview
This PR consolidates SqlCommand platform-specific code by removing separate SqlCommand.netfx.cs and SqlCommand.netcore.cs files and moving shared implementation into the main SqlCommand.cs file. The changes also create a new SqlCommand.Batch.cs partial file for batch-related functionality and update method signatures for consistency.
Key Changes:
- Eliminated platform-specific SqlCommand partial files (netfx and netcore)
- Consolidated shared implementation into main SqlCommand.cs
- Created SqlCommand.Batch.cs for batch RPC operations
- Renamed parameter
synchronoustoisSynchronousin WriteEndExecuteEvent calls
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| SqlCommand.cs | Added consolidated implementation including AsyncState, batch fields, and helper methods previously split across platform files |
| SqlCommand.Batch.cs | New partial containing batch RPC mode methods |
| SqlCommand.Xml.cs | Updated parameter name from synchronous to isSynchronous |
| SqlCommand.Scalar.cs | Updated parameter name from synchronous to isSynchronous |
| SqlCommand.Reader.cs | Updated parameter name and added explicit parameter for GetRPCObject and BuildParamList calls |
| SqlCommand.NonQuery.cs | Updated parameter name from synchronous to isSynchronous |
| SqlCommand.Encryption.cs | Added explicit forSpDescribeParameterEncryption parameter to GetRPCObject call |
| SqlCommand.netfx.cs | File deleted - content moved to shared files |
| SqlCommand.netcore.cs | File deleted - content moved to shared files |
| Microsoft.Data.SqlClient.csproj (both) | Removed references to deleted platform files, added reference to SqlCommand.Batch.cs |
| SqlUtil.cs | Added TODO comment for PendingBeginXXXExists method |
| return; | ||
| } | ||
|
|
||
| // Remove the entry from the caceh since it was inconsistent |
There was a problem hiding this comment.
Corrected spelling of 'cache' to 'cache'.
Suggested change
| // Remove the entry from the caceh since it was inconsistent | |
| // Remove the entry from the cache since it was inconsistent |
| options |= TdsEnums.RPC_PARAM_DEFAULT; | ||
| } | ||
|
|
||
| // Detect incorrectly derived type names unchanged yb the caller and fix |
There was a problem hiding this comment.
Corrected spelling of 'by' to 'by'.
Suggested change
| // Detect incorrectly derived type names unchanged yb the caller and fix | |
| // Detect incorrectly derived type names unchanged by the caller and fix |
Comment on lines
513
to
531
| sqlParam.Size = paramList.Length; | ||
| sqlParam.Value = paramList; |
There was a problem hiding this comment.
Lines 529-530 swap the order of setting Size and Value properties. This change appears to be incidental to the refactoring and should be called out explicitly if intentional, or reverted if unintentional. The original order was Value then Size.
Suggested change
| sqlParam.Size = paramList.Length; | |
| sqlParam.Value = paramList; | |
| sqlParam.Value = paramList; | |
| sqlParam.Size = paramList.Length; |
There was a problem hiding this comment.
Theoretically should make no difference what order they go in, but ok
Comment on lines
2127
to
2136
| if (part is not null && 2 <= part.Length) | ||
| { | ||
| if (part[0] == '[' && part[part.Length - 1] == ']') | ||
| { | ||
| // strip outer '[' & ']' | ||
| part = part.Substring(1, part.Length - 2); | ||
|
|
||
| // undo quoted "]" from "]]" to "]" | ||
| part = part.Replace("]]", "]"); | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
Suggested change
| if (part is not null && 2 <= part.Length) | |
| { | |
| if (part[0] == '[' && part[part.Length - 1] == ']') | |
| { | |
| // strip outer '[' & ']' | |
| part = part.Substring(1, part.Length - 2); | |
| // undo quoted "]" from "]]" to "]" | |
| part = part.Replace("]]", "]"); | |
| } | |
| if (part is not null && part.Length >= 2 && part[0] == '[' && part[part.Length - 1] == ']') | |
| { | |
| // strip outer '[' & ']' | |
| part = part.Substring(1, part.Length - 2); | |
| // undo quoted "]" from "]]" to "]" | |
| part = part.Replace("]]", "]"); |
Comment on lines
3231
to
3235
| if (!parser.MARSOn) | ||
| { | ||
| if (activeConnection.AsyncCommandInProgress) | ||
| throw SQL.MARSUnsupportedOnConnection(); | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
Suggested change
| if (!parser.MARSOn) | |
| { | |
| if (activeConnection.AsyncCommandInProgress) | |
| throw SQL.MARSUnsupportedOnConnection(); | |
| } | |
| if (!parser.MARSOn && activeConnection.AsyncCommandInProgress) | |
| throw SQL.MARSUnsupportedOnConnection(); |
| { | ||
| Cancel(); | ||
| } | ||
| catch |
There was a problem hiding this comment.
Generic catch clause.
Suggested change
| catch | |
| catch (Exception) |
Comment on lines
2457
to
2546
| catch (Exception e) | ||
| { | ||
| // Put the state object back in the cache. | ||
| // Do not reset the async state, since this is managed by the user Begin/End, | ||
| // not internally. | ||
| if (ADP.IsCatchableExceptionType(e)) | ||
| { | ||
| ReliablePutStateObject(); | ||
| } | ||
|
|
||
| // Check if we have an error we can retry | ||
| bool shouldRetry = e is EnclaveDelegate.RetryableEnclaveQueryExecutionException; | ||
| if (e is SqlException sqlException) | ||
| { | ||
| for (int i = 0; i < sqlException.Errors.Count; i++) | ||
| { | ||
| bool isConversionError = | ||
| sqlException.Errors[i].Number == TdsEnums.TCE_CONVERSION_ERROR_CLIENT_RETRY; | ||
| bool isEnclaveSessionError = | ||
| sqlException.Errors[i].Number == TdsEnums.TCE_ENCLAVE_INVALID_SESSION_HANDLE; | ||
|
|
||
| if ((usedCache && isConversionError) || | ||
| (ShouldUseEnclaveBasedWorkflow && isEnclaveSessionError)) | ||
| { | ||
| shouldRetry = true; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (!shouldRetry) | ||
| { | ||
| // If we cannot retry, reset the async state to make sure we leave is clean. | ||
| CachedAsyncState?.ResetAsyncState(); | ||
|
|
||
| try | ||
| { | ||
| _activeConnection.GetOpenTdsConnection().DecrementAsyncCount(); | ||
| globalCompletion.TrySetException(e); | ||
| } | ||
| catch (Exception e2) | ||
| { | ||
| globalCompletion.TrySetException(e2); | ||
| } | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| // Remove the entry from the caceh since it was inconsistent | ||
| SqlQueryMetadataCache.GetInstance().InvalidateCacheEntry(this); | ||
| InvalidateEnclaveSession(); | ||
|
|
||
| // Kick off the retry | ||
| try | ||
| { | ||
| _internalEndExecuteInitiated = false; | ||
| Task<object> retryTask = (Task<object>)retryFunc( | ||
| this, | ||
| behavior, | ||
| /*asyncCallback:*/ null, // @TODO: Is this ever not null? | ||
| stateObject, | ||
| TdsParserStaticMethods.GetRemainingTimeout(timeout, firstAttemptStart), | ||
| /*isRetry:*/ true, | ||
| asyncWrite); | ||
|
|
||
| retryTask.ContinueWith( | ||
| static (retryTask, state) => | ||
| { | ||
| TaskCompletionSource<object> completionSource = (TaskCompletionSource<object>)state; | ||
| if (retryTask.IsFaulted) | ||
| { | ||
| completionSource.TrySetException(retryTask.Exception.InnerException); | ||
| } | ||
| else if (retryTask.IsCanceled) | ||
| { | ||
| completionSource.TrySetCanceled(); | ||
| } | ||
| else | ||
| { | ||
| completionSource.TrySetResult(retryTask.Result); | ||
| } | ||
| }, | ||
| state: globalCompletion, | ||
| TaskScheduler.Default); | ||
| } | ||
| catch (Exception e2) | ||
| { | ||
| globalCompletion.TrySetException(e2); | ||
| } | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
| _activeConnection.GetOpenTdsConnection().DecrementAsyncCount(); | ||
| globalCompletion.TrySetException(e); | ||
| } | ||
| catch (Exception e2) |
There was a problem hiding this comment.
Generic catch clause.
Suggested change
| catch (Exception e2) | |
| catch (Exception e2) when (!(e2 is OutOfMemoryException || e2 is StackOverflowException || e2 is ThreadAbortException)) |
Comment on lines
+2542
to
+2545
| catch (Exception e2) | ||
| { | ||
| globalCompletion.TrySetException(e2); | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
* fAddSeparation => fAddSeparator * revert order or scale/precision * value => val * FIX BUG numChars GetActualScale => GetActualSize
…e made - will annotate.
benrr101
commented
Nov 3, 2025
| int actualBytes = parser.GetEncodingCharLength( | ||
| value: s, | ||
| numChars: sqlParam.GetActualScale(), | ||
| numChars: sqlParam.GetActualSize(), |
There was a problem hiding this comment.
Bug was here - number of characters should be Size not Scale
| string s = null; | ||
| if (behavior is CommandBehavior.SchemaOnly or CommandBehavior.KeyInfo) | ||
|
|
||
| bool hasKeyInfo = (behavior & CommandBehavior.KeyInfo) == CommandBehavior.KeyInfo; |
There was a problem hiding this comment.
Wasn't confident in using is here since CommandBehavior is a [Flags] enum. And didn't like how the checks were so long and used >once, so I factored them out.
| string s = null; | ||
|
|
||
| if (behavior is CommandBehavior.SchemaOnly) | ||
| if ((behavior & CommandBehavior.SchemaOnly) == CommandBehavior.SchemaOnly) |
There was a problem hiding this comment.
Not confident on using is on a [Flags] enum.
| // Ensure that the connection is open and that the parser is in the correct state | ||
| // @TODO: Remove cast when possible. | ||
| SqlInternalConnectionTds tdsConnection = (SqlInternalConnectionTds)_activeConnection.InnerConnection; | ||
| SqlInternalConnectionTds tdsConnection = _activeConnection.InnerConnection as SqlInternalConnectionTds; |
There was a problem hiding this comment.
Further down we check for null. as operator will give null if it's not the right type, but a straight cast will throw an invalid cast exception. Changed back to as to preserve original behavior.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
In the final installment of merging the SqlCommand class, I've mopped up the remaining methods and whatnot into the main SqlCommand partial and a new partial for batch RPC methods (not convinced this is the right idea, but otherwise the main partial becomes way too big, imo). This PR completes the SqlCommand merge (and leaves one class left to merge). As with the other PRs as part of this series, each commit merges a small collection of members. It is recommended to step through the commits one at a time to review.
This PR merges the following members:
Issues
More work towards #1261
Testing
CI should validate 😬