Skip to content

Add dynamic module support to Firestore #2432

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 27 commits into from
Feb 23, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
bbf7faa
Upgrade Firestore's dependency on firebase-auth-interop to 19.0.2 (wa…
dconeybe Feb 5, 2021
e0d512a
Replace InternalAuthProvider with Deferred<InternalAuthProvider>.
dconeybe Feb 5, 2021
a9984aa
Fix Android Studio run configurations for Firestore_Integration_Test …
dconeybe Feb 5, 2021
5ecb626
A bit of work
dconeybe Feb 9, 2021
f28f823
Fix FirebaseAuthCredentialsProvider
dconeybe Feb 10, 2021
cbdc18d
Merge remote-tracking branch 'origin/master' into DynamicModuleSuppor…
dconeybe Feb 10, 2021
2a32b7a
Undo some changes in EmptyCredentialsProvider.java and Firestore_Inte…
dconeybe Feb 10, 2021
38d236a
FirestoreMultiDbComponentTest.java: Fix build due to changes to the D…
dconeybe Feb 10, 2021
e52d063
Format code by running ./gradlew googleJavaFormat
dconeybe Feb 10, 2021
1a750cb
FirebaseAuthCredentialsProvider.java: add back accidentally-removed '…
dconeybe Feb 10, 2021
306fe80
Deferred.java: Change handle(T) to handle(Deferred<T>) based on code …
dconeybe Feb 10, 2021
7a83a63
FirestoreMultiDbComponentTest.java: fix build error due to changing h…
dconeybe Feb 10, 2021
dc839de
Merge remote-tracking branch 'origin/master' into DynamicModuleSuppor…
dconeybe Feb 10, 2021
c049e31
Started unit tests for FirebaseAuthCredentialsProvider
dconeybe Feb 11, 2021
28e3713
FirebaseAuthCredentialsProviderTest.java: completed the unit tests, e…
dconeybe Feb 11, 2021
d55cf30
FirebaseAuthCredentialsProviderTest.java: completed unit tests
dconeybe Feb 11, 2021
991f35e
format code
dconeybe Feb 11, 2021
72fc8cb
Merge remote-tracking branch 'origin/master' into DynamicModuleSuppor…
dconeybe Feb 16, 2021
db16188
Merge remote-tracking branch 'origin/master' into DynamicModuleSuppor…
dconeybe Feb 18, 2021
6cd946f
Revert changes to OptionalProvider.java and Deferred.java
dconeybe Feb 18, 2021
6829deb
Fix the build break that resulting from reverting changes to Deferred
dconeybe Feb 18, 2021
5739d32
Merge remote-tracking branch 'origin/master' into DynamicModuleSuppor…
dconeybe Feb 19, 2021
05d3fb9
Applied code review feedback
dconeybe Feb 19, 2021
a435e3b
FirebaseAuthCredentialsProvider.java: Remove incorrect @Nullable anno…
dconeybe Feb 22, 2021
7c0bb34
Merge remote-tracking branch 'origin/master' into DynamicModuleSuppor…
dconeybe Feb 23, 2021
be0a08e
Change Provider<InternalAuthProvider> internalAuthProviderProvider to…
dconeybe Feb 23, 2021
90403bc
Simplify the unnecessarily-complicated dance
dconeybe Feb 23, 2021
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
2 changes: 1 addition & 1 deletion firebase-firestore/firebase-firestore.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ dependencies {
implementation 'com.google.android.gms:play-services-base:17.0.0'

implementation 'com.squareup.okhttp:okhttp:2.7.5'
implementation('com.google.firebase:firebase-auth-interop:18.0.0') {
implementation('com.google.firebase:firebase-auth-interop:19.0.2') {
exclude group: "com.google.firebase", module: "firebase-common"
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,10 @@
import androidx.test.core.app.ApplicationProvider;
import androidx.test.ext.junit.runners.AndroidJUnit4;
import com.google.android.gms.tasks.Task;
import com.google.firebase.firestore.auth.EmptyCredentialsProvider;
import com.google.firebase.firestore.model.SnapshotVersion;
import com.google.firebase.firestore.model.mutation.Mutation;
import com.google.firebase.firestore.model.mutation.MutationResult;
import com.google.firebase.firestore.testutil.EmptyCredentialsProvider;
import com.google.firebase.firestore.testutil.IntegrationTestUtil;
import com.google.firebase.firestore.util.AsyncQueue;
import com.google.firebase.firestore.util.AsyncQueue.TimerId;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
import com.google.firebase.firestore.ListenerRegistration;
import com.google.firebase.firestore.MetadataChanges;
import com.google.firebase.firestore.QuerySnapshot;
import com.google.firebase.firestore.auth.EmptyCredentialsProvider;
import com.google.firebase.firestore.auth.User;
import com.google.firebase.firestore.core.DatabaseInfo;
import com.google.firebase.firestore.local.Persistence;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import com.google.firebase.emulators.EmulatedServiceSettings;
import com.google.firebase.firestore.FirebaseFirestoreException.Code;
import com.google.firebase.firestore.auth.CredentialsProvider;
import com.google.firebase.firestore.auth.EmptyCredentialsProvider;
import com.google.firebase.firestore.auth.FirebaseAuthCredentialsProvider;
import com.google.firebase.firestore.core.ActivityScope;
import com.google.firebase.firestore.core.AsyncEventListener;
Expand All @@ -48,6 +47,7 @@
import com.google.firebase.firestore.util.Function;
import com.google.firebase.firestore.util.Logger;
import com.google.firebase.firestore.util.Logger.Level;
import com.google.firebase.inject.Deferred;
import java.io.ByteArrayInputStream;
import java.io.InputStream;
import java.nio.ByteBuffer;
Expand Down Expand Up @@ -117,7 +117,7 @@ private static FirebaseFirestore getInstance(@NonNull FirebaseApp app, @NonNull
static FirebaseFirestore newInstance(
@NonNull Context context,
@NonNull FirebaseApp app,
@Nullable InternalAuthProvider authProvider,
@NonNull Deferred<InternalAuthProvider> authProvider,
@NonNull String database,
@NonNull InstanceRegistry instanceRegistry,
@Nullable GrpcMetadataProvider metadataProvider) {
Expand All @@ -129,13 +129,7 @@ static FirebaseFirestore newInstance(

AsyncQueue queue = new AsyncQueue();

CredentialsProvider provider;
if (authProvider == null) {
Logger.debug(TAG, "Firebase Auth not available, falling back to unauthenticated usage.");
provider = new EmptyCredentialsProvider();
} else {
provider = new FirebaseAuthCredentialsProvider(authProvider);
}
CredentialsProvider provider = new FirebaseAuthCredentialsProvider(authProvider);

// Firestore uses a different database for each app name. Note that we don't use
// app.getPersistenceKey() here because it includes the application ID which is related
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.google.firebase.FirebaseOptions;
import com.google.firebase.auth.internal.InternalAuthProvider;
import com.google.firebase.firestore.remote.GrpcMetadataProvider;
import com.google.firebase.inject.Deferred;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.Map;
Expand All @@ -40,13 +41,13 @@ class FirestoreMultiDbComponent

private final FirebaseApp app;
private final Context context;
private final InternalAuthProvider authProvider;
private final Deferred<InternalAuthProvider> authProvider;
private final GrpcMetadataProvider metadataProvider;

FirestoreMultiDbComponent(
@NonNull Context context,
@NonNull FirebaseApp app,
@Nullable InternalAuthProvider authProvider,
@NonNull Deferred<InternalAuthProvider> authProvider,
@Nullable GrpcMetadataProvider metadataProvider) {
this.context = context;
this.app = app;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,14 @@ public List<Component<?>> getComponents() {
.add(Dependency.required(Context.class))
.add(Dependency.optionalProvider(HeartBeatInfo.class))
.add(Dependency.optionalProvider(UserAgentPublisher.class))
.add(Dependency.optional(InternalAuthProvider.class))
.add(Dependency.deferred(InternalAuthProvider.class))
.add(Dependency.optional(FirebaseOptions.class))
.factory(
c ->
new FirestoreMultiDbComponent(
c.get(Context.class),
c.get(FirebaseApp.class),
c.get(InternalAuthProvider.class),
c.getDeferred(InternalAuthProvider.class),
new FirebaseClientGrpcMetadataProvider(
c.getProvider(UserAgentPublisher.class),
c.getProvider(HeartBeatInfo.class),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,22 @@

package com.google.firebase.firestore.auth;

import android.annotation.SuppressLint;
import androidx.annotation.GuardedBy;
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import com.google.android.gms.tasks.Task;
import com.google.android.gms.tasks.Tasks;
import com.google.firebase.FirebaseApiNotAvailableException;
import com.google.firebase.FirebaseApp;
import com.google.firebase.auth.GetTokenResult;
import com.google.firebase.auth.internal.IdTokenListener;
import com.google.firebase.auth.internal.InternalAuthProvider;
import com.google.firebase.firestore.util.Executors;
import com.google.firebase.firestore.util.Listener;
import com.google.firebase.firestore.util.Logger;
import com.google.firebase.inject.Deferred;
import com.google.firebase.inject.Provider;

/**
* FirebaseAuthCredentialsProvider uses Firebase Auth via {@link FirebaseApp} to get an auth token.
Expand All @@ -40,50 +45,54 @@ public final class FirebaseAuthCredentialsProvider extends CredentialsProvider {

private static final String LOG_TAG = "FirebaseAuthCredentialsProvider";

private final InternalAuthProvider authProvider;

/**
* The listener registered with FirebaseApp; used to stop receiving auth changes once
* changeListener is removed.
*/
private final IdTokenListener idTokenListener;
private final IdTokenListener idTokenListener = result -> onIdTokenChanged();

/** The listener to be notified of credential changes (sign-in / sign-out, token changes). */
@Nullable private Listener<User> changeListener;
/**
* The {@link Provider} that gives access to the {@link InternalAuthProvider} instance; initially,
* its {@link Provider#get} method returns {@code null}, but will be changed to a new {@link
* Provider} once the "auth" module becomes available.
*/
@Nullable
Copy link
Contributor

Choose a reason for hiding this comment

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

This is currently not nullable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@GuardedBy("this")
private InternalAuthProvider internalAuthProvider;

/** The current user as reported to us via our IdTokenListener. */
private User currentUser;
/** The listener to be notified of credential changes (sign-in / sign-out, token changes). */
@Nullable
@GuardedBy("this")
private Listener<User> changeListener;

/** Counter used to detect if the token changed while a getToken request was outstanding. */
@GuardedBy("this")
private int tokenCounter;

@GuardedBy("this")
private boolean forceRefresh;

/** Creates a new FirebaseAuthCredentialsProvider. */
public FirebaseAuthCredentialsProvider(InternalAuthProvider authProvider) {
this.authProvider = authProvider;
this.idTokenListener =
token -> {
@SuppressLint("ProviderAssignment") // TODO: Remove this @SuppressLint once b/181014061 is fixed.
public FirebaseAuthCredentialsProvider(Deferred<InternalAuthProvider> deferredAuthProvider) {
deferredAuthProvider.whenAvailable(
provider -> {
synchronized (this) {
currentUser = getUser();
tokenCounter++;

if (changeListener != null) {
changeListener.onValue(currentUser);
}
internalAuthProvider = provider.get();
onIdTokenChanged();
internalAuthProvider.addIdTokenListener(idTokenListener);
}
};
currentUser = getUser();
tokenCounter = 0;

authProvider.addIdTokenListener(idTokenListener);
});
}

@Override
public synchronized Task<String> getToken() {
boolean doForceRefresh = forceRefresh;
if (internalAuthProvider == null) {
return Tasks.forException(new FirebaseApiNotAvailableException("auth is not available"));
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure you want to throw in this case? Shouldn't this case be treated the same as it is in the EmptyCredentialsProvider and return Task.forResult(null)?

public Task<String> getToken() {
TaskCompletionSource<String> source = new TaskCompletionSource<>();
source.setResult(null);
return source.getTask();
}

Additionally, is EmptyCredentialsProvider still needed or can it be removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that throwing FirebaseApiNotAvailableException is the correct thing to do here. See FirestoreCallCredentials:

if (exception instanceof FirebaseApiNotAvailableException) {
Logger.debug(LOG_TAG, "Firebase Auth API not available, not using authentication.");
metadataApplier.apply(new Metadata());

By failing with FirebaseApiNotAvailableException it causes FirestoreCallCredentials to go down the code path where auth is not available, which logs an appropriate message. I could be wrong about this, but it sure looks correct to me.

Regarding EmptyCredentialsProvider, it is now only used in tests. I've moved it to the test utility package. Good point.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What are your thoughts on this in light of the code in FirestoreCallCredentials that I linked to?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I've left the FirebaseApiNotAvailableException in there.

}

Task<GetTokenResult> res = internalAuthProvider.getAccessToken(forceRefresh);
forceRefresh = false;
Task<GetTokenResult> res = authProvider.getAccessToken(doForceRefresh);

// Take note of the current value of the tokenCounter so that this method can fail (with a
// FirebaseFirestoreException) if there is a token change while the request is outstanding.
Expand Down Expand Up @@ -118,18 +127,29 @@ public synchronized void setChangeListener(@NonNull Listener<User> changeListene
this.changeListener = changeListener;

// Fire the initial event.
changeListener.onValue(currentUser);
changeListener.onValue(getUser());
}

@Override
public synchronized void removeChangeListener() {
changeListener = null;
authProvider.removeIdTokenListener(idTokenListener);

if (internalAuthProvider != null) {
internalAuthProvider.removeIdTokenListener(idTokenListener);
}
}

/** Invoked when the auth token changes. */
private synchronized void onIdTokenChanged() {
tokenCounter++;
if (changeListener != null) {
changeListener.onValue(getUser());
}
}

/** Returns the current {@link User} as obtained from the given FirebaseApp instance. */
private User getUser() {
@Nullable String uid = authProvider.getUid();
/** Returns the current {@link User} as obtained from the given InternalAuthProvider. */
private synchronized User getUser() {
@Nullable String uid = (internalAuthProvider == null) ? null : internalAuthProvider.getUid();
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

return uid != null ? new User(uid) : User.UNAUTHENTICATED;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
import com.google.firebase.FirebaseOptions;
import com.google.firebase.auth.internal.InternalAuthProvider;
import com.google.firebase.firestore.remote.GrpcMetadataProvider;
import com.google.firebase.firestore.testutil.ImmediateDeferred;
import com.google.firebase.inject.Deferred;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.robolectric.RobolectricTestRunner;
Expand Down Expand Up @@ -58,7 +60,9 @@ private static FirebaseApp createApp(String appName) {
private static FirestoreMultiDbComponent createComponent(FirebaseApp firebaseApp) {
Context context = InstrumentationRegistry.getInstrumentation().getContext();
InternalAuthProvider authProvider = mock(InternalAuthProvider.class);
Deferred<InternalAuthProvider> deferredAuthProvider = new ImmediateDeferred<>(authProvider);
GrpcMetadataProvider metadataProvider = mock(GrpcMetadataProvider.class);
return new FirestoreMultiDbComponent(context, firebaseApp, authProvider, metadataProvider);
return new FirestoreMultiDbComponent(
context, firebaseApp, deferredAuthProvider, metadataProvider);
}
}
Loading