From 9a843fba26818b69077201de7571a8e1ffc2d5fe Mon Sep 17 00:00:00 2001 From: tomnatan Date: Fri, 24 Sep 2021 12:31:20 +0000 Subject: [PATCH] Automatically remove other owned overrides when package override flag changes At the moment we are using remove entries in the package override flags to indicate that an override is no longer needed, this changes removes that logic and instead adds support for automatically removing overrides whose change ID is in the owned_changed_ids flag but not in the package override flag each time that flag changes. Bug: 201058202 Test: atest FrameworksMockingServicesTests:AppCompatOverridesParserTest Test: atest FrameworksMockingServicesTests:AppCompatOverridesServiceTest Change-Id: I064e726e5291556ebdfddba6b2a8b1492068d74b --- .../overrides/AppCompatOverridesParser.java | 60 +------ .../overrides/AppCompatOverridesService.java | 80 ++++++---- .../AppCompatOverridesParserTest.java | 131 ++++++---------- .../AppCompatOverridesServiceTest.java | 146 +++++++++++------- 4 files changed, 199 insertions(+), 218 deletions(-) diff --git a/services/core/java/com/android/server/compat/overrides/AppCompatOverridesParser.java b/services/core/java/com/android/server/compat/overrides/AppCompatOverridesParser.java index a81213df6fe30..11dc1dbd25ccf 100644 --- a/services/core/java/com/android/server/compat/overrides/AppCompatOverridesParser.java +++ b/services/core/java/com/android/server/compat/overrides/AppCompatOverridesParser.java @@ -175,29 +175,25 @@ final class AppCompatOverridesParser { /** * Parses the given {@code configStr}, that is expected to be a comma separated list of changes - * overrides, and returns a {@link PackageOverrides}. + * overrides, and returns a map from change ID to {@link PackageOverride} instances to add. * *

Each change override is in the following format: - * ':::'. If is empty, - * this indicates that any override for the specified change ID should be removed. + * ':::'. * *

If there are multiple overrides that should be added with the same change ID, the one * that best fits the given {@code versionCode} is added. * *

Any overrides whose change ID is in {@code changeIdsToSkip} are ignored. * - *

If a change override entry in {@code configStr} is invalid, it will be ignored. If the - * same change ID is both added and removed, i.e., has a change override entry with an empty - * enabled and another with a non-empty enabled, the change ID will only be removed. + *

If a change override entry in {@code configStr} is invalid, it will be ignored. */ - static PackageOverrides parsePackageOverrides( - String configStr, long versionCode, Set changeIdsToSkip) { + static Map parsePackageOverrides(String configStr, long versionCode, + Set changeIdsToSkip) { if (configStr.isEmpty()) { - return new PackageOverrides(); + return emptyMap(); } PackageOverrideComparator comparator = new PackageOverrideComparator(versionCode); Map overridesToAdd = new ArrayMap<>(); - Set overridesToRemove = new ArraySet<>(); for (String overrideEntryString : configStr.split(",")) { List changeIdAndVersions = Arrays.asList(overrideEntryString.split(":", 4)); if (changeIdAndVersions.size() != 4) { @@ -220,16 +216,6 @@ final class AppCompatOverridesParser { String maxVersionCodeStr = changeIdAndVersions.get(2); String enabledStr = changeIdAndVersions.get(3); - if (enabledStr.isEmpty()) { - if (!minVersionCodeStr.isEmpty() || !maxVersionCodeStr.isEmpty()) { - Slog.w( - TAG, - "min/max version code should be empty if enabled is empty: " - + overrideEntryString); - } - overridesToRemove.add(changeId); - continue; - } if (!BOOLEAN_PATTERN.matcher(enabledStr).matches()) { Slog.w(TAG, "Invalid enabled string in override entry: " + overrideEntryString); continue; @@ -262,39 +248,7 @@ final class AppCompatOverridesParser { } } - for (Long changeId : overridesToRemove) { - if (overridesToAdd.containsKey(changeId)) { - Slog.w( - TAG, - "Change ID [" - + changeId - + "] is both added and removed in package override flag: " - + configStr); - overridesToAdd.remove(changeId); - } - } - - return new PackageOverrides(overridesToAdd, overridesToRemove); - } - - /** - * A container for a map from change ID to {@link PackageOverride} to add and a set of change - * IDs to remove overrides for. - * - *

The map of overrides to add and the set of overrides to remove are mutually exclusive. - */ - static final class PackageOverrides { - public final Map overridesToAdd; - public final Set overridesToRemove; - - PackageOverrides() { - this(emptyMap(), emptySet()); - } - - PackageOverrides(Map overridesToAdd, Set overridesToRemove) { - this.overridesToAdd = overridesToAdd; - this.overridesToRemove = overridesToRemove; - } + return overridesToAdd; } /** diff --git a/services/core/java/com/android/server/compat/overrides/AppCompatOverridesService.java b/services/core/java/com/android/server/compat/overrides/AppCompatOverridesService.java index 63ae1af42f086..6aed4b023297b 100644 --- a/services/core/java/com/android/server/compat/overrides/AppCompatOverridesService.java +++ b/services/core/java/com/android/server/compat/overrides/AppCompatOverridesService.java @@ -49,7 +49,6 @@ import com.android.internal.compat.CompatibilityOverrideConfig; import com.android.internal.compat.CompatibilityOverridesToRemoveConfig; import com.android.internal.compat.IPlatformCompat; import com.android.server.SystemService; -import com.android.server.compat.overrides.AppCompatOverridesParser.PackageOverrides; import java.util.ArrayList; import java.util.Arrays; @@ -128,20 +127,25 @@ public final class AppCompatOverridesService { } /** - * Same as {@link #applyOverrides(Properties, Map)} except all properties of the given {@code - * namespace} are fetched via {@link DeviceConfig#getProperties}. + * Same as {@link #applyOverrides(Properties, Set, Map)} except all properties of the given + * {@code namespace} are fetched via {@link DeviceConfig#getProperties}. */ - private void applyAllOverrides(String namespace, + private void applyAllOverrides(String namespace, Set ownedChangeIds, Map> packageToChangeIdsToSkip) { - applyOverrides(DeviceConfig.getProperties(namespace), packageToChangeIdsToSkip); + applyOverrides(DeviceConfig.getProperties(namespace), ownedChangeIds, + packageToChangeIdsToSkip); } /** * Iterates all package override flags in the given {@code properties}, and for each flag whose - * package is installed on the device, parses its value and applies the overrides in it with + * package is installed on the device, parses its value and adds the overrides in it with * respect to the package's current installed version. + * + *

In addition, for each package, removes any override that wasn't just added, whose change + * ID is in {@code ownedChangeIds} but not in the respective set in {@code + * packageToChangeIdsToSkip}. */ - private void applyOverrides(Properties properties, + private void applyOverrides(Properties properties, Set ownedChangeIds, Map> packageToChangeIdsToSkip) { Set packageNames = new ArraySet<>(properties.getKeyset()); packageNames.remove(FLAG_OWNED_CHANGE_IDS); @@ -154,15 +158,16 @@ public final class AppCompatOverridesService { } applyPackageOverrides(properties.getString(packageName, /* defaultValue= */ ""), - packageName, versionCode, - packageToChangeIdsToSkip.getOrDefault(packageName, emptySet())); + packageName, versionCode, ownedChangeIds, + packageToChangeIdsToSkip.getOrDefault(packageName, emptySet()), + /* removeOtherOwnedOverrides= */ true); } } /** - * Applies all overrides in all supported namespaces for the given {@code packageName}. + * Adds all overrides in all supported namespaces for the given {@code packageName}. */ - private void applyAllPackageOverrides(String packageName) { + private void addAllPackageOverrides(String packageName) { Long versionCode = getVersionCodeOrNull(packageName); if (versionCode == null) { return; @@ -171,26 +176,40 @@ public final class AppCompatOverridesService { for (String namespace : mSupportedNamespaces) { // We apply overrides for each namespace separately so that if there is a failure for // one namespace, the other namespaces won't be affected. + Set ownedChangeIds = getOwnedChangeIds(namespace); applyPackageOverrides( DeviceConfig.getString(namespace, packageName, /* defaultValue= */ ""), - packageName, versionCode, - getOverridesToRemove(namespace).getOrDefault(packageName, emptySet())); + packageName, versionCode, ownedChangeIds, + getOverridesToRemove(namespace, ownedChangeIds).getOrDefault(packageName, + emptySet()), /* removeOtherOwnedOverrides */ false); } } /** - * Calls {@link AppCompatOverridesParser#parsePackageOverrides} on the given arguments, adds the - * resulting {@link PackageOverrides#overridesToAdd} via {@link - * IPlatformCompat#putOverridesOnReleaseBuilds}, and removes the resulting {@link - * PackageOverrides#overridesToRemove} via {@link - * IPlatformCompat#removeOverridesOnReleaseBuilds}. + * Calls {@link AppCompatOverridesParser#parsePackageOverrides} on the given arguments and adds + * the resulting overrides via {@link IPlatformCompat#putOverridesOnReleaseBuilds}. + * + *

In addition, if {@code removeOtherOwnedOverrides} is true, removes any override that + * wasn't just added, whose change ID is in {@code ownedChangeIds} but not in {@code + * changeIdsToSkip}, via {@link IPlatformCompat#removeOverridesOnReleaseBuilds}. */ - private void applyPackageOverrides(String configStr, String packageName, - long versionCode, Set changeIdsToSkip) { - PackageOverrides packageOverrides = AppCompatOverridesParser.parsePackageOverrides( + private void applyPackageOverrides(String configStr, String packageName, long versionCode, + Set ownedChangeIds, Set changeIdsToSkip, + boolean removeOtherOwnedOverrides) { + Map overridesToAdd = AppCompatOverridesParser.parsePackageOverrides( configStr, versionCode, changeIdsToSkip); - putPackageOverrides(packageName, packageOverrides.overridesToAdd); - removePackageOverrides(packageName, packageOverrides.overridesToRemove); + putPackageOverrides(packageName, overridesToAdd); + + if (!removeOtherOwnedOverrides) { + return; + } + Set overridesToRemove = new ArraySet<>(); + for (Long changeId : ownedChangeIds) { + if (!overridesToAdd.containsKey(changeId) && !changeIdsToSkip.contains(changeId)) { + overridesToRemove.add(changeId); + } + } + removePackageOverrides(packageName, overridesToRemove); } /** @@ -227,10 +246,11 @@ public final class AppCompatOverridesService { * {@code namespace} and parses it into a map from package name to a set of change IDs to * remove for that package. */ - private Map> getOverridesToRemove(String namespace) { + private Map> getOverridesToRemove(String namespace, + Set ownedChangeIds) { return mOverridesParser.parseRemoveOverrides( DeviceConfig.getString(namespace, FLAG_REMOVE_OVERRIDES, /* defaultValue= */ ""), - getOwnedChangeIds(namespace)); + ownedChangeIds); } /** @@ -333,7 +353,9 @@ public final class AppCompatOverridesService { boolean ownedChangedIdsFlagChanged = properties.getKeyset().contains( FLAG_OWNED_CHANGE_IDS); - Map> overridesToRemove = getOverridesToRemove(mNamespace); + Set ownedChangeIds = getOwnedChangeIds(mNamespace); + Map> overridesToRemove = getOverridesToRemove(mNamespace, + ownedChangeIds); if (removeOverridesFlagChanged || ownedChangedIdsFlagChanged) { // In both cases it's possible that overrides that weren't removed before should // now be removed. @@ -343,9 +365,9 @@ public final class AppCompatOverridesService { if (removeOverridesFlagChanged) { // We need to re-apply all overrides in the namespace since the remove overrides // flag might have blocked some of them from being applied before. - applyAllOverrides(mNamespace, overridesToRemove); + applyAllOverrides(mNamespace, ownedChangeIds, overridesToRemove); } else { - applyOverrides(properties, overridesToRemove); + applyOverrides(properties, ownedChangeIds, overridesToRemove); } } } @@ -392,7 +414,7 @@ public final class AppCompatOverridesService { switch (action) { case ACTION_PACKAGE_ADDED: case ACTION_PACKAGE_CHANGED: - applyAllPackageOverrides(packageName); + addAllPackageOverrides(packageName); break; case ACTION_PACKAGE_REMOVED: if (!isInstalledForAnyUser(packageName)) { diff --git a/services/tests/mockingservicestests/src/com/android/server/compat/overrides/AppCompatOverridesParserTest.java b/services/tests/mockingservicestests/src/com/android/server/compat/overrides/AppCompatOverridesParserTest.java index bf97042d7b884..df19be4a9cfe6 100644 --- a/services/tests/mockingservicestests/src/com/android/server/compat/overrides/AppCompatOverridesParserTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/compat/overrides/AppCompatOverridesParserTest.java @@ -31,8 +31,6 @@ import android.util.ArraySet; import androidx.test.filters.SmallTest; -import com.android.server.compat.overrides.AppCompatOverridesParser.PackageOverrides; - import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -183,115 +181,84 @@ public class AppCompatOverridesParserTest { } @Test - public void parsePackageOverrides_emptyConfig_returnsEmpty() { - PackageOverrides result = AppCompatOverridesParser.parsePackageOverrides(/* configStr= */ - "", /* versionCode= */ 0, /* changeIdsToSkip= */ emptySet()); + public void parsePackageOverrides_emptyConfigNoOwnedChangeIds_returnsEmpty() { + Map result = AppCompatOverridesParser.parsePackageOverrides( + /* configStr= */ "", /* versionCode= */ 0, /* changeIdsToSkip= */ emptySet()); - assertThat(result.overridesToAdd).isEmpty(); - assertThat(result.overridesToRemove).isEmpty(); + assertThat(result).isEmpty(); } @Test public void parsePackageOverrides_configWithSingleOverride_returnsOverride() { - PackageOverrides result = AppCompatOverridesParser.parsePackageOverrides(/* configStr= */ - "123:::true", /* versionCode= */ 5, /* changeIdsToSkip= */ emptySet()); - - assertThat(result.overridesToAdd).hasSize(1); - assertThat(result.overridesToAdd.get(123L)).isEqualTo( - new PackageOverride.Builder().setEnabled(true).build()); - } - - @Test - public void parsePackageOverrides_configWithMultipleOverridesToAdd_returnsOverrides() { - PackageOverrides result = AppCompatOverridesParser.parsePackageOverrides(/* configStr= */ - "910:3:4:false,78:10::false,12:::false,34:1:2:true,34:10::true,56::2:true," - + "56:3:4:false,34:4:8:true,78:6:7:true,910:5::true,1112::5:true," - + "56:6::true,1112:6:7:false", /* versionCode= */ - 5, /* changeIdsToSkip= */ emptySet()); - - assertThat(result.overridesToAdd).hasSize(6); - assertThat(result.overridesToAdd.get(12L)).isEqualTo( - new PackageOverride.Builder().setEnabled(false).build()); - assertThat(result.overridesToAdd.get(34L)).isEqualTo( - new PackageOverride.Builder().setMinVersionCode(4).setMaxVersionCode(8).setEnabled( - true).build()); - assertThat(result.overridesToAdd.get(56L)).isEqualTo( - new PackageOverride.Builder().setMinVersionCode(3).setMaxVersionCode(4).setEnabled( - false).build()); - assertThat(result.overridesToAdd.get(78L)).isEqualTo( - new PackageOverride.Builder().setMinVersionCode(6).setMaxVersionCode(7).setEnabled( - true).build()); - assertThat(result.overridesToAdd.get(910L)).isEqualTo( - new PackageOverride.Builder().setMinVersionCode(5).setEnabled(true).build()); - assertThat(result.overridesToAdd.get(1112L)).isEqualTo( - new PackageOverride.Builder().setMaxVersionCode(5).setEnabled(true).build()); - assertThat(result.overridesToRemove).isEmpty(); - } - - @Test - public void parsePackageOverrides_configWithMultipleOverridesToRemove_returnsOverrides() { - PackageOverrides result = AppCompatOverridesParser.parsePackageOverrides(/* configStr= */ - "12:::,34:1:2:", /* versionCode= */ 5, /* changeIdsToSkip= */ emptySet()); - - assertThat(result.overridesToRemove).containsExactly(12L, 34L); - assertThat(result.overridesToAdd).isEmpty(); - } - - @Test - public void parsePackageOverrides_configWithBothOverridesToAddAndRemove_returnsOverrides() { - // Note that change 56 is both added and removed, therefore it will only be removed. - PackageOverrides result = AppCompatOverridesParser.parsePackageOverrides(/* configStr= */ - "56:::,12:::true,34:::,56:3:7:true", /* versionCode= */ 5, /* changeIdsToSkip= */ + Map result = AppCompatOverridesParser.parsePackageOverrides( + /* configStr= */ "123:::true", /* versionCode= */ 5, /* changeIdsToSkip= */ emptySet()); - assertThat(result.overridesToAdd).hasSize(1); - assertThat(result.overridesToAdd.get(12L)).isEqualTo( + assertThat(result).hasSize(1); + assertThat(result.get(123L)).isEqualTo( new PackageOverride.Builder().setEnabled(true).build()); - assertThat(result.overridesToRemove).containsExactly(34L, 56L); + } + + @Test + public void parsePackageOverrides_configWithMultipleOverrides_returnsOverrides() { + Map result = AppCompatOverridesParser.parsePackageOverrides( + /* configStr= */ "910:3:4:false,78:10::false,12:::false,34:1:2:true,34:10::true," + + "56::2:true,56:3:4:false,34:4:8:true,78:6:7:true,910:5::true," + + "1112::5:true,56:6::true,1112:6:7:false", /* versionCode= */ + 5, /* changeIdsToSkip= */ emptySet()); + + assertThat(result).hasSize(6); + assertThat(result.get(12L)).isEqualTo( + new PackageOverride.Builder().setEnabled(false).build()); + assertThat(result.get(34L)).isEqualTo( + new PackageOverride.Builder().setMinVersionCode(4).setMaxVersionCode(8).setEnabled( + true).build()); + assertThat(result.get(56L)).isEqualTo( + new PackageOverride.Builder().setMinVersionCode(3).setMaxVersionCode(4).setEnabled( + false).build()); + assertThat(result.get(78L)).isEqualTo( + new PackageOverride.Builder().setMinVersionCode(6).setMaxVersionCode(7).setEnabled( + true).build()); + assertThat(result.get(910L)).isEqualTo( + new PackageOverride.Builder().setMinVersionCode(5).setEnabled(true).build()); + assertThat(result.get(1112L)).isEqualTo( + new PackageOverride.Builder().setMaxVersionCode(5).setEnabled(true).build()); } @Test public void parsePackageOverrides_changeIdsToSkipSpecified_returnsWithoutChangeIdsToSkip() { - ArraySet changeIdsToSkip = new ArraySet<>(); - changeIdsToSkip.add(34L); - changeIdsToSkip.add(56L); - changeIdsToSkip.add(910L); - PackageOverrides result = AppCompatOverridesParser.parsePackageOverrides(/* configStr= */ - "12:::true,34:::,56:3:7:true,78:::", /* versionCode= */ 5, changeIdsToSkip); + ArraySet changeIdsToSkip = new ArraySet<>(Arrays.asList(34L, 56L)); + Map result = AppCompatOverridesParser.parsePackageOverrides( + /* configStr= */ "12:::true,56:3:7:true", /* versionCode= */ 5, changeIdsToSkip); - assertThat(result.overridesToAdd).hasSize(1); - assertThat(result.overridesToAdd.get(12L)).isEqualTo( + assertThat(result).hasSize(1); + assertThat(result.get(12L)).isEqualTo( new PackageOverride.Builder().setEnabled(true).build()); - assertThat(result.overridesToRemove).containsExactly(78L); } @Test public void parsePackageOverrides_changeIdsToSkipContainsAllIds_returnsEmpty() { - ArraySet changeIdsToSkip = new ArraySet<>(); - changeIdsToSkip.add(12L); - changeIdsToSkip.add(34L); - PackageOverrides result = AppCompatOverridesParser.parsePackageOverrides(/* configStr= */ - "12:::true,34:::", /* versionCode= */ 5, changeIdsToSkip); + ArraySet changeIdsToSkip = new ArraySet<>(Arrays.asList(12L, 34L)); + Map result = AppCompatOverridesParser.parsePackageOverrides( + /* configStr= */ "12:::true", /* versionCode= */ 5, changeIdsToSkip); - assertThat(result.overridesToAdd).isEmpty(); - assertThat(result.overridesToRemove).isEmpty(); + assertThat(result).isEmpty(); } @Test public void parsePackageOverrides_someOverridesAreInvalid_returnsWithoutInvalidOverrides() { // We add a valid entry before and after the invalid ones to make sure they are applied. - PackageOverrides result = AppCompatOverridesParser.parsePackageOverrides(/* configStr= */ - "12:::True,56:1:2:FALSE,56:3:true,78:4:8:true:,C1:::true,910:::no," - + "1112:1:ten:true,1112:one:10:true,,1314:7:3:false,34:one:ten:", + Map result = AppCompatOverridesParser.parsePackageOverrides( + /* configStr= */ "12:::True,56:1:2:FALSE,56:3:true,78:4:8:true:,C1:::true,910:::no," + + "1112:1:ten:true,1112:one:10:true,,1314:7:3:false,34:::", /* versionCode= */ 5, /* changeIdsToSkip= */ emptySet()); - assertThat(result.overridesToAdd).hasSize(2); - assertThat(result.overridesToAdd.get(12L)).isEqualTo( + assertThat(result).hasSize(2); + assertThat(result.get(12L)).isEqualTo( new PackageOverride.Builder().setEnabled(true).build()); - assertThat(result.overridesToAdd.get(56L)).isEqualTo( + assertThat(result.get(56L)).isEqualTo( new PackageOverride.Builder().setMinVersionCode(1).setMaxVersionCode(2).setEnabled( false).build()); - assertThat(result.overridesToRemove).containsExactly(34L); } private static ApplicationInfo createAppInfo(String packageName) { diff --git a/services/tests/mockingservicestests/src/com/android/server/compat/overrides/AppCompatOverridesServiceTest.java b/services/tests/mockingservicestests/src/com/android/server/compat/overrides/AppCompatOverridesServiceTest.java index 312927206a801..007191f026319 100644 --- a/services/tests/mockingservicestests/src/com/android/server/compat/overrides/AppCompatOverridesServiceTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/compat/overrides/AppCompatOverridesServiceTest.java @@ -95,7 +95,6 @@ public class AppCompatOverridesServiceTest { private static final String PACKAGE_2 = "com.android.test2"; private static final String PACKAGE_3 = "com.android.test3"; private static final String PACKAGE_4 = "com.android.test4"; - private static final String PACKAGE_5 = "com.android.test5"; private MockContext mMockContext; private BroadcastReceiver mPackageReceiver; @@ -157,16 +156,14 @@ public class AppCompatOverridesServiceTest { mockGetApplicationInfoNotInstalled(PACKAGE_2); mockGetApplicationInfo(PACKAGE_3, /* versionCode= */ 10); mockGetApplicationInfo(PACKAGE_4, /* versionCode= */ 1); - mockGetApplicationInfo(PACKAGE_5, /* versionCode= */ 1); mService.registerDeviceConfigListeners(); DeviceConfig.setProperties(new Properties.Builder(NAMESPACE_1) - .setString(PACKAGE_1, "123:::true,456::1:false,456:2::true") + .setString(FLAG_OWNED_CHANGE_IDS, "123,456,789") + .setString(PACKAGE_1, "123:::true,456::1:false,456:2::true,789:::false") .setString(PACKAGE_2, "123:::true") - .setString(PACKAGE_3, "123:1:9:true,123:10:11:false,123:11::true,456:::") - .setString(PACKAGE_4, "") - .setString(PACKAGE_5, "123:::,789:::") - .setString(FLAG_OWNED_CHANGE_IDS, "123,456,789").build()); + .setString(PACKAGE_3, "123:1:9:true,123:10:11:false,123:11::true") + .setString(PACKAGE_4, "").build()); Map addedOverrides; // Package 1 @@ -175,11 +172,13 @@ public class AppCompatOverridesServiceTest { verify(mPlatformCompat, never()).removeOverridesOnReleaseBuilds( any(CompatibilityOverridesToRemoveConfig.class), eq(PACKAGE_1)); addedOverrides = mOverridesToAddConfigCaptor.getValue().overrides; - assertThat(addedOverrides).hasSize(2); + assertThat(addedOverrides).hasSize(3); assertThat(addedOverrides.get(123L)).isEqualTo( new PackageOverride.Builder().setEnabled(true).build()); assertThat(addedOverrides.get(456L)).isEqualTo( new PackageOverride.Builder().setMinVersionCode(2).setEnabled(true).build()); + assertThat(addedOverrides.get(789L)).isEqualTo( + new PackageOverride.Builder().setEnabled(false).build()); // Package 2 verify(mPlatformCompat, never()).putOverridesOnReleaseBuilds( any(CompatibilityOverrideConfig.class), eq(PACKAGE_2)); @@ -195,24 +194,37 @@ public class AppCompatOverridesServiceTest { assertThat(addedOverrides.get(123L)).isEqualTo( new PackageOverride.Builder().setMinVersionCode(10).setMaxVersionCode( 11).setEnabled(false).build()); - assertThat(mOverridesToRemoveConfigCaptor.getValue().changeIds).containsExactly(456L); + assertThat(mOverridesToRemoveConfigCaptor.getValue().changeIds).containsExactly(456L, 789L); // Package 4 verify(mPlatformCompat, never()).putOverridesOnReleaseBuilds( any(CompatibilityOverrideConfig.class), eq(PACKAGE_4)); - verify(mPlatformCompat, never()).removeOverridesOnReleaseBuilds( - any(CompatibilityOverridesToRemoveConfig.class), eq(PACKAGE_4)); - // Package 5 - verify(mPlatformCompat, never()).putOverridesOnReleaseBuilds( - any(CompatibilityOverrideConfig.class), eq(PACKAGE_5)); verify(mPlatformCompat).removeOverridesOnReleaseBuilds( - mOverridesToRemoveConfigCaptor.capture(), eq(PACKAGE_5)); - assertThat(mOverridesToRemoveConfigCaptor.getValue().changeIds).containsExactly(123L, 789L); + mOverridesToRemoveConfigCaptor.capture(), eq(PACKAGE_4)); + assertThat(mOverridesToRemoveConfigCaptor.getValue().changeIds).containsExactly(123L, 456L, + 789L); + } + + @Test + public void onPropertiesChanged_ownedChangeIdsFlagNotSet_onlyAddsOverrides() + throws Exception { + mockGetApplicationInfo(PACKAGE_1, /* versionCode= */ 0); + + mService.registerDeviceConfigListeners(); + DeviceConfig.setProperties(new Properties.Builder(NAMESPACE_1) + .setString(PACKAGE_1, "123:::true").build()); + + verify(mPlatformCompat).putOverridesOnReleaseBuilds(mOverridesToAddConfigCaptor.capture(), + eq(PACKAGE_1)); + verify(mPlatformCompat, never()).removeOverridesOnReleaseBuilds( + any(CompatibilityOverridesToRemoveConfig.class), eq(PACKAGE_1)); + assertThat(mOverridesToAddConfigCaptor.getValue().overrides.keySet()).containsExactly(123L); } @Test public void onPropertiesChanged_removeOverridesFlagSetBefore_skipsOverridesToRemove() throws Exception { DeviceConfig.setProperties(new Properties.Builder(NAMESPACE_1) + .setString(FLAG_OWNED_CHANGE_IDS, "123,456,789") .setString(FLAG_REMOVE_OVERRIDES, PACKAGE_1 + "=123:456," + PACKAGE_2 + "=123") .setString(PACKAGE_1, "123:::true") .setString(PACKAGE_4, "123:::true").build()); @@ -222,7 +234,7 @@ public class AppCompatOverridesServiceTest { mService.registerDeviceConfigListeners(); DeviceConfig.setProperties(new Properties.Builder(NAMESPACE_1) - .setString(PACKAGE_1, "123:::true,456:::,789:::false") + .setString(PACKAGE_1, "123:::true,789:::false") .setString(PACKAGE_2, "123:::true") .setString(PACKAGE_3, "456:::true").build()); @@ -235,14 +247,16 @@ public class AppCompatOverridesServiceTest { // Package 2 verify(mPlatformCompat, never()).putOverridesOnReleaseBuilds( any(CompatibilityOverrideConfig.class), eq(PACKAGE_2)); - verify(mPlatformCompat, never()).removeOverridesOnReleaseBuilds( - any(CompatibilityOverridesToRemoveConfig.class), eq(PACKAGE_2)); + verify(mPlatformCompat).removeOverridesOnReleaseBuilds( + mOverridesToRemoveConfigCaptor.capture(), eq(PACKAGE_2)); + assertThat(mOverridesToRemoveConfigCaptor.getValue().changeIds).containsExactly(456L, 789L); // Package 3 verify(mPlatformCompat).putOverridesOnReleaseBuilds(mOverridesToAddConfigCaptor.capture(), eq(PACKAGE_3)); - verify(mPlatformCompat, never()).removeOverridesOnReleaseBuilds( - any(CompatibilityOverridesToRemoveConfig.class), eq(PACKAGE_3)); + verify(mPlatformCompat).removeOverridesOnReleaseBuilds( + mOverridesToRemoveConfigCaptor.capture(), eq(PACKAGE_3)); assertThat(mOverridesToAddConfigCaptor.getValue().overrides.keySet()).containsExactly(456L); + assertThat(mOverridesToRemoveConfigCaptor.getValue().changeIds).containsExactly(123L, 789L); // Package 4 (not applied because it hasn't changed after the listener was added) verify(mPlatformCompat, never()).putOverridesOnReleaseBuilds( any(CompatibilityOverrideConfig.class), eq(PACKAGE_4)); @@ -253,27 +267,44 @@ public class AppCompatOverridesServiceTest { @Test public void onPropertiesChanged_removeOverridesFlagChangedNoPackageOverridesFlags_removesOnly() throws Exception { + DeviceConfig.setProperties(new Properties.Builder(NAMESPACE_1) + .setString(FLAG_OWNED_CHANGE_IDS, "123,456,789") + .setString(PACKAGE_1, "") + .setString(PACKAGE_2, "").build()); + mockGetApplicationInfo(PACKAGE_1, /* versionCode= */ 0); + mockGetApplicationInfo(PACKAGE_2, /* versionCode= */ 0); + mService.registerDeviceConfigListeners(); DeviceConfig.setProperties(new Properties.Builder(NAMESPACE_1) .setString(FLAG_REMOVE_OVERRIDES, - PACKAGE_1 + "=123:456," + PACKAGE_2 + "=789").build()); + PACKAGE_1 + "=123:456," + PACKAGE_2 + "=*").build()); // Package 1 - verify(mPlatformCompat).removeOverridesOnReleaseBuilds( + verify(mPlatformCompat, never()).putOverridesOnReleaseBuilds( + any(CompatibilityOverrideConfig.class), eq(PACKAGE_1)); + verify(mPlatformCompat, times(2)).removeOverridesOnReleaseBuilds( mOverridesToRemoveConfigCaptor.capture(), eq(PACKAGE_1)); - assertThat(mOverridesToRemoveConfigCaptor.getValue().changeIds).containsExactly(123L, 456L); + List configs = + mOverridesToRemoveConfigCaptor.getAllValues(); + assertThat(configs.size()).isAtLeast(2); + assertThat(configs.get(configs.size() - 2).changeIds).containsExactly(123L, 456L); + assertThat(configs.get(configs.size() - 1).changeIds).containsExactly(789L); // Package 2 + verify(mPlatformCompat, never()).putOverridesOnReleaseBuilds( + any(CompatibilityOverrideConfig.class), eq(PACKAGE_2)); verify(mPlatformCompat).removeOverridesOnReleaseBuilds( mOverridesToRemoveConfigCaptor.capture(), eq(PACKAGE_2)); - assertThat(mOverridesToRemoveConfigCaptor.getValue().changeIds).containsExactly(789L); + assertThat(mOverridesToRemoveConfigCaptor.getValue().changeIds).containsExactly(123L, 456L, + 789L); } @Test public void onPropertiesChanged_removeOverridesFlagAndSomePackageOverrideFlagsChanged_ok() throws Exception { DeviceConfig.setProperties(new Properties.Builder(NAMESPACE_1) + .setString(FLAG_OWNED_CHANGE_IDS, "123,456,789") .setString(FLAG_REMOVE_OVERRIDES, PACKAGE_1 + "=123:456") - .setString(PACKAGE_1, "123:::true,456:::,789:::false") + .setString(PACKAGE_1, "123:::true,789:::false") .setString(PACKAGE_3, "456:::false,789:::true").build()); mockGetApplicationInfo(PACKAGE_1, /* versionCode= */ 0); mockGetApplicationInfo(PACKAGE_2, /* versionCode= */ 0); @@ -282,7 +313,7 @@ public class AppCompatOverridesServiceTest { mService.registerDeviceConfigListeners(); DeviceConfig.setProperties(new Properties.Builder(NAMESPACE_1) .setString(FLAG_REMOVE_OVERRIDES, PACKAGE_2 + "=123," + PACKAGE_3 + "=789") - .setString(PACKAGE_2, "123:::true,456:::").build()); + .setString(PACKAGE_2, "123:::true").build()); // Package 1 verify(mPlatformCompat).putOverridesOnReleaseBuilds(mOverridesToAddConfigCaptor.capture(), @@ -301,14 +332,17 @@ public class AppCompatOverridesServiceTest { mOverridesToRemoveConfigCaptor.getAllValues(); assertThat(configs.size()).isAtLeast(2); assertThat(configs.get(configs.size() - 2).changeIds).containsExactly(123L); - assertThat(configs.get(configs.size() - 1).changeIds).containsExactly(456L); + assertThat(configs.get(configs.size() - 1).changeIds).containsExactly(456L, 789L); // Package 3 verify(mPlatformCompat).putOverridesOnReleaseBuilds(mOverridesToAddConfigCaptor.capture(), eq(PACKAGE_3)); - verify(mPlatformCompat).removeOverridesOnReleaseBuilds( + verify(mPlatformCompat, times(2)).removeOverridesOnReleaseBuilds( mOverridesToRemoveConfigCaptor.capture(), eq(PACKAGE_3)); assertThat(mOverridesToAddConfigCaptor.getValue().overrides.keySet()).containsExactly(456L); - assertThat(mOverridesToRemoveConfigCaptor.getValue().changeIds).containsExactly(789L); + configs = mOverridesToRemoveConfigCaptor.getAllValues(); + assertThat(configs.size()).isAtLeast(2); + assertThat(configs.get(configs.size() - 2).changeIds).containsExactly(789L); + assertThat(configs.get(configs.size() - 1).changeIds).containsExactly(123L); } @Test @@ -338,9 +372,10 @@ public class AppCompatOverridesServiceTest { // Package 2 verify(mPlatformCompat).putOverridesOnReleaseBuilds(mOverridesToAddConfigCaptor.capture(), eq(PACKAGE_2)); - verify(mPlatformCompat, never()).removeOverridesOnReleaseBuilds( - any(CompatibilityOverridesToRemoveConfig.class), eq(PACKAGE_2)); + verify(mPlatformCompat).removeOverridesOnReleaseBuilds( + mOverridesToRemoveConfigCaptor.capture(), eq(PACKAGE_2)); assertThat(mOverridesToAddConfigCaptor.getValue().overrides.keySet()).containsExactly(123L); + assertThat(mOverridesToRemoveConfigCaptor.getValue().changeIds).containsExactly(456L, 789L); // Package 3 verify(mPlatformCompat, never()).putOverridesOnReleaseBuilds( any(CompatibilityOverrideConfig.class), eq(PACKAGE_3)); @@ -362,10 +397,11 @@ public class AppCompatOverridesServiceTest { mService.registerDeviceConfigListeners(); DeviceConfig.setProperties(new Properties.Builder(NAMESPACE_1) - .setString(PACKAGE_1, "123:::true,456:::") - .setString(PACKAGE_2, "123:::true,456:::") - .setString(PACKAGE_3, "123:::true,456:::") - .setString(PACKAGE_4, "123:::true,456:::").build()); + .setString(FLAG_OWNED_CHANGE_IDS, "123,456") + .setString(PACKAGE_1, "123:::true") + .setString(PACKAGE_2, "123:::true") + .setString(PACKAGE_3, "123:::true") + .setString(PACKAGE_4, "123:::true").build()); // Package 1 verify(mPlatformCompat).putOverridesOnReleaseBuilds(any(CompatibilityOverrideConfig.class), @@ -478,12 +514,16 @@ public class AppCompatOverridesServiceTest { @Test public void packageReceiver_packageAddedIntent_appliesOverridesFromAllNamespaces() throws Exception { + // We're adding the owned_change_ids flag to make sure it's ignored. DeviceConfig.setProperties(new Properties.Builder(NAMESPACE_1) - .setString(PACKAGE_1, "101:::true,103:::") + .setString(FLAG_OWNED_CHANGE_IDS, "101,102,103") + .setString(PACKAGE_1, "101:::true") .setString(PACKAGE_2, "102:::false").build()); DeviceConfig.setProperties(new Properties.Builder(NAMESPACE_2) + .setString(FLAG_OWNED_CHANGE_IDS, "201,202,203") .setString(PACKAGE_3, "201:::false").build()); DeviceConfig.setProperties(new Properties.Builder(NAMESPACE_3) + .setString(FLAG_OWNED_CHANGE_IDS, "301,302") .setString(PACKAGE_1, "301:::true,302:::false") .setString(PACKAGE_2, "302:::false").build()); mockGetApplicationInfo(PACKAGE_1, /* versionCode= */ 0); @@ -493,19 +533,18 @@ public class AppCompatOverridesServiceTest { verify(mPlatformCompat, times(2)).putOverridesOnReleaseBuilds( mOverridesToAddConfigCaptor.capture(), eq(PACKAGE_1)); - verify(mPlatformCompat).removeOverridesOnReleaseBuilds( - mOverridesToRemoveConfigCaptor.capture(), eq(PACKAGE_1)); + verify(mPlatformCompat, never()).removeOverridesOnReleaseBuilds( + any(CompatibilityOverridesToRemoveConfig.class), eq(PACKAGE_1)); List configs = mOverridesToAddConfigCaptor.getAllValues(); assertThat(configs.get(0).overrides.keySet()).containsExactly(101L); assertThat(configs.get(1).overrides.keySet()).containsExactly(301L, 302L); - assertThat(mOverridesToRemoveConfigCaptor.getValue().changeIds).containsExactly(103L); } @Test public void packageReceiver_packageChangedIntent_appliesOverrides() throws Exception { DeviceConfig.setProperties(new Properties.Builder(NAMESPACE_1) - .setString(PACKAGE_1, "101:::true,103:::").build()); + .setString(PACKAGE_1, "101:::true,103:::false").build()); mockGetApplicationInfo(PACKAGE_1, /* versionCode= */ 0); mPackageReceiver.onReceive(mMockContext, @@ -513,10 +552,10 @@ public class AppCompatOverridesServiceTest { verify(mPlatformCompat).putOverridesOnReleaseBuilds( mOverridesToAddConfigCaptor.capture(), eq(PACKAGE_1)); - verify(mPlatformCompat).removeOverridesOnReleaseBuilds( - mOverridesToRemoveConfigCaptor.capture(), eq(PACKAGE_1)); - assertThat(mOverridesToAddConfigCaptor.getValue().overrides.keySet()).containsExactly(101L); - assertThat(mOverridesToRemoveConfigCaptor.getValue().changeIds).containsExactly(103L); + verify(mPlatformCompat, never()).removeOverridesOnReleaseBuilds( + any(CompatibilityOverridesToRemoveConfig.class), eq(PACKAGE_1)); + assertThat(mOverridesToAddConfigCaptor.getValue().overrides.keySet()).containsExactly(101L, + 103L); } @Test @@ -524,13 +563,13 @@ public class AppCompatOverridesServiceTest { throws Exception { DeviceConfig.setProperties(new Properties.Builder(NAMESPACE_1) .setString(FLAG_REMOVE_OVERRIDES, PACKAGE_1 + "=103," + PACKAGE_2 + "=101") - .setString(PACKAGE_1, "101:::true,103:::") + .setString(PACKAGE_1, "101:::true,103:::false") .setString(PACKAGE_2, "102:::false").build()); DeviceConfig.setProperties(new Properties.Builder(NAMESPACE_2) .setString(PACKAGE_1, "201:::false").build()); DeviceConfig.setProperties(new Properties.Builder(NAMESPACE_3) .setString(FLAG_REMOVE_OVERRIDES, PACKAGE_1 + "=301," + PACKAGE_3 + "=302") - .setString(PACKAGE_1, "301:::true,302:::false,303:::") + .setString(PACKAGE_1, "301:::true,302:::false,303:::true") .setString(PACKAGE_3, "302:::false").build()); mockGetApplicationInfo(PACKAGE_1, /* versionCode= */ 0); @@ -539,13 +578,12 @@ public class AppCompatOverridesServiceTest { verify(mPlatformCompat, times(3)).putOverridesOnReleaseBuilds( mOverridesToAddConfigCaptor.capture(), eq(PACKAGE_1)); - verify(mPlatformCompat).removeOverridesOnReleaseBuilds( - mOverridesToRemoveConfigCaptor.capture(), eq(PACKAGE_1)); + verify(mPlatformCompat, never()).removeOverridesOnReleaseBuilds( + any(CompatibilityOverridesToRemoveConfig.class), eq(PACKAGE_1)); List configs = mOverridesToAddConfigCaptor.getAllValues(); assertThat(configs.get(0).overrides.keySet()).containsExactly(101L); assertThat(configs.get(1).overrides.keySet()).containsExactly(201L); - assertThat(configs.get(2).overrides.keySet()).containsExactly(302L); - assertThat(mOverridesToRemoveConfigCaptor.getValue().changeIds).containsExactly(303L); + assertThat(configs.get(2).overrides.keySet()).containsExactly(302L, 303L); } @Test @@ -573,7 +611,7 @@ public class AppCompatOverridesServiceTest { throws Exception { DeviceConfig.setProperties(new Properties.Builder(NAMESPACE_1) .setString(FLAG_OWNED_CHANGE_IDS, "101,102,103") - .setString(PACKAGE_1, "101:::true,103:::").build()); + .setString(PACKAGE_1, "101:::true,103:::false").build()); DeviceConfig.setProperties(new Properties.Builder(NAMESPACE_2) .setString(FLAG_OWNED_CHANGE_IDS, "201,202") .setString(PACKAGE_1, "202:::false").build()); @@ -593,14 +631,14 @@ public class AppCompatOverridesServiceTest { throws Exception { DeviceConfig.setProperties(new Properties.Builder(NAMESPACE_1) .setString(FLAG_OWNED_CHANGE_IDS, "101,102,103") - .setString(PACKAGE_1, "101:::true,103:::") + .setString(PACKAGE_1, "101:::true,103:::false") .setString(PACKAGE_2, "102:::false").build()); DeviceConfig.setProperties(new Properties.Builder(NAMESPACE_2) .setString(FLAG_OWNED_CHANGE_IDS, "201") .setString(PACKAGE_3, "201:::false").build()); DeviceConfig.setProperties(new Properties.Builder(NAMESPACE_3) .setString(FLAG_OWNED_CHANGE_IDS, "301,302") - .setString(PACKAGE_1, "302:::") + .setString(PACKAGE_1, "302:::false") .setString(PACKAGE_2, "301:::true").build()); mockGetApplicationInfoNotInstalled(PACKAGE_1);