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

FIRStorageUploadTask: fail on invalid file URL #2458

Merged
merged 12 commits into from Mar 4, 2019

Conversation

@maksymmalyhin
Copy link
Contributor

@maksymmalyhin maksymmalyhin commented Feb 28, 2019

Partial fix for issue #2350.

  • Fail early with an informative error in case of an invalid file URL.

Another part of the fix has to be done in GTMSessionUploadFetcher (following issue)

@bstpierr
Copy link
Contributor

@bstpierr bstpierr commented Feb 28, 2019

It looks like the style linter needs to be run.

You might need to overwrite the Google version of clang-format.

Loading

@maksymmalyhin
Copy link
Contributor Author

@maksymmalyhin maksymmalyhin commented Feb 28, 2019

@bstpierr Thank you, I'll do that. Probably, I'll add the linter to my git hooks to avoid such errors in future.

Loading

@ryanwilson ryanwilson requested review from schmidt-sebastian and removed request for asciimike Feb 28, 2019
Copy link
Member

@ryanwilson ryanwilson left a comment

Please fix the linting error and wait for a review from @schmidt-sebastian, otherwise LGTM.

Loading

if (![_fileURL checkResourceIsReachableAndReturnError:&fileReachabilityError]) {
if (outError != NULL) {
NSString *description =
[NSString stringWithFormat:@"File at URL: %@ is not reachable", _fileURL.absoluteString];
Copy link
Member

@ryanwilson ryanwilson Feb 28, 2019

Choose a reason for hiding this comment

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

Nit: punctuation at the end of the string.

Loading

NSString *description =
[NSString stringWithFormat:@"File at URL: %@ is not reachable", _fileURL.absoluteString];
*outError = [NSError errorWithDomain:FIRStorageErrorDomain
code:FIRStorageErrorCodeUnknown
Copy link
Member

@ryanwilson ryanwilson Feb 28, 2019

Choose a reason for hiding this comment

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

I think this is fine for now, but we probably want to add a case to the to the FIRStorageErrorCode enum.

That will require an API review (I can go over the process with you) so we won't be able to do it this release.

Loading

Copy link
Contributor Author

@maksymmalyhin maksymmalyhin Feb 28, 2019

Choose a reason for hiding this comment

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

I may create an issue for that so we don't forget about it.

Loading

Copy link
Member

@ryanwilson ryanwilson Feb 28, 2019

Choose a reason for hiding this comment

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

Once this is approved by the Storage team and lands, that's a great idea.

Loading

Copy link
Contributor Author

@maksymmalyhin maksymmalyhin Feb 28, 2019

Choose a reason for hiding this comment

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

Created - #2459

Loading

Copy link
Member

@schmidt-sebastian schmidt-sebastian Mar 2, 2019

Choose a reason for hiding this comment

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

Just throwing this out there: Did you consider FIRStorageErrorCodeObjectNotFound? It's used today when a file doesn't exist on the backend, which isn't all that different from a locally missing file.

Loading

Copy link
Contributor Author

@maksymmalyhin maksymmalyhin Mar 3, 2019

Choose a reason for hiding this comment

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

My concern with FIRStorageErrorCodeObjectNotFound is following: a typical file upload operation involves a remote object reference and a file URL, so if we have a single error code for both it may be confusing for users.

Loading

@@ -162,4 +162,28 @@ - (void)testCopy {
XCTAssertNotEqual(ref, copiedRef);
}

- (void)test_GivenReferenceWithPath_WhenPutNonExistingFile_ThenCompletionIsCalledWithError {
Copy link
Member

@ryanwilson ryanwilson Feb 28, 2019

Choose a reason for hiding this comment

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

Although I do think this is a better naming format, it doesn't match the naming for the rest of the file. Maybe something like testReferenceWithNonExistentFileFailsWithCompletion would match better?

Loading

Copy link
Contributor Author

@maksymmalyhin maksymmalyhin Feb 28, 2019

Choose a reason for hiding this comment

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

yeah, sounds good, I'll correct this.

Loading

Copy link
Member

@schmidt-sebastian schmidt-sebastian left a comment

One question about the location of the existence check, but otherwise looks good!

Loading

@@ -72,6 +72,13 @@ - (void)dealloc {
- (void)enqueue {
__weak FIRStorageUploadTask *weakSelf = self;

NSError *contentValidationError;
Copy link
Member

@schmidt-sebastian schmidt-sebastian Mar 2, 2019

Choose a reason for hiding this comment

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

Should this be move to the dispatchAsync block below to execute close to the actual upload?

Loading

Copy link
Contributor Author

@maksymmalyhin maksymmalyhin Mar 3, 2019

Choose a reason for hiding this comment

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

yeah, that makes sense

Loading

NSString *description =
[NSString stringWithFormat:@"File at URL: %@ is not reachable", _fileURL.absoluteString];
*outError = [NSError errorWithDomain:FIRStorageErrorDomain
code:FIRStorageErrorCodeUnknown
Copy link
Member

@schmidt-sebastian schmidt-sebastian Mar 2, 2019

Choose a reason for hiding this comment

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

Just throwing this out there: Did you consider FIRStorageErrorCodeObjectNotFound? It's used today when a file doesn't exist on the backend, which isn't all that different from a locally missing file.

Loading

@schmidt-sebastian schmidt-sebastian merged commit 4dfebfc into master Mar 4, 2019
2 checks passed
Loading
@schmidt-sebastian
Copy link
Member

@schmidt-sebastian schmidt-sebastian commented Mar 4, 2019

Thanks for your contribution @maksymmalyhin!

Loading

@maksymmalyhin
Copy link
Contributor Author

@maksymmalyhin maksymmalyhin commented Mar 4, 2019

@schmidt-sebastian I'm happy to help!

Loading

@maksymmalyhin maksymmalyhin deleted the mm-2350-putFileCompletion branch Mar 4, 2019
@maksymmalyhin maksymmalyhin self-assigned this Mar 15, 2019
*outError = [NSError errorWithDomain:FIRStorageErrorDomain
code:FIRStorageErrorCodeUnknown
userInfo:@{
NSUnderlyingErrorKey : fileReachabilityError,
Copy link

@LinusU LinusU Apr 17, 2019

Choose a reason for hiding this comment

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

I'm trying to upload an mp4 file from Flutter with firebase_storage (got the file from image_picker, seems to work fine with jpeg from image_picker) and I'm hitting an app crash here.

fileReachabilityError is nil and thus I'm getting an NSInvalidException since you cannot add nil values to an NSDitctionary.

For some reason _fileURL is nil, which makes [_fileURL checkResourceIsReachableAndReturnError:&fileReachabilityError] return just nil, which triggers the if (!...) branch.

Sorry for not providing too much info, I'm still in the process of tracking it down but thought it would be good to see if anyone had any ideas ☺️

ping @maksymmalyhin

edit: the problem is that both _uploadData and _fileURL is null, because it was created with a nil data from Flutter

edit2: the problem is that image_picker returns a File object with a path like /file:///foo/bar/baz.mp4...

Loading

Copy link
Contributor Author

@maksymmalyhin maksymmalyhin Apr 17, 2019

Choose a reason for hiding this comment

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

@LinusU Thank you for the report. That's a good case. Yeah, this part of code definitely does not expect _fileURL to be nil. We will fix it.

In the meanwhile, would you be able to post a comment about it to #2350 or create a separate ticket, so it is not lost.

As a workaround you should check the fileURL != nil before passing it to the putFile method.

Loading

Copy link

@LinusU LinusU Apr 17, 2019

Choose a reason for hiding this comment

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

I've submitted flutter/plugins#1504 and flutter/plugins#1505 to fix this in Flutter, but would be great to see a fix here as well ☺️

I'll put a comment in the issue you linked open a new issue about it here as well!

edit: here we go 👉 #2852

Loading

Copy link
Contributor Author

@maksymmalyhin maksymmalyhin Apr 17, 2019

Choose a reason for hiding this comment

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

@LinusU thanks!

Loading

@firebase firebase locked and limited conversation to collaborators Oct 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants