0
\$\begingroup\$

I have an app that uses the file system a lot to store generated files (zip files and scss/css fil). When a user generates a file, the metadata and other information is stored in the db for later retrieval and action in another part of the app. Through testing or error, I realized the DB and file system could become out of sync, so I created a function to walk through the db and match each record to a directory, and then the other way around (walk through the file system and match it to the db.

I use both fs-plus (fs) and fs-extra (fse) to achieve the recursive search and removal of directories.

The code seems fragile and confusing, any feedback would be appreciated.

walkStyles: function() {
  // First let's lookup styles and make sure there is a directory for them.  If not, delete it from the db.
  Style.find({system: false}, function(err, result) {
    if (err) {
      console.log(err);
      return null;
    }
    if (!result) {
      console.log('No styles found in db');
      return null;
    }
    _.forEach(result, function(style)  {
    // make sure I can access the style fil.
      fs.access('./content/userstyles/' + style._id + '/style.css', function(err) {
        if (err && err.code === 'ENOENT') {
          //Can't find the style file, i need to delete it
          Style.remove({_id: style._id}, function(err, result) {
            if (err) {
              console.log('Error removing style from the db');
            }
              if (result) {
                console.log('Out of sync style pruned');
              }
            });
          }
        });
        console.log('Matched style from DB to disk.   No action.');
    });
  });
  // let's walk the other way and make sure all styles found on disk have a match in the db at the folder level.
  fsp.traverseTree('./content/userstyles',  // walk through the content directory
     function(file) {
    console.log('File: ' + file);
    },
  function(dir) {
    let _id = dir.replace('content\\userstyles\\', '');  // on dir, trim to just the id.  this can be matched to the DB _id field.
    console.log(_id);
    Style.find({_id: _id}, function(err, result) {
      if (err) {
        console.log(err);
      }
      if (result) {  // found the style
        console.log('Style synced from disk to db.  No action.');
      }
      if (!result) {  // didn't find the style.
        console.log('Style found on disk, but not in the db.   Removing from disk.');
        fs.remove('./content/userstyles/' + _id, function(err) {
          if (err) {
            console.log(err);
          }
        });
      }
    });
  },
    function() {
  console.log('done checking styles.');
});
}
\$\endgroup\$
1
  • \$\begingroup\$ You could look into cleaning this up with promises. \$\endgroup\$
    – elclanrs
    Jun 12 '16 at 10:12
1
\$\begingroup\$
./content/userstyles/' + style._id + '/style.css

This appears to be a very brittle convention. It assumes the styles live in a specific directory. Not following the convention, it might lead to more unsynced files. You need a more robust way to handle this. For instance, ID'ing the file using its path.

fs.access('./content/userstyles/' + style._id + '/style.css', function(err) {
    if (err && err.code === 'ENOENT') {
      //Can't find the style file, i need to delete it
      Style.remove({_id: style._id}, function(err, result) {

Instead of a back-to-back filesystem and DB operation, why not operate in batches. Like first get a list of files from DB. Next, check if you can access them all. Only after then do you wipe the DB with missing ones. This way, you have a much more linear set of operations instead of one step doing too many at a time.

It can even be simplified into something like similar to this pseudo-code implementation:

db.getStyles()                                      // Get all styles
  .map(path => { path, hasAccess: hasAccess(path)}) // Get access
  .filter(file => !file.hasAccess)                  // Get unaccessible
  .forEach(path => db.delete(path));                // Delete

Now if I rememer correctly, fs-plus has a method to list down all the possible paths under a given path. I think it was listTreeSync. This way, instead of having a callback run on every file and directory, you can simply analyze the array of paths returned by the function. Equates to lesser filesystem operations.

fsp.listTreeSync(dir)
   .map(path => extractId(path))
   .forEach(id=> storeOrDeleteId(id));

Also, I recommend writing it in a synchronous manner. Unless you're multiplexing other operations in the same app together with this, you'll see no benefit from writing this in an asynchronous manner if this is the only operation that runs at a given time.

\$\endgroup\$
1
  • \$\begingroup\$ First off, amazing use the map/filter. I never thought of anything close to that. I also feel like a ding dong because I just realized listTreeSync (etc) is SYNCHRONOUS instead of async.... I had it the wrong way around since I expected synchronous to be default. \$\endgroup\$ Jun 13 '16 at 3:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.