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

Adds putFileHandle and putFileHandleAsync #12580

Merged
merged 12 commits into from
Mar 27, 2024

Conversation

mattcomi
Copy link
Contributor

This adds ability to upload from FileHandle which makes it possible to upload large files from an iOS file extension.

Uploading a large (+100MB) file from an app extension isn't currently possible. StorageReference has putData and putFile on its public interface. putData won't work because an app extension has a 120MB memory limit and putFile doesn't work in app extensions.

Under the hood, FirebaseStorage uses GTMSessionUploadFetcher to perform uploads. The GTMSessionUploadFetcher supports upload from FileHandle.

This PR exposes that ability to StorageReference. See #12579.

Copy link

google-cla bot commented Mar 19, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

Thanks! This looks like a nice API addition.

I'll run this through our internal API review process and let you know how it goes.

Package.swift Outdated Show resolved Hide resolved
@mattcomi mattcomi force-pushed the mattcomi/put-file-handle branch from 52f1e29 to 8cf8107 Compare March 20, 2024 04:22
@mattcomi
Copy link
Contributor Author

@paulb777 Thanks for the feedback! I made the requested changes and ran the integration tests locally.

@paulb777 paulb777 self-assigned this Mar 20, 2024
@paulb777 paulb777 added this to the 10.24.0 - M146 milestone Mar 20, 2024
@paulb777
Copy link
Member

Thanks @mattcomi! I've kicked off the internal API review process and see if we can get it through in time for our next release.

@paulb777
Copy link
Member

After discussion with the team, we've decided we don't want to add new Objective-C APIs, since we want to encourage Swift adoption and minimize are ObjC API footprint.

Would you remove the @objc attributes, Objective C API test changes, and __putFileHandle?

Thanks!

@mattcomi
Copy link
Contributor Author

mattcomi commented Mar 22, 2024

Done 👍 Also, without needing to support Objective-C support, we can use Swift default parameters. This means there is no longer a need for the two separate overloads putFileHandle:metadata: and putFileHandle:metadata:completion:.

@ncooke3
Copy link
Member

ncooke3 commented Mar 25, 2024

Hi @mattcomi, do you think the use case here could be addressed by using FileHandle as an implementation detail of the existing putFile API? So, you pass in the URL you'd like to upload, and Firebase Storage would create a FileHandle using the initForReading initializer for the given URL, and pass it to GTMSessionFetcher.

The advantage here is that the existing putFile API becomes more robust and a new API is not needed. But, this idea breaks down in the event that you are passing around FileHandles in your app already (as opposed to URLs) because it doesn't look like there is a good way to go from FileHandle → URL. Thoughts?

@mattcomi
Copy link
Contributor Author

mattcomi commented Mar 26, 2024

@ncooke3 thanks for the feedback! You raised a great point and I was able to revise my approach with something dramatically simpler.

I investigated exactly why FileHandle works in app extensions but URL does not. The reason is that GTMSessionUploadFetcher uses a background session when it is init with URL, but not when init with Data or a FileHandle. This is why the putFile documentation says "putData should be used instead of putFile in Extensions." The fix is for UploadStorageTask to disable useBackgroundSession when it is being used within an app extension.

Let me know what you think of this alternate approach. It does have the limitation you describe: when passed a FileHandle, the developer has no way of reverse solving its URL, but I've certainly never faced this problem personally.

Sidenote: One advantage I originally thought FileHandle would offer is the ability to upload from a pipe or a socket, but this isn't possible because GTMSessionUploadFetcher tries to seekToEndOfFile and this is an exception unless the FileHandle is init from a file.

Copy link
Member

@ncooke3 ncooke3 left a comment

Choose a reason for hiding this comment

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

LGTM after the suggested changes. Thank you, @mattcomi!

FirebaseStorage/CHANGELOG.md Outdated Show resolved Hide resolved
@paulb777 paulb777 merged commit 8230f73 into firebase:main Mar 27, 2024
86 checks passed
cgrindel-self-hosted-renovate bot referenced this pull request in cgrindel/rules_swift_package_manager Apr 9, 2024
….24.0" (#1007)

This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
|
[firebase/firebase-ios-sdk](https://togithub.com/firebase/firebase-ios-sdk)
| minor | `from: "10.23.1"` -> `from: "10.24.0"` |

---

### Release Notes

<details>
<summary>firebase/firebase-ios-sdk (firebase/firebase-ios-sdk)</summary>

###
[`v10.24.0`](https://togithub.com/firebase/firebase-ios-sdk/releases/tag/10.24.0):
Firebase Apple 10.24.0

[Compare
Source](https://togithub.com/firebase/firebase-ios-sdk/compare/10.23.1...10.24.0)

The Firebase Apple SDK (10.24.0) is now available. For more details, see
the [Firebase Apple SDK release
notes.](https://firebase.google.com/support/release-notes/ios#10.24.0)

To install this SDK, see [Add Firebase to your
project.](https://firebase.google.com/docs/ios/setup)

#### What's Changed

- Remove calls to fstat in crashlytics by
[@&#8203;volantwish](https://togithub.com/volantwish) in
[https://github.com/firebase/firebase-ios-sdk/pull/12531](https://togithub.com/firebase/firebase-ios-sdk/pull/12531)
- fix unit tests by
[@&#8203;themiswang](https://togithub.com/themiswang) in
[https://github.com/firebase/firebase-ios-sdk/pull/12553](https://togithub.com/firebase/firebase-ios-sdk/pull/12553)
- \[Release] Add release note for signed artifact changes by
[@&#8203;ncooke3](https://togithub.com/ncooke3) in
[https://github.com/firebase/firebase-ios-sdk/pull/12558](https://togithub.com/firebase/firebase-ios-sdk/pull/12558)
- Fix typo by [@&#8203;paulb777](https://togithub.com/paulb777) in
[https://github.com/firebase/firebase-ios-sdk/pull/12565](https://togithub.com/firebase/firebase-ios-sdk/pull/12565)
- \[Firestore] Add a check to ensure FirestoreInternal has same public
headers as Firestore by [@&#8203;ncooke3](https://togithub.com/ncooke3)
in
[https://github.com/firebase/firebase-ios-sdk/pull/12575](https://togithub.com/firebase/firebase-ios-sdk/pull/12575)
- Carthage 10.23.0 by [@&#8203;paulb777](https://togithub.com/paulb777)
in
[https://github.com/firebase/firebase-ios-sdk/pull/12588](https://togithub.com/firebase/firebase-ios-sdk/pull/12588)
- Initial CI for visionOS by
[@&#8203;paulb777](https://togithub.com/paulb777) in
[https://github.com/firebase/firebase-ios-sdk/pull/12578](https://togithub.com/firebase/firebase-ios-sdk/pull/12578)
- Upgrade cmake build to grpc 162 by
[@&#8203;wu-hui](https://togithub.com/wu-hui) in
[https://github.com/firebase/firebase-ios-sdk/pull/12417](https://togithub.com/firebase/firebase-ios-sdk/pull/12417)
- More visionOS CI by [@&#8203;paulb777](https://togithub.com/paulb777)
in
[https://github.com/firebase/firebase-ios-sdk/pull/12608](https://togithub.com/firebase/firebase-ios-sdk/pull/12608)
- Update versions for Release 10.24.0 by
[@&#8203;paulb777](https://togithub.com/paulb777) in
[https://github.com/firebase/firebase-ios-sdk/pull/12594](https://togithub.com/firebase/firebase-ios-sdk/pull/12594)
- \[Release Tooling] Update XCFramework structure by
[@&#8203;ncooke3](https://togithub.com/ncooke3) in
[https://github.com/firebase/firebase-ios-sdk/pull/12595](https://togithub.com/firebase/firebase-ios-sdk/pull/12595)
- \[Docs] Update `FirebaseCore/CHANGELOG.md` with correct issue # by
[@&#8203;ncooke3](https://togithub.com/ncooke3) in
[https://github.com/firebase/firebase-ios-sdk/pull/12639](https://togithub.com/firebase/firebase-ios-sdk/pull/12639)
- Adds putFileHandle and putFileHandleAsync by
[@&#8203;mattcomi](https://togithub.com/mattcomi) in
[https://github.com/firebase/firebase-ios-sdk/pull/12580](https://togithub.com/firebase/firebase-ios-sdk/pull/12580)
- \[CocoaPods] Lock FirestoreInternal version to Firestore by
[@&#8203;paulb777](https://togithub.com/paulb777) in
[https://github.com/firebase/firebase-ios-sdk/pull/12654](https://togithub.com/firebase/firebase-ios-sdk/pull/12654)
- Merge 10.23.1 by [@&#8203;paulb777](https://togithub.com/paulb777) in
[https://github.com/firebase/firebase-ios-sdk/pull/12655](https://togithub.com/firebase/firebase-ios-sdk/pull/12655)
- \[Crashlytics] Fix missing Swift header error by
[@&#8203;ncooke3](https://togithub.com/ncooke3) in
[https://github.com/firebase/firebase-ios-sdk/pull/12659](https://togithub.com/firebase/firebase-ios-sdk/pull/12659)
- Move MIEQ to composite index tests by
[@&#8203;milaGGL](https://togithub.com/milaGGL) in
[https://github.com/firebase/firebase-ios-sdk/pull/12416](https://togithub.com/firebase/firebase-ios-sdk/pull/12416)
- feat: add basic support to build frameworks and zip them for the Apple
Watch by [@&#8203;jasesuperhero](https://togithub.com/jasesuperhero) in
[https://github.com/firebase/firebase-ios-sdk/pull/12624](https://togithub.com/firebase/firebase-ios-sdk/pull/12624)
- Release note for watchOS zip/Carthage by
[@&#8203;paulb777](https://togithub.com/paulb777) in
[https://github.com/firebase/firebase-ios-sdk/pull/12670](https://togithub.com/firebase/firebase-ios-sdk/pull/12670)
- \[Crashlytics] Remove mach_absolute_time usages by
[@&#8203;paulb777](https://togithub.com/paulb777) in
[https://github.com/firebase/firebase-ios-sdk/pull/12664](https://togithub.com/firebase/firebase-ios-sdk/pull/12664)
- Update to SwiftFormat 0.53.5 by
[@&#8203;paulb777](https://togithub.com/paulb777) in
[https://github.com/firebase/firebase-ios-sdk/pull/12665](https://togithub.com/firebase/firebase-ios-sdk/pull/12665)
- Fix typo: Timout -> Timeout by
[@&#8203;Jager-yoo](https://togithub.com/Jager-yoo) in
[https://github.com/firebase/firebase-ios-sdk/pull/12672](https://togithub.com/firebase/firebase-ios-sdk/pull/12672)
- \[Crashlytics] Regenerate privacy manifest by
[@&#8203;ncooke3](https://togithub.com/ncooke3) in
[https://github.com/firebase/firebase-ios-sdk/pull/12675](https://togithub.com/firebase/firebase-ios-sdk/pull/12675)
- \[Release] Version changelog entries for 10.24.0 by
[@&#8203;ncooke3](https://togithub.com/ncooke3) in
[https://github.com/firebase/firebase-ios-sdk/pull/12677](https://togithub.com/firebase/firebase-ios-sdk/pull/12677)
- \[Release Tooling] Fix METADATA.md regression introduced in
[#&#8203;12595](https://togithub.com/firebase/firebase-ios-sdk/issues/12595)
by [@&#8203;ncooke3](https://togithub.com/ncooke3) in
[https://github.com/firebase/firebase-ios-sdk/pull/12661](https://togithub.com/firebase/firebase-ios-sdk/pull/12661)
- Analytics 10.24.0 by
[@&#8203;tsunghung](https://togithub.com/tsunghung) in
[https://github.com/firebase/firebase-ios-sdk/pull/12693](https://togithub.com/firebase/firebase-ios-sdk/pull/12693)
- \[Release] Update binary SPM Firestore distro for 10.24.0 by
[@&#8203;ncooke3](https://togithub.com/ncooke3) in
[https://github.com/firebase/firebase-ios-sdk/pull/12708](https://togithub.com/firebase/firebase-ios-sdk/pull/12708)
- \[Release] Update Firestore's binary deps in Package.swift by
[@&#8203;ncooke3](https://togithub.com/ncooke3) in
[https://github.com/firebase/firebase-ios-sdk/pull/12709](https://togithub.com/firebase/firebase-ios-sdk/pull/12709)

#### New Contributors

- [@&#8203;volantwish](https://togithub.com/volantwish) made their first
contribution in
[https://github.com/firebase/firebase-ios-sdk/pull/12531](https://togithub.com/firebase/firebase-ios-sdk/pull/12531)
- [@&#8203;mattcomi](https://togithub.com/mattcomi) made their first
contribution in
[https://github.com/firebase/firebase-ios-sdk/pull/12580](https://togithub.com/firebase/firebase-ios-sdk/pull/12580)
- [@&#8203;jasesuperhero](https://togithub.com/jasesuperhero) made their
first contribution in
[https://github.com/firebase/firebase-ios-sdk/pull/12624](https://togithub.com/firebase/firebase-ios-sdk/pull/12624)
- [@&#8203;Jager-yoo](https://togithub.com/Jager-yoo) made their first
contribution in
[https://github.com/firebase/firebase-ios-sdk/pull/12672](https://togithub.com/firebase/firebase-ios-sdk/pull/12672)

**Full Changelog**:
firebase/firebase-ios-sdk@10.23.1...10.24.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config help](https://togithub.com/renovatebot/renovate/discussions) if
that's undesired.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://togithub.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4xMDkuNCIsInVwZGF0ZWRJblZlciI6IjM2LjEwOS40IiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: Self-hosted Renovate Bot <361546+cgrindel-self-hosted-renovate[bot]@users.noreply.github.enterprise.com>
@firebase firebase locked and limited conversation to collaborators Apr 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants