[DO NOT MERGE] Visuazlie diffs of feat/azure split #3901
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
- Added .NET 10 as a target for the Abstractions and Azure tests.
paulmedynski
added
the
DO NOT MERGE
Contributor
|
@paulmedynski Is this planned for the January preview? |
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request visualizes the differences between the main branch and the feat/azure-split feature branch. As explicitly stated in the description, this PR is not intended to be merged but rather serves to display the complete set of changes that will be submitted in smaller, logical chunks for proper review.
Changes:
- Restructures authentication providers by extracting Azure/Active Directory authentication into separate extension packages (Abstractions and Azure)
- Refactors build system to support multiple package generation with separate versioning for MDS, AKV Provider, SqlServer, and Azure packages
- Removes Azure.Core and Azure.Identity dependencies from the core MDS package
- Updates test infrastructure to work with the new package structure
Reviewed changes
Copilot reviewed 155 out of 155 changed files in this pull request and generated 23 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/targets/add-ons/GenerateAkvPackage.targets | New build target for generating AKV provider NuGet package |
| tools/targets/add-ons/GenerateAKVProviderNugetPackage.targets | Removed old AKV package generation target |
| tools/targets/GenerateThisAssemblyCs.targets | Enhanced to support configurable CLS compliance and namespace |
| tools/targets/GenerateSqlServerPackage.targets | New build target for SqlServer package generation |
| tools/targets/GenerateMdsPackage.targets | New build target for MDS package generation with dependency injection |
| tools/targets/CopySniDllsForNetFxProjectReferenceBuilds.targets | New target to handle SNI DLL copying for .NET Framework project builds |
| tools/specs/*.nuspec | Updated to use ReferenceType property and add AbstractionsPackageVersion dependencies |
| tools/props/Versions.props | Restructured versioning to support multiple packages with separate version properties |
| src/Microsoft.SqlServer.Server/StringsHelper.cs | Added braces around single-line if statements for consistency |
| src/Microsoft.Data.SqlClient/tests/tools/Microsoft.Data.SqlClient.TestUtilities/* | Updated test utilities to be netstandard2.0 library, removed Azure dependencies |
| src/Microsoft.Data.SqlClient/tests/UnitTests/* | Added new unit test for SqlAuthenticationProviderManager |
| src/Microsoft.Data.SqlClient/tests/ManualTests/* | Refactored AAD tests to use new provider architecture, added custom test providers |
| src/Microsoft.Data.SqlClient/tests/FunctionalTests/* | Updated authentication provider tests for new architecture |
| src/Microsoft.Data.SqlClient/tests/Directory.* | Cleaned up package references and build properties |
| src/Microsoft.Data.SqlClient/tests/Common/* | Updated SNI DLL handling |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/* | Removed ActiveDirectoryAuthenticationProvider, SqlAuthenticationProvider, SqlAuthenticationParameters, SqlAuthenticationToken; updated internal classes to use new token structure |
| src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj | Unified project file for all target frameworks, removed Azure dependencies, added Abstractions package reference |
| src/Microsoft.Data.SqlClient/netfx/src/* | Removed Azure dependencies, updated to reference Abstractions package |
| src/Microsoft.Data.SqlClient/netfx/ref/* | Removed authentication provider API surface from reference assembly |
| src/Microsoft.Data.SqlClient/netcore/src/* | Removed Azure dependencies, updated to reference Abstractions package |
| src/Microsoft.Data.SqlClient/netcore/ref/* | Removed authentication provider API surface from reference assembly |
| src/Microsoft.Data.SqlClient/add-ons/* | Simplified build properties, removed local package versioning |
| src/Microsoft.Data.SqlClient.sln | Added new Microsoft.Data.SqlClient.Extensions projects (Abstractions and Azure with tests) |
| src/Microsoft.Data.SqlClient.Extensions/Azure/test/* | New test project for Azure authentication provider |
| src/Microsoft.Data.SqlClient.Extensions/Azure/src/AzureVersions.props | Versioning properties for Azure package |
| @@ -0,0 +1,13 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
There was a problem hiding this comment.
The title of the PR contains a spelling error: "Visuazlie" should be "Visualize".
Comment on lines
+318
to
+336
| foreach (var key in keys) | ||
| { | ||
| if (!string.IsNullOrEmpty(key.Trim())) | ||
| { | ||
| bool removeKey = false; | ||
| foreach (var keyToRemove in keysToRemove) | ||
| { | ||
| if (key.Trim().ToLower().StartsWith(keyToRemove.Trim().ToLower(), StringComparison.Ordinal)) | ||
| { | ||
| removeKey = true; | ||
| break; | ||
| } | ||
| } | ||
| if (!removeKey) | ||
| { | ||
| res += key + ";"; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop looks as if it might be testing whether every sequence element satisfies a predicate - consider using '.All(...)'.
Comment on lines
+267
to
277
| foreach (SqlAuthenticationMethod candidateMethod in Instance._authenticationsWithAppSpecifiedProvider) | ||
| { | ||
| if (candidateMethod == authenticationMethod) | ||
| { | ||
| _sqlAuthLogger.LogError(nameof(SqlAuthenticationProviderManager), methodName, $"Failed to add provider {GetProviderType(provider)} because a user-defined provider with type {GetProviderType(_providers[authenticationMethod])} already existed for authentication {authenticationMethod}."); | ||
| return false; // return here to avoid replacing user-defined provider | ||
| Instance._sqlAuthLogger.LogError(nameof(SqlAuthenticationProviderManager), methodName, $"Failed to add provider {GetProviderType(provider)} because a user-defined provider with type {GetProviderType(Instance._providers[authenticationMethod])} already existed for authentication {authenticationMethod}."); | ||
|
|
||
| // The app has already specified a Provider for this | ||
| // authentication method, so we won't override it. | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
Comment on lines
+323
to
+330
| foreach (var keyToRemove in keysToRemove) | ||
| { | ||
| if (key.Trim().ToLower().StartsWith(keyToRemove.Trim().ToLower(), StringComparison.Ordinal)) | ||
| { | ||
| removeKey = true; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
Comment on lines
+347
to
+359
| foreach (string cp in connProps) | ||
| { | ||
| if (!string.IsNullOrEmpty(cp.Trim())) | ||
| { | ||
| foreach (var key in keys) | ||
| { | ||
| if (cp.Trim().ToLower().StartsWith(key.Trim().ToLower(), StringComparison.Ordinal)) | ||
| { | ||
| return cp.Substring(cp.IndexOf('=') + 1); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| private static readonly ConcurrentDictionary<PublicClientAppKey, IPublicClientApplication> s_pcaMap = new(); | ||
| private static readonly ConcurrentDictionary<TokenCredentialKey, TokenCredentialData> s_tokenCredentialMap = new(); | ||
| private static SemaphoreSlim s_pcaMapModifierSemaphore = new(1, 1); | ||
| private static SemaphoreSlim s_tokenCredentialMapModifierSemaphore = new(1, 1); |
There was a problem hiding this comment.
Field 's_tokenCredentialMapModifierSemaphore' can be 'readonly'.
| internal class TokenCredentialData | ||
| { | ||
| public TokenCredential _tokenCredential; | ||
| public byte[] _secretHash; |
There was a problem hiding this comment.
Field '_secretHash' can be 'readonly'.
| /// </summary> | ||
| private static readonly ConcurrentDictionary<PublicClientAppKey, IPublicClientApplication> s_pcaMap = new(); | ||
| private static readonly ConcurrentDictionary<TokenCredentialKey, TokenCredentialData> s_tokenCredentialMap = new(); | ||
| private static SemaphoreSlim s_pcaMapModifierSemaphore = new(1, 1); |
There was a problem hiding this comment.
Field 's_pcaMapModifierSemaphore' can be 'readonly'.
|
|
||
| internal class TokenCredentialData | ||
| { | ||
| public TokenCredential _tokenCredential; |
There was a problem hiding this comment.
Field '_tokenCredential' can be 'readonly'.
Comment on lines
+506
to
+539
| if (customWebUI != null) | ||
| { | ||
| return await app.AcquireTokenInteractive(scopes) | ||
| .WithCorrelationId(connectionId) | ||
| .WithCustomWebUi(customWebUI) | ||
| .WithLoginHint(userId) | ||
| .ExecuteAsync(ctsInteractive.Token) | ||
| .ConfigureAwait(false); | ||
| } | ||
| else | ||
| { | ||
| /* | ||
| * We will use the MSAL Embedded or System web browser which changes by Default in MSAL according to this table: | ||
| * | ||
| * Framework Embedded System Default | ||
| * ------------------------------------------- | ||
| * .NET Classic Yes Yes^ Embedded | ||
| * .NET Core No Yes^ System | ||
| * .NET Standard No No NONE | ||
| * UWP Yes No Embedded | ||
| * Xamarin.Android Yes Yes System | ||
| * Xamarin.iOS Yes Yes System | ||
| * Xamarin.Mac Yes No Embedded | ||
| * | ||
| * ^ Requires "http://localhost" redirect URI | ||
| * | ||
| * https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/MSAL.NET-uses-web-browser#at-a-glance | ||
| */ | ||
| return await app.AcquireTokenInteractive(scopes) | ||
| .WithCorrelationId(connectionId) | ||
| .WithLoginHint(userId) | ||
| .ExecuteAsync(ctsInteractive.Token) | ||
| .ConfigureAwait(false); | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement return - consider using '?' to express intent better.
Contributor
Author
|
@ErikEJ - Yes, 7.0 Preview 4. This PR won't be merged; the series noted in the description will contain the actual changes. |
Contributor
|
@paulmedynski looking forward to give it a spin! |
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
This PR is to visualize all of the diffs between main and feat/azure-split. It will not be merged. I will be creating a series of PRs to perform the merge in logical chunks suitable for review.
Actual PR series: