Fixes AB#3588022 Optimize load() and getIdTokensForAccountRecord() with filter-then-clone#3100
Open
siddhijain wants to merge 5 commits intodevfrom
Open
Fixes AB#3588022 Optimize load() and getIdTokensForAccountRecord() with filter-then-clone#3100siddhijain wants to merge 5 commits intodevfrom
siddhijain wants to merge 5 commits intodevfrom
Conversation
Apply filter-then-clone optimization to MsalOAuth2TokenCache.load() and getIdTokensForAccountRecord() methods behind the existing ENABLE_FILTER_THEN_CLONE_IN_MEMORY_CACHE flight. When the flight is enabled, these methods now call the direct (non-input-list) getCredentialsFilteredBy overloads which filter on uncloned in-memory references first, then clone only the matching credentials. This avoids cloning all cached credentials on every AcquireTokenSilent cache-hit path. Changes: - Add new getCredentialsFilteredBy overload with kid parameter to IAccountCredentialCache, SharedPreferencesAccountCredentialCache, and SharedPreferencesAccountCredentialCacheWithMemoryCache - Gate load() to skip preload pattern and use direct filtered lookups when flight is enabled - Gate getIdTokensForAccountRecord() with same optimization pattern - Add tests for new kid overload (flight enabled/disabled) - Add MsalOAuth2TokenCache-level tests for flight-gated load() and getIdTokensForAccountRecord() paths Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
❌ Work item link check failed. Description does not contain AB#{ID}. Click here to Learn more. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the existing “filter-then-clone” optimization (behind ENABLE_FILTER_THEN_CLONE_IN_MEMORY_CACHE) to MsalOAuth2TokenCache.load() and getIdTokensForAccountRecord() by using new direct getCredentialsFilteredBy(...) overloads (including a kid parameter) so only matching in-memory credentials are cloned on cache-hit paths.
Changes:
- Added a new
IAccountCredentialCache.getCredentialsFilteredBy(..., requestedClaims, kid)overload and implemented it in bothSharedPreferencesAccountCredentialCacheandSharedPreferencesAccountCredentialCacheWithMemoryCache. - Updated
MsalOAuth2TokenCache.load()andgetIdTokensForAccountRecord()to use direct filtered lookups when the flight is enabled, retaining the legacy “preload all then filter” path when disabled. - Added/extended tests for flight-gated behavior and the new overload; updated
changelog.txt.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| common4j/src/main/com/microsoft/identity/common/java/cache/SharedPreferencesAccountCredentialCacheWithMemoryCache.java | Adds direct kid-aware filtering with filter-then-clone behavior when flight is enabled. |
| common4j/src/main/com/microsoft/identity/common/java/cache/SharedPreferencesAccountCredentialCache.java | Adds direct kid-aware filtering overload (non-memory implementation). |
| common4j/src/main/com/microsoft/identity/common/java/cache/MsalOAuth2TokenCache.java | Adds flight-gated use of direct filtered lookups in load() and getIdTokensForAccountRecord(). |
| common4j/src/main/com/microsoft/identity/common/java/cache/IAccountCredentialCache.java | Extends cache interface with a new direct kid overload. |
| common/src/test/java/com/microsoft/identity/common/SharedPreferencesAccountCredentialCacheWithMemoryCacheTest.java | Adds tests for the new overload (flight enabled/disabled). |
| common/src/test/java/com/microsoft/identity/common/MsalOAuth2TokenCacheTest.java | Adds flight-gated tests for load() and getIdTokensForAccountRecord() using memory cache implementation. |
| changelog.txt | Adds a vNext PATCH entry describing the optimization. |
| @@ -1,5 +1,6 @@ | |||
| vNext | |||
| ---------- | |||
| - [PATCH] Extend filter-then-clone optimization to load() and getIdTokensForAccountRecord() in MsalOAuth2TokenCache: when ENABLE_FILTER_THEN_CLONE_IN_MEMORY_CACHE flight is enabled, skip clone-all preload and call direct flight-gated overloads that clone only matching credentials; add new getCredentialsFilteredBy overload with kid support (#TBD) | |||
There was a problem hiding this comment.
Changelog entry still contains the placeholder (#TBD). Impact: release notes become ambiguous and the repo’s changelog convention (linking to an issue/PR number) isn’t met. Recommendation: replace #TBD with the actual PR/issue reference before merging.
Suggested change
| - [PATCH] Extend filter-then-clone optimization to load() and getIdTokensForAccountRecord() in MsalOAuth2TokenCache: when ENABLE_FILTER_THEN_CLONE_IN_MEMORY_CACHE flight is enabled, skip clone-all preload and call direct flight-gated overloads that clone only matching credentials; add new getCredentialsFilteredBy overload with kid support (#TBD) | |
| - [PATCH] Extend filter-then-clone optimization to load() and getIdTokensForAccountRecord() in MsalOAuth2TokenCache: when ENABLE_FILTER_THEN_CLONE_IN_MEMORY_CACHE flight is enabled, skip clone-all preload and call direct flight-gated overloads that clone only matching credentials; add new getCredentialsFilteredBy overload with kid support (#<ACTUAL_PR_OR_ISSUE_NUMBER>) |
Contributor
Author
There was a problem hiding this comment.
@copilot help me take care of this comment by updating the change log with the right PR number 3100.
Contributor
There was a problem hiding this comment.
Done in cab62bc. Replaced #TBD with #3100 in the changelog entry.
siddhijain
changed the title
|
✅ Work item link check complete. Description contains link AB#3588022 to an Azure Boards work item. |
…WithKid tests
- Set kid="kid1" on AccessTokenRecord in both test methods
- Add assertions for matching kid ("kid1" → returns 1 result)
- Add assertions for non-matching kid ("kid2" → returns 0 results)
- Keep null kid assertions (null → returns all, no kid filter applied)
- Cover both flight-disabled and flight-enabled variants
Agent-Logs-Url: https://github.com/AzureAD/microsoft-authentication-library-common-for-android/sessions/54f98b54-83f3-4bca-92d7-e1544068086f
Co-authored-by: siddhijain <30181294+siddhijain@users.noreply.github.com>
…kid) overload The caller (MsalOAuth2TokenCache.load()) already gates entry to this overload via the ENABLE_FILTER_THEN_CLONE_IN_MEMORY_CACHE flight. The inner check was redundant. Now always uses the optimized filter-then-clone path. Note: The 9-param overload retains its flight check since it has callers (BrokerOAuth2TokenCache, MsalCppOAuth2TokenCache) that don't check the flight at their level. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Agent-Logs-Url: https://github.com/AzureAD/microsoft-authentication-library-common-for-android/sessions/3337a801-8112-4523-b1aa-73acde270714 Co-authored-by: siddhijain <30181294+siddhijain@users.noreply.github.com>
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
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.
Fixes AB#3588022 Apply filter-then-clone optimization to MsalOAuth2TokenCache.load() and getIdTokensForAccountRecord() methods behind the existing ENABLE_FILTER_THEN_CLONE_IN_MEMORY_CACHE flight.
When the flight is enabled, these methods now call the direct (non-input-list) getCredentialsFilteredBy overloads which filter on uncloned in-memory references first, then clone only the matching credentials. This avoids cloning all cached credentials on every AcquireTokenSilent cache-hit path.
Changes: