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

[WIP] Introduce UploadService to storage emulator #4233

Closed
wants to merge 39 commits into from

Conversation

tonyjhuang
Copy link
Contributor

@tonyjhuang tonyjhuang commented Mar 1, 2022

Description

Pulls all upload related logic out of StorageLayer into a separate UploadService. Updates the firebase.ts and gcloud.ts to use the new UploadService.

Other changes:

  • Pulls Persistence into persistence.ts
  • Injects Persistence into StorageLayer constructor since it's now shared with UploadService (and DI is just good practice)
  • Migrates reset logic out of StorageLayer into StorageEmulator
  • Pulls multipart request body parsing out into multipart.ts, refactors it to be more flexible, adds spec tests

@tonyjhuang tonyjhuang changed the title Tonyjhuang/upload Introduce UploadService to storage emulator Mar 1, 2022
Copy link
Member

@tohhsinpei tohhsinpei left a 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.

src/emulator/storage/apis/firebase.ts Outdated Show resolved Hide resolved
src/emulator/storage/apis/firebase.ts Show resolved Hide resolved
src/emulator/storage/apis/firebase.ts Outdated Show resolved Hide resolved
src/emulator/storage/apis/firebase.ts Outdated Show resolved Hide resolved
src/emulator/storage/apis/firebase.ts Outdated Show resolved Hide resolved
src/emulator/storage/upload.ts Outdated Show resolved Hide resolved
src/emulator/storage/upload.ts Outdated Show resolved Hide resolved
* @throws {NotFoundError} if the resumable upload does not exist.
* @throws {NotCancellableError} if the resumable upload can not be cancelled.
*/
public cancelResumableUpload(uploadId: string): Upload {
Copy link
Member

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?

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

src/emulator/storage/upload.ts Outdated Show resolved Hide resolved
src/emulator/storage/upload.ts Show resolved Hide resolved
@tonyjhuang
Copy link
Contributor Author

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.

@tonyjhuang tonyjhuang requested review from sam-gc and removed request for sam-gc March 3, 2022 02:41
@tonyjhuang tonyjhuang marked this pull request as draft March 3, 2022 03:05
@tonyjhuang tonyjhuang changed the title Introduce UploadService to storage emulator [WIP] Introduce UploadService to storage emulator Mar 3, 2022
Base automatically changed from tonyjhuang/refactor-get to master March 3, 2022 15:42
@tonyjhuang tonyjhuang closed this Mar 9, 2022
@tonyjhuang tonyjhuang deleted the tonyjhuang/upload branch March 9, 2022 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants