fix non-deterministic occurrences of "document not found" errors (#9873)

CHANGELOG

@@ -1,6 +1,9 @@
 v3.3.25 (2019-XX-XX)
 --------------------
 
+* Fixed non-deterministic occurrences of "document not found" errors in sharded 
+  collections with custom shard keys (i.e. non-`_key`) and multi-document lookups.
+
 * Fixed cut'n'pasting code from the documentation into arangosh.
 
 * Fixed bug in MoveShard::abort which causes a duplicate entry in the follower list. (Internal Bug #4378)

arangod/Cluster/ClusterMethods.cpp

@@ -262,6 +262,16 @@ static void mergeResults(std::vector<std::pair<ShardID, VPackValueLength>> const
 ///        well
 ////////////////////////////////////////////////////////////////////////////////
 
+namespace {
+// velocypack representation of object
+// {"error":true,"errorMessage":"document not found","errorNum":1202}
+static const char* notFoundSlice = 
+  "\x14\x36\x45\x65\x72\x72\x6f\x72\x1a\x4c\x65\x72\x72\x6f\x72\x4d"
+  "\x65\x73\x73\x61\x67\x65\x52\x64\x6f\x63\x75\x6d\x65\x6e\x74\x20"
+  "\x6e\x6f\x74\x20\x66\x6f\x75\x6e\x64\x48\x65\x72\x72\x6f\x72\x4e"
+  "\x75\x6d\x29\xb2\x04\x03";
+} // namespace
+
 static void mergeResultsAllShards(std::vector<std::shared_ptr<VPackBuilder>> const& results,
                                   std::shared_ptr<VPackBuilder>& resultBody,
                                   std::unordered_map<int, size_t>& errorCounter,
@@ -270,12 +280,7 @@ static void mergeResultsAllShards(std::vector<std::shared_ptr<VPackBuilder>> con
   TRI_ASSERT(errorCounter.find(TRI_ERROR_ARANGO_DOCUMENT_NOT_FOUND) ==
              errorCounter.end());
   size_t realNotFound = 0;
-  VPackBuilder cmp;
-  cmp.openObject();
-  cmp.add(StaticStrings::Error, VPackValue(true));
-  cmp.add(StaticStrings::ErrorNum, VPackValue(TRI_ERROR_ARANGO_DOCUMENT_NOT_FOUND));
-  cmp.close();
-  VPackSlice notFound = cmp.slice();
+  
   resultBody->clear();
   resultBody->openArray();
   for (VPackValueLength currentIndex = 0; currentIndex < expectedResults; ++currentIndex) {
@@ -284,7 +289,14 @@ static void mergeResultsAllShards(std::vector<std::shared_ptr<VPackBuilder>> con
       VPackSlice oneRes = it->slice();
       TRI_ASSERT(oneRes.isArray());
       oneRes = oneRes.at(currentIndex);
-      if (basics::VelocyPackHelper::compare(oneRes, notFound, false) != 0) {
+      
+      int errorNum = TRI_ERROR_NO_ERROR;
+      VPackSlice errorNumSlice = oneRes.get(StaticStrings::ErrorNum);
+      if (errorNumSlice.isNumber()) {
+        errorNum = errorNumSlice.getNumber<int>();
+      }
+      if ((errorNum != TRI_ERROR_NO_ERROR && errorNum != TRI_ERROR_ARANGO_DOCUMENT_NOT_FOUND) ||
+          oneRes.hasKey(StaticStrings::KeyString)) {
         // This is the correct result
         // Use it
         resultBody->add(oneRes);
@@ -294,7 +306,7 @@ static void mergeResultsAllShards(std::vector<std::shared_ptr<VPackBuilder>> con
     }
     if (!foundRes) {
       // Found none, use NOT_FOUND
-      resultBody->add(notFound);
+      resultBody->add(VPackSlice(reinterpret_cast<uint8_t const*>(notFoundSlice)));
       realNotFound++;
     }
   }

js/common/tests/shell/shell-babies.js

@@ -0,0 +1,232 @@
+/*jshint globalstrict:false, strict:false, maxlen: 5000 */
+/*global fail, assertTrue, assertFalse, assertEqual, assertNotEqual, assertTypeOf */
+
+////////////////////////////////////////////////////////////////////////////////
+/// @brief test the babies interface
+///
+/// DISCLAIMER
+///
+/// Copyright 2010-2012 triagens GmbH, Cologne, Germany
+///
+/// Licensed under the Apache License, Version 2.0 (the "License");
+/// you may not use this file except in compliance with the License.
+/// You may obtain a copy of the License at
+///
+///     http://www.apache.org/licenses/LICENSE-2.0
+///
+/// Unless required by applicable law or agreed to in writing, software
+/// distributed under the License is distributed on an "AS IS" BASIS,
+/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+/// See the License for the specific language governing permissions and
+/// limitations under the License.
+///
+/// Copyright holder is triAGENS GmbH, Cologne, Germany
+///
+/// @author Jan Steemann
+/// @author Copyright 2012, triAGENS GmbH, Cologne, Germany
+////////////////////////////////////////////////////////////////////////////////
+
+const jsunity = require("jsunity");
+const arangodb = require("@arangodb");
+const ERRORS = arangodb.errors;
+const db = arangodb.db;
+
+function BabiesStandardShardingSuite () {
+  'use strict';
+
+  const n = 50;
+  const cn = "UnitTestsCollectionBabies";
+  let c;
+
+  let keys = [];
+  for (let i = 0; i < n; ++i) {
+    keys.push("test" + i);
+  }
+
+  return {
+
+    setUp : function () {
+      db._drop(cn);
+      c = db._create(cn, { numberOfShards: 7 });
+      for (let i = 0; i < n; ++i) {
+        c.insert({ _key: "test" + i, value:  i });
+      }
+    },
+
+    tearDown : function () {
+      db._drop(cn);
+    },
+
+    testGetBabiesStandard : function () {
+      let docs = c.document(keys);
+      for (let i = 0; i < n; ++i) {
+        assertFalse(docs[i].hasOwnProperty("error"));
+        assertEqual(i, docs[i].value);
+        assertEqual("test" + i, docs[i]._key);
+      }
+    },
+    
+    testGetNonExistingBabiesStandard : function () {
+      let keys = [];
+      for (let i = 0; i < n; ++i) {
+        keys.push("peng" + i);
+      }
+      let docs = c.document(keys);
+      for (let i = 0; i < n; ++i) {
+        assertTrue(docs[i].error);
+        assertFalse(docs[i].hasOwnProperty("_key"));
+        assertEqual(ERRORS.ERROR_ARANGO_DOCUMENT_NOT_FOUND.code, docs[i].errorNum);
+      }
+    },
+    
+    testInsertBabiesStandard : function () {
+      c.truncate();
+      let docs = [];
+      for (let i = 0; i < n; ++i) {
+        docs.push({ _key: "test" + i, value: i });
+      }
+      docs = c.insert(docs);
+      for (let i = 0; i < n; ++i) {
+        assertFalse(docs[i].hasOwnProperty("error"));
+        assertEqual("test" + i, docs[i]._key);
+      }
+    },
+    
+    testRemoveBabiesStandard : function () {
+      let docs = c.remove(keys);
+      for (let i = 0; i < n; ++i) {
+        assertFalse(docs[i].hasOwnProperty("error"));
+        assertEqual("test" + i, docs[i]._key);
+      }
+    },
+    
+    testUpdateBabiesStandard : function () {
+      let docs = [];
+      for (let i = 0; i < n; ++i) {
+        docs.push({ value: i * 2 });
+      }
+      docs = c.update(keys, docs);
+      for (let i = 0; i < n; ++i) {
+        assertFalse(docs[i].hasOwnProperty("error"));
+        assertTrue(docs[i].hasOwnProperty("_oldRev"));
+        assertEqual("test" + i, docs[i]._key);
+      }
+    },
+    
+    testReplaceBabiesStandard : function () {
+      let docs = [];
+      for (let i = 0; i < n; ++i) {
+        docs.push({ value: i * 2 });
+      }
+      docs = c.replace(keys, docs);
+      for (let i = 0; i < n; ++i) {
+        assertFalse(docs[i].hasOwnProperty("error"));
+        assertTrue(docs[i].hasOwnProperty("_oldRev"));
+        assertEqual("test" + i, docs[i]._key);
+      }
+    },
+    
+
+  };
+}
+
+function BabiesCustomShardingSuite () {
+  'use strict';
+
+  const n = 50;
+  const cn = "UnitTestsCollectionBabies";
+  let c;
+  let keys;
+
+  return {
+
+    setUp : function () {
+      db._drop(cn);
+      c = db._create(cn, { numberOfShards: 7, shardKeys: ["value"] });
+      keys = [];
+      for (let i = 0; i < n; ++i) {
+        keys.push(c.insert({ value:  i })._key);
+      }
+    },
+
+    tearDown : function () {
+      db._drop(cn);
+    },
+
+    testGetBabiesCustom : function () {
+      let docs = c.document(keys);
+      for (let i = 0; i < n; ++i) {
+        assertFalse(docs[i].hasOwnProperty("error"));
+        assertEqual(i, docs[i].value);
+        assertEqual(keys[i], docs[i]._key);
+      }
+    },
+    
+    testGetNonExistingBabiesCustom : function () {
+      let keys = [];
+      for (let i = 0; i < n; ++i) {
+        keys.push("peng" + i);
+      }
+      let docs = c.document(keys);
+      for (let i = 0; i < n; ++i) {
+        assertTrue(docs[i].error);
+        assertFalse(docs[i].hasOwnProperty("_key"));
+        assertEqual(ERRORS.ERROR_ARANGO_DOCUMENT_NOT_FOUND.code, docs[i].errorNum);
+      }
+    },
+    
+    testInsertBabiesCustom : function () {
+      c.truncate();
+      let docs = [];
+      for (let i = 0; i < n; ++i) {
+        docs.push({ value: i });
+      }
+      docs = c.insert(docs);
+      for (let i = 0; i < n; ++i) {
+        assertFalse(docs[i].hasOwnProperty("error"));
+        assertTrue(docs[i].hasOwnProperty("_key"));
+      }
+    },
+    
+    testRemoveBabiesCustom : function () {
+      let docs = c.remove(keys);
+      for (let i = 0; i < n; ++i) {
+        assertFalse(docs[i].hasOwnProperty("error"));
+        assertEqual(keys[i], docs[i]._key);
+      }
+    },
+    
+    testUpdateBabiesCustom : function () {
+      let docs = [];
+      for (let i = 0; i < n; ++i) {
+        docs.push({ value: i, value2: i * 2 });
+      }
+      docs = c.update(keys, docs);
+      for (let i = 0; i < n; ++i) {
+        assertFalse(docs[i].hasOwnProperty("error"));
+        assertTrue(docs[i].hasOwnProperty("_oldRev"));
+        assertEqual(keys[i], docs[i]._key);
+      }
+    },
+    
+    testReplaceBabiesCustom : function () {
+      let docs = [];
+      for (let i = 0; i < n; ++i) {
+        docs.push({ value: i, value2: i * 2 });
+      }
+      docs = c.replace(keys, docs);
+      for (let i = 0; i < n; ++i) {
+        assertFalse(docs[i].hasOwnProperty("error"));
+        assertTrue(docs[i].hasOwnProperty("_oldRev"));
+        assertEqual(keys[i], docs[i]._key);
+      }
+    },
+    
+
+  };
+}
+
+jsunity.run(BabiesStandardShardingSuite);
+jsunity.run(BabiesCustomShardingSuite);
+
+return jsunity.done();