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, windows): putFile(), putString(), putData() & Task streaming event fixes #12723

Merged
merged 24 commits into from
May 10, 2024

Conversation

russellwheatley
Copy link
Member

@russellwheatley russellwheatley commented Apr 29, 2024

Description

Fixes

  1. putData() is now writing the data as intended.
  2. putString() is now decoding from base64 and base64Url before writing string (previously writing string as base64).
  3. putFile() metadata was not working correctly.
  4. putFile() we have also had to do a hack to pass metadata as null on windows, it was throwing exception in pigeon even though it is allowable as a nullable value. Not ideal, but it works.
  5. customMetadata is now returned with the snapshot which fixes: [FIREBASE_STORAGE]: Wrong json key for customMetadata #12711
  6. Some responses were coming back completely incorrect, they have been fixed by using EncodableValue() to pass values back to Flutter.
  7. task!.snapshotEvents events were never firing. The storage event listener (onProgress, and onPause) were incorrectly passed to the API handler (e.g. putBytes(), putFile(), etc). This is the correct way to instantiate.
  8. task.pause(), task.cancel(), task.resume() were also broken. The incorrect encoding of values passed back to Flutter stopped the flutter side from having proper values to parse.
  9. The error map was never passed back in the stream event handling so exception were not being thrown correctly when listening to events.
  10. Fixed an issue in Firebase core C++ were AppOptions (equivalent to FirebaseOptions in Flutter side) was creating every property as an empty string. For example, storage bucket option was being passed back to Flutter as an empty string.
  11. snapshot was missing path property causing exception on Flutter side when handling File streaming events.
  12. metadata as a property on snapshot was also missing bucket property.

Tests

  1. Storage windows now has integration tests albeit using a live Firebase project. There are some tests that are skipped which are largely stream handling events, I spent quite a bit of time on this. It works when running the app but fails in a test environment. The good news is most tests are being used.
  2. I updated the putData(), putFile() and putString() tests to also check the content is correct and not gibberish or base64 encoding.
  3. I've found a bug in the C++ SDK, if you try to update metadata on an object that does not exist, it will throw an unauthorized exception rather than an object-not-found exception. I checked native, it is a firebase SDK bug.

Related Issues

fixes #12743
fixes #12711

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.

@russellwheatley russellwheatley changed the title fix(storage, windows): putString & putData API fixes fix(storage, windows): putFile(), putString(), putData() & Task streaming event fixes May 3, 2024
@russellwheatley russellwheatley marked this pull request as ready for review May 10, 2024 11:17
Comment on lines +263 to +264
pigeonSettableMetadata =
MethodChannelFirebaseStorage.getPigeonSettableMetaData(metadata);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, probably an issue in Pigeon, but the workaround looks OK

@russellwheatley russellwheatley merged commit de69e07 into master May 10, 2024
24 of 27 checks passed
@russellwheatley russellwheatley deleted the storage-windows branch May 10, 2024 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants