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

- (void)test_GivenReferenceWithPath_WhenPutNonExistingFile_ThenCompletionIsCalledWithError {

This comment has been minimized.

Copy link
@ryanwilson

ryanwilson Feb 28, 2019

Member

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?

This comment has been minimized.

Copy link
@maksymmalyhin

maksymmalyhin Feb 28, 2019

Author Contributor

yeah, sounds good, I'll correct this.

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);
XCTAssertEqualObjects(error.localizedDescription,
@"File at URL: file:///some_non_existing-folder/file.data is not reachable");
}];

[self waitForExpectationsWithTimeout:0.5 handler:NULL];
}

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

NSError *contentValidationError;

This comment has been minimized.

Copy link
@schmidt-sebastian

schmidt-sebastian Mar 2, 2019

Member

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

This comment has been minimized.

Copy link
@maksymmalyhin

maksymmalyhin Mar 3, 2019

Author Contributor

yeah, that makes sense

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

[self dispatchAsync:^() {
FIRStorageUploadTask *strongSelf = weakSelf;

@@ -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];

This comment has been minimized.

Copy link
@ryanwilson

ryanwilson Feb 28, 2019

Member

Nit: punctuation at the end of the string.

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