Skip to content

[Storage] Improve how GetFile handles paths #1782

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.

Sign up for GitHub

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 5 commits into from
Aug 6, 2025
Merged

[Storage] Improve how GetFile handles paths #1782

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3f9fc90
[Storage] Improve how GetFile handles paths
a-maurice Jul 30, 2025
5c0be22
Update based on feedback
a-maurice Aug 5, 2025
5b1d94f
Update storage_reference_ios.mm
a-maurice Aug 5, 2025
c429475
Update storage_reference_ios.mm
a-maurice Aug 5, 2025
4d3a96c
Revert "Update storage_reference_ios.mm"
a-maurice Aug 5, 2025
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
4 changes: 4 additions & 0 deletions release_build_files/readme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,6 +613,10 @@ workflow use only during the development of your app, not for publicly shipping
code.

## Release Notes
### Upcoming
- Changes
- Storage (iOS): Handle absolute paths being provided to GetFile. (#1724)

### 13.0.0
- Changes
- General (Android): Update to Firebase Android BoM version 34.0.0.
Expand Down
3 changes: 2 additions & 1 deletion storage/integration_test/src/integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,8 @@ TEST_F(FirebaseStorageTest, TestPutFileAndGetFile) {
std::string path = PathForResource() + kGetFileTestFile;
// Cloud Storage expects a URI, so add file:// in front of local
// paths.
std::string file_path = kFileUriScheme + path;
// TODO: Revert, just testing to see if this will pass without.
std::string file_path = /*kFileUriScheme +*/ path;

LogDebug("Saving to local file: %s", path.c_str());

Expand Down
16 changes: 15 additions & 1 deletion storage/src/ios/storage_reference_ios.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,21 @@
// Cache a copy of the impl and storage, in case this is destroyed before the thread runs.
FIRStorageReference* my_impl = impl();
StorageInternal* storage = storage_;
NSURL* local_file_url = [NSURL URLWithString:@(path)];
NSString* path_string = @(path);
NSURL* local_file_url = nil;
if ([path_string hasPrefix:@"file://"]) {
// If it starts with the prefix, load it assuming a URL string.
local_file_url = [NSURL URLWithString:path_string];
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Can this be breaking since before it always used URLWithString and now it only uses URLWithString if there is a file:// prefix?

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 previously it would fail later if you tried to use a different url type, since it is calling into Storage ObjC's writeToFile, which is documented to take in a fileUrl (though looking over the code, I don't immediately spot anything that enforces that). The other possibility I was thinking of doing was trying URLWithString first, and if that returns nil fall back to fileURLWithPath. Do you think that would make more sense (and it would resolve the problem you are describing)?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, that would seem like a safer change to do URLWithString first and fall back. However, I'm also ok as is, if you've tested the different combinations.

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'll change it to that then, just in case there is some odd case I'm not aware of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I tried that, and it has it's own weird problems of URLWithString succeeding and generating not nil, but incorrect file urls from the full paths. So I think I will go with checking for "file://", since it does seem to be necessary for the writeToFile logic anyway.

// Otherwise, assume it is a file path.
local_file_url = [NSURL fileURLWithPath:path_string];
}
// If we still failed to convert the path, error out.
if (local_file_url == nil) {
future_impl->Complete(handle, kErrorUnknown,
"Unable to convert provided path to valid URL");
return GetFileLastResult();
}
util::DispatchAsyncSafeMainQueue(^() {
FIRStorageDownloadTask *download_task;
{
Expand Down
Loading