fix(config_compiler): "/" mistaken as path separator in merged map key (

#192) Fixes #190
rime · Mar 7, 2018 · 831ffba · 831ffba
1 parent 3cbc9cb
commit 831ffba
Show file tree

Hide file tree

Showing 2 changed files with 53 additions and 32 deletions.
diff --git a/src/rime/config/config_compiler.cc b/src/rime/config/config_compiler.cc
@@ -155,33 +155,39 @@ inline static string StripOperator(const string& key, bool adding) {
 }
 
 // defined in config_data.cc
-bool TraverseCopyOnWrite(an<ConfigItemRef> root, const string& path,
-                         function<bool (an<ConfigItemRef> target)> writer);
+an<ConfigItemRef> TypeCheckedCopyOnWrite(an<ConfigItemRef> parent,
+                                         const string& key);
+an<ConfigItemRef> TraverseCopyOnWrite(an<ConfigItemRef> head,
+                                      const string& path);
 
-static bool EditNode(an<ConfigItemRef> target,
+static bool EditNode(an<ConfigItemRef> head,
                      const string& key,
                      const an<ConfigItem>& value,
                      bool merge_tree) {
-  DLOG(INFO) << "EditNode(" << key << "," << merge_tree << ")";
+  DLOG(INFO) << "edit node: " << key << ", merge_tree: " << merge_tree;
   bool appending = IsAppending(key);
   bool merging = IsMerging(key, value, merge_tree);
-  auto writer = [=](an<ConfigItemRef> target) {
-    if ((appending || merging) && **target) {
-      DLOG(INFO) << "writer: editing node";
-      return !value ||
-      (appending && (AppendToString(target, As<ConfigValue>(value)) ||
-                     AppendToList(target, As<ConfigList>(value)))) ||
-      (merging && MergeTree(target, As<ConfigMap>(value)));
-    } else {
-      DLOG(INFO) << "writer: overwriting node";
-      *target = value;
-      return true;
-    }
-  };
   string path = StripOperator(key, appending || merging);
   DLOG(INFO) << "appending: " << appending << ", merging: " << merging
              << ", path: " << path;
-  return TraverseCopyOnWrite(target, path, writer);
+  auto find_target_node =
+      merge_tree ? &TypeCheckedCopyOnWrite : &TraverseCopyOnWrite;
+  auto target = find_target_node(head, path);
+  if (!target) {
+    // error finding target node; cannot write
+    return false;
+  }
+  if ((appending || merging) && **target) {
+    DLOG(INFO) << "writer: editing node";
+    return !value ||  // no-op
+        (appending && (AppendToString(target, As<ConfigValue>(value)) ||
+                       AppendToList(target, As<ConfigList>(value)))) ||
+        (merging && MergeTree(target, As<ConfigMap>(value)));
+  } else {
+    DLOG(INFO) << "writer: overwriting node";
+    *target = value;
+    return true;
+  }
 }
 
 bool PatchLiteral::Resolve(ConfigCompiler* compiler) {

diff --git a/src/rime/config/config_data.cc b/src/rime/config/config_data.cc
@@ -159,37 +159,52 @@ class ConfigDataRootRef : public ConfigItemRef {
   ConfigData* data_;
 };
 
-bool TraverseCopyOnWrite(an<ConfigItemRef> root, const string& path,
-                         function<bool (an<ConfigItemRef> target)> writer) {
+an<ConfigItemRef> TypeCheckedCopyOnWrite(an<ConfigItemRef> parent,
+                                         const string& key) {
+  // special case to allow editing current node by __append: __merge: /+: /=:
+  if (key.empty()) {
+    return parent;
+  }
+  bool is_list = ConfigData::IsListItemReference(key);
+  auto expected_node_type = is_list ? ConfigItem::kList : ConfigItem::kMap;
+  an<ConfigItem> existing_node = *parent;
+  if (existing_node && existing_node->type() != expected_node_type) {
+    LOG(ERROR) << "copy on write failed; incompatible node type: " << key;
+    return nullptr;
+  }
+  return Cow(parent, key);
+}
+
+an<ConfigItemRef> TraverseCopyOnWrite(an<ConfigItemRef> head,
+                                      const string& path) {
   DLOG(INFO) << "TraverseCopyOnWrite(" << path << ")";
   if (path.empty() || path == "/") {
-    return writer(root);
+    return head;
   }
-  an<ConfigItemRef> head = root;
   vector<string> keys = ConfigData::SplitPath(path);
   size_t n = keys.size();
   for (size_t i = 0; i < n; ++i) {
     const auto& key = keys[i];
-    bool is_list = ConfigData::IsListItemReference(key);
-    auto expected_node_type = is_list ? ConfigItem::kList : ConfigItem::kMap;
-    an<ConfigItem> existing_node = *head;
-    if (existing_node && existing_node->type() != expected_node_type) {
-      LOG(ERROR) << "copy on write failed; incompatible node type: " << key;
-      return false;
+    if (auto child = TypeCheckedCopyOnWrite(head, key)) {
+      head = child;
+    } else {
+      LOG(ERROR) << "while writing to " << path;
+      return nullptr;
     }
-    head = Cow(head, key);
   }
-  return writer(head);
+  return head;
 }
 
 bool ConfigData::TraverseWrite(const string& path, an<ConfigItem> item) {
   LOG(INFO) << "write: " << path;
   auto root = New<ConfigDataRootRef>(this);
-  return TraverseCopyOnWrite(root, path, [=](an<ConfigItemRef> target) {
+  if (auto target = TraverseCopyOnWrite(root, path)) {
     *target = item;
     set_modified();
     return true;
-  });
+  } else {
+    return false;
+  }
 }
 
 vector<string> ConfigData::SplitPath(const string& path) {