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

Add oAuthEndpoint to _oAuthClients cache key #6298

Merged
merged 2 commits into from
May 2, 2022
Merged

Conversation

craigjensen
Copy link
Contributor

@craigjensen craigjensen commented Apr 22, 2022

Fixes #6308

Description

We cache OAuthClients by appId so changing the endpoint only affects newly created clients that are not already cached. This PR adds the endpoint to the cache key so we can support changing the endpoint at runtime.

Specific Changes

  • Added OAuthEndpoint to the cache key for the _oAuthClients cache

Testing

Added tests for

@craigjensen craigjensen self-assigned this Apr 22, 2022
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 301650

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 17 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.08%) to 78.786%

Files with Coverage Reduction New Missed Lines %
/libraries/Microsoft.Bot.Builder/BotFrameworkAdapter.cs 17 47.91%
Totals Coverage Status
Change from base Build 301524: 0.08%
Covered Lines: 25318
Relevant Lines: 32135

💛 - Coveralls

@@ -1408,6 +1407,8 @@ protected virtual async Task<OAuthClient> CreateOAuthApiClientAsync(ITurnContext
OAuthClientConfig.EmulateOAuthCards = true;
}

var oAuthEndpoint = OAuthClientConfig.OAuthEndpoint;

Choose a reason for hiding this comment

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

The assumption is that we should not be calling these from different threads as we are still caching a static variable. A context switch before this line could result in incorrect endpoint being used if two threads set it to different values. I wonder if the approach is to do better documentation or improve synchronization here? Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there will always be a race when using a static like this. As long as we're setting it globally via an FCB I don't think it will matter. For this PR, I wanted to minimize changes and get something that will allow us to change it globally. If we end up needing to set the value on a per-call basis then we'll need another PR to redesign the API and remove the static.

@craigjensen craigjensen marked this pull request as ready for review May 2, 2022 22:07
@craigjensen craigjensen requested a review from a team as a code owner May 2, 2022 22:07
@craigjensen craigjensen added the Automation: No parity PR does not need to be applied to other languages. label May 2, 2022
@tracyboehrer tracyboehrer merged commit 60c70ca into main May 2, 2022
@tracyboehrer tracyboehrer deleted the cjensen/oauth branch May 2, 2022 22:25
tracyboehrer pushed a commit that referenced this pull request May 2, 2022
* Add oAuthEndpoint to _oAuthClients cache key

* Add CreateOAuthClientWithDifferentEndpoints test

Co-authored-by: Craig Jensen <crjens@hotmail.com>
tracyboehrer added a commit that referenced this pull request May 3, 2022
* Add oAuthEndpoint to _oAuthClients cache key

* Add CreateOAuthClientWithDifferentEndpoints test

Co-authored-by: Craig Jensen <crjens@hotmail.com>

Co-authored-by: craigjensen <cjensen@microsoft.com>
Co-authored-by: Craig Jensen <crjens@hotmail.com>
tracyboehrer pushed a commit that referenced this pull request May 16, 2022
* Add oAuthEndpoint to _oAuthClients cache key

* Add CreateOAuthClientWithDifferentEndpoints test

Co-authored-by: Craig Jensen <crjens@hotmail.com>
tracyboehrer added a commit that referenced this pull request May 16, 2022
* Add oAuthEndpoint to _oAuthClients cache key

* Add CreateOAuthClientWithDifferentEndpoints test

Co-authored-by: Craig Jensen <crjens@hotmail.com>

Co-authored-by: craigjensen <cjensen@microsoft.com>
Co-authored-by: Craig Jensen <crjens@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Automation: No parity PR does not need to be applied to other languages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing OAuthClientConfig.OAuthEndpoint has no affect due to OAuthClient caching
6 participants