Merge pull request #5296 from NREL/alfalfa_id_validation

Relax Alfalfa Point ID Requirements
NREL · Nov 8, 2024 · ae56871 · ae56871
2 parents b5a6c58 + d5c11a6
commit ae56871
Show file tree

Hide file tree

Showing 5 changed files with 32 additions and 86 deletions.
diff --git a/src/alfalfa/AlfalfaJSON.cpp b/src/alfalfa/AlfalfaJSON.cpp
@@ -83,9 +83,9 @@ namespace alfalfa {
 
     Json::Value AlfalfaJSON_Impl::toJSON() const {
       Json::Value root;
-      for (const auto& point : m_points) {
+      for (Json::ArrayIndex i = 0; const auto& point : points()) {
         // No guard here as the toJSON call will throw an exception if the id does not exist.
-        root[point.id().get()] = point.toJSON();
+        root[i++] = point.toJSON();
       }
       return root;
     }

diff --git a/src/alfalfa/AlfalfaPoint.cpp b/src/alfalfa/AlfalfaPoint.cpp
@@ -7,12 +7,11 @@
 
 #include <memory>
 #include <string_view>
+#include <algorithm>
 
 namespace openstudio {
 namespace alfalfa {
 
-  static constexpr std::string_view ID_VALID_CHARS_MSG = "IDs can only contain letters, numbers, and the following special characters _-[]():";
-
   namespace detail {
 
     AlfalfaPoint_Impl::AlfalfaPoint_Impl(const std::string& display_name) {
@@ -22,11 +21,7 @@ namespace alfalfa {
     Json::Value AlfalfaPoint_Impl::toJSON() const {
       Json::Value point;
 
-      if (auto id_ = id()) {
-        point["id"] = *id_;
-      } else {
-        throw std::runtime_error(fmt::format("Point requires a valid ID for export. {}", ID_VALID_CHARS_MSG));
-      }
+      point["id"] = id();
       point["name"] = displayName();
       if (auto input_ = input()) {
         point["input"]["type"] = input_->typeName();
@@ -80,37 +75,24 @@ namespace alfalfa {
     }
 
     void AlfalfaPoint_Impl::setId(const std::string& id) {
-      if (isValidId(id)) {
-        m_id = id;
-      } else {
-        throw std::runtime_error(ID_VALID_CHARS_MSG.data());
+      if (id.empty()) {
+        throw std::runtime_error("Id must have non-zero length");
       }
+      m_id = toIdString(id);
     }
 
-    boost::optional<std::string> AlfalfaPoint_Impl::id() const {
-      boost::optional<std::string> result = m_id;
-      if (!result.is_initialized()) {
-        std::string id = toIdString(displayName());
-        if (isValidId(id)) {
-          result = id;
-        } else {
-          LOG(Warn, fmt::format("Display name '{}' does not produce a valid point ID. Manually set a valid ID or export will fail.", displayName()));
-        }
+    std::string AlfalfaPoint_Impl::id() const {
+      if (m_id.empty()) {
+        return toIdString(displayName());
       }
-      return result;
+      return m_id;
     }
 
     void AlfalfaPoint_Impl::setDisplayName(const std::string& display_name) {
       if (display_name.empty()) {
         throw std::runtime_error("Display name must have non-zero length");
       }
       m_display_name = display_name;
-      if (!m_id.is_initialized()) {
-        const std::string id = toIdString(display_name);
-        if (!isValidId(id)) {
-          LOG(Warn, fmt::format("Display name '{}' does not produce a valid point ID. Manually set a valid ID or export will fail.", display_name));
-        }
-      }
     }
 
     std::string AlfalfaPoint_Impl::displayName() const {
@@ -125,17 +107,13 @@ namespace alfalfa {
       return m_optional;
     }
 
-    bool AlfalfaPoint_Impl::isValidId(const std::string& id) {
-      return !id.empty() && boost::regex_match(id, boost::regex(R"(^[A-Za-z0-9_\-\[\]:()]*$)"));
-    }
-
     std::string AlfalfaPoint_Impl::toIdString(const std::string& str) {
-      return boost::regex_replace(str, boost::regex(" "), "_");
+      std::string id_string = str;
+      std::replace(id_string.begin(), id_string.end(), ' ', '_');
+      return id_string;
     }
   }  // namespace detail
 
-  // AlfalfaPoint::AlfalfaPoint(const AlfalfaPoint& point) : m_impl(point.m_impl) {}
-
   AlfalfaPoint::AlfalfaPoint(const std::string& display_name) : m_impl(std::make_shared<detail::AlfalfaPoint_Impl>(display_name)) {}
 
   void AlfalfaPoint::setInput(const AlfalfaComponent& component) {
@@ -162,7 +140,7 @@ namespace alfalfa {
     return m_impl->units();
   }
 
-  boost::optional<std::string> AlfalfaPoint::id() const {
+  std::string AlfalfaPoint::id() const {
     return m_impl->id();
   }
 

diff --git a/src/alfalfa/AlfalfaPoint.hpp b/src/alfalfa/AlfalfaPoint.hpp
@@ -69,7 +69,7 @@ namespace alfalfa {
     /**
      * Get id of point. By default this will be a version of the display name with spaces removed.
      */
-    boost::optional<std::string> id() const;
+    std::string id() const;
 
     /**
      * Set id of point. This is the component which will uniquely identify the point in the API.

diff --git a/src/alfalfa/AlfalfaPoint_Impl.hpp b/src/alfalfa/AlfalfaPoint_Impl.hpp
@@ -32,7 +32,7 @@ namespace alfalfa {
 
       boost::optional<std::string> units() const;
 
-      boost::optional<std::string> id() const;
+      std::string id() const;
 
       void setId(const std::string& id);
 
@@ -47,13 +47,11 @@ namespace alfalfa {
      private:
       static std::string toIdString(const std::string& str);
 
-      static bool isValidId(const std::string& id);
-
       boost::optional<AlfalfaComponent> m_input;
       boost::optional<AlfalfaComponent> m_output;
       std::string m_display_name;
       boost::optional<std::string> m_units;
-      boost::optional<std::string> m_id;
+      std::string m_id;
       bool m_optional = true;
 
       // configure logging

diff --git a/src/alfalfa/test/AlfalfaJSON_GTest.cpp b/src/alfalfa/test/AlfalfaJSON_GTest.cpp
@@ -1,5 +1,6 @@
 #include <fmt/core.h>
 #include <gtest/gtest.h>
+#include <stdexcept>
 #include <vector>
 
 #include "../AlfalfaJSON.hpp"
@@ -194,75 +195,46 @@ TEST(AlfalfaJSON, json_serialization) {
   const bool parsing_success = Json::parseFromStream(r_builder, ifs, &root, &formatted_errors);
   EXPECT_TRUE(parsing_success);
   EXPECT_EQ(alfalfa.toJSON(), root);
-  for (const AlfalfaPoint& point : alfalfa.points()) {
-    EXPECT_EQ(root[point.id().get()], point.toJSON());
+  for (Json::ArrayIndex i = 0; const auto& point : alfalfa.points()) {
+    EXPECT_EQ(root[i++], point.toJSON());
   }
 }
 
 TEST(AlfalfaJSON, point_exceptions_logging) {
-  const std::string ID_VALID_CHARS_MSG = "IDs can only contain letters, numbers, and the following special characters _-[]():";
-  const std::string DISPLAY_NAME_VALID_CHARS_MSG =
-    "Display name '{}' does not produce a valid point ID. Manually set a valid ID or export will fail.";
+  const std::string DISPLAY_NAME_EMPTY_ERROR = "Display name must have non-zero length";
+  const std::string ID_EMPTY_ERROR = "Id must have non-zero length";
   const std::string LOG_CHANNEL = "openstudio.AlfalfaPoint";
   StringStreamLogSink ss;
   ss.setLogLevel(Warn);
 
-  const AlfalfaPoint point("Point");
+  AlfalfaPoint point("Point");
   ASSERT_EQ(0, ss.logMessages().size());
   point.id();
   ASSERT_EQ(0, ss.logMessages().size());
 
   //Test logging in constructor
-  AlfalfaPoint invalid_point("Point$$$");
-  ASSERT_EQ(1, ss.logMessages().size());
-  LogMessage invalid_id_msg = ss.logMessages().at(0);
-  EXPECT_EQ(invalid_id_msg.logMessage(), fmt::format(fmt::runtime(DISPLAY_NAME_VALID_CHARS_MSG), "Point$$$"));
-  EXPECT_EQ(invalid_id_msg.logLevel(), Warn);
-  EXPECT_EQ(invalid_id_msg.logChannel(), LOG_CHANNEL);
-  ss.resetStringStream();
-  ASSERT_EQ(ss.logMessages().size(), 0);
-
-  // Test logging when getting id()
-  const boost::optional<std::string> invalid_point_id = invalid_point.id();
-  EXPECT_FALSE(invalid_point_id.is_initialized());
-  ASSERT_EQ(ss.logMessages().size(), 1);
-  invalid_id_msg = ss.logMessages().at(0);
-  EXPECT_EQ(invalid_id_msg.logMessage(), fmt::format(fmt::runtime(DISPLAY_NAME_VALID_CHARS_MSG), "Point$$$"));
-  EXPECT_EQ(invalid_id_msg.logLevel(), Warn);
-  EXPECT_EQ(invalid_id_msg.logChannel(), LOG_CHANNEL);
-  ss.resetStringStream();
-  ASSERT_EQ(ss.logMessages().size(), 0);
-
-  // Test exception handling in setId()
   EXPECT_THROW(
     {
       try {
-        invalid_point.setId("Point_123_$$$");
+        const AlfalfaPoint invalid_point("");
       } catch (const std::runtime_error& error) {
-        EXPECT_EQ(error.what(), ID_VALID_CHARS_MSG);
+        EXPECT_EQ(error.what(), DISPLAY_NAME_EMPTY_ERROR);
         throw;
       }
     },
     std::runtime_error);
-  ASSERT_EQ(ss.logMessages().size(), 0);
 
-  // Test exception handling in toJSON()
+  // Test exception handling in setId()
   EXPECT_THROW(
     {
       try {
-        Json::Value root = invalid_point.toJSON();
+        point.setId("");
       } catch (const std::runtime_error& error) {
-        EXPECT_EQ(error.what(), "Point requires a valid ID for export. " + ID_VALID_CHARS_MSG);
+        EXPECT_EQ(error.what(), ID_EMPTY_ERROR);
         throw;
       }
     },
     std::runtime_error);
-  ASSERT_EQ(ss.logMessages().size(), 1);
-  invalid_id_msg = ss.logMessages().at(0);
-  EXPECT_EQ(invalid_id_msg.logMessage(), fmt::format(fmt::runtime(DISPLAY_NAME_VALID_CHARS_MSG), "Point$$$"));
-  EXPECT_EQ(invalid_id_msg.logLevel(), Warn);
-  EXPECT_EQ(invalid_id_msg.logChannel(), LOG_CHANNEL);
-  ss.resetStringStream();
   ASSERT_EQ(ss.logMessages().size(), 0);
 
   //Test Calls work when provided with legal input
@@ -278,8 +250,7 @@ TEST(AlfalfaJSON, point_exceptions_logging) {
   // Test that changing display name changes the ID if an ID has not been set.
   valid_point.setDisplayName("Another Good Point");
   ASSERT_EQ(ss.logMessages().size(), 0);
-  ASSERT_TRUE(valid_point.id().is_initialized());
-  ASSERT_EQ(valid_point.id().get(), "Another_Good_Point");
+  ASSERT_EQ(valid_point.id(), "Another_Good_Point");
   ASSERT_EQ(ss.logMessages().size(), 0);
 
   Json::Value valid_json = valid_point.toJSON();
@@ -289,16 +260,15 @@ TEST(AlfalfaJSON, point_exceptions_logging) {
   const std::string new_id = "Another_Valid_Point(123)";
   valid_point.setId(new_id);
   ASSERT_EQ(ss.logMessages().size(), 0);
-  ASSERT_TRUE(valid_point.id().is_initialized());
-  ASSERT_EQ(valid_point.id().get(), new_id);
+  ASSERT_EQ(valid_point.id(), new_id);
 
   valid_json = valid_point.toJSON();
   ASSERT_EQ(ss.logMessages().size(), 0);
 
   // Test that once an ID is set, setting a new display name won't throw a warning.
   valid_point.setDisplayName("Valid Name, but Invalid Id $$$");
   ASSERT_EQ(ss.logMessages().size(), 0);
-  ASSERT_EQ(valid_point.id().get(), new_id);
+  ASSERT_EQ(valid_point.id(), new_id);
 
   valid_json = valid_point.toJSON();
   ASSERT_EQ(ss.logMessages().size(), 0);