From 25846953ce0a976435972f928e94506538152998 Mon Sep 17 00:00:00 2001 From: Dave Mankoff Date: Wed, 12 Jul 2023 19:32:11 +0000 Subject: [PATCH 1/6] Revert^2 "A new Client-Side FeatureFlags library." 4765b4597b950a623cb61c275596119be6e2dacd Bug: 279054964 Change-Id: I9bb79e6f6503af780406a989d79a50c2881db5db --- core/java/android/flags/BooleanFlag.java | 67 +++++++++ .../android/flags/DynamicBooleanFlag.java | 62 +++++++++ core/java/android/flags/DynamicFlag.java | 27 ++++ core/java/android/flags/FeatureFlags.java | 131 ++++++++++++++++++ core/java/android/flags/FeatureFlagsFake.java | 49 +++++++ core/java/android/flags/Flag.java | 40 ++++++ core/java/android/flags/FusedOffFlag.java | 60 ++++++++ core/java/android/flags/FusedOnFlag.java | 60 ++++++++ core/java/android/flags/OWNERS | 7 + .../src/android/flags/FeatureFlagsTest.java | 54 ++++++++ 10 files changed, 557 insertions(+) create mode 100644 core/java/android/flags/BooleanFlag.java create mode 100644 core/java/android/flags/DynamicBooleanFlag.java create mode 100644 core/java/android/flags/DynamicFlag.java create mode 100644 core/java/android/flags/FeatureFlags.java create mode 100644 core/java/android/flags/FeatureFlagsFake.java create mode 100644 core/java/android/flags/Flag.java create mode 100644 core/java/android/flags/FusedOffFlag.java create mode 100644 core/java/android/flags/FusedOnFlag.java create mode 100644 core/java/android/flags/OWNERS create mode 100644 core/tests/coretests/src/android/flags/FeatureFlagsTest.java diff --git a/core/java/android/flags/BooleanFlag.java b/core/java/android/flags/BooleanFlag.java new file mode 100644 index 0000000000000..5519961a917fa --- /dev/null +++ b/core/java/android/flags/BooleanFlag.java @@ -0,0 +1,67 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.flags; + +import android.annotation.NonNull; + +/** + * A flag representing a true or false value. + * + * The value will always be the same during the lifetime of the process it is read in. + * + * @hide + */ +public class BooleanFlag implements Flag { + private final String mNamespace; + private final String mName; + private final boolean mDefault; + + /** + * @param namespace A namespace for this flag. See {@link android.provider.DeviceConfig}. + * @param name A name for this flag. + * @param defaultValue The value of this flag if no other override is present. + */ + BooleanFlag(String namespace, String name, boolean defaultValue) { + mNamespace = namespace; + mName = name; + mDefault = defaultValue; + } + + @Override + @NonNull + public Boolean getDefault() { + return mDefault; + } + + @Override + @NonNull + public String getNamespace() { + return mNamespace; + } + + @Override + @NonNull + public String getName() { + return mName; + } + + @Override + @NonNull + public String toString() { + return getNamespace() + "." + getName() + "[" + getDefault() + "]"; + } +} diff --git a/core/java/android/flags/DynamicBooleanFlag.java b/core/java/android/flags/DynamicBooleanFlag.java new file mode 100644 index 0000000000000..d76fd9ec9a3c6 --- /dev/null +++ b/core/java/android/flags/DynamicBooleanFlag.java @@ -0,0 +1,62 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.flags; + +/** + * A flag representing a true or false value. + * + * The value may be different from one read to the next. + * + * @hide + */ +public class DynamicBooleanFlag implements DynamicFlag { + + private final String mNamespace; + private final String mName; + private final boolean mDefault; + + /** + * @param namespace A namespace for this flag. See {@link android.provider.DeviceConfig}. + * @param name A name for this flag. + * @param defaultValue The value of this flag if no other override is present. + */ + DynamicBooleanFlag(String namespace, String name, boolean defaultValue) { + mNamespace = namespace; + mName = name; + mDefault = defaultValue; + } + + @Override + public String getNamespace() { + return mNamespace; + } + + @Override + public String getName() { + return mName; + } + + @Override + public Boolean getDefault() { + return mDefault; + } + + @Override + public String toString() { + return getNamespace() + "." + getName() + "[" + getDefault() + "]"; + } +} diff --git a/core/java/android/flags/DynamicFlag.java b/core/java/android/flags/DynamicFlag.java new file mode 100644 index 0000000000000..8583ea6034d89 --- /dev/null +++ b/core/java/android/flags/DynamicFlag.java @@ -0,0 +1,27 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.flags; + +/** + * A flag for which the value may be different from one read to the next. + * + * @param The type of value that this flag stores. E.g. Boolean or String. + * + * @hide + */ +public interface DynamicFlag extends Flag { +} diff --git a/core/java/android/flags/FeatureFlags.java b/core/java/android/flags/FeatureFlags.java new file mode 100644 index 0000000000000..c7d9d57e68b59 --- /dev/null +++ b/core/java/android/flags/FeatureFlags.java @@ -0,0 +1,131 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.flags; + +import android.annotation.NonNull; + +import java.util.HashSet; +import java.util.Set; + +/** + * A class for querying constants from the system - primarily booleans. + * + * Clients using this class can define their flags and their default values in one place, + * can override those values on running devices for debugging and testing purposes, and can control + * what flags are available to be used on release builds. + * + * TODO(b/279054964): A lot. This is skeleton code right now. + * @hide + */ +public class FeatureFlags { + private static FeatureFlags sInstance; + private static final Object sInstanceLock = new Object(); + + private final Set mListeners = new HashSet<>(); + + /** + * Obtain a per-process instance of FeatureFlags. + * @return + */ + @NonNull + public static FeatureFlags getInstance() { + synchronized (sInstanceLock) { + if (sInstance == null) { + sInstance = new FeatureFlags(); + } + } + + return sInstance; + } + + FeatureFlags() { + } + + /** + * Returns whether the supplied flag is true or not. + * + * {@link BooleanFlag} should only be used in debug builds. They do not get optimized out. + * + * The first time a flag is read, its value is cached for the lifetime of the process. + */ + public boolean isEnabled(@NonNull BooleanFlag flag) { + return flag.getDefault(); + } + + /** + * Returns whether the supplied flag is true or not. + * + * Always returns false. + */ + public boolean isEnabled(@NonNull FusedOffFlag flag) { + return false; + } + + /** + * Returns whether the supplied flag is true or not. + * + * Always returns true; + */ + public boolean isEnabled(@NonNull FusedOnFlag flag) { + return true; + } + + /** + * Returns whether the supplied flag is true or not. + * + * Can return a different value for the flag each time it is called if an override comes in. + */ + public boolean isCurrentlyEnabled(@NonNull DynamicBooleanFlag flag) { + return flag.getDefault(); + } + + /** + * Add a listener to be alerted when a {@link DynamicFlag} changes. + * + * See also {@link #removeChangeListener(ChangeListener)}. + * + * @param listener The listener to add. + */ + public void addChangeListener(@NonNull ChangeListener listener) { + mListeners.add(listener); + } + + /** + * Remove a listener that was added earlier. + * + * See also {@link #addChangeListener(ChangeListener)}. + * + * @param listener The listener to remove. + */ + public void removeChangeListener(@NonNull ChangeListener listener) { + mListeners.remove(listener); + } + + /** + * A simpler listener that is alerted when a {@link DynamicFlag} changes. + * + * See {@link #addChangeListener(ChangeListener)} + */ + public interface ChangeListener { + /** + * Called when a {@link DynamicFlag} changes. + * + * @param flag The flag that has changed. + */ + void onFlagChanged(DynamicFlag flag); + } +} diff --git a/core/java/android/flags/FeatureFlagsFake.java b/core/java/android/flags/FeatureFlagsFake.java new file mode 100644 index 0000000000000..03bf2b6d33d26 --- /dev/null +++ b/core/java/android/flags/FeatureFlagsFake.java @@ -0,0 +1,49 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.flags; + +import android.annotation.NonNull; + +/** + * An implementation of {@link FeatureFlags} for testing. + * @hide + */ +public class FeatureFlagsFake extends FeatureFlags { + public FeatureFlagsFake() { + super(); + } + + @Override + public boolean isEnabled(@NonNull BooleanFlag flag) { + return flag.getDefault(); + } + + @Override + public boolean isEnabled(@NonNull FusedOffFlag flag) { + return false; + } + + @Override + public boolean isEnabled(@NonNull FusedOnFlag flag) { + return true; + } + + @Override + public boolean isCurrentlyEnabled(@NonNull DynamicBooleanFlag flag) { + return flag.getDefault(); + } +} diff --git a/core/java/android/flags/Flag.java b/core/java/android/flags/Flag.java new file mode 100644 index 0000000000000..ae0a6ba33f44b --- /dev/null +++ b/core/java/android/flags/Flag.java @@ -0,0 +1,40 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.flags; + +import android.annotation.NonNull; + +/** + * Base class for constants read via {@link android.flags.FeatureFlags}. + * + * @param The type of value that this flag stores. E.g. Boolean or String. + * + * @hide + */ +public interface Flag { + /** The namespace for a flag. Should combine uniquely with its name. */ + @NonNull + String getNamespace(); + + /** The name of the flag. Should combine uniquely with its namespace. */ + @NonNull + String getName(); + + /** The value of this flag if no override has been set. Null values are not supported. */ + @NonNull + T getDefault(); +} diff --git a/core/java/android/flags/FusedOffFlag.java b/core/java/android/flags/FusedOffFlag.java new file mode 100644 index 0000000000000..999d0f9817801 --- /dev/null +++ b/core/java/android/flags/FusedOffFlag.java @@ -0,0 +1,60 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.flags; + +import android.annotation.NonNull; + +/** + * A flag representing a false value. + * + * The flag can never be changed or overridden. It is false at compile time. + * + * @hide + */ +public final class FusedOffFlag implements Flag { + private final String mNamespace; + private final String mName; + + FusedOffFlag(String namespace, String name) { + mNamespace = namespace; + mName = name; + } + + @Override + @NonNull + public Boolean getDefault() { + return false; + } + + @Override + @NonNull + public String getNamespace() { + return mNamespace; + } + + @Override + @NonNull + public String getName() { + return mName; + } + + @Override + @NonNull + public String toString() { + return getNamespace() + "." + getName() + "[false]"; + } +} diff --git a/core/java/android/flags/FusedOnFlag.java b/core/java/android/flags/FusedOnFlag.java new file mode 100644 index 0000000000000..4f33bee8abe12 --- /dev/null +++ b/core/java/android/flags/FusedOnFlag.java @@ -0,0 +1,60 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.flags; + +import android.annotation.NonNull; + +/** + * A flag representing a true value. + * + * The flag can never be changed or overridden. It is true at compile time. + * + * @hide + */ +public final class FusedOnFlag implements Flag { + private final String mNamespace; + private final String mName; + + FusedOnFlag(String namespace, String name) { + mNamespace = namespace; + mName = name; + } + + @Override + @NonNull + public Boolean getDefault() { + return true; + } + + @Override + @NonNull + public String getNamespace() { + return mNamespace; + } + + @Override + @NonNull + public String getName() { + return mName; + } + + @Override + @NonNull + public String toString() { + return getNamespace() + "." + getName() + "[true]"; + } +} diff --git a/core/java/android/flags/OWNERS b/core/java/android/flags/OWNERS new file mode 100644 index 0000000000000..fa125c4a159cc --- /dev/null +++ b/core/java/android/flags/OWNERS @@ -0,0 +1,7 @@ +# Bug component: 1306523 + +mankoff@google.com +pixel@google.com + +dsandler@android.com + diff --git a/core/tests/coretests/src/android/flags/FeatureFlagsTest.java b/core/tests/coretests/src/android/flags/FeatureFlagsTest.java new file mode 100644 index 0000000000000..dc6006a8e5fa4 --- /dev/null +++ b/core/tests/coretests/src/android/flags/FeatureFlagsTest.java @@ -0,0 +1,54 @@ +/* + * Copyright (C) 2020 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.flags; + +import static com.google.common.truth.Truth.assertThat; + +import android.platform.test.annotations.Presubmit; + +import androidx.test.filters.SmallTest; + +import org.junit.After; +import org.junit.Before; +import org.junit.Ignore; +import org.junit.Test; +import org.mockito.MockitoAnnotations; + +@SmallTest +@Presubmit +public final class FeatureFlagsTest { + + @Before + public void setup() { + MockitoAnnotations.initMocks(this); + } + + @After + public void tearDown() { + } + + @Test + public void testPass() { + assertThat(true).isTrue(); + } + + @Ignore + @Test + public void testFail() { + assertThat(false).isTrue(); + } +} From 0c72a321d288593386c6ae05572dd88541ef4dc3 Mon Sep 17 00:00:00 2001 From: Dave Mankoff Date: Wed, 12 Jul 2023 19:32:11 +0000 Subject: [PATCH 2/6] Revert^2 "Define FeatureFlagsService." This revert contains one small fix: The prior change included changes in core/java/Android.bp and a corresponding line in services/flags/Android.bp that causes problems in StrictJavaPackagesTest #testBootClasspathAndSystemServerClasspath_nonDuplicateClasses Specifically resulting in this bug being filed: http://b/290929382 de960774a08274e68a4021e706f7611f0c0a880e Bug: 279054964 Change-Id: Ia2ac95bb22144ff8be84c711604db72d039a814d --- core/java/android/content/Context.java | 7 + core/java/android/flags/IFeatureFlags.aidl | 65 +++ .../android/flags/IFeatureFlagsCallback.aidl | 31 ++ core/java/android/flags/SyncableFlag.aidl | 22 + core/java/android/flags/SyncableFlag.java | 99 +++++ services/Android.bp | 1 + services/flags/Android.bp | 17 + services/flags/OWNERS | 6 + .../flags/DynamicFlagBinderDelegate.java | 273 +++++++++++++ .../server/flags/FeatureFlagsBinder.java | 86 ++++ .../server/flags/FeatureFlagsService.java | 59 +++ .../com/android/server/flags/FlagCache.java | 103 +++++ .../server/flags/FlagOverrideStore.java | 120 ++++++ .../server/flags/FlagsShellCommand.java | 214 ++++++++++ .../server/flags/GlobalSettingsProxy.java | 68 ++++ .../android/server/flags/SettingsProxy.java | 381 ++++++++++++++++++ .../java/com/android/server/SystemServer.java | 7 + services/tests/servicestests/Android.bp | 1 + .../server/flags/FeatureFlagsServiceTest.java | 179 ++++++++ .../android/server/flags/FlagCacheTest.java | 86 ++++ .../server/flags/FlagOverrideStoreTest.java | 111 +++++ .../src/com/android/server/flags/OWNERS | 1 + 22 files changed, 1937 insertions(+) create mode 100644 core/java/android/flags/IFeatureFlags.aidl create mode 100644 core/java/android/flags/IFeatureFlagsCallback.aidl create mode 100644 core/java/android/flags/SyncableFlag.aidl create mode 100644 core/java/android/flags/SyncableFlag.java create mode 100644 services/flags/Android.bp create mode 100644 services/flags/OWNERS create mode 100644 services/flags/java/com/android/server/flags/DynamicFlagBinderDelegate.java create mode 100644 services/flags/java/com/android/server/flags/FeatureFlagsBinder.java create mode 100644 services/flags/java/com/android/server/flags/FeatureFlagsService.java create mode 100644 services/flags/java/com/android/server/flags/FlagCache.java create mode 100644 services/flags/java/com/android/server/flags/FlagOverrideStore.java create mode 100644 services/flags/java/com/android/server/flags/FlagsShellCommand.java create mode 100644 services/flags/java/com/android/server/flags/GlobalSettingsProxy.java create mode 100644 services/flags/java/com/android/server/flags/SettingsProxy.java create mode 100644 services/tests/servicestests/src/com/android/server/flags/FeatureFlagsServiceTest.java create mode 100644 services/tests/servicestests/src/com/android/server/flags/FlagCacheTest.java create mode 100644 services/tests/servicestests/src/com/android/server/flags/FlagOverrideStoreTest.java create mode 100644 services/tests/servicestests/src/com/android/server/flags/OWNERS diff --git a/core/java/android/content/Context.java b/core/java/android/content/Context.java index 6d82922484bc8..2a6d84b1acc68 100644 --- a/core/java/android/content/Context.java +++ b/core/java/android/content/Context.java @@ -5297,6 +5297,13 @@ public abstract class Context { @SystemApi public static final String APP_PREDICTION_SERVICE = "app_prediction"; + /** + * Used for reading system-wide, overridable flags. + * + * @hide + */ + public static final String FEATURE_FLAGS_SERVICE = "feature_flags"; + /** * Official published name of the search ui service. * diff --git a/core/java/android/flags/IFeatureFlags.aidl b/core/java/android/flags/IFeatureFlags.aidl new file mode 100644 index 0000000000000..1eef47feaa8b3 --- /dev/null +++ b/core/java/android/flags/IFeatureFlags.aidl @@ -0,0 +1,65 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + + package android.flags; + +import android.flags.IFeatureFlagsCallback; +import android.flags.SyncableFlag; + +/** + * Binder interface for communicating with {@link com.android.server.flags.FeatureFlagsService}. + * + * This interface is used by {@link android.flags.FeatureFlags} and developers should use that to + * interface with the service. FeatureFlags is the "client" in this documentation. + * + * The methods allow client apps to communicate what flags they care about, and receive back + * current values for those flags. For stable flags, this is the finalized value until the device + * restarts. For {@link DynamicFlag}s, this is the last known value, though it may change in the + * future. Clients can listen for changes to flag values so that it can react accordingly. + * @hide + */ +interface IFeatureFlags { + /** + * Synchronize with the {@link com.android.server.flags.FeatureFlagsService} about flags of + * interest. + * + * The client should pass in a list of flags that it is using as {@link SyncableFlag}s, which + * includes what it thinks the default values of the flags are. + * + * The response will contain a list of matching SyncableFlags, whose values are set to what the + * value of the flags actually are. The client should update its internal state flag data to + * match. + * + * Generally speaking, if a flag that is passed in is new to the FeatureFlagsService, the + * service will cache the passed-in value, and return it back out. If, however, a different + * client has synced that flag with the service previously, FeatureFlagsService will return the + * existing cached value, which may or may not be what the current client passed in. This allows + * FeatureFlagsService to keep clients in agreement with one another. + */ + List syncFlags(in List flagList); + + /** + * Pass in an {@link IFeatureFlagsCallback} that will be called whenever a {@link DymamicFlag} + * changes. + */ + void registerCallback(IFeatureFlagsCallback callback); + + /** + * Remove a {@link IFeatureFlagsCallback} that was previously registered with + * {@link #registerCallback}. + */ + void unregisterCallback(IFeatureFlagsCallback callback); +} \ No newline at end of file diff --git a/core/java/android/flags/IFeatureFlagsCallback.aidl b/core/java/android/flags/IFeatureFlagsCallback.aidl new file mode 100644 index 0000000000000..f708667ea4c42 --- /dev/null +++ b/core/java/android/flags/IFeatureFlagsCallback.aidl @@ -0,0 +1,31 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + + package android.flags; + +import android.flags.SyncableFlag; + +/** + * Callback for {@link IFeatureFlags#registerCallback} to get alerts when a {@link DynamicFlag} + * changes. + * + * DynamicFlags can change at run time. Stable flags will never result in a call to this method. + * + * @hide + */ +oneway interface IFeatureFlagsCallback { + void onFlagChange(in SyncableFlag flag); +} \ No newline at end of file diff --git a/core/java/android/flags/SyncableFlag.aidl b/core/java/android/flags/SyncableFlag.aidl new file mode 100644 index 0000000000000..1526ec148967d --- /dev/null +++ b/core/java/android/flags/SyncableFlag.aidl @@ -0,0 +1,22 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.flags; + +/** + * A parcelable data class for serializing {@link Flag} across a Binder. + */ +parcelable SyncableFlag; \ No newline at end of file diff --git a/core/java/android/flags/SyncableFlag.java b/core/java/android/flags/SyncableFlag.java new file mode 100644 index 0000000000000..54c4c6d5fdf96 --- /dev/null +++ b/core/java/android/flags/SyncableFlag.java @@ -0,0 +1,99 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.flags; + +import android.annotation.NonNull; +import android.os.Parcel; +import android.os.Parcelable; + +import com.android.internal.annotations.VisibleForTesting; + +/** + * @hide + */ +public final class SyncableFlag implements Parcelable { + private final String mNamespace; + private final String mName; + private String mValue; + private final boolean mDynamic; + + @VisibleForTesting(visibility = VisibleForTesting.Visibility.PACKAGE) + public SyncableFlag( + @NonNull String namespace, + @NonNull String name, + @NonNull String value, + boolean dynamic) { + mNamespace = namespace; + mName = name; + mValue = value; + mDynamic = dynamic; + } + + public void setValue(@NonNull String value) { + mValue = value; + } + + @NonNull + public String getNamespace() { + return mNamespace; + } + + @NonNull + public String getName() { + return mName; + } + + @NonNull + public String getValue() { + return mValue; + } + + @NonNull + public boolean isDynamic() { + return mDynamic; + } + + @NonNull + public static final Parcelable.Creator CREATOR = new Parcelable.Creator<>() { + public SyncableFlag createFromParcel(Parcel in) { + return new SyncableFlag( + in.readString(), in.readString(), in.readString(), in.readBoolean()); + } + + public SyncableFlag[] newArray(int size) { + return new SyncableFlag[size]; + } + }; + + @Override + public int describeContents() { + return 0; + } + + @Override + public void writeToParcel(@NonNull Parcel dest, int flags) { + dest.writeString(mNamespace); + dest.writeString(mName); + dest.writeString(mValue); + dest.writeBoolean(mDynamic); + } + + @Override + public String toString() { + return getNamespace() + "." + getName() + "[" + getValue() + "]"; + } +} diff --git a/services/Android.bp b/services/Android.bp index b0a0e5e44a8cb..453f572341452 100644 --- a/services/Android.bp +++ b/services/Android.bp @@ -159,6 +159,7 @@ java_library { "services.coverage", "services.credentials", "services.devicepolicy", + "services.flags", "services.midi", "services.musicsearch", "services.net", diff --git a/services/flags/Android.bp b/services/flags/Android.bp new file mode 100644 index 0000000000000..2d0337dce74fd --- /dev/null +++ b/services/flags/Android.bp @@ -0,0 +1,17 @@ +package { + // See: http://go/android-license-faq + // A large-scale-change added 'default_applicable_licenses' to import + // all of the 'license_kinds' from "frameworks_base_license" + // to get the below license kinds: + // SPDX-license-identifier-Apache-2.0 + default_applicable_licenses: ["frameworks_base_license"], +} + +java_library_static { + name: "services.flags", + defaults: ["platform_service_defaults"], + srcs: [ + "java/**/*.java", + ], + libs: ["services.core"], +} diff --git a/services/flags/OWNERS b/services/flags/OWNERS new file mode 100644 index 0000000000000..3925b5c13c2d2 --- /dev/null +++ b/services/flags/OWNERS @@ -0,0 +1,6 @@ +# Bug component: 1306523 + +mankoff@google.com + +pixel@google.com +dsandler@android.com diff --git a/services/flags/java/com/android/server/flags/DynamicFlagBinderDelegate.java b/services/flags/java/com/android/server/flags/DynamicFlagBinderDelegate.java new file mode 100644 index 0000000000000..322a52bf01cc9 --- /dev/null +++ b/services/flags/java/com/android/server/flags/DynamicFlagBinderDelegate.java @@ -0,0 +1,273 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server.flags; + +import android.annotation.NonNull; +import android.flags.IFeatureFlagsCallback; +import android.flags.SyncableFlag; +import android.os.Build; +import android.os.IBinder; +import android.os.RemoteException; +import android.provider.DeviceConfig; +import android.util.Slog; + +import com.android.internal.os.BackgroundThread; + +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import java.util.function.Function; + +/** + * Handles DynamicFlags for {@link FeatureFlagsBinder}. + * + * Dynamic flags are simultaneously simpler and more complicated than process stable flags. We can + * return whatever value is last known for a flag is, without too much worry about the flags + * changing (they are dynamic after all). However, we have to alert all the relevant clients + * about those flag changes, and need to be able to restore to a default value if the flag gets + * reset/erased during runtime. + */ +class DynamicFlagBinderDelegate { + + private final FlagOverrideStore mFlagStore; + private final FlagCache mDynamicFlags = new FlagCache<>(); + private final Map> mCallbacks = new HashMap<>(); + private static final Function> NEW_CALLBACK_SET = + k -> new HashSet<>(); + + private final DeviceConfig.OnPropertiesChangedListener mDeviceConfigListener = + new DeviceConfig.OnPropertiesChangedListener() { + @Override + public void onPropertiesChanged(@NonNull DeviceConfig.Properties properties) { + String ns = properties.getNamespace(); + for (String name : properties.getKeyset()) { + // Don't alert for flags we don't care about. + // Don't alert for flags that have been overridden locally. + if (!mDynamicFlags.contains(ns, name) || mFlagStore.contains(ns, name)) { + continue; + } + mFlagChangeCallback.onFlagChanged( + ns, name, properties.getString(name, null)); + } + } + }; + + private final FlagOverrideStore.FlagChangeCallback mFlagChangeCallback = + (namespace, name, value) -> { + // Don't bother with callbacks for non-dynamic flags. + if (!mDynamicFlags.contains(namespace, name)) { + return; + } + + // Don't bother with callbacks if nothing changed. + // Handling erasure (null) is special, as we may be restoring back to a value + // we were already at. + DynamicFlagData data = mDynamicFlags.getOrNull(namespace, name); + if (data == null) { + return; // shouldn't happen, but better safe than sorry. + } + if (value == null) { + if (data.getValue().equals(data.getDefaultValue())) { + return; + } + value = data.getDefaultValue(); + } else if (data.getValue().equals(value)) { + return; + } + data.setValue(value); + + final Set cbCopy; + synchronized (mCallbacks) { + cbCopy = new HashSet<>(); + + for (Integer pid : mCallbacks.keySet()) { + if (data.containsPid(pid)) { + cbCopy.addAll(mCallbacks.get(pid)); + } + } + } + SyncableFlag sFlag = new SyncableFlag(namespace, name, value, true); + cbCopy.forEach(cb -> { + try { + cb.onFlagChange(sFlag); + } catch (RemoteException e) { + Slog.w( + FeatureFlagsService.TAG, + "Failed to communicate flag change to client."); + } + }); + }; + + DynamicFlagBinderDelegate(FlagOverrideStore flagStore) { + mFlagStore = flagStore; + mFlagStore.setChangeCallback(mFlagChangeCallback); + } + + void syncDynamicFlag(int pid, SyncableFlag sf) { + if (!sf.isDynamic()) { + return; + } + + String ns = sf.getNamespace(); + String name = sf.getName(); + + // Dynamic flags don't need any special threading or synchronization considerations. + // We simply give them whatever the current value is. + // However, we do need to keep track of dynamic flags, so that we can alert + // about changes coming in from adb, DeviceConfig, or other sources. + // And also so that we can keep flags relatively consistent across processes. + + // If we already have a value cached, just use that. + String value = null; + DynamicFlagData data = mDynamicFlags.getOrNull(ns, name); + if (data != null) { + value = data.getValue(); + } else { + // Put the value in the cache for future reference. + data = new DynamicFlagData(ns, name); + mDynamicFlags.setIfChanged(ns, name, data); + } + // If we're not in a release build, flags can be overridden locally on device. + if (!Build.IS_USER && value == null) { + value = mFlagStore.get(ns, name); + } + // If we still don't have a value, maybe DeviceConfig does? + // Fallback to sf.getValue() here as well. + if (value == null) { + value = DeviceConfig.getString(ns, name, sf.getValue()); + } + // DeviceConfig listeners are per-namespace. + if (!mDynamicFlags.containsNamespace(ns)) { + DeviceConfig.addOnPropertiesChangedListener( + ns, BackgroundThread.getExecutor(), mDeviceConfigListener); + } + data.addClientPid(pid); + data.setValue(value); + // Store the default value so that if an override gets erased, we can restore + // to something. + data.setDefaultValue(sf.getValue()); + + sf.setValue(value); + } + + + void registerCallback(int pid, IFeatureFlagsCallback callback) { + // Always add callback so that we don't end up with a possible race/leak. + // We remove the callback directly if we fail to call #linkToDeath. + // If we tried to add the callback after we linked, then we could end up in a + // scenario where we link, then the binder dies, firing our BinderGriever which tries + // to remove the callback (which has not yet been added), then finally we add the + // callback, creating a leak. + Set callbacks; + synchronized (mCallbacks) { + callbacks = mCallbacks.computeIfAbsent(pid, NEW_CALLBACK_SET); + callbacks.add(callback); + } + try { + callback.asBinder().linkToDeath(new BinderGriever(pid), 0); + } catch (RemoteException e) { + Slog.e( + FeatureFlagsService.TAG, + "Failed to link to binder death. Callback not registered."); + synchronized (mCallbacks) { + callbacks.remove(callback); + } + } + } + + void unregisterCallback(int pid, IFeatureFlagsCallback callback) { + // No need to unlink, since the BinderGriever will essentially be a no-op. + // We would have to track our BinderGriever's in a map otherwise. + synchronized (mCallbacks) { + Set callbacks = + mCallbacks.computeIfAbsent(pid, NEW_CALLBACK_SET); + callbacks.remove(callback); + } + } + + private static class DynamicFlagData { + private final String mNamespace; + private final String mName; + private final Set mPids = new HashSet<>(); + private String mValue; + private String mDefaultValue; + + private DynamicFlagData(String namespace, String name) { + mNamespace = namespace; + mName = name; + } + + String getValue() { + return mValue; + } + + void setValue(String value) { + mValue = value; + } + + String getDefaultValue() { + return mDefaultValue; + } + + void setDefaultValue(String value) { + mDefaultValue = value; + } + + void addClientPid(int pid) { + mPids.add(pid); + } + + boolean containsPid(int pid) { + return mPids.contains(pid); + } + + @Override + public boolean equals(Object other) { + if (other == null || !(other instanceof DynamicFlagData)) { + return false; + } + + DynamicFlagData o = (DynamicFlagData) other; + + return mName.equals(o.mName) && mNamespace.equals(o.mNamespace) + && mValue.equals(o.mValue) && mDefaultValue.equals(o.mDefaultValue); + } + + @Override + public int hashCode() { + return mName.hashCode() + mNamespace.hashCode() + + mValue.hashCode() + mDefaultValue.hashCode(); + } + } + + + private class BinderGriever implements IBinder.DeathRecipient { + private final int mPid; + + private BinderGriever(int pid) { + mPid = pid; + } + + @Override + public void binderDied() { + synchronized (mCallbacks) { + mCallbacks.remove(mPid); + } + } + } +} diff --git a/services/flags/java/com/android/server/flags/FeatureFlagsBinder.java b/services/flags/java/com/android/server/flags/FeatureFlagsBinder.java new file mode 100644 index 0000000000000..dc97fde758dcc --- /dev/null +++ b/services/flags/java/com/android/server/flags/FeatureFlagsBinder.java @@ -0,0 +1,86 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server.flags; + +import android.annotation.NonNull; +import android.annotation.SystemApi; +import android.flags.IFeatureFlags; +import android.flags.IFeatureFlagsCallback; +import android.flags.SyncableFlag; +import android.os.Build; +import android.os.ParcelFileDescriptor; + +import java.io.FileOutputStream; +import java.util.List; + +class FeatureFlagsBinder extends IFeatureFlags.Stub { + private final FlagOverrideStore mFlagStore; + private final FlagsShellCommand mShellCommand; + private final FlagCache mFlagCache = new FlagCache<>(); + private final DynamicFlagBinderDelegate mDynamicFlagDelegate; + + FeatureFlagsBinder(FlagOverrideStore flagStore, FlagsShellCommand shellCommand) { + mFlagStore = flagStore; + mShellCommand = shellCommand; + mDynamicFlagDelegate = new DynamicFlagBinderDelegate(flagStore); + } + + @Override + public void registerCallback(IFeatureFlagsCallback callback) { + mDynamicFlagDelegate.registerCallback(getCallingPid(), callback); + } + + @Override + public void unregisterCallback(IFeatureFlagsCallback callback) { + mDynamicFlagDelegate.unregisterCallback(getCallingPid(), callback); + } + + @Override + public List syncFlags(List incomingFlags) { + int pid = getCallingPid(); + for (SyncableFlag sf : incomingFlags) { + String ns = sf.getNamespace(); + String name = sf.getName(); + if (sf.isDynamic()) { + mDynamicFlagDelegate.syncDynamicFlag(pid, sf); + } else { + synchronized (mFlagCache) { + String value = mFlagCache.getOrNull(ns, name); + if (value == null) { + String overrideValue = Build.IS_USER ? null : mFlagStore.get(ns, name); + value = overrideValue != null ? overrideValue : sf.getValue(); + mFlagCache.setIfChanged(ns, name, value); + } + sf.setValue(value); + } + } + } + return incomingFlags; + } + + @SystemApi + public int handleShellCommand( + @NonNull ParcelFileDescriptor in, + @NonNull ParcelFileDescriptor out, + @NonNull ParcelFileDescriptor err, + @NonNull String[] args) { + FileOutputStream fout = new FileOutputStream(out.getFileDescriptor()); + FileOutputStream ferr = new FileOutputStream(err.getFileDescriptor()); + + return mShellCommand.process(args, fout, ferr); + } +} diff --git a/services/flags/java/com/android/server/flags/FeatureFlagsService.java b/services/flags/java/com/android/server/flags/FeatureFlagsService.java new file mode 100644 index 0000000000000..111fad0ce3b0a --- /dev/null +++ b/services/flags/java/com/android/server/flags/FeatureFlagsService.java @@ -0,0 +1,59 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.android.server.flags; + +import android.content.Context; +import android.util.Slog; + +import com.android.server.SystemService; + +/** + * A service that manages syncing {@link android.flags.FeatureFlags} across processes. + * + * This service holds flags stable for at least the lifetime of a process, meaning that if + * a process comes online with a flag set to true, any other process that connects here and + * tries to read the same flag will also receive the flag as true. The flag will remain stable + * until either all of the interested processes have died, or the device restarts. + * + * TODO(279054964): Add to dumpsys + * @hide + */ +public class FeatureFlagsService extends SystemService { + + static final String TAG = "FeatureFlagsService"; + private final FlagOverrideStore mFlagStore; + private final FlagsShellCommand mShellCommand; + + /** + * Initializes the system service. + * + * @param context The system server context. + */ + public FeatureFlagsService(Context context) { + super(context); + mFlagStore = new FlagOverrideStore( + new GlobalSettingsProxy(context.getContentResolver())); + mShellCommand = new FlagsShellCommand(mFlagStore); + } + + @Override + public void onStart() { + Slog.d(TAG, "Started Feature Flag Service"); + publishBinderService( + Context.FEATURE_FLAGS_SERVICE, new FeatureFlagsBinder(mFlagStore, mShellCommand)); + } + +} diff --git a/services/flags/java/com/android/server/flags/FlagCache.java b/services/flags/java/com/android/server/flags/FlagCache.java new file mode 100644 index 0000000000000..cee1578a5ddec --- /dev/null +++ b/services/flags/java/com/android/server/flags/FlagCache.java @@ -0,0 +1,103 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server.flags; + +import java.util.HashMap; +import java.util.Map; +import java.util.function.Function; + +/** + * Threadsafe cache of values that stores the supplied default on cache miss. + * + * @param The type of value to store. + */ +public class FlagCache { + private final Function> mNewHashMap = k -> new HashMap<>(); + + // Cache is organized first by namespace, then by name. All values are stored as strings. + final Map> mCache = new HashMap<>(); + + FlagCache() { + } + + /** + * Returns true if the namespace exists in the cache already. + */ + boolean containsNamespace(String namespace) { + synchronized (mCache) { + return mCache.containsKey(namespace); + } + } + + /** + * Returns true if the value is stored in the cache. + */ + boolean contains(String namespace, String name) { + synchronized (mCache) { + Map nsCache = mCache.get(namespace); + return nsCache != null && nsCache.containsKey(name); + } + } + + /** + * Sets the value if it is different from what is currently stored. + * + * If the value is not set, or the current value is null, it will store the value and + * return true. + * + * @return True if the value was set. False if the value is the same. + */ + boolean setIfChanged(String namespace, String name, V value) { + synchronized (mCache) { + Map nsCache = mCache.computeIfAbsent(namespace, mNewHashMap); + V curValue = nsCache.get(name); + if (curValue == null || !curValue.equals(value)) { + nsCache.put(name, value); + return true; + } + return false; + } + } + + /** + * Gets the current value from the cache, setting it if it is currently absent. + * + * @return The value that is now in the cache after the call to the method. + */ + V getOrSet(String namespace, String name, V defaultValue) { + synchronized (mCache) { + Map nsCache = mCache.computeIfAbsent(namespace, mNewHashMap); + V value = nsCache.putIfAbsent(name, defaultValue); + return value == null ? defaultValue : value; + } + } + + /** + * Gets the current value from the cache, returning null if not present. + * + * @return The value that is now in the cache if there is one. + */ + V getOrNull(String namespace, String name) { + synchronized (mCache) { + Map nsCache = mCache.get(namespace); + if (nsCache == null) { + return null; + } + return nsCache.get(name); + } + } +} diff --git a/services/flags/java/com/android/server/flags/FlagOverrideStore.java b/services/flags/java/com/android/server/flags/FlagOverrideStore.java new file mode 100644 index 0000000000000..9866b1c7bb1cd --- /dev/null +++ b/services/flags/java/com/android/server/flags/FlagOverrideStore.java @@ -0,0 +1,120 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server.flags; + +import android.database.Cursor; +import android.provider.Settings; + +import com.android.internal.annotations.VisibleForTesting; + +import java.util.HashMap; +import java.util.Map; + +/** + * Persistent storage for the {@link FeatureFlagsService}. + * + * The implementation stores data in Settings. (generally {@link Settings.Global} + * is expected). + */ +@VisibleForTesting(visibility = VisibleForTesting.Visibility.PACKAGE) +public class FlagOverrideStore { + private static final String KEYNAME_PREFIX = "flag|"; + private static final String NAMESPACE_NAME_SEPARATOR = "."; + + private final SettingsProxy mSettingsProxy; + + private FlagChangeCallback mCallback; + + FlagOverrideStore(SettingsProxy settingsProxy) { + mSettingsProxy = settingsProxy; + } + + void setChangeCallback(FlagChangeCallback callback) { + mCallback = callback; + } + + /** Returns true if a non-null value is in the store. */ + boolean contains(String namespace, String name) { + return get(namespace, name) != null; + } + + /** Put a value in the store. */ + void set(String namespace, String name, String value) { + mSettingsProxy.putString(getPropName(namespace, name), value); + mCallback.onFlagChanged(namespace, name, value); + } + + /** Read a value out of the store. */ + @VisibleForTesting + public String get(String namespace, String name) { + return mSettingsProxy.getString(getPropName(namespace, name)); + } + + /** Erase a value from the store. */ + void erase(String namespace, String name) { + set(namespace, name, null); + } + + Map> getFlags() { + return getFlagsForNamespace(null); + } + + Map> getFlagsForNamespace(String namespace) { + Cursor c = mSettingsProxy.getContentResolver().query( + Settings.Global.CONTENT_URI, + new String[]{Settings.NameValueTable.NAME, Settings.NameValueTable.VALUE}, + null, // Doesn't support a "LIKE" query + null, + null + ); + + if (c == null) { + return Map.of(); + } + int keynamePrefixLength = KEYNAME_PREFIX.length(); + Map> results = new HashMap<>(); + while (c.moveToNext()) { + String key = c.getString(0); + if (!key.startsWith(KEYNAME_PREFIX) + || key.indexOf(NAMESPACE_NAME_SEPARATOR, keynamePrefixLength) < 0) { + continue; + } + String value = c.getString(1); + if (value == null || value.isEmpty()) { + continue; + } + String ns = key.substring(keynamePrefixLength, key.indexOf(NAMESPACE_NAME_SEPARATOR)); + if (namespace != null && !namespace.equals(ns)) { + continue; + } + String name = key.substring(key.indexOf(NAMESPACE_NAME_SEPARATOR) + 1); + results.putIfAbsent(ns, new HashMap<>()); + results.get(ns).put(name, value); + } + c.close(); + return results; + } + + @VisibleForTesting(visibility = VisibleForTesting.Visibility.PRIVATE) + static String getPropName(String namespace, String name) { + return KEYNAME_PREFIX + namespace + NAMESPACE_NAME_SEPARATOR + name; + } + + interface FlagChangeCallback { + void onFlagChanged(String namespace, String name, String value); + } +} diff --git a/services/flags/java/com/android/server/flags/FlagsShellCommand.java b/services/flags/java/com/android/server/flags/FlagsShellCommand.java new file mode 100644 index 0000000000000..b7896ee187145 --- /dev/null +++ b/services/flags/java/com/android/server/flags/FlagsShellCommand.java @@ -0,0 +1,214 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server.flags; + +import com.android.internal.annotations.VisibleForTesting; +import com.android.internal.util.FastPrintWriter; + +import java.io.OutputStream; +import java.io.PrintWriter; +import java.util.Locale; +import java.util.Map; + +/** + * Process command line input for the flags service. + */ +@VisibleForTesting(visibility = VisibleForTesting.Visibility.PACKAGE) +public class FlagsShellCommand { + private final FlagOverrideStore mFlagStore; + + FlagsShellCommand(FlagOverrideStore flagStore) { + mFlagStore = flagStore; + } + + /** + * Interpret the command supplied in the constructor. + * + * @return Zero on success or non-zero on error. + */ + public int process( + String[] args, + OutputStream out, + OutputStream err) { + PrintWriter outPw = new FastPrintWriter(out); + PrintWriter errPw = new FastPrintWriter(err); + + if (args.length == 0) { + return printHelp(outPw); + } + switch (args[0].toLowerCase(Locale.ROOT)) { + case "help": + return printHelp(outPw); + case "list": + return listCmd(args, outPw, errPw); + case "set": + return setCmd(args, outPw, errPw); + case "get": + return getCmd(args, outPw, errPw); + case "erase": + return eraseCmd(args, outPw, errPw); + default: + return unknownCmd(outPw); + } + } + + private int printHelp(PrintWriter outPw) { + outPw.println("Feature Flags command, allowing listing, setting, getting, and erasing of"); + outPw.println("local flag overrides on a device."); + outPw.println(); + outPw.println("Commands:"); + outPw.println(" list [namespace]"); + outPw.println(" List all flag overrides. Namespace is optional."); + outPw.println(); + outPw.println(" get "); + outPw.println(" Return the string value of a specific flag, or "); + outPw.println(); + outPw.println(" set "); + outPw.println(" Set a specific flag"); + outPw.println(); + outPw.println(" erase "); + outPw.println(" Unset a specific flag"); + outPw.flush(); + return 0; + } + + private int listCmd(String[] args, PrintWriter outPw, PrintWriter errPw) { + if (!validateNumArguments(args, 0, 1, args[0], errPw)) { + errPw.println("Expected `" + args[0] + " [namespace]`"); + errPw.flush(); + return -1; + } + Map> overrides; + if (args.length == 2) { + overrides = mFlagStore.getFlagsForNamespace(args[1]); + } else { + overrides = mFlagStore.getFlags(); + } + if (overrides.isEmpty()) { + outPw.println("No overrides set"); + } else { + int longestNamespaceLen = "namespace".length(); + int longestFlagLen = "flag".length(); + int longestValLen = "value".length(); + for (Map.Entry> namespace : overrides.entrySet()) { + longestNamespaceLen = Math.max(longestNamespaceLen, namespace.getKey().length()); + for (Map.Entry flag : namespace.getValue().entrySet()) { + longestFlagLen = Math.max(longestFlagLen, flag.getKey().length()); + longestValLen = Math.max(longestValLen, flag.getValue().length()); + } + } + outPw.print(String.format("%-" + longestNamespaceLen + "s", "namespace")); + outPw.print(' '); + outPw.print(String.format("%-" + longestFlagLen + "s", "flag")); + outPw.print(' '); + outPw.println("value"); + for (int i = 0; i < longestNamespaceLen; i++) { + outPw.print('='); + } + outPw.print(' '); + for (int i = 0; i < longestFlagLen; i++) { + outPw.print('='); + } + outPw.print(' '); + for (int i = 0; i < longestValLen; i++) { + outPw.print('='); + } + outPw.println(); + for (Map.Entry> namespace : overrides.entrySet()) { + for (Map.Entry flag : namespace.getValue().entrySet()) { + outPw.print( + String.format("%-" + longestNamespaceLen + "s", namespace.getKey())); + outPw.print(' '); + outPw.print(String.format("%-" + longestFlagLen + "s", flag.getKey())); + outPw.print(' '); + outPw.println(flag.getValue()); + } + } + } + outPw.flush(); + return 0; + } + + private int setCmd(String[] args, PrintWriter outPw, PrintWriter errPw) { + if (!validateNumArguments(args, 3, args[0], errPw)) { + errPw.println("Expected `" + args[0] + " `"); + errPw.flush(); + return -1; + } + mFlagStore.set(args[1], args[2], args[3]); + outPw.println("Flag " + args[1] + "." + args[2] + " is now " + args[3]); + outPw.flush(); + return 0; + } + + private int getCmd(String[] args, PrintWriter outPw, PrintWriter errPw) { + if (!validateNumArguments(args, 2, args[0], errPw)) { + errPw.println("Expected `" + args[0] + " `"); + errPw.flush(); + return -1; + } + + String value = mFlagStore.get(args[1], args[2]); + outPw.print(args[1] + "." + args[2] + " is "); + if (value == null || value.isEmpty()) { + outPw.println(""); + } else { + outPw.println("\"" + value.translateEscapes() + "\""); + } + outPw.flush(); + return 0; + } + + private int eraseCmd(String[] args, PrintWriter outPw, PrintWriter errPw) { + if (!validateNumArguments(args, 2, args[0], errPw)) { + errPw.println("Expected `" + args[0] + " `"); + errPw.flush(); + return -1; + } + mFlagStore.erase(args[1], args[2]); + outPw.println("Erased " + args[1] + "." + args[2]); + return 0; + } + + private int unknownCmd(PrintWriter outPw) { + outPw.println("This command is unknown."); + printHelp(outPw); + outPw.flush(); + return -1; + } + + private boolean validateNumArguments( + String[] args, int exactly, String cmdName, PrintWriter errPw) { + return validateNumArguments(args, exactly, exactly, cmdName, errPw); + } + + private boolean validateNumArguments( + String[] args, int min, int max, String cmdName, PrintWriter errPw) { + int len = args.length - 1; // Discount the command itself. + if (len < min) { + errPw.println( + "Less than " + min + " arguments provided for \"" + cmdName + "\" command."); + return false; + } else if (len > max) { + errPw.println( + "More than " + max + " arguments provided for \"" + cmdName + "\" command."); + return false; + } + + return true; + } +} diff --git a/services/flags/java/com/android/server/flags/GlobalSettingsProxy.java b/services/flags/java/com/android/server/flags/GlobalSettingsProxy.java new file mode 100644 index 0000000000000..acb7bb5a49db5 --- /dev/null +++ b/services/flags/java/com/android/server/flags/GlobalSettingsProxy.java @@ -0,0 +1,68 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server.flags; + +import android.content.ContentResolver; +import android.net.Uri; +import android.provider.Settings; + +class GlobalSettingsProxy implements SettingsProxy { + private final ContentResolver mContentResolver; + + GlobalSettingsProxy(ContentResolver contentResolver) { + mContentResolver = contentResolver; + } + + @Override + public ContentResolver getContentResolver() { + return mContentResolver; + } + + @Override + public Uri getUriFor(String name) { + return Settings.Global.getUriFor(name); + } + + @Override + public String getStringForUser(String name, int userHandle) { + return Settings.Global.getStringForUser(mContentResolver, name, userHandle); + } + + @Override + public boolean putString(String name, String value, boolean overrideableByRestore) { + throw new UnsupportedOperationException( + "This method only exists publicly for Settings.System and Settings.Secure"); + } + + @Override + public boolean putStringForUser(String name, String value, int userHandle) { + return Settings.Global.putStringForUser(mContentResolver, name, value, userHandle); + } + + @Override + public boolean putStringForUser(String name, String value, String tag, boolean makeDefault, + int userHandle, boolean overrideableByRestore) { + return Settings.Global.putStringForUser( + mContentResolver, name, value, tag, makeDefault, userHandle, + overrideableByRestore); + } + + @Override + public boolean putString(String name, String value, String tag, boolean makeDefault) { + return Settings.Global.putString(mContentResolver, name, value, tag, makeDefault); + } +} diff --git a/services/flags/java/com/android/server/flags/SettingsProxy.java b/services/flags/java/com/android/server/flags/SettingsProxy.java new file mode 100644 index 0000000000000..c6e85d5d1dc8f --- /dev/null +++ b/services/flags/java/com/android/server/flags/SettingsProxy.java @@ -0,0 +1,381 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server.flags; + +import android.annotation.NonNull; +import android.annotation.Nullable; +import android.annotation.UserIdInt; +import android.content.ContentResolver; +import android.database.ContentObserver; +import android.net.Uri; +import android.provider.Settings; + +/** + * Wrapper class meant to enable hermetic testing of {@link Settings}. + * + * Implementations of this class are expected to be constructed with a {@link ContentResolver} or, + * otherwise have access to an implicit one. All the proxy methods in this class exclude + * {@link ContentResolver} from their signature and rely on an internally defined one instead. + * + * Most methods in the {@link Settings} classes have default implementations defined. + * Implementations of this interfac need only concern themselves with getting and putting Strings. + * They should also override any methods for a class they are proxying that _are not_ defined, and + * throw an appropriate {@link UnsupportedOperationException}. For instance, {@link Settings.Global} + * does not define {@link #putString(String, String, boolean)}, so an implementation of this + * interface that proxies through to it should throw an exception when that method is called. + * + * This class adds in the following helpers as well: + * - {@link #getBool(String)} + * - {@link #putBool(String, boolean)} + * - {@link #registerContentObserver(Uri, ContentObserver)} + * + * ... and similar variations for all of those. + */ +public interface SettingsProxy { + + /** + * Returns the {@link ContentResolver} this instance uses. + */ + ContentResolver getContentResolver(); + + /** + * Construct the content URI for a particular name/value pair, + * useful for monitoring changes with a ContentObserver. + * @param name to look up in the table + * @return the corresponding content URI, or null if not present + */ + Uri getUriFor(String name); + + /**See {@link Settings.Secure#getString(ContentResolver, String)} */ + String getStringForUser(String name, int userHandle); + + /**See {@link Settings.Secure#putString(ContentResolver, String, String, boolean)} */ + boolean putString(String name, String value, boolean overrideableByRestore); + + /** See {@link Settings.Secure#putStringForUser(ContentResolver, String, String, int)} */ + boolean putStringForUser(String name, String value, int userHandle); + + /** + * See {@link Settings.Secure#putStringForUser(ContentResolver, String, String, String, boolean, + * int, boolean)} + */ + boolean putStringForUser(@NonNull String name, @Nullable String value, @Nullable String tag, + boolean makeDefault, @UserIdInt int userHandle, boolean overrideableByRestore); + + /** See {@link Settings.Secure#putString(ContentResolver, String, String, String, boolean)} */ + boolean putString(@NonNull String name, @Nullable String value, @Nullable String tag, + boolean makeDefault); + + /** + * Returns the user id for the associated {@link ContentResolver}. + */ + default int getUserId() { + return getContentResolver().getUserId(); + } + + /** See {@link Settings.Secure#getString(ContentResolver, String)} */ + default String getString(String name) { + return getStringForUser(name, getUserId()); + } + + /** See {@link Settings.Secure#putString(ContentResolver, String, String)} */ + default boolean putString(String name, String value) { + return putStringForUser(name, value, getUserId()); + } + /** See {@link Settings.Secure#getIntForUser(ContentResolver, String, int, int)} */ + default int getIntForUser(String name, int def, int userHandle) { + String v = getStringForUser(name, userHandle); + try { + return v != null ? Integer.parseInt(v) : def; + } catch (NumberFormatException e) { + return def; + } + } + + /** See {@link Settings.Secure#getInt(ContentResolver, String)} */ + default int getInt(String name) throws Settings.SettingNotFoundException { + return getIntForUser(name, getUserId()); + } + + /** See {@link Settings.Secure#getIntForUser(ContentResolver, String, int)} */ + default int getIntForUser(String name, int userHandle) + throws Settings.SettingNotFoundException { + String v = getStringForUser(name, userHandle); + try { + return Integer.parseInt(v); + } catch (NumberFormatException e) { + throw new Settings.SettingNotFoundException(name); + } + } + + /** See {@link Settings.Secure#putInt(ContentResolver, String, int)} */ + default boolean putInt(String name, int value) { + return putIntForUser(name, value, getUserId()); + } + + /** See {@link Settings.Secure#putIntForUser(ContentResolver, String, int, int)} */ + default boolean putIntForUser(String name, int value, int userHandle) { + return putStringForUser(name, Integer.toString(value), userHandle); + } + + /** + * Convenience function for retrieving a single settings value + * as a boolean. Note that internally setting values are always + * stored as strings; this function converts the string to a boolean + * for you. The default value will be returned if the setting is + * not defined or not a boolean. + * + * @param name The name of the setting to retrieve. + * @param def Value to return if the setting is not defined. + * + * @return The setting's current value, or 'def' if it is not defined + * or not a valid boolean. + */ + default boolean getBool(String name, boolean def) { + return getBoolForUser(name, def, getUserId()); + } + + /** See {@link #getBool(String, boolean)}. */ + default boolean getBoolForUser(String name, boolean def, int userHandle) { + return getIntForUser(name, def ? 1 : 0, userHandle) != 0; + } + + /** + * Convenience function for retrieving a single settings value + * as a boolean. Note that internally setting values are always + * stored as strings; this function converts the string to a boolean + * for you. + *

+ * This version does not take a default value. If the setting has not + * been set, or the string value is not a number, + * it throws {@link Settings.SettingNotFoundException}. + * + * @param name The name of the setting to retrieve. + * + * @throws Settings.SettingNotFoundException Thrown if a setting by the given + * name can't be found or the setting value is not a boolean. + * + * @return The setting's current value. + */ + default boolean getBool(String name) throws Settings.SettingNotFoundException { + return getBoolForUser(name, getUserId()); + } + + /** See {@link #getBool(String)}. */ + default boolean getBoolForUser(String name, int userHandle) + throws Settings.SettingNotFoundException { + return getIntForUser(name, userHandle) != 0; + } + + /** + * Convenience function for updating a single settings value as a + * boolean. This will either create a new entry in the table if the + * given name does not exist, or modify the value of the existing row + * with that name. Note that internally setting values are always + * stored as strings, so this function converts the given value to a + * string before storing it. + * + * @param name The name of the setting to modify. + * @param value The new value for the setting. + * @return true if the value was set, false on database errors + */ + default boolean putBool(String name, boolean value) { + return putBoolForUser(name, value, getUserId()); + } + + /** See {@link #putBool(String, boolean)}. */ + default boolean putBoolForUser(String name, boolean value, int userHandle) { + return putIntForUser(name, value ? 1 : 0, userHandle); + } + + /** See {@link Settings.Secure#getLong(ContentResolver, String, long)} */ + default long getLong(String name, long def) { + return getLongForUser(name, def, getUserId()); + } + + /** See {@link Settings.Secure#getLongForUser(ContentResolver, String, long, int)} */ + default long getLongForUser(String name, long def, int userHandle) { + String valString = getStringForUser(name, userHandle); + long value; + try { + value = valString != null ? Long.parseLong(valString) : def; + } catch (NumberFormatException e) { + value = def; + } + return value; + } + + /** See {@link Settings.Secure#getLong(ContentResolver, String)} */ + default long getLong(String name) throws Settings.SettingNotFoundException { + return getLongForUser(name, getUserId()); + } + + /** See {@link Settings.Secure#getLongForUser(ContentResolver, String, int)} */ + default long getLongForUser(String name, int userHandle) + throws Settings.SettingNotFoundException { + String valString = getStringForUser(name, userHandle); + try { + return Long.parseLong(valString); + } catch (NumberFormatException e) { + throw new Settings.SettingNotFoundException(name); + } + } + + /** See {@link Settings.Secure#putLong(ContentResolver, String, long)} */ + default boolean putLong(String name, long value) { + return putLongForUser(name, value, getUserId()); + } + + /** See {@link Settings.Secure#putLongForUser(ContentResolver, String, long, int)} */ + default boolean putLongForUser(String name, long value, int userHandle) { + return putStringForUser(name, Long.toString(value), userHandle); + } + + /** See {@link Settings.Secure#getFloat(ContentResolver, String, float)} */ + default float getFloat(String name, float def) { + return getFloatForUser(name, def, getUserId()); + } + + /** See {@link Settings.Secure#getFloatForUser(ContentResolver, String, int)} */ + default float getFloatForUser(String name, float def, int userHandle) { + String v = getStringForUser(name, userHandle); + try { + return v != null ? Float.parseFloat(v) : def; + } catch (NumberFormatException e) { + return def; + } + } + + + /** See {@link Settings.Secure#getFloat(ContentResolver, String)} */ + default float getFloat(String name) throws Settings.SettingNotFoundException { + return getFloatForUser(name, getUserId()); + } + + /** See {@link Settings.Secure#getFloatForUser(ContentResolver, String, int)} */ + default float getFloatForUser(String name, int userHandle) + throws Settings.SettingNotFoundException { + String v = getStringForUser(name, userHandle); + if (v == null) { + throw new Settings.SettingNotFoundException(name); + } + try { + return Float.parseFloat(v); + } catch (NumberFormatException e) { + throw new Settings.SettingNotFoundException(name); + } + } + + /** See {@link Settings.Secure#putFloat(ContentResolver, String, float)} */ + default boolean putFloat(String name, float value) { + return putFloatForUser(name, value, getUserId()); + } + + /** See {@link Settings.Secure#putFloatForUser(ContentResolver, String, float, int)} */ + default boolean putFloatForUser(String name, float value, int userHandle) { + return putStringForUser(name, Float.toString(value), userHandle); + } + + /** + * Convenience wrapper around + * {@link ContentResolver#registerContentObserver(Uri, boolean, ContentObserver)}.' + * + * Implicitly calls {@link #getUriFor(String)} on the passed in name. + */ + default void registerContentObserver(String name, ContentObserver settingsObserver) { + registerContentObserver(getUriFor(name), settingsObserver); + } + + /** + * Convenience wrapper around + * {@link ContentResolver#registerContentObserver(Uri, boolean, ContentObserver)}.' + */ + default void registerContentObserver(Uri uri, ContentObserver settingsObserver) { + registerContentObserverForUser(uri, settingsObserver, getUserId()); + } + + /** + * Convenience wrapper around + * {@link ContentResolver#registerContentObserver(Uri, boolean, ContentObserver)}. + * + * Implicitly calls {@link #getUriFor(String)} on the passed in name. + */ + default void registerContentObserver(String name, boolean notifyForDescendants, + ContentObserver settingsObserver) { + registerContentObserver(getUriFor(name), notifyForDescendants, settingsObserver); + } + + /** + * Convenience wrapper around + * {@link ContentResolver#registerContentObserver(Uri, boolean, ContentObserver)}.' + */ + default void registerContentObserver(Uri uri, boolean notifyForDescendants, + ContentObserver settingsObserver) { + registerContentObserverForUser(uri, notifyForDescendants, settingsObserver, getUserId()); + } + + /** + * Convenience wrapper around + * {@link ContentResolver#registerContentObserver(Uri, boolean, ContentObserver, int)} + * + * Implicitly calls {@link #getUriFor(String)} on the passed in name. + */ + default void registerContentObserverForUser( + String name, ContentObserver settingsObserver, int userHandle) { + registerContentObserverForUser( + getUriFor(name), settingsObserver, userHandle); + } + + /** + * Convenience wrapper around + * {@link ContentResolver#registerContentObserver(Uri, boolean, ContentObserver, int)} + */ + default void registerContentObserverForUser( + Uri uri, ContentObserver settingsObserver, int userHandle) { + registerContentObserverForUser( + uri, false, settingsObserver, userHandle); + } + + /** + * Convenience wrapper around + * {@link ContentResolver#registerContentObserver(Uri, boolean, ContentObserver, int)} + * + * Implicitly calls {@link #getUriFor(String)} on the passed in name. + */ + default void registerContentObserverForUser( + String name, boolean notifyForDescendants, ContentObserver settingsObserver, + int userHandle) { + registerContentObserverForUser( + getUriFor(name), notifyForDescendants, settingsObserver, userHandle); + } + + /** + * Convenience wrapper around + * {@link ContentResolver#registerContentObserver(Uri, boolean, ContentObserver, int)} + */ + default void registerContentObserverForUser( + Uri uri, boolean notifyForDescendants, ContentObserver settingsObserver, + int userHandle) { + getContentResolver().registerContentObserver( + uri, notifyForDescendants, settingsObserver, userHandle); + } + + /** See {@link ContentResolver#unregisterContentObserver(ContentObserver)}. */ + default void unregisterContentObserver(ContentObserver settingsObserver) { + getContentResolver().unregisterContentObserver(settingsObserver); + } +} diff --git a/services/java/com/android/server/SystemServer.java b/services/java/com/android/server/SystemServer.java index 6960da74dd1a3..5f45485d36f4a 100644 --- a/services/java/com/android/server/SystemServer.java +++ b/services/java/com/android/server/SystemServer.java @@ -132,6 +132,7 @@ import com.android.server.display.DisplayManagerService; import com.android.server.display.color.ColorDisplayService; import com.android.server.dreams.DreamManagerService; import com.android.server.emergency.EmergencyAffordanceService; +import com.android.server.flags.FeatureFlagsService; import com.android.server.gpu.GpuService; import com.android.server.grammaticalinflection.GrammaticalInflectionService; import com.android.server.graphics.fonts.FontManagerService; @@ -1115,6 +1116,12 @@ public final class SystemServer implements Dumpable { mSystemServiceManager.startService(DeviceIdentifiersPolicyService.class); t.traceEnd(); + // Starts a service for reading runtime flag overrides, and keeping processes + // in sync with one another. + t.traceBegin("StartFeatureFlagsService"); + mSystemServiceManager.startService(FeatureFlagsService.class); + t.traceEnd(); + // Uri Grants Manager. t.traceBegin("UriGrantsManagerService"); mSystemServiceManager.startService(UriGrantsManagerService.Lifecycle.class); diff --git a/services/tests/servicestests/Android.bp b/services/tests/servicestests/Android.bp index ee3a773580a37..0530f892a48e8 100644 --- a/services/tests/servicestests/Android.bp +++ b/services/tests/servicestests/Android.bp @@ -34,6 +34,7 @@ android_test { "services.core", "services.credentials", "services.devicepolicy", + "services.flags", "services.net", "services.people", "services.usage", diff --git a/services/tests/servicestests/src/com/android/server/flags/FeatureFlagsServiceTest.java b/services/tests/servicestests/src/com/android/server/flags/FeatureFlagsServiceTest.java new file mode 100644 index 0000000000000..8455b88ae9aff --- /dev/null +++ b/services/tests/servicestests/src/com/android/server/flags/FeatureFlagsServiceTest.java @@ -0,0 +1,179 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server.flags; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assertWithMessage; + +import static org.junit.Assert.fail; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.eq; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import android.flags.IFeatureFlagsCallback; +import android.flags.SyncableFlag; +import android.os.IBinder; +import android.os.RemoteException; +import android.platform.test.annotations.Presubmit; + +import androidx.test.filters.SmallTest; + +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnit; +import org.mockito.junit.MockitoRule; + +import java.util.List; + +@Presubmit +@SmallTest +public class FeatureFlagsServiceTest { + private static final String NS = "ns"; + private static final String NAME = "name"; + private static final String PROP_NAME = FlagOverrideStore.getPropName(NS, NAME); + + @Rule + public final MockitoRule mockito = MockitoJUnit.rule(); + + @Mock + private FlagOverrideStore mFlagStore; + @Mock + private FlagsShellCommand mFlagCommand; + @Mock + private IFeatureFlagsCallback mIFeatureFlagsCallback; + @Mock + private IBinder mIFeatureFlagsCallbackAsBinder; + + private FeatureFlagsBinder mFeatureFlagsService; + + @Before + public void setup() { + when(mIFeatureFlagsCallback.asBinder()).thenReturn(mIFeatureFlagsCallbackAsBinder); + mFeatureFlagsService = new FeatureFlagsBinder(mFlagStore, mFlagCommand); + } + + @Test + public void testRegisterCallback() { + mFeatureFlagsService.registerCallback(mIFeatureFlagsCallback); + try { + verify(mIFeatureFlagsCallbackAsBinder).linkToDeath(any(), eq(0)); + } catch (RemoteException e) { + fail("Our mock threw a Remote Exception?"); + } + } + + @Test + public void testSyncFlags_noOverrides() { + List inputFlags = List.of( + new SyncableFlag(NS, "a", "false", false), + new SyncableFlag(NS, "b", "true", false), + new SyncableFlag(NS, "c", "false", false) + ); + + List outputFlags = mFeatureFlagsService.syncFlags(inputFlags); + + assertThat(inputFlags.size()).isEqualTo(outputFlags.size()); + + for (SyncableFlag inpF: inputFlags) { + boolean found = false; + for (SyncableFlag outF : outputFlags) { + if (compareSyncableFlagsNames(inpF, outF)) { + found = true; + break; + } + } + assertWithMessage("Failed to find input flag " + inpF + " in the output") + .that(found).isTrue(); + } + } + + @Test + public void testSyncFlags_withSomeOverrides() { + List inputFlags = List.of( + new SyncableFlag(NS, "a", "false", false), + new SyncableFlag(NS, "b", "true", false), + new SyncableFlag(NS, "c", "false", false) + ); + + assertThat(mFlagStore).isNotNull(); + when(mFlagStore.get(NS, "c")).thenReturn("true"); + List outputFlags = mFeatureFlagsService.syncFlags(inputFlags); + + assertThat(inputFlags.size()).isEqualTo(outputFlags.size()); + + for (SyncableFlag inpF: inputFlags) { + boolean found = false; + for (SyncableFlag outF : outputFlags) { + if (compareSyncableFlagsNames(inpF, outF)) { + found = true; + + // Once we've found "c", do an extra check + if (outF.getName().equals("c")) { + assertWithMessage("Flag " + outF + "was not returned with an override") + .that(outF.getValue()).isEqualTo("true"); + } + break; + } + } + assertWithMessage("Failed to find input flag " + inpF + " in the output") + .that(found).isTrue(); + } + } + + + @Test + public void testSyncFlags_twoCallsWithDifferentDefaults() { + List inputFlagsFirst = List.of( + new SyncableFlag(NS, "a", "false", false) + ); + List inputFlagsSecond = List.of( + new SyncableFlag(NS, "a", "true", false), + new SyncableFlag(NS, "b", "false", false) + ); + + List outputFlagsFirst = mFeatureFlagsService.syncFlags(inputFlagsFirst); + List outputFlagsSecond = mFeatureFlagsService.syncFlags(inputFlagsSecond); + + assertThat(inputFlagsFirst.size()).isEqualTo(outputFlagsFirst.size()); + assertThat(inputFlagsSecond.size()).isEqualTo(outputFlagsSecond.size()); + + // This test only cares that the "a" flag passed in the second time came out with the + // same value that was passed in the first time. + + boolean found = false; + for (SyncableFlag second : outputFlagsSecond) { + if (compareSyncableFlagsNames(second, inputFlagsFirst.get(0))) { + found = true; + assertThat(second.getValue()).isEqualTo(inputFlagsFirst.get(0).getValue()); + break; + } + } + + assertWithMessage( + "Failed to find flag " + inputFlagsFirst.get(0) + " in the second calls output") + .that(found).isTrue(); + } + + private static boolean compareSyncableFlagsNames(SyncableFlag a, SyncableFlag b) { + return a.getNamespace().equals(b.getNamespace()) + && a.getName().equals(b.getName()) + && a.isDynamic() == b.isDynamic(); + } +} diff --git a/services/tests/servicestests/src/com/android/server/flags/FlagCacheTest.java b/services/tests/servicestests/src/com/android/server/flags/FlagCacheTest.java new file mode 100644 index 0000000000000..c2cf540d1d625 --- /dev/null +++ b/services/tests/servicestests/src/com/android/server/flags/FlagCacheTest.java @@ -0,0 +1,86 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server.flags; + +import static com.google.common.truth.Truth.assertThat; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class FlagCacheTest { + private static final String NS = "ns"; + private static final String NAME = "name"; + + FlagCache mFlagCache = new FlagCache(); + + @Test + public void testGetOrNull_unset() { + assertThat(mFlagCache.getOrNull(NS, NAME)).isNull(); + } + + @Test + public void testGetOrSet_unset() { + assertThat(mFlagCache.getOrSet(NS, NAME, "value")).isEqualTo("value"); + } + + @Test + public void testGetOrSet_alreadySet() { + mFlagCache.setIfChanged(NS, NAME, "value"); + assertThat(mFlagCache.getOrSet(NS, NAME, "newvalue")).isEqualTo("value"); + } + + @Test + public void testSetIfChanged_unset() { + assertThat(mFlagCache.setIfChanged(NS, NAME, "value")).isTrue(); + } + + @Test + public void testSetIfChanged_noChange() { + mFlagCache.setIfChanged(NS, NAME, "value"); + assertThat(mFlagCache.setIfChanged(NS, NAME, "value")).isFalse(); + } + + @Test + public void testSetIfChanged_changing() { + mFlagCache.setIfChanged(NS, NAME, "value"); + assertThat(mFlagCache.setIfChanged(NS, NAME, "newvalue")).isTrue(); + } + + @Test + public void testContainsNamespace_unset() { + assertThat(mFlagCache.containsNamespace(NS)).isFalse(); + } + + @Test + public void testContainsNamespace_set() { + mFlagCache.setIfChanged(NS, NAME, "value"); + assertThat(mFlagCache.containsNamespace(NS)).isTrue(); + } + + @Test + public void testContains_unset() { + assertThat(mFlagCache.contains(NS, NAME)).isFalse(); + } + + @Test + public void testContains_set() { + mFlagCache.setIfChanged(NS, NAME, "value"); + assertThat(mFlagCache.contains(NS, NAME)).isTrue(); + } +} diff --git a/services/tests/servicestests/src/com/android/server/flags/FlagOverrideStoreTest.java b/services/tests/servicestests/src/com/android/server/flags/FlagOverrideStoreTest.java new file mode 100644 index 0000000000000..6cc3acfb6125a --- /dev/null +++ b/services/tests/servicestests/src/com/android/server/flags/FlagOverrideStoreTest.java @@ -0,0 +1,111 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server.flags; + +import static com.google.common.truth.Truth.assertThat; + +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import android.platform.test.annotations.Presubmit; + +import androidx.test.filters.SmallTest; + +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnit; +import org.mockito.junit.MockitoRule; + +@Presubmit +@SmallTest +public class FlagOverrideStoreTest { + private static final String NS = "ns"; + private static final String NAME = "name"; + private static final String PROP_NAME = FlagOverrideStore.getPropName(NS, NAME); + + @Rule + public final MockitoRule mockito = MockitoJUnit.rule(); + + @Mock + private SettingsProxy mSettingsProxy; + @Mock + private FlagOverrideStore.FlagChangeCallback mCallback; + + private FlagOverrideStore mFlagStore; + + @Before + public void setup() { + mFlagStore = new FlagOverrideStore(mSettingsProxy); + mFlagStore.setChangeCallback(mCallback); + } + + @Test + public void testSet_unset() { + mFlagStore.set(NS, NAME, "value"); + verify(mSettingsProxy).putString(PROP_NAME, "value"); + } + + @Test + public void testSet_setTwice() { + mFlagStore.set(NS, NAME, "value"); + mFlagStore.set(NS, NAME, "newvalue"); + verify(mSettingsProxy).putString(PROP_NAME, "value"); + verify(mSettingsProxy).putString(PROP_NAME, "newvalue"); + } + + @Test + public void testGet_unset() { + assertThat(mFlagStore.get(NS, NAME)).isNull(); + } + + @Test + public void testGet_set() { + when(mSettingsProxy.getString(PROP_NAME)).thenReturn("value"); + assertThat(mFlagStore.get(NS, NAME)).isEqualTo("value"); + } + + @Test + public void testErase() { + mFlagStore.erase(NS, NAME); + verify(mSettingsProxy).putString(PROP_NAME, null); + } + + @Test + public void testContains_unset() { + assertThat(mFlagStore.contains(NS, NAME)).isFalse(); + } + + @Test + public void testContains_set() { + when(mSettingsProxy.getString(PROP_NAME)).thenReturn("value"); + assertThat(mFlagStore.contains(NS, NAME)).isTrue(); + } + + @Test + public void testCallback_onSet() { + mFlagStore.set(NS, NAME, "value"); + verify(mCallback).onFlagChanged(NS, NAME, "value"); + } + + @Test + public void testCallback_onErase() { + mFlagStore.erase(NS, NAME); + verify(mCallback).onFlagChanged(NS, NAME, null); + } +} diff --git a/services/tests/servicestests/src/com/android/server/flags/OWNERS b/services/tests/servicestests/src/com/android/server/flags/OWNERS new file mode 100644 index 0000000000000..7ed369e371060 --- /dev/null +++ b/services/tests/servicestests/src/com/android/server/flags/OWNERS @@ -0,0 +1 @@ +include /services/flags/OWNERS From 7f18f9dc15b822e5caa8e1b3d4c91f84d21c8e90 Mon Sep 17 00:00:00 2001 From: Dave Mankoff Date: Wed, 12 Jul 2023 19:32:11 +0000 Subject: [PATCH 3/6] Revert^2 "Connect FeatureFlags with FeatureFlagsService." 0f40cb16a7bd899545a398645856a2fe26022f66 Bug: 279054964 Change-Id: I3656b393f5b079dd5f9e59cb1926b33d8b95086f --- core/java/android/flags/BooleanFlag.java | 25 +- core/java/android/flags/BooleanFlagBase.java | 54 ++++ .../android/flags/DynamicBooleanFlag.java | 22 +- core/java/android/flags/FeatureFlags.java | 279 ++++++++++++++++-- core/java/android/flags/FeatureFlagsFake.java | 78 ++++- core/java/android/flags/FusedOffFlag.java | 31 +- core/java/android/flags/FusedOnFlag.java | 31 +- core/java/android/flags/SyncableFlag.java | 9 +- .../src/android/flags/FeatureFlagsTest.java | 131 +++++++- .../src/android/flags/IFeatureFlagsFake.java | 63 ++++ .../flags/DynamicFlagBinderDelegate.java | 6 +- .../server/flags/FeatureFlagsBinder.java | 10 +- 12 files changed, 594 insertions(+), 145 deletions(-) create mode 100644 core/java/android/flags/BooleanFlagBase.java create mode 100644 core/tests/coretests/src/android/flags/IFeatureFlagsFake.java diff --git a/core/java/android/flags/BooleanFlag.java b/core/java/android/flags/BooleanFlag.java index 5519961a917fa..ae9ccf8c52c36 100644 --- a/core/java/android/flags/BooleanFlag.java +++ b/core/java/android/flags/BooleanFlag.java @@ -25,9 +25,7 @@ import android.annotation.NonNull; * * @hide */ -public class BooleanFlag implements Flag { - private final String mNamespace; - private final String mName; +public class BooleanFlag extends BooleanFlagBase { private final boolean mDefault; /** @@ -36,8 +34,7 @@ public class BooleanFlag implements Flag { * @param defaultValue The value of this flag if no other override is present. */ BooleanFlag(String namespace, String name, boolean defaultValue) { - mNamespace = namespace; - mName = name; + super(namespace, name); mDefault = defaultValue; } @@ -46,22 +43,4 @@ public class BooleanFlag implements Flag { public Boolean getDefault() { return mDefault; } - - @Override - @NonNull - public String getNamespace() { - return mNamespace; - } - - @Override - @NonNull - public String getName() { - return mName; - } - - @Override - @NonNull - public String toString() { - return getNamespace() + "." + getName() + "[" + getDefault() + "]"; - } } diff --git a/core/java/android/flags/BooleanFlagBase.java b/core/java/android/flags/BooleanFlagBase.java new file mode 100644 index 0000000000000..f4141ecab01b1 --- /dev/null +++ b/core/java/android/flags/BooleanFlagBase.java @@ -0,0 +1,54 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.flags; + +import android.annotation.NonNull; + +abstract class BooleanFlagBase implements Flag { + + private final String mNamespace; + private final String mName; + + /** + * @param namespace A namespace for this flag. See {@link android.provider.DeviceConfig}. + * @param name A name for this flag. + */ + BooleanFlagBase(String namespace, String name) { + mNamespace = namespace; + mName = name; + } + + public abstract Boolean getDefault(); + + @Override + @NonNull + public String getNamespace() { + return mNamespace; + } + + @Override + @NonNull + public String getName() { + return mName; + } + + @Override + @NonNull + public String toString() { + return getNamespace() + "." + getName() + "[" + getDefault() + "]"; + } +} diff --git a/core/java/android/flags/DynamicBooleanFlag.java b/core/java/android/flags/DynamicBooleanFlag.java index d76fd9ec9a3c6..92009c60d91a1 100644 --- a/core/java/android/flags/DynamicBooleanFlag.java +++ b/core/java/android/flags/DynamicBooleanFlag.java @@ -23,10 +23,8 @@ package android.flags; * * @hide */ -public class DynamicBooleanFlag implements DynamicFlag { +public class DynamicBooleanFlag extends BooleanFlagBase implements DynamicFlag { - private final String mNamespace; - private final String mName; private final boolean mDefault; /** @@ -35,28 +33,12 @@ public class DynamicBooleanFlag implements DynamicFlag { * @param defaultValue The value of this flag if no other override is present. */ DynamicBooleanFlag(String namespace, String name, boolean defaultValue) { - mNamespace = namespace; - mName = name; + super(namespace, name); mDefault = defaultValue; } - @Override - public String getNamespace() { - return mNamespace; - } - - @Override - public String getName() { - return mName; - } - @Override public Boolean getDefault() { return mDefault; } - - @Override - public String toString() { - return getNamespace() + "." + getName() + "[" + getDefault() + "]"; - } } diff --git a/core/java/android/flags/FeatureFlags.java b/core/java/android/flags/FeatureFlags.java index c7d9d57e68b59..2c722a486e817 100644 --- a/core/java/android/flags/FeatureFlags.java +++ b/core/java/android/flags/FeatureFlags.java @@ -17,8 +17,19 @@ package android.flags; import android.annotation.NonNull; +import android.content.Context; +import android.os.RemoteException; +import android.os.ServiceManager; +import android.util.ArraySet; +import android.util.Log; +import com.android.internal.annotations.VisibleForTesting; + +import java.util.ArrayList; +import java.util.HashMap; import java.util.HashSet; +import java.util.List; +import java.util.Map; import java.util.Set; /** @@ -32,14 +43,20 @@ import java.util.Set; * @hide */ public class FeatureFlags { + private static final String TAG = "FeatureFlags"; private static FeatureFlags sInstance; private static final Object sInstanceLock = new Object(); + private final Set> mKnownFlags = new ArraySet<>(); + private final Set> mDirtyFlags = new ArraySet<>(); + + private IFeatureFlags mIFeatureFlags; + private final Map> mBooleanOverrides = new HashMap<>(); private final Set mListeners = new HashSet<>(); /** * Obtain a per-process instance of FeatureFlags. - * @return + * @return A singleton instance of {@link FeatureFlags}. */ @NonNull public static FeatureFlags getInstance() { @@ -52,7 +69,124 @@ public class FeatureFlags { return sInstance; } - FeatureFlags() { + /** See {@link FeatureFlagsFake}. */ + @VisibleForTesting(visibility = VisibleForTesting.Visibility.PRIVATE) + public static void setInstance(FeatureFlags instance) { + synchronized (sInstanceLock) { + sInstance = instance; + } + } + + private final IFeatureFlagsCallback mIFeatureFlagsCallback = new IFeatureFlagsCallback.Stub() { + @Override + public void onFlagChange(SyncableFlag flag) { + for (Flag f : mKnownFlags) { + if (flagEqualsSyncableFlag(f, flag)) { + if (f instanceof DynamicFlag) { + if (f instanceof DynamicBooleanFlag) { + String value = flag.getValue(); + if (value == null) { // Null means any existing overrides were erased. + value = ((DynamicBooleanFlag) f).getDefault().toString(); + } + addBooleanOverride(flag.getNamespace(), flag.getName(), value); + } + FeatureFlags.this.onFlagChange((DynamicFlag) f); + } + break; + } + } + } + }; + + private FeatureFlags() { + this(null); + } + + @VisibleForTesting(visibility = VisibleForTesting.Visibility.PRIVATE) + public FeatureFlags(IFeatureFlags iFeatureFlags) { + mIFeatureFlags = iFeatureFlags; + + if (mIFeatureFlags != null) { + try { + mIFeatureFlags.registerCallback(mIFeatureFlagsCallback); + } catch (RemoteException e) { + // Won't happen in tests. + } + } + } + + /** + * Construct a new {@link BooleanFlag}. + * + * Use this instead of constructing a {@link BooleanFlag} directly, as it registers the flag + * with the internals of the flagging system. + */ + @NonNull + public static BooleanFlag booleanFlag( + @NonNull String namespace, @NonNull String name, boolean def) { + return getInstance().addFlag(new BooleanFlag(namespace, name, def)); + } + + /** + * Construct a new {@link FusedOffFlag}. + * + * Use this instead of constructing a {@link FusedOffFlag} directly, as it registers the + * flag with the internals of the flagging system. + */ + @NonNull + public static FusedOffFlag fusedOffFlag(@NonNull String namespace, @NonNull String name) { + return getInstance().addFlag(new FusedOffFlag(namespace, name)); + } + + /** + * Construct a new {@link FusedOnFlag}. + * + * Use this instead of constructing a {@link FusedOnFlag} directly, as it registers the flag + * with the internals of the flagging system. + */ + @NonNull + public static FusedOnFlag fusedOnFlag(@NonNull String namespace, @NonNull String name) { + return getInstance().addFlag(new FusedOnFlag(namespace, name)); + } + + /** + * Construct a new {@link DynamicBooleanFlag}. + * + * Use this instead of constructing a {@link DynamicBooleanFlag} directly, as it registers + * the flag with the internals of the flagging system. + */ + @NonNull + public static DynamicBooleanFlag dynamicBooleanFlag( + @NonNull String namespace, @NonNull String name, boolean def) { + return getInstance().addFlag(new DynamicBooleanFlag(namespace, name, def)); + } + + /** + * Add a listener to be alerted when a {@link DynamicFlag} changes. + * + * See also {@link #removeChangeListener(ChangeListener)}. + * + * @param listener The listener to add. + */ + public void addChangeListener(@NonNull ChangeListener listener) { + mListeners.add(listener); + } + + /** + * Remove a listener that was added earlier. + * + * See also {@link #addChangeListener(ChangeListener)}. + * + * @param listener The listener to remove. + */ + public void removeChangeListener(@NonNull ChangeListener listener) { + mListeners.remove(listener); + } + + protected void onFlagChange(@NonNull DynamicFlag flag) { + for (ChangeListener l : mListeners) { + l.onFlagChanged(flag); + } } /** @@ -63,7 +197,7 @@ public class FeatureFlags { * The first time a flag is read, its value is cached for the lifetime of the process. */ public boolean isEnabled(@NonNull BooleanFlag flag) { - return flag.getDefault(); + return getBooleanInternal(flag); } /** @@ -90,31 +224,138 @@ public class FeatureFlags { * Can return a different value for the flag each time it is called if an override comes in. */ public boolean isCurrentlyEnabled(@NonNull DynamicBooleanFlag flag) { - return flag.getDefault(); + return getBooleanInternal(flag); + } + + private boolean getBooleanInternal(Flag flag) { + sync(); + Map ns = mBooleanOverrides.get(flag.getNamespace()); + Boolean value = null; + if (ns != null) { + value = ns.get(flag.getName()); + } + if (value == null) { + throw new IllegalStateException("Boolean flag being read but was not synced: " + flag); + } + + return value; + } + + private > T addFlag(T flag) { + synchronized (FeatureFlags.class) { + mDirtyFlags.add(flag); + mKnownFlags.add(flag); + } + return flag; + } + + protected void sync() { + synchronized (FeatureFlags.class) { + if (mDirtyFlags.isEmpty()) { + return; + } + syncInternal(mDirtyFlags); + mDirtyFlags.clear(); + } } /** - * Add a listener to be alerted when a {@link DynamicFlag} changes. + * Called when new flags have been declared. Gives the implementation a chance to act on them. * - * See also {@link #removeChangeListener(ChangeListener)}. - * - * @param listener The listener to add. + * Guaranteed to be called from a synchronized, thread-safe context. */ - public void addChangeListener(@NonNull ChangeListener listener) { - mListeners.add(listener); + protected void syncInternal(Set> dirtyFlags) { + IFeatureFlags iFeatureFlags = bind(); + List syncableFlags = new ArrayList<>(); + for (Flag f : dirtyFlags) { + syncableFlags.add(flagToSyncableFlag(f)); + } + + List serverFlags = List.of(); // Need to initialize the list with something. + try { + // New values come back from the service. + serverFlags = iFeatureFlags.syncFlags(syncableFlags); + } catch (RemoteException e) { + e.rethrowFromSystemServer(); + } + + for (Flag f : dirtyFlags) { + boolean found = false; + for (SyncableFlag sf : serverFlags) { + if (flagEqualsSyncableFlag(f, sf)) { + if (f instanceof BooleanFlag || f instanceof DynamicBooleanFlag) { + addBooleanOverride(sf.getNamespace(), sf.getName(), sf.getValue()); + } + found = true; + break; + } + } + if (!found) { + if (f instanceof BooleanFlag) { + addBooleanOverride( + f.getNamespace(), + f.getName(), + ((BooleanFlag) f).getDefault() ? "true" : "false"); + } + } + } } - /** - * Remove a listener that was added earlier. - * - * See also {@link #addChangeListener(ChangeListener)}. - * - * @param listener The listener to remove. - */ - public void removeChangeListener(@NonNull ChangeListener listener) { - mListeners.remove(listener); + private void addBooleanOverride(String namespace, String name, String override) { + Map nsOverrides = mBooleanOverrides.get(namespace); + if (nsOverrides == null) { + nsOverrides = new HashMap<>(); + mBooleanOverrides.put(namespace, nsOverrides); + } + nsOverrides.put(name, parseBoolean(override)); } + private SyncableFlag flagToSyncableFlag(Flag f) { + return new SyncableFlag( + f.getNamespace(), + f.getName(), + f.getDefault().toString(), + f instanceof DynamicFlag); + } + + private IFeatureFlags bind() { + if (mIFeatureFlags == null) { + mIFeatureFlags = IFeatureFlags.Stub.asInterface( + ServiceManager.getService(Context.FEATURE_FLAGS_SERVICE)); + try { + mIFeatureFlags.registerCallback(mIFeatureFlagsCallback); + } catch (RemoteException e) { + Log.e(TAG, "Failed to listen for flag changes!"); + } + } + + return mIFeatureFlags; + } + + static boolean parseBoolean(String value) { + // Check for a truish string. + boolean result = value.equalsIgnoreCase("true") + || value.equals("1") + || value.equalsIgnoreCase("t") + || value.equalsIgnoreCase("on"); + if (!result) { // Expect a falsish string, else log an error. + if (!(value.equalsIgnoreCase("false") + || value.equals("0") + || value.equalsIgnoreCase("f") + || value.equalsIgnoreCase("off"))) { + Log.e(TAG, + "Tried parsing " + value + " as boolean but it doesn't look like one. " + + "Value expected to be one of true|false, 1|0, t|f, on|off."); + } + } + return result; + } + + private static boolean flagEqualsSyncableFlag(Flag f, SyncableFlag sf) { + return f.getName().equals(sf.getName()) && f.getNamespace().equals(sf.getNamespace()); + } + + /** * A simpler listener that is alerted when a {@link DynamicFlag} changes. * diff --git a/core/java/android/flags/FeatureFlagsFake.java b/core/java/android/flags/FeatureFlagsFake.java index 03bf2b6d33d26..daedcdae0b5f5 100644 --- a/core/java/android/flags/FeatureFlagsFake.java +++ b/core/java/android/flags/FeatureFlagsFake.java @@ -18,32 +18,98 @@ package android.flags; import android.annotation.NonNull; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; + /** * An implementation of {@link FeatureFlags} for testing. + * + * Before you read a flag from using this Fake, you must set that flag using + * {@link #setFlagValue(BooleanFlagBase, boolean)}. This ensures that your tests are deterministic. + * + * If you are relying on {@link FeatureFlags#getInstance()} to access FeatureFlags in your code + * under test, (instead of dependency injection), you can pass an instance of this fake to + * {@link FeatureFlags#setInstance(FeatureFlags)}. Be sure to call that method again, passing null, + * to ensure hermetic testing - you don't want static state persisting between your test methods. + * * @hide */ public class FeatureFlagsFake extends FeatureFlags { - public FeatureFlagsFake() { - super(); + private final Map mFlagValues = new HashMap<>(); + private final Set mReadFlags = new HashSet<>(); + + public FeatureFlagsFake(IFeatureFlags iFeatureFlags) { + super(iFeatureFlags); } @Override public boolean isEnabled(@NonNull BooleanFlag flag) { - return flag.getDefault(); + return requireFlag(flag); } @Override public boolean isEnabled(@NonNull FusedOffFlag flag) { - return false; + return requireFlag(flag); } @Override public boolean isEnabled(@NonNull FusedOnFlag flag) { - return true; + return requireFlag(flag); } @Override public boolean isCurrentlyEnabled(@NonNull DynamicBooleanFlag flag) { - return flag.getDefault(); + return requireFlag(flag); } + + @Override + protected void syncInternal(Set> dirtyFlags) { + } + + /** + * Explicitly set a flag's value for reading in tests. + * + * You _must_ call this for every flag your code-under-test will read. Otherwise, an + * {@link IllegalStateException} will be thrown. + * + * You are able to set values for {@link FusedOffFlag} and {@link FusedOnFlag}, despite those + * flags having a fixed value at compile time, since unit tests should still test the state of + * those flags as both true and false. I.e. a flag that is off might be turned on in a future + * build or vice versa. + * + * You can not call this method _after_ a non-dynamic flag has been read. Non-dynamic flags + * are held stable in the system, so changing a value after reading would not match + * real-implementation behavior. + * + * Calling this method will trigger any {@link android.flags.FeatureFlags.ChangeListener}s that + * are registered for the supplied flag if the flag is a {@link DynamicFlag}. + * + * @param flag The BooleanFlag that you want to set a value for. + * @param value The value that the flag should return when accessed. + */ + public void setFlagValue(@NonNull BooleanFlagBase flag, boolean value) { + if (!(flag instanceof DynamicBooleanFlag) && mReadFlags.contains(flag)) { + throw new RuntimeException( + "You can not set the value of a flag after it has been read. Tried to set " + + flag + " to " + value + " but it already " + mFlagValues.get(flag)); + } + mFlagValues.put(flag, value); + if (flag instanceof DynamicBooleanFlag) { + onFlagChange((DynamicFlag) flag); + } + } + + private boolean requireFlag(BooleanFlagBase flag) { + if (!mFlagValues.containsKey(flag)) { + throw new IllegalStateException( + "Tried to access " + flag + " in test but no overrided specified. You must " + + "call #setFlagValue for each flag read in a test."); + } + mReadFlags.add(flag); + + return mFlagValues.get(flag); + } + } diff --git a/core/java/android/flags/FusedOffFlag.java b/core/java/android/flags/FusedOffFlag.java index 999d0f9817801..5f435beb26421 100644 --- a/core/java/android/flags/FusedOffFlag.java +++ b/core/java/android/flags/FusedOffFlag.java @@ -17,6 +17,7 @@ package android.flags; import android.annotation.NonNull; +import android.provider.DeviceConfig; /** * A flag representing a false value. @@ -25,13 +26,13 @@ import android.annotation.NonNull; * * @hide */ -public final class FusedOffFlag implements Flag { - private final String mNamespace; - private final String mName; - +public final class FusedOffFlag extends BooleanFlagBase { + /** + * @param namespace A namespace for this flag. See {@link DeviceConfig}. + * @param name A name for this flag. + */ FusedOffFlag(String namespace, String name) { - mNamespace = namespace; - mName = name; + super(namespace, name); } @Override @@ -39,22 +40,4 @@ public final class FusedOffFlag implements Flag { public Boolean getDefault() { return false; } - - @Override - @NonNull - public String getNamespace() { - return mNamespace; - } - - @Override - @NonNull - public String getName() { - return mName; - } - - @Override - @NonNull - public String toString() { - return getNamespace() + "." + getName() + "[false]"; - } } diff --git a/core/java/android/flags/FusedOnFlag.java b/core/java/android/flags/FusedOnFlag.java index 4f33bee8abe12..ea8d9b29b3cec 100644 --- a/core/java/android/flags/FusedOnFlag.java +++ b/core/java/android/flags/FusedOnFlag.java @@ -17,6 +17,7 @@ package android.flags; import android.annotation.NonNull; +import android.provider.DeviceConfig; /** * A flag representing a true value. @@ -25,13 +26,13 @@ import android.annotation.NonNull; * * @hide */ -public final class FusedOnFlag implements Flag { - private final String mNamespace; - private final String mName; - +public final class FusedOnFlag extends BooleanFlagBase { + /** + * @param namespace A namespace for this flag. See {@link DeviceConfig}. + * @param name A name for this flag. + */ FusedOnFlag(String namespace, String name) { - mNamespace = namespace; - mName = name; + super(namespace, name); } @Override @@ -39,22 +40,4 @@ public final class FusedOnFlag implements Flag { public Boolean getDefault() { return true; } - - @Override - @NonNull - public String getNamespace() { - return mNamespace; - } - - @Override - @NonNull - public String getName() { - return mName; - } - - @Override - @NonNull - public String toString() { - return getNamespace() + "." + getName() + "[true]"; - } } diff --git a/core/java/android/flags/SyncableFlag.java b/core/java/android/flags/SyncableFlag.java index 54c4c6d5fdf96..0b2a4d9e4f988 100644 --- a/core/java/android/flags/SyncableFlag.java +++ b/core/java/android/flags/SyncableFlag.java @@ -20,18 +20,15 @@ import android.annotation.NonNull; import android.os.Parcel; import android.os.Parcelable; -import com.android.internal.annotations.VisibleForTesting; - /** * @hide */ public final class SyncableFlag implements Parcelable { private final String mNamespace; private final String mName; - private String mValue; + private final String mValue; private final boolean mDynamic; - @VisibleForTesting(visibility = VisibleForTesting.Visibility.PACKAGE) public SyncableFlag( @NonNull String namespace, @NonNull String name, @@ -43,10 +40,6 @@ public final class SyncableFlag implements Parcelable { mDynamic = dynamic; } - public void setValue(@NonNull String value) { - mValue = value; - } - @NonNull public String getNamespace() { return mNamespace; diff --git a/core/tests/coretests/src/android/flags/FeatureFlagsTest.java b/core/tests/coretests/src/android/flags/FeatureFlagsTest.java index dc6006a8e5fa4..3fc94394d12c2 100644 --- a/core/tests/coretests/src/android/flags/FeatureFlagsTest.java +++ b/core/tests/coretests/src/android/flags/FeatureFlagsTest.java @@ -13,7 +13,6 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - package android.flags; import static com.google.common.truth.Truth.assertThat; @@ -22,33 +21,135 @@ import android.platform.test.annotations.Presubmit; import androidx.test.filters.SmallTest; -import org.junit.After; import org.junit.Before; -import org.junit.Ignore; import org.junit.Test; -import org.mockito.MockitoAnnotations; + +import java.util.List; +import java.util.concurrent.atomic.AtomicBoolean; + @SmallTest @Presubmit -public final class FeatureFlagsTest { +public class FeatureFlagsTest { + + IFeatureFlagsFake mIFeatureFlagsFake = new IFeatureFlagsFake(); + FeatureFlags mFeatureFlags = new FeatureFlags(mIFeatureFlagsFake); @Before public void setup() { - MockitoAnnotations.initMocks(this); - } - - @After - public void tearDown() { + FeatureFlags.setInstance(mFeatureFlags); } @Test - public void testPass() { - assertThat(true).isTrue(); + public void testFusedOff_Disabled() { + FusedOffFlag flag = FeatureFlags.fusedOffFlag("test", "a"); + assertThat(mFeatureFlags.isEnabled(flag)).isFalse(); } - @Ignore @Test - public void testFail() { - assertThat(false).isTrue(); + public void testFusedOn_Enabled() { + FusedOnFlag flag = FeatureFlags.fusedOnFlag("test", "a"); + assertThat(mFeatureFlags.isEnabled(flag)).isTrue(); + } + + @Test + public void testBooleanFlag_DefaultDisabled() { + BooleanFlag flag = FeatureFlags.booleanFlag("test", "a", false); + assertThat(mFeatureFlags.isEnabled(flag)).isFalse(); + } + + @Test + public void testBooleanFlag_DefaultEnabled() { + BooleanFlag flag = FeatureFlags.booleanFlag("test", "a", true); + assertThat(mFeatureFlags.isEnabled(flag)).isTrue(); + } + + @Test + public void testDynamicBooleanFlag_DefaultDisabled() { + DynamicBooleanFlag flag = FeatureFlags.dynamicBooleanFlag("test", "a", false); + assertThat(mFeatureFlags.isCurrentlyEnabled(flag)).isFalse(); + } + + @Test + public void testDynamicBooleanFlag_DefaultEnabled() { + DynamicBooleanFlag flag = FeatureFlags.dynamicBooleanFlag("test", "a", true); + assertThat(mFeatureFlags.isCurrentlyEnabled(flag)).isTrue(); + } + + @Test + public void testBooleanFlag_OverrideBeforeRead() { + BooleanFlag flag = FeatureFlags.booleanFlag("test", "a", false); + SyncableFlag syncableFlag = new SyncableFlag( + flag.getNamespace(), flag.getName(), "true", false); + + mIFeatureFlagsFake.setFlagOverrides(List.of(syncableFlag)); + + assertThat(mFeatureFlags.isEnabled(flag)).isTrue(); + } + + @Test + public void testFusedOffFlag_OverrideHasNoEffect() { + FusedOffFlag flag = FeatureFlags.fusedOffFlag("test", "a"); + SyncableFlag syncableFlag = new SyncableFlag( + flag.getNamespace(), flag.getName(), "true", false); + + mIFeatureFlagsFake.setFlagOverrides(List.of(syncableFlag)); + + assertThat(mFeatureFlags.isEnabled(flag)).isFalse(); + } + + @Test + public void testFusedOnFlag_OverrideHasNoEffect() { + FusedOnFlag flag = FeatureFlags.fusedOnFlag("test", "a"); + SyncableFlag syncableFlag = new SyncableFlag( + flag.getNamespace(), flag.getName(), "false", false); + + mIFeatureFlagsFake.setFlagOverrides(List.of(syncableFlag)); + + assertThat(mFeatureFlags.isEnabled(flag)).isTrue(); + } + + @Test + public void testDynamicFlag_OverrideBeforeRead() { + DynamicBooleanFlag flag = FeatureFlags.dynamicBooleanFlag("test", "a", false); + SyncableFlag syncableFlag = new SyncableFlag( + flag.getNamespace(), flag.getName(), "true", true); + + mIFeatureFlagsFake.setFlagOverrides(List.of(syncableFlag)); + + // Changes to true + assertThat(mFeatureFlags.isCurrentlyEnabled(flag)).isTrue(); + } + + @Test + public void testDynamicFlag_OverrideAfterRead() { + DynamicBooleanFlag flag = FeatureFlags.dynamicBooleanFlag("test", "a", false); + SyncableFlag syncableFlag = new SyncableFlag( + flag.getNamespace(), flag.getName(), "true", true); + + // Starts false + assertThat(mFeatureFlags.isCurrentlyEnabled(flag)).isFalse(); + + mIFeatureFlagsFake.setFlagOverrides(List.of(syncableFlag)); + + // Changes to true + assertThat(mFeatureFlags.isCurrentlyEnabled(flag)).isTrue(); + } + + @Test + public void testDynamicFlag_FiresListener() { + DynamicBooleanFlag flag = FeatureFlags.dynamicBooleanFlag("test", "a", false); + AtomicBoolean called = new AtomicBoolean(false); + FeatureFlags.ChangeListener listener = flag1 -> called.set(true); + + mFeatureFlags.addChangeListener(listener); + + SyncableFlag syncableFlag = new SyncableFlag( + flag.getNamespace(), flag.getName(), flag.getDefault().toString(), true); + + mIFeatureFlagsFake.setFlagOverrides(List.of(syncableFlag)); + + // Fires listener. + assertThat(called.get()).isTrue(); } } diff --git a/core/tests/coretests/src/android/flags/IFeatureFlagsFake.java b/core/tests/coretests/src/android/flags/IFeatureFlagsFake.java new file mode 100644 index 0000000000000..70b65ce1eb577 --- /dev/null +++ b/core/tests/coretests/src/android/flags/IFeatureFlagsFake.java @@ -0,0 +1,63 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package android.flags; + +import android.os.IBinder; +import android.os.RemoteException; + +import java.util.HashSet; +import java.util.List; +import java.util.Set; + +class IFeatureFlagsFake implements IFeatureFlags { + + private final Set mCallbacks = new HashSet<>(); + + List mOverrides; + + @Override + public IBinder asBinder() { + return null; + } + + @Override + public List syncFlags(List flagList) { + return mOverrides == null ? flagList : mOverrides; + } + + @Override + public void registerCallback(IFeatureFlagsCallback callback) { + mCallbacks.add(callback); + } + + @Override + public void unregisterCallback(IFeatureFlagsCallback callback) { + mCallbacks.remove(callback); + } + + public void setFlagOverrides(List flagList) { + mOverrides = flagList; + for (SyncableFlag sf : flagList) { + for (IFeatureFlagsCallback cb : mCallbacks) { + try { + cb.onFlagChange(sf); + } catch (RemoteException e) { + // does not happen in fakes. + } + } + } + } +} diff --git a/services/flags/java/com/android/server/flags/DynamicFlagBinderDelegate.java b/services/flags/java/com/android/server/flags/DynamicFlagBinderDelegate.java index 322a52bf01cc9..8da5aae6abdd6 100644 --- a/services/flags/java/com/android/server/flags/DynamicFlagBinderDelegate.java +++ b/services/flags/java/com/android/server/flags/DynamicFlagBinderDelegate.java @@ -118,9 +118,9 @@ class DynamicFlagBinderDelegate { mFlagStore.setChangeCallback(mFlagChangeCallback); } - void syncDynamicFlag(int pid, SyncableFlag sf) { + SyncableFlag syncDynamicFlag(int pid, SyncableFlag sf) { if (!sf.isDynamic()) { - return; + return sf; } String ns = sf.getNamespace(); @@ -162,7 +162,7 @@ class DynamicFlagBinderDelegate { // to something. data.setDefaultValue(sf.getValue()); - sf.setValue(value); + return new SyncableFlag(sf.getNamespace(), sf.getName(), value, true); } diff --git a/services/flags/java/com/android/server/flags/FeatureFlagsBinder.java b/services/flags/java/com/android/server/flags/FeatureFlagsBinder.java index dc97fde758dcc..86b464dbdf5db 100644 --- a/services/flags/java/com/android/server/flags/FeatureFlagsBinder.java +++ b/services/flags/java/com/android/server/flags/FeatureFlagsBinder.java @@ -25,6 +25,7 @@ import android.os.Build; import android.os.ParcelFileDescriptor; import java.io.FileOutputStream; +import java.util.ArrayList; import java.util.List; class FeatureFlagsBinder extends IFeatureFlags.Stub { @@ -52,11 +53,13 @@ class FeatureFlagsBinder extends IFeatureFlags.Stub { @Override public List syncFlags(List incomingFlags) { int pid = getCallingPid(); + List outputFlags = new ArrayList<>(); for (SyncableFlag sf : incomingFlags) { String ns = sf.getNamespace(); String name = sf.getName(); + SyncableFlag outFlag; if (sf.isDynamic()) { - mDynamicFlagDelegate.syncDynamicFlag(pid, sf); + outFlag = mDynamicFlagDelegate.syncDynamicFlag(pid, sf); } else { synchronized (mFlagCache) { String value = mFlagCache.getOrNull(ns, name); @@ -65,11 +68,12 @@ class FeatureFlagsBinder extends IFeatureFlags.Stub { value = overrideValue != null ? overrideValue : sf.getValue(); mFlagCache.setIfChanged(ns, name, value); } - sf.setValue(value); + outFlag = new SyncableFlag(sf.getNamespace(), sf.getName(), value, false); } } + outputFlags.add(outFlag); } - return incomingFlags; + return outputFlags; } @SystemApi From 6a9b4540f3ccab29c14812aa2374083436689b00 Mon Sep 17 00:00:00 2001 From: Dave Mankoff Date: Wed, 12 Jul 2023 19:32:11 +0000 Subject: [PATCH 4/6] Revert^2 "Add Local FeatureFlags client to system servier." c44e351a789e269038e771163e9e0a4a3f109253 Bug: 279054964 Change-Id: Ie2db80c85948d879c6330dd1f2463a2e8659cb6b --- core/java/android/flags/FeatureFlags.java | 3 ++- .../java/com/android/server/flags/FeatureFlagsService.java | 5 ++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/core/java/android/flags/FeatureFlags.java b/core/java/android/flags/FeatureFlags.java index 2c722a486e817..93e56b1724345 100644 --- a/core/java/android/flags/FeatureFlags.java +++ b/core/java/android/flags/FeatureFlags.java @@ -110,7 +110,8 @@ public class FeatureFlags { try { mIFeatureFlags.registerCallback(mIFeatureFlagsCallback); } catch (RemoteException e) { - // Won't happen in tests. + // Shouldn't happen with things passed into tests. + Log.e(TAG, "Could not register callbacks!", e); } } } diff --git a/services/flags/java/com/android/server/flags/FeatureFlagsService.java b/services/flags/java/com/android/server/flags/FeatureFlagsService.java index 111fad0ce3b0a..a9de173486393 100644 --- a/services/flags/java/com/android/server/flags/FeatureFlagsService.java +++ b/services/flags/java/com/android/server/flags/FeatureFlagsService.java @@ -16,6 +16,7 @@ package com.android.server.flags; import android.content.Context; +import android.flags.FeatureFlags; import android.util.Slog; import com.android.server.SystemService; @@ -52,8 +53,10 @@ public class FeatureFlagsService extends SystemService { @Override public void onStart() { Slog.d(TAG, "Started Feature Flag Service"); + FeatureFlagsBinder service = new FeatureFlagsBinder(mFlagStore, mShellCommand); publishBinderService( - Context.FEATURE_FLAGS_SERVICE, new FeatureFlagsBinder(mFlagStore, mShellCommand)); + Context.FEATURE_FLAGS_SERVICE, service); + publishLocalService(FeatureFlags.class, new FeatureFlags(service)); } } From fdc60c874914dc95131604ff8d34e6fc25207bf0 Mon Sep 17 00:00:00 2001 From: Dave Mankoff Date: Wed, 12 Jul 2023 19:32:11 +0000 Subject: [PATCH 5/6] Revert "Revert "Add permissions to FeatureFlagService and overri..." Revert submission 24024260-revert-23727772-b279054964-flag-api-udc-qpr-dev-YOBAYFNKLI Reason for revert: Restoring for b/279054964. Fix included. Reverted changes: /q/submissionid:24024260-revert-23727772-b279054964-flag-api-udc-qpr-dev-YOBAYFNKLI Bug: 279054964 Change-Id: I2ea2b3b6000f47242e5a07e7efd39264b53a0c27 --- core/java/android/flags/DynamicFlag.java | 4 + core/java/android/flags/FeatureFlags.java | 8 +- core/java/android/flags/Flag.java | 5 + core/java/android/flags/IFeatureFlags.aidl | 24 ++++ core/java/android/flags/SyncableFlag.java | 24 +++- .../com/android/internal/flags/CoreFlags.java | 93 ++++++++++++++ core/res/AndroidManifest.xml | 18 +++ .../src/android/flags/IFeatureFlagsFake.java | 50 ++++++++ .../flags/DynamicFlagBinderDelegate.java | 43 ++++--- .../server/flags/FeatureFlagsBinder.java | 80 +++++++++++- .../server/flags/FeatureFlagsService.java | 53 +++++++- .../server/flags/FlagOverrideStore.java | 6 +- .../server/flags/FeatureFlagsServiceTest.java | 118 +++++++++++++++++- 13 files changed, 499 insertions(+), 27 deletions(-) create mode 100644 core/java/com/android/internal/flags/CoreFlags.java diff --git a/core/java/android/flags/DynamicFlag.java b/core/java/android/flags/DynamicFlag.java index 8583ea6034d89..68819c58c064d 100644 --- a/core/java/android/flags/DynamicFlag.java +++ b/core/java/android/flags/DynamicFlag.java @@ -24,4 +24,8 @@ package android.flags; * @hide */ public interface DynamicFlag extends Flag { + @Override + default boolean isDynamic() { + return true; + } } diff --git a/core/java/android/flags/FeatureFlags.java b/core/java/android/flags/FeatureFlags.java index 93e56b1724345..8d3112c35d512 100644 --- a/core/java/android/flags/FeatureFlags.java +++ b/core/java/android/flags/FeatureFlags.java @@ -250,7 +250,13 @@ public class FeatureFlags { return flag; } - protected void sync() { + /** + * Sync any known flags that have not yet been synced. + * + * This is called implicitly when any flag is read, and is not generally needed except in + * exceptional circumstances. + */ + public void sync() { synchronized (FeatureFlags.class) { if (mDirtyFlags.isEmpty()) { return; diff --git a/core/java/android/flags/Flag.java b/core/java/android/flags/Flag.java index ae0a6ba33f44b..0ab3c8a5d6172 100644 --- a/core/java/android/flags/Flag.java +++ b/core/java/android/flags/Flag.java @@ -37,4 +37,9 @@ public interface Flag { /** The value of this flag if no override has been set. Null values are not supported. */ @NonNull T getDefault(); + + /** Returns true if the value of this flag can change at runtime. */ + default boolean isDynamic() { + return false; + } } diff --git a/core/java/android/flags/IFeatureFlags.aidl b/core/java/android/flags/IFeatureFlags.aidl index 1eef47feaa8b3..3efcec97fe6d5 100644 --- a/core/java/android/flags/IFeatureFlags.aidl +++ b/core/java/android/flags/IFeatureFlags.aidl @@ -62,4 +62,28 @@ interface IFeatureFlags { * {@link #registerCallback}. */ void unregisterCallback(IFeatureFlagsCallback callback); + + /** + * Query the {@link com.android.server.flags.FeatureFlagsService} for flags, but don't + * cache them. See {@link #syncFlags}. + * + * You almost certainly don't want this method. This is intended for the Flag Flipper + * application that needs to query the state of system but doesn't want to affect it by + * doing so. All other clients should use {@link syncFlags}. + */ + List queryFlags(in List flagList); + + /** + * Change a flags value in the system. + * + * This is intended for use by the Flag Flipper application. + */ + void overrideFlag(in SyncableFlag flag); + + /** + * Restore a flag to its default value. + * + * This is intended for use by the Flag Flipper application. + */ + void resetFlag(in SyncableFlag flag); } \ No newline at end of file diff --git a/core/java/android/flags/SyncableFlag.java b/core/java/android/flags/SyncableFlag.java index 0b2a4d9e4f988..449bcc3c49f53 100644 --- a/core/java/android/flags/SyncableFlag.java +++ b/core/java/android/flags/SyncableFlag.java @@ -28,16 +28,28 @@ public final class SyncableFlag implements Parcelable { private final String mName; private final String mValue; private final boolean mDynamic; + private final boolean mOverridden; public SyncableFlag( @NonNull String namespace, @NonNull String name, @NonNull String value, boolean dynamic) { + this(namespace, name, value, dynamic, false); + } + + public SyncableFlag( + @NonNull String namespace, + @NonNull String name, + @NonNull String value, + boolean dynamic, + boolean overridden + ) { mNamespace = namespace; mName = name; mValue = value; mDynamic = dynamic; + mOverridden = overridden; } @NonNull @@ -55,16 +67,23 @@ public final class SyncableFlag implements Parcelable { return mValue; } - @NonNull public boolean isDynamic() { return mDynamic; } + public boolean isOverridden() { + return mOverridden; + } + @NonNull public static final Parcelable.Creator CREATOR = new Parcelable.Creator<>() { public SyncableFlag createFromParcel(Parcel in) { return new SyncableFlag( - in.readString(), in.readString(), in.readString(), in.readBoolean()); + in.readString(), + in.readString(), + in.readString(), + in.readBoolean(), + in.readBoolean()); } public SyncableFlag[] newArray(int size) { @@ -83,6 +102,7 @@ public final class SyncableFlag implements Parcelable { dest.writeString(mName); dest.writeString(mValue); dest.writeBoolean(mDynamic); + dest.writeBoolean(mOverridden); } @Override diff --git a/core/java/com/android/internal/flags/CoreFlags.java b/core/java/com/android/internal/flags/CoreFlags.java new file mode 100644 index 0000000000000..f177ef88c38f7 --- /dev/null +++ b/core/java/com/android/internal/flags/CoreFlags.java @@ -0,0 +1,93 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.internal.flags; + +import android.flags.BooleanFlag; +import android.flags.DynamicBooleanFlag; +import android.flags.FeatureFlags; +import android.flags.FusedOffFlag; +import android.flags.FusedOnFlag; +import android.flags.SyncableFlag; + +import java.util.ArrayList; +import java.util.List; + +/** + * Flags defined here are can be read by code in core. + * + * Flags not defined here will throw a security exception if third-party processes attempts to read + * them. + * + * DO NOT define a flag here unless you explicitly intend for that flag to be readable by code that + * runs inside a third party process. + */ +public abstract class CoreFlags { + private static final List sKnownFlags = new ArrayList<>(); + + public static BooleanFlag BOOL_FLAG = booleanFlag("core", "bool_flag", false); + public static FusedOffFlag OFF_FLAG = fusedOffFlag("core", "off_flag"); + public static FusedOnFlag ON_FLAG = fusedOnFlag("core", "on_flag"); + public static DynamicBooleanFlag DYN_FLAG = dynamicBooleanFlag("core", "dyn_flag", true); + + /** Returns true if the passed in flag matches a flag in this class. */ + public static boolean isCoreFlag(SyncableFlag flag) { + for (SyncableFlag knownFlag : sKnownFlags) { + if (knownFlag.getName().equals(flag.getName()) + && knownFlag.getNamespace().equals(flag.getNamespace())) { + return true; + } + } + return false; + } + + public static List getCoreFlags() { + return sKnownFlags; + } + + private static BooleanFlag booleanFlag(String namespace, String name, boolean defaultValue) { + BooleanFlag f = FeatureFlags.booleanFlag(namespace, name, defaultValue); + + sKnownFlags.add(new SyncableFlag(namespace, name, Boolean.toString(defaultValue), false)); + + return f; + } + + private static FusedOffFlag fusedOffFlag(String namespace, String name) { + FusedOffFlag f = FeatureFlags.fusedOffFlag(namespace, name); + + sKnownFlags.add(new SyncableFlag(namespace, name, "false", false)); + + return f; + } + + private static FusedOnFlag fusedOnFlag(String namespace, String name) { + FusedOnFlag f = FeatureFlags.fusedOnFlag(namespace, name); + + sKnownFlags.add(new SyncableFlag(namespace, name, "true", false)); + + return f; + } + + private static DynamicBooleanFlag dynamicBooleanFlag( + String namespace, String name, boolean defaultValue) { + DynamicBooleanFlag f = FeatureFlags.dynamicBooleanFlag(namespace, name, defaultValue); + + sKnownFlags.add(new SyncableFlag(namespace, name, Boolean.toString(defaultValue), true)); + + return f; + } +} diff --git a/core/res/AndroidManifest.xml b/core/res/AndroidManifest.xml index a01c7b6355c14..10cf353bf5e9b 100644 --- a/core/res/AndroidManifest.xml +++ b/core/res/AndroidManifest.xml @@ -7655,6 +7655,24 @@ + + + + + + + diff --git a/core/tests/coretests/src/android/flags/IFeatureFlagsFake.java b/core/tests/coretests/src/android/flags/IFeatureFlagsFake.java index 70b65ce1eb577..bc5d8aa3ac733 100644 --- a/core/tests/coretests/src/android/flags/IFeatureFlagsFake.java +++ b/core/tests/coretests/src/android/flags/IFeatureFlagsFake.java @@ -38,6 +38,56 @@ class IFeatureFlagsFake implements IFeatureFlags { return mOverrides == null ? flagList : mOverrides; } + @Override + public List queryFlags(List flagList) { + return mOverrides == null ? flagList : mOverrides; } + + @Override + public void overrideFlag(SyncableFlag syncableFlag) { + SyncableFlag match = findFlag(syncableFlag); + if (match != null) { + mOverrides.remove(match); + } + + mOverrides.add(syncableFlag); + + for (IFeatureFlagsCallback cb : mCallbacks) { + try { + cb.onFlagChange(syncableFlag); + } catch (RemoteException e) { + // does not happen in fakes. + } + } + } + + @Override + public void resetFlag(SyncableFlag syncableFlag) { + SyncableFlag match = findFlag(syncableFlag); + if (match != null) { + mOverrides.remove(match); + } + + for (IFeatureFlagsCallback cb : mCallbacks) { + try { + cb.onFlagChange(syncableFlag); + } catch (RemoteException e) { + // does not happen in fakes. + } + } + } + + private SyncableFlag findFlag(SyncableFlag syncableFlag) { + SyncableFlag match = null; + for (SyncableFlag sf : mOverrides) { + if (sf.getName().equals(syncableFlag.getName()) + && sf.getNamespace().equals(syncableFlag.getNamespace())) { + match = sf; + break; + } + } + + return match; + } @Override public void registerCallback(IFeatureFlagsCallback callback) { mCallbacks.add(callback); diff --git a/services/flags/java/com/android/server/flags/DynamicFlagBinderDelegate.java b/services/flags/java/com/android/server/flags/DynamicFlagBinderDelegate.java index 8da5aae6abdd6..0db328792cf3c 100644 --- a/services/flags/java/com/android/server/flags/DynamicFlagBinderDelegate.java +++ b/services/flags/java/com/android/server/flags/DynamicFlagBinderDelegate.java @@ -132,25 +132,8 @@ class DynamicFlagBinderDelegate { // about changes coming in from adb, DeviceConfig, or other sources. // And also so that we can keep flags relatively consistent across processes. - // If we already have a value cached, just use that. - String value = null; DynamicFlagData data = mDynamicFlags.getOrNull(ns, name); - if (data != null) { - value = data.getValue(); - } else { - // Put the value in the cache for future reference. - data = new DynamicFlagData(ns, name); - mDynamicFlags.setIfChanged(ns, name, data); - } - // If we're not in a release build, flags can be overridden locally on device. - if (!Build.IS_USER && value == null) { - value = mFlagStore.get(ns, name); - } - // If we still don't have a value, maybe DeviceConfig does? - // Fallback to sf.getValue() here as well. - if (value == null) { - value = DeviceConfig.getString(ns, name, sf.getValue()); - } + String value = getFlagValue(ns, name, sf.getValue()); // DeviceConfig listeners are per-namespace. if (!mDynamicFlags.containsNamespace(ns)) { DeviceConfig.addOnPropertiesChangedListener( @@ -200,6 +183,30 @@ class DynamicFlagBinderDelegate { } } + String getFlagValue(String namespace, String name, String defaultValue) { + // If we already have a value cached, just use that. + String value = null; + DynamicFlagData data = mDynamicFlags.getOrNull(namespace, name); + if (data != null) { + value = data.getValue(); + } else { + // Put the value in the cache for future reference. + data = new DynamicFlagData(namespace, name); + mDynamicFlags.setIfChanged(namespace, name, data); + } + // If we're not in a release build, flags can be overridden locally on device. + if (!Build.IS_USER && value == null) { + value = mFlagStore.get(namespace, name); + } + // If we still don't have a value, maybe DeviceConfig does? + // Fallback to sf.getValue() here as well. + if (value == null) { + value = DeviceConfig.getString(namespace, name, defaultValue); + } + + return value; + } + private static class DynamicFlagData { private final String mNamespace; private final String mName; diff --git a/services/flags/java/com/android/server/flags/FeatureFlagsBinder.java b/services/flags/java/com/android/server/flags/FeatureFlagsBinder.java index 86b464dbdf5db..1fa85325aea6a 100644 --- a/services/flags/java/com/android/server/flags/FeatureFlagsBinder.java +++ b/services/flags/java/com/android/server/flags/FeatureFlagsBinder.java @@ -24,6 +24,9 @@ import android.flags.SyncableFlag; import android.os.Build; import android.os.ParcelFileDescriptor; +import com.android.internal.flags.CoreFlags; +import com.android.server.flags.FeatureFlagsService.PermissionsChecker; + import java.io.FileOutputStream; import java.util.ArrayList; import java.util.List; @@ -33,11 +36,16 @@ class FeatureFlagsBinder extends IFeatureFlags.Stub { private final FlagsShellCommand mShellCommand; private final FlagCache mFlagCache = new FlagCache<>(); private final DynamicFlagBinderDelegate mDynamicFlagDelegate; + private final PermissionsChecker mPermissionsChecker; - FeatureFlagsBinder(FlagOverrideStore flagStore, FlagsShellCommand shellCommand) { + FeatureFlagsBinder( + FlagOverrideStore flagStore, + FlagsShellCommand shellCommand, + PermissionsChecker permissionsChecker) { mFlagStore = flagStore; mShellCommand = shellCommand; mDynamicFlagDelegate = new DynamicFlagBinderDelegate(flagStore); + mPermissionsChecker = permissionsChecker; } @Override @@ -50,11 +58,29 @@ class FeatureFlagsBinder extends IFeatureFlags.Stub { mDynamicFlagDelegate.unregisterCallback(getCallingPid(), callback); } + // Note: The internals of this method should be kept in sync with queryFlags + // as they both should return identical results. The difference is that this method + // caches any values it receives and/or reads, whereas queryFlags does not. + @Override public List syncFlags(List incomingFlags) { int pid = getCallingPid(); List outputFlags = new ArrayList<>(); + + boolean hasFullSyncPrivileges = false; + SecurityException permissionFailureException = null; + try { + assertSyncPermission(); + hasFullSyncPrivileges = true; + } catch (SecurityException e) { + permissionFailureException = e; + } + for (SyncableFlag sf : incomingFlags) { + if (!hasFullSyncPrivileges && !CoreFlags.isCoreFlag(sf)) { + throw permissionFailureException; + } + String ns = sf.getNamespace(); String name = sf.getName(); SyncableFlag outFlag; @@ -76,6 +102,58 @@ class FeatureFlagsBinder extends IFeatureFlags.Stub { return outputFlags; } + @Override + public void overrideFlag(SyncableFlag flag) { + assertWritePermission(); + mFlagStore.set(flag.getNamespace(), flag.getName(), flag.getValue()); + } + + @Override + public void resetFlag(SyncableFlag flag) { + assertWritePermission(); + mFlagStore.erase(flag.getNamespace(), flag.getName()); + } + + @Override + public List queryFlags(List incomingFlags) { + assertSyncPermission(); + List outputFlags = new ArrayList<>(); + for (SyncableFlag sf : incomingFlags) { + String ns = sf.getNamespace(); + String name = sf.getName(); + String value; + String storeValue = mFlagStore.get(ns, name); + boolean overridden = storeValue != null; + + if (sf.isDynamic()) { + value = mDynamicFlagDelegate.getFlagValue(ns, name, sf.getValue()); + } else { + value = mFlagCache.getOrNull(ns, name); + if (value == null) { + value = Build.IS_USER ? null : storeValue; + if (value == null) { + value = sf.getValue(); + } + } + } + outputFlags.add(new SyncableFlag( + sf.getNamespace(), sf.getName(), value, sf.isDynamic(), overridden)); + } + + return outputFlags; + } + + private void assertSyncPermission() { + mPermissionsChecker.assertSyncPermission(); + clearCallingIdentity(); + } + + private void assertWritePermission() { + mPermissionsChecker.assertWritePermission(); + clearCallingIdentity(); + } + + @SystemApi public int handleShellCommand( @NonNull ParcelFileDescriptor in, diff --git a/services/flags/java/com/android/server/flags/FeatureFlagsService.java b/services/flags/java/com/android/server/flags/FeatureFlagsService.java index a9de173486393..93b9e9e0dc8cf 100644 --- a/services/flags/java/com/android/server/flags/FeatureFlagsService.java +++ b/services/flags/java/com/android/server/flags/FeatureFlagsService.java @@ -15,10 +15,15 @@ */ package com.android.server.flags; +import static android.Manifest.permission.SYNC_FLAGS; +import static android.Manifest.permission.WRITE_FLAGS; + import android.content.Context; +import android.content.pm.PackageManager; import android.flags.FeatureFlags; import android.util.Slog; +import com.android.internal.annotations.VisibleForTesting; import com.android.server.SystemService; /** @@ -53,10 +58,56 @@ public class FeatureFlagsService extends SystemService { @Override public void onStart() { Slog.d(TAG, "Started Feature Flag Service"); - FeatureFlagsBinder service = new FeatureFlagsBinder(mFlagStore, mShellCommand); + FeatureFlagsBinder service = new FeatureFlagsBinder( + mFlagStore, mShellCommand, new PermissionsChecker(getContext())); publishBinderService( Context.FEATURE_FLAGS_SERVICE, service); publishLocalService(FeatureFlags.class, new FeatureFlags(service)); } + @Override + public void onBootPhase(int phase) { + super.onBootPhase(phase); + + if (phase == PHASE_SYSTEM_SERVICES_READY) { + // Immediately sync our core flags so that they get locked in. We don't want third-party + // apps to override them, and syncing immediately is the easiest way to prevent that. + FeatureFlags.getInstance().sync(); + } + } + + /** + * Delegate for checking flag permissions. + */ + @VisibleForTesting + public static class PermissionsChecker { + private final Context mContext; + + @VisibleForTesting(visibility = VisibleForTesting.Visibility.PACKAGE) + public PermissionsChecker(Context context) { + mContext = context; + } + + /** + * Ensures that the caller has {@link SYNC_FLAGS} permission. + */ + @VisibleForTesting(visibility = VisibleForTesting.Visibility.PACKAGE) + public void assertSyncPermission() { + if (mContext.checkCallingOrSelfPermission(SYNC_FLAGS) + != PackageManager.PERMISSION_GRANTED) { + throw new SecurityException( + "Non-core flag queried. Requires SYNC_FLAGS permission!"); + } + } + + /** + * Ensures that the caller has {@link WRITE_FLAGS} permission. + */ + @VisibleForTesting(visibility = VisibleForTesting.Visibility.PACKAGE) + public void assertWritePermission() { + if (mContext.checkCallingPermission(WRITE_FLAGS) != PackageManager.PERMISSION_GRANTED) { + throw new SecurityException("Requires WRITE_FLAGS permission!"); + } + } + } } diff --git a/services/flags/java/com/android/server/flags/FlagOverrideStore.java b/services/flags/java/com/android/server/flags/FlagOverrideStore.java index 9866b1c7bb1cd..b1ddc7e67f68c 100644 --- a/services/flags/java/com/android/server/flags/FlagOverrideStore.java +++ b/services/flags/java/com/android/server/flags/FlagOverrideStore.java @@ -53,7 +53,8 @@ public class FlagOverrideStore { } /** Put a value in the store. */ - void set(String namespace, String name, String value) { + @VisibleForTesting + public void set(String namespace, String name, String value) { mSettingsProxy.putString(getPropName(namespace, name), value); mCallback.onFlagChanged(namespace, name, value); } @@ -65,7 +66,8 @@ public class FlagOverrideStore { } /** Erase a value from the store. */ - void erase(String namespace, String name) { + @VisibleForTesting + public void erase(String namespace, String name) { set(namespace, name, null); } diff --git a/services/tests/servicestests/src/com/android/server/flags/FeatureFlagsServiceTest.java b/services/tests/servicestests/src/com/android/server/flags/FeatureFlagsServiceTest.java index 8455b88ae9aff..df4731fb0bb71 100644 --- a/services/tests/servicestests/src/com/android/server/flags/FeatureFlagsServiceTest.java +++ b/services/tests/servicestests/src/com/android/server/flags/FeatureFlagsServiceTest.java @@ -21,6 +21,7 @@ import static com.google.common.truth.Truth.assertWithMessage; import static org.junit.Assert.fail; import static org.mockito.Mockito.any; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.eq; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -60,13 +61,16 @@ public class FeatureFlagsServiceTest { private IFeatureFlagsCallback mIFeatureFlagsCallback; @Mock private IBinder mIFeatureFlagsCallbackAsBinder; + @Mock + private FeatureFlagsService.PermissionsChecker mPermissionsChecker; private FeatureFlagsBinder mFeatureFlagsService; @Before public void setup() { when(mIFeatureFlagsCallback.asBinder()).thenReturn(mIFeatureFlagsCallbackAsBinder); - mFeatureFlagsService = new FeatureFlagsBinder(mFlagStore, mFlagCommand); + mFeatureFlagsService = new FeatureFlagsBinder( + mFlagStore, mFlagCommand, mPermissionsChecker); } @Test @@ -79,6 +83,40 @@ public class FeatureFlagsServiceTest { } } + @Test + public void testOverrideFlag_requiresWritePermission() { + SecurityException exc = new SecurityException("not allowed"); + doThrow(exc).when(mPermissionsChecker).assertWritePermission(); + + SyncableFlag f = new SyncableFlag(NS, "a", "false", false); + + try { + mFeatureFlagsService.overrideFlag(f); + fail("Should have thrown exception"); + } catch (SecurityException e) { + assertThat(exc).isEqualTo(e); + } catch (Exception e) { + fail("should have thrown a security exception"); + } + } + + @Test + public void testResetFlag_requiresWritePermission() { + SecurityException exc = new SecurityException("not allowed"); + doThrow(exc).when(mPermissionsChecker).assertWritePermission(); + + SyncableFlag f = new SyncableFlag(NS, "a", "false", false); + + try { + mFeatureFlagsService.resetFlag(f); + fail("Should have thrown exception"); + } catch (SecurityException e) { + assertThat(exc).isEqualTo(e); + } catch (Exception e) { + fail("should have thrown a security exception"); + } + } + @Test public void testSyncFlags_noOverrides() { List inputFlags = List.of( @@ -137,7 +175,6 @@ public class FeatureFlagsServiceTest { } } - @Test public void testSyncFlags_twoCallsWithDifferentDefaults() { List inputFlagsFirst = List.of( @@ -171,6 +208,83 @@ public class FeatureFlagsServiceTest { .that(found).isTrue(); } + @Test + public void testQueryFlags_onlyOnce() { + List inputFlags = List.of( + new SyncableFlag(NS, "a", "false", false), + new SyncableFlag(NS, "b", "true", false), + new SyncableFlag(NS, "c", "false", false) + ); + + List outputFlags = mFeatureFlagsService.queryFlags(inputFlags); + + assertThat(inputFlags.size()).isEqualTo(outputFlags.size()); + + for (SyncableFlag inpF: inputFlags) { + boolean found = false; + for (SyncableFlag outF : outputFlags) { + if (compareSyncableFlagsNames(inpF, outF)) { + found = true; + break; + } + } + assertWithMessage("Failed to find input flag " + inpF + " in the output") + .that(found).isTrue(); + } + } + + @Test + public void testQueryFlags_twoCallsWithDifferentDefaults() { + List inputFlagsFirst = List.of( + new SyncableFlag(NS, "a", "false", false) + ); + List inputFlagsSecond = List.of( + new SyncableFlag(NS, "a", "true", false), + new SyncableFlag(NS, "b", "false", false) + ); + + List outputFlagsFirst = mFeatureFlagsService.queryFlags(inputFlagsFirst); + List outputFlagsSecond = mFeatureFlagsService.queryFlags(inputFlagsSecond); + + assertThat(inputFlagsFirst.size()).isEqualTo(outputFlagsFirst.size()); + assertThat(inputFlagsSecond.size()).isEqualTo(outputFlagsSecond.size()); + + // This test only cares that the "a" flag passed in the second time came out with the + // same value that was passed in (i.e. it wasn't cached). + + boolean found = false; + for (SyncableFlag second : outputFlagsSecond) { + if (compareSyncableFlagsNames(second, inputFlagsSecond.get(0))) { + found = true; + assertThat(second.getValue()).isEqualTo(inputFlagsSecond.get(0).getValue()); + break; + } + } + + assertWithMessage( + "Failed to find flag " + inputFlagsSecond.get(0) + " in the second calls output") + .that(found).isTrue(); + } + + @Test + public void testOverrideFlag() { + SyncableFlag f = new SyncableFlag(NS, "a", "false", false); + + mFeatureFlagsService.overrideFlag(f); + + verify(mFlagStore).set(f.getNamespace(), f.getName(), f.getValue()); + } + + @Test + public void testResetFlag() { + SyncableFlag f = new SyncableFlag(NS, "a", "false", false); + + mFeatureFlagsService.resetFlag(f); + + verify(mFlagStore).erase(f.getNamespace(), f.getName()); + } + + private static boolean compareSyncableFlagsNames(SyncableFlag a, SyncableFlag b) { return a.getNamespace().equals(b.getNamespace()) && a.getName().equals(b.getName()) From 9cf5bb454e057577798fed95c938e2b94f665597 Mon Sep 17 00:00:00 2001 From: Dave Mankoff Date: Wed, 12 Jul 2023 19:32:11 +0000 Subject: [PATCH 6/6] Revert^2 "Allow flags to add metadata directly in code." d7d04dccb9b91a426a7a291bc1e4ef0728e3b361 Bug: 279054964 Change-Id: I8cf53ca92628366332c767a9b1709ba989c7b972 --- core/java/android/flags/BooleanFlag.java | 6 +++ core/java/android/flags/BooleanFlagBase.java | 28 ++++++++++++++ .../android/flags/DynamicBooleanFlag.java | 6 +++ core/java/android/flags/Flag.java | 37 +++++++++++++++++++ core/java/android/flags/FusedOffFlag.java | 6 +++ core/java/android/flags/FusedOnFlag.java | 6 +++ 6 files changed, 89 insertions(+) diff --git a/core/java/android/flags/BooleanFlag.java b/core/java/android/flags/BooleanFlag.java index ae9ccf8c52c36..d4a35b25f6236 100644 --- a/core/java/android/flags/BooleanFlag.java +++ b/core/java/android/flags/BooleanFlag.java @@ -43,4 +43,10 @@ public class BooleanFlag extends BooleanFlagBase { public Boolean getDefault() { return mDefault; } + + @Override + public BooleanFlag defineMetaData(String label, String description, String categoryName) { + super.defineMetaData(label, description, categoryName); + return this; + } } diff --git a/core/java/android/flags/BooleanFlagBase.java b/core/java/android/flags/BooleanFlagBase.java index f4141ecab01b1..985dbe3f2f01e 100644 --- a/core/java/android/flags/BooleanFlagBase.java +++ b/core/java/android/flags/BooleanFlagBase.java @@ -22,6 +22,9 @@ abstract class BooleanFlagBase implements Flag { private final String mNamespace; private final String mName; + private String mLabel; + private String mDescription; + private String mCategoryName; /** * @param namespace A namespace for this flag. See {@link android.provider.DeviceConfig}. @@ -30,6 +33,7 @@ abstract class BooleanFlagBase implements Flag { BooleanFlagBase(String namespace, String name) { mNamespace = namespace; mName = name; + mLabel = name; } public abstract Boolean getDefault(); @@ -46,6 +50,30 @@ abstract class BooleanFlagBase implements Flag { return mName; } + @Override + public BooleanFlagBase defineMetaData(String label, String description, String categoryName) { + mLabel = label; + mDescription = description; + mCategoryName = categoryName; + return this; + } + + @Override + @NonNull + public String getLabel() { + return mLabel; + } + + @Override + public String getDescription() { + return mDescription; + } + + @Override + public String getCategoryName() { + return mCategoryName; + } + @Override @NonNull public String toString() { diff --git a/core/java/android/flags/DynamicBooleanFlag.java b/core/java/android/flags/DynamicBooleanFlag.java index 92009c60d91a1..271a8c5f4d15f 100644 --- a/core/java/android/flags/DynamicBooleanFlag.java +++ b/core/java/android/flags/DynamicBooleanFlag.java @@ -41,4 +41,10 @@ public class DynamicBooleanFlag extends BooleanFlagBase implements DynamicFlag { default boolean isDynamic() { return false; } + + /** + * Add human-readable details to the flag. Flag client's are not required to set this. + * + * See {@link #getLabel()}, {@link #getDescription()}, and {@link #getCategoryName()}. + * + * @return Returns `this`, to make a fluent api. + */ + Flag defineMetaData(String label, String description, String categoryName); + + /** + * A human-readable name for the flag. Defaults to {@link #getName()} + * + * See {@link #defineMetaData(String, String, String)} + */ + @NonNull + default String getLabel() { + return getName(); + } + + /** + * A human-readable description for the flag. Defaults to null if unset. + * + * See {@link #defineMetaData(String, String, String)} + */ + default String getDescription() { + return null; + } + + /** + * A human-readable category name for the flag. Defaults to null if unset. + * + * See {@link #defineMetaData(String, String, String)} + */ + default String getCategoryName() { + return null; + } } diff --git a/core/java/android/flags/FusedOffFlag.java b/core/java/android/flags/FusedOffFlag.java index 5f435beb26421..6844b8faafef0 100644 --- a/core/java/android/flags/FusedOffFlag.java +++ b/core/java/android/flags/FusedOffFlag.java @@ -40,4 +40,10 @@ public final class FusedOffFlag extends BooleanFlagBase { public Boolean getDefault() { return false; } + + @Override + public FusedOffFlag defineMetaData(String label, String description, String categoryName) { + super.defineMetaData(label, description, categoryName); + return this; + } } diff --git a/core/java/android/flags/FusedOnFlag.java b/core/java/android/flags/FusedOnFlag.java index ea8d9b29b3cec..e9adba7595c11 100644 --- a/core/java/android/flags/FusedOnFlag.java +++ b/core/java/android/flags/FusedOnFlag.java @@ -40,4 +40,10 @@ public final class FusedOnFlag extends BooleanFlagBase { public Boolean getDefault() { return true; } + + @Override + public FusedOnFlag defineMetaData(String label, String description, String categoryName) { + super.defineMetaData(label, description, categoryName); + return this; + } }