clean up key generators a bit (#5573)

arangod/Aql/ClusterBlocks.cpp

@@ -41,6 +41,8 @@
 #include "Cluster/ServerState.h"
 #include "Scheduler/JobGuard.h"
 #include "Scheduler/SchedulerFeature.h"
+#include "VocBase/KeyGenerator.h"
+#include "VocBase/LogicalCollection.h"
 #include "VocBase/ticks.h"
 #include "VocBase/vocbase.h"
 
@@ -830,9 +832,8 @@ size_t DistributeBlock::sendToClient(AqlItemBlock* cur) {
 /// @brief create a new document key, argument is unused here
 #ifndef USE_ENTERPRISE
 std::string DistributeBlock::createKey(VPackSlice) const {
-  ClusterInfo* ci = ClusterInfo::instance();
+  auto collInfo = _collection->getCollection();
-  uint64_t uid = ci->uniqid();
+  return collInfo->keyGenerator()->generate();
-  return std::to_string(uid);
 }
 #endif
 

arangod/Cluster/ClusterMethods.cpp

@@ -35,6 +35,7 @@
 #include "Indexes/Index.h"
 #include "Utils/CollectionNameResolver.h"
 #include "Utils/OperationOptions.h"
+#include "VocBase/KeyGenerator.h"
 #include "VocBase/LogicalCollection.h"
 #include "VocBase/ticks.h"
 
@@ -381,8 +382,7 @@ static int distributeBabyOnShards(
     VPackSlice keySlice = node.get(StaticStrings::KeyString);
     if (keySlice.isNone()) {
       // The user did not specify a key, let's create one:
-      uint64_t uid = ci->uniqid();
+      _key = collinfo->keyGenerator()->generate();
-      _key = arangodb::basics::StringUtils::itoa(uid);
     } else {
       userSpecifiedKey = true;
     }

arangod/VocBase/KeyGenerator.cpp

@@ -22,13 +22,11 @@
 ////////////////////////////////////////////////////////////////////////////////
 
 #include "KeyGenerator.h"
-#include "Basics/conversions.h"
 #include "Basics/MutexLocker.h"
 #include "Basics/NumberUtils.h"
 #include "Basics/StringUtils.h"
-#include "Basics/tri-strings.h"
 #include "Basics/VelocyPackHelper.h"
-#include "Basics/voc-errors.h"
+#include "Cluster/ClusterInfo.h"
 #include "VocBase/ticks.h"
 #include "VocBase/vocbase.h"
 
@@ -113,9 +111,6 @@ static std::array<bool, 256> const keyCharLookupTable = { {
 KeyGenerator::KeyGenerator(bool allowUserKeys)
     : _allowUserKeys(allowUserKeys) {}
 
-/// @brief destroy the key generator
-KeyGenerator::~KeyGenerator() {}
-
 /// @brief get the generator type from VelocyPack
 KeyGenerator::GeneratorType KeyGenerator::generatorType(
     VPackSlice const& parameters) {
@@ -143,6 +138,20 @@ KeyGenerator::GeneratorType KeyGenerator::generatorType(
   return KeyGenerator::TYPE_UNKNOWN;
 }
 
+bool KeyGenerator::canUseType(VPackSlice const& parameters) {
+  auto type = generatorType(parameters);
+  if (type == KeyGenerator::TYPE_UNKNOWN) {
+    return false;
+  }
+
+  if (ServerState::instance()->isCoordinator()) {
+    // cluster only supports key generator type "traditional"
+    return type == KeyGenerator::TYPE_TRADITIONAL;
+  }
+  // single-server supports all types
+  return true;
+}
+
 /// @brief create a key generator based on the options specified
 KeyGenerator* KeyGenerator::factory(VPackSlice const& options) {
   KeyGenerator::GeneratorType type;
@@ -168,10 +177,11 @@ KeyGenerator* KeyGenerator::factory(VPackSlice const& options) {
   }
 
   if (type == TYPE_TRADITIONAL) {
-    return new TraditionalKeyGenerator(allowUserKeys);
+    if (ServerState::instance()->isCoordinator()) {
-  }
+      return new TraditionalKeyGeneratorCluster(allowUserKeys);
-
+    }
-  else if (type == TYPE_AUTOINCREMENT) {
+    return new TraditionalKeyGeneratorSingle(allowUserKeys);
+  } else if (type == TYPE_AUTOINCREMENT) {
     uint64_t offset = 0;
     uint64_t increment = 1;
 
@@ -224,38 +234,21 @@ int KeyGenerator::globalCheck(char const* p, size_t length, bool isRestore) {
     return TRI_ERROR_ARANGO_DOCUMENT_KEY_UNEXPECTED;
   }
 
-  if (length == 0) {
+  if (length == 0 || length > maxKeyLength) {
-    // user key is empty
+    // user key is empty or user key is too long
-    return TRI_ERROR_ARANGO_DOCUMENT_KEY_BAD;
-  }
-
-  if (length > TRI_VOC_KEY_MAX_LENGTH) {
-    // user key is too long
     return TRI_ERROR_ARANGO_DOCUMENT_KEY_BAD;
   }
 
   return TRI_ERROR_NO_ERROR;
 }
 
-/// @brief return a VelocyPack representation of the generator
-///        Not virtual because this is identical for all of them
-std::shared_ptr<VPackBuilder> KeyGenerator::toVelocyPack() const {
-  auto builder = std::make_shared<VPackBuilder>();
-  toVelocyPack(*builder);
-  builder->close();
-  return builder;
-}
-
 /// @brief create the key generator
 TraditionalKeyGenerator::TraditionalKeyGenerator(bool allowUserKeys)
-    : KeyGenerator(allowUserKeys), _lastValue(0) {}
+    : KeyGenerator(allowUserKeys) {}
-
-/// @brief destroy the key generator
-TraditionalKeyGenerator::~TraditionalKeyGenerator() {}
 
 /// @brief validate a key
 bool TraditionalKeyGenerator::validateKey(char const* key, size_t len) {
-  if (len == 0 || len > TRI_VOC_KEY_MAX_LENGTH) {
+  if (len == 0 || len > maxKeyLength) {
     return false;
   }
 
@@ -270,8 +263,40 @@ bool TraditionalKeyGenerator::validateKey(char const* key, size_t len) {
   return true;
 }
 
+/// @brief validate a key
+int TraditionalKeyGenerator::validate(char const* p, size_t length, bool isRestore) {
+  int res = globalCheck(p, length, isRestore);
+
+  if (res != TRI_ERROR_NO_ERROR) {
+    return res;
+  }
+
+  // validate user-supplied key
+  if (!validateKey(p, length)) {
+    return TRI_ERROR_ARANGO_DOCUMENT_KEY_BAD;
+  }
+
+  return TRI_ERROR_NO_ERROR;
+}
+
+/// @brief track usage of a key - default implementation is to throw!
+void TraditionalKeyGenerator::track(char const*, size_t) {
+  THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_INTERNAL, "this key generator cannot track keys");
+}
+
+/// @brief create a VPack representation of the generator
+void TraditionalKeyGenerator::toVelocyPack(VPackBuilder& builder) const {
+  TRI_ASSERT(!builder.isClosed());
+  builder.add("type", VPackValue(name()));
+  builder.add("allowUserKeys", VPackValue(_allowUserKeys));
+}
+
+/// @brief create the key generator
+TraditionalKeyGeneratorSingle::TraditionalKeyGeneratorSingle(bool allowUserKeys)
+    : TraditionalKeyGenerator(allowUserKeys), _lastValue(0) {}
+
 /// @brief generate a key
-std::string TraditionalKeyGenerator::generate() {
+std::string TraditionalKeyGeneratorSingle::generate() {
   TRI_voc_tick_t tick = TRI_NewTickServer();
   
   {
@@ -286,24 +311,19 @@ std::string TraditionalKeyGenerator::generate() {
 
   if (tick == UINT64_MAX) {
     // sanity check
-    return "";
+    return std::string();
   }
   return arangodb::basics::StringUtils::itoa(tick);
 }
 
 /// @brief validate a key
-int TraditionalKeyGenerator::validate(char const* p, size_t length, bool isRestore) {
+int TraditionalKeyGeneratorSingle::validate(char const* p, size_t length, bool isRestore) {
-  int res = globalCheck(p, length, isRestore);
+  int res = TraditionalKeyGenerator::validate(p, length, isRestore);
 
   if (res != TRI_ERROR_NO_ERROR) {
     return res;
   }
 
-  // validate user-supplied key
-  if (!validateKey(p, length)) {
-    return TRI_ERROR_ARANGO_DOCUMENT_KEY_BAD;
-  }
-  
   if (length > 0 && p[0] >= '0' && p[0] <= '9') {
     // potentially numeric key
     uint64_t value = NumberUtils::atoi_zero<uint64_t>(p, p + length);
@@ -322,7 +342,7 @@ int TraditionalKeyGenerator::validate(char const* p, size_t length, bool isResto
 }
 
 /// @brief track usage of a key
-void TraditionalKeyGenerator::track(char const* p, size_t length) {
+void TraditionalKeyGeneratorSingle::track(char const* p, size_t length) {
   // check the numeric key part
   if (length > 0 && p[0] >= '0' && p[0] <= '9') {
     // potentially numeric key
@@ -340,13 +360,23 @@ void TraditionalKeyGenerator::track(char const* p, size_t length) {
 }
 
 /// @brief create a VPack representation of the generator
-void TraditionalKeyGenerator::toVelocyPack(VPackBuilder& builder) const {
+void TraditionalKeyGeneratorSingle::toVelocyPack(VPackBuilder& builder) const {
   TRI_ASSERT(!builder.isClosed());
-  builder.add("type", VPackValue(name()));
+  TraditionalKeyGenerator::toVelocyPack(builder);
-  builder.add("allowUserKeys", VPackValue(_allowUserKeys));
   builder.add("lastValue", VPackValue(_lastValue));
 }
 
+/// @brief create the key generator
+TraditionalKeyGeneratorCluster::TraditionalKeyGeneratorCluster(bool allowUserKeys)
+    : TraditionalKeyGenerator(allowUserKeys) {}
+
+/// @brief generate a key
+std::string TraditionalKeyGeneratorCluster::generate() {
+  ClusterInfo* ci = ClusterInfo::instance();
+  uint64_t uid = ci->uniqid();
+  return std::to_string(uid);
+}
+
 /// @brief create the generator
 AutoIncrementKeyGenerator::AutoIncrementKeyGenerator(bool allowUserKeys,
                                                      uint64_t offset,
@@ -356,12 +386,9 @@ AutoIncrementKeyGenerator::AutoIncrementKeyGenerator(bool allowUserKeys,
       _offset(offset),
       _increment(increment) {}
 
-/// @brief destroy the generator
-AutoIncrementKeyGenerator::~AutoIncrementKeyGenerator() {}
-
 /// @brief validate a numeric key
 bool AutoIncrementKeyGenerator::validateKey(char const* key, size_t len) {
-  if (len == 0 || len > TRI_VOC_KEY_MAX_LENGTH) {
+  if (len == 0 || len > maxKeyLength) {
     return false;
   }
 
@@ -508,5 +535,5 @@ bool TRI_ValidateDocumentIdKeyGenerator(char const* key, size_t len,
   ++pos;
 
   // validate document key
-  return TraditionalKeyGenerator::validateKey(p, len - pos);
+  return TraditionalKeyGeneratorSingle::validateKey(p, len - pos);
 }

arangod/VocBase/KeyGenerator.h

@@ -26,13 +26,11 @@
 
 #include "Basics/Common.h"
 #include "Basics/Mutex.h"
+#include "Cluster/ServerState.h"
 #include "VocBase/vocbase.h"
 
 #include <array>
 
-/// @brief maximum length of a key in a collection
-#define TRI_VOC_KEY_MAX_LENGTH (254)
-
 namespace arangodb {
 namespace velocypack {
 class Builder;
@@ -48,18 +46,25 @@ class KeyGenerator {
     TYPE_AUTOINCREMENT = 2
   };
 
+  /// @brief maximum length of a key in a collection
+  static constexpr size_t maxKeyLength = 254;
+
  protected:
   /// @brief create the generator
-  explicit KeyGenerator(bool);
+  explicit KeyGenerator(bool allowUserKeys);
 
  public:
   /// @brief destroy the generator
-  virtual ~KeyGenerator();
+  virtual ~KeyGenerator() = default;
 
  public:
   /// @brief get the generator type from VelocyPack
   static GeneratorType generatorType(arangodb::velocypack::Slice const&);
 
+  /// @brief check if the specified key generator in the VelocyPack
+  /// can be used in this setup (cluster / single-server)
+  static bool canUseType(arangodb::velocypack::Slice const&);
+
   /// @brief create a key generator based on the options specified
   static KeyGenerator* factory(arangodb::velocypack::Slice const&);
 
@@ -75,9 +80,6 @@ class KeyGenerator {
   /// @brief track usage of a key
   virtual void track(char const* p, size_t length) = 0;
 
-  /// @brief return a VelocyPack representation of the generator
-  std::shared_ptr<arangodb::velocypack::Builder> toVelocyPack() const;
-
   /// @brief build a VelocyPack representation of the generator in the builder
   virtual void toVelocyPack(arangodb::velocypack::Builder&) const = 0;
 
@@ -89,18 +91,32 @@ class KeyGenerator {
   bool _allowUserKeys;
 };
 
-class TraditionalKeyGenerator final : public KeyGenerator {
+class TraditionalKeyGenerator : public KeyGenerator {
  public:
   /// @brief create the generator
-  explicit TraditionalKeyGenerator(bool);
+  explicit TraditionalKeyGenerator(bool allowUserKeys);
 
-  /// @brief destroy the generator
-  ~TraditionalKeyGenerator();
-
- public:
   /// @brief validate a key
   static bool validateKey(char const* key, size_t len);
 
+  /// @brief validate a key
+  int validate(char const* p, size_t length, bool isRestore) override;
+
+  /// @brief return the generator name (must be lowercase)
+  static char const* name() { return "traditional"; }
+  
+  /// @brief track usage of a key - default implementation is to throw!
+  void track(char const* p, size_t length) override;
+  
+  /// @brief build a VelocyPack representation of the generator in the builder
+  virtual void toVelocyPack(arangodb::velocypack::Builder&) const override;
+};
+
+class TraditionalKeyGeneratorSingle final : public TraditionalKeyGenerator {
+ public:
+  /// @brief create the generator
+  explicit TraditionalKeyGeneratorSingle(bool allowUserKeys);
+
  public:
   
   bool trackKeys() const override { return true; }
@@ -112,10 +128,7 @@ class TraditionalKeyGenerator final : public KeyGenerator {
   int validate(char const* p, size_t length, bool isRestore) override;
 
   /// @brief track usage of a key
-  void track(char const* p, size_t length) override final;
+  void track(char const* p, size_t length) override;
-
-  /// @brief return the generator name (must be lowercase)
-  static std::string name() { return "traditional"; }
 
   /// @brief build a VelocyPack representation of the generator in the builder
   virtual void toVelocyPack(arangodb::velocypack::Builder&) const override;
@@ -126,14 +139,24 @@ class TraditionalKeyGenerator final : public KeyGenerator {
   uint64_t _lastValue;
 };
 
+class TraditionalKeyGeneratorCluster final : public TraditionalKeyGenerator {
+ public:
+  /// @brief create the generator
+  explicit TraditionalKeyGeneratorCluster(bool allowUserKeys);
+
+ public:
+  
+  bool trackKeys() const override { return false; }
+
+  /// @brief generate a key
+  std::string generate() override;
+};
+
 class AutoIncrementKeyGenerator final : public KeyGenerator {
  public:
   /// @brief create the generator
   AutoIncrementKeyGenerator(bool, uint64_t, uint64_t);
 
-  /// @brief destroy the generator
-  ~AutoIncrementKeyGenerator();
-
  public:
   /// @brief validate a key
   static bool validateKey(char const* key, size_t len);
@@ -149,10 +172,10 @@ class AutoIncrementKeyGenerator final : public KeyGenerator {
   int validate(char const* p, size_t length, bool isRestore) override;
 
   /// @brief track usage of a key
-  void track(char const* p, size_t length) override final;
+  void track(char const* p, size_t length) override;
 
   /// @brief return the generator name (must be lowercase)
-  static std::string name() { return "autoincrement"; }
+  static char const* name() { return "autoincrement"; }
 
   /// @brief build a VelocyPack representation of the generator in the builder
   virtual void toVelocyPack(arangodb::velocypack::Builder&) const override;

arangod/VocBase/LogicalCollection.cpp

@@ -251,17 +251,9 @@ LogicalCollection::LogicalCollection(
     }
 
     VPackSlice keyGenSlice = info.get("keyOptions");
-    if (keyGenSlice.isObject()) {
+    if (!KeyGenerator::canUseType(keyGenSlice)) {
-      keyGenSlice = keyGenSlice.get("type");
+      THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_CLUSTER_UNSUPPORTED,
-      if (keyGenSlice.isString()) {
+                                     "the specified key generator is not supported for sharded collections");
-        StringRef tmp(keyGenSlice);
-        if (!tmp.empty() && tmp != "traditional") {
-          THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_CLUSTER_UNSUPPORTED,
-                                         "non-traditional key generators are "
-                                         "not supported for sharded "
-                                         "collections");
-        }
-      }
     }
   }