diff --git a/CHANGELOG b/CHANGELOG index ea73a8a596b1..c3adbe9050f8 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,10 @@ devel ----- +* Fix unique _key generation in two cases: AQL INSERT operations in a loop + without user specified _key in collections with exactly one shard or with + multiple shard, non-standard sharding and `forceOneShardAttributeValue`. + * Fix BTS-1732 and BTS-2177: Possible crashes during shutdown, and maintainer mode (i.e. non-production) assertions during tests. diff --git a/arangod/Cluster/ClusterInfo.cpp b/arangod/Cluster/ClusterInfo.cpp index b6c37d45e69f..cdb9155646d7 100644 --- a/arangod/Cluster/ClusterInfo.cpp +++ b/arangod/Cluster/ClusterInfo.cpp @@ -620,6 +620,11 @@ uint64_t ClusterInfo::uniqid(uint64_t count) { return idCounter.fetch_add(1); } + TRI_IF_FAILURE("always-fetch-new-cluster-wide-uniqid") { + uint64_t result = _agency.uniqid(count, 0.0); + return result; + } + std::lock_guard mutexLocker{_idLock}; if (_uniqid._currentValue + count - 1 <= _uniqid._upperValue) { @@ -661,6 +666,21 @@ uint64_t ClusterInfo::uniqid(uint64_t count) { return result; } +////////////////////////////////////////////////////////////////////////////// +/// @brief get a number of cluster-wide unique IDs, returns the first +/// one and guarantees that are reserved for the caller. +/// This variant uses _agency to directly get things from the agency. +////////////////////////////////////////////////////////////////////////////// + +std::optional ClusterInfo::uniqidFromAgency(uint64_t number) { + try { + uint64_t result = _agency.uniqid(number, 0.0); + return {result}; + } catch (...) { + return {}; + } +} + //////////////////////////////////////////////////////////////////////////////// /// @brief flush the caches (used for testing) //////////////////////////////////////////////////////////////////////////////// diff --git a/arangod/Cluster/ClusterInfo.h b/arangod/Cluster/ClusterInfo.h index 40c3fb16441f..ca6c61d929df 100644 --- a/arangod/Cluster/ClusterInfo.h +++ b/arangod/Cluster/ClusterInfo.h @@ -316,6 +316,15 @@ class ClusterInfo final { uint64_t uniqid(uint64_t = 1); + ////////////////////////////////////////////////////////////////////////////// + /// @brief get a number of cluster-wide unique IDs, returns the first + /// one and guarantees that are reserved for the caller. + /// This variant uses _agency to directly get things from the agency. + /// If the optional value is empty, an error occurred. + ////////////////////////////////////////////////////////////////////////////// + + std::optional uniqidFromAgency(uint64_t number); + /** * @brief Agency dump including replicated log and compaction * @param body Builder to fill with dump diff --git a/arangod/RestHandler/RestAdminClusterHandler.cpp b/arangod/RestHandler/RestAdminClusterHandler.cpp index cf0561d8c55e..13d29589c8f4 100644 --- a/arangod/RestHandler/RestAdminClusterHandler.cpp +++ b/arangod/RestHandler/RestAdminClusterHandler.cpp @@ -30,6 +30,7 @@ #include #include +#include "Agency/AgencyComm.h" #include "Agency/AgencyPaths.h" #include "Agency/AsyncAgencyComm.h" #include "Agency/Supervision.h" @@ -329,6 +330,7 @@ RestAdminClusterHandler::RestAdminClusterHandler(ArangodServer& server, std::string const RestAdminClusterHandler::Health = "health"; std::string const RestAdminClusterHandler::NumberOfServers = "numberOfServers"; +std::string const RestAdminClusterHandler::UniqId = "uniqId"; std::string const RestAdminClusterHandler::Maintenance = "maintenance"; std::string const RestAdminClusterHandler::NodeVersion = "nodeVersion"; std::string const RestAdminClusterHandler::NodeEngine = "nodeEngine"; @@ -399,6 +401,9 @@ auto RestAdminClusterHandler::executeAsync() -> futures::Future { } else if (command == NumberOfServers) { co_await handleNumberOfServers(); co_return; + } else if (command == UniqId) { + co_await handleUniqId(); + co_return; } else if (command == Maintenance) { co_await handleMaintenance(); co_return; @@ -2070,6 +2075,168 @@ async RestAdminClusterHandler::handleNumberOfServers() { } } +async RestAdminClusterHandler::handleUniqId() { + if (!ServerState::instance()->isCoordinator()) { + generateError(rest::ResponseCode::FORBIDDEN, TRI_ERROR_HTTP_FORBIDDEN, + "only allowed on coordinators"); + co_return; + } + + // Only PUT method is allowed and always requires admin privileges + if (!ExecContext::current().isAdminUser()) { + generateError(rest::ResponseCode::FORBIDDEN, TRI_ERROR_HTTP_FORBIDDEN); + co_return; + } + + switch (request()->requestType()) { + case rest::RequestType::PUT: + co_return co_await handlePutUniqId(); + default: + generateError(rest::ResponseCode::METHOD_NOT_ALLOWED, + TRI_ERROR_HTTP_METHOD_NOT_ALLOWED); + co_return; + } +} + +// The following RestHandler method has two purposes: +// (1) Fetch and reserve a certain range of globally-unique IDs from the +// cluster. This is, when the `number` query parameter is given. +// (2) Make sure that subsequent ranges which anybody will reserve will +// always be ranges whose members are larger than the given value. +// This is, then the `minimum` query parameter is given. +// In the first case, there is a limitation to at most 1000000 keys at +// a time, in the second case the minimum must be at most 2^60 and if the +// currently maximal ID is already larger, nothing is done. +// Both cases return a range of reserved keys. + +async RestAdminClusterHandler::handlePutUniqId() { + if (AsyncAgencyCommManager::INSTANCE == nullptr) { + generateError(rest::ResponseCode::FORBIDDEN, TRI_ERROR_HTTP_FORBIDDEN, + "not allowed on single servers"); + co_return; + } + + // Parse query parameters + auto& req = *request(); + + std::string numberParam = req.value("number"); + std::string minimumParam = req.value("minimum"); + + // Validate that exactly one parameter is provided + if (numberParam.empty() && minimumParam.empty()) { + generateError(rest::ResponseCode::BAD, TRI_ERROR_BAD_PARAMETER, + "either 'number' or 'minimum' parameter must be provided"); + co_return; + } + + if (!numberParam.empty() && !minimumParam.empty()) { + generateError( + rest::ResponseCode::BAD, TRI_ERROR_BAD_PARAMETER, + "only one of 'number' or 'minimum' parameter can be provided"); + co_return; + } + + try { + uint64_t smallest, largest; + + auto& ci = server().getFeature().clusterInfo(); + + if (!numberParam.empty()) { + // Mode 1: Get a specific number of unique IDs + uint64_t number; + try { + number = std::stoull(numberParam); + } catch (std::exception const&) { + generateError(rest::ResponseCode::BAD, TRI_ERROR_BAD_PARAMETER, + "parameter 'number' must be a valid positive integer"); + co_return; + } + + if (number < 1 || number > 1000000) { + generateError(rest::ResponseCode::BAD, TRI_ERROR_BAD_PARAMETER, + "parameter 'number' must be between 1 and 1000000"); + co_return; + } + + // Use AgencyComm via ClusterInfo to get unique IDs: + auto val = ci.uniqidFromAgency(number); + if (!val.has_value()) { + generateError(rest::ResponseCode::SERVICE_UNAVAILABLE, + TRI_ERROR_HTTP_SERVICE_UNAVAILABLE, + "could not talk to agency"); + co_return; + } + + smallest = val.value(); + largest = smallest + number - 1; + + } else { + // Mode 2: Ensure LatestID is at least the minimum value + uint64_t minimum; + try { + minimum = std::stoull(minimumParam); + } catch (std::exception const&) { + generateError(rest::ResponseCode::BAD, TRI_ERROR_BAD_PARAMETER, + "parameter 'minimum' must be a valid positive integer"); + co_return; + } + + // Limit to 2^60 + constexpr uint64_t maxValue = 1ULL << 60; + if (minimum > maxValue) { + generateError(rest::ResponseCode::BAD, TRI_ERROR_BAD_PARAMETER, + "parameter 'minimum' must not exceed 2^60"); + co_return; + } + + // Read current LatestID to see if we need to update it + auto val = ci.uniqidFromAgency(1); + if (!val.has_value()) { + generateError(rest::ResponseCode::SERVICE_UNAVAILABLE, + TRI_ERROR_HTTP_SERVICE_UNAVAILABLE, + "could not talk to agency"); + co_return; + } + uint64_t currentValue = val.value(); + + if (currentValue >= minimum) { + // Current value is already >= minimum, return current range + smallest = currentValue; + largest = currentValue; + } else { + // Need to update LatestID to minimum + uint64_t diff = minimum - currentValue; + val = ci.uniqidFromAgency(diff); + if (!val.has_value()) { + generateError(rest::ResponseCode::SERVICE_UNAVAILABLE, + TRI_ERROR_HTTP_SERVICE_UNAVAILABLE, + "could not talk to agency"); + co_return; + } + smallest = val.value(); + largest = smallest + diff - 1; + } + } + + // Generate response JSON + VPackBuilder builder; + { + VPackObjectBuilder ob(&builder); + builder.add("smallest", VPackValue(std::to_string(smallest))); + builder.add("largest", VPackValue(std::to_string(largest))); + } + + generateOk(rest::ResponseCode::OK, builder); + + } catch (basics::Exception const& e) { + generateError(rest::ResponseCode::SERVER_ERROR, TRI_ERROR_HTTP_SERVER_ERROR, + e.what()); + } catch (std::exception const& e) { + generateError(rest::ResponseCode::SERVER_ERROR, TRI_ERROR_HTTP_SERVER_ERROR, + e.what()); + } +} + async RestAdminClusterHandler::handleHealth() { // We allow this API whenever one is authenticated in some way. There used // to be a check for isAdminUser here. However, we want that the UI with diff --git a/arangod/RestHandler/RestAdminClusterHandler.h b/arangod/RestHandler/RestAdminClusterHandler.h index ea69a778f36f..3705e7cea400 100644 --- a/arangod/RestHandler/RestAdminClusterHandler.h +++ b/arangod/RestHandler/RestAdminClusterHandler.h @@ -51,6 +51,7 @@ class RestAdminClusterHandler : public RestVocbaseBaseHandler { static std::string const CancelJob; static std::string const Health; static std::string const NumberOfServers; + static std::string const UniqId; static std::string const Maintenance; static std::string const NodeVersion; static std::string const NodeStatistics; @@ -89,6 +90,9 @@ class RestAdminClusterHandler : public RestVocbaseBaseHandler { async handleGetNumberOfServers(); async handlePutNumberOfServers(); + async handleUniqId(); + async handlePutUniqId(); + async handleNodeVersion(); async handleNodeStatistics(); async handleNodeEngine(); diff --git a/arangod/VocBase/KeyGenerator.cpp b/arangod/VocBase/KeyGenerator.cpp index 013bd8a0fb0c..0ea17876d888 100644 --- a/arangod/VocBase/KeyGenerator.cpp +++ b/arangod/VocBase/KeyGenerator.cpp @@ -291,7 +291,8 @@ class TraditionalKeyGeneratorSingle final : public TraditionalKeyGenerator { bool allowUserKeys, uint64_t lastValue) : TraditionalKeyGenerator(collection, allowUserKeys), _lastValue(lastValue) { - TRI_ASSERT(!ServerState::instance()->isCoordinator()); + TRI_ASSERT(!ServerState::instance()->isCoordinator() && + !ServerState::instance()->isDBServer()); } /// @brief build a VelocyPack representation of the generator in the builder @@ -368,14 +369,15 @@ class TraditionalKeyGeneratorCoordinator final explicit TraditionalKeyGeneratorCoordinator( ClusterInfo& ci, LogicalCollection const& collection, bool allowUserKeys) : TraditionalKeyGenerator(collection, allowUserKeys), _ci(ci) { - TRI_ASSERT(ServerState::instance()->isCoordinator()); + TRI_ASSERT(ServerState::instance()->isCoordinator() || + ServerState::instance()->isDBServer()); } private: /// @brief generate a key value (internal) uint64_t generateValue() override { - TRI_ASSERT(ServerState::instance()->isCoordinator()); - TRI_ASSERT(_collection.numberOfShards() != 1 || _collection.isSmart()); + TRI_ASSERT(ServerState::instance()->isCoordinator() || + ServerState::instance()->isDBServer()); TRI_IF_FAILURE("KeyGenerator::generateOnCoordinator") { // for testing purposes only THROW_ARANGO_EXCEPTION(TRI_ERROR_DEBUG); @@ -496,8 +498,6 @@ class PaddedKeyGeneratorSingle final : public PaddedKeyGenerator { private: /// @brief generate a key uint64_t generateValue() override { - TRI_ASSERT(ServerState::instance()->isSingleServer() || - _collection.numberOfShards() == 1); TRI_IF_FAILURE("KeyGenerator::generateOnSingleServer") { // for testing purposes only THROW_ARANGO_EXCEPTION(TRI_ERROR_DEBUG); @@ -519,14 +519,15 @@ class PaddedKeyGeneratorCoordinator final : public PaddedKeyGenerator { LogicalCollection const& collection, bool allowUserKeys, uint64_t lastValue) : PaddedKeyGenerator(collection, allowUserKeys, lastValue), _ci(ci) { - TRI_ASSERT(ServerState::instance()->isCoordinator()); + TRI_ASSERT(ServerState::instance()->isCoordinator() || + ServerState::instance()->isDBServer()); } private: /// @brief generate a key value (internal) uint64_t generateValue() override { - TRI_ASSERT(ServerState::instance()->isCoordinator()); - TRI_ASSERT(_collection.numberOfShards() != 1 || _collection.isSmart()); + TRI_ASSERT(ServerState::instance()->isCoordinator() || + ServerState::instance()->isDBServer()); TRI_IF_FAILURE("KeyGenerator::generateOnCoordinator") { // for testing purposes only THROW_ARANGO_EXCEPTION(TRI_ERROR_DEBUG); @@ -717,7 +718,20 @@ std::unordered_mapisCoordinator()) { + if (ServerState::instance()->isCoordinator() || + ServerState::instance()->isDBServer()) { + // We are in a cluster where keys need to be globally unique. Under + // normal circumstances, coordinators create the keys. + // However, there are cases, in which this can happen on a DBServer, + // amongst them are: + // - collections with a single shard and an AQL query which does + // INSERT in a loop, no _key given by query + // - collections with multiple shards, non-standard sharding, an + // AQL query which does INSERT in a loop, no key given by query + // and usage of the option `forceOneShardAttributeValue` + // In these cases we have to generate a key using a cluster-wide + // unique identifier, so we use the "Coordinator" key generator + // also on DBServers. auto& ci = collection.vocbase() .server() .getFeature() @@ -802,7 +816,20 @@ std::unordered_mapisCoordinator()) { + if (ServerState::instance()->isCoordinator() || + ServerState::instance()->isDBServer()) { + // We are in a cluster where keys need to be globally unique. Under + // normal circumstances, coordinators create the keys. + // However, there are cases, in which this can happen on a DBServer, + // amongst them are: + // - collections with a single shard and an AQL query which does + // INSERT in a loop, no _key given by query + // - collections with multiple shards, non-standard sharding, an + // AQL query which does INSERT in a loop, no key given by query + // and usage of the option `forceOneShardAttributeValue` + // In these cases we have to generate a key using a cluster-wide + // unique identifier, so we use the "Coordinator" key generator + // also on DBServers. auto& ci = collection.vocbase() .server() .getFeature() diff --git a/tests/Transaction/ManagerTest.cpp b/tests/Transaction/ManagerTest.cpp index 01ca954a3a40..07335e8fae58 100644 --- a/tests/Transaction/ManagerTest.cpp +++ b/tests/Transaction/ManagerTest.cpp @@ -304,7 +304,8 @@ TEST_F(TransactionManagerTest, simple_transaction_and_commit_is_follower) { ASSERT_TRUE( trx.state()->hasHint(transaction::Hints::Hint::IS_FOLLOWER_TRX)); - auto doc = arangodb::velocypack::Parser::fromJson("{ \"abc\": 1}"); + auto doc = arangodb::velocypack::Parser::fromJson( + "{ \"_key\": \"blablabla\", \"abc\": 1}"); OperationOptions opts; auto opRes = trx.insert(coll->name(), doc->slice(), opts); diff --git a/tests/js/client/shell/shell-key-generation-one-shard-cluster.js b/tests/js/client/shell/shell-key-generation-one-shard-cluster.js new file mode 100644 index 000000000000..9917dfca37eb --- /dev/null +++ b/tests/js/client/shell/shell-key-generation-one-shard-cluster.js @@ -0,0 +1,317 @@ +/*jshint globalstrict:false, strict:false */ +/*global arango, assertEqual, assertTrue, assertFalse, assertMatch */ + +// ////////////////////////////////////////////////////////////////////////////// +// / DISCLAIMER +// / +// / Copyright 2014-2025 ArangoDB GmbH, Cologne, Germany +// / Copyright 2004-2014 triAGENS GmbH, Cologne, Germany +// / +// / Licensed under the Business Source License 1.1 (the "License"); +// / you may not use this file except in compliance with the License. +// / You may obtain a copy of the License at +// / +// / https://github.com/arangodb/arangodb/blob/devel/LICENSE +// / +// / 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 ArangoDB GmbH, Cologne, Germany +// / +/// @author Test Author Max Neunhoeffer +/// @author Copyright 2025, ArangoDB GmbH, Cologne, Germany +// ////////////////////////////////////////////////////////////////////////////// + +const jsunity = require("jsunity"); +const arangodb = require("@arangodb"); +const db = arangodb.db; +const internal = require("internal"); +const isCluster = internal.isCluster(); +let { instanceRole } = require('@arangodb/testutils/instance'); +let IM = global.instanceManager; + +const cn = "UnitTestsKeyGenerationOneShard"; + +//////////////////////////////////////////////////////////////////////////////// +/// @brief test suite: key generation in one-shard collection +//////////////////////////////////////////////////////////////////////////////// + +function KeyGenerationOneShardSuite() { + 'use strict'; + + return { + + setUp: function () { + // Clean up any existing collection + try { + db._drop(cn); + } catch (err) { + // Collection might not exist, ignore + } + }, + + tearDown: function () { + IM.debugClearFailAt('always-fetch-new-cluster-wide-uniqid', instanceRole.dbServer); + // Clean up the test collection + try { + db._drop(cn); + } catch (err) { + // Collection might not exist, ignore + } + }, + + testKeyGenerationOneShardCollection: function () { + // Create a collection with exactly one shard + let c = db._create(cn, { + numberOfShards: 1, + keyOptions: { + type: "traditional", + allowUserKeys: false + } + }); + + // Verify the collection has exactly one shard + assertEqual(1, c.shards().length, "Collection should have exactly one shard"); + assertEqual(cn, c.name(), "Collection name should match"); + + // Set failure points to always fetch new cluster-wide unique IDs from agency on dbservers: + IM.debugSetFailAt('always-fetch-new-cluster-wide-uniqid', instanceRole.dbServer); + + let res = arango.PUT("/_admin/cluster/uniqId?minimum=100000000",{}); + assertFalse(res.error); + assertEqual(200, res.code, `must return HTTP code 200 but got ${res.code}`); + + // Insert documents using the AQL statement + let query = ` + LET l = [{Hello:1},{Hello:2}] + FOR doc IN l + INSERT doc INTO ${cn} + RETURN NEW + `; + + let result = db._query(query).toArray(); + + // Verify we got 2 results back + assertEqual(2, result.length, "Should have inserted 2 documents"); + + // Check that both documents have automatically generated keys + result.forEach((doc, index) => { + assertTrue(doc.hasOwnProperty('_key'), `Document ${index} should have _key property`); + assertTrue(doc.hasOwnProperty('_id'), `Document ${index} should have _id property`); + assertTrue(doc.hasOwnProperty('_rev'), `Document ${index} should have _rev property`); + assertTrue(doc.hasOwnProperty('Hello'), `Document ${index} should have Hello property`); + + // Verify the Hello property values + if (index === 0) { + assertEqual(1, doc.Hello, `First document should have Hello: 1`); + } else { + assertEqual(2, doc.Hello, `Second document should have Hello: 2`); + } + + // Verify the _key is a valid traditional key (numeric string) + assertMatch(/^\d+$/, doc._key, `_key should be a numeric string: ${doc._key}`); + + // Verify the _id follows the pattern collectionName/_key + assertMatch(new RegExp(`^${cn}/\\d+$`), doc._id, `_id should follow pattern: ${doc._id}`); + }); + + // Verify the collection count + assertEqual(2, c.count(), "Collection should contain exactly 2 documents"); + + // Verify the documents exist in the collection + let allDocs = c.toArray(); + assertEqual(2, allDocs.length, "Collection should return exactly 2 documents"); + + // Check that the keys are large: + let keys = allDocs.map(doc => parseInt(doc._key)).sort((a, b) => a - b); + assertTrue(keys[0] >= 100000000, `First key must be at least 100000000 and not ${keys[0]}`); + assertTrue(keys[1] >= 100000000, `Second key be must be at leaset 100000000 and not ${keys[1]}`); + }, + + testKeyGenerationOneShardCollectionWithPaddedKeyOptions: function () { + // Create a collection with exactly one shard and padded key options + let c = db._create(cn, { + numberOfShards: 1, + keyOptions: { + type: "padded", + allowUserKeys: false, + } + }); + + // Verify the collection has exactly one shard + assertEqual(1, c.shards().length, "Collection should have exactly one shard"); + + // Set failure points to always fetch new cluster-wide unique IDs from agency on dbservers: + IM.debugSetFailAt('always-fetch-new-cluster-wide-uniqid', instanceRole.dbServer); + + let res = arango.PUT("/_admin/cluster/uniqId?minimum=100000000",{}); + assertFalse(res.error); + assertEqual(200, res.code, `must return HTTP code 200 but got ${res.code}`); + + // Insert documents using the AQL statement + let query = ` + LET l = [{Hello:1},{Hello:2}] + FOR doc IN l + INSERT doc INTO ${cn} + RETURN NEW + `; + + let result = db._query(query).toArray(); + + // Verify we got 2 results back + assertEqual(2, result.length, "Should have inserted 2 documents"); + + // Check that both documents have automatically generated keys + result.forEach((doc, index) => { + assertTrue(doc.hasOwnProperty('_key'), `Document ${index} should have _key property`); + assertTrue(doc.hasOwnProperty('_id'), `Document ${index} should have _id property`); + assertTrue(doc.hasOwnProperty('_rev'), `Document ${index} should have _rev property`); + assertTrue(doc.hasOwnProperty('Hello'), `Document ${index} should have Hello property`); + }); + + // Verify the collection count + assertEqual(2, c.count(), "Collection should contain exactly 2 documents"); + + // Verify the documents exist in the collection + let allDocs = c.toArray(); + assertEqual(2, allDocs.length, "Collection should return exactly 2 documents"); + + // Check that the keys are large, note that in the padded case we see hex numbers!: + let keys = allDocs.map(doc => doc._key); + assertTrue(keys[0] >= "0000000005f5e100", `First key must be at least 0000000005f5e100 and not ${keys[0]}`); + assertTrue(keys[1] >= "0000000005f5e100", `Second key must be at least 0000000005f5e100 and not ${keys[1]}`); + }, + + testKeyGenerationMultiShardCollectionForced: function () { + // Create a collection with 3 shards: + let c = db._create(cn, { + numberOfShards: 3, + shardKeys: ["s"], + keyOptions: { + type: "traditional", + allowUserKeys: false + } + }); + + // Verify the collection has three shards + assertEqual(3, c.shards().length, "Collection should have three shards"); + assertEqual(cn, c.name(), "Collection name should match"); + + // Set failure points to always fetch new cluster-wide unique IDs from agency on dbservers: + IM.debugSetFailAt('always-fetch-new-cluster-wide-uniqid', instanceRole.dbServer); + + let res = arango.PUT("/_admin/cluster/uniqId?minimum=100000000",{}); + assertFalse(res.error); + assertEqual(200, res.code, `must return HTTP code 200 but got ${res.code}`); + + // Insert documents using the AQL statement + let query = ` + LET l = [{Hello:1, s: "a"},{Hello:2, s: "a"}] + FOR doc IN l + INSERT doc INTO ${cn} + RETURN NEW + `; + + let result = db._query(query, {}, {forceOneShardAttributeValue: "a"}).toArray(); + + // Verify we got 2 results back + assertEqual(2, result.length, "Should have inserted 2 documents"); + + // Check that both documents have automatically generated keys + result.forEach((doc, index) => { + assertTrue(doc.hasOwnProperty('_key'), `Document ${index} should have _key property`); + assertTrue(doc.hasOwnProperty('_id'), `Document ${index} should have _id property`); + assertTrue(doc.hasOwnProperty('_rev'), `Document ${index} should have _rev property`); + assertTrue(doc.hasOwnProperty('Hello'), `Document ${index} should have Hello property`); + + // Verify the Hello property values + if (index === 0) { + assertEqual(1, doc.Hello, `First document should have Hello: 1`); + } else { + assertEqual(2, doc.Hello, `Second document should have Hello: 2`); + } + + // Verify the _key is a valid traditional key (numeric string) + assertMatch(/^\d+$/, doc._key, `_key should be a numeric string: ${doc._key}`); + + // Verify the _id follows the pattern collectionName/_key + assertMatch(new RegExp(`^${cn}/\\d+$`), doc._id, `_id should follow pattern: ${doc._id}`); + }); + + // Verify the collection count + assertEqual(2, c.count(), "Collection should contain exactly 2 documents"); + + // Verify the documents exist in the collection + let allDocs = c.toArray(); + assertEqual(2, allDocs.length, "Collection should return exactly 2 documents"); + + // Check that the keys are large: + let keys = allDocs.map(doc => parseInt(doc._key)).sort((a, b) => a - b); + assertTrue(keys[0] >= 100000000, "First key must be at least 100000000"); + assertTrue(keys[1] >= 100000000, "Second key be must be at leaset 100000000"); + }, + + testKeyGenerationMultiShardCollectionForcedWithPaddedKeyOptions: function () { + // Create a collection with exactly one shard and padded key options + let c = db._create(cn, { + numberOfShards: 3, + shardKeys: ["s"], + keyOptions: { + type: "padded", + allowUserKeys: false, + } + }); + + // Verify the collection has exactly one shard + assertEqual(3, c.shards().length, "Collection should have exactly three shards"); + + // Set failure points to always fetch new cluster-wide unique IDs from agency on dbservers: + IM.debugSetFailAt('always-fetch-new-cluster-wide-uniqid', instanceRole.dbServer); + + let res = arango.PUT("/_admin/cluster/uniqId?minimum=100000000",{}); + assertFalse(res.error); + assertEqual(200, res.code, `must return HTTP code 200 but got ${res.code}`); + + // Insert documents using the AQL statement + let query = ` + LET l = [{Hello:1, s: "a"},{Hello:2, s: "a"}] + FOR doc IN l + INSERT doc INTO ${cn} + RETURN NEW + `; + + let result = db._query(query, {}, {forceOneShardAttributeValue: "a"}).toArray(); + + // Verify we got 2 results back + assertEqual(2, result.length, "Should have inserted 2 documents"); + + // Check that both documents have automatically generated keys + result.forEach((doc, index) => { + assertTrue(doc.hasOwnProperty('_key'), `Document ${index} should have _key property`); + assertTrue(doc.hasOwnProperty('_id'), `Document ${index} should have _id property`); + assertTrue(doc.hasOwnProperty('_rev'), `Document ${index} should have _rev property`); + assertTrue(doc.hasOwnProperty('Hello'), `Document ${index} should have Hello property`); + }); + + // Verify the collection count + assertEqual(2, c.count(), "Collection should contain exactly 2 documents"); + + // Verify the documents exist in the collection + let allDocs = c.toArray(); + assertEqual(2, allDocs.length, "Collection should return exactly 2 documents"); + + // Check that the keys are large, note that in the padded case we see hex numbers!: + let keys = allDocs.map(doc => doc._key); + assertTrue(keys[0] >= "0000000005f5e100", `First key must be at least 0000000005f5e100 and not ${keys[0]}`); + assertTrue(keys[1] >= "0000000005f5e100", `Second key must be at least 0000000005f5e100 and not ${keys[1]}`); + }, + + }; +} + +// Run the appropriate test suite based on cluster mode +jsunity.run(KeyGenerationOneShardSuite); +return jsunity.done(); diff --git a/tests/js/client/shell/shell-keygen.js b/tests/js/client/shell/shell-keygen.js index cf66b3caed50..e863636bde00 100644 --- a/tests/js/client/shell/shell-keygen.js +++ b/tests/js/client/shell/shell-keygen.js @@ -1138,31 +1138,17 @@ function KeyGenerationLocationSuite() { db._drop(cn); }, - testThatFailurePointsWork1: function () { - if (!cluster) { - return; - } - - let c = db._create(cn, {keyOptions: {type: "traditional"}, numberOfShards: 1}); - - IM.debugSetFailAt("KeyGenerator::generateOnSingleServer", filter); - try { - c.insert({}); - } catch (err) { - assertEqual(ERRORS.ERROR_DEBUG.code, err.errorNum); - } - }, - - testThatFailurePointsWork2: function () { + testThatFailurePointsWork: function () { if (!cluster) { return; } let c = db._create(cn, {keyOptions: {type: "traditional"}, numberOfShards: 2}); - IM.debugSetFailAt("KeyGenerator::generateOnCoordinator", filter); + IM.debugSetFailAt("KeyGenerator::generateOnCoordinator"); try { c.insert({}); + fail(); } catch (err) { assertEqual(ERRORS.ERROR_DEBUG.code, err.errorNum); } @@ -1173,8 +1159,8 @@ function KeyGenerationLocationSuite() { return; } - // fail if we generate a key on a coordinator - IM.debugSetFailAt("KeyGenerator::generateOnCoordinator", filter); + // fail if we generate a key with the single server key generator + IM.debugSetFailAt("KeyGenerator::generateOnSingleServer"); generators().forEach((generator) => { let c = db._create(cn, {keyOptions: {type: generator}, numberOfShards: 1}); @@ -1209,8 +1195,8 @@ function KeyGenerationLocationSuite() { return; } - // fail if we generate a key on a coordinator - IM.debugSetFailAt("KeyGenerator::generateOnCoordinator", filter); + // fail if we generate a key with the single server key generator: + IM.debugSetFailAt("KeyGenerator::generateOnSingleServer"); generators().forEach((generator) => { let c = db._create(cn, {keyOptions: {type: generator}, numberOfShards: 1, shardKeys: ["id"]}); @@ -1246,8 +1232,8 @@ function KeyGenerationLocationSuite() { return; } - // fail if we generate a key on a coordinator - IM.debugSetFailAt("KeyGenerator::generateOnCoordinator", filter); + // fail if we generate a key with the single server key generator: + IM.debugSetFailAt("KeyGenerator::generateOnSingleServer"); db._createDatabase(cn, {sharding: "single"}); try { @@ -1383,6 +1369,289 @@ function KeyGenerationLocationSuite() { }; } +function UpsertKeyGenerationSuite() { + 'use strict'; + + let generators = function () { + return ["traditional", "padded"]; + }; + + return { + setUp: function () { + db._drop(cn); + try { + graphs._drop(gn, true); + } catch (err) { + } + }, + + tearDown: function () { + IM.debugClearFailAt(); + db._drop(cn); + try { + graphs._drop(gn, true); + } catch (err) { + } + }, + + testUpsertSingleShardCollection: function () { + if (!cluster) { + return; + } + + // fail if we generate a key with the single server key generator + IM.debugSetFailAt("KeyGenerator::generateOnSingleServer"); + + generators().forEach((generator) => { + let c = db._create(cn, {keyOptions: {type: generator}, numberOfShards: 1}); + try { + // UPSERT should not fail because key generation happens with the + // coordinator key generator (potentially on the DBServer!) + let result = db._query(`UPSERT {value:1} INSERT {Hello:1, value:1} UPDATE {value:1} INTO ${cn} RETURN NEW`); + assertEqual(1, result.toArray().length); + assertEqual(1, c.count()); + } finally { + db._drop(cn); + } + }); + }, + + testUpsertMultiShardCollection: function () { + if (!cluster) { + return; + } + + // fail if we generate a key with the single server key generator + IM.debugSetFailAt("KeyGenerator::generateOnSingleServer"); + + generators().forEach((generator) => { + let c = db._create(cn, {keyOptions: {type: generator}, numberOfShards: 2}); + try { + // UPSERT should not fail because key generation happens with + // the coordinator key generator (potentially on the DBServer!) + let result = db._query(`UPSERT {value:1} INSERT {Hello:1, value:1} UPDATE {value:1} INTO ${cn} RETURN NEW`); + assertEqual(1, result.toArray().length); + assertEqual(1, c.count()); + } finally { + db._drop(cn); + } + }); + }, + + testUpsertSingleShardEdgeCollection: function () { + if (!cluster) { + return; + } + + // fail if we generate a key with the single server key generator + IM.debugSetFailAt("KeyGenerator::generateOnSingleServer"); + + generators().forEach((generator) => { + let c = db._createEdgeCollection(cn, {keyOptions: {type: generator}, numberOfShards: 1}); + try { + // UPSERT should not fail because key generation happens on coordinator + let result = db._query(`UPSERT {value:1} INSERT {Hello:1, value:1, _from: "${vn}/test", _to: "${vn}/test"} UPDATE {value:1} INTO ${cn} RETURN NEW`); + assertEqual(1, result.toArray().length); + assertEqual(1, c.count()); + } finally { + db._drop(cn); + } + }); + }, + + testUpsertMultiShardEdgeCollection: function () { + if (!cluster) { + return; + } + + // fail if we generate a key with the single server key generator + IM.debugSetFailAt("KeyGenerator::generateOnSingleServer"); + + generators().forEach((generator) => { + let c = db._createEdgeCollection(cn, {keyOptions: {type: generator}, numberOfShards: 2}); + try { + // UPSERT should not fail because key generation happens with the + // coordinator key generator + let result = db._query(`UPSERT {value:1} INSERT {Hello:1, value:1, _from: "${vn}/test", _to: "${vn}/test"} UPDATE {value:1} INTO ${cn} RETURN NEW`); + assertEqual(1, result.toArray().length); + assertEqual(1, c.count()); + } finally { + db._drop(cn); + } + }); + }, + + testUpsertNonStandardShardingCollection: function () { + if (!cluster) { + return; + } + + // fail if we generate a key with the single server key generator + IM.debugSetFailAt("KeyGenerator::generateOnSingleServer"); + + generators().forEach((generator) => { + let c = db._create(cn, {keyOptions: {type: generator}, numberOfShards: 2, shardKeys: ["id"]}); + try { + // UPSERT should not fail because key generation happens on coordinator + let result = db._query(`UPSERT {value:1} INSERT {Hello:1, value:1, id: "test"} UPDATE {value:1} INTO ${cn} RETURN NEW`); + assertEqual(1, result.toArray().length); + result = db._query(`UPSERT {value:2, id: "test"} INSERT {Hello:1, value:2, id: "test"} UPDATE {value:2} INTO ${cn} RETURN NEW`); + assertEqual(1, result.toArray().length); + assertEqual(2, c.count()); + } finally { + db._drop(cn); + } + }); + }, + + testUpsertSmartVertexCollection: function () { + if (!isEnterprise || !cluster) { + return; + } + + // fail if we generate a key with the single server key generator + IM.debugSetFailAt("KeyGenerator::generateOnSingleServer"); + + const smartGraphProperties = { + numberOfShards: 2, + smartGraphAttribute: "value" + }; + + graphs._create(gn, [graphs._relation(en, vn, vn)], null, smartGraphProperties); + try { + // UPSERT should not fail because key generation uses proper generator + let result = db._query(`UPSERT {value:"1"} INSERT {Hello:1, value:"1"} UPDATE {value:"1"} INTO ${vn} RETURN NEW`); + assertEqual(1, result.toArray().length); + assertEqual(1, db[vn].count()); + } finally { + graphs._drop(gn, true); + } + }, + + testUpsertSmartEdgeCollection: function () { + if (!isEnterprise || !cluster) { + return; + } + + // fail if we generate a key with the single server key generator + IM.debugSetFailAt("KeyGenerator::generateOnSingleServer"); + + const smartGraphProperties = { + numberOfShards: 2, + smartGraphAttribute: "value" + }; + + graphs._create(gn, [graphs._relation(en, vn, vn)], null, smartGraphProperties); + try { + // UPSERT should not fail because key generation uses proper generator + let result = db._query(`UPSERT {value:"1"} INSERT {Hello:1, value:"1", _from: "${vn}/test:1", _to: "${vn}/test:1"} UPDATE {value:"1"} INTO ${en} RETURN NEW`); + assertEqual(1, result.toArray().length); + assertEqual(1, db[en].count()); + } finally { + graphs._drop(gn, true); + } + }, + + testUpsertEnterpriseVertexCollection: function () { + if (!isEnterprise || !cluster) { + return; + } + // fail if we generate a key on a DB server + IM.debugSetFailAt("KeyGenerator::generateOnSingleServer"); + + const enterpriseGraphProperties = { + numberOfShards: 2, + isSmart: true + }; + + graphs._create(gn, [graphs._relation(en, vn, vn)], null, enterpriseGraphProperties); + try { + // UPSERT should not fail because key generation uses proper location + let result = db._query(`UPSERT {value:1} INSERT {Hello:1, value:1} UPDATE {value:1} INTO ${vn} RETURN NEW`); + assertEqual(1, result.toArray().length); + assertEqual(1, db[vn].count()); + } finally { + graphs._drop(gn, true); + } + }, + + testUpsertEnterpriseEdgeCollection: function () { + if (!isEnterprise || !cluster) { + return; + } + + // fail if we generate a key with the single server key generator + IM.debugSetFailAt("KeyGenerator::generateOnSingleServer"); + + const enterpriseGraphProperties = { + numberOfShards: 2, + isSmart: true + }; + + graphs._create(gn, [graphs._relation(en, vn, vn)], null, enterpriseGraphProperties); + try { + // UPSERT should not fail because key generation uses proper generator + let result = db._query(`UPSERT {value:1} INSERT {Hello:1, value:1, _from: "${vn}/test", _to: "${vn}/test"} UPDATE {value:1} INTO ${en} RETURN NEW`); + assertEqual(1, result.toArray().length); + assertEqual(1, db[en].count()); + } finally { + graphs._drop(gn, true); + } + }, + + testUpsertDisjointSmartVertexCollection: function () { + if (!isEnterprise || !cluster) { + return; + } + + // fail if we generate a key with the single server key generator + IM.debugSetFailAt("KeyGenerator::generateOnSingleServer"); + + const disjointSmartGraphProperties = { + numberOfShards: 2, + smartGraphAttribute: "value", + isDisjoint: true + }; + + graphs._create(gn, [graphs._relation(en, vn, vn)], null, disjointSmartGraphProperties); + try { + // UPSERT should not fail because key generation uses proper generator + let result = db._query(`UPSERT {value:"1"} INSERT {Hello:1, value:"1"} UPDATE {value:"1"} INTO ${vn} RETURN NEW`); + assertEqual(1, result.toArray().length); + assertEqual(1, db[vn].count()); + } finally { + graphs._drop(gn, true); + } + }, + + testUpsertDisjointSmartEdgeCollection: function () { + if (!isEnterprise || !cluster) { + return; + } + + // fail if we generate a key with the single server key generator + IM.debugSetFailAt("KeyGenerator::generateOnSingleServer"); + + const disjointSmartGraphProperties = { + numberOfShards: 2, + smartGraphAttribute: "value", + isDisjoint: true + }; + + graphs._create(gn, [graphs._relation(en, vn, vn)], null, disjointSmartGraphProperties); + try { + // UPSERT should not fail because key generation uses proper generator + let result = db._query(`UPSERT {value:"1"} INSERT {Hello:1, value:"1", _from: "${vn}/test:1", _to: "${vn}/test:1"} UPDATE {value:"1"} INTO ${en} RETURN NEW`); + assertEqual(1, result.toArray().length); + assertEqual(1, db[en].count()); + } finally { + graphs._drop(gn, true); + } + }, + + }; +} + function KeyGenerationLocationSmartGraphSuite() { 'use strict'; @@ -1408,8 +1677,8 @@ function KeyGenerationLocationSmartGraphSuite() { testSingleShardSmartVertexInserts: function () { if (cluster) { - // fail if we generate a key on a coordinator - IM.debugSetFailAt("KeyGenerator::generateOnCoordinator", filter); + // fail if we generate a key with the single server key generator + IM.debugSetFailAt("KeyGenerator::generateOnSingleServer"); } else { // single server: we can actually get here with the SmartGraph simulator! IM.debugSetFailAt("KeyGenerator::generateOnCoordinator", filter); @@ -1525,6 +1794,7 @@ jsunity.run(PersistedLastValueSuite); if (IM.debugCanUseFailAt()) { jsunity.run(KeyGenerationLocationSuite); + jsunity.run(UpsertKeyGenerationSuite); } if (isEnterprise && IM.debugCanUseFailAt()) { jsunity.run(KeyGenerationLocationSmartGraphSuite);