Do not remove protected keys in DefaultCache. (#1193)

When user calls DefaultCache::Remove() and
DefaultCache::RemoveKeysWithPrefix() we should not remove protected
keys. This is now implemented and tested.

Relates-To: OLPEDGE-2521

Signed-off-by: Andrei Popescu <[email protected]>

Author: Andrei Popescu

olp-cpp-sdk-core/include/olp/core/cache/DefaultCache.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2019-2020 HERE Europe B.V.
+ * Copyright (C) 2019-2021 HERE Europe B.V.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -277,4 +277,4 @@ class CORE_API DefaultCache : public KeyValueCache {
 }  // namespace cache
 }  // namespace olp
 
-#endif // OLP_SDK_ENABLE_DEFAULT_CACHE
+#endif  // OLP_SDK_ENABLE_DEFAULT_CACHE

olp-cpp-sdk-core/src/cache/DefaultCacheImpl.cpp

@@ -346,15 +346,25 @@ KeyValueCache::ValueTypePtr DefaultCacheImpl::Get(const std::string& key) {
 
     return value;
   }
-
   return nullptr;
 }
 
 bool DefaultCacheImpl::Remove(const std::string& key) {
   std::lock_guard<std::mutex> lock(cache_lock_);
+
   if (!is_open_) {
     return false;
   }
+
+  // In case the key is protected do not remove it
+  if (protected_keys_.IsProtected(key)) {
+    OLP_SDK_LOG_INFO_F(kLogTag,
+                       "Remove() called on a protected key, ignoring, key='%s'",
+                       key.c_str());
+
+    return false;
+  }
+
   // protected data could be removed by user
   if (memory_cache_) {
     memory_cache_->Remove(key);
@@ -374,19 +384,27 @@ bool DefaultCacheImpl::Remove(const std::string& key) {
 
 bool DefaultCacheImpl::RemoveKeysWithPrefix(const std::string& key) {
   std::lock_guard<std::mutex> lock(cache_lock_);
+
   if (!is_open_) {
     return false;
   }
 
+  auto filter = [&](const std::string& cache_key) {
+    return protected_keys_.IsProtected(cache_key);
+  };
+
   if (memory_cache_) {
-    memory_cache_->RemoveKeysWithPrefix(key);
+    memory_cache_->RemoveKeysWithPrefix(key, filter);
   }
 
+  // No need to check here for protected key as these are not added to LRU from
+  // the start
   RemoveKeysWithPrefixLru(key);
 
   if (mutable_cache_) {
     uint64_t removed_data_size = 0;
-    auto result = mutable_cache_->RemoveKeysWithPrefix(key, removed_data_size);
+    auto result =
+        mutable_cache_->RemoveKeysWithPrefix(key, removed_data_size, filter);
     mutable_cache_data_size_ -= removed_data_size;
     return result;
   }
@@ -1037,5 +1055,6 @@ uint64_t DefaultCacheImpl::Size(uint64_t new_size) {
   mutable_cache_->Compact();
   return evicted;
 }
+
 }  // namespace cache
 }  // namespace olp

olp-cpp-sdk-core/src/cache/DiskCache.cpp

@@ -384,34 +384,46 @@ DiskCache::OperationOutcome DiskCache::ApplyBatch(
 }
 
 bool DiskCache::RemoveKeysWithPrefix(const std::string& prefix,
-                                     uint64_t& removed_data_size) {
+                                     uint64_t& removed_data_size,
+                                     const RemoveFilterFunc& filter) {
   uint64_t data_size = 0u;
-  if (!database_) {
-    removed_data_size = 0u;
-    OLP_SDK_LOG_ERROR(kLogTag,
-                      "RemoveKeysWithPrefix: Database is uninitialized");
-    return false;
-  }
-
-  auto batch = std::make_unique<leveldb::WriteBatch>();
+  removed_data_size = 0u;
 
+  // As we remove data probably it is not wise to flood the leveldb memory cache
   leveldb::ReadOptions opts;
   opts.verify_checksums = check_crc_;
-  opts.fill_cache = true;
+  opts.fill_cache = false;
   auto iterator = NewIterator(opts);
+  if (!iterator) {
+    OLP_SDK_LOG_WARNING(kLogTag,
+                        "RemoveKeysWithPrefix: Database is uninitialized");
+    return false;
+  }
 
-  if (prefix.empty()) {
-    for (iterator->SeekToFirst(); iterator->Valid(); iterator->Next()) {
-      batch->Delete(iterator->key());
-      data_size += iterator->value().size() + iterator->key().size();
-    }
+  auto batch = std::make_unique<leveldb::WriteBatch>();
+  auto prefix_slice = leveldb::Slice(prefix);
+  const bool prefix_empty = prefix_slice.empty();
+
+  if (prefix_empty) {
+    iterator->SeekToFirst();
   } else {
-    for (iterator->Seek(prefix);
-         iterator->Valid() && iterator->key().starts_with(prefix);
-         iterator->Next()) {
-      batch->Delete(iterator->key());
-      data_size += iterator->value().size() + iterator->key().size();
+    iterator->Seek(prefix_slice);
+  }
+
+  auto condition = [&](const leveldb::Iterator& it) {
+    return it.Valid() && (prefix_empty || it.key().starts_with(prefix_slice));
+  };
+
+  for (; condition(*iterator); iterator->Next()) {
+    auto key = iterator->key();
+
+    // Do not delete if protected
+    if (filter && filter(key.ToString())) {
+      continue;
     }
+
+    batch->Delete(key);
+    data_size += iterator->value().size() + key.size();
   }
 
   auto result = ApplyBatch(std::move(batch));

olp-cpp-sdk-core/src/cache/DiskCache.h

@@ -38,6 +38,7 @@
 #include <olp/core/cache/CacheSettings.h>
 #include <olp/core/cache/KeyValueCache.h>
 #include <olp/core/client/ApiError.h>
+#include <olp/core/client/ApiNoResult.h>
 #include <olp/core/client/ApiResponse.h>
 
 namespace leveldb {
@@ -91,11 +92,14 @@ class DiskCache {
   static constexpr uint64_t kSizeMax = std::numeric_limits<uint64_t>::max();
 
   /// No error type
-  struct NoError {};
+  using NoError = client::ApiNoResult;
 
   /// Operation result type
   using OperationOutcome = client::ApiResponse<NoError, client::ApiError>;
 
+  /// Will be used to filter out keys to be removed in case they are protected.
+  using RemoveFilterFunc = std::function<bool(const std::string&)>;
+
   /// Logger that forwards leveldb log messages to our logging framework.
   class LevelDBLogger : public leveldb::Logger {
     void Logv(const char* format, va_list ap) override;
@@ -140,7 +144,8 @@ class DiskCache {
 
   /// Empty prefix deleted everything from DB. Returns size of removed data.
   bool RemoveKeysWithPrefix(const std::string& prefix,
-                            uint64_t& removed_data_size);
+                            uint64_t& removed_data_size,
+                            const RemoveFilterFunc& filter = nullptr);
 
   /// Check if cache contains data with the key.
   bool Contains(const std::string& key);

olp-cpp-sdk-core/src/cache/InMemoryCache.cpp

@@ -93,10 +93,18 @@ bool InMemoryCache::Remove(const std::string& key) {
   return item_tuples_.Erase(key);
 }
 
-void InMemoryCache::RemoveKeysWithPrefix(const std::string& key_prefix) {
+void InMemoryCache::RemoveKeysWithPrefix(const std::string& key_prefix,
+                                         const RemoveFilterFunc& filter) {
   std::lock_guard<std::mutex> lock{mutex_};
+
   for (auto it = item_tuples_.begin(); it != item_tuples_.end();) {
     if (it->key().substr(0, key_prefix.length()) == key_prefix) {
+      // Check if this key is not protected, and if it is do not remove
+      if (filter && filter(it->key())) {
+        ++it;
+        continue;
+      }
+
       // we allow concurrent modifications.
       it = item_tuples_.Erase(it);
     } else {

olp-cpp-sdk-core/src/cache/InMemoryCache.h

@@ -47,6 +47,9 @@ class InMemoryCache {
   using TimeProvider = std::function<time_t()>;
   using ModelCacheCostFunc = std::function<std::size_t(const ItemTuple&)>;
 
+  /// Will be used to filter out keys to be removed in case they are protected.
+  using RemoveFilterFunc = std::function<bool(const std::string&)>;
+
   /// Default cache cost based on size.
   struct DefaultCacheCost {
     std::size_t operator()(const ItemTuple& value) const {
@@ -75,7 +78,8 @@ class InMemoryCache {
   void Clear();
 
   bool Remove(const std::string& key);
-  void RemoveKeysWithPrefix(const std::string& key_prefix);
+  void RemoveKeysWithPrefix(const std::string& key_prefix,
+                            const RemoveFilterFunc& filter = nullptr);
   bool Contains(const std::string& key) const;
 
  protected: