Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Fix | PoolGroupProviderInfo Typing#3417

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
benrr101 merged 2 commits intomainfromdev/russellben/fix-poolproviderinfo
Jun 13, 2025

Conversation

benrr101
Copy link
Contributor

Description

This PR does two main changes:

  1. Reverts a small change in netfx SqlConnectionFactory class fromCleanup | Remove Dead SMI Code (Part 1) #3393.
    I thought it was a safe change to make, but I neglected to notice that the poolgroupproviderinfo was being passed in from the caller of the method. I noticed this issue while working on merging SqlConnectionFactory, since I made this change on the netfx version but not on the netcore version.
  2. Makes SqlConnectionFactory CreateConnection classes have strongly typed poolgroupproviderinfo arguments.
    Not sure why these were always being passed in as object, but they are now being passed in as DbConnectionPoolGroupProviderInfo, which is what they should have been from the beginning.

Issues

No issue currently correspond to these changes.

Testing

Well, it still builds. Kinda disappointed the initial issue didn't cause CI failures.

Make pool provider info arguments in *ConnectionFactory methods strongly typed
@CopilotCopilotAI review requested due to automatic review settingsJune 12, 2025 22:49
@benrr101benrr101 requested a review froma team as acode ownerJune 12, 2025 22:49
@benrr101benrr101 added Area\NetfxIssues that apply only to .NET Framework target runtime or the 'netfx' project folder. Code Health 💊Issues/PRs that are targeted to source code quality improvements. labelsJun 12, 2025
Copy link
Contributor

@CopilotCopilotAI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Pull Request Overview

This PR restores the abstract signatures for pool provider info methods on the NetFX side and strengthens the typing ofpoolGroupProviderInfo across both NetFX and NetCoreSqlConnectionFactory implementations.

  • RevertedCreateConnectionPoolGroupProviderInfo in NetFX to its abstract form so callers pass a strongly typedDbConnectionPoolGroupProviderInfo.
  • Updated allCreateConnection overloads in both NetFX and NetCore to acceptDbConnectionPoolGroupProviderInfo instead ofobject.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

FileDescription
src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionFactory.csMadeCreateConnectionPoolProviderInfo andCreateConnectionPoolGroupProviderInfo abstract for strong typing.
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.csChanged constructor parameter fromobject providerInfo toDbConnectionPoolGroupProviderInfo and reformatted base call.
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnectionFactory.csUpdatedCreateConnection overrides to useDbConnectionPoolGroupProviderInfo, improved null‐propagation, and adjusted provider info propagation.
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnectionFactory.csMirrored strong-typing changes and reformattedCreateConnection call to use named arguments for clarity.
Comments suppressed due to low confidence (2)

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs:58

  • Consider using 'as SqlConnection' instead of a direct cast to avoid an InvalidCastException ifowningConnection is ever not a SqlConnection, matching the NetFX implementation.
SqlConnection sqlOwningConnection = (SqlConnection)owningConnection;

src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionFactory.cs:94

  • [nitpick] Add an XML documentation comment for this abstract method to explain its purpose and parameters, improving the clarity of the public API.
internal abstract DbConnectionPoolProviderInfo CreateConnectionPoolProviderInfo(

@saurabh500
Copy link
Contributor

Not sure why these were always being passed in as object, but they are now being passed in as DbConnectionPoolGroupProviderInfo, which is what they should have been from the beginning.

In System.Data.dll which had ODBC/OleDB/(once OracleClient) and SqlClient, the base classes for conn pool were being used to manage pools for all the providers.
DbConnectionPoolGroupProviderInfo was provider specific class and which was being passed to provider specific pool implementation (ODBC pool would have its own quirks and so would OleDb).

The code you are modifying was passthrough, which needed to pass around an object.
Now that there is only SqlClient and its pool, the change you have made, makes sense.

cheenamalhotra and benrr101 reacted with thumbs up emoji

Copy link
Contributor

@paulmedynskipaulmedynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

One question.

@codecovCodecov
Copy link

codecovbot commentedJun 13, 2025•
edited
Loading

Codecov Report

Attention: Patch coverage is90.00000% with2 lines in your changes missing coverage. Please review.

Project coverage is 63.58%. Comparing base(2385ca7) to head(2c161c3).
Report is 1 commits behind head on main.

Files with missing linesPatch %Lines
...c/Microsoft/Data/SqlClient/SqlConnectionFactory.cs50.00%2 Missing⚠️
Additional details and impacted files
@@            Coverage Diff            @@##           main    #3417       +/-   ##=========================================+ Coverage      0   63.58%   +63.58%=========================================  Files         0      299      +299       Lines         0    65418    +65418     =========================================+ Hits          0    41594    +41594- Misses        0    23824    +23824
FlagCoverage Δ
addons92.58% <ø> (?)
netcore66.87% <100.00%> (?)
netfx66.01% <60.00%> (?)

Flags with carried forward coverage won't be shown.Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report?Share it here.

🚀 New features to boost your workflow:
  • ❄️Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@benrr101benrr101 merged commit1058566 intomainJun 13, 2025
251 checks passed
@benrr101benrr101 deleted the dev/russellben/fix-poolproviderinfo branchJune 13, 2025 20:58
@paulmedynskipaulmedynski added this to the6.1-preview2 milestoneJun 23, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

Copilot code reviewCopilotCopilot left review comments

@cheenamalhotracheenamalhotracheenamalhotra approved these changes

@paulmedynskipaulmedynskipaulmedynski approved these changes

Assignees
No one assigned
Labels
Area\NetfxIssues that apply only to .NET Framework target runtime or the 'netfx' project folder.Code Health 💊Issues/PRs that are targeted to source code quality improvements.
Projects
None yet
Milestone
6.1-preview2
Development

Successfully merging this pull request may close these issues.

4 participants
@benrr101@saurabh500@cheenamalhotra@paulmedynski
[8]ページ先頭
©2009-2025 Movatter.jp