From 0c1d3e30b62dc7c94b7ae32b97089309de7ff956 Mon Sep 17 00:00:00 2001
From: Aleksei Bavshin <alebastr89@gmail.com>
Date: Wed, 15 Sep 2021 22:14:12 +0700
Subject: [PATCH] fix(config): preserve explicit null when merging objects

---
 src/config.cpp             | 9 +++++++--
 test/config.cpp            | 2 ++
 test/config/include-1.json | 3 ++-
 test/config/include.json   | 3 ++-
 4 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/src/config.cpp b/src/config.cpp
index 63f18c26..63149cbd 100644
--- a/src/config.cpp
+++ b/src/config.cpp
@@ -1,5 +1,6 @@
 #include "config.hpp"
 
+#include <fmt/ostream.h>
 #include <spdlog/spdlog.h>
 #include <unistd.h>
 #include <wordexp.h>
@@ -86,11 +87,15 @@ void Config::mergeConfig(Json::Value &a_config_, Json::Value &b_config_) {
     a_config_ = b_config_;
   } else if (a_config_.isObject() && b_config_.isObject()) {
     for (const auto &key : b_config_.getMemberNames()) {
-      if (a_config_[key].isObject() && b_config_[key].isObject()) {
+      // [] creates key with default value. Use `get` to avoid that.
+      if (a_config_.get(key, Json::Value::nullSingleton()).isObject() &&
+          b_config_[key].isObject()) {
         mergeConfig(a_config_[key], b_config_[key]);
-      } else if (a_config_[key].isNull()) {
+      } else if (!a_config_.isMember(key)) {
         // do not allow overriding value set by top or previously included config
         a_config_[key] = b_config_[key];
+      } else {
+        spdlog::trace("Option {} is already set; ignoring value {}", key, b_config_[key]);
       }
     }
   } else {
diff --git a/test/config.cpp b/test/config.cpp
index 343a1c11..f09f5da6 100644
--- a/test/config.cpp
+++ b/test/config.cpp
@@ -67,6 +67,8 @@ TEST_CASE("Load simple config with include", "[config]") {
     REQUIRE(data["height"].asInt() == 30);
     // config override behavior: preserve value from the top config
     REQUIRE(data["position"].asString() == "top");
+    // config override behavior: explicit null is still a value and should be preserved
+    REQUIRE((data.isMember("nullOption") && data["nullOption"].isNull()));
   }
   SECTION("select configs for configured output") {
     auto configs = conf.getOutputConfigs("HDMI-0", "Fake HDMI output #0");
diff --git a/test/config/include-1.json b/test/config/include-1.json
index 853111c7..7c47a882 100644
--- a/test/config/include-1.json
+++ b/test/config/include-1.json
@@ -2,5 +2,6 @@
   "layer": "top",
   "position": "bottom",
   "height": 30,
-  "output": ["HDMI-0", "DP-0"]
+  "output": ["HDMI-0", "DP-0"],
+  "nullOption": "not null"
 }
diff --git a/test/config/include.json b/test/config/include.json
index 098cae01..c46aaf24 100644
--- a/test/config/include.json
+++ b/test/config/include.json
@@ -1,4 +1,5 @@
 {
   "include": ["test/config/include-1.json", "test/config/include-2.json"],
-  "position": "top"
+  "position": "top",
+  "nullOption": null
 }