Make it harder to mistake deletions for legitimate writes in commit hooks

src/kv/experimental.h

@@ -278,22 +278,18 @@ namespace kv
       {
         return [hook](Version v, const UntypedMap::Write& w) {
           Write typed_w;
-          for (const auto& [uk, version_uv] : w)
+          for (const auto& [uk, opt_uv] : w)
           {
-            if (version_uv.version == NoVersion)
+            if (!opt_uv.has_value())
             {
-              // Deletions are indicated by {NoVersion, {}}. The second
-              // element in the serialised representation is an empty vector,
-              // which may not be safely serialisable! So we duplicate the old
-              // behaviour and use default-constructed V
-              typed_w[KSerialiser::from_serialised(uk)] =
-                VersionV{NoVersion, V{}};
+              // Deletions are indicated by nullopt. We cannot deserialise them,
+              // they are deletions here as well
+              typed_w[KSerialiser::from_serialised(uk)] = std::nullopt;
             }
             else
             {
               typed_w[KSerialiser::from_serialised(uk)] =
-                VersionV{version_uv.version,
-                         VSerialiser::from_serialised(version_uv.value)};
+                VSerialiser::from_serialised(opt_uv.value());
             }
           }
 

src/kv/map.h

@@ -157,11 +157,11 @@ namespace kv
         for (auto it = change_set.writes.begin(); it != change_set.writes.end();
              ++it)
         {
-          if (it->second.version >= 0)
+          if (it->second.has_value())
           {
             // Write the new value with the global version.
             changes = true;
-            state = state.put(it->first, VersionV{v, it->second.value});
+            state = state.put(it->first, VersionV{v, it->second.value()});
           }
           else
           {
@@ -364,7 +364,7 @@ namespace kv
       for (auto it = change_set.writes.begin(); it != change_set.writes.end();
            ++it)
       {
-        if (!is_deleted(it->second.version))
+        if (it->second.has_value())
         {
           ++write_ctr;
         }
@@ -382,17 +382,17 @@ namespace kv
       for (auto it = change_set.writes.begin(); it != change_set.writes.end();
            ++it)
       {
-        if (!is_deleted(it->second.version))
+        if (it->second.has_value())
         {
-          s.serialise_write(it->first, it->second.value);
+          s.serialise_write(it->first, it->second.value());
         }
       }
 
       s.serialise_count_header(remove_ctr);
       for (auto it = change_set.writes.begin(); it != change_set.writes.end();
            ++it)
       {
-        if (is_deleted(it->second.version))
+        if (!it->second.has_value())
         {
           s.serialise_remove(it->first);
         }
@@ -427,14 +427,14 @@ namespace kv
       for (size_t i = 0; i < ctr; ++i)
       {
         auto w = d.template deserialise_write<K, V>();
-        change_set.writes[std::get<0>(w)] = {0, std::get<1>(w)};
+        change_set.writes[std::get<0>(w)] = std::get<1>(w);
       }
 
       ctr = d.deserialise_remove_header();
       for (size_t i = 0; i < ctr; ++i)
       {
         auto r = d.template deserialise_remove<K>();
-        change_set.writes[r] = {NoVersion, V()};
+        change_set.writes[r] = std::nullopt;
       }
 
       return view;

src/kv/test/kv_test.cpp

@@ -423,7 +423,8 @@ TEST_CASE_TEMPLATE(
     REQUIRE(global_writes.size() == 0);
     REQUIRE(local_writes.size() == 1);
     const auto& latest_writes = local_writes.front();
-    REQUIRE(latest_writes.at("key1").value == "value1");
+    REQUIRE(latest_writes.at("key1").has_value());
+    REQUIRE(latest_writes.at("key1").value() == "value1");
     INFO("Local removals are not seen");
     REQUIRE(latest_writes.find("key2") == latest_writes.end());
     REQUIRE(latest_writes.size() == 1);
@@ -462,8 +463,13 @@ TEST_CASE_TEMPLATE(
     INFO("Old writes are not included");
     REQUIRE(latest_writes.find("key1") == latest_writes.end());
     INFO("Visible removals are included");
-    REQUIRE(latest_writes.at("key2").version == kv::NoVersion);
-    REQUIRE(latest_writes.at("key3").value == "value3");
+    const auto it2 = latest_writes.find("key2");
+    REQUIRE(it2 != latest_writes.end());
+    REQUIRE(!it2->second.has_value());
+    const auto it3 = latest_writes.find("key3");
+    REQUIRE(it3 != latest_writes.end());
+    REQUIRE(it3->second.has_value());
+    REQUIRE(it3->second.value() == "value3");
     REQUIRE(latest_writes.size() == 2);
 
     local_writes.clear();
@@ -511,8 +517,12 @@ TEST_CASE_TEMPLATE(
     kv_store.compact(1);
 
     REQUIRE(global_writes.size() == 1);
-    REQUIRE(global_writes.at(0).version == 1);
-    REQUIRE(global_writes.at(0).writes.at("key1").value == "value1");
+    const auto& latest_writes = global_writes.front();
+    REQUIRE(latest_writes.version == 1);
+    const auto it1 = latest_writes.writes.find("key1");
+    REQUIRE(it1 != latest_writes.writes.end());
+    REQUIRE(it1->second.has_value());
+    REQUIRE(it1->second.value() == "value1");
 
     global_writes.clear();
     kv_store.clear();
@@ -543,9 +553,14 @@ TEST_CASE_TEMPLATE(
     // hook
     REQUIRE(global_writes.size() == 2);
     REQUIRE(global_writes.at(0).version == 1);
-    REQUIRE(global_writes.at(0).writes.at("key1").value == "value1");
-    REQUIRE(global_writes.at(1).version == 2);
-    REQUIRE(global_writes.at(1).writes.at("key2").value == "value2");
+    const auto it1 = global_writes.at(0).writes.find("key1");
+    REQUIRE(it1 != global_writes.at(0).writes.end());
+    REQUIRE(it1->second.has_value());
+    REQUIRE(it1->second.value() == "value1");
+    const auto it2 = global_writes.at(1).writes.find("key2");
+    REQUIRE(it2 != global_writes.at(1).writes.end());
+    REQUIRE(it2->second.has_value());
+    REQUIRE(it2->second.value() == "value2");
 
     global_writes.clear();
     kv_store.clear();
@@ -576,7 +591,10 @@ TEST_CASE_TEMPLATE(
     // hook
     REQUIRE(global_writes.size() == 1);
     REQUIRE(global_writes.at(0).version == 1);
-    REQUIRE(global_writes.at(0).writes.at("key1").value == "value1");
+    const auto it1 = global_writes.at(0).writes.find("key1");
+    REQUIRE(it1 != global_writes.at(0).writes.end());
+    REQUIRE(it1->second.has_value());
+    REQUIRE(it1->second.value() == "value1");
 
     global_writes.clear();
     kv_store.clear();
@@ -601,7 +619,10 @@ TEST_CASE_TEMPLATE(
     // Only writes since the last compact are passed to the global hook
     REQUIRE(global_writes.size() == 1);
     REQUIRE(global_writes.at(0).version == 2);
-    REQUIRE(global_writes.at(0).writes.at("key2").value == "value2");
+    const auto it2 = global_writes.at(0).writes.find("key2");
+    REQUIRE(it2 != global_writes.at(0).writes.end());
+    REQUIRE(it2->second.has_value());
+    REQUIRE(it2->second.value() == "value2");
 
     global_writes.clear();
     kv_store.clear();

src/kv/tx_view.h

@@ -31,7 +31,8 @@ namespace kv
   using Read = std::unordered_map<K, Version, H>;
 
   template <typename K, typename V, typename H = std::hash<K>>
-  using Write = std::unordered_map<K, VersionV<V>, H>;
+  using Write =
+    std::unordered_map<K, std::optional<V>, H>; //< nullopt indicates a deletion
 
   // This is a container for a write-set + dependencies. It can be applied to a
   // given state, or used to track a set of operations on a state
@@ -96,13 +97,9 @@ namespace kv
       auto write = tx_changes.writes.find(key);
       if (write != tx_changes.writes.end())
       {
-        // Return empty for a key that has been removed.
-        if (is_deleted(write->second.version))
-        {
-          return std::nullopt;
-        }
-
-        return write->second.value;
+        // May be empty, for a key that has been removed. This matches the
+        // return semantics
+        return write->second;
       }
 
       // If the key doesn't exist, return empty and record that we depend on
@@ -174,7 +171,7 @@ namespace kv
     bool put(const K& key, const V& value)
     {
       // Record in the write set.
-      tx_changes.writes[key] = {0, value};
+      tx_changes.writes[key] = value;
       return true;
     }
 
@@ -203,7 +200,7 @@ namespace kv
         else
         {
           // If we have written, change the write set to indicate a remove.
-          write->second = {NoVersion, V()};
+          write->second = std::nullopt;
         }
 
         return true;
@@ -219,7 +216,7 @@ namespace kv
       tx_changes.writes.emplace(
         std::piecewise_construct,
         std::forward_as_tuple(key),
-        std::forward_as_tuple(NoVersion, V()));
+        std::forward_as_tuple(std::nullopt));
       return true;
     }
 
@@ -247,8 +244,8 @@ namespace kv
            write != tx_changes.writes.end();
            ++write)
       {
-        if (!is_deleted(write->second.version))
-          if (!f(write->first, write->second.value))
+        if (write->second.has_value())
+          if (!f(write->first, write->second.value()))
             return false;
       }
       return true;