Allow trailing comments in config file:

Treat all `#` characters in config files as comments (and remove)
*unless* the `#` is immediately preceded by `\`. Write a warning
to log file when trailing comments are found/ignored in the config
to let operators know that the treatment of trailing `#` has changed.

Fixes #3121

src/ripple/app/main/Main.cpp

@@ -670,6 +670,17 @@ int run (int argc, char** argv)
     // No arguments. Run server.
     if (!vm.count ("parameters"))
     {
+        // TODO: this comment can be removed in a future release -
+        // say 1.7 or higher
+        if (config->had_trailing_comments())
+        {
+            JLOG(logs->journal("Application").warn()) <<
+            "Trailing comments were seen in your config file. " <<
+            "The treatment of inline/trailing comments has changed recently. " <<
+            "Any `#` characters NOT intended to delimit comments should be " <<
+            "preceded by a \\";
+        }
+
         // We want at least 1024 file descriptors. We'll
         // tweak this further.
         if (!adjustDescriptorLimit(1024, logs->journal("Application")))

src/ripple/basics/BasicConfig.h

@@ -25,6 +25,7 @@
 #include <boost/beast/core/string.hpp>
 #include <boost/lexical_cast.hpp>
 #include <boost/optional.hpp>
+#include <algorithm>
 #include <map>
 #include <ostream>
 #include <string>
@@ -47,6 +48,7 @@ private:
     std::string name_;
     std::vector <std::string> lines_;
     std::vector <std::string> values_;
+    bool had_trailing_comments_ = false;
 
 public:
     /** Create an empty section. */
@@ -153,12 +155,14 @@ public:
     T
     value_or(std::string const& name, T const& other) const
     {
-        auto const iter = cont().find(name);
+        auto const v = get<T>(name);
-        if (iter == cont().end())
+        return v.is_initialized() ? *v : other;
-            return other;
-        return boost::lexical_cast<T>(iter->second);
     }
 
+    // indicates if trailing comments were seen
+    // during the appending of any lines/values
+    bool had_trailing_comments() const { return had_trailing_comments_; }
+
     friend
     std::ostream&
     operator<< (std::ostream&, Section const& section);
@@ -243,6 +247,13 @@ public:
     std::ostream&
     operator<< (std::ostream& ss, BasicConfig const& c);
 
+    // indicates if trailing comments were seen
+    // in any loaded Sections
+    bool had_trailing_comments() const {
+        return std::any_of(map_.cbegin(), map_.cend(),
+            [](auto s){ return s.second.had_trailing_comments(); });
+    }
+
 protected:
     void
     build (IniFileSections const& ifs);
@@ -251,50 +262,41 @@ protected:
 //------------------------------------------------------------------------------
 
 /** Set a value from a configuration Section
-    If the named value is not found, the variable is unchanged.
+    If the named value is not found or doesn't parse as a T,
+    the variable is unchanged.
     @return `true` if value was set.
 */
 template <class T>
 bool
 set (T& target, std::string const& name, Section const& section)
 {
-    auto const [val, found] = section.find (name);
+    bool found_and_valid = false;
-    if (! found)
-        return false;
     try
     {
-        target = boost::lexical_cast <T> (val);
+        auto const val = section.get<T> (name);
-        return true;
+        if ((found_and_valid = val.is_initialized()))
+            target = *val;
     }
     catch (boost::bad_lexical_cast&)
     {
     }
-    return false;
+    return found_and_valid;
 }
 
 /** Set a value from a configuration Section
-    If the named value is not found, the variable is assigned the default.
+    If the named value is not found or doesn't cast to T,
-    @return `true` if named value was found in the Section.
+    the variable is assigned the default.
+    @return `true` if the named value was found and is valid.
 */
 template <class T>
 bool
 set (T& target, T const& defaultValue,
     std::string const& name, Section const& section)
 {
-    auto const [val, found] = section.find (name);
+    bool found_and_valid = set<T>(target, name, section);
-    if (! found)
+    if (!found_and_valid)
-        return false;
-    try
-    {
-        // VFALCO TODO Use try_lexical_convert (boost 1.56.0)
-        target = boost::lexical_cast <T> (val);
-        return true;
-    }
-    catch (boost::bad_lexical_cast&)
-    {
         target = defaultValue;
-    }
+    return found_and_valid;
-    return false;
 }
 
 /** Retrieve a key/value pair from a section.
@@ -307,12 +309,9 @@ T
 get (Section const& section,
     std::string const& name, T const& defaultValue = T{})
 {
-    auto const [val, found] = section.find (name);
-    if (!found)
-        return defaultValue;
     try
     {
-        return boost::lexical_cast <T> (val);
+        return section.value_or<T> (name, defaultValue);
     }
     catch (boost::bad_lexical_cast&)
     {
@@ -325,14 +324,14 @@ std::string
 get (Section const& section,
     std::string const& name, const char* defaultValue)
 {
-    auto const [val, found] = section.find (name);
+    bool found_and_valid = false;
-    if (!found)
-        return defaultValue;
     try
     {
-        return boost::lexical_cast <std::string> (val);
+        auto const val = section.get<std::string> (name);
+        if ((found_and_valid = val.is_initialized()))
+            return *val;
     }
-    catch(std::exception const&)
+    catch (boost::bad_lexical_cast&)
     {
     }
     return defaultValue;
@@ -343,18 +342,7 @@ bool
 get_if_exists (Section const& section,
     std::string const& name, T& v)
 {
-    auto const [val, found] = section.find (name);
+    return set<T>(v, name, section);
-    if (!found)
-        return false;
-    try
-    {
-        v = boost::lexical_cast <T> (val);
-        return true;
-    }
-    catch (boost::bad_lexical_cast&)
-    {
-    }
-    return false;
 }
 
 template <>
@@ -364,12 +352,10 @@ get_if_exists<bool> (Section const& section,
     std::string const& name, bool& v)
 {
     int intVal = 0;
-    if (get_if_exists (section, name, intVal))
+    auto stat = get_if_exists(section, name, intVal);
-    {
+    if (stat)
         v = bool (intVal);
-        return true;
+    return stat;
-    }
-    return false;
 }
 
 } // ripple

src/ripple/basics/impl/BasicConfig.cpp

@@ -18,6 +18,7 @@
 //==============================================================================
 
 #include <ripple/basics/BasicConfig.h>
+#include <ripple/basics/StringUtilities.h>
 #include <boost/regex.hpp>
 #include <algorithm>
 
@@ -53,14 +54,54 @@ Section::append (std::vector <std::string> const& lines)
     );
 
     lines_.reserve (lines_.size() + lines.size());
-    for (auto const& line : lines)
+    for (auto line : lines)
     {
+        auto remove_comment = [](std::string& val)->bool
+        {
+            bool removed_trailing = false;
+            auto comment = val.find('#');
+            while(comment != std::string::npos)
+            {
+                if (comment == 0)
+                {
+                    // entire value is a comment. In most cases, this
+                    // would have already been handled by the file reader
+                    val = "";
+                    break;
+                }
+                else if (val.at(comment-1) == '\\')
+                {
+                    // we have an escaped comment char. Erase the escape char
+                    // and keep looking
+                    val.erase(comment-1,1);
+                }
+                else
+                {
+                    // this must be a real comment. Extract the value
+                    // as a substring and stop looking.
+                    val = trim_whitespace(val.substr(0, comment));
+                    removed_trailing = true;
+                    break;
+                }
+
+                comment = val.find('#', comment);
+            }
+            return removed_trailing;
+        };
+
+        if (remove_comment(line) && !line.empty())
+            had_trailing_comments_ = true;
+
+        if (line.empty())
+            continue;
+
         boost::smatch match;
-        lines_.push_back (line);
         if (boost::regex_match (line, match, re1))
             set (match[1], match[2]);
         else
             values_.push_back (line);
+
+        lines_.push_back (std::move(line));
     }
 }
 

src/ripple/core/impl/Config.cpp

@@ -149,7 +149,7 @@ getIniFileSection (IniFileSections& secSource, std::string const& strSection)
     IniFileSections::mapped_type* smtResult;
     it  = secSource.find (strSection);
     if (it == secSource.end ())
-        smtResult   = 0;
+        smtResult   = nullptr;
     else
         smtResult   = & (it->second);
     return smtResult;

src/test/core/Config_test.cpp

@@ -786,6 +786,159 @@ r.ripple.com 51235
 
     }
 
+    void testComments()
+    {
+        struct TestCommentData
+        {
+            std::string_view line;
+            std::string_view field;
+            std::string_view expect;
+            bool had_comment;
+        };
+
+        std::array< TestCommentData, 13> tests = {{
+            { "password = aaaa\\#bbbb",   "password", "aaaa#bbbb", false},
+            { "password = aaaa#bbbb",     "password", "aaaa", true},
+            { "password = aaaa #bbbb",    "password", "aaaa", true},
+            // since the value is all comment, this doesn't parse as k=v :
+            { "password = #aaaa #bbbb",   "", "password =", true},
+            { "password = aaaa\\# #bbbb", "password", "aaaa#", true},
+            { "password = aaaa\\##bbbb",  "password", "aaaa#", true},
+            { "aaaa#bbbb",    "", "aaaa", true},
+            { "aaaa\\#bbbb",  "", "aaaa#bbbb", false},
+            { "aaaa\\##bbbb", "", "aaaa#", true},
+            { "aaaa #bbbb",   "", "aaaa", true},
+            { "1 #comment",   "", "1", true},
+            { "#whole thing is comment",   "", "", false},
+            { "  #whole comment with space",   "", "", false}
+        }};
+
+        for (auto const& t : tests)
+        {
+            Section s;
+            s.append(t.line.data());
+            BEAST_EXPECT(s.had_trailing_comments() == t.had_comment);
+            if (t.field.empty())
+            {
+                BEAST_EXPECTS(s.legacy() == t.expect, s.legacy());
+            }
+            else
+            {
+                std::string field;
+                BEAST_EXPECTS(set(field, t.field.data(), s), t.line);
+                BEAST_EXPECTS(field == t.expect, t.line);
+            }
+        }
+
+        {
+            Section s;
+            s.append("online_delete = 3000");
+            std::uint32_t od = 0;
+            BEAST_EXPECT(set(od, "online_delete", s));
+            BEAST_EXPECTS(od == 3000, *(s.get<std::string>("online_delete")));
+        }
+
+        {
+            Section s;
+            s.append("online_delete = 2000 #my comment on this");
+            std::uint32_t od = 0;
+            BEAST_EXPECT(set(od, "online_delete", s));
+            BEAST_EXPECTS(od == 2000, *(s.get<std::string>("online_delete")));
+        }
+    }
+
+    void testGetters()
+    {
+        using namespace std::string_literals;
+        Section s {"MySection"};
+        s.append("a_string = mystring");
+        s.append("positive_int = 2");
+        s.append("negative_int = -3");
+        s.append("bool_ish = 1");
+
+        {
+            auto val_1 = "value 1"s;
+            BEAST_EXPECT(set(val_1, "a_string", s));
+            BEAST_EXPECT(val_1 == "mystring");
+
+            auto val_2 = "value 2"s;
+            BEAST_EXPECT(!set(val_2, "not_a_key", s));
+            BEAST_EXPECT(val_2 == "value 2");
+            BEAST_EXPECT(!set(val_2, "default"s, "not_a_key", s));
+            BEAST_EXPECT(val_2 == "default");
+
+            auto val_3 = get<std::string>(s, "a_string");
+            BEAST_EXPECT(val_3 == "mystring");
+            auto val_4 = get<std::string>(s, "not_a_key");
+            BEAST_EXPECT(val_4 == "");
+            auto val_5 = get<std::string>(s, "not_a_key", "default");
+            BEAST_EXPECT(val_5 == "default");
+
+            auto val_6 = "value 6"s;
+            BEAST_EXPECT(get_if_exists(s, "a_string", val_6));
+            BEAST_EXPECT(val_6 == "mystring");
+
+            auto val_7 = "value 7"s;
+            BEAST_EXPECT(!get_if_exists(s, "not_a_key", val_7));
+            BEAST_EXPECT(val_7 == "value 7");
+        }
+
+        {
+            int val_1 = 1;
+            BEAST_EXPECT(set(val_1, "positive_int", s));
+            BEAST_EXPECT(val_1 == 2);
+
+            int val_2 = 2;
+            BEAST_EXPECT(set(val_2, "negative_int", s));
+            BEAST_EXPECT(val_2 == -3);
+
+            int val_3 = 3;
+            BEAST_EXPECT(!set(val_3, "a_string", s));
+            BEAST_EXPECT(val_3 == 3);
+
+            auto val_4 = get<int>(s, "positive_int");
+            BEAST_EXPECT(val_4 == 2);
+            auto val_5 = get<int>(s, "not_a_key");
+            BEAST_EXPECT(val_5 == 0);
+            auto val_6 = get<int>(s, "not_a_key", 5);
+            BEAST_EXPECT(val_6 == 5);
+            auto val_7 = get<int>(s, "a_string", 6);
+            BEAST_EXPECT(val_7 == 6);
+
+            int val_8 = 8;
+            BEAST_EXPECT(get_if_exists(s, "positive_int", val_8));
+            BEAST_EXPECT(val_8 == 2);
+
+            auto val_9 = 9;
+            BEAST_EXPECT(!get_if_exists(s, "not_a_key", val_9));
+            BEAST_EXPECT(val_9 == 9);
+
+            auto val_10 = 10;
+            BEAST_EXPECT(!get_if_exists(s, "a_string", val_10));
+            BEAST_EXPECT(val_10 == 10);
+
+            BEAST_EXPECT(s.get<int>("not_a_key") == boost::none);
+            try
+            {
+                s.get<int>("a_string");
+                fail();
+            }
+            catch(boost::bad_lexical_cast&)
+            {
+                pass();
+            }
+        }
+
+        {
+            bool flag_1 = false;
+            BEAST_EXPECT(get_if_exists(s, "bool_ish", flag_1));
+            BEAST_EXPECT(flag_1 == true);
+
+            bool flag_2 = false;
+            BEAST_EXPECT(!get_if_exists(s, "not_a_key", flag_2));
+            BEAST_EXPECT(flag_2 == false);
+        }
+    }
 
     void run () override
     {
@@ -797,6 +950,8 @@ r.ripple.com 51235
         testSetup (true);
         testPort ();
         testWhitespace ();
+        testComments ();
+        testGetters ();
     }
 };