Avoid code duplication through a templated function and improve JSON parsing of Boolean flags (#8773)

* - avoid code duplication through a templated function
- enable switching off boolean flags via JSON and not just on as before

* ran clang-format

* change in response to review

---------

Co-authored-by: ptosco <paolo.tosco@novartis.com>

Code/GraphMol/SmilesParse/SmilesJSONParsers.cpp

@@ -9,6 +9,7 @@
 //
 
 #define USE_BETTER_ENUMS
+#include <RDGeneral/JSONHelpers.h>
 #include <RDGeneral/BoostStartInclude.h>
 #include <boost/property_tree/ptree.hpp>
 #include <boost/property_tree/json_parser.hpp>
@@ -49,19 +50,9 @@ void updateCXSmilesFieldsFromJSON(std::uint32_t &cxSmilesFields,
     std::istringstream ss;
     ss.str(details_json);
     boost::property_tree::read_json(ss, pt);
-    auto cxSmilesFieldsFromJson =
-        (+SmilesWrite::CXSmilesFields::CX_NONE)._to_integral();
     bool haveCXSmilesFields = false;
-    for (const auto *key : SmilesWrite::CXSmilesFields::_names()) {
+    auto cxSmilesFieldsFromJson =
-      const auto it = pt.find(key);
+        flagsFromJson<SmilesWrite::CXSmilesFields>(pt, &haveCXSmilesFields);
-      if (it != pt.not_found()) {
-        haveCXSmilesFields = true;
-        if (it->second.get_value<bool>()) {
-          cxSmilesFieldsFromJson |=
-              SmilesWrite::CXSmilesFields::_from_string(key)._to_integral();
-        }
-      }
-    }
     if (haveCXSmilesFields) {
       cxSmilesFields = cxSmilesFieldsFromJson;
     }

Code/GraphMol/SmilesParse/cxsmiles_test.cpp

@@ -18,6 +18,7 @@
 #include <GraphMol/MarvinParse/MarvinParser.h>
 #include <GraphMol/Chirality.h>
 #include <GraphMol/SmilesParse/CanonicalizeStereoGroups.h>
+#include <GraphMol/SmilesParse/SmilesJSONParsers.h>
 #include "SmilesParse.h"
 #include "SmilesWrite.h"
 #include "SmartsWrite.h"
@@ -1659,4 +1660,64 @@ TEST_CASE("Github #8586: MolFromSmiles loses atom maps if cxsmiles is used") {
                                                  dlabel));
     CHECK(dlabel == "d");
   }
-}
+}
+
+TEST_CASE("Test CXSmilesFields option parsing from JSON") {
+  SECTION("Empty JSON string preserves current values") {
+    std::uint32_t cxSmilesFields =
+        SmilesWrite::CXSmilesFields::CX_ALL_BUT_COORDS;
+    unsigned int restoreBondDirs = RestoreBondDirOptionClear;
+    updateCXSmilesFieldsFromJSON(cxSmilesFields, restoreBondDirs, "{}");
+    CHECK(cxSmilesFields == SmilesWrite::CXSmilesFields::CX_ALL_BUT_COORDS);
+    CHECK(restoreBondDirs == RestoreBondDirOptionClear);
+  }
+  SECTION("No CXSmilesFields key preserves current values") {
+    std::uint32_t cxSmilesFields =
+        SmilesWrite::CXSmilesFields::CX_ALL_BUT_COORDS;
+    unsigned int restoreBondDirs = RestoreBondDirOptionTrue;
+    updateCXSmilesFieldsFromJSON(
+        cxSmilesFields, restoreBondDirs,
+        "{\"restoreBondDirOption\":\"RestoreBondDirOptionClear\"}");
+    CHECK(cxSmilesFields == SmilesWrite::CXSmilesFields::CX_ALL_BUT_COORDS);
+    CHECK(restoreBondDirs == RestoreBondDirOptionClear);
+  }
+  SECTION("Multiple CXSmilesFields keys with true value are ORed together") {
+    std::uint32_t cxSmilesFields = SmilesWrite::CXSmilesFields::CX_ALL;
+    unsigned int restoreBondDirs = RestoreBondDirOptionClear;
+    updateCXSmilesFieldsFromJSON(
+        cxSmilesFields, restoreBondDirs,
+        "{\"CX_MOLFILE_VALUES\":true,\"CX_COORDS\":true}");
+    CHECK(cxSmilesFields == (SmilesWrite::CXSmilesFields::CX_MOLFILE_VALUES |
+                             SmilesWrite::CXSmilesFields::CX_COORDS));
+    CHECK(restoreBondDirs == RestoreBondDirOptionClear);
+  }
+  SECTION(
+      "Multiple CXSmilesFields keys with true value are ORed together; adding unrelated false values makes no difference") {
+    std::uint32_t cxSmilesFields = SmilesWrite::CXSmilesFields::CX_ALL;
+    unsigned int restoreBondDirs = RestoreBondDirOptionClear;
+    updateCXSmilesFieldsFromJSON(
+        cxSmilesFields, restoreBondDirs,
+        "{\"CX_MOLFILE_VALUES\":true,\"CX_COORDS\":true,\"CX_ATOM_PROPS\":false,\"CX_BOND_CFG\":false}");
+    CHECK(cxSmilesFields == (SmilesWrite::CXSmilesFields::CX_MOLFILE_VALUES |
+                             SmilesWrite::CXSmilesFields::CX_COORDS));
+    CHECK(restoreBondDirs == RestoreBondDirOptionClear);
+  }
+  SECTION(
+      "Multiple CXSmilesFields keys with true value are ORed together, then AND NOTed with ORed false values") {
+    std::uint32_t cxSmilesFields = SmilesWrite::CXSmilesFields::CX_ALL;
+    unsigned int restoreBondDirs = RestoreBondDirOptionClear;
+    updateCXSmilesFieldsFromJSON(cxSmilesFields, restoreBondDirs,
+                                 "{\"CX_ALL\":true,\"CX_COORDS\":false}");
+    CHECK(cxSmilesFields == SmilesWrite::CXSmilesFields::CX_ALL_BUT_COORDS);
+    CHECK(restoreBondDirs == RestoreBondDirOptionClear);
+  }
+  SECTION(
+      "Multiple CXSmilesFields keys with true value are ORed together, then AND NOTed with ORed false values; order does not matter") {
+    std::uint32_t cxSmilesFields = SmilesWrite::CXSmilesFields::CX_ALL;
+    unsigned int restoreBondDirs = RestoreBondDirOptionClear;
+    updateCXSmilesFieldsFromJSON(cxSmilesFields, restoreBondDirs,
+                                 "{\"CX_COORDS\":false,\"CX_ALL\":true}");
+    CHECK(cxSmilesFields == SmilesWrite::CXSmilesFields::CX_ALL_BUT_COORDS);
+    CHECK(restoreBondDirs == RestoreBondDirOptionClear);
+  }
+}

Code/MinimalLib/JSONParsers.cpp

@@ -13,6 +13,7 @@
 #include <GraphMol/MolPickler.h>
 #include <GraphMol/FileParsers/PNGParser.h>
 #include <GraphMol/SmilesParse/SmilesJSONParsers.h>
+#include <RDGeneral/JSONHelpers.h>
 #include <RDGeneral/BoostStartInclude.h>
 #include <boost/property_tree/ptree.hpp>
 #include <boost/property_tree/json_parser.hpp>
@@ -30,16 +31,8 @@ void updatePropertyPickleOptionsFromJSON(unsigned int &propFlags,
     boost::property_tree::read_json(ss, pt);
     const auto nodeIt = pt.find("propertyFlags");
     if (nodeIt != pt.not_found()) {
-      auto propertyFlagsFromJson =
+      propFlags =
-          (+PicklerOps::PropertyPickleOptions::NoProps)._to_integral();
+          flagsFromJson<PicklerOps::PropertyPickleOptions>(nodeIt->second);
-      for (const auto *key : PicklerOps::PropertyPickleOptions::_names()) {
-        if (nodeIt->second.get(key, false)) {
-          propertyFlagsFromJson |=
-              PicklerOps::PropertyPickleOptions::_from_string(key)
-                  ._to_integral();
-        }
-      }
-      propFlags = propertyFlagsFromJson;
     }
   }
 }
@@ -51,15 +44,7 @@ void updateSanitizeFlagsFromJSON(unsigned int &sanitizeFlags,
     boost::property_tree::ptree pt;
     ss.str(details_json);
     boost::property_tree::read_json(ss, pt);
-    auto sanitizeFlagsFromJson =
+    sanitizeFlags = flagsFromJson<MolOps::SanitizeFlags>(pt);
-        (+MolOps::SanitizeFlags::SANITIZE_NONE)._to_integral();
-    for (const auto *key : MolOps::SanitizeFlags::_names()) {
-      if (pt.get(key, false)) {
-        sanitizeFlagsFromJson |=
-            MolOps::SanitizeFlags::_from_string(key)._to_integral();
-      }
-    }
-    sanitizeFlags = sanitizeFlagsFromJson;
   }
 }
 

Code/RDGeneral/JSONHelpers.h

@@ -0,0 +1,36 @@
+#include <RDGeneral/BoostStartInclude.h>
+#include <boost/property_tree/ptree.hpp>
+#include <RDGeneral/BoostEndInclude.h>
+#include <iostream>
+#include <array>
+
+// Convert a JSON object with boolean keys to a bitwise enum flag set.
+// The T enum must have been declared as a BETTER_ENUM.
+// The JSON object should have keys corresponding to the enum names, and boolean
+// values. Example JSON: { "FlagA": true, "FlagB": false, "FlagC": true,
+// "FlagD": false } This would set FlagA and FlagC in the resulting flag set,
+// then clear FlagB and FlagD. If keysFound is provided and non-null, it will be
+// set to true if any of the enum keys were found in the JSON object, false
+// otherwise.
+template <typename T>
+typename T::_integral flagsFromJson(const boost::property_tree::ptree &pt,
+                                    bool *keysFound = nullptr) {
+  std::array<typename T::_integral, 2> flagsByType;
+  flagsByType.fill(static_cast<T::_integral>(0));
+  if (keysFound) {
+    *keysFound = false;
+  }
+  for (const auto *key : T::_names()) {
+    const auto it = pt.find(key);
+    if (it == pt.not_found()) {
+      continue;
+    }
+    if (keysFound) {
+      *keysFound = true;
+    }
+    auto value = T::_from_string(key)._to_integral();
+    auto i = static_cast<unsigned int>(it->second.template get_value<bool>());
+    flagsByType[i] |= value;
+  }
+  return (flagsByType[1] & ~flagsByType[0]);
+}