Add type safety for NTP server objects

NTPServers is our last usage of nlohmann::json in a readJson unpack.
The capability and unit tests are left in place for that type in case we
need them in the future, but for now, document them as deprecated.

Tested: Redfish service validator passes.  Redfish protocol validator
passes most tests (1 known failure in SSE is unrelated to this change).

Change-Id: If4b2ea061a941cc23d47189af7ff453094dc7dca
Signed-off-by: Ed Tanous <ed@tanous.net>

2 files changed, 48 insertions, 22 deletions

diff --git a/redfish-core/include/utils/json_utils.hpp b/redfish-core/include/utils/json_utils.hpp
index d7d87a60cf..a013847872 100644
--- a/redfish-core/include/utils/json_utils.hpp
+++ b/redfish-core/include/utils/json_utils.hpp
@@ -151,7 +151,7 @@ UnpackErrorCode unpackValueVariant(nlohmann::json& j, std::string_view key,
 {
     if constexpr (Index < std::variant_size_v<std::variant<Args...>>)
     {
-        std::variant_alternative_t<Index, std::variant<Args...>> type;
+        std::variant_alternative_t<Index, std::variant<Args...>> type{};
         UnpackErrorCode unpack = unpackValueWithErrorCode(j, key, type);
         if (unpack == UnpackErrorCode::success)
         {
@@ -235,6 +235,23 @@ UnpackErrorCode unpackValueWithErrorCode(nlohmann::json& jsonValue,
             return UnpackErrorCode::invalidType;
         }
     }
+    else if constexpr (IsVector<Type>::value)
+    {
+        nlohmann::json::object_t* obj =
+            jsonValue.get_ptr<nlohmann::json::object_t*>();
+        if (obj == nullptr)
+        {
+            return UnpackErrorCode::invalidType;
+        }
+
+        for (const auto& val : *obj)
+        {
+            value.emplace_back();
+            ret = unpackValueWithErrorCode<typename Type::value_type>(
+                      val, key, value.back()) &&
+                  ret;
+        }
+    }
     else
     {
         using JsonType = std::add_const_t<std::add_pointer_t<Type>>;
@@ -403,7 +420,6 @@ using UnpackVariant = std::variant<
     bool*,
     double*,
     std::string*,
-    nlohmann::json*,
     nlohmann::json::object_t*,
     std::variant<std::string, std::nullptr_t>*,
     std::variant<uint8_t, std::nullptr_t>*,
@@ -425,7 +441,6 @@ using UnpackVariant = std::variant<
     //std::vector<bool>*,
     std::vector<double>*,
     std::vector<std::string>*,
-    std::vector<nlohmann::json>*,
     std::vector<nlohmann::json::object_t>*,
     std::optional<uint8_t>*,
     std::optional<uint16_t>*,
@@ -437,7 +452,6 @@ using UnpackVariant = std::variant<
     std::optional<bool>*,
     std::optional<double>*,
     std::optional<std::string>*,
-    std::optional<nlohmann::json>*,
     std::optional<nlohmann::json::object_t>*,
     std::optional<std::vector<uint8_t>>*,
     std::optional<std::vector<uint16_t>>*,
@@ -449,7 +463,6 @@ using UnpackVariant = std::variant<
     //std::optional<std::vector<bool>>*,
     std::optional<std::vector<double>>*,
     std::optional<std::vector<std::string>>*,
-    std::optional<std::vector<nlohmann::json>>*,
     std::optional<std::vector<nlohmann::json::object_t>>*,
     std::optional<std::variant<std::string, std::nullptr_t>>*,
     std::optional<std::variant<uint8_t, std::nullptr_t>>*,
@@ -462,7 +475,15 @@ using UnpackVariant = std::variant<
     std::optional<std::variant<double, std::nullptr_t>>*,
     std::optional<std::variant<bool, std::nullptr_t>>*,
     std::optional<std::vector<std::variant<nlohmann::json::object_t, std::nullptr_t>>>*,
-    std::optional<std::variant<nlohmann::json::object_t, std::nullptr_t>>*
+    std::optional<std::vector<std::variant<std::string, nlohmann::json::object_t, std::nullptr_t>>>*,
+
+    // Note, these types are kept for historical completeness, but should not be used,
+    // As they do not provide object type safety.  Instead, rely on nlohmann::json::object_t
+    // Will be removed Q2 2025
+    nlohmann::json*,
+    std::optional<std::vector<nlohmann::json>>*,
+    std::vector<nlohmann::json>*,
+    std::optional<nlohmann::json>*
 >;
 // clang-format on
 
diff --git a/redfish-core/lib/network_protocol.hpp b/redfish-core/lib/network_protocol.hpp
index 00b5d9e309..747abbf97d 100644
--- a/redfish-core/lib/network_protocol.hpp
+++ b/redfish-core/lib/network_protocol.hpp
@@ -259,17 +259,24 @@ inline void handleNTPProtocolEnabled(
                     "TimeSyncMethod", "NTP/ProtocolEnabled", timeSyncMethod);
 }
 
+// Redfish states that ip addresses can be
+// string, to set a value
+// null, to delete the value
+// object_t, empty json object, to ignore the value
+using IpAddress =
+    std::variant<std::string, nlohmann::json::object_t, std::nullptr_t>;
+
 inline void
     handleNTPServersPatch(const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
-                          const std::vector<nlohmann::json>& ntpServerObjects,
+                          const std::vector<IpAddress>& ntpServerObjects,
                           std::vector<std::string> currentNtpServers)
 {
     std::vector<std::string>::iterator currentNtpServer =
         currentNtpServers.begin();
     for (size_t index = 0; index < ntpServerObjects.size(); index++)
     {
-        const nlohmann::json& ntpServer = ntpServerObjects[index];
-        if (ntpServer.is_null())
+        const IpAddress& ntpServer = ntpServerObjects[index];
+        if (std::holds_alternative<std::nullptr_t>(ntpServer))
         {
             // Can't delete an item that doesn't exist
             if (currentNtpServer == currentNtpServers.end())
@@ -284,22 +291,22 @@ inline void
             continue;
         }
         const nlohmann::json::object_t* ntpServerObject =
-            ntpServer.get_ptr<const nlohmann::json::object_t*>();
+            std::get_if<nlohmann::json::object_t>(&ntpServer);
         if (ntpServerObject != nullptr)
         {
             if (!ntpServerObject->empty())
             {
-                messages::propertyValueNotInList(asyncResp->res, ntpServer,
-                                                 "NTP/NTPServers/" +
-                                                     std::to_string(index));
+                messages::propertyValueNotInList(
+                    asyncResp->res, *ntpServerObject,
+                    "NTP/NTPServers/" + std::to_string(index));
                 return;
             }
             // Can't retain an item that doesn't exist
             if (currentNtpServer == currentNtpServers.end())
             {
-                messages::propertyValueOutOfRange(asyncResp->res, ntpServer,
-                                                  "NTP/NTPServers/" +
-                                                      std::to_string(index));
+                messages::propertyValueOutOfRange(
+                    asyncResp->res, *ntpServerObject,
+                    "NTP/NTPServers/" + std::to_string(index));
 
                 return;
             }
@@ -308,13 +315,10 @@ inline void
             continue;
         }
 
-        const std::string* ntpServerStr =
-            ntpServer.get_ptr<const std::string*>();
+        const std::string* ntpServerStr = std::get_if<std::string>(&ntpServer);
         if (ntpServerStr == nullptr)
         {
-            messages::propertyValueTypeError(asyncResp->res, ntpServer,
-                                             "NTP/NTPServers/" +
-                                                 std::to_string(index));
+            messages::internalError(asyncResp->res);
             return;
         }
         if (currentNtpServer == currentNtpServers.end())
@@ -470,7 +474,8 @@ inline void handleManagersNetworkProtocolPatch(
         return;
     }
     std::optional<std::string> newHostName;
-    std::optional<std::vector<nlohmann::json>> ntpServerObjects;
+
+    std::optional<std::vector<IpAddress>> ntpServerObjects;
     std::optional<bool> ntpEnabled;
     std::optional<bool> ipmiEnabled;
     std::optional<bool> sshEnabled;