Skip to content

APIScan | Fix Builder Mistake #3357

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

APIScan | Fix Builder Mistake #3357

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...soft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,7 +556,7 @@ private IPublicClientApplication CreateClientAppInstance(PublicClientAppKey publ
#if NETFRAMEWORK
if (_iWin32WindowFunc is not null)
{
builder.WithParentActivityOrWindow(_iWin32WindowFunc);
builder = builder.WithParentActivityOrWindow(_iWin32WindowFunc);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this actually necessary? Chaining typically returns the same object the method was called on, not a new object. Pretty sure WithParentActivityOrWindow() is setting internal state on builder and then returning builder. I'd be very surprised if it was returning a different instance of a Builder.

The API docs don't say either way, but the source code returns this:

https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/428838e8ac9588e34c115e30e410e66855700b52/src/client/Microsoft.Identity.Client/AppConfig/PublicClientApplicationBuilder.cs#L287

I think your original change was fine (i.e. it worked) and maybe that's why there were no test failures. This might be slightly more robust, but I doubt MSAL will change its chaining behaviour, and break a bunch of calling code.

Copy link
Member

Choose a reason for hiding this comment

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

Because config is local to builder creation, I think it is necessary to store the updated config in the builder instance.

You can validate the builder assignment need by updating another config that overrides something else that can be validated easily in a standalone console app, where you can directly work with MSAL APIs for testing the theory.

}
#endif

Expand Down
Loading