diff options
| author | 2021-09-24 12:31:20 +0000 | |
|---|---|---|
| committer | 2021-09-24 15:32:05 +0000 | |
| commit | 9a843fba26818b69077201de7571a8e1ffc2d5fe (patch) | |
| tree | de6e4f7e459958daea9fc8264ed4f7d9a64e841c | |
| parent | 66889d3212ba8dd3ba6b844648248e6a4fe42cea (diff) | |
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
4 files changed, 188 insertions, 207 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 a81213df6fe3..11dc1dbd25cc 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. * * <p>Each change override is in the following format: - * '<change-id>:<min-version-code?>:<max-version-code?>:<enabled?>'. If <enabled> is empty, - * this indicates that any override for the specified change ID should be removed. + * '<change-id>:<min-version-code?>:<max-version-code?>:<enabled>'. * * <p>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. * * <p>Any overrides whose change ID is in {@code changeIdsToSkip} are ignored. * - * <p>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. + * <p>If a change override entry in {@code configStr} is invalid, it will be ignored. */ - static PackageOverrides parsePackageOverrides( - String configStr, long versionCode, Set<Long> changeIdsToSkip) { + static Map<Long, PackageOverride> parsePackageOverrides(String configStr, long versionCode, + Set<Long> changeIdsToSkip) { if (configStr.isEmpty()) { - return new PackageOverrides(); + return emptyMap(); } PackageOverrideComparator comparator = new PackageOverrideComparator(versionCode); Map<Long, PackageOverride> overridesToAdd = new ArrayMap<>(); - Set<Long> overridesToRemove = new ArraySet<>(); for (String overrideEntryString : configStr.split(",")) { List<String> 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. - * - * <p>The map of overrides to add and the set of overrides to remove are mutually exclusive. - */ - static final class PackageOverrides { - public final Map<Long, PackageOverride> overridesToAdd; - public final Set<Long> overridesToRemove; - - PackageOverrides() { - this(emptyMap(), emptySet()); - } - - PackageOverrides(Map<Long, PackageOverride> overridesToAdd, Set<Long> 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 63ae1af42f08..6aed4b023297 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<Long> ownedChangeIds, Map<String, Set<Long>> 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. + * + * <p>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<Long> ownedChangeIds, Map<String, Set<Long>> packageToChangeIdsToSkip) { Set<String> 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<Long> 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}. + * + * <p>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<Long> changeIdsToSkip) { - PackageOverrides packageOverrides = AppCompatOverridesParser.parsePackageOverrides( + private void applyPackageOverrides(String configStr, String packageName, long versionCode, + Set<Long> ownedChangeIds, Set<Long> changeIdsToSkip, + boolean removeOtherOwnedOverrides) { + Map<Long, PackageOverride> overridesToAdd = AppCompatOverridesParser.parsePackageOverrides( configStr, versionCode, changeIdsToSkip); - putPackageOverrides(packageName, packageOverrides.overridesToAdd); - removePackageOverrides(packageName, packageOverrides.overridesToRemove); + putPackageOverrides(packageName, overridesToAdd); + + if (!removeOtherOwnedOverrides) { + return; + } + Set<Long> 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<String, Set<Long>> getOverridesToRemove(String namespace) { + private Map<String, Set<Long>> getOverridesToRemove(String namespace, + Set<Long> 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<String, Set<Long>> overridesToRemove = getOverridesToRemove(mNamespace); + Set<Long> ownedChangeIds = getOwnedChangeIds(mNamespace); + Map<String, Set<Long>> 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 bf97042d7b88..df19be4a9cfe 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<Long, PackageOverride> 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()); + Map<Long, PackageOverride> result = AppCompatOverridesParser.parsePackageOverrides( + /* configStr= */ "123:::true", /* versionCode= */ 5, /* changeIdsToSkip= */ + emptySet()); - assertThat(result.overridesToAdd).hasSize(1); - assertThat(result.overridesToAdd.get(123L)).isEqualTo( + assertThat(result).hasSize(1); + assertThat(result.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= */ + public void parsePackageOverrides_configWithMultipleOverrides_returnsOverrides() { + Map<Long, PackageOverride> 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( + assertThat(result).hasSize(6); + assertThat(result.get(12L)).isEqualTo( new PackageOverride.Builder().setEnabled(false).build()); - assertThat(result.overridesToAdd.get(34L)).isEqualTo( + assertThat(result.get(34L)).isEqualTo( new PackageOverride.Builder().setMinVersionCode(4).setMaxVersionCode(8).setEnabled( true).build()); - assertThat(result.overridesToAdd.get(56L)).isEqualTo( + assertThat(result.get(56L)).isEqualTo( new PackageOverride.Builder().setMinVersionCode(3).setMaxVersionCode(4).setEnabled( false).build()); - assertThat(result.overridesToAdd.get(78L)).isEqualTo( + assertThat(result.get(78L)).isEqualTo( new PackageOverride.Builder().setMinVersionCode(6).setMaxVersionCode(7).setEnabled( true).build()); - assertThat(result.overridesToAdd.get(910L)).isEqualTo( + assertThat(result.get(910L)).isEqualTo( new PackageOverride.Builder().setMinVersionCode(5).setEnabled(true).build()); - assertThat(result.overridesToAdd.get(1112L)).isEqualTo( + assertThat(result.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= */ - emptySet()); - - assertThat(result.overridesToAdd).hasSize(1); - assertThat(result.overridesToAdd.get(12L)).isEqualTo( - new PackageOverride.Builder().setEnabled(true).build()); - assertThat(result.overridesToRemove).containsExactly(34L, 56L); } @Test public void parsePackageOverrides_changeIdsToSkipSpecified_returnsWithoutChangeIdsToSkip() { - ArraySet<Long> 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); - - assertThat(result.overridesToAdd).hasSize(1); - assertThat(result.overridesToAdd.get(12L)).isEqualTo( + ArraySet<Long> changeIdsToSkip = new ArraySet<>(Arrays.asList(34L, 56L)); + Map<Long, PackageOverride> result = AppCompatOverridesParser.parsePackageOverrides( + /* configStr= */ "12:::true,56:3:7:true", /* versionCode= */ 5, changeIdsToSkip); + + 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<Long> changeIdsToSkip = new ArraySet<>(); - changeIdsToSkip.add(12L); - changeIdsToSkip.add(34L); - PackageOverrides result = AppCompatOverridesParser.parsePackageOverrides(/* configStr= */ - "12:::true,34:::", /* versionCode= */ 5, changeIdsToSkip); - - assertThat(result.overridesToAdd).isEmpty(); - assertThat(result.overridesToRemove).isEmpty(); + ArraySet<Long> changeIdsToSkip = new ArraySet<>(Arrays.asList(12L, 34L)); + Map<Long, PackageOverride> result = AppCompatOverridesParser.parsePackageOverrides( + /* configStr= */ "12:::true", /* versionCode= */ 5, changeIdsToSkip); + + 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<Long, PackageOverride> 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 312927206a80..007191f02631 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<Long, PackageOverride> 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<CompatibilityOverridesToRemoveConfig> 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<CompatibilityOverrideConfig> 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<CompatibilityOverrideConfig> 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); |