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
@@ -162,4 +162,29 @@ - (void)testCopy {
XCTAssertNotEqual(ref, copiedRef);
}

- (void)testReferenceWithNonExistentFileFailsWithCompletion {
NSString *tempFilePath = [NSTemporaryDirectory() stringByAppendingPathComponent:@"temp.data"];
FIRStorageReference *ref = [self.storage referenceWithPath:tempFilePath];

NSURL *dummyFileURL = [NSURL fileURLWithPath:@"some_non_existing-folder/file.data"];

XCTestExpectation *expectation = [self expectationWithDescription:@"completionExpectation"];

[ref putFile:dummyFileURL
metadata:nil
completion:^(FIRStorageMetadata *_Nullable metadata, NSError *_Nullable error) {
[expectation fulfill];
XCTAssertNotNil(error);
XCTAssertNil(metadata);

XCTAssertEqualObjects(error.domain, FIRStorageErrorDomain);
XCTAssertEqual(error.code, FIRStorageErrorCodeUnknown);
NSString *expectedDescription = [NSString
stringWithFormat:@"File at URL: %@ is not reachable.", dummyFileURL.absoluteString];
XCTAssertEqualObjects(error.localizedDescription, expectedDescription);
}];

[self waitForExpectationsWithTimeout:0.5 handler:NULL];
}

@end
@@ -79,6 +79,13 @@ - (void)enqueue {
return;
}

NSError *contentValidationError;
if (![strongSelf isContentToUploadValid:&contentValidationError]) {
strongSelf.error = contentValidationError;
[strongSelf finishTaskWithStatus:FIRStorageTaskStatusFailure snapshot:strongSelf.snapshot];
return;
}

strongSelf.state = FIRStorageTaskStateQueueing;

NSMutableURLRequest *request = [strongSelf.baseRequest mutableCopy];
@@ -145,9 +152,8 @@ - (void)enqueue {
self.state = FIRStorageTaskStateFailed;
self.error = [FIRStorageErrors errorWithServerError:error reference:self.reference];
self.metadata = self->_uploadMetadata;
[self fireHandlersForStatus:FIRStorageTaskStatusFailure snapshot:self.snapshot];
[self removeAllObservers];
self->_fetcherCompletion = nil;

[self finishTaskWithStatus:FIRStorageTaskStatusFailure snapshot:self.snapshot];
return;
}

@@ -164,9 +170,7 @@ - (void)enqueue {
self.error = [FIRStorageErrors errorWithInvalidRequest:data];
}

[self fireHandlersForStatus:FIRStorageTaskStatusSuccess snapshot:self.snapshot];
[self removeAllObservers];
self->_fetcherCompletion = nil;
[self finishTaskWithStatus:FIRStorageTaskStatusSuccess snapshot:self.snapshot];
};
#pragma clang diagnostic pop

@@ -177,6 +181,37 @@ - (void)enqueue {
}];
}

- (void)finishTaskWithStatus:(FIRStorageTaskStatus)status
snapshot:(FIRStorageTaskSnapshot *)snapshot {
[self fireHandlersForStatus:status snapshot:self.snapshot];
[self removeAllObservers];
self->_fetcherCompletion = nil;
}

- (BOOL)isContentToUploadValid:(NSError **)outError {
if (_uploadData != nil) {
return YES;
}

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

This comment has been minimized.

Copy link
@ryanwilson

ryanwilson Feb 28, 2019

Member

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.

This comment has been minimized.

Copy link
@maksymmalyhin

maksymmalyhin Feb 28, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@ryanwilson

ryanwilson Feb 28, 2019

Member

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

This comment has been minimized.

Copy link
@maksymmalyhin

maksymmalyhin Feb 28, 2019

Author Contributor

Created - #2459

This comment has been minimized.

Copy link
@schmidt-sebastian

schmidt-sebastian Mar 2, 2019

Member

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.

This comment has been minimized.

Copy link
@maksymmalyhin

maksymmalyhin Mar 3, 2019

Author Contributor

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.

userInfo:@{
NSUnderlyingErrorKey : fileReachabilityError,

This comment has been minimized.

Copy link
@LinusU

LinusU Apr 17, 2019

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...

This comment has been minimized.

Copy link
@maksymmalyhin

maksymmalyhin Apr 17, 2019

Author Contributor

@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.

This comment has been minimized.

Copy link
@LinusU

LinusU Apr 17, 2019

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

This comment has been minimized.

Copy link
@maksymmalyhin

maksymmalyhin Apr 17, 2019

Author Contributor

@LinusU thanks!

NSLocalizedDescriptionKey : description
}];
}

return NO;
}

return YES;
}

#pragma mark - Upload Management

- (void)cancel {
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.