fix: don't fail if ~/.config doesn't exist #1428

shaffeeullah · 2021-03-23T20:20:00Z

Fixes #1420 🦕

…affeeullah/succeedIfConfigDoesntExist

codecov · 2021-03-23T20:25:56Z

Codecov Report

Merging #1428 (1aa3608) into master (bd9ef18) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1428   +/-   ##
=======================================
  Coverage   99.10%   99.10%           
=======================================
  Files          14       14           
  Lines       12163    12187   +24     
  Branches      604      532   -72     
=======================================
+ Hits        12054    12078   +24     
  Misses        109      109

Impacted Files	Coverage Δ
src/file.ts	`99.94% <100.00%> (+<0.01%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd9ef18...1aa3608. Read the comment docs.

JustinBeckwith · 2021-03-23T21:23:29Z

src/file.ts

@@ -1802,7 +1802,9 @@ class File extends ServiceObject<File> {
      const configDir = xdgBasedir.config || os.tmpdir();

      fs.access(configDir, fs.constants.W_OK, err => {
-        if (err) {
+        if (err && fs.existsSync(configDir)) {


Generally you want to avoid calling Sync methods inside of production level code paths. They block the node.js event loop, and file system access is slow. If we wanted to keep down this path, I think we'd have to make 2 distinct calls to fs.access with varying flags: one with R_OK and one with W_OK.

That having been said ... the pattern of checking for a given directory or file before starting a write is generally frowned upon. This is specifically called out in the fs docs:

Do not use fs.access() to check for the accessibility of a file before calling fs.open(), fs.readFile() or fs.writeFile(). Doing so introduces a race condition, since other processes may change the file's state between the two calls. Instead, user code should open/read/write the file directly and handle the error raised if the file is not accessible.

From here:
https://nodejs.org/api/fs.html#fs_fs_access_path_mode_callback

Should we instead handle errors that come back directly from startSimpleUpload_, and give a good error message based on the resulting stream?

Adding to the first point - if we decide to keep going down the pre-check route, it's possible you can do a single fs.stat and do bit math to play with the resulting mode - I'm out of my depth here though (@bcoe may know more)

…affeeullah/succeedIfConfigDoesntExist

stephenplusplus · 2021-03-29T20:44:29Z

src/file.ts

+              if (options.resumable) {
+                const error = new ResumableUploadError(
+                  [
+                    'A resumable upload could not be performed. The directory,',


I think there are two causes of failures now:

The directory doesn't exist, and can't be created

The directory exists, but cannot be written to

Should we generalize the error message or maybe do a separate message for each?

Great callout! I've created a separate issue to track this here.

shaffeeullah · 2021-03-29T21:07:56Z

src/file.ts

+          this.startResumableUpload_(fileWriteStream, options);
+        };
+        maybeCreateFolder().catch(e => {
+          throw new Error(e);


This line is not covered by code coverage, but I'm not sure how to get to this case. This error shouldn't get thrown. Any ideas for how to write a test for this are welcome.

Maybe related, do we need the function “maybeCreateFolder”? Can the code inside simply execute when the fs.access callback runs?

Good point. Fixed!

stephenplusplus · 2021-03-30T16:47:49Z

src/file.ts

@@ -1801,26 +1801,30 @@ class File extends ServiceObject<File> {
      // https://github.com/yeoman/configstore/blob/f09f067e50e6a636cfc648a6fc36a522062bd49d/index.js#L11
      const configDir = xdgBasedir.config || os.tmpdir();

-      fs.access(configDir, fs.constants.W_OK, err => {


I attempted a bit of a code re-sort and added comments for clarity. Hopefully it's not too much, and it's helpful for future developers. Feel free to take only pieces if there are any parts that you think are useful:

// The logic below attempts to mimic the resumable upload library, // gcs-resumable-upload. That library requires a writable configuration // directory in order to work. If we wait for that library to discover any // issues, we've already started a resumable upload which is difficult to back // out of. We want to catch any errors first, so we can choose a simple, non- // resumable upload instead. // Same as configstore (used by gcs-resumable-upload): // https://github.com/yeoman/configstore/blob/f09f067e50e6a636cfc648a6fc36a522062bd49d/index.js#L11 const configDir = xdgBasedir.config || os.tmpdir(); fs.access(configDir, fs.constants.W_OK, accessErr => { if (!accessErr) { // A configuration directory exists, and it's writable. gcs-resumable-upload // should have everything it needs to work. this.startResumableUpload_(fileWriteStream, options); return; }; // The configuration directory is either not writable, or it doesn't exist. // gcs-resumable-upload will attempt to create it for the user, but we'll try // it now to confirm that it won't have any issues. That way, if we catch the // issue before we start the resumable upload, we can instead start a simple // upload. fs.mkdir(configDir, {mode: 0o0700}, err => { if (!err) { // We successfully created a configuration directory that // gcs-resumable-upload will use. this.startResumableUpload_(fileWriteStream, options); return; } if (options.resumable) { // The user wanted a resumable upload, but we couldn't create a // configuration directory, which means gcs-resumable-upload will fail. const error = new ResumableUploadError( [ 'A resumable upload could not be performed. The directory,', `${configDir}, is not writable. You may try another upload,`, 'this time setting `options.resumable` to `false`.', ].join(' ') ); stream.destroy(error); } else { // The user didn't care, resumable or not. Fall back to simple upload. this.startSimpleUpload_(fileWriteStream, options); } }); });

I really like this! Thanks for the additions. I like that it notes the extent to which we bleed the requirements of gcs-resumable-upload into this library.

shaffeeullah · 2021-03-30T19:55:09Z

test/file.ts

+      });
+
+      it('should succeed if resumable requested but config dir doesnt exist', done => {
+        assert.strictEqual(fs.existsSync(configDir), false);


fs.access(configDir, fs.constants.W_OK, accessErr => { is not throwing an error for this test and I'm really confused as to why. @stephenplusplus any ideas?

I believe that was because we were stubbing fs.access earlier in the test file to always pass. Checking into this caused me to get in the editor and write a bit of code as I worked through the issue, and I ended up with a few tests to cover the changes in the PR. I'm sorry I keep doing this. It seems to work out that when I'm investigating an issue, I end up writing a bunch of code to test my theories out. So, here is what I ended up with: https://gist.github.com/stephenplusplus/dccf237542b7a702ebff90f03f2a557a. Don't worry about using any of it if you wanted to go a different way with the tests.

No need to apologize! Thanks so much for doing this!!! Super super helpful :) Really appreciate it

Good catch noting that fs.access was being stubbed. I totally missed that.

@stephenplusplus

…7542b7a702ebff90f03f2a557a provided by @stephenplusplus

stephenplusplus · 2021-04-01T15:35:15Z

package.json

@@ -111,6 +112,7 @@
    "node-fetch": "^2.2.0",
    "normalize-newline": "^3.0.0",
    "proxyquire": "^2.1.3",
+    "rimraf": "^3.0.2",


I think we can remove this dependency now.

stephenplusplus · 2021-04-01T16:56:25Z

Thank you! LGTM!

shaffeeullah added 2 commits March 23, 2021 20:18

fix: don't fail if ~/.config doesn't exist

2061a4c

Merge branch 'master' of github.com:googleapis/nodejs-storage into sh…

2ab5d96

…affeeullah/succeedIfConfigDoesntExist

shaffeeullah requested a review from a team March 23, 2021 20:20

shaffeeullah requested a review from a team as a code owner March 23, 2021 20:20

product-auto-label bot added the api: storage Issues related to the googleapis/nodejs-storage API. label Mar 23, 2021

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Mar 23, 2021

shaffeeullah requested a review from stephenplusplus March 23, 2021 20:20

linted files

18fed40

shaffeeullah added 2 commits March 23, 2021 20:55

refactored if statement

1254078

refactored tests

cfd7935

JustinBeckwith suggested changes Mar 23, 2021

View reviewed changes

shaffeeullah added 9 commits March 29, 2021 18:31

refactored code

a18e0b2

Merge branch 'master' of github.com:googleapis/nodejs-storage into sh…

b16534f

…affeeullah/succeedIfConfigDoesntExist

log statments to debug remote failing test

76c9127

log statments to debug remote failing test

899bd7b

log statments to debug remote failing test

25421cd

log statments to debug remote failing test

7e7494a

updated test to ensure directory exists

b64c0b5

removed logging statements

6c0b6eb

linted files

ddc8e36

shaffeeullah requested a review from JustinBeckwith March 29, 2021 20:29

stephenplusplus reviewed Mar 29, 2021

View reviewed changes

shaffeeullah requested a review from frankyn March 29, 2021 20:49

shaffeeullah commented Mar 29, 2021

View reviewed changes

shaffeeullah mentioned this pull request Mar 29, 2021

Improve error message for resumable uploads #1431

Closed

refactored code

36c46d5

shaffeeullah requested a review from stephenplusplus March 30, 2021 15:49

frankyn approved these changes Mar 30, 2021

View reviewed changes

stephenplusplus reviewed Mar 30, 2021

View reviewed changes

added comments suggested by @stephenplusplus

dbdcbc0

shaffeeullah requested a review from stephenplusplus March 30, 2021 17:03

shaffeeullah commented Mar 30, 2021

View reviewed changes

shaffeeullah added 2 commits March 31, 2021 20:42

test updated from diff https://gist.github.com/stephenplusplus/dccf23…

b1ab1cc

…7542b7a702ebff90f03f2a557a provided by @stephenplusplus

Merge branch 'master' into shaffeeullah/succeedIfConfigDoesntExist

04f6353

JustinBeckwith approved these changes Mar 31, 2021

View reviewed changes

stephenplusplus suggested changes Apr 1, 2021

View reviewed changes

removed unused dependency

1aa3608

shaffeeullah requested a review from stephenplusplus April 1, 2021 16:01

stephenplusplus approved these changes Apr 1, 2021

View reviewed changes

shaffeeullah merged commit 3cfaba1 into master Apr 1, 2021

shaffeeullah deleted the shaffeeullah/succeedIfConfigDoesntExist branch April 1, 2021 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: don't fail if ~/.config doesn't exist #1428

fix: don't fail if ~/.config doesn't exist #1428

shaffeeullah commented Mar 23, 2021

codecov bot commented Mar 23, 2021 •

edited

Loading

JustinBeckwith Mar 23, 2021

JustinBeckwith Mar 23, 2021

stephenplusplus Mar 29, 2021

shaffeeullah Mar 29, 2021

shaffeeullah Mar 29, 2021

stephenplusplus Mar 29, 2021

shaffeeullah Mar 30, 2021

stephenplusplus Mar 30, 2021

shaffeeullah Mar 30, 2021

shaffeeullah Mar 30, 2021

stephenplusplus Mar 31, 2021

shaffeeullah Mar 31, 2021

shaffeeullah Mar 31, 2021

stephenplusplus Apr 1, 2021

stephenplusplus commented Apr 1, 2021

fix: don't fail if ~/.config doesn't exist #1428

fix: don't fail if ~/.config doesn't exist #1428

Conversation

shaffeeullah commented Mar 23, 2021

codecov bot commented Mar 23, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenplusplus commented Apr 1, 2021

codecov bot commented Mar 23, 2021 •

edited

Loading