-
Notifications
You must be signed in to change notification settings - Fork 300
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
Use string for SNI instead of byte[] #2790
Conversation
250c893
to
89d85fb
Compare
89d85fb
to
f45bf6b
Compare
f7be4fd
to
ccb77e8
Compare
52d058f
to
bbb5636
Compare
@mdaigle here's the next step on the SSPI change |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2790 +/- ##
==========================================
- Coverage 72.79% 66.20% -6.60%
==========================================
Files 283 281 -2
Lines 59139 58847 -292
==========================================
- Hits 43051 38960 -4091
- Misses 16088 19887 +3799
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIProxy.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/NativeSSPIContextProvider.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/SSPIContextProvider.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with this area of the code, so I reviewed for consistency and side-effects mainly.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Interop/Windows/Sni/SniNativeWrapper.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the naming and init comments. Just a couple more try-finally tweaks, and this will be good to go!
Looks good, and definitely simplifies the internal APIs, reducing cognitive load. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have one issue that I'd like addressed, then just a couple nits for naming consistency.
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectNative.cs
Outdated
Show resolved
Hide resolved
...icrosoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObject.netcore.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs
Show resolved
Hide resolved
...Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectManaged.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/BufferWriterExtensions.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/NativeSSPIContextProvider.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/NegotiateSSPIContextProvider.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/SSPIContextProvider.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/SSPIContextProvider.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlObjectPool.cs
Outdated
Show resolved
Hide resolved
…ent/TdsParserStateObjectNative.cs Co-authored-by: Malcolm Daigle <[email protected]>
…ent/TdsParserStateObject.netcore.cs Co-authored-by: Malcolm Daigle <[email protected]>
…ent/TdsParserStateObjectManaged.cs Co-authored-by: Malcolm Daigle <[email protected]>
…/NativeSSPIContextProvider.cs Co-authored-by: Malcolm Daigle <[email protected]>
…/SSPIContextProvider.cs Co-authored-by: Malcolm Daigle <[email protected]>
…/SSPIContextProvider.cs Co-authored-by: Malcolm Daigle <[email protected]>
Looks good. Will make sure the CI passes, then merge. |
(FYI there's an auto merge option you can enable in the settings to do so when all conditions are met) |
This change replaces byte[] usage for SNI values to be string. There was a lot of decoding done in different areas and with this it becomes clearer that the value is a server name. The only place that actually needs the
byte[]
representation is the native sni - so it makes it easier to keep it as a string as much as possible.Part of #2253