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.

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

Jump to bottom

fix: refactor createReadStream to remove unnecessary stream #2153

Merged
merged 16 commits into from
Mar 2, 2023
Merged

fix: refactor createReadStream to remove unnecessary stream #2153

merged 16 commits into from
Mar 2, 2023

Conversation

ddelgrosso1
Copy link
Contributor

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

@ddelgrosso1 ddelgrosso1 requested review from a team as code owners February 17, 2023 16:23
@product-auto-label product-auto-label bot added size: l Pull request size is large. api: storage Issues related to the googleapis/nodejs-storage API. labels Feb 17, 2023
@ddelgrosso1 ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 17, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 17, 2023
@ddelgrosso1 ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 17, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 17, 2023
@ddelgrosso1 ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 17, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 17, 2023
@ddelgrosso1
Copy link
Contributor Author

Summary of things change: 1. De-nest all the callbacks in createReadStream so it is a bit easier to read 2. Move validation logic into HashStreamValidator 3. Remove HandoffStream which was only functioning to delay validation until all data was available.

@ddelgrosso1 ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 17, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 17, 2023
shaffeeullah
shaffeeullah approved these changes Feb 22, 2023
View reviewed changes
Copy link
Contributor

@shaffeeullah shaffeeullah left a comment

Choose a reason for hiding this comment

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

thanks, denis!!

danielbankhead
danielbankhead reviewed Feb 22, 2023
View reviewed changes
Copy link
Contributor

@danielbankhead danielbankhead left a comment

Choose a reason for hiding this comment

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

Added some suggestions, mostly for the hashstream validator

src/hash-stream-validator.ts Show resolved Hide resolved
src/hash-stream-validator.ts Outdated Show resolved Hide resolved
src/hash-stream-validator.ts Outdated Show resolved Hide resolved
src/hash-stream-validator.ts Outdated Show resolved Hide resolved
src/file.ts Show resolved Hide resolved
src/hash-stream-validator.ts Outdated Show resolved Hide resolved
src/file.ts Outdated Show resolved Hide resolved
src/file.ts Outdated Show resolved Hide resolved
src/file.ts Outdated Show resolved Hide resolved
src/file.ts Outdated Show resolved Hide resolved
ddelgrosso1 and others added 11 commits February 23, 2023 09:04
@ddelgrosso1 @danielbankhead
Update src/hash-stream-validator.ts
956f04d 
Co-authored-by: Daniel Bankhead <[email protected]>
@ddelgrosso1 @danielbankhead
Update src/hash-stream-validator.ts
4b75af2 
Co-authored-by: Daniel Bankhead <[email protected]>
@ddelgrosso1 @danielbankhead
Update src/hash-stream-validator.ts
f006c5c 
Co-authored-by: Daniel Bankhead <[email protected]>
@ddelgrosso1 @danielbankhead
Update src/hash-stream-validator.ts
d8f822f 
Co-authored-by: Daniel Bankhead <[email protected]>
@ddelgrosso1 @danielbankhead
Update src/file.ts
66f73dc 
Co-authored-by: Daniel Bankhead <[email protected]>
@ddelgrosso1 @danielbankhead
Update src/hash-stream-validator.ts
c6867fe 
Co-authored-by: Daniel Bankhead <[email protected]>
@ddelgrosso1 @danielbankhead
Update src/hash-stream-validator.ts
68500eb 
Co-authored-by: Daniel Bankhead <[email protected]>
@ddelgrosso1 @danielbankhead
Update src/file.ts
8308462 
Co-authored-by: Daniel Bankhead <[email protected]>
@ddelgrosso1 @danielbankhead
Update src/file.ts
764807e 
Co-authored-by: Daniel Bankhead <[email protected]>
@ddelgrosso1 @danielbankhead
Update src/file.ts
2c62893 
Co-authored-by: Daniel Bankhead <[email protected]>
@ddelgrosso1 @danielbankhead
Update src/file.ts
7d9b03a 
Co-authored-by: Daniel Bankhead <[email protected]>
@ddelgrosso1
Copy link
Contributor Author

@danielbankhead I accepted most of the suggestions but had to fix up a few things. We cannot deduce whether or not crc32c or md5 checks are enabled purely by the presence of a hash. The server returns these values in the headers and the user may not want the validation so the boolean has to be set from user land. Additionally, createWriteStream uses the HashStreamValidator to calculate hashes when uploading. For this reason I had to bring back the updateHashesOnly flag so that it may be passed in the appropriate place so that validation is not performed.

@ddelgrosso1 ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 23, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 23, 2023
danielbankhead
danielbankhead reviewed Feb 23, 2023
View reviewed changes
src/hash-stream-validator.ts
Comment on lines +76 to +94
if (this.updateHashesOnly) {
callback();
return;
}

// If we're doing validation, assume the worst-- a data integrity
// mismatch. If not, these tests won't be performed, and we can assume
// the best.
// We must check if the server decompressed the data on serve because hash
// validation is not possible in this case.
let failed = this.crc32cEnabled || this.md5Enabled;

if (this.crc32cEnabled && this.crc32cExpected) {
failed = !this.test('crc32c', this.crc32cExpected);
}

if (this.md5Enabled && this.md5Expected) {
failed = !this.test('md5', this.md5Expected);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I see the confusion - I think this block should only be checked in the presence of this.crc32cExpected or this.md5Expected (otherwise, if either aren't available then this would always fail).

danielbankhead
danielbankhead approved these changes Feb 23, 2023
View reviewed changes
Copy link
Contributor

@danielbankhead danielbankhead left a comment

Choose a reason for hiding this comment

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

LGTM, the _flush suggestion is optional

@ddelgrosso1 ddelgrosso1 merged commit 2c97310 into googleapis:main Mar 2, 2023
@ddelgrosso1 ddelgrosso1 deleted the refactor-createreadstream branch March 2, 2023 15:15
@release-please release-please bot mentioned this pull request Mar 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shaffeeullah shaffeeullah shaffeeullah approved these changes

@danielbankhead danielbankhead danielbankhead approved these changes

Assignees
No one assigned
Labels
api: storage Issues related to the googleapis/nodejs-storage API. size: l Pull request size is large.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ddelgrosso1 @danielbankhead @shaffeeullah