Add dynamic module support to Firestore #2432

dconeybe · 2021-02-10T19:19:10Z

Context: https://developer.android.com/guide/app-bundle/play-feature-delivery

Note to Googlers: This work is tracked by b/177833559.

…s 18.0.0)

Note that testRestartFirestoreLeadsToNewInstance() fails with this change; fix this.

…and Firestore_Unit_Tests

…tBug177833559

…gration_Tests__Firestore_Emulator_.xml that weren't meant to be pushed up

…eferred interface

…synchronized' modifier to removeChangeListener()

google-oss-bot · 2021-02-10T19:33:30Z

Coverage Report

Affected SDKs

`firebase-firestore`

SDK overall coverage changed from 48.64% (f56a977) to 48.72% (3b36cfc4) by +0.08%.

Filename	Base (`f56a977`)	Head (3b36cfc4)	Diff
AbstractStream.java	36.90%	34.52%	-2.38%
AsyncQueue.java	77.89%	76.88%	-1.01%
FirebaseAuthCredentialsProvider.java	41.67%	100.00%	+58.33%
FirebaseFirestore.java	44.08%	42.95%	-1.13%

Test Logs

Notes

HTML coverage reports can be produced locally with ./gradlew <product>:checkCoverage.
Report files are located at <product-build-dir>/reports/jacoco/.

Head commit (3b36cfc4) is created by Prow via merging commits: f56a977 90403bc.

google-oss-bot · 2021-02-10T19:37:54Z

Binary Size Report

Affected SDKs

firebase-firestore

Type Base (f56a977) Head (3b36cfc4) Diff

aar 1.10 MB 1.10 MB +92 B (+0.0%)

apk (aggressive) 438 kB 438 kB +216 B (+0.0%)

apk (release) 3.19 MB 3.19 MB -56 B (-0.0%)

Test Logs

Notes

Head commit (3b36cfc4) is created by Prow via merging commits: f56a977 90403bc.

vkryachko · 2021-02-10T19:38:19Z

firebase-components/src/main/java/com/google/firebase/inject/Deferred.java

  /** Used by dependers to register their callbacks. */
  interface DeferredHandler<T> {
    @DeferredApi
-    void handle(Provider<T> provider);
+    void handle(T instance);


I would like to avoid this, since for one it's a breaking change and also is less general and breaks laziness(if eagerness is not required by other products).

What is this achieving other than removing the need to call get() once?

The main advantage is indeed the removal of the need to call get(): instead of giving you an indirect reference to the data it gives you the data directly. When I used this code I first thought "why isn't it just giving me the instance? why is it giving me a provider to the instance?" I see your points about lazy initialization though.

What if the argument to handle() is changed from Provider<T> to Deferred<T> and the this instance is passed as the argument? Since Deferred now extends Provider this provides one authoritative way to get the instance: call get() on the Deferred object.

Also, since this is an internal-only API (and I don't see any products using Deferred yet), could you explain the concerns about this being a breaking change?

Note: The discussion about modifying Deferred to extend Provider occurred here: #2418 (review)

I feel like Deferred and Provider are completely disjoint concepts - even though related in the context of dependency injection. The fact that OptionalProvider implements both is just an internal implementation detail that I would prefer not to leak into the interface.

What if the argument to handle() is changed from Provider to Deferred and the this instance is passed as the argument? Since Deferred now extends Provider this provides one authoritative way to get the instance: call get() on the Deferred object.

I guess we could do that, but I don't see the benefits of this approach, afaict it's completely isomorphic to the existing interface however:

has the downside I described above

provides 2 ways of doing the same thing, i.e. calling get on the deferred itself or on the provider passed into handle()

constrains the implementation of deferred as its contract becomes more complicated.

Looks like rtdb's change has no problem with deferred not extending provider and instead uses an atomic reference to determine if auth is present or not, can we do the same in firestore?

I've reverted the changes to Deferred and OptionalProvider. It is indeed possible to use Deferred without the changes I suggested and I've made those changes as well.

…review feedback

…andle(T) to handle(Deferred<T>)

…tBug177833559 at 0d2d21e

dconeybe · 2021-02-10T21:11:41Z

/test device-check-changed

dconeybe · 2021-02-10T22:03:23Z

/test device-check-changed

dconeybe · 2021-02-11T05:48:37Z

/test device-check-changed

…xcept for one

schmidt-sebastian · 2021-02-18T20:41:42Z

...estore/src/main/java/com/google/firebase/firestore/auth/FirebaseAuthCredentialsProvider.java

+            internalAuthProviderProviderRef.set(provider);
+            currentUser = getUser(internalAuthProviderProviderRef.get());
+            if (changeListener != null) {
+              changeListener.onValue(currentUser);


The changeListener only enqueues work on our internal queue. I think it is okay to execute on the main thread.

schmidt-sebastian · 2021-02-18T20:42:24Z

...estore/src/main/java/com/google/firebase/firestore/auth/FirebaseAuthCredentialsProvider.java

+
+    InternalAuthProvider internalAuthProvider = internalAuthProviderProviderRef.get().get();
+    if (internalAuthProvider == null) {
+      return Tasks.forException(new FirebaseApiNotAvailableException("auth is not available"));


This should fall back to "unauthenticated" auth and return null.

schmidt-sebastian · 2021-02-18T20:43:41Z

...estore/src/main/java/com/google/firebase/firestore/auth/FirebaseAuthCredentialsProvider.java

  }

  @Override
  public synchronized Task<String> getToken() {
    boolean doForceRefresh = forceRefresh;
    forceRefresh = false;
-    Task<GetTokenResult> res = authProvider.getAccessToken(doForceRefresh);
+


Hm, do we know what this dance above is for?

I mean, I understand what it's doing in this class but I'm not sure how it fits into the bigger picture of auth in firestore. Since it was pre-existing code, I'm leaving the semantics as-is.

Can you inline doForceRefresh?

What do you mean by "inline"?

You can ignore this since this is existing code, but I can't figure out why we need this extra variable. By inlining I mean replace "doForceRefresh" with "forceRefresh" (and setting "forceRefresh" to false a bit later).

I can't figure it out either. The only difference it would make would be if internalAuthProvider.getAccessToken() throws an exception, which seems like a pretty extreme case against which I doubt we need to guard. I've implemented this "inlining" of doForceRefresh.

schmidt-sebastian · 2021-02-18T20:44:44Z

...estore/src/main/java/com/google/firebase/firestore/auth/FirebaseAuthCredentialsProvider.java

-    @Nullable String uid = authProvider.getUid();
+  /** Returns the current {@link User} as obtained from the given InternalAuthProvider. */
+  private static User getUser(Provider<InternalAuthProvider> provider) {
+    InternalAuthProvider internalAuthProvider = provider.get();


Do we need to pass this in as a reference?

I had changed the getUser() method to a static method to make it explicit at the call sites that it depended on the instance of InternalAuthProvider. I've since changed it back to a non-static method that implicitly depends on this.internalAuthProviderProvider and got rid of the effectively-unused currentUser variable.

schmidt-sebastian · 2021-02-18T20:50:55Z

...estore/src/main/java/com/google/firebase/firestore/auth/FirebaseAuthCredentialsProvider.java

+  /** Returns the current {@link User} as obtained from the given InternalAuthProvider. */
+  private static User getUser(Provider<InternalAuthProvider> provider) {
+    InternalAuthProvider internalAuthProvider = provider.get();
+    @Nullable String uid = (internalAuthProvider == null) ? null : internalAuthProvider.getUid();


Is the internalAuthProvider ever null? Looks like you initialize it to a null returning provider above.

Yes, this.internalAuthProviderProvider.get() initially returns null; therefore, this null guard is required.

Never mind, I got confused because this is a ProviderProvider. I think this will be more obvious when this is just a Provider.

schmidt-sebastian · 2021-02-18T20:51:26Z

...estore/src/main/java/com/google/firebase/firestore/auth/FirebaseAuthCredentialsProvider.java

@@ -40,7 +44,7 @@

  private static final String LOG_TAG = "FirebaseAuthCredentialsProvider";

-  private final InternalAuthProvider authProvider;
+  private AtomicReference<Provider<InternalAuthProvider>> internalAuthProviderProviderRef;


Should we be mixing synchronized and AtomicReference in this file?

schmidt-sebastian · 2021-02-18T20:56:43Z

...re/src/test/java/com/google/firebase/firestore/auth/FirebaseAuthCredentialsProviderTest.java

+
+    assertThat(task.isComplete()).isTrue();
+    assertThat(task.isSuccessful()).isFalse();
+    assertThat(task.getException()).isInstanceOf(FirebaseApiNotAvailableException.class);


As discussed above, this should return a successful Task with a result of null.

I'll update this test based on the result of the discussion about FirebaseApiNotAvailableException in another comment.

"I won't update the test because my code is correct".

Came up with an answer for you.

schmidt-sebastian · 2021-02-18T21:01:14Z

...re/src/test/java/com/google/firebase/firestore/auth/FirebaseAuthCredentialsProviderTest.java

+  }
+
+  /** An implementation of {@link Deferred} whose provider is always available. */
+  private static final class ImmediateDeferred implements Deferred<InternalAuthProvider> {


Would it make sense to move these classes to separate package or utility class, so that they can be used from both test files? We could have something similar to the Tasks class with "Deferred.forX()" style factory methods.

I also wonder if ImmediateDeferred should be part of firebase-common.

Done. I've moved these 3 Deferred implementations into the testUtil package. I'm slightly against the Deferred.forX() factory methods because they add a layer of indirection at the call site. But I could definitely be swayed since it would give consistency with the Tasks class.

I also wonder if ImmediateDeferred should be part of firebase-common.

What would be your rationale for doing this move?

schmidt-sebastian · 2021-02-18T21:04:17Z

...re/src/test/java/com/google/firebase/firestore/auth/FirebaseAuthCredentialsProviderTest.java

+    private final AtomicReference<Provider<InternalAuthProvider>> internalAuthProviderProviderRef =
+        new AtomicReference<>();
+
+    @Override
+    public void whenAvailable(@NonNull DeferredHandler<InternalAuthProvider> handler) {
+      Provider<InternalAuthProvider> internalAuthProviderProvider;
+      synchronized (lock) {


Again, I think we should not be mixing synchronized and AtomicReference.

Since this is a test class, you could also synchronize on the object itself and use public synchronized void whenAvailable() as the method signature.

I also wonder if these methods need to be synchronized at all. Does Mockito invoke the methods from a different thread?

Agreed. I've changed the methods to just be declared as "synchronized". The synchronization is probably not strictly required; however, for correctness and to cover future use cases that may call whenAvailable from other threads, I'm making the class thread safe now.

…tBug177833559

dconeybe · 2021-02-19T21:04:48Z

/test smoke-tests

schmidt-sebastian

I added some more comments (after guessing what your reply on my last round was :) )

schmidt-sebastian · 2021-02-22T16:53:50Z

...estore/src/main/java/com/google/firebase/firestore/auth/FirebaseAuthCredentialsProvider.java

  }

  @Override
  public synchronized Task<String> getToken() {
    boolean doForceRefresh = forceRefresh;
    forceRefresh = false;
-    Task<GetTokenResult> res = authProvider.getAccessToken(doForceRefresh);
+


Can you inline doForceRefresh?

schmidt-sebastian · 2021-02-22T16:54:45Z

...estore/src/main/java/com/google/firebase/firestore/auth/FirebaseAuthCredentialsProvider.java

+
+    InternalAuthProvider internalAuthProvider = internalAuthProviderProviderRef.get().get();
+    if (internalAuthProvider == null) {
+      return Tasks.forException(new FirebaseApiNotAvailableException("auth is not available"));


Please fix.

schmidt-sebastian · 2021-02-22T16:56:28Z

...estore/src/main/java/com/google/firebase/firestore/auth/FirebaseAuthCredentialsProvider.java

+   * its {@link Provider#get} method returns {@code null}, but will be changed to a new {@link
+   * Provider} once the "auth" module becomes available.
+   */
+  @Nullable


This is currently not nullable.

schmidt-sebastian · 2021-02-22T16:57:35Z

...estore/src/main/java/com/google/firebase/firestore/auth/FirebaseAuthCredentialsProvider.java

+   */
+  @Nullable
+  @GuardedBy("this")
+  private Provider<InternalAuthProvider> internalAuthProviderProvider = () -> null;


I think you could simplify your code if this was just a InternalAuthProvider. You can "unbox" the Provider in whenAvailable.

You are correct; however, there is a custom lint check that complains when a "deferred" object is saved to an instance variable. Storing the Provider makes the lint check happy.

The lint error is "Provider.get() assignment to a field detected" (

firebase-android-sdk/tools/lint/src/main/kotlin/ProviderAssignmentDetector.kt

Lines 47 to 50 in f56a977

context.report(

INVALID_PROVIDER_ASSIGNMENT,

context.getCallLocation(node, includeReceiver = false, includeArguments = true),

"Provider.get() assignment to a field detected.")

)

hm, it seems that this lint check is too strict in this case, meaning that it is not unreasonable to assign to a field from a @DeferredApi method - which the whenAvailable() callback lambda is an instance of. Please file a bug at go/fireescape-bug and feel free to @SuppressLint it or leave your code as is for now.

Done. I've changed the instance variable to simply @Nullable private InternalAuthProvider internalAuthProvider, added a SuppressLint to the constructor, and filed b/181014061 about the the erroneous lint check.

schmidt-sebastian · 2021-02-22T17:00:19Z

...estore/src/main/java/com/google/firebase/firestore/auth/FirebaseAuthCredentialsProvider.java

+  /** Returns the current {@link User} as obtained from the given InternalAuthProvider. */
+  private static User getUser(Provider<InternalAuthProvider> provider) {
+    InternalAuthProvider internalAuthProvider = provider.get();
+    @Nullable String uid = (internalAuthProvider == null) ? null : internalAuthProvider.getUid();


Never mind, I got confused because this is a ProviderProvider. I think this will be more obvious when this is just a Provider.

…tation from internalAuthProviderProvider

dconeybe · 2021-02-22T20:07:31Z

/test device-check-changed

Deferred use lgtm

schmidt-sebastian

This LGTM, but I do think we should use SuppressLint and get rid of the ProviderProvider.

schmidt-sebastian · 2021-02-23T04:04:24Z

...estore/src/main/java/com/google/firebase/firestore/auth/FirebaseAuthCredentialsProvider.java

  }

  @Override
  public synchronized Task<String> getToken() {
    boolean doForceRefresh = forceRefresh;
    forceRefresh = false;
-    Task<GetTokenResult> res = authProvider.getAccessToken(doForceRefresh);
+


You can ignore this since this is existing code, but I can't figure out why we need this extra variable. By inlining I mean replace "doForceRefresh" with "forceRefresh" (and setting "forceRefresh" to false a bit later).

schmidt-sebastian · 2021-02-23T04:06:42Z

...estore/src/main/java/com/google/firebase/firestore/auth/FirebaseAuthCredentialsProvider.java

+
+    InternalAuthProvider internalAuthProvider = internalAuthProviderProviderRef.get().get();
+    if (internalAuthProvider == null) {
+      return Tasks.forException(new FirebaseApiNotAvailableException("auth is not available"));


Your answer wasn't quite what I predicted. You have changed my mind. I was expecting the consumer to be a bit simpler than it actually is. Please disregard.

schmidt-sebastian · 2021-02-23T04:07:22Z

...re/src/test/java/com/google/firebase/firestore/auth/FirebaseAuthCredentialsProviderTest.java

+
+    assertThat(task.isComplete()).isTrue();
+    assertThat(task.isSuccessful()).isFalse();
+    assertThat(task.getException()).isInstanceOf(FirebaseApiNotAvailableException.class);


"I won't update the test because my code is correct".

Came up with an answer for you.

…tBug177833559

… InternalAuthProvider internalAuthProvider

dconeybe added 6 commits February 5, 2021 15:31

Upgrade Firestore's dependency on firebase-auth-interop to 19.0.2 (wa…

bbf7faa

…s 18.0.0)

Replace InternalAuthProvider with Deferred<InternalAuthProvider>.

e0d512a

Note that testRestartFirestoreLeadsToNewInstance() fails with this change; fix this.

Fix Android Studio run configurations for Firestore_Integration_Test …

a9984aa

…and Firestore_Unit_Tests

A bit of work

5ecb626

Fix FirebaseAuthCredentialsProvider

f28f823

Merge remote-tracking branch 'origin/master' into DynamicModuleSuppor…

cbdc18d

…tBug177833559

googlebot added the cla: yes Override cla label Feb 10, 2021

google-oss-bot added the size/L label Feb 10, 2021

dconeybe added 4 commits February 10, 2021 14:20

Undo some changes in EmptyCredentialsProvider.java and Firestore_Inte…

2a32b7a

…gration_Tests__Firestore_Emulator_.xml that weren't meant to be pushed up

FirestoreMultiDbComponentTest.java: Fix build due to changes to the D…

38d236a

…eferred interface

Format code by running ./gradlew googleJavaFormat

e52d063

FirebaseAuthCredentialsProvider.java: add back accidentally-removed '…

1a750cb

…synchronized' modifier to removeChangeListener()

google-oss-bot added size/M and removed size/L labels Feb 10, 2021

dconeybe mentioned this pull request Feb 10, 2021

Make Auth an deferred dependency #2418

Merged

vkryachko requested changes Feb 10, 2021

View reviewed changes

dconeybe added 3 commits February 10, 2021 14:52

Deferred.java: Change handle(T) to handle(Deferred<T>) based on code …

306fe80

…review feedback

FirestoreMultiDbComponentTest.java: fix build error due to changing h…

7a83a63

…andle(T) to handle(Deferred<T>)

Merge remote-tracking branch 'origin/master' into DynamicModuleSuppor…

dc839de

…tBug177833559 at 0d2d21e

Started unit tests for FirebaseAuthCredentialsProvider

c049e31

google-oss-bot added size/L and removed size/M labels Feb 11, 2021

dconeybe added 3 commits February 11, 2021 14:28

FirebaseAuthCredentialsProviderTest.java: completed the unit tests, e…

28e3713

…xcept for one

FirebaseAuthCredentialsProviderTest.java: completed unit tests

d55cf30

format code

991f35e

schmidt-sebastian reviewed Feb 18, 2021

View reviewed changes

schmidt-sebastian assigned dconeybe and unassigned schmidt-sebastian Feb 18, 2021

dconeybe added 2 commits February 19, 2021 13:32

Merge remote-tracking branch 'origin/master' into DynamicModuleSuppor…

5739d32

…tBug177833559

Applied code review feedback

05d3fb9

google-oss-bot added size/XL and removed size/L labels Feb 19, 2021

dconeybe requested a review from schmidt-sebastian February 20, 2021 19:32

dconeybe assigned vkryachko and schmidt-sebastian and unassigned dconeybe Feb 20, 2021

schmidt-sebastian reviewed Feb 22, 2021

View reviewed changes

schmidt-sebastian assigned dconeybe and unassigned schmidt-sebastian Feb 22, 2021

FirebaseAuthCredentialsProvider.java: Remove incorrect @nullable anno…

a435e3b

…tation from internalAuthProviderProvider

dconeybe assigned schmidt-sebastian and unassigned dconeybe Feb 22, 2021

dconeybe requested a review from schmidt-sebastian February 22, 2021 19:46

schmidt-sebastian approved these changes Feb 23, 2021

View reviewed changes

schmidt-sebastian assigned dconeybe and unassigned schmidt-sebastian Feb 23, 2021

dconeybe added 3 commits February 23, 2021 09:59

Merge remote-tracking branch 'origin/master' into DynamicModuleSuppor…

7c0bb34

…tBug177833559

Change Provider<InternalAuthProvider> internalAuthProviderProvider to…

be0a08e

… InternalAuthProvider internalAuthProvider

Simplify the unnecessarily-complicated dance

90403bc

dconeybe merged commit f54d96a into master Feb 23, 2021

dconeybe deleted the dconeybe/DynamicModuleSupportBug177833559 branch February 23, 2021 16:04

firebase locked and limited conversation to collaborators Mar 26, 2021

	context.report(
	INVALID_PROVIDER_ASSIGNMENT,
	context.getCallLocation(node, includeReceiver = false, includeArguments = true),
	"Provider.get() assignment to a field detected.")

Add dynamic module support to Firestore #2432

Add dynamic module support to Firestore #2432

Uh oh!

Conversation

dconeybe commented Feb 10, 2021

Uh oh!

google-oss-bot commented Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report

Affected SDKs

firebase-firestore

Test Logs

Notes

Uh oh!

google-oss-bot commented Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

firebase-firestore

Test Logs

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dconeybe commented Feb 10, 2021

Uh oh!

dconeybe commented Feb 10, 2021

Uh oh!

dconeybe commented Feb 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-oss-bot commented Feb 10, 2021 •

edited

Loading

`firebase-firestore`

google-oss-bot commented Feb 10, 2021 •

edited

Loading

`firebase-firestore`