do not create _modules collection for new databases/installations (#7291)

CHANGELOG

@@ -1,6 +1,15 @@
 devel
 -----
 
+* do not create `_modules` collection for new databases/installations
+
+  `_modules` is only needed for custom modules, and in case a custom
+  module is defined via `defineModule`, the _modules collection will
+  be created lazily automatically.
+
+  Existing modules in existing `_modules` collections will remain
+  functional even after this change
+
 * fixed Foxx queues not retrying jobs with infinite `maxFailures`
 
 * force connection timeout to be 7 seconds to allow libcurl time to retry lost DNS

arangod/VocBase/Methods/AqlUserFunctions.cpp

@@ -49,9 +49,11 @@ using namespace arangodb;
 
 namespace {
 std::string const collectionName("_aqlfunctions");
-  // Must not start with `_`, may contain alphanumerical characters, should have at least one set of double colons followed by more alphanumerical characters.
+
+// Must not start with `_`, may contain alphanumerical characters, should have at least one set of double colons followed by more alphanumerical characters.
 std::regex const funcRegEx("[a-zA-Z0-9][a-zA-Z0-9_]*(::[a-zA-Z0-9_]+)+", std::regex::ECMAScript);
-  // we may filter less restrictive:
+
+// we may filter less restrictive:
 std::regex const funcFilterRegEx("[a-zA-Z0-9_]+(::[a-zA-Z0-9_]*)*", std::regex::ECMAScript);
 
 bool isValidFunctionName(std::string const& testName) {
@@ -66,6 +68,7 @@ void reloadAqlUserFunctions() {
   std::string const def("reloadAql");
   V8DealerFeature::DEALER->addGlobalContextMethod(def);
 }
+
 } // unnamed - namespace
 
 Result arangodb::unregisterUserFunction(
@@ -312,7 +315,7 @@ Result arangodb::registerUserFunction(
     SingleCollectionTransaction trx(ctx, collectionName, AccessMode::Type::WRITE);
 
     res = trx.begin();
-    if (!res.ok()) {
+    if (res.fail()) {
       return res;
     }
 

arangod/VocBase/Methods/Upgrade.cpp

@@ -224,18 +224,6 @@ void methods::Upgrade::registerTasks() {
           /*system*/ Flags::DATABASE_EXCEPT_SYSTEM,
           /*cluster*/ Flags::CLUSTER_NONE | Flags::CLUSTER_COORDINATOR_GLOBAL,
           /*database*/ DATABASE_INIT, &UpgradeTasks::addDefaultUserOther);
-  addTask("updateUserModels",
-          "convert documents in _users collection to new format",
-          /*system*/ Flags::DATABASE_SYSTEM,
-          /*cluster*/ Flags::CLUSTER_NONE | Flags::CLUSTER_COORDINATOR_GLOBAL,
-          /*database*/ DATABASE_UPGRADE,  // DATABASE_EXISTING
-          &UpgradeTasks::updateUserModels);
-  addTask("createModules", "setup _modules collection",
-          /*system*/ Flags::DATABASE_ALL,
-          /*cluster*/ Flags::CLUSTER_NONE | Flags::CLUSTER_COORDINATOR_GLOBAL,
-          /*database*/ DATABASE_INIT | DATABASE_UPGRADE | DATABASE_EXISTING,
-          &UpgradeTasks::createModules);
-  // FIXME simon: Determine whether this is still necessary
   addTask("setupAnalyzers", "setup _iresearch_analyzers collection",
           /*system*/ Flags::DATABASE_SYSTEM,
           /*cluster*/ Flags::CLUSTER_NONE | Flags::CLUSTER_COORDINATOR_GLOBAL,

arangod/VocBase/Methods/UpgradeTasks.cpp

@@ -17,7 +17,7 @@
 ///
 /// Copyright holder is ArangoDB GmbH, Cologne, Germany
 ///
-/// @author Simon Gräter
+/// @author Simon Grätzer
 ////////////////////////////////////////////////////////////////////////////////
 
 #include "UpgradeTasks.h"
@@ -291,22 +291,6 @@ bool UpgradeTasks::addDefaultUserOther(
   return true;
 }
 
-bool UpgradeTasks::updateUserModels(
-    TRI_vocbase_t& vocbase,
-    arangodb::velocypack::Slice const& slice
-) {
-  TRI_ASSERT(vocbase.isSystem());
-  // TODO isn't this done on the fly ?
-  return true;
-}
-
-bool UpgradeTasks::createModules(
-    TRI_vocbase_t& vocbase,
-    arangodb::velocypack::Slice const& slice
-) {
-  return ::createSystemCollection(&vocbase, "_modules");
-}
-
 bool UpgradeTasks::setupAnalyzers(
     TRI_vocbase_t& vocbase,
     arangodb::velocypack::Slice const& slice

arangod/VocBase/Methods/UpgradeTasks.h

@@ -38,8 +38,6 @@ struct UpgradeTasks {
   static bool setupUsers(TRI_vocbase_t& vocbase, velocypack::Slice const& slice);
   static bool createUsersIndex(TRI_vocbase_t& vocbase, velocypack::Slice const& slice);
   static bool addDefaultUserOther(TRI_vocbase_t& vocbase, velocypack::Slice const& slice);
-  static bool updateUserModels(TRI_vocbase_t& vocbase, velocypack::Slice const& slice);
-  static bool createModules(TRI_vocbase_t& vocbase, velocypack::Slice const& slice);
   static bool setupAnalyzers(TRI_vocbase_t& vocbase, velocypack::Slice const& slice);
   static bool createRouting(TRI_vocbase_t& vocbase, velocypack::Slice const& slice);
   static bool insertRedirections(TRI_vocbase_t& vocbase, velocypack::Slice const& slice);

js/common/bootstrap/modules.js

@@ -389,7 +389,8 @@
     try {
       var dbModule = Module._dbCache[request];
 
-      if (!dbModule && internal.db !== undefined && internal.db._modules !== undefined) {
+      // _modules is an optional collection. only use it if it is present.
+      if (!dbModule && internal.db !== undefined && internal.db._modules) {
         dbModule = internal.db._modules.firstExample({path: request});
 
         if (!dbModule) {

js/common/modules/@arangodb/common.js

@@ -152,6 +152,7 @@ exports.defineModule = function(path, file) {
   let mc = internal.db._collection('_modules');
 
   if (mc === null) {
+    // lazily create _modules collection if it does not yet exist
     const DEFAULT_REPLICATION_FACTOR_SYSTEM = internal.DEFAULT_REPLICATION_FACTOR_SYSTEM;
     mc = internal.db._create('_modules', {
       isSystem: true,

js/server/bootstrap/modules/internal.js

@@ -177,37 +177,42 @@
     console.debug('autoloading actions');
 
     try {
-      var modules = exports.db._collection('_modules');
+      let modules = exports.db._collection('_modules');
 
-      if (modules !== null) {
-        modules = modules.byExample({ autoload: true }).toArray();
-        
-        modules.forEach(function (module) {
-          // this module is only meant to be executed in one thread
-          if (exports.threadNumber !== 0 && !module.perThread) {
-            return;
-          }
-
-          console.debug('autoloading module: %s', module.path);
-
-          try {
-            // require a module
-            if (module.path !== undefined) {
-              require(module.path);
-            }
-
-            // execute a user function
-            else if (module.func !== undefined) {
-              /*eslint-disable */
-              var func = new Function(module.func)
-              /*eslint-enable */
-              func();
-            }
-          } catch (err) {
-            console.error('error while loading startup module "%s": %s', module.name || module.path, String(err));
-          }
-        });
+      if (modules === null) {
+        // _modules is an optional collection. if it does not exist,
+        // we can simply go on and ignore it
+        console.debug('autoloading actions finished, no _modules collection found');
+        return;
       }
+
+      modules = modules.byExample({ autoload: true }).toArray();
+        
+      modules.forEach(function (module) {
+        // this module is only meant to be executed in one thread
+        if (exports.threadNumber !== 0 && !module.perThread) {
+          return;
+        }
+
+        console.debug('autoloading module: %s', module.path);
+
+        try {
+          // require a module
+          if (module.path !== undefined) {
+            require(module.path);
+          }
+
+          // execute a user function
+          else if (module.func !== undefined) {
+            /*eslint-disable */
+            var func = new Function(module.func)
+            /*eslint-enable */
+            func();
+          }
+        } catch (err) {
+          console.error('error while loading startup module "%s": %s', module.name || module.path, String(err));
+        }
+      });
     } catch (err) {
       console.error('error while loading startup modules: %s', String(err));
     }

tests/rb/HttpInterface/api-http-spec.rb

@@ -12,6 +12,9 @@ describe ArangoDB do
 
   context "binary data" do
     before do
+      # make sure system collections exist
+      ArangoDB.post("/_admin/execute", :body => "var db = require('internal').db; try { db._create('_modules', { isSystem: true, distributeShardsLike: '_graphs' }); } catch (err) {} try { db._create('_routing', { isSystem: true, distributeShardsLike: '_graphs' }); } catch (err) {}")   
+       
       # clean up first
       ArangoDB.delete("/_api/document/_modules/UnitTestRoutingTest")
       ArangoDB.delete("/_api/document/_routing/UnitTestRoutingTest")
@@ -32,6 +35,9 @@ describe ArangoDB do
     after do
       ArangoDB.delete("/_api/document/_modules/UnitTestRoutingTest")
       ArangoDB.delete("/_api/document/_routing/UnitTestRoutingTest")
+      
+      # drop collections
+      ArangoDB.post("/_admin/execute", :body => "var db = require('internal').db; try { db._drop('_modules', true); } catch (err) {} try { db._drop('_routing', true); } catch (err) {}")   
     end
 
     it "checks handling of a request with binary data" do