Skip to content

Commit c34def7

Browse files
siddhijainCopilot
siddhijain
and
Copilot
committed
Refactor flight to filter-then-clone and address review feedback
Replace the uncloned-references approach with filter-then-clone: - getAccounts()/getCredentials() always return defensive copies - getAccountsFilteredBy()/getCredentialsFilteredBy() with flight on: filter on uncloned in-memory refs first, then clone only matches - Extract cloneItems() generic helper to DRY the clone loops Telemetry and documentation: - Record is_filter_then_clone_enabled span attribute in both getAccounts() and getCredentials() for diagnostic consistency - Update AttributeName Javadoc to reference FilteredBy methods - Update flight Javadoc to accurately describe behavior Tests: - Add tests for getAccountsFilteredBy/getCredentialsFilteredBy with flight enabled: verifies filtering works and returns cloned objects - Update getAccounts/getCredentials flight-enabled tests to verify cloned objects (not shared references) - Fix test flakiness: call getAccounts()/getCredentials() before resetCallCounts() to ensure initial load completes deterministically Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent ef15fbf commit c34def7

4 files changed

Lines changed: 241 additions & 39 deletions

File tree

‎common/src/test/java/com/microsoft/identity/common/SharedPreferencesAccountCredentialCacheWithMemoryCacheTest.java‎

Lines changed: 102 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -70,9 +70,7 @@
7070
import static org.junit.Assert.assertNotNull;
7171
import static org.junit.Assert.assertNotSame;
7272
import static org.junit.Assert.assertNull;
73-
import static org.junit.Assert.assertSame;
7473
import static org.junit.Assert.assertTrue;
75-
import static org.junit.Assert.fail;
7674
import static org.mockito.Mockito.when;
7775

7876
@RunWith(RobolectricTestRunner.class)
@@ -2573,7 +2571,7 @@ public void getAccounts_flightDisabled_returnsMutableListOfClones() {
25732571
}
25742572

25752573
@Test
2576-
public void getAccounts_flightEnabled_returnsUnmodifiableListWithSharedReferences() {
2574+
public void getAccounts_flightEnabled_stillReturnsMutableListOfClones() {
25772575
enableFilterThenCloneFlight();
25782576
try {
25792577
final AccountRecord account = buildDefaultAccountRecord();
@@ -2584,16 +2582,17 @@ public void getAccounts_flightEnabled_returnsUnmodifiableListWithSharedReference
25842582
final List<AccountRecord> accounts2 = mSharedPreferencesAccountCredentialCache.getAccounts();
25852583
assertEquals(1, accounts2.size());
25862584

2587-
// When flight is enabled, objects are shared references (not cloned)
2588-
assertSame(accounts1.get(0), accounts2.get(0));
2585+
// Even with flight enabled, getAccounts() still returns clones
2586+
assertNotSame(accounts1.get(0), accounts2.get(0));
2587+
assertEquals(accounts1.get(0), accounts2.get(0));
25892588

2590-
// The returned list should be unmodifiable
2591-
try {
2592-
accounts1.add(new AccountRecord());
2593-
fail("Expected UnsupportedOperationException from unmodifiable list");
2594-
} catch (final UnsupportedOperationException e) {
2595-
// expected
2596-
}
2589+
// Mutating a returned object should not affect subsequent retrievals
2590+
accounts1.get(0).setLocalAccountId("mutated");
2591+
final List<AccountRecord> accounts3 = mSharedPreferencesAccountCredentialCache.getAccounts();
2592+
assertNotEquals("mutated", accounts3.get(0).getLocalAccountId());
2593+
2594+
// The returned list should be mutable (no exception thrown)
2595+
accounts1.add(new AccountRecord());
25972596
} finally {
25982597
resetFlight();
25992598
}
@@ -2626,7 +2625,7 @@ public void getCredentials_flightDisabled_returnsMutableListOfClones() {
26262625
}
26272626

26282627
@Test
2629-
public void getCredentials_flightEnabled_returnsUnmodifiableListWithSharedReferences() {
2628+
public void getCredentials_flightEnabled_stillReturnsMutableListOfClones() {
26302629
enableFilterThenCloneFlight();
26312630
try {
26322631
final RefreshTokenRecord rt = buildDefaultRefreshToken();
@@ -2637,16 +2636,91 @@ public void getCredentials_flightEnabled_returnsUnmodifiableListWithSharedRefere
26372636
final List<Credential> creds2 = mSharedPreferencesAccountCredentialCache.getCredentials();
26382637
assertEquals(1, creds2.size());
26392638

2640-
// When flight is enabled, objects are shared references (not cloned)
2641-
assertSame(creds1.get(0), creds2.get(0));
2639+
// Even with flight enabled, getCredentials() still returns clones
2640+
assertNotSame(creds1.get(0), creds2.get(0));
2641+
assertEquals(creds1.get(0), creds2.get(0));
2642+
2643+
// Mutating a returned object should not affect subsequent retrievals
2644+
creds1.get(0).setCachedAt("mutated");
2645+
final List<Credential> creds3 = mSharedPreferencesAccountCredentialCache.getCredentials();
2646+
assertNotEquals("mutated", creds3.get(0).getCachedAt());
2647+
2648+
// The returned list should be mutable (no exception thrown)
2649+
creds1.add(new RefreshTokenRecord());
2650+
} finally {
2651+
resetFlight();
2652+
}
2653+
}
2654+
2655+
// =====================================================================
2656+
// Tests verifying filter-then-clone optimization in FilteredBy methods
2657+
// =====================================================================
2658+
2659+
@Test
2660+
public void getAccountsFilteredBy_flightEnabled_returnsClonedMatches() {
2661+
enableFilterThenCloneFlight();
2662+
try {
2663+
final AccountRecord account = buildDefaultAccountRecord();
2664+
mSharedPreferencesAccountCredentialCache.saveAccount(account);
2665+
2666+
// Also save a non-matching account
2667+
final AccountRecord otherAccount = new AccountRecord();
2668+
otherAccount.setHomeAccountId("other-home-id");
2669+
otherAccount.setEnvironment("other.environment.com");
2670+
otherAccount.setRealm("other-realm");
2671+
otherAccount.setLocalAccountId("other-local-id");
2672+
otherAccount.setUsername("other-user");
2673+
otherAccount.setAuthorityType(AUTHORITY_TYPE);
2674+
mSharedPreferencesAccountCredentialCache.saveAccount(otherAccount);
2675+
2676+
// Filter by the default account's realm — should return only one match
2677+
final List<AccountRecord> filtered = mSharedPreferencesAccountCredentialCache
2678+
.getAccountsFilteredBy(HOME_ACCOUNT_ID, ENVIRONMENT, REALM);
2679+
assertEquals(1, filtered.size());
2680+
assertEquals(HOME_ACCOUNT_ID, filtered.get(0).getHomeAccountId());
2681+
2682+
// Returned objects should be clones — mutating should not affect cache
2683+
filtered.get(0).setLocalAccountId("mutated");
2684+
final List<AccountRecord> filtered2 = mSharedPreferencesAccountCredentialCache
2685+
.getAccountsFilteredBy(HOME_ACCOUNT_ID, ENVIRONMENT, REALM);
2686+
assertNotEquals("mutated", filtered2.get(0).getLocalAccountId());
2687+
} finally {
2688+
resetFlight();
2689+
}
2690+
}
2691+
2692+
@Test
2693+
public void getCredentialsFilteredBy_flightEnabled_returnsClonedMatches() {
2694+
enableFilterThenCloneFlight();
2695+
try {
2696+
final RefreshTokenRecord rt = buildDefaultRefreshToken();
2697+
mSharedPreferencesAccountCredentialCache.saveCredential(rt);
26422698

2643-
// The returned list should be unmodifiable
2644-
try {
2645-
creds1.add(new RefreshTokenRecord());
2646-
fail("Expected UnsupportedOperationException from unmodifiable list");
2647-
} catch (final UnsupportedOperationException e) {
2648-
// expected
2649-
}
2699+
// Also save a non-matching credential
2700+
final RefreshTokenRecord otherRt = new RefreshTokenRecord();
2701+
otherRt.setCredentialType(CredentialType.RefreshToken.name());
2702+
otherRt.setHomeAccountId("other-home-id");
2703+
otherRt.setEnvironment("other.environment.com");
2704+
otherRt.setClientId("other-client-id");
2705+
otherRt.setCachedAt(CACHED_AT);
2706+
otherRt.setSecret(SECRET);
2707+
mSharedPreferencesAccountCredentialCache.saveCredential(otherRt);
2708+
2709+
// Filter by the default credential's attributes — should return only one match
2710+
final List<Credential> filtered = mSharedPreferencesAccountCredentialCache
2711+
.getCredentialsFilteredBy(
2712+
HOME_ACCOUNT_ID, ENVIRONMENT, CredentialType.RefreshToken,
2713+
CLIENT_ID, null, null, null, null, null);
2714+
assertEquals(1, filtered.size());
2715+
assertEquals(CLIENT_ID, filtered.get(0).getClientId());
2716+
2717+
// Returned objects should be clones — mutating should not affect cache
2718+
filtered.get(0).setCachedAt("mutated");
2719+
final List<Credential> filtered2 = mSharedPreferencesAccountCredentialCache
2720+
.getCredentialsFilteredBy(
2721+
HOME_ACCOUNT_ID, ENVIRONMENT, CredentialType.RefreshToken,
2722+
CLIENT_ID, null, null, null, null, null);
2723+
assertNotEquals("mutated", filtered2.get(0).getCachedAt());
26502724
} finally {
26512725
resetFlight();
26522726
}
@@ -2735,7 +2809,9 @@ public void removeAccount_doesNotCallKeySetOrGetAll() throws Exception {
27352809
final AccountRecord account = buildDefaultAccountRecord();
27362810
cache.saveAccount(account);
27372811

2738-
// Reset counters after initial load (which may call getAll/keySet for loading)
2812+
// Force initial load to complete (getAccounts blocks on waitForInitialLoad),
2813+
// then reset counters so only removeAccount() calls are measured.
2814+
cache.getAccounts();
27392815
trackingStorage.resetCallCounts();
27402816

27412817
cache.removeAccount(account);
@@ -2757,7 +2833,9 @@ public void removeCredential_doesNotCallKeySetOrGetAll() throws Exception {
27572833
final RefreshTokenRecord rt = buildDefaultRefreshToken();
27582834
cache.saveCredential(rt);
27592835

2760-
// Reset counters after initial load (which may call getAll/keySet for loading)
2836+
// Force initial load to complete (getCredentials blocks on waitForInitialLoad),
2837+
// then reset counters so only removeCredential() calls are measured.
2838+
cache.getCredentials();
27612839
trackingStorage.resetCallCounts();
27622840

27632841
cache.removeCredential(rt);

‎common4j/src/main/com/microsoft/identity/common/java/cache/SharedPreferencesAccountCredentialCacheWithMemoryCache.java‎

Lines changed: 135 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@
4141
import com.microsoft.identity.common.java.util.ported.Predicate;
4242

4343
import java.util.ArrayList;
44-
import java.util.Collections;
4544
import java.util.HashMap;
4645
import java.util.HashSet;
4746
import java.util.Iterator;
@@ -117,6 +116,26 @@ private void waitForInitialLoad() {
117116
}
118117
}
119118

119+
/**
120+
* Clones each element of {@code items} and returns the cloned list.
121+
* Used by filter-then-clone paths to defensively copy only matching items.
122+
*/
123+
@SuppressWarnings("unchecked")
124+
@NonNull
125+
private <T extends AccountCredentialBase> List<T> cloneItems(
126+
@NonNull final List<T> items,
127+
@NonNull final String methodTag) {
128+
final List<T> cloned = new ArrayList<>(items.size());
129+
for (final T item : items) {
130+
try {
131+
cloned.add((T) item.clone());
132+
} catch (final CloneNotSupportedException e) {
133+
Logger.error(methodTag, "Failed to clone " + item.getClass().getSimpleName(), e);
134+
}
135+
}
136+
return cloned;
137+
}
138+
120139
@Override
121140
public void saveAccount(@NonNull final AccountRecord accountInput) {
122141
final String methodTag = TAG + ":saveAccount";
@@ -272,14 +291,11 @@ public List<AccountRecord> getAccounts() {
272291
final boolean useFilterThenClone = CommonFlightsManager.INSTANCE
273292
.getFlightsProvider()
274293
.isFlightEnabled(CommonFlight.ENABLE_FILTER_THEN_CLONE_IN_MEMORY_CACHE);
294+
SpanExtension.current().setAttribute(
295+
AttributeName.is_filter_then_clone_enabled.name(), useFilterThenClone);
275296

276297
synchronized (mCacheLock) {
277298
waitForInitialLoad();
278-
if (useFilterThenClone) {
279-
Logger.info(methodTag, "Found [" + mCachedAccountRecordsWithKeys.size() + "] Accounts...");
280-
return Collections.unmodifiableList(
281-
new ArrayList<>(mCachedAccountRecordsWithKeys.values()));
282-
}
283299
final List<AccountRecord> accounts = new ArrayList<>();
284300
for (AccountRecord record : mCachedAccountRecordsWithKeys.values()) {
285301
try {
@@ -302,6 +318,23 @@ public List<AccountRecord> getAccountsFilteredBy(
302318
final String methodTag = TAG + ":getAccountsFilteredBy";
303319
Logger.verbose(methodTag, "Loading Accounts...");
304320

321+
final boolean useFilterThenClone = CommonFlightsManager.INSTANCE
322+
.getFlightsProvider()
323+
.isFlightEnabled(CommonFlight.ENABLE_FILTER_THEN_CLONE_IN_MEMORY_CACHE);
324+
325+
if (useFilterThenClone) {
326+
synchronized (mCacheLock) {
327+
waitForInitialLoad();
328+
final List<AccountRecord> unclonedAccounts =
329+
new ArrayList<>(mCachedAccountRecordsWithKeys.values());
330+
final List<AccountRecord> matchingUncloned = getAccountsFilteredByInternal(
331+
homeAccountId, environment, realm, unclonedAccounts);
332+
final List<AccountRecord> clonedMatches = cloneItems(matchingUncloned, methodTag);
333+
Logger.verbose(methodTag, "Found [" + clonedMatches.size() + "] matching Accounts...");
334+
return clonedMatches;
335+
}
336+
}
337+
305338
final List<AccountRecord> allAccounts = getAccounts();
306339

307340
final List<AccountRecord> matchingAccounts = getAccountsFilteredByInternal(
@@ -372,12 +405,6 @@ public List<Credential> getCredentials() {
372405

373406
synchronized (mCacheLock) {
374407
waitForInitialLoad();
375-
if (useFilterThenClone) {
376-
// Return uncloned references in an unmodifiable list.
377-
// All callers have been verified as read-only on the returned items.
378-
return Collections.unmodifiableList(
379-
new ArrayList<>(mCachedCredentialsWithKeys.values()));
380-
}
381408
ArrayList<Credential> credentials = new ArrayList<>();
382409
for (Credential credential : mCachedCredentialsWithKeys.values()) {
383410
try {
@@ -405,6 +432,36 @@ public List<Credential> getCredentialsFilteredBy(
405432
final String methodTag = TAG + ":getCredentialsFilteredBy";
406433
Logger.verbose(methodTag, "getCredentialsFilteredBy()");
407434

435+
final boolean useFilterThenClone = CommonFlightsManager.INSTANCE
436+
.getFlightsProvider()
437+
.isFlightEnabled(CommonFlight.ENABLE_FILTER_THEN_CLONE_IN_MEMORY_CACHE);
438+
439+
if (useFilterThenClone) {
440+
synchronized (mCacheLock) {
441+
waitForInitialLoad();
442+
final List<Credential> unclonedCredentials =
443+
new ArrayList<>(mCachedCredentialsWithKeys.values());
444+
final List<Credential> matchingUncloned = getCredentialsFilteredByInternal(
445+
unclonedCredentials,
446+
homeAccountId,
447+
environment,
448+
credentialType,
449+
clientId,
450+
applicationIdentifier,
451+
mamEnrollmentIdentifier,
452+
realm,
453+
target,
454+
authScheme,
455+
null,
456+
null,
457+
false
458+
);
459+
final List<Credential> clonedMatches = cloneItems(matchingUncloned, methodTag);
460+
Logger.verbose(methodTag, "Found [" + clonedMatches.size() + "] matching Credentials...");
461+
return clonedMatches;
462+
}
463+
}
464+
408465
final List<Credential> allCredentials = getCredentials();
409466

410467
final List<Credential> matchingCredentials = getCredentialsFilteredByInternal(
@@ -480,6 +537,36 @@ public List<Credential> getCredentialsFilteredBy(
480537
final String methodTag = TAG + ":getCredentialsFilteredBy";
481538
Logger.verbose(methodTag, "getCredentialsFilteredBy()");
482539

540+
final boolean useFilterThenClone = CommonFlightsManager.INSTANCE
541+
.getFlightsProvider()
542+
.isFlightEnabled(CommonFlight.ENABLE_FILTER_THEN_CLONE_IN_MEMORY_CACHE);
543+
544+
if (useFilterThenClone) {
545+
synchronized (mCacheLock) {
546+
waitForInitialLoad();
547+
final List<Credential> unclonedCredentials =
548+
new ArrayList<>(mCachedCredentialsWithKeys.values());
549+
final List<Credential> matchingUncloned = getCredentialsFilteredByInternal(
550+
unclonedCredentials,
551+
homeAccountId,
552+
environment,
553+
credentialType,
554+
clientId,
555+
applicationIdentifier,
556+
mamEnrollmentIdentifier,
557+
realm,
558+
target,
559+
authScheme,
560+
requestedClaims,
561+
null,
562+
false
563+
);
564+
final List<Credential> clonedMatches = cloneItems(matchingUncloned, methodTag);
565+
Logger.verbose(methodTag, "Found [" + clonedMatches.size() + "] matching Credentials...");
566+
return clonedMatches;
567+
}
568+
}
569+
483570
final List<Credential> allCredentials = getCredentials();
484571

485572
final List<Credential> matchingCredentials = getCredentialsFilteredByInternal(
@@ -591,6 +678,42 @@ public List<Credential> getCredentialsFilteredBy(@Nullable final String homeAcco
591678
@Nullable final String target,
592679
@Nullable final String authScheme,
593680
@Nullable final String requestedClaims) {
681+
final String methodTag = TAG + ":getCredentialsFilteredBy";
682+
683+
final boolean useFilterThenClone = CommonFlightsManager.INSTANCE
684+
.getFlightsProvider()
685+
.isFlightEnabled(CommonFlight.ENABLE_FILTER_THEN_CLONE_IN_MEMORY_CACHE);
686+
687+
if (useFilterThenClone) {
688+
synchronized (mCacheLock) {
689+
waitForInitialLoad();
690+
final List<Credential> unclonedCredentials =
691+
new ArrayList<>(mCachedCredentialsWithKeys.values());
692+
final List<Credential> result = new ArrayList<>();
693+
for (final CredentialType type : credentialTypes) {
694+
result.addAll(cloneItems(
695+
getCredentialsFilteredByInternal(
696+
unclonedCredentials,
697+
homeAccountId,
698+
environment,
699+
type,
700+
clientId,
701+
applicationIdentifier,
702+
mamEnrollmentIdentifier,
703+
realm,
704+
target,
705+
authScheme,
706+
requestedClaims,
707+
null,
708+
false
709+
),
710+
methodTag
711+
));
712+
}
713+
return result;
714+
}
715+
}
716+
594717
final List<Credential> allCredentials = getCredentials();
595718

596719
final List<Credential> result = new ArrayList<>();

‎common4j/src/main/com/microsoft/identity/common/java/flighting/CommonFlight.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -255,8 +255,9 @@ public enum CommonFlight implements IFlightConfig {
255255

256256
/**
257257
* Flight to enable filter-then-clone optimization in SharedPreferencesAccountCredentialCacheWithMemoryCache.
258-
* When enabled, getCredentials()/getAccounts() returns uncloned references and
259-
* getCredentialsFilteredBy()/getAccountsFilteredBy() filters first, then clones only matching items.
258+
* When enabled, getCredentialsFilteredBy()/getAccountsFilteredBy() filters on in-memory
259+
* references first, then clones only the matching items — avoiding the cost of
260+
* cloning the entire cache when only a subset is needed.
260261
*/
261262
ENABLE_FILTER_THEN_CLONE_IN_MEMORY_CACHE("EnableFilterThenCloneInMemoryCache", false);
262263

0 commit comments

Comments
 (0)