Firebase Storage Component Registration by schmidt-sebastian · Pull Request #277 · firebase/firebase-android-sdk

schmidt-sebastian · 2019-03-13T09:36:59Z

This definitely falls under the umbrella "I don't know what I am doing". But here we go...

This follows the pattern in Firestore to register the component for Firebase Storage. It changes FirebaseStorage to only ever work with a single app, but keeps the option to get different instances for different storage buckets.

I also added getInstanceImpl as a valid name for the FirebaseApp.get() call, as I didn't want to add yet another getInstance() overload (which would result in an ambiguous null cast).

So what does everyone think? :)

vkryachko

@schmidt-sebastian Awesome! Thanks for picking it up.

One important thing is that we also need storage to stop integrating with Auth via FirebaseApp(I think it uses the deprecated #getToken(), but maybe something else as well) and start using firebase-auth-interop's InternalAuthProvider like e.g. firestore does:

firebase-android-sdk/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreRegistrar.java

Line 44 in 622e798

.add(Dependency.optional(InternalAuthProvider.class))

Feel free to punt it to a separate PR.

vkryachko · 2019-03-13T12:39:45Z

firebase-storage/src/main/java/com/google/firebase/storage/FirebaseStorage.java

-    }
+    checkNotNull(app, "Provided FirebaseApp must not be null.");
+    FirebaseStorageComponent component = app.get(FirebaseStorageComponent.class);
+    checkNotNull(component, "Firestore component is not present.");


s/Firestore/Storage/

vkryachko · 2019-03-13T12:41:27Z

firebase-storage/src/main/java/com/google/firebase/storage/FirebaseStorage.java

  }

+  @NonNull
+  static FirebaseStorage newInstance(@Nullable String bucketName, @NonNull FirebaseApp app) {


Do you actually need this method? Would it make sense to make the constructor package-private and call it directly in the Component? As a side-effect I think it will also make it possible to mock FirebaseStorage.

No, we don't need it. I was trying to follow the precedent set by Firestore. Inlined.

vkryachko · 2019-03-13T12:48:02Z

firebase-storage/src/main/java/com/google/firebase/storage/FirebaseStorageComponent.java

+
+class FirebaseStorageComponent {
+  /** A static map from storage buckets to FirebaseStorage instances. */
+  private static final Map<String, FirebaseStorage> instances = new HashMap<>();


Note this should not be static, otherwise the instances will be shared between all FirebaseApps.

Oh yes, that makes sense. Thanks for catching this.

vkryachko · 2019-03-13T13:06:15Z

firebase-storage/src/main/java/com/google/firebase/storage/FirebaseStorageComponent.java

+    return storage;
+  }
+
+  /** @hide */


I think @hide is not needed for non-public classes/symbols

The original method was public. I have removed it now that I reduced the visibility.

vkryachko · 2019-03-13T13:06:52Z

firebase-storage/src/main/java/com/google/firebase/storage/FirebaseStorageComponent.java

+    this.app = app;
+  }
+
+  /** Provides instances of Firestore for given database names. */


s/Firestore/Storage/

schmidt-sebastian

Updated the PR. Most of the changes now are related to the switch to InternalAuthTokenProvider,

schmidt-sebastian · 2019-03-14T02:17:05Z

firebase-storage/src/main/java/com/google/firebase/storage/FirebaseStorage.java

  }

+  @NonNull
+  static FirebaseStorage newInstance(@Nullable String bucketName, @NonNull FirebaseApp app) {


No, we don't need it. I was trying to follow the precedent set by Firestore. Inlined.

schmidt-sebastian · 2019-03-14T02:18:06Z

firebase-storage/src/main/java/com/google/firebase/storage/FirebaseStorage.java

-    }
+    checkNotNull(app, "Provided FirebaseApp must not be null.");
+    FirebaseStorageComponent component = app.get(FirebaseStorageComponent.class);
+    checkNotNull(component, "Firestore component is not present.");


schmidt-sebastian · 2019-03-14T02:18:49Z

firebase-storage/src/main/java/com/google/firebase/storage/FirebaseStorageComponent.java

+
+class FirebaseStorageComponent {
+  /** A static map from storage buckets to FirebaseStorage instances. */
+  private static final Map<String, FirebaseStorage> instances = new HashMap<>();


Oh yes, that makes sense. Thanks for catching this.

schmidt-sebastian · 2019-03-14T02:18:57Z

firebase-storage/src/main/java/com/google/firebase/storage/FirebaseStorageComponent.java

+    this.app = app;
+  }
+
+  /** Provides instances of Firestore for given database names. */


schmidt-sebastian · 2019-03-14T02:19:40Z

firebase-storage/src/main/java/com/google/firebase/storage/FirebaseStorageComponent.java

+    return storage;
+  }
+
+  /** @hide */


The original method was public. I have removed it now that I reduced the visibility.

schmidt-sebastian · 2019-03-14T04:48:07Z

/test connected-check-changed

vkryachko · 2019-03-14T12:52:54Z

firebase-storage/src/main/java/com/google/firebase/storage/FirebaseStorage.java

  public static FirebaseStorage getInstance(@NonNull FirebaseApp app, @NonNull String url) {
    // noinspection ConstantConditions
    Preconditions.checkArgument(app != null, "Null is not a valid value for the FirebaseApp.");
+    Preconditions.checkArgument(


optional nit: elsewhere you switched to staticly imported checks, might make sense to do the same throughout for consistency.

The rest of the SDK doesn't use static imports, so I changed this file to use the qualified import checkNotNull.

vkryachko · 2019-03-14T13:02:42Z

firebase-storage/src/main/java/com/google/firebase/storage/StorageRegistrar.java

+    return Arrays.asList(
+        Component.builder(FirebaseStorageComponent.class)
+            .add(Dependency.required(FirebaseApp.class))
+            .add(Dependency.optional(InternalAuthProvider.class))


With my limited understanding of storage it looks like InternalAuthProvider is not needed right away(when storage initializes) unlike in Firestore, but only used in response to user calls to downloads and what not. If that is the case consider switching to Dependency.optionalProvider(InternalAuthProvider.class), what it will do is defer initialization of InternalAuthProvider until it is needed as well as prevent potential dependency cycles in the future.

That said, currently the only implementor of InternalAuthProvider is FirebaseAuth which is eager, so deferred initialization will not apply, but it seems a relatively easy change to do(especially if the Provider<T> is hidden behind storage.getAuthProvider()) to bulletproof storage for the future. wdyt?

Your understanding is correct. We only fetch a token once a user requests a file upload or download, so we don't need the AuthProvider until later. The change for this was relatively minor, so I updated this PR.

BTW, if wasn't immediately obvious to me whether optionalProvider makes Provider nullable or whether it makes Provider.get() nullable. I think it's the former (from looking through the code and playing with it). Maybe we can document this?

vkryachko · 2019-03-14T13:03:59Z

firebase-storage/src/main/java/com/google/firebase/storage/UploadTask.java

  private final AdaptiveStreamBuffer mStreamBuffer;
  // Active, current mutable state.
  private final AtomicLong mBytesUploaded = new AtomicLong(0);
+  @Nullable final InternalAuthProvider mAuthProvider;


could it be private?

schmidt-sebastian

Updated the PR. My test app is able to authenticate with these changes.

schmidt-sebastian · 2019-03-15T05:18:02Z

firebase-storage/src/main/java/com/google/firebase/storage/FirebaseStorage.java

  public static FirebaseStorage getInstance(@NonNull FirebaseApp app, @NonNull String url) {
    // noinspection ConstantConditions
    Preconditions.checkArgument(app != null, "Null is not a valid value for the FirebaseApp.");
+    Preconditions.checkArgument(


The rest of the SDK doesn't use static imports, so I changed this file to use the qualified import checkNotNull.

schmidt-sebastian · 2019-03-15T05:22:02Z

firebase-storage/src/main/java/com/google/firebase/storage/StorageRegistrar.java

+    return Arrays.asList(
+        Component.builder(FirebaseStorageComponent.class)
+            .add(Dependency.required(FirebaseApp.class))
+            .add(Dependency.optional(InternalAuthProvider.class))


Your understanding is correct. We only fetch a token once a user requests a file upload or download, so we don't need the AuthProvider until later. The change for this was relatively minor, so I updated this PR.

BTW, if wasn't immediately obvious to me whether optionalProvider makes Provider nullable or whether it makes Provider.get() nullable. I think it's the former (from looking through the code and playing with it). Maybe we can document this?

schmidt-sebastian · 2019-03-15T05:22:12Z

firebase-storage/src/main/java/com/google/firebase/storage/UploadTask.java

  private final AdaptiveStreamBuffer mStreamBuffer;
  // Active, current mutable state.
  private final AtomicLong mBytesUploaded = new AtomicLong(0);
+  @Nullable final InternalAuthProvider mAuthProvider;


schmidt-sebastian · 2019-03-15T06:37:30Z

/test connected-check-changed

schmidt-sebastian · 2019-03-15T07:14:56Z

/test connected-check-changed

vkryachko

LGTM

Looks like we have a legit failure in firebase-database-collection whose minSdkVersion is less than min version of playservices-base, I feel like this has alway been the case but now it suddently started failing(I'll take a look at and fix it)

vkryachko · 2019-03-15T12:51:34Z

firebase-storage/src/main/java/com/google/firebase/storage/StorageRegistrar.java

 @RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
 public class StorageRegistrar implements ComponentRegistrar {
  @Override
+  @Keep


nit: no need to @Keep this method, just the class is enough

vkryachko · 2019-03-15T12:59:10Z

/retest

googlebot added the cla: yes Override cla label Mar 13, 2019

google-oss-bot added the size/L label Mar 13, 2019

Firebase Storage Component Registration

7ba9359

schmidt-sebastian force-pushed the mrschmidt-storagecomponents branch from c02e1b2 to 7ba9359 Compare March 13, 2019 09:38

schmidt-sebastian requested a review from vkryachko March 13, 2019 09:39

schmidt-sebastian assigned vkryachko Mar 13, 2019

vkryachko reviewed Mar 13, 2019

View reviewed changes

schmidt-sebastian force-pushed the mrschmidt-storagecomponents branch 2 times, most recently from ed00904 to 339880f Compare March 14, 2019 03:06

schmidt-sebastian commented Mar 14, 2019

View reviewed changes

Feedback & using InternalAuthProvider

4039ddd

schmidt-sebastian force-pushed the mrschmidt-storagecomponents branch from 339880f to 4039ddd Compare March 14, 2019 03:07

Fix comment

ede02d0

schmidt-sebastian mentioned this pull request Mar 14, 2019

Fix Firestore POJO tests #281

Merged

vkryachko reviewed Mar 14, 2019

View reviewed changes

schmidt-sebastian added 2 commits March 15, 2019 12:28

Merge branch 'master' into mrschmidt-storagecomponents

7d576ab

Feedback + Using Optional Provider

39196fa

schmidt-sebastian commented Mar 15, 2019

View reviewed changes

schmidt-sebastian mentioned this pull request Mar 15, 2019

Firebase Database Component Registration #286

Merged

vkryachko approved these changes Mar 15, 2019

View reviewed changes

Removing duplicate @Keep

12bec63

schmidt-sebastian merged commit 17e086f into master Mar 16, 2019

vkryachko deleted the mrschmidt-storagecomponents branch March 19, 2019 14:09

firebase locked and limited conversation to collaborators Oct 12, 2019

Conversation

schmidt-sebastian commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vkryachko left a comment

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!

schmidt-sebastian left a comment

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!

schmidt-sebastian commented Mar 14, 2019

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!

schmidt-sebastian left a comment

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!

schmidt-sebastian commented Mar 15, 2019

Uh oh!

schmidt-sebastian commented Mar 15, 2019

Uh oh!

vkryachko left a comment

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!

vkryachko commented Mar 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

schmidt-sebastian commented Mar 13, 2019 •

edited

Loading