Skip to content
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

fix(storage): ensure bucket value is used to create FirebaseStorage instance to stop incorrect default bucket usage #11844

Merged
merged 19 commits into from Nov 9, 2023

Conversation

russellwheatley
Copy link
Member

@russellwheatley russellwheatley commented Nov 7, 2023

Description

  1. Passes the bucket value along with the Firebase app name and uses those values for every FirebaseStorage invocation for apple and android. For instance, android/apple were allowing some methods to create FirebaseStorage without bucket value here. This resulted in multiple incorrectly assigned default buckets when the user tried to use non-default bucket. All the following API would not work as intended if a user selected a non-default bucket:

https://github.com/firebase/flutterfire/blob/master/packages/firebase_storage/firebase_storage/android/src/main/java/io/flutter/plugins/firebase/storage/FlutterFirebaseStoragePlugin.java#L290
https://github.com/firebase/flutterfire/blob/master/packages/firebase_storage/firebase_storage/android/src/main/java/io/flutter/plugins/firebase/storage/FlutterFirebaseStoragePlugin.java#L304
https://github.com/firebase/flutterfire/blob/master/packages/firebase_storage/firebase_storage/android/src/main/java/io/flutter/plugins/firebase/storage/FlutterFirebaseStoragePlugin.java#L324
https://github.com/firebase/flutterfire/blob/master/packages/firebase_storage/firebase_storage/android/src/main/java/io/flutter/plugins/firebase/storage/FlutterFirebaseStoragePlugin.java#L346
https://github.com/firebase/flutterfire/blob/master/packages/firebase_storage/firebase_storage/android/src/main/java/io/flutter/plugins/firebase/storage/FlutterFirebaseStoragePlugin.java#L376
https://github.com/firebase/flutterfire/blob/master/packages/firebase_storage/firebase_storage/android/src/main/java/io/flutter/plugins/firebase/storage/FlutterFirebaseStoragePlugin.java#L417
https://github.com/firebase/flutterfire/blob/master/packages/firebase_storage/firebase_storage/android/src/main/java/io/flutter/plugins/firebase/storage/FlutterFirebaseStoragePlugin.java#L445
https://github.com/firebase/flutterfire/blob/master/packages/firebase_storage/firebase_storage/android/src/main/java/io/flutter/plugins/firebase/storage/FlutterFirebaseStoragePlugin.java#L489

  1. Removed unused methods that should've been removed during Pigeon migration.
  2. Updated maxResults to be required, and passed 1000 if user did not pass it as a value to ListOptions, we set it here to 1000. This is a required parameter on the native SDKs when calling list() on a reference. As it is currently implemented, it would throw a NullPointerException as it could be null if user didn't update maxResults with a value.
  3. Written a new file for e2e tests specifically for a second bucket.

As an interesting aside, if the user uses a non-default bucket, you also need to set the emulator for it (true for web, iOS and android). For instance, this would call the Firebase server:

await FirebaseStorage.instance.useStorageEmulator(emulatorHost, 9199);

final bucket = FirebaseStorage.instanceFor(
	bucket:'flutterfire-e2e-tests-two',
);

// This needs setting to use the emulator:
// bucket.useStorageEmulator(emulatorHost, 9199);

final ref = bucket
    .ref()
    .child('flutter-tests')
    .child('test.txt');
await ref.putString('hello!');

Related Issues

fixes #11805
fixes #11808
fixes #11802

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]).
This will ensure a smooth and quick review process. Updating the pubspec.yaml and changelogs is not required.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (melos run analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

@@ -424,8 +424,6 @@ void setupReferenceTests() {
),
);
},
// TODO(salakar): Firebase storage emulator incorrectly returns `object-not-found` instead of `unauthorized`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice


expect(url, contains('/$secondaryBucket/'));
},
skip: true, // does Firebase Emulator support custom bucket?
Copy link
Contributor

Choose a reason for hiding this comment

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

When was this added? I guess she should have caught it in the previous PR

Copy link
Member Author

Choose a reason for hiding this comment

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

I think at the time the e2e tests were written, the emulator did not support a non-default bucket.

@russellwheatley russellwheatley merged commit 49542f3 into master Nov 9, 2023
23 checks passed
@russellwheatley russellwheatley deleted the storage-10249 branch November 9, 2023 12:17
@firebase firebase locked and limited conversation to collaborators Dec 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants