Skip to content

FIRStorageUploadTask: fail on invalid file URL #2458

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 12 commits into from
Mar 4, 2019
Merged

FIRStorageUploadTask: fail on invalid file URL #2458

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
bd55a09
FIRStorageUploadTask: validate data and fileURL before upload (#2350)
maksymmalyhin Feb 27, 2019
ef986b9
FIRStorageReferenceTests: Verify a specific error (#2350)
maksymmalyhin Feb 27, 2019
0f11535
Code cleanup. More informative error.
maksymmalyhin Feb 28, 2019
92bca47
FIRStorageReference: error message, test fix (#2350)
maksymmalyhin Feb 28, 2019
fc70bdb
Run ./scripts/style.sh
maksymmalyhin Feb 28, 2019
5009de3
#2350: Punctuation
maksymmalyhin Feb 28, 2019
6a731ae
FIRStorageReferenceTests: naming (#2350)
maksymmalyhin Feb 28, 2019
c335c43
Run ./scripts/style.sh
maksymmalyhin Feb 28, 2019
3dbf22c
Tests fixed (#2350)
maksymmalyhin Feb 28, 2019
1155787
FIRStorageReferenceTests fixed for Mac OS (#2350)
maksymmalyhin Feb 28, 2019
6c4ee95
Merge branch 'master' into mm-2350-putFileCompletion
maksymmalyhin Mar 3, 2019
d376ee1
FIRStorageUploadTask: validate content just before uploading (#2350)
maksymmalyhin Mar 3, 2019
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
24 changes: 24 additions & 0 deletions Example/Storage/Tests/Unit/FIRStorageReferenceTests.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,4 +162,28 @@ - (void)testCopy {
XCTAssertNotEqual(ref, copiedRef);
}

- (void)test_GivenReferenceWithPath_WhenPutNonExistingFile_ThenCompletionIsCalledWithError {
Copy link
Member

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?

Copy link
Contributor Author

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.

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
47 changes: 41 additions & 6 deletions Firebase/Storage/FIRStorageUploadTask.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,13 @@ - (void)dealloc {
- (void)enqueue {
__weak FIRStorageUploadTask *weakSelf = self;

NSError *contentValidationError;
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, that makes sense

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

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

Expand Down Expand Up @@ -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;
}

Expand All @@ -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

Expand All @@ -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];
Copy link
Member

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.

*outError = [NSError errorWithDomain:FIRStorageErrorDomain
code:FIRStorageErrorCodeUnknown
Copy link
Member

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.

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 may create an issue for that so we don't forget about it.

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created - #2459

Copy link
Contributor

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.

Copy link
Contributor Author

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.

userInfo:@ {
NSUnderlyingErrorKey: fileReachabilityError,
NSLocalizedDescriptionKey: description
}];
}

return NO;
}

return YES;
}

#pragma mark - Upload Management

- (void)cancel {
Expand Down