Skip to content

Make ABT Dynamic-Module compliant. #2524

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import androidx.annotation.WorkerThread;
import com.google.firebase.analytics.connector.AnalyticsConnector;
import com.google.firebase.analytics.connector.AnalyticsConnector.ConditionalUserProperty;
import com.google.firebase.inject.Provider;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.util.ArrayDeque;
Expand Down Expand Up @@ -56,7 +57,7 @@ public class FirebaseABTesting {
static final String ORIGIN_LAST_KNOWN_START_TIME_KEY_FORMAT = "%s_lastKnownExperimentStartTime";

/** The App's Firebase Analytics client. */
private final AnalyticsConnector analyticsConnector;
private final Provider<AnalyticsConnector> analyticsConnector;

/** The name of an ABT client. */
private final String originService;
Expand Down Expand Up @@ -89,7 +90,7 @@ public class FirebaseABTesting {
*/
public FirebaseABTesting(
Context unusedAppContext,
AnalyticsConnector analyticsConnector,
Provider<AnalyticsConnector> analyticsConnector,
@OriginService String originService) {
this.analyticsConnector = analyticsConnector;
this.originService = originService;
Expand Down Expand Up @@ -333,11 +334,11 @@ private static List<AbtExperimentInfo> convertMapsToExperimentInfos(
}

private void addExperimentToAnalytics(ConditionalUserProperty experiment) {
analyticsConnector.setConditionalUserProperty(experiment);
analyticsConnector.get().setConditionalUserProperty(experiment);
}

private void throwAbtExceptionIfAnalyticsIsNull() throws AbtException {
if (analyticsConnector == null) {
if (analyticsConnector.get() == null) {
throw new AbtException(
"The Analytics SDK is not available. "
+ "Please check that the Analytics SDK is included in your app dependencies.");
Expand All @@ -350,14 +351,16 @@ private void throwAbtExceptionIfAnalyticsIsNull() throws AbtException {
* breaking, or if the underlying Analytics clear method is failing.
*/
private void removeExperimentFromAnalytics(String experimentId) {
analyticsConnector.clearConditionalUserProperty(
experimentId, /*clearEventName=*/ null, /*clearEventParams=*/ null);
analyticsConnector
.get()
.clearConditionalUserProperty(
experimentId, /*clearEventName=*/ null, /*clearEventParams=*/ null);
}

@WorkerThread
private int getMaxUserPropertiesInAnalytics() {
if (maxUserProperties == null) {
maxUserProperties = analyticsConnector.getMaxUserProperties(originService);
maxUserProperties = analyticsConnector.get().getMaxUserProperties(originService);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking: @WorkerThread methods don't need to check for null?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check is not required for the same reason you mentioned below, the check is done prior to calling this method

}
return maxUserProperties;
}
Expand All @@ -370,7 +373,8 @@ private int getMaxUserPropertiesInAnalytics() {
*/
@WorkerThread
private List<ConditionalUserProperty> getAllExperimentsInAnalytics() {
return analyticsConnector.getConditionalUserProperties(
originService, /*propertyNamePrefix=*/ "");
return analyticsConnector
.get()
.getConditionalUserProperties(originService, /*propertyNamePrefix=*/ "");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.google.firebase.abt.FirebaseABTesting;
import com.google.firebase.abt.FirebaseABTesting.OriginService;
import com.google.firebase.analytics.connector.AnalyticsConnector;
import com.google.firebase.inject.Provider;
import java.util.HashMap;
import java.util.Map;

Expand All @@ -37,11 +38,11 @@ public class AbtComponent {
private final Map<String, FirebaseABTesting> abtOriginInstances = new HashMap<>();

private final Context appContext;
private final AnalyticsConnector analyticsConnector;
private final Provider<AnalyticsConnector> analyticsConnector;

/** Firebase ABT Component constructor. */
@VisibleForTesting(otherwise = VisibleForTesting.PACKAGE_PRIVATE)
protected AbtComponent(Context appContext, AnalyticsConnector analyticsConnector) {
protected AbtComponent(Context appContext, Provider<AnalyticsConnector> analyticsConnector) {
this.appContext = appContext;
this.analyticsConnector = analyticsConnector;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,12 @@ public List<Component<?>> getComponents() {
return Arrays.asList(
Component.builder(AbtComponent.class)
.add(Dependency.required(Context.class))
.add(Dependency.optional(AnalyticsConnector.class))
.add(Dependency.optionalProvider(AnalyticsConnector.class))
.factory(
container ->
new AbtComponent(
container.get(Context.class), container.get(AnalyticsConnector.class)))
container.get(Context.class),
container.getProvider(AnalyticsConnector.class)))
.build(),
LibraryVersionComponent.create("fire-abt", BuildConfig.VERSION_NAME));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ public void setUp() {
firebaseAbt =
new FirebaseABTesting(
/*unusedAppContext=*/ null,
/*analyticsConnector=*/ mockAnalyticsConnector,
/*analyticsConnector=*/ () -> mockAnalyticsConnector,
ORIGIN_SERVICE);

when(mockAnalyticsConnector.getMaxUserProperties(ORIGIN_SERVICE))
Expand Down Expand Up @@ -189,7 +189,7 @@ public void replaceAllExperiments_totalExperimentsExceedsAnalyticsLimit_oldExper
public void replaceAllExperiments_analyticsSdkUnavailable_throwsAbtException() {
firebaseAbt =
new FirebaseABTesting(
/*unusedAppContext=*/ null, /*analyticsConnector=*/ null, ORIGIN_SERVICE);
/*unusedAppContext=*/ null, /*analyticsConnector=*/ () -> null, ORIGIN_SERVICE);

AbtException actualException =
assertThrows(
Expand Down Expand Up @@ -243,7 +243,7 @@ public void removeAllExperiments_existExperimentsInAnalytics_experimentsClearedF
public void removeAllExperiments_analyticsSdkUnavailable_throwsAbtException() {
firebaseAbt =
new FirebaseABTesting(
/*unusedAppContext=*/ null, /*analyticsConnector=*/ null, ORIGIN_SERVICE);
/*unusedAppContext=*/ null, /*analyticsConnector=*/ () -> null, ORIGIN_SERVICE);

AbtException actualException =
assertThrows(AbtException.class, () -> firebaseAbt.removeAllExperiments());
Expand Down Expand Up @@ -281,7 +281,7 @@ public void getAllExperiments_existExperimentsInAnalytics_returnAllExperiments()
public void getAllExperiments_analyticsSdkUnavailable_throwsAbtException() {
firebaseAbt =
new FirebaseABTesting(
/*unusedAppContext=*/ null, /*analyticsConnector=*/ null, ORIGIN_SERVICE);
/*unusedAppContext=*/ null, /*analyticsConnector=*/ () -> null, ORIGIN_SERVICE);

AbtException actualException =
assertThrows(AbtException.class, () -> firebaseAbt.getAllExperiments());
Expand Down
14 changes: 11 additions & 3 deletions tools/lint/src/main/kotlin/ProviderAssignmentDetector.kt
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,17 @@ class ProviderAssignmentDetector : Detector(), SourceCodeScanner {
if (!isProviderGet(method)) {
return
}
val assignmentTarget = node.getParentOfType<JavaUAssignmentExpression>(
JavaUAssignmentExpression::class.java,
true)?.leftOperand as? UReferenceExpression ?: return
val assignmentExpression = node
.getParentOfType<JavaUAssignmentExpression>(
JavaUAssignmentExpression::class.java, true)
val assignmentTarget = assignmentExpression?.leftOperand as? UReferenceExpression ?: return

// This would only be true if assigning the result of get(),
// in cases like foo = p.get().someMethod() there would be an intermediate parent
// and we don't want to trigger in such cases.
if (assignmentExpression != node.uastParent?.uastParent) {
return
}
assignmentTarget.resolve()?.let {
if (it is PsiField) {
context.report(
Expand Down
17 changes: 17 additions & 0 deletions tools/lint/src/test/kotlin/ProviderAssignmentDetectorTests.kt
Original file line number Diff line number Diff line change
Expand Up @@ -87,4 +87,21 @@ class ProviderAssignmentDetectorTests : LintDetectorTest() {
.run()
.expectClean()
}

fun test_assignmentOfAPropertyOrMethodOfTheProvidedObject_shouldSucceed() {
lint().files(java(providerSource()), java("""
import com.google.firebase.inject.Provider;

class Foo {
private final int length;
private final String s;
Foo(Provider<String> p) {
length = p.get().length;
s = p.get().toString();
}
}
""".trimIndent()))
.run()
.expectClean()
}
}