From f80982b52bcd4a6d6df5bba927ee03a8d47771c2 Mon Sep 17 00:00:00 2001
From: Ichthyostega <prg@ichthyostega.de>
Date: Sat, 13 Feb 2016 22:14:31 +0100
Subject: [PATCH] gen-node: fix insidious data conssitency problem

I assumed that, since GenNode is composed of copyable and
assignable types, the standard implementation will do.
But I overlooked the run time type check on the opaque
payload type within lib::Variant. When a type mismatch
is detected, the default implementation has already
assigned and thus altered the IDs.

So we need to roll our own implementation, and to add
insult to injury, we can't use the copy-and-swap idiom either.
---
 .../interact/state-map-grouping-storage.hpp   |  6 ++-
 src/lib/diff/gen-node.hpp                     | 40 ++++++++++++++++++-
 src/lib/variant.hpp                           |  2 +
 .../state-map-grouping-storage-test.cpp       | 37 ++++++++++++++---
 4 files changed, 77 insertions(+), 8 deletions(-)

diff --git a/src/gui/interact/state-map-grouping-storage.hpp b/src/gui/interact/state-map-grouping-storage.hpp
index c4d1eb3d7..138144ee3 100644
--- a/src/gui/interact/state-map-grouping-storage.hpp
+++ b/src/gui/interact/state-map-grouping-storage.hpp
@@ -39,6 +39,7 @@
 #include "lib/error.hpp"
 #include "lib/idi/entry-id.hpp"
 #include "lib/diff/gen-node.hpp"
+#include "lib/util.hpp"
 
 #include <boost/noncopyable.hpp>
 #include <unordered_map>
@@ -52,6 +53,7 @@ namespace interact {
   using lib::idi::BareEntryID;
   using lib::diff::GenNode;
   using lib::diff::Ref;
+  using util::unConst;
   using std::string;
   
   
@@ -117,7 +119,9 @@ namespace interact {
       void
       record (BareEntryID const& elementID, GenNode const& stateMark)
         {
-          elmTable_[elementID].emplace (stateMark);
+          auto res = elmTable_[elementID].emplace (stateMark);
+          if (not res.second)
+            unConst(*res.first) = stateMark; // update existing contents
         }
       
       
diff --git a/src/lib/diff/gen-node.hpp b/src/lib/diff/gen-node.hpp
index 7672f11d2..b883861ce 100644
--- a/src/lib/diff/gen-node.hpp
+++ b/src/lib/diff/gen-node.hpp
@@ -269,8 +269,44 @@ namespace diff{
       GenNode(Ref &  r);
       GenNode(Ref && r);
       
-      GenNode& operator= (GenNode const&)  =default;
-      GenNode& operator= (GenNode&&)       =default;
+      /** copy assignment
+       * @remarks we need to define our own version here for sake of sanity.
+       *    The reason is that we use inline storage (embedded within lib::Variant)
+       *    and that we deliberately _erase_ the actual type of data stored inline.
+       *    Because we still do want copy assignment, in case the payload data
+       *    supports this, we use a "virtual copy operator", where in the end
+       *    the storage buffer within lib::Variant has to decide if assignment
+       *    is possible. Only data with the same type may be assigned and we
+       *    prevent change of the (implicit) data type through assignment.
+       *    This check might throw, and for that reason we're better off
+       *    to perform the _data assignment_ first. The probability for
+       *    EntryID assignment to fail is low (but it may happen!).
+       * @note the use of inline storage turns swapping of data
+       *    into an expensive operation, involving a temporary.
+       *    This rules out the copy-and-swap idiom.
+       */
+      GenNode&
+      operator= (GenNode const& o)
+        {
+          if (&o != this)
+            {
+              data = o.data;
+              idi = o.idi;
+            }
+          return *this;
+        }
+      
+      GenNode&
+      operator= (GenNode&& o)
+        {
+          ASSERT (&o != this);
+          data = std::forward<DataCap>(o.data);
+          idi  = std::forward<ID>(o.idi);
+          return *this;
+        }
+      
+      //note: NOT defining a swap operation, because swapping inline storage is pointless!
+      
       
       
       
diff --git a/src/lib/variant.hpp b/src/lib/variant.hpp
index 66d990384..ad34f0f22 100644
--- a/src/lib/variant.hpp
+++ b/src/lib/variant.hpp
@@ -452,6 +452,8 @@ namespace lib {
           return *this;
         }
       
+      //note: NOT defining a swap operation, because swapping inline storage is pointless!
+      
       
       /** diagnostic helper */
       operator string()  const;
diff --git a/tests/gui/interact/state-map-grouping-storage-test.cpp b/tests/gui/interact/state-map-grouping-storage-test.cpp
index 575c286e7..a761f23ce 100644
--- a/tests/gui/interact/state-map-grouping-storage-test.cpp
+++ b/tests/gui/interact/state-map-grouping-storage-test.cpp
@@ -22,7 +22,7 @@
 
 
 #include "lib/test/run.hpp"
-//#include "lib/test/test-helper.hpp"
+#include "lib/test/test-helper.hpp"
 //#include "gui/ctrl/bus-term.hpp"
 //#include "gui/interact/presentation-state-manager.hpp"
 #include "gui/interact/state-map-grouping-storage.hpp"
@@ -60,7 +60,7 @@ namespace interact{
 namespace test {
   
 //  using proc::control::LUMIERA_ERROR_UNBOUND_ARGUMENTS;
-//  using lumiera::error::LUMIERA_ERROR_WRONG_TYPE;
+  using lumiera::error::LUMIERA_ERROR_WRONG_TYPE;
   
   namespace { // test fixture...
     
@@ -90,9 +90,10 @@ namespace test {
           EntryID<int> quack ("quack");
           
           GenNode poodle{"poodle", "Pudel"};
+          GenNode toyPoodle{"poodle", "Zwergpudel"};
           GenNode pseudoPoodle {"poodle", false};
-          
           GenNode mastiff{"mastiff", "Dogge"};
+          GenNode duck{"duck", "Ente"};
           
           StateMapGroupingStorage storage;
           
@@ -103,10 +104,36 @@ namespace test {
           CHECK (not isnil(storage));
           CHECK (1 == storage.size());
           
-          cout << poodle <<endl;
-          cout << storage.retrieve(woof, "poodle") <<endl;
           CHECK (poodle == storage.retrieve(woof, "poodle"));
           CHECK (not isSameObject (poodle, storage.retrieve(woof, "poodle")));
+          
+          CHECK (Ref::NO == storage.retrieve(wooof, "poodle"));
+          CHECK (Ref::NO == storage.retrieve(woof, "pooodle"));
+          
+          storage.record (woof, mastiff);
+          CHECK (2 == storage.size());
+          CHECK (poodle == storage.retrieve(woof, "poodle"));
+          CHECK (mastiff == storage.retrieve(woof, "mastiff"));
+          
+          // upgrade the poodle
+          storage.record (woof, toyPoodle);
+          CHECK (2 == storage.size());
+          CHECK (poodle    != storage.retrieve(woof, "poodle"));
+          CHECK (toyPoodle == storage.retrieve(woof, "poodle"));
+          
+          // since properties are keyed just by ID-string,
+          // we might attempt sneak in a fake poodle
+          // fortunately GenNode disallows cross-type assignments
+          VERIFY_ERROR (WRONG_TYPE, storage.record (woof, pseudoPoodle) );
+          
+          CHECK (2 == storage.size());
+          cout << toyPoodle <<endl;
+          cout << storage.retrieve(woof, "poodle") <<endl;
+          cout << size_t(toyPoodle.idi.getHash()) <<endl;
+          cout << size_t(storage.retrieve(woof, "poodle").idi.getHash()) <<endl;
+          cout << size_t(pseudoPoodle.idi.getHash()) <<endl;
+          CHECK (toyPoodle == storage.retrieve(woof, "poodle"));
+          CHECK (mastiff == storage.retrieve(woof, "mastiff"));
         }
     };