Merge 'config: round-trip boolean configuration variables' from Avi Kivity

When you SELECT a boolean from system.config, it reads as true/false, but this isn't accepted
on UPDATE (instead, we accept 1/0). This is surprising and annoying, so accept true/false in
both directions.

Not a regression, so a backport isn't strictly necessary.

Closes scylladb/scylladb#19792

* github.com:scylladb/scylladb:
  config: specialize from-string conversion for bool
  config: wrap boost::lexical_cast<> when converting from strings

(cherry picked from commit 9eb47b3ef0)

db/config.cc

@@ -99,6 +99,21 @@ error_injection_list_to_json(const std::vector<db::config::error_injection_at_st
     return value_to_json("error_injection_list");
 }
 
+template <>
+bool
+config_from_string(std::string_view value) {
+    // boost::lexical_cast doesn't accept true/false, which are our output representations
+    // for bools. We want round-tripping, so we need to accept true/false. For backward
+    // compatibility, we also accept 1/0. #19791.
+    if (value == "true" || value == "1") {
+        return true;
+    } else if (value == "false" || value == "0") {
+        return false;
+    } else {
+        throw boost::bad_lexical_cast(typeid(std::string_view), typeid(bool));
+    }
+}
+
 template <>
 const config_type config_type_for<bool> = config_type("bool", value_to_json<bool>);
 
@@ -177,7 +192,7 @@ struct convert<seastar::log_level> {
         if (!convert<std::string>::decode(node, tmp)) {
             return false;
         }
-        rhs = boost::lexical_cast<seastar::log_level>(tmp);
+        rhs = utils::config_from_string<seastar::log_level>(tmp);
         return true;
     }
 };

test/cql-pytest/test_virtual_tables.py

@@ -86,6 +86,30 @@ def test_system_config_read(scylla_only, cql):
     assert isinstance(obj, str)
     assert obj and obj.isascii() and obj.isprintable()
 
+# Verify that boolean configuration items round-trip and use the yaml/json
+# representation (true/false). #19791.
+def test_system_config_update_boolean(scylla_only, cql):
+    var = 'compaction_enforce_min_threshold'
+    value = cql.execute(f"SELECT value FROM system.config WHERE name = '{var}'")[0].value
+    assert value in ('true', 'false')
+    other = 'true' if value == 'false' else 'false'
+    cql.execute(f"UPDATE system.config SET value = '{other}' WHERE name = '{var}'")
+    readback = cql.execute(f"SELECT value FROM system.config WHERE name = '{var}'")[0].value
+    assert readback == other
+
+    # just for completeness, check that writing 0/1 works too
+    cql.execute(f"UPDATE system.config SET value = '0' WHERE name = '{var}'")
+    readback = cql.execute(f"SELECT value FROM system.config WHERE name = '{var}'")[0].value
+    assert readback == 'false'
+    cql.execute(f"UPDATE system.config SET value = '1' WHERE name = '{var}'")
+    readback = cql.execute(f"SELECT value FROM system.config WHERE name = '{var}'")[0].value
+    assert readback == 'true'
+
+    # restore original
+    cql.execute(f"UPDATE system.config SET value = '{value}' WHERE name = '{var}'")
+    readback = cql.execute(f"SELECT value FROM system.config WHERE name = '{var}'")[0].value
+    assert readback == value
+
 def test_token_ring_vnodes(scylla_only, cql, test_keyspace_vnodes):
     rows = list(cql.execute(f"SELECT * FROM system.token_ring WHERE keyspace_name = '{test_keyspace_vnodes}'"))
     num_tokens = int(list(cql.execute("SELECT value FROM system.config WHERE name = 'num_tokens'"))[0].value)

utils/config_file_impl.hh

@@ -22,6 +22,18 @@
 
 #include <seastar/json/json_elements.hh>
 
+namespace utils {
+
+template <typename T>
+T config_from_string(std::string_view string_representation) {
+    return boost::lexical_cast<T>(string_representation);
+}
+
+template <>
+bool config_from_string(std::string_view string_representation);
+
+}
+
 namespace YAML {
 
 /*
@@ -84,7 +96,7 @@ std::istream& operator>>(std::istream& is, std::unordered_map<K, V, Args...>& ma
     is >> tmp;
 
     for (auto& p : tmp) {
-        map[boost::lexical_cast<K>(p.first)] = boost::lexical_cast<V>(p.second);
+        map[utils::config_from_string<K>(p.first)] = utils::config_from_string<V>(p.second);
     }
     return is;
 }
@@ -94,7 +106,7 @@ std::istream& operator>>(std::istream& is, std::vector<V, Args...>& dst) {
     std::vector<seastar::sstring> tmp;
     is >> tmp;
     for (auto& v : tmp) {
-        dst.emplace_back(boost::lexical_cast<V>(v));
+        dst.emplace_back(utils::config_from_string<V>(v));
     }
     return is;
 }
@@ -126,7 +138,7 @@ void validate(boost::any& out, const std::vector<std::string>& in, std::unordere
                 ve = s.begin() + i->position();
             }
 
-            (*p)[boost::lexical_cast<K>(k)] = boost::lexical_cast<V>(sstring(vs, ve));
+            (*p)[utils::config_from_string<K>(k)] = utils::config_from_string<V>(sstring(vs, ve));
         }
     }
 }
@@ -210,7 +222,7 @@ bool utils::config_file::named_value<T>::set_value(sstring value, config_source
         return false;
     }
 
-    (*this)(boost::lexical_cast<T>(value), src);
+    (*this)(config_from_string<T>(value), src);
     return true;
 }
 
@@ -232,7 +244,7 @@ future<bool> utils::config_file::named_value<T>::set_value_on_all_shards(sstring
         co_return false;
     }
 
-    co_await smp::invoke_on_all([this, value = boost::lexical_cast<T>(value), src] () {
+    co_await smp::invoke_on_all([this, value = config_from_string<T>(value), src] () {
         (*this)(value, src);
     });
     co_return true;