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

Cache level should be L1 in case of default in memory cache. #4423

Merged
merged 2 commits into from
Nov 17, 2023

Conversation

neha-bhargava
Copy link
Contributor

Fixes #4414

Changes proposed in this request
Fix the cache level, when in memory cache is used cache level will be L1Cache.

Testing
Updated tests to verify

Performance impact

Documentation

  • All relevant documentation is updated.

@gladjohn
Copy link
Contributor

looks good @neha-bhargava. In the RequestBase there is also a check to see if cache level is set if not we set to None. But, is no test coverage for that scenario I guess. Maybe you can file a work item to track that.

@neha-bhargava
Copy link
Contributor Author

neha-bhargava commented Nov 17, 2023

looks good @neha-bhargava. In the RequestBase there is also a check to see if cache level is set if not we set to None. But, is no test coverage for that scenario I guess. Maybe you can file a work item to track that.

We do have tests to cover all the cache levels here: https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/nebharg/CacheLevelBug/tests/Microsoft.Identity.Test.Unit/PublicApiTests/TelemetryClientTests.cs#L245. So it is all covered. The cache level is unknown when the cache level is not provided by the user implementing custom cache. But when the custom cache is not used the cache level should be L1Cache, I updated the test to make sure that is covered.

Copy link
Contributor

@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 @neha-bhargava

@neha-bhargava neha-bhargava merged commit 9792dc6 into main Nov 17, 2023
6 checks passed
@neha-bhargava neha-bhargava deleted the nebharg/CacheLevelBug branch November 17, 2023 19:30
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.

[Bug] Cache level should be L1 when MSAL's in memory cache is used.
3 participants