Fix slash handling in keys.

In actual use '/' is a path delimiter indicating a child KvpFrame. The
previous implementation created keys for single frames with embedded '/'
characters.

Memory management issues in make_vector necessitated changing Path to a
std::vector<std::string>>.

src/backend/xml/test/test-kvp-frames.c

@@ -132,7 +132,7 @@ test_kvp_frames1(void)
         test_val1 = get_random_kvp_value(i % KVP_TYPE_FRAME);
 
         test_frame1 = kvp_frame_new();
-        test_key = get_random_string();
+        test_key = get_random_string_without("/");
 
         kvp_frame_set_slot(test_frame1, test_key, test_val1);
 

src/libqof/qof/kvp_frame.cpp

@@ -39,6 +39,8 @@ extern "C"
 #include <algorithm>
 #include <vector>
 
+static const char delim = '/';
+
 KvpFrameImpl::KvpFrameImpl(const KvpFrameImpl & rhs) noexcept
 {
     std::for_each(rhs.m_valuemap.begin(), rhs.m_valuemap.end(),
@@ -51,12 +53,28 @@ KvpFrameImpl::KvpFrameImpl(const KvpFrameImpl & rhs) noexcept
     );
 }
 
+static inline Path
+make_vector(std::string key)
+{
+    Path path;
+    for (auto length = key.find(delim); length != std::string::npos;)
+    {
+        path.push_back(key.substr(0, length));
+        key = key.substr(length + 1);
+        length = key.find(delim);
+    }
+    path.push_back(key);
+    return path;
+}
+
 KvpValue*
 KvpFrameImpl::set(const char* key, KvpValue* value) noexcept
 {
     if (!key) return nullptr;
-    auto spot = m_valuemap.find(key);
+    if (strchr(key, delim))
+        return set(make_vector(key), value);
     KvpValue* ret {nullptr};
+    auto spot = m_valuemap.find(key);
     if (spot != m_valuemap.end())
     {
         qof_string_cache_remove(spot->first);
@@ -66,7 +84,8 @@ KvpFrameImpl::set(const char* key, KvpValue* value) noexcept
 
     if (value)
     {
-        auto cachedkey = static_cast<const char *>(qof_string_cache_insert(key));
+        auto cachedkey =
+            static_cast<const char *>(qof_string_cache_insert(key));
         m_valuemap.insert({cachedkey,value});
     }
 
@@ -79,7 +98,7 @@ walk_path_or_nullptr(const KvpFrameImpl* frame, Path& path)
     KvpFrameImpl* cur_frame = const_cast<KvpFrameImpl*>(frame);
     for(auto key:path)
     {
-        auto slot = cur_frame->get_slot(key);
+        auto slot = cur_frame->get_slot(key.c_str());
         if (slot == nullptr || slot->get_type() != KVP_TYPE_FRAME)
             return nullptr;
         cur_frame = slot->get<KvpFrame*>();
@@ -87,37 +106,52 @@ walk_path_or_nullptr(const KvpFrameImpl* frame, Path& path)
     return cur_frame;
 }
 
-
 KvpValue*
 KvpFrameImpl::set(Path path, KvpValue* value) noexcept
 {
-    const char* last_key = path.back();
+    auto last_key = path.back();
     path.pop_back();
     auto cur_frame = walk_path_or_nullptr(this, path);
     if (cur_frame == nullptr)
         return nullptr;
-    return cur_frame->set(last_key, value);
+    if (last_key.find(delim) != std::string::npos)
+        return set(make_vector(last_key), value);
+    return cur_frame->set(last_key.c_str(), value);
+}
+
+static inline KvpFrameImpl*
+walk_path_and_create(KvpFrameImpl* frame, Path path)
+{
+     for(auto key:path)
+    {
+        if (key.find(delim) != std::string::npos)
+        {
+            frame = walk_path_and_create(frame, make_vector(key));
+            continue;
+        }
+        auto slot = frame->get_slot(key.c_str());
+        if (slot == nullptr || slot->get_type() != KVP_TYPE_FRAME)
+        {
+            auto new_frame = new KvpFrame;
+            delete frame->set(key.c_str(), new KvpValue{new_frame});
+            frame = new_frame;
+            continue;
+        }
+        frame = slot->get<KvpFrame*>();
+    }
+    return frame;
 }
 
 KvpValue*
 KvpFrameImpl::set_path(Path path, KvpValue* value) noexcept
 {
     auto cur_frame = this;
-    const char* last_key = path.back();
+    auto last_key = path.back();
     path.pop_back();
-    for(auto key:path)
+    cur_frame = walk_path_and_create(const_cast<KvpFrame*>(this), path);
-    {
+    if (last_key.find(delim) != std::string::npos)
-        auto slot = cur_frame->get_slot(key);
+        return set_path(make_vector(last_key), value);
-        if (slot == nullptr || slot->get_type() != KVP_TYPE_FRAME)
+    return cur_frame->set(last_key.c_str(), value);
-        {
-            auto new_frame = new KvpFrame;
-            delete cur_frame->set(key, new KvpValue{new_frame});
-            cur_frame = new_frame;
-            continue;
-        }
-        cur_frame = slot->get<KvpFrame*>();
-    }
-    return cur_frame->set(last_key, value);
 }
 
 std::string
@@ -173,6 +207,9 @@ KvpFrameImpl::for_each_slot(void (*proc)(const char *key, KvpValue *value,
 KvpValueImpl *
 KvpFrameImpl::get_slot(const char * key) const noexcept
 {
+    if (!key) return nullptr;
+    if (strchr(key, delim))
+        return get_slot(make_vector(key));
     auto spot = m_valuemap.find(key);
     if (spot == m_valuemap.end())
         return nullptr;
@@ -182,12 +219,14 @@ KvpFrameImpl::get_slot(const char * key) const noexcept
 KvpValueImpl *
 KvpFrameImpl::get_slot(Path path) const noexcept
 {
-    const char* last_key = path.back();
+    auto last_key = path.back();
     path.pop_back();
     auto cur_frame = walk_path_or_nullptr(this, path);
     if (cur_frame == nullptr)
         return nullptr;
-    return cur_frame->get_slot(last_key);
+    if (last_key.find(delim) != std::string::npos)
+        return get_slot(make_vector(last_key));
+    return cur_frame->get_slot(last_key.c_str());
 
 }
 

src/libqof/qof/kvp_frame.hpp

@@ -41,7 +41,7 @@ class cstring_comparer
     }
 };
 
-using Path = std::vector<const char*>;
+using Path = std::vector<std::string>;
 
 struct KvpFrameImpl
 {

src/libqof/qof/test/test-kvp-frame.cpp

@@ -57,10 +57,14 @@ TEST_F (KvpFrameTest, SetLocal)
     auto f1 = new KvpFrameImpl;
     auto v1 = new KvpValueImpl {15.0};
     auto v2 = new KvpValueImpl { (int64_t)52};
-    std::string k1 {"first key"};
+    const char* k1 = "first key";
+    const char* k2 = "first key/second key";
 
-    EXPECT_EQ (nullptr, f1->set (k1.c_str (), v1));
+    EXPECT_EQ (nullptr, f1->set (k1, v1));
-    EXPECT_EQ (v1, f1->set (k1.c_str (), v2));
+    auto first_frame = new KvpFrame;
+    EXPECT_EQ (v1, f1->set (k1, new KvpValue{first_frame}));
+    EXPECT_EQ (nullptr, f1->set(k2, v2));
+    EXPECT_EQ (v2, first_frame->get_slot("second key"));
 
     delete f1; //this should also delete v2.
     delete v1;
@@ -81,6 +85,24 @@ TEST_F (KvpFrameTest, SetPath)
     delete v1;
 }
 
+TEST_F (KvpFrameTest, SetPathSlash)
+{
+    Path path1 {"top", "second/twenty", "twenty-first"};
+    Path path2 {"top", "third", "thirty-first"};
+    auto v1 = new KvpValueImpl {15.0};
+    auto v2 = new KvpValueImpl { (int64_t)52};
+
+    EXPECT_EQ (nullptr, t_root.set(path1, v1));
+    EXPECT_EQ (nullptr, t_root.set(path1, v2));
+    auto second_frame = t_root.get_slot("top")->get<KvpFrame*>()->get_slot("second")->get<KvpFrame*>();
+    second_frame->set("twenty", new KvpValue{new KvpFrame});
+    EXPECT_EQ (nullptr, t_root.set(path1, v1));
+    EXPECT_EQ (v1, t_root.set(path1, v2));
+    EXPECT_EQ (v2, t_root.get_slot(path1));
+    EXPECT_EQ (nullptr, t_root.get_slot(path2));
+    delete v1;
+}
+
 TEST_F (KvpFrameTest, SetPathWithCreate)
 {
     Path path1 {"top", "second", "twenty-first"};
@@ -95,6 +117,22 @@ TEST_F (KvpFrameTest, SetPathWithCreate)
     EXPECT_EQ (v1, t_root.get_slot(path2));
 }
 
+TEST_F (KvpFrameTest, SetPathWithCreateSlash)
+{
+    Path path1 {"top", "second/twenty", "twenty-first"};
+    Path path2 {"top", "third", "thirty-first"};
+    Path path1a {"top", "second", "twenty", "twenty-first"};
+    auto v1 = new KvpValueImpl {15.0};
+    auto v2 = new KvpValueImpl { (int64_t)52};
+
+    EXPECT_EQ (nullptr, t_root.set_path(path1, v1));
+    EXPECT_EQ (v1, t_root.set_path(path1, v2));
+    EXPECT_EQ (nullptr, t_root.set_path(path2, v1));
+    EXPECT_EQ (v2, t_root.get_slot(path1));
+    EXPECT_EQ (v2, t_root.get_slot(path1a));
+    EXPECT_EQ (v1, t_root.get_slot(path2));
+}
+
 TEST_F (KvpFrameTest, GetKeys)
 {
     auto k1 = "top";
@@ -117,6 +155,7 @@ TEST_F (KvpFrameTest, GetLocalSlot)
     auto k1 = "first";
     auto k2 = "third";
     auto k3 = "doesn't exist";
+    auto k4 = "top/first";
 
     auto frameval = t_root.get_slot("top");
     ASSERT_EQ(frameval->get_type(), KVP_TYPE_FRAME);
@@ -124,12 +163,15 @@ TEST_F (KvpFrameTest, GetLocalSlot)
     EXPECT_EQ (t_int_val, f1->get_slot(k1));
     EXPECT_EQ (t_str_val, f1->get_slot(k2));
     EXPECT_EQ (nullptr, f1->get_slot(k3));
+    EXPECT_EQ (t_int_val, t_root.get_slot(k4));
 }
 
 TEST_F (KvpFrameTest, GetSlotPath)
 {
     Path path1 {"top", "second", "twenty-first"};
     Path path2 {"top", "third", "thirty-first"};
+    Path path3 {"top", "second", "twenty", "twenty-first"};
+    Path path3a {"top", "second/twenty", "twenty-first"};
     auto v1 = new KvpValueImpl {15.0};
     auto v2 = new KvpValueImpl { (int64_t)52};
 
@@ -137,7 +179,8 @@ TEST_F (KvpFrameTest, GetSlotPath)
     EXPECT_EQ (nullptr, t_root.set(path2, v2));
     EXPECT_EQ (v1, t_root.get_slot(path1));
     EXPECT_EQ (nullptr, t_root.get_slot(path2));
-    delete v2;
+    t_root.set_path(path3, v1);
+    EXPECT_EQ (v1, t_root.get_slot(path3a));
 }
 
 TEST_F (KvpFrameTest, Empty)