Aggregation: Fix and forward all responses

We only attempt prefix matching when we receive a 200 response.  For
the retry policy we consider 2XX and 404 to be valid codes.  Instead we
should forward all responses to the client and let them decide what
action they want to take.  As part of that we should always attempt to
do prefix fixing on the response.

Also fixes an oversight where we attempt to do prefix fixing on
"OriginOfCondition" properties.  That property is only a URI when it is
an Action parameter in a SubmitTestEvent request.  It is an object when
it appears as a response property.

Adds test cases for the above fixes.

Tested:
Tests pass. Queries to top level collections and aggregated URIs still
return expected results with added prefixes.

Signed-off-by: Carson Labrado <clabrado@google.com>
Change-Id: Ic76324ceab618160061be5f3c687af20a857fa25

2 files changed, 127 insertions, 37 deletions

diff --git a/redfish-core/include/redfish_aggregator.hpp b/redfish-core/include/redfish_aggregator.hpp
index 8923fdd2bb..b3252a3e16 100644
--- a/redfish-core/include/redfish_aggregator.hpp
+++ b/redfish-core/include/redfish_aggregator.hpp
@@ -35,7 +35,7 @@ constexpr std::array nonUriProperties{
     // "HostName", // Isn't actually a Redfish URI
     "Image",
     "MetricProperty",
-    "OriginOfCondition",
+    // "OriginOfCondition", // Is URI when in request, but is object in response
     "TaskMonitor",
     "target", // normal string, but target URI for POST to invoke an action
 };
@@ -191,15 +191,10 @@ class RedfishAggregator
     static inline boost::system::error_code
         aggregationRetryHandler(unsigned int respCode)
     {
-        // As a default, assume 200X is alright.
-        // We don't need to retry on a 404
-        if ((respCode < 200) || ((respCode >= 300) && (respCode != 404)))
-        {
-            return boost::system::errc::make_error_code(
-                boost::system::errc::result_out_of_range);
-        }
-
-        // Return 0 if the response code is valid
+        // Allow all response codes because we want to surface any satellite
+        // issue to the client
+        BMCWEB_LOG_DEBUG << "Received " << respCode
+                         << " response from satellite";
         return boost::system::errc::make_error_code(
             boost::system::errc::success);
     }
@@ -584,6 +579,19 @@ class RedfishAggregator
         }
     }
 
+  public:
+    RedfishAggregator(const RedfishAggregator&) = delete;
+    RedfishAggregator& operator=(const RedfishAggregator&) = delete;
+    RedfishAggregator(RedfishAggregator&&) = delete;
+    RedfishAggregator& operator=(RedfishAggregator&&) = delete;
+    ~RedfishAggregator() = default;
+
+    static RedfishAggregator& getInstance()
+    {
+        static RedfishAggregator handler;
+        return handler;
+    }
+
     // Processes the response returned by a satellite BMC and loads its
     // contents into asyncResp
     static void
@@ -591,14 +599,7 @@ class RedfishAggregator
                         const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
                         crow::Response& resp)
     {
-        // No processing needed if the request wasn't successful
-        if (resp.resultInt() != 200)
-        {
-            BMCWEB_LOG_DEBUG << "No need to parse satellite response";
-            asyncResp->res.stringResponse = std::move(resp.stringResponse);
-            return;
-        }
-
+        // We want to attempt prefix fixing regardless of response code
         // The resp will not have a json component
         // We need to create a json from resp's stringResponse
         if (resp.getHeaderValue("Content-Type") == "application/json")
@@ -614,9 +615,6 @@ class RedfishAggregator
 
             BMCWEB_LOG_DEBUG << "Successfully parsed satellite response";
 
-            // TODO: For collections we  want to add the satellite responses to
-            // our response rather than just straight overwriting them if our
-            // local handling was successful (i.e. would return a 200).
             addPrefixes(jsonVal, prefix);
 
             BMCWEB_LOG_DEBUG << "Added prefix to parsed satellite response";
@@ -630,8 +628,8 @@ class RedfishAggregator
         {
             if (!resp.body().empty())
             {
-                // We received a 200 response without the correct Content-Type
-                // so return an Operation Failed error
+                // We received a valid response without the correct
+                // Content-Type so return an Operation Failed error
                 BMCWEB_LOG_ERROR
                     << "Satellite response must be of type \"application/json\"";
                 messages::operationFailed(asyncResp->res);
@@ -762,19 +760,6 @@ class RedfishAggregator
         }
     } // End processCollectionResponse()
 
-  public:
-    RedfishAggregator(const RedfishAggregator&) = delete;
-    RedfishAggregator& operator=(const RedfishAggregator&) = delete;
-    RedfishAggregator(RedfishAggregator&&) = delete;
-    RedfishAggregator& operator=(RedfishAggregator&&) = delete;
-    ~RedfishAggregator() = default;
-
-    static RedfishAggregator& getInstance()
-    {
-        static RedfishAggregator handler;
-        return handler;
-    }
-
     // Entry point to Redfish Aggregation
     // Returns Result stating whether or not we still need to locally handle the
     // request
diff --git a/test/redfish-core/include/redfish_aggregator_test.cpp b/test/redfish-core/include/redfish_aggregator_test.cpp
index 1b3f9244be..665446c137 100644
--- a/test/redfish-core/include/redfish_aggregator_test.cpp
+++ b/test/redfish-core/include/redfish_aggregator_test.cpp
@@ -1,9 +1,13 @@
 #include "redfish_aggregator.hpp"
 
+#include <nlohmann/json.hpp>
+
 #include <gtest/gtest.h> // IWYU pragma: keep
 
 namespace redfish
 {
+namespace
+{
 
 TEST(IsPropertyUri, SupportedPropertyReturnsTrue)
 {
@@ -11,7 +15,6 @@ TEST(IsPropertyUri, SupportedPropertyReturnsTrue)
     EXPECT_TRUE(isPropertyUri("@odata.id"));
     EXPECT_TRUE(isPropertyUri("Image"));
     EXPECT_TRUE(isPropertyUri("MetricProperty"));
-    EXPECT_TRUE(isPropertyUri("OriginOfCondition"));
     EXPECT_TRUE(isPropertyUri("TaskMonitor"));
     EXPECT_TRUE(isPropertyUri("target"));
 }
@@ -29,6 +32,7 @@ TEST(IsPropertyUri, SpeificallyIgnoredPropertyReturnsFalse)
     EXPECT_FALSE(isPropertyUri("@odata.context"));
     EXPECT_FALSE(isPropertyUri("Destination"));
     EXPECT_FALSE(isPropertyUri("HostName"));
+    EXPECT_FALSE(isPropertyUri("OriginOfCondition"));
 }
 
 TEST(IsPropertyUri, UnsupportedPropertyReturnsFalse)
@@ -136,4 +140,105 @@ TEST(addPrefixToItem, TopLevelCollections)
         EXPECT_EQ(jsonRequest["@odata.id"], initial);
     }
 }
+
+TEST(addPrefixes, ParseJsonObject)
+{
+    nlohmann::json parameter;
+    parameter["Name"] = "/redfish/v1/Chassis/fakeName";
+    parameter["@odata.id"] = "/redfish/v1/Chassis/fakeChassis";
+
+    addPrefixes(parameter, "abcd");
+    EXPECT_EQ(parameter["Name"], "/redfish/v1/Chassis/fakeName");
+    EXPECT_EQ(parameter["@odata.id"], "/redfish/v1/Chassis/abcd_fakeChassis");
+}
+
+TEST(addPrefixes, ParseJsonArray)
+{
+    nlohmann::json array = nlohmann::json::parse(R"(
+    {
+      "Conditions": [
+        {
+          "Message": "This is a test",
+          "@odata.id": "/redfish/v1/Chassis/TestChassis"
+        },
+        {
+          "Message": "This is also a test",
+          "@odata.id": "/redfish/v1/Chassis/TestChassis2"
+        }
+      ]
+    }
+    )",
+                                                 nullptr, false);
+
+    addPrefixes(array, "5B42");
+    EXPECT_EQ(array["Conditions"][0]["@odata.id"],
+              "/redfish/v1/Chassis/5B42_TestChassis");
+    EXPECT_EQ(array["Conditions"][1]["@odata.id"],
+              "/redfish/v1/Chassis/5B42_TestChassis2");
+}
+
+TEST(addPrefixes, ParseJsonObjectNestedArray)
+{
+    nlohmann::json objWithArray = nlohmann::json::parse(R"(
+    {
+      "Status": {
+        "Conditions": [
+          {
+            "Message": "This is a test",
+            "MessageId": "Test",
+            "OriginOfCondition": {
+              "@odata.id": "/redfish/v1/Chassis/TestChassis"
+            },
+            "Severity": "Critical"
+          }
+        ],
+        "Health": "Critical",
+        "State": "Enabled"
+      }
+    }
+    )",
+                                                        nullptr, false);
+
+    addPrefixes(objWithArray, "5B42");
+    nlohmann::json& array = objWithArray["Status"]["Conditions"];
+    EXPECT_EQ(array[0]["OriginOfCondition"]["@odata.id"],
+              "/redfish/v1/Chassis/5B42_TestChassis");
+}
+
+// Attempts to perform prefix fixing on a response with response code "result".
+// Fixing should always occur
+void assertProcessResponse(unsigned result)
+{
+    nlohmann::json jsonResp;
+    jsonResp["@odata.id"] = "/redfish/v1/Chassis/TestChassis";
+    jsonResp["Name"] = "Test";
+
+    crow::Response resp;
+    resp.body() =
+        jsonResp.dump(2, ' ', true, nlohmann::json::error_handler_t::replace);
+    resp.addHeader("Content-Type", "application/json");
+    resp.result(result);
+
+    auto asyncResp = std::make_shared<bmcweb::AsyncResp>();
+    RedfishAggregator::processResponse("prefix", asyncResp, resp);
+
+    EXPECT_EQ(asyncResp->res.jsonValue["Name"], "Test");
+    EXPECT_EQ(asyncResp->res.jsonValue["@odata.id"],
+              "/redfish/v1/Chassis/prefix_TestChassis");
+    EXPECT_EQ(asyncResp->res.resultInt(), result);
+}
+
+TEST(processResponse, validResponseCodes)
+{
+    assertProcessResponse(100);
+    assertProcessResponse(200);
+    assertProcessResponse(204);
+    assertProcessResponse(300);
+    assertProcessResponse(404);
+    assertProcessResponse(405);
+    assertProcessResponse(500);
+    assertProcessResponse(507);
+}
+
+} // namespace
 } // namespace redfish