Fix sensor override

c1d019a6056a2a0ef50e577b3139ab5a8dc49355 Sensor Optimization

Recently changed the way Ids were calculated in the sensor subsystem.
Unfortunately, it wasn't clear to the author that this would effect the
sensor override system, which relies on matching up a member ID with a
dbus path, and was broken by this change.

This commit breaks out the code to calculate the type and name from a
given URI segment into a helper method.

Tested: Inspection only.  Very few systems support this feature.  Code appears more correct than previously, which is known broken, so the lack of testing here seems reasonable.

Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I9aa8099a947a36b5ce914bc07ae60f1ebf0d209b

3 files changed, 69 insertions, 16 deletions

diff --git a/meson.build b/meson.build
index 967b30933f..997dd1d1d6 100644
--- a/meson.build
+++ b/meson.build
@@ -367,6 +367,7 @@ srcfiles_unittest = files(
   'test/redfish-core/include/utils/stl_utils_test.cpp',
   'test/redfish-core/include/utils/time_utils_test.cpp',
   'test/redfish-core/lib/chassis_test.cpp',
+  'test/redfish-core/lib/sensors_test.cpp',
   'test/redfish-core/lib/log_services_dump_test.cpp',
   'test/redfish-core/lib/service_root_test.cpp',
   'test/redfish-core/lib/thermal_subsystem_test.cpp',
diff --git a/redfish-core/lib/sensors.hpp b/redfish-core/lib/sensors.hpp
index 627ba7e50e..e53a7d2809 100644
--- a/redfish-core/lib/sensors.hpp
+++ b/redfish-core/lib/sensors.hpp
@@ -21,6 +21,7 @@
 #include <boost/algorithm/string/classification.hpp>
 #include <boost/algorithm/string/find.hpp>
 #include <boost/algorithm/string/predicate.hpp>
+#include <boost/algorithm/string/replace.hpp>
 #include <boost/algorithm/string/split.hpp>
 #include <boost/range/algorithm/replace_copy_if.hpp>
 #include <dbus_singleton.hpp>
@@ -732,7 +733,7 @@ inline void objectPropertiesToJson(
 {
     if (chassisSubNode == sensors::node::sensors)
     {
-        std::string subNodeEscaped(chassisSubNode);
+        std::string subNodeEscaped(sensorType);
         subNodeEscaped.erase(
             std::remove(subNodeEscaped.begin(), subNodeEscaped.end(), '_'),
             subNodeEscaped.end());
@@ -2578,6 +2579,24 @@ inline bool
     return false;
 }
 
+inline std::pair<std::string, std::string>
+    splitSensorNameAndType(std::string_view sensorId)
+{
+    size_t index = sensorId.find('_');
+    if (index == std::string::npos)
+    {
+        return std::make_pair<std::string, std::string>("", "");
+    }
+    std::string sensorType{sensorId.substr(0, index)};
+    std::string sensorName{sensorId.substr(index + 1)};
+    // fan_pwm and fan_tach need special handling
+    if (sensorType == "fantach" || sensorType == "fanpwm")
+    {
+        sensorType.insert(3, 1, '_');
+    }
+    return std::make_pair(sensorType, sensorName);
+}
+
 /**
  * @brief Entry point for overriding sensor values of given sensor
  *
@@ -2634,8 +2653,10 @@ inline void setSensorsOverride(
         for (const auto& item : overrideMap)
         {
             const auto& sensor = item.first;
-            if (!findSensorNameUsingSensorPath(sensor, *sensorsList,
-                                               *sensorNames))
+            std::pair<std::string, std::string> sensorNameType =
+                splitSensorNameAndType(sensor);
+            if (!findSensorNameUsingSensorPath(sensorNameType.second,
+                                               *sensorsList, *sensorNames))
             {
                 BMCWEB_LOG_INFO << "Unable to find memberId " << item.first;
                 messages::resourceNotFound(sensorAsyncResp->asyncResp->res,
@@ -2876,33 +2897,28 @@ inline void handleSensorGet(App& app, const crow::Request& req,
     {
         return;
     }
-    size_t index = sensorId.find('_');
-    if (index == std::string::npos)
+    std::pair<std::string, std::string> nameType =
+        splitSensorNameAndType(sensorId);
+    if (nameType.first.empty() || nameType.second.empty())
     {
         messages::resourceNotFound(asyncResp->res, sensorId, "Sensor");
         return;
     }
+
     asyncResp->res.jsonValue["@odata.id"] = crow::utility::urlFromPieces(
         "redfish", "v1", "Chassis", chassisId, "Sensors", sensorId);
-    std::string sensorType = sensorId.substr(0, index);
-    std::string sensorName = sensorId.substr(index + 1);
-    // fan_pwm and fan_tach need special handling
-    if (sensorType == "fantach" || sensorType == "fanpwm")
-    {
-        sensorType.insert(3, 1, '_');
-    }
 
     BMCWEB_LOG_DEBUG << "Sensor doGet enter";
 
     const std::array<const char*, 1> interfaces = {
         "xyz.openbmc_project.Sensor.Value"};
-    std::string sensorPath =
-        "/xyz/openbmc_project/sensors/" + sensorType + '/' + sensorName;
+    std::string sensorPath = "/xyz/openbmc_project/sensors/" + nameType.first +
+                             '/' + nameType.second;
     // Get a list of all of the sensors that implement Sensor.Value
     // and get the path and service name associated with the sensor
     crow::connections::systemBus->async_method_call(
-        [asyncResp, sensorPath,
-         sensorName](const boost::system::error_code ec,
+        [asyncResp,
+         sensorPath](const boost::system::error_code ec,
                      const ::dbus::utility::MapperGetObject& subtree) {
         BMCWEB_LOG_DEBUG << "respHandler1 enter";
         if (ec)
diff --git a/test/redfish-core/lib/sensors_test.cpp b/test/redfish-core/lib/sensors_test.cpp
new file mode 100644
index 0000000000..591b911690
--- /dev/null
+++ b/test/redfish-core/lib/sensors_test.cpp
@@ -0,0 +1,36 @@
+#include "sensors.hpp"
+
+#include <gmock/gmock.h> // IWYU pragma: keep
+#include <gtest/gtest.h> // IWYU pragma: keep
+
+// IWYU pragma: no_include <gtest/gtest-message.h>
+// IWYU pragma: no_include <gtest/gtest-test-part.h>
+// IWYU pragma: no_include "gtest/gtest_pred_impl.h"
+// IWYU pragma: no_include <gmock/gmock-matchers.h>
+// IWYU pragma: no_include <gtest/gtest-matchers.h>
+
+namespace redfish
+{
+namespace
+{
+
+TEST(SplitSensorNameAndType, Type)
+{
+    EXPECT_EQ(splitSensorNameAndType("fantach_foo_1").first, "fan_tach");
+    EXPECT_EQ(splitSensorNameAndType("temperature_foo2").first, "temperature");
+}
+
+TEST(SplitSensorNameAndType, Name)
+{
+    EXPECT_EQ(splitSensorNameAndType("fantach_foo_1").second, "foo_1");
+    EXPECT_EQ(splitSensorNameAndType("temperature_foo2").second, "foo2");
+}
+
+TEST(SplitSensorNameAndType, Error)
+{
+    EXPECT_TRUE(splitSensorNameAndType("fantach").first.empty());
+    EXPECT_TRUE(splitSensorNameAndType("temperature").second.empty());
+}
+
+} // namespace
+} // namespace redfish