-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Changes from 10 commits
bd55a09
ef986b9
0f11535
92bca47
fc70bdb
5009de3
6a731ae
c335c43
3dbf22c
1155787
6c4ee95
d376ee1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,6 +72,13 @@ - (void)dealloc { | |
- (void)enqueue { | ||
__weak FIRStorageUploadTask *weakSelf = self; | ||
|
||
NSError *contentValidationError; | ||
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]; | ||
*outError = [NSError errorWithDomain:FIRStorageErrorDomain | ||
code:FIRStorageErrorCodeUnknown | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Created - #2459 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My concern with |
||
userInfo:@{ | ||
NSUnderlyingErrorKey : fileReachabilityError, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm trying to upload an mp4 file from Flutter with
For some reason 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 edit2: the problem is that There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 edit: here we go 👉 #2852 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, that makes sense