arangodb · KVS85 · Jul 29, 2021 · Jul 19, 2021 · Jul 20, 2021 · Jul 20, 2021
diff --git a/CHANGELOG b/CHANGELOG
@@ -1,6 +1,12 @@
 v3.6.16 (XXXX-XX-XX)
 --------------------
 
+* Make `--javascript.copy-installation` also copy the `node_modules`
+  subdirectory. This is required so we have a full copy of the JavaScript
+  dependencies and not one that excludes some infrequently changed modules.
+  In addition, file copying now intentionally excludes .map files as they are
+  not needed.
+
 * Updated JavaScript dependencies, including breaking changes to non-public
   modules. We recommend always bundling your own copy of third-party modules,
   even ones listed as public.

diff --git a/arangod/V8Server/V8DealerFeature.cpp b/arangod/V8Server/V8DealerFeature.cpp
@@ -247,33 +247,10 @@ void V8DealerFeature::validateOptions(std::shared_ptr<ProgramOptions> options) {
 
   ctx->normalizePath(_moduleDirectories, "javascript.module-directory", false);
 
-  // try to append the current version name to the startup directory,
-  // so instead of "/path/to/js" we will get "/path/to/js/3.4.0"
-  std::string const versionAppendix =
-      std::regex_replace(rest::Version::getServerVersion(), std::regex("-.*$"),
-                         "");
-  std::string versionedPath =
-      basics::FileUtils::buildFilename(_startupDirectory, versionAppendix);
-
-  LOG_TOPIC("604da", DEBUG, Logger::V8)
-      << "checking for existence of version-specific startup-directory '"
-      << versionedPath << "'";
-  if (basics::FileUtils::isDirectory(versionedPath)) {
-    // version-specific js path exists!
-    _startupDirectory = versionedPath;
-  }
-
-  for (auto& it : _moduleDirectories) {
-    versionedPath = basics::FileUtils::buildFilename(it, versionAppendix);
-
-    LOG_TOPIC("8e21a", DEBUG, Logger::V8)
-        << "checking for existence of version-specific module-directory '"
-        << versionedPath << "'";
-    if (basics::FileUtils::isDirectory(versionedPath)) {
-      // version-specific js path exists!
-      it = versionedPath;
+  for (auto const& it : _moduleDirectories) {
+    if (!it.empty()) {
+      v8security.addToInternalWhitelist(it, FSAccessType::READ);
     }
-    v8security.addToInternalWhitelist(it, FSAccessType::READ);
   }
 
   // check whether app-path was specified
@@ -309,26 +286,38 @@ void V8DealerFeature::start() {
     // now check if we have a js directory inside the database directory, and if
     // it looks good
     auto& dbPathFeature = server().getFeature<DatabasePathFeature>();
-    const std::string dbJSPath =
+    std::string const dbJSPath =
         FileUtils::buildFilename(dbPathFeature.directory(), "js");
    const std::string checksumFile =
+    std::string const checksumFile =
         FileUtils::buildFilename(dbJSPath, StaticStrings::checksumFileJs);
-    const std::string serverPath = FileUtils::buildFilename(dbJSPath, "server");
-    const std::string commonPath = FileUtils::buildFilename(dbJSPath, "common");
+    std::string const serverPath = FileUtils::buildFilename(dbJSPath, "server");
+    std::string const commonPath = FileUtils::buildFilename(dbJSPath, "common");
+    std::string const nodeModulesPath = FileUtils::buildFilename(dbJSPath, "node", "node_modules");
     if (FileUtils::isDirectory(dbJSPath) && FileUtils::exists(checksumFile) &&
         FileUtils::isDirectory(serverPath) && FileUtils::isDirectory(commonPath)) {
-      // only load node modules from original startup path
-      _nodeModulesDirectory = _startupDirectory;
       // js directory inside database directory looks good. now use it!
       _startupDirectory = dbJSPath;
+      // older versions didn't copy node_modules. so check if it exists inside the
+      // database directory or not. 
+      if (FileUtils::isDirectory(nodeModulesPath)) {
+        _nodeModulesDirectory = nodeModulesPath;
+      } else {
+        _nodeModulesDirectory = _startupDirectory;
+      }
     }
   }
+
+  V8SecurityFeature& v8security = server().getFeature<V8SecurityFeature>();
+  v8security.addToInternalWhitelist(_startupDirectory, FSAccessType::READ);
+  if (!_nodeModulesDirectory.empty()) {
+    v8security.addToInternalWhitelist(_nodeModulesDirectory, FSAccessType::READ);
+  }
 
   LOG_TOPIC("77c97", DEBUG, Logger::V8)
       << "effective startup-directory: " << _startupDirectory
       << ", effective module-directories: " << _moduleDirectories
       << ", node-modules-directory: " << _nodeModulesDirectory;
-
+  
   _startupLoader.setDirectory(_startupDirectory);
   ServerState::instance()->setJavaScriptPath(_startupDirectory);
 
@@ -458,9 +447,9 @@ void V8DealerFeature::copyInstallationFiles() {
 
   _nodeModulesDirectory = _startupDirectory;
 
-  const std::string checksumFile =
+  std::string const checksumFile =
       FileUtils::buildFilename(_startupDirectory, StaticStrings::checksumFileJs);
-  const std::string copyChecksumFile =
+  std::string const copyChecksumFile =
       FileUtils::buildFilename(copyJSPath, StaticStrings::checksumFileJs);
 
   bool overwriteCopy = false;
@@ -470,7 +459,7 @@ void V8DealerFeature::copyInstallationFiles() {
   } else {
     try {
       overwriteCopy =
-          (FileUtils::slurp(copyChecksumFile) != FileUtils::slurp(checksumFile));
+          (StringUtils::trim(FileUtils::slurp(copyChecksumFile)) != StringUtils::trim(FileUtils::slurp(checksumFile)));
     } catch (basics::Exception const& e) {
       LOG_TOPIC("efa47", ERR, Logger::V8) << "Error reading '" << StaticStrings::checksumFileJs
                                  << "' from disk: " << e.what();
@@ -505,40 +494,46 @@ void V8DealerFeature::copyInstallationFiles() {
       FATAL_ERROR_EXIT();
     }
 
-    // intentionally do not copy js/node/node_modules...
+    // intentionally do not copy js/node/node_modules/estlint!
     // we avoid copying this directory because it contains 5000+ files at the
-    // moment, and copying them one by one is darn slow at least on Windows...
+    // moment, and copying them one by one is slow. In addition, eslint is not
+    // needed in release builds
     std::string const versionAppendix =
         std::regex_replace(rest::Version::getServerVersion(),
                            std::regex("-.*$"), "");
-    std::string const nodeModulesPath =
-        FileUtils::buildFilename("js", "node", "node_modules");
-    std::string const nodeModulesPathVersioned =
-        basics::FileUtils::buildFilename("js", versionAppendix, "node",
-                                         "node_modules");
+    std::string const eslintPath =
+        FileUtils::buildFilename("js", "node", "node_modules", "eslint");
+
+    // .bin directories could be harmful, and .map files are large and unnecessary
+    std::string const binDirectory = std::string(TRI_DIR_SEPARATOR_STR) + ".bin" + TRI_DIR_SEPARATOR_STR;
 
-    std::regex const binRegex("[/\\\\]\\.bin[/\\\\]", std::regex::ECMAScript);
+    size_t copied = 0;
 
-    auto filter = [&nodeModulesPath, &nodeModulesPathVersioned, &binRegex](std::string const& filename) -> bool {
-      if (std::regex_search(filename, binRegex)) {
+    auto filter = [&eslintPath, &binDirectory, &copied](std::string const& filename) -> bool {
+      if (filename.size() >= 4 && filename.compare(filename.size() - 4, 4, ".map") == 0) {
+ 
8000
       // filename ends with ".map". filter it out!
+        return true;
+      } 
+      if (filename.find(binDirectory) != std::string::npos) {
         // don't copy files in .bin
         return true;
       }
+
       std::string normalized = filename;
       FileUtils::normalizePath(normalized);
-      if ((!nodeModulesPath.empty() && 
-           normalized.size() >= nodeModulesPath.size() &&
-           normalized.substr(normalized.size() - nodeModulesPath.size(), nodeModulesPath.size()) == nodeModulesPath) ||
-          (!nodeModulesPathVersioned.empty() &&
-           normalized.size() >= nodeModulesPathVersioned.size() &&
-           normalized.substr(normalized.size() - nodeModulesPathVersioned.size(), nodeModulesPathVersioned.size()) == nodeModulesPathVersioned)) {
+      if ((normalized.size() >= eslintPath.size() &&
+           normalized.compare(normalized.size() - eslintPath.size(), eslintPath.size(), eslintPath) == 0)) {
         // filter it out!
         return true;
       }
+
       // let the file/directory pass through
+      ++copied;
       return false;
     };
 
+    double start = TRI_microtime();
+
     std::string error;
     if (!FileUtils::copyRecursive(_startupDirectory, copyJSPath, filter, error)) {
       LOG_TOPIC("45261", FATAL, Logger::V8) << "Error copying JS installation files to '"
@@ -561,8 +556,14 @@ void V8DealerFeature::copyInstallationFiles() {
             << copyJSPath << "': " << error;
       }
     }
+
+    LOG_TOPIC("38e1e", INFO, Logger::V8) 
+      << "copying " << copied << " JS installation file(s) took " << Logger::FIXED(TRI_microtime() - start, 6) << "s";
   }
+
+  // finally switch over the paths
   _startupDirectory = copyJSPath;
+  _nodeModulesDirectory = basics::FileUtils::buildFilename(copyJSPath, "node", "node_modules");
 }
 
 V8Context* V8DealerFeature::addContext() {

diff --git a/tests/js/client/server_permissions/test-copy-installation.js b/tests/js/client/server_permissions/test-copy-installation.js
@@ -1,5 +1,5 @@
 /*jshint globalstrict:false, strict:false */
-/* global getOptions, assertEqual, arango */
+/* global getOptions, assertEqual, assertTrue, assertFalse, arango */
 
 ////////////////////////////////////////////////////////////////////////////////
 /// @brief test for security-related server options
@@ -34,7 +34,10 @@ if (getOptions === true) {
     'javascript.allow-admin-execute': 'true',
   };
 }
-var jsunity = require('jsunity');
+
+const db = require("@arangodb").db;
+const fs = require("fs");
+const jsunity = require('jsunity');
 
 function testSuite() {
   const errors = require('@arangodb').errors;
@@ -44,12 +47,17 @@ function testSuite() {
     tearDown: function() {},
 
     testCanExecuteAction : function() {
-      // this test just does something, and it doesn't really matter
-      // the key thing in this test is to get the server started using
-      // the `--javascript.copy-installation true` setting without failing
-      let data = "return 'test!'";
-      let result = arango.POST("/_admin/execute", data);
-      assertEqual("test!", result);
+      // fetch server-side database directory name
+      let data = "return require('@arangodb').db._path();";
+      let dbPath = arango.POST("/_admin/execute", data);
+      if (db._engine().name === 'rocksdb') {
+        let jsPath = fs.join(dbPath, "js");
+        assertTrue(fs.exists(jsPath));
+        assertTrue(fs.exists(fs.join(jsPath, "node")));
+        assertTrue(fs.exists(fs.join(jsPath, "node", "node_modules")));
+        assertTrue(fs.exists(fs.join(jsPath, "node", "node_modules", "lodash")));
+        assertFalse(fs.exists(fs.join(jsPath, "node", "eslint")));
+      }
     },
 
   };