Skip to content

Merge | Managed SNI #3345

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

Open
wants to merge 29 commits into
base: main
Choose a base branch
from
Open

Merge | Managed SNI #3345

wants to merge 29 commits into from

Conversation

benrr101
Copy link
Contributor

Description: This PR looks a lot bigger than it really is. The goal here is to move all the files for netcore's managed SNI implementation into the common project. I also took the liberty to make some other changes at the same time.

  • Move all SNI files to the common project
  • Suffix all managed SNI files with ".netcore"
  • Wrap all files in #if NET
  • Rename SNI namespace to ManagedSni - I feel there's a good chance we can (and should) separate native SNI implementation into its own namespace as well, so having this separation will be useful
  • Rename SNI to Sni to follow naming conventions

Testing: Project builds on windows and linux. There is no change to functionality, just moving code around.

@benrr101 benrr101 added this to the 6.1-preview2 milestone May 13, 2025
@benrr101 benrr101 requested review from a team and Copilot May 13, 2025 18:52
@benrr101 benrr101 added the Common Project 🚮 Things that relate to the common project project label May 13, 2025
Copy link

@Copilot Copilot AI 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 moves all .NET Core managed SNI implementation files into the common project while updating their file names, namespace, and type names to follow new naming conventions. Key changes include moving files to a shared location, renaming types (e.g. SNIPacket to SniPacket, SNIAsyncCallback to SniAsyncCallback), and wrapping the code with “#if NET” conditionals.

Reviewed Changes

Copilot reviewed 35 out of 35 changed files in this pull request and generated no comments.

File Description
SniPacket.netcore.cs Renaming of SNIPacket to SniPacket and updating internal references consistently.
SniNpHandle.netcore.cs Updated namespace and type names from SNINpHandle to SniNpHandle with consistent logging calls.
SniMarsHandle.netcore.cs Renamed classes and updated event/logging references to adhere to the new ManagedSni naming conventions.
(Other files) Similar renaming updates and #if NET wrapping applied across the managed SNI implementation files.
Comments suppressed due to low confidence (3)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniPacket.netcore.cs:15

  • Ensure that the renaming from SNIPacket to SniPacket and similar types consistently uses the new naming convention (lowercase 'i') across the entire file.
namespace Microsoft.Data.SqlClient.ManagedSni

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniCommon.netcore.cs:63

  • Verify that all log calls and references have been updated to use 'SniCommon' consistently, matching the updated naming convention.
SqlClientEventSource.Log.TrySNITraceEvent(nameof(SniCommon), ...

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniMarsHandle.netcore.cs:293

  • Double-check that internal event scope usages reflect 'SniMarsHandle' accurately to avoid mis-references following the renaming.
using (TrySNIEventScope.Create(nameof(SniMarsHandle)))

Copy link
Contributor

@mdaigle mdaigle left a comment

Choose a reason for hiding this comment

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

All the changes look good to me. I'll take another look and approve when the CI is passing.

paulmedynski
paulmedynski previously approved these changes May 16, 2025
paulmedynski
paulmedynski previously approved these changes May 20, 2025
Copy link

codecov bot commented May 21, 2025

Codecov Report

Attention: Patch coverage is 70.71429% with 123 lines in your changes missing coverage. Please review.

Project coverage is 65.27%. Comparing base (072f318) to head (23b0553).

Files with missing lines Patch % Lines
.../Data/SqlClient/ManagedSni/SniTcpHandle.netcore.cs 62.85% 26 Missing ⚠️
...soft/Data/SqlClient/ManagedSni/SniProxy.netcore.cs 33.33% 22 Missing ⚠️
...t/Data/SqlClient/ManagedSni/SniNpHandle.netcore.cs 60.00% 18 Missing ⚠️
...ta/SqlClient/ManagedSni/LocalDB.netcore.Windows.cs 0.00% 11 Missing ⚠️
.../SqlClient/ManagedSni/SniMarsConnection.netcore.cs 75.00% 11 Missing ⚠️
...Data/SqlClient/ManagedSni/SniMarsHandle.netcore.cs 87.50% 8 Missing ⚠️
...oft/Data/SqlClient/ManagedSni/SniCommon.netcore.cs 58.82% 7 Missing ⚠️
...ft/Data/SqlClient/ManagedSni/SsrpClient.netcore.cs 50.00% 7 Missing ⚠️
...qlClient/ManagedSni/SslOverTdsStream.NetCoreApp.cs 0.00% 4 Missing ⚠️
...a/SqlClient/ManagedSni/SniNetworkStream.netcore.cs 85.71% 3 Missing ⚠️
... and 3 more
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3345   +/-   ##
=======================================
  Coverage   65.26%   65.27%           
=======================================
  Files         299      300    +1     
  Lines       65640    65640           
=======================================
+ Hits        42842    42846    +4     
+ Misses      22798    22794    -4     
Flag Coverage Δ
addons 92.58% <ø> (ø)
netcore 68.48% <70.71%> (+0.02%) ⬆️
netfx 66.54% <ø> (+0.01%) ⬆️

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.

mdaigle
mdaigle previously approved these changes May 21, 2025
# Conflicts:
#	src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj
#	src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/PacketHandle.Windows.cs
paulmedynski
paulmedynski previously approved these changes May 29, 2025
mdaigle
mdaigle previously approved these changes May 29, 2025
@benrr101 benrr101 dismissed stale reviews from mdaigle and paulmedynski via 23b0553 May 29, 2025 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Common Project 🚮 Things that relate to the common project project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants