From b303e3302c830e713f202cc3546c7e4ce398edc7 Mon Sep 17 00:00:00 2001 From: jsteemann Date: Wed, 18 Mar 2020 13:42:20 +0100 Subject: [PATCH 1/3] fixed issue #11275 --- CHANGELOG | 2 ++ arangod/RocksDBEngine/RocksDBVPackIndex.cpp | 16 +++++++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index f1850734b5df..552c93a8906c 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -53,6 +53,8 @@ devel * Supervision to clean up zombie servers after 24h, if no responsibility for shards. +* Fixed issue #11275: indexes backward compatibility broken in 3.5+. + * Fix SORT RAND() LIMIT 1 optimization for RocksDB when only a projection of the attributes was used. When a projection was used and that projection was covered by an index (e.g. `_key` via the primary index), then the access diff --git a/arangod/RocksDBEngine/RocksDBVPackIndex.cpp b/arangod/RocksDBEngine/RocksDBVPackIndex.cpp index 19007050723f..6f3d39c6ea3a 100644 --- a/arangod/RocksDBEngine/RocksDBVPackIndex.cpp +++ b/arangod/RocksDBEngine/RocksDBVPackIndex.cpp @@ -750,13 +750,16 @@ namespace { bool attributesEqual(VPackSlice first, VPackSlice second, std::vector::const_iterator begin, std::vector::const_iterator end) { + TRI_ASSERT(first.isObject()); + TRI_ASSERT(second.isObject()); + for (; begin != end; ++begin) { // fetch subattribute first = first.get(begin->name); - second = second.get(begin->name); if (first.isExternal()) { first = first.resolveExternal(); } + second = second.get(begin->name); if (second.isExternal()) { second = second.resolveExternal(); } @@ -784,6 +787,17 @@ namespace { if (notF1 || notF2) { // one of the paths was not found break; } + + // check if, after fetching the subattribute, we are point to a non-object. + // e.g. if the index is on field ["a.b"], the first iteration of this loop + // will look for subattribute "a" in the original document. this will always + // work. however, when looking for "b", we have to make sure that "a" was + // an object. otherwise we must not call Slice::get() on it. In case one of + // the subattributes we found so far is not an object, we fall back to the + // regular comparison + if (!first.isObject() || !second.isObject()) { + break; + } } return basics::VelocyPackHelper::equal(first, second, true); From 88d30d0f84f37dce5e440b5802a4b46726e0f322 Mon Sep 17 00:00:00 2001 From: jsteemann Date: Wed, 18 Mar 2020 14:16:29 +0100 Subject: [PATCH 2/3] added test, fixed it --- arangod/RocksDBEngine/RocksDBVPackIndex.cpp | 25 +++++++++------------ tests/js/common/shell/shell-hash-index.js | 22 ++++++++++++++++-- 2 files changed, 31 insertions(+), 16 deletions(-) diff --git a/arangod/RocksDBEngine/RocksDBVPackIndex.cpp b/arangod/RocksDBEngine/RocksDBVPackIndex.cpp index 6f3d39c6ea3a..7f56b076184c 100644 --- a/arangod/RocksDBEngine/RocksDBVPackIndex.cpp +++ b/arangod/RocksDBEngine/RocksDBVPackIndex.cpp @@ -750,10 +750,18 @@ namespace { bool attributesEqual(VPackSlice first, VPackSlice second, std::vector::const_iterator begin, std::vector::const_iterator end) { - TRI_ASSERT(first.isObject()); - TRI_ASSERT(second.isObject()); - for (; begin != end; ++begin) { + // check if, after fetching the subattribute, we are point to a non-object. + // e.g. if the index is on field ["a.b"], the first iteration of this loop + // will look for subattribute "a" in the original document. this will always + // work. however, when looking for "b", we have to make sure that "a" was + // an object. otherwise we must not call Slice::get() on it. In case one of + // the subattributes we found so far is not an object, we fall back to the + // regular comparison + if (!first.isObject() || !second.isObject()) { + break; + } + // fetch subattribute first = first.get(begin->name); if (first.isExternal()) { @@ -787,17 +795,6 @@ namespace { if (notF1 || notF2) { // one of the paths was not found break; } - - // check if, after fetching the subattribute, we are point to a non-object. - // e.g. if the index is on field ["a.b"], the first iteration of this loop - // will look for subattribute "a" in the original document. this will always - // work. however, when looking for "b", we have to make sure that "a" was - // an object. otherwise we must not call Slice::get() on it. In case one of - // the subattributes we found so far is not an object, we fall back to the - // regular comparison - if (!first.isObject() || !second.isObject()) { - break; - } } return basics::VelocyPackHelper::equal(first, second, true); diff --git a/tests/js/common/shell/shell-hash-index.js b/tests/js/common/shell/shell-hash-index.js index 6d93336aa56d..916a78ad4471 100644 --- a/tests/js/common/shell/shell-hash-index.js +++ b/tests/js/common/shell/shell-hash-index.js @@ -1,5 +1,5 @@ /*jshint globalstrict:false, strict:false */ -/*global fail, assertEqual, assertTrue, assertFalse, assertNotEqual */ +/*global fail, assertEqual, assertTrue, assertFalse, assertNull, assertNotEqual */ //////////////////////////////////////////////////////////////////////////////// /// @brief test the hash index @@ -604,7 +604,25 @@ function HashIndexSuite() { } i *= 2; } - } + }, + + testUniqueIndexNullSubattribute : function () { + let idx = collection.ensureIndex({ type: "hash", unique: true, fields: ["a.b"] }); + + assertEqual("hash", idx.type); + assertEqual(true, idx.unique); + assertEqual(["a.b"], idx.fields); + assertEqual(true, idx.isNewlyCreated); + + // as "a" is null here, "a.b" should also be null, at least it should not fail when accessing it via the index + collection.insert({ _key: "test", a : null }); + collection.update("test", { something: "test2" }); + + let doc = collection.document("test"); + assertNull(doc.a); + assertEqual("test2", doc.something); + }, + }; } From d412daf58b254f53f112e5d542be6f2efa20bd7e Mon Sep 17 00:00:00 2001 From: jsteemann Date: Wed, 18 Mar 2020 14:27:15 +0100 Subject: [PATCH 3/3] fix CHANGELOG entry --- CHANGELOG | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 552c93a8906c..12ebb38903b1 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,8 @@ devel ----- +* Fixed issue #11275: indexes backward compatibility broken in 3.5+. + * Changed behavior for creating new collections in OneShard databases (i.e. databases with "sharding" attribute set to "single"): @@ -53,8 +55,6 @@ devel * Supervision to clean up zombie servers after 24h, if no responsibility for shards. -* Fixed issue #11275: indexes backward compatibility broken in 3.5+. - * Fix SORT RAND() LIMIT 1 optimization for RocksDB when only a projection of the attributes was used. When a projection was used and that projection was covered by an index (e.g. `_key` via the primary index), then the access