Test early if the given collections do exist. If not, don't create th…

…e view.
arangodb · jsteemann · Sep 17, 2018 · Sep 14, 2018 · Sep 14, 2018 · Sep 14, 2018
commit f7064043341439b1d5c4ee641038cd6ad4194f5f
diff --git a/arangod/IResearch/IResearchViewCoordinator.cpp b/arangod/IResearch/IResearchViewCoordinator.cpp
@@ -133,7 +133,7 @@ bool IResearchViewCoordinator::emplace(
   auto& properties = info.isObject() ? info : emptyObjectSlice(); // if no 'info' then assume defaults
   std::string error;
 
-  bool hasLinks = properties.hasKey("links");
+  bool hasLinks = properties.hasKey(StaticStrings::LinksField);
 
   auto view = std::shared_ptr<IResearchViewCoordinator>(
     new IResearchViewCoordinator(vocbase, info, planVersion)

diff --git a/arangod/IResearch/IResearchViewSingleServer.cpp b/arangod/IResearch/IResearchViewSingleServer.cpp
@@ -57,27 +57,34 @@ namespace iresearch {
   auto& properties = info.isObject() ? info : emptyObjectSlice(); // if no 'info' then assume defaults
   std::string error;
 
-  bool hasLinks = properties.hasKey("links");
+  bool hasLinks = properties.hasKey(StaticStrings::LinksField);
 
   if (hasLinks && isNew) {
 
-    // check link auth as per https://github.com/arangodb/backlog/issues/459
-    if (arangodb::ExecContext::CURRENT) {
+    arangodb::velocypack::ObjectIterator iterator{info.get(StaticStrings::LinksField)};
 
-      // check new links
-      if (info.hasKey(StaticStrings::LinksField)) {
-        for (arangodb::velocypack::ObjectIterator itr(info.get(StaticStrings::LinksField)); itr.valid(); ++itr) {
-          if (!itr.key().isString()) {
-            continue; // not a resolvable collection (invalid jSON)
-          }
+    for (auto itr : iterator) {
+      if (!itr.key.isString()) {
+        continue; // not a resolvable collection (invalid jSON)
+      }
+
+      auto colname = itr.key.copyString();
 
-          auto collection= vocbase.lookupCollection(itr.key().copyString());
+      // check if the collection exists
+      auto collection = vocbase.lookupCollection(colname);
+      if (!collection) {
+
+        LOG_TOPIC(WARN, arangodb::iresearch::TOPIC) << "Could not create view: "
+          << "Collection not found: " << colname;
+        continue ;
+        /*TRI_set_errno(TRI_ERROR_ARANGO_DATA_SOURCE_NOT_FOUND);
+        return nullptr;*/
+      }
 
-          if (collection
-              && !arangodb::ExecContext::CURRENT->canUseCollection(vocbase.name(), collection->name(), arangodb::auth::Level::RO)) {
-            return nullptr;
-          }
-        }
+      // check if the collection can be used
+      if (arangodb::ExecContext::CURRENT &&
+        !arangodb::ExecContext::CURRENT->canUseCollection(vocbase.name(), collection->name(), arangodb::auth::Level::RO)) {
+        return nullptr;
       }
     }
   }

diff --git a/tests/js/common/shell/shell-view-arangosearch-link-noncluster.js b/tests/js/common/shell/shell-view-arangosearch-link-noncluster.js
@@ -74,12 +74,29 @@ function IResearchLinkSuite () {
     ////////////////////////////////////////////////////////////////////////////
     testHandlingCreateWithLinks : function () {
       var meta = { links: { 'testCollection' : { includeAllFields: true } } };
-      var view = db._createView("badView", "arangosearch", meta);
+      var view = db._createView("someView", "arangosearch", meta);
       var links = view.properties().links;
       assertNotEqual(links['testCollection'], undefined);
       view.drop();
     },
 
+    ////////////////////////////////////////////////////////////////////////////
+    /// @brief don't create view when collections do not exist or the user
+    ///   is not allowed to access them.
+    ////////////////////////////////////////////////////////////////////////////
+    testHandlingCreateWithBadLinks : function () {
+      var meta = { links: { 'nonExistentCollection': {}, 'testCollection' : { includeAllFields: true } } };
+      var view;
+      try {
+        view = db._createView("someView", "arangosearch", meta);
+        fail();
+      } catch(e) {
+         assertEqual(ERRORS.ERROR_ARANGO_DATA_SOURCE_NOT_FOUND.code, e.errorNum);
+      }
+
+      assertNull(db._view("someView"));
+    },
+
     ////////////////////////////////////////////////////////////////////////////
     /// @brief create a view and add/drop link
     ////////////////////////////////////////////////////////////////////////////

diff --git a/tests/js/server/replication/replication-ongoing.js b/tests/js/server/replication/replication-ongoing.js
@@ -140,7 +140,7 @@ const compare = function(masterFunc, masterFunc2, slaveFuncOngoing, slaveFuncFin
       console.log("slave has caught up. state.lastLogTick:", state.lastLogTick, "slaveState.lastAppliedContinuousTick:", slaveState.state.lastAppliedContinuousTick, "slaveState.lastProcessedContinuousTick:", slaveState.state.lastProcessedContinuousTick);
       break;
     }
-      
+
     if (!printed) {
       console.log("waiting for slave to catch up");
       printed = true;
@@ -160,7 +160,7 @@ function BaseTestConfig() {
   'use strict';
 
   return {
-    
+
     ////////////////////////////////////////////////////////////////////////////////
     /// @brief test duplicate _key issue and replacement
     ////////////////////////////////////////////////////////////////////////////////
@@ -192,7 +192,7 @@ function BaseTestConfig() {
         }
       );
     },
-    
+
     testSecondaryKeyConflict: function() {
       connectToMaster();
 
@@ -223,7 +223,7 @@ function BaseTestConfig() {
         }
       );
     },
-   
+
     /////
8000
///////////////////////////////////////////////////////////////////////////
     /// @brief test collection creation
     ////////////////////////////////////////////////////////////////////////////////
@@ -705,7 +705,7 @@ function BaseTestConfig() {
             db._create(cn);
             let view = db._createView("UnitTestsSyncView", "arangosearch", {});
             let links = {};
-            links[cn] =  { 
+            links[cn] =  {
               includeAllFields: true,
               fields: {
                 text: { analyzers: [ "text_en" ] }
@@ -720,7 +720,43 @@ function BaseTestConfig() {
           if (!state.arangoSearchEnabled) {
             return;
           }
-
+
+          let view = db._view("UnitTestsSyncView");
+          assertTrue(view !== null);
+          let props = view.properties();
+          assertEqual(Object.keys(props.links).length, 1);
+          assertTrue(props.hasOwnProperty("links"));
+          assertTrue(props.links.hasOwnProperty(cn));
+        },
+        {}
+      );
+    },
+
+    testViewCreateWithLinks: function() {
+      connectToMaster();
+
+      compare(
+        function() {},
+        function(state) {
+          try {
+            db._create(cn);
+            let links = {};
+            links[cn] =  {
+              includeAllFields: true,
+              fields: {
+                text: { analyzers: [ "text_en" ] }
+              }
+            };
+            let view = db._createView("UnitTestsSyncView", "arangosearch", {"links": links});
+            state.arangoSearchEnabled = true;
+          } catch (err) { }
+        },
+        function() {},
+        function(state) {
+          if (!state.arangoSearchEnabled) {
+            return;
+          }
+
           let view = db._view("UnitTestsSyncView");
           assertTrue(view !== null);
           let props = view.properties();
@@ -741,7 +777,7 @@ function BaseTestConfig() {
             db._create(cn);
             let view = db._createView("UnitTestsSyncView", "arangosearch", {});
             let links = {};
-            links[cn] =  { 
+            links[cn] =  {
               includeAllFields: true,
               fields: {
                 text: { analyzers: [ "text_en" ] }
@@ -770,7 +806,7 @@ function BaseTestConfig() {
           if (!state.arangoSearchEnabled) {
             return;
           }
-    
+
           let view = db._view("UnitTestsSyncViewRenamed");
           assertTrue(view !== null);
           let props = view.properties();
@@ -805,7 +841,7 @@ function BaseTestConfig() {
           if (!state.arangoSearchEnabled) {
             return;
           }
-    
+
           let view = db._view("UnitTestsSyncView");
           assertTrue(view === null);
         },
@@ -903,7 +939,7 @@ function ReplicationOtherDBSuite() {
       db._dropDatabase(dbName);
     } catch (e) {
     }
-    
+
     db._createDatabase(dbName);
 
     connectToMaster();
@@ -931,7 +967,7 @@ function ReplicationOtherDBSuite() {
       replication.applier.forget();
     } catch (e) {
     }
-    
+
     db._useDatabase("_system");
     try {
       db._dropDatabase(dbName);
@@ -1051,17 +1087,17 @@ function ReplicationOtherDBSuite() {
     // Collection should be empty
     assertEqual(0, collectionCount(cn));
 
-    // now test if the replication is actually 
+    // now test if the replication is actually
     // switched off
- 
+
     // Section - Master
     connectToMaster();
     // Insert some documents
     db._collection(cn).save(docs);
     // Flush wal to trigger replication
     internal.wal.flush(true, true);
 
-    const lastLogTick = replication.logger.state().state.lastLogTick;    
+    const lastLogTick = replication.logger.state().state.lastLogTick;
 
     // Section - Slave
     connectToSlave();
@@ -1130,7 +1166,7 @@ function ReplicationOtherDBSuite() {
     }
 
     // Now test if the Slave did replicate the new database directly...
-    assertEqual(50, collectionCount(cn), 
+    assertEqual(50, collectionCount(cn),
     "The slave inserted the new collection data into the old one, it skipped the drop.");
   };