Skip to content
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

make cca creation not async #1085

Merged
merged 2 commits into from
Mar 24, 2021
Merged

make cca creation not async #1085

merged 2 commits into from
Mar 24, 2021

Conversation

jennyf19
Copy link
Collaborator

@DavidParks8 thanks for the suggestion of making cca not async. i think this was done back it was a sample and needed for something else...as things moved to MSAL, we were able to remove that functionality out of id web. @jmprieur

@jennyf19 jennyf19 requested a review from jmprieur March 24, 2021 16:44
@@ -421,6 +417,8 @@ public async Task ReplyForbiddenWithWwwAuthenticateHeaderAsync(IEnumerable<strin
httpResponse.StatusCode = (int)HttpStatusCode.Forbidden;

headers[HeaderNames.WWWAuthenticate] = new StringValues($"{Constants.Bearer} {parameterString}");

await Task.CompletedTask.ConfigureAwait(false); // we don't want to take a breaking change right now. will be for 2.0
Copy link
Contributor

@DavidParks8 DavidParks8 Mar 24, 2021

Choose a reason for hiding this comment

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

This is one of those cases where the task should be returned instead of awaited. #Resolved

Copy link
Collaborator Author

@jennyf19 jennyf19 Mar 24, 2021

Choose a reason for hiding this comment

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

yes...i talked w/ @jmprieur about this one and we are going to wait until we move to 2.* and take a breaking change. so this will come soon. #Resolved

@@ -17,8 +17,7 @@ public abstract class MsalAbstractTokenCacheProvider : IMsalTokenCacheProvider
/// Initializes the token cache serialization.
/// </summary>
/// <param name="tokenCache">Token cache to serialize/deserialize.</param>
/// <returns>A <see cref="Task"/> that represents a completed initialization operation.</returns>
public Task InitializeAsync(ITokenCache tokenCache)
public void Initialize(ITokenCache tokenCache)
Copy link
Contributor

@DavidParks8 DavidParks8 Mar 24, 2021

Choose a reason for hiding this comment

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

This is technically a breaking change. Is there anything special you need to do to rev the library version? #Resolved

Copy link
Collaborator Author

@jennyf19 jennyf19 Mar 24, 2021

Choose a reason for hiding this comment

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

we are going w/henrik's suggestion and doing obsolete attributes so we don't have to take a major version right now. #Resolved

@henrik-me
Copy link
Contributor

henrik-me commented Mar 24, 2021

We could start by deprecating and pointing to the updated functions? #Resolved

@henrik-me
Copy link
Contributor

henrik-me commented Mar 24, 2021

    private IConfidentialClientApplication? _application;

imo, ahould use the lazy pattern, or the pattern David suggested in his PR. #Resolved


Refers to: src/Microsoft.Identity.Web/TokenAcquisition.cs:36 in 1281902. [](commit_id = 1281902, deletion_comment = False)

@jennyf19
Copy link
Collaborator Author

yes...good idea, i've fixed that.


In reply to: 806008756 [](ancestors = 806008756)

@jennyf19
Copy link
Collaborator Author

    private IConfidentialClientApplication? _application;

I'm just fixing this cca async issue, which will enable david to remove some code, i'm not going to work on something from that PR specifically.


In reply to: 806012208 [](ancestors = 806012208)


Refers to: src/Microsoft.Identity.Web/TokenAcquisition.cs:36 in 1281902. [](commit_id = 1281902, deletion_comment = False)

@sonarcloud
Copy link

sonarcloud bot commented Mar 24, 2021

Copy link
Collaborator

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @jennyf19
we'll implement default methods on the interface before releasing (to avoid a breaking change as we've added methods on the interface)

@jennyf19
Copy link
Collaborator Author

@jmprieur I'll make a 2.* project board and start a running list of the breaking changes so we don't forget

@jennyf19 jennyf19 merged commit 5dd0981 into master Mar 24, 2021
@jennyf19 jennyf19 deleted the jennyf/async branch March 24, 2021 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants