Use fmt::format to compute keys in variables::collection::Collection

- Refactored code to avoid code duplication - Use std::string_view in variables::collection::Collection - compartment & compartment2 arguments were being passed by value and creating an unnecessary std::string (heap-allocation & copy) - Replace const std::string& with std::string_view to relax client requirements (do not need to create a std::string instance)
eduar-hte · Aug 28, 2024 · b24aa8d · b24aa8d
1 parent 89cd94c
commit b24aa8d
Show file tree

Hide file tree

Showing 6 changed files with 93 additions and 107 deletions.
diff --git a/headers/modsecurity/collection/collection.h b/headers/modsecurity/collection/collection.h
@@ -16,11 +16,10 @@
 
 #ifdef __cplusplus
 #include <string>
-#include <unordered_map>
-#include <list>
 #include <vector>
 #include <algorithm>
-#include <memory>
+#include <string_view>
+#include <fmt/format.h>
 #endif
 
 
@@ -43,14 +42,14 @@ namespace collection {
 
 class Collection {
  public:
-    explicit Collection(const std::string &a) : m_name(a) { }
+    explicit Collection(std::string_view a) : m_name(a) { }
     virtual ~Collection() { }
 
-    virtual bool storeOrUpdateFirst(const std::string &key,
-        const std::string &value) = 0;
+    virtual bool storeOrUpdateFirst(const std::string& key,
+        std::string_view value) = 0;
 
-    virtual bool updateFirst(const std::string &key,
-        const std::string &value) = 0;
+    virtual bool updateFirst(const std::string& key,
+        std::string_view value) = 0;
 
     virtual void del(const std::string& key) = 0;
 
@@ -70,131 +69,124 @@ class Collection {
 
 
     /* storeOrUpdateFirst */
-    virtual bool storeOrUpdateFirst(const std::string &key,
-        std::string compartment, const std::string &value) {
-        std::string nkey = compartment + "::" + key;
-        return storeOrUpdateFirst(nkey, value);
+    bool storeOrUpdateFirst(std::string_view key,
+        std::string_view compartment, std::string_view value) {
+        return storeOrUpdateFirst(nkey(compartment, key), value);
     }
 
 
-    virtual bool storeOrUpdateFirst(const std::string &key,
-        std::string compartment, std::string compartment2,
-        const std::string &value) {
-        std::string nkey = compartment + "::" + compartment2 + "::" + key;
-        return storeOrUpdateFirst(nkey, value);
+    bool storeOrUpdateFirst(std::string_view key,
+        std::string_view compartment, std::string_view compartment2,
+        std::string_view value) {
+        return storeOrUpdateFirst(nkey(compartment, compartment2, key), value);
     }
 
 
     /* updateFirst */
-    virtual bool updateFirst(const std::string &key, std::string compartment,
-        const std::string &value) {
-        std::string nkey = compartment + "::" + key;
-        return updateFirst(nkey, value);
+    bool updateFirst(std::string_view key, std::string_view compartment,
+        std::string_view value) {
+        return updateFirst(nkey(compartment, key), value);
     }
 
 
-    virtual bool updateFirst(const std::string &key, std::string compartment,
-        std::string compartment2, const std::string &value) {
-        std::string nkey = compartment + "::" + compartment2 + "::" + key;
-        return updateFirst(nkey, value);
+    bool updateFirst(std::string_view key, std::string_view compartment,
+        std::string_view compartment2, std::string_view value) {
+        return updateFirst(nkey(compartment, compartment2, key), value);
     }
 
 
     /* del */
-    virtual void del(const std::string& key, std::string compartment) {
-        std::string nkey = compartment + "::" + key;
-        del(nkey);
+    void del(std::string_view key, std::string_view compartment) {
+        del(nkey(compartment, key));
     }
 
 
-    virtual void del(const std::string& key, std::string compartment,
-        std::string compartment2) {
-        std::string nkey = compartment + "::" + compartment2 + "::" + key;
-        del(nkey);
+    void del(std::string_view key, std::string_view compartment,
+        std::string_view compartment2) {
+        del(nkey(compartment, compartment2, key));
     }
 
 
     /* setExpiry */
-    virtual void setExpiry(const std::string& key, std::string compartment,
+    void setExpiry(std::string_view key, std::string_view compartment,
         int32_t expiry_seconds) {
-        std::string nkey = compartment + "::" + key;
-        setExpiry(nkey, expiry_seconds);
+        setExpiry(nkey(compartment, key), expiry_seconds);
     }
 
 
-    virtual void setExpiry(const std::string& key, std::string compartment,
-        std::string compartment2, int32_t expiry_seconds) {
-        std::string nkey = compartment + "::" + compartment2 + "::" + key;
-        setExpiry(nkey, expiry_seconds);
+    void setExpiry(std::string_view key, std::string_view compartment,
+        std::string_view compartment2, int32_t expiry_seconds) {
+        setExpiry(nkey(compartment, compartment2, key), expiry_seconds);
     }
 
 
     /* resolveFirst */
-    virtual std::unique_ptr<std::string> resolveFirst(const std::string& var,
-        std::string compartment) {
-        std::string nkey = compartment + "::" + var;
-        return resolveFirst(nkey);
+    std::unique_ptr<std::string> resolveFirst(std::string_view var,
+        std::string_view compartment) {
+        return resolveFirst(nkey(compartment, var));
     }
 
 
-    virtual std::unique_ptr<std::string> resolveFirst(const std::string& var,
-        std::string compartment, std::string compartment2) {
-        std::string nkey = compartment + "::" + compartment2 + "::" + var;
-        return resolveFirst(nkey);
+    std::unique_ptr<std::string> resolveFirst(std::string_view var,
+        std::string_view compartment, std::string_view compartment2) {
+        return resolveFirst(nkey(compartment, compartment2, var));
     }
 
 
     /* resolveSingleMatch */
-    virtual void resolveSingleMatch(const std::string& var,
-        std::string compartment, std::vector<const VariableValue *> *l) {
-        std::string nkey = compartment + "::" + var;
-        resolveSingleMatch(nkey, l);
+    void resolveSingleMatch(std::string_view var,
+        std::string_view compartment, std::vector<const VariableValue *> *l) {
+        resolveSingleMatch(nkey(compartment, var), l);
     }
 
 
-    virtual void resolveSingleMatch(const std::string& var,
-        std::string compartment, std::string compartment2,
+    void resolveSingleMatch(std::string_view var,
+        std::string_view compartment, std::string_view compartment2,
         std::vector<const VariableValue *> *l) {
-        std::string nkey = compartment + "::" + compartment2 + "::" + var;
-        resolveSingleMatch(nkey, l);
+        resolveSingleMatch(nkey(compartment, compartment2, var), l);
     }
 
 
     /* resolveMultiMatches */
-    virtual void resolveMultiMatches(const std::string& var,
-        std::string compartment, std::vector<const VariableValue *> *l,
+    void resolveMultiMatches(std::string_view var,
+        std::string_view compartment, std::vector<const VariableValue *> *l,
         variables::KeyExclusions &ke) {
-        std::string nkey = compartment + "::" + var;
-        resolveMultiMatches(nkey, l, ke);
+        resolveMultiMatches(nkey(compartment, var), l, ke);
     }
 
 
-    virtual void resolveMultiMatches(const std::string& var,
-        std::string compartment, std::string compartment2,
+    void resolveMultiMatches(std::string_view var,
+        std::string_view compartment, std::string_view compartment2,
         std::vector<const VariableValue *> *l,
         variables::KeyExclusions &ke) {
-        std::string nkey = compartment + "::" + compartment2 + "::" + var;
-        resolveMultiMatches(nkey, l, ke);
+        resolveMultiMatches(nkey(compartment, compartment2, var), l, ke);
     }
 
 
     /* resolveRegularExpression */
-    virtual void resolveRegularExpression(const std::string& var,
-        std::string compartment, std::vector<const VariableValue *> *l,
+    void resolveRegularExpression(std::string_view var,
+        std::string_view compartment, std::vector<const VariableValue *> *l,
         variables::KeyExclusions &ke) {
-        std::string nkey = compartment + "::" + var;
-        resolveRegularExpression(nkey, l, ke);
+        resolveRegularExpression(nkey(compartment, var), l, ke);
     }
 
 
-    virtual void resolveRegularExpression(const std::string& var,
-        std::string compartment, std::string compartment2,
+    void resolveRegularExpression(std::string_view var,
+        std::string_view compartment, std::string_view compartment2,
         std::vector<const VariableValue *> *l, variables::KeyExclusions &ke) {
-        std::string nkey = compartment + "::" + compartment2 + "::" + var;
-        resolveRegularExpression(nkey, l, ke);
+        resolveRegularExpression(nkey(compartment, compartment2, var), l, ke);
     }
 
     std::string m_name;
+
+ protected:
+
+    static inline std::string nkey(std::string_view compartment, std::string_view key) {
+        return fmt::format("{}::{}", compartment, key);
+    }
+    static inline std::string nkey(std::string_view compartment, std::string_view compartment2, std::string_view key) {
+        return fmt::format("{}::{}::{}", compartment, compartment2, key);
+    }
 };
 
 }  // namespace collection

diff --git a/src/collection/backend/collection_data.h b/src/collection/backend/collection_data.h
@@ -16,6 +16,7 @@
 
 #ifdef __cplusplus
 #include <string>
+#include <string_view>
 #include <chrono>
 #endif
 
@@ -24,22 +25,20 @@
 #define SRC_COLLECTION_DATA_H_
 
 #ifdef __cplusplus
-namespace modsecurity {
-namespace collection {
-namespace backend {
+namespace modsecurity::collection::backend {
 
 class CollectionData {
 public:
     CollectionData() :
         m_hasValue(false),
 	m_hasExpiryTime(false) { }
 
-    CollectionData(const std::string &value) :
+    CollectionData(std::string_view value) :
         m_hasValue(true),
 	m_hasExpiryTime(false),
 	m_value(value) { }
 
-    void setValue(const std::string &value) {
+    void setValue(std::string_view value) {
         m_value = value;
         m_hasValue = true;
     }
@@ -60,9 +59,7 @@ class CollectionData {
     std::chrono::system_clock::time_point m_expiryTime;
 };
 
-}  // namespace backend
-}  // namespace collection
-}  // namespace modsecurity
+}  // namespace modsecurity::collection::backend
 #endif
 
 #endif  // SRC_COLLECTION_DATA_H_
diff --git a/src/collection/backend/in_memory-per_process.cc b/src/collection/backend/in_memory-per_process.cc
@@ -35,7 +35,7 @@ namespace collection {
 namespace backend {
 
 
-InMemoryPerProcess::InMemoryPerProcess(const std::string &name) :
+InMemoryPerProcess::InMemoryPerProcess(std::string_view name) :
     Collection(name) {
     m_map.reserve(1000);
 }
@@ -46,7 +46,7 @@ InMemoryPerProcess::~InMemoryPerProcess() {
 
 
 template<typename Map>
-inline void __store(Map &map, std::string key, std::string value) {
+inline void __store(Map &map, const std::string& key, std::string_view value) {
     // NOTE: should be called with write-lock previously acquired
 
     map.emplace(key, value);
@@ -55,8 +55,8 @@ inline void __store(Map &map, std::string key, std::string value) {
 
 template<typename Map>
 inline bool __updateFirst(Map &map,
-    const std::string &key,
-    const std::string &value) {
+    const std::string& key,
+    std::string_view value) {
     // NOTE: should be called with write-lock previously acquired
 
     if (auto search = map.find(key); search != map.end()) {
@@ -68,14 +68,14 @@ inline bool __updateFirst(Map &map,
 }
 
 
-void InMemoryPerProcess::store(const std::string &key, const std::string &value) {
+void InMemoryPerProcess::store(const std::string& key, std::string_view value) {
     const std::lock_guard lock(m_mutex); // write lock (exclusive access)
     __store(m_map, key, value);
 }
 
 
-bool InMemoryPerProcess::storeOrUpdateFirst(const std::string &key,
-    const std::string &value) {
+bool InMemoryPerProcess::storeOrUpdateFirst(const std::string& key,
+    std::string_view value) {
     const std::lock_guard lock(m_mutex); // write lock (exclusive access)
     if (__updateFirst(m_map, key, value) == false) {
         __store(m_map, key, value);
@@ -84,8 +84,8 @@ bool InMemoryPerProcess::storeOrUpdateFirst(const std::string &key,
 }
 
 
-bool InMemoryPerProcess::updateFirst(const std::string &key,
-    const std::string &value) {
+bool InMemoryPerProcess::updateFirst(const std::string& key,
+    std::string_view value) {
     const std::lock_guard lock(m_mutex); // write lock (exclusive access)
     return __updateFirst(m_map, key, value);
 }

diff --git a/src/collection/backend/in_memory-per_process.h b/src/collection/backend/in_memory-per_process.h
@@ -73,15 +73,15 @@ struct MyHash{
 class InMemoryPerProcess :
     public Collection {
  public:
-    explicit InMemoryPerProcess(const std::string &name);
+    explicit InMemoryPerProcess(std::string_view name);
     ~InMemoryPerProcess() override;
-    void store(const std::string &key, const std::string &value);
+    void store(const std::string& key, std::string_view value);
 
-    bool storeOrUpdateFirst(const std::string &key,
-        const std::string &value) override;
+    bool storeOrUpdateFirst(const std::string& key,
+        std::string_view value) override;
 
-    bool updateFirst(const std::string &key,
-        const std::string &value) override;
+    bool updateFirst(const std::string& key,
+        std::string_view value) override;
 
     void del(const std::string& key) override;
 
@@ -101,17 +101,14 @@ class InMemoryPerProcess :
         variables::KeyExclusions &ke) override;
 
     /* store */
-    virtual void store(const std::string &key, std::string &compartment,
-        std::string value) {
-        const auto nkey = compartment + "::" + key;
-        store(nkey, value);
+    void store(std::string_view key, std::string_view compartment,
+        std::string_view value) {
+        store(nkey(compartment, key), value);
     }
 
-
-    virtual void store(const std::string &key, const std::string &compartment,
-        std::string compartment2, std::string value) {
-        const auto nkey = compartment + "::" + compartment2 + "::" + key;
-        store(nkey, value);
+    void store(std::string_view key, std::string_view compartment,
+        std::string_view compartment2, std::string_view value) {
+        store(nkey(compartment, compartment2, key), value);
     }
 
  private:

diff --git a/src/collection/backend/lmdb.cc b/src/collection/backend/lmdb.cc
@@ -296,7 +296,7 @@ void LMDB::delIfExpired(const std::string& key) {
 }
 
 bool LMDB::storeOrUpdateFirst(const std::string &key,
-    const std::string &value) {
+    std::string_view value) {
     int rc;
     MDB_txn *txn;
     MDB_val mdb_key;
@@ -400,7 +400,7 @@ void LMDB::resolveSingleMatch(const std::string& var,
 
 
 bool LMDB::updateFirst(const std::string &key,
-    const std::string &value) {
+    std::string_view value) {
     int rc;
     MDB_txn *txn;
     MDB_val mdb_key;