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
25 changes: 25 additions & 0 deletions Example/Storage/Tests/Unit/FIRStorageReferenceTests.m
Expand Up @@ -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);
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
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];
*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,
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...

Copy link
Contributor Author

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.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LinusU thanks!

NSLocalizedDescriptionKey : description
}];
}

return NO;
}

return YES;
}

#pragma mark - Upload Management

- (void)cancel {
Expand Down
7 changes: 4 additions & 3 deletions Firestore/Source/Local/FSTLocalStore.mm
Expand Up @@ -376,9 +376,10 @@ - (nullable FSTMutationBatch *)nextMutationBatchAfterBatchID:(BatchId)batchID {
}

- (nullable FSTMaybeDocument *)readDocument:(const DocumentKey &)key {
return self.persistence.run("ReadDocument", [&]() -> FSTMaybeDocument *_Nullable {
return [self.localDocuments documentForKey:key];
});
return self.persistence.run(
"ReadDocument", [&]() -> FSTMaybeDocument *_Nullable {
return [self.localDocuments documentForKey:key];
});
}

- (FSTQueryData *)allocateQuery:(FSTQuery *)query {
Expand Down
6 changes: 4 additions & 2 deletions Firestore/core/src/firebase/firestore/model/field_value.cc
Expand Up @@ -468,7 +468,8 @@ void FieldValue::SwitchTo(const Type type) {
case Type::Object:
object_value_.~unique_ptr<ObjectValue>();
break;
default: {} // The other types where there is nothing to worry about.
default: {
} // The other types where there is nothing to worry about.
}
tag_ = type;
// Must call constructor explicitly for any non-POD type to initialize.
Expand Down Expand Up @@ -506,7 +507,8 @@ void FieldValue::SwitchTo(const Type type) {
new (&object_value_)
std::unique_ptr<ObjectValue>(absl::make_unique<ObjectValue>());
break;
default: {} // The other types where there is nothing to worry about.
default: {
} // The other types where there is nothing to worry about.
}
}

Expand Down