-
Notifications
You must be signed in to change notification settings - Fork 902
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
[WIP] Introduce UploadService to storage emulator #4233
Conversation
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.
I really like this, upload.ts
is such a happy file. Left some minor comments. Also, you may need to rebase for some of the cancel logic, since I've merged #4210.
* @throws {NotFoundError} if the resumable upload does not exist. | ||
* @throws {NotCancellableError} if the resumable upload can not be cancelled. | ||
*/ | ||
public cancelResumableUpload(uploadId: string): 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.
I would pass an Upload
rather than its id here and have the caller worry about NotFoundError
, similarly for finalise. I've floated this idea before... Thoughts?
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.
I like the pattern, but I wasn't able to make it work for the following reason: since an Upload
is essentially just a js object, any caller can construct arbitrary, non-db backed instances of it.
Ex:
# some bad caller or bug
public continueNonexistentUpload() {
uploadService.continueResumableUpload({
id: 'someNonexistentUploadId',
status: ACTIVE
...
});
}
public finalizeNonexistentUpload() {
uploadService.finalizeResumableUpload({
id: 'bogusValue',
status: 'ACTIVE'
});
}
Just because an Upload
exists, doesn't mean that its registered in UploadService
and unless UploadService
assumes all Upload
objects passed to it are valid, we still need to verify the state of the Upload
in memory or else we run the risk of saving bogus data to our internal state.
Welp, this PR kind of got away from me and ended up being pretty large. This is in part due to the additional refactor of the multipart parsing logic. |
Description
Pulls all upload related logic out of
StorageLayer
into a separateUploadService
. Updates thefirebase.ts
andgcloud.ts
to use the newUploadService
.Other changes:
Persistence
intopersistence.ts
Persistence
intoStorageLayer
constructor since it's now shared withUploadService
(and DI is just good practice)StorageLayer
intoStorageEmulator
multipart.ts
, refactors it to be more flexible, adds spec tests