Aggregation: Improve prefix fixup matching

Utilize the new array of top level collection URIs to determine if a
given URI in the response needs to have the aggregation prefix added.
This removes the need to check for specific collections like
/redfish/v1/UpdateService/FirmwareInventory which do not fit the
generic format of /redfish/v1/<collection>.

Future patches will use this same approach to improve the logic for
initially determining if and how a request should be aggregated.

This patch also adds a series of unit tests for the function
responsible for adding a prefix to a given URI.  Cases covered include
valid URIs that involve a selection of aggregated resources, top level
collection URIs, other invalid URIs, and URIs with a trailing "/".

Tested:
Unit tests pass.

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

4 files changed, 192 insertions, 87 deletions

diff --git a/redfish-core/include/aggregation_utils.hpp b/redfish-core/include/aggregation_utils.hpp
index bb1782f1b0..673b56873c 100644
--- a/redfish-core/include/aggregation_utils.hpp
+++ b/redfish-core/include/aggregation_utils.hpp
@@ -15,52 +15,52 @@
 
 namespace redfish
 {
-// Note that each URI actually begins with "/redfish/v1/"
+// Note that each URI actually begins with "/redfish/v1"
 // They've been omitted to save space and reduce search time
 constexpr std::array<std::string_view, 44> topCollections{
-    "AggregationService/Aggregates",
-    "AggregationService/AggregationSources",
-    "AggregationService/ConnectionMethods",
-    "Cables",
-    "Chassis",
-    "ComponentIntegrity",
-    "CompositionService/ActivePool",
-    "CompositionService/CompositionReservations",
-    "CompositionService/FreePool",
-    "CompositionService/ResourceBlocks",
-    "CompositionService/ResourceZones",
-    "EventService/Subscriptions",
-    "Fabrics",
-    "Facilities",
-    "JobService/Jobs",
-    "JobService/Log/Entries",
-    "KeyService/NVMeoFKeyPolicies",
-    "KeyService/NVMeoFSecrets",
-    "LicenseService/Licenses",
-    "Managers",
-    "NVMeDomains",
-    "PowerEquipment/ElectricalBuses",
-    "PowerEquipment/FloorPDUs",
-    "PowerEquipment/PowerShelves",
-    "PowerEquipment/RackPDUs",
-    "PowerEquipment/Switchgear",
-    "PowerEquipment/TransferSwitches",
-    "RegisteredClients",
-    "Registries",
-    "ResourceBlocks",
-    "Storage",
-    "StorageServices",
-    "StorageSystems",
-    "Systems",
-    "TaskService/Tasks",
-    "TelemetryService/LogService/Entries",
-    "TelemetryService/MetricDefinitions",
-    "TelemetryService/MetricReportDefinitions",
-    "TelemetryService/MetricReports",
-    "TelemetryService/Triggers",
-    "UpdateService/ClientCertificates",
-    "UpdateService/FirmwareInventory",
-    "UpdateService/RemoteServerCertificates",
-    "UpdateService/SoftwareInventory",
+    "/AggregationService/Aggregates",
+    "/AggregationService/AggregationSources",
+    "/AggregationService/ConnectionMethods",
+    "/Cables",
+    "/Chassis",
+    "/ComponentIntegrity",
+    "/CompositionService/ActivePool",
+    "/CompositionService/CompositionReservations",
+    "/CompositionService/FreePool",
+    "/CompositionService/ResourceBlocks",
+    "/CompositionService/ResourceZones",
+    "/EventService/Subscriptions",
+    "/Fabrics",
+    "/Facilities",
+    "/JobService/Jobs",
+    "/JobService/Log/Entries",
+    "/KeyService/NVMeoFKeyPolicies",
+    "/KeyService/NVMeoFSecrets",
+    "/LicenseService/Licenses",
+    "/Managers",
+    "/NVMeDomains",
+    "/PowerEquipment/ElectricalBuses",
+    "/PowerEquipment/FloorPDUs",
+    "/PowerEquipment/PowerShelves",
+    "/PowerEquipment/RackPDUs",
+    "/PowerEquipment/Switchgear",
+    "/PowerEquipment/TransferSwitches",
+    "/RegisteredClients",
+    "/Registries",
+    "/ResourceBlocks",
+    "/Storage",
+    "/StorageServices",
+    "/StorageSystems",
+    "/Systems",
+    "/TaskService/Tasks",
+    "/TelemetryService/LogService/Entries",
+    "/TelemetryService/MetricDefinitions",
+    "/TelemetryService/MetricReportDefinitions",
+    "/TelemetryService/MetricReports",
+    "/TelemetryService/Triggers",
+    "/UpdateService/ClientCertificates",
+    "/UpdateService/FirmwareInventory",
+    "/UpdateService/RemoteServerCertificates",
+    "/UpdateService/SoftwareInventory",
 };
 } // namespace redfish
diff --git a/redfish-core/include/redfish_aggregator.hpp b/redfish-core/include/redfish_aggregator.hpp
index 293dad2491..bd56d680fd 100644
--- a/redfish-core/include/redfish_aggregator.hpp
+++ b/redfish-core/include/redfish_aggregator.hpp
@@ -72,53 +72,66 @@ static void addPrefixToItem(nlohmann::json& item, std::string_view prefix)
     // version mismatches between aggregator and satellite BMCs.  For now
     // assume that the aggregator has all the schemas and versions that the
     // aggregated server has.
-    std::string collectionItem;
     if (crow::utility::readUrlSegments(thisUrl, "redfish", "v1", "JsonSchemas",
-                                       std::ref(collectionItem),
                                        crow::utility::OrMorePaths()))
     {
         BMCWEB_LOG_DEBUG << "Skipping JsonSchemas URI prefix fixing";
         return;
     }
 
-    // We don't need to add prefixes to these URIs since
-    // /redfish/v1/UpdateService/ itself is not a collection:
-    // /redfish/v1/UpdateService/FirmwareInventory
-    // /redfish/v1/UpdateService/SoftwareInventory
-    if (crow::utility::readUrlSegments(thisUrl, "redfish", "v1",
-                                       "UpdateService", "FirmwareInventory") ||
-        crow::utility::readUrlSegments(thisUrl, "redfish", "v1",
-                                       "UpdateService", "SoftwareInventory"))
+    // The first two segments should be "/redfish/v1".  We need to check that
+    // before we can search topCollections
+    if (!crow::utility::readUrlSegments(thisUrl, "redfish", "v1",
+                                        crow::utility::OrMorePaths()))
     {
-        BMCWEB_LOG_DEBUG << "Skipping UpdateService URI prefix fixing";
         return;
     }
 
-    // We need to add a prefix to FirmwareInventory and SoftwareInventory
-    // resources:
-    // /redfish/v1/UpdateService/FirmwareInventory/<id>
-    // /redfish/v1/UpdateService/SoftwareInventory/<id>
-    std::string collectionName;
-    if (crow::utility::readUrlSegments(
-            thisUrl, "redfish", "v1", "UpdateService", std::ref(collectionName),
-            std::ref(collectionItem), crow::utility::OrMorePaths()))
+    // Check array adding a segment each time until collection is identified
+    // Add prefix to segment after the collection
+    const boost::urls::segments_view urlSegments = thisUrl.segments();
+    bool addedPrefix = false;
+    boost::urls::url url("/");
+    boost::urls::segments_view::iterator it = urlSegments.begin();
+    const boost::urls::segments_view::const_iterator end = urlSegments.end();
+
+    // Skip past the leading "/redfish/v1"
+    it++;
+    it++;
+    for (; it != end; it++)
     {
-        collectionItem.insert(0, "_");
-        collectionItem.insert(0, prefix);
-        item = crow::utility::replaceUrlSegment(thisUrl, 4, collectionItem);
-        return;
+        // Trailing "/" will result in an empty segment.  In that case we need
+        // to return so we don't apply a prefix to top level collections such
+        // as "/redfish/v1/Chassis/"
+        if ((*it).empty())
+        {
+            return;
+        }
+
+        if (std::binary_search(topCollections.begin(), topCollections.end(),
+                               url.buffer()))
+        {
+            std::string collectionItem(prefix);
+            collectionItem += "_" + (*it);
+            url.segments().push_back(collectionItem);
+            it++;
+            addedPrefix = true;
+            break;
+        }
+
+        url.segments().push_back(*it);
+    }
+
+    // Finish constructing the URL here (if needed) to avoid additional checks
+    for (; it != end; it++)
+    {
+        url.segments().push_back(*it);
     }
 
-    // If we reach here then we need to add a prefix to resource IDs that take
-    // the general form of "/redfish/v1/<collection>/<id> such as:
-    // /redfish/v1/Chassis/foo
-    if (crow::utility::readUrlSegments(
-            thisUrl, "redfish", "v1", std::ref(collectionName),
-            std::ref(collectionItem), crow::utility::OrMorePaths()))
+    if (addedPrefix)
     {
-        collectionItem.insert(0, "_");
-        collectionItem.insert(0, prefix);
-        item = crow::utility::replaceUrlSegment(thisUrl, 3, collectionItem);
+        url.segments().insert(url.segments().begin(), {"redfish", "v1"});
+        item = url;
     }
 }
 
@@ -164,12 +177,6 @@ class RedfishAggregator
 
     RedfishAggregator()
     {
-        // TODO: Remove in future patches, this is here to prevent compiler
-        // warnings and get an accurate idea of the increase in the size of the
-        // binary.
-        BMCWEB_LOG_DEBUG << "There are " << topCollections.size()
-                         << " top level collections:";
-
         getSatelliteConfigs(constructorCallback);
 
         // Setup the retry policy to be used by Redfish Aggregation
@@ -776,7 +783,7 @@ class RedfishAggregator
     {
         using crow::utility::OrMorePaths;
         using crow::utility::readUrlSegments;
-        const boost::urls::url_view& url = thisReq.urlView;
+        const boost::urls::url_view url = thisReq.urlView;
         // UpdateService is the only top level resource that is not a Collection
         if (readUrlSegments(url, "redfish", "v1", "UpdateService"))
         {
diff --git a/scripts/generate_schema_collections.py b/scripts/generate_schema_collections.py
index c9f8b5956d..d6efa0efe2 100755
--- a/scripts/generate_schema_collections.py
+++ b/scripts/generate_schema_collections.py
@@ -266,17 +266,17 @@ def generate_top_collections():
             "\n"
             "namespace redfish\n"
             "{{\n"
-            '// Note that each URI actually begins with "/redfish/v1/"\n'
+            '// Note that each URI actually begins with "/redfish/v1"\n'
             "// They've been omitted to save space and reduce search time\n"
             "constexpr std::array<std::string_view, {TOTAL}> "
             "topCollections{{\n".format(WARNING=WARNING, TOTAL=TOTAL)
         )
 
         for collection in sorted(top_collections):
-            # All URIs start with "/redfish/v1/".  We can omit that portion to
+            # All URIs start with "/redfish/v1".  We can omit that portion to
             # save memory and reduce lookup time
             hpp_file.write(
-                '    "{}",\n'.format(collection.split("/redfish/v1/")[1])
+                '    "{}",\n'.format(collection.split("/redfish/v1")[1])
             )
 
         hpp_file.write("};\n} // namespace redfish\n")
diff --git a/test/redfish-core/include/redfish_aggregator_test.cpp b/test/redfish-core/include/redfish_aggregator_test.cpp
index e5c718148b..1b3f9244be 100644
--- a/test/redfish-core/include/redfish_aggregator_test.cpp
+++ b/test/redfish-core/include/redfish_aggregator_test.cpp
@@ -38,4 +38,102 @@ TEST(IsPropertyUri, UnsupportedPropertyReturnsFalse)
     EXPECT_FALSE(isPropertyUri("Id"));
 }
 
+TEST(addPrefixToItem, ValidURIs)
+{
+    nlohmann::json jsonRequest;
+    constexpr std::array validRoots{"Cables",
+                                    "Chassis",
+                                    "Fabrics",
+                                    "PowerEquipment/FloorPDUs",
+                                    "Systems",
+                                    "TaskService/Tasks",
+                                    "TelemetryService/LogService/Entries",
+                                    "UpdateService/SoftwareInventory"};
+
+    // We're only testing prefix fixing so it's alright that some of the
+    // resulting URIs will not actually be possible as defined by the schema
+    constexpr std::array validIDs{"1",
+                                  "1/",
+                                  "Test",
+                                  "Test/",
+                                  "Extra_Test",
+                                  "Extra_Test/",
+                                  "Extra_Test/Sensors",
+                                  "Extra_Test/Sensors/",
+                                  "Extra_Test/Sensors/power_sensor",
+                                  "Extra_Test/Sensors/power_sensor/"};
+
+    // Construct URIs which should have prefix fixing applied
+    for (const auto& root : validRoots)
+    {
+        for (const auto& id : validIDs)
+        {
+            std::string initial("/redfish/v1/" + std::string(root) + "/");
+            std::string correct(initial + "asdfjkl_" + std::string(id));
+            initial += id;
+            jsonRequest["@odata.id"] = initial;
+            addPrefixToItem(jsonRequest["@odata.id"], "asdfjkl");
+            EXPECT_EQ(jsonRequest["@odata.id"], correct);
+        }
+    }
+}
+
+TEST(addPrefixToItem, UnsupportedURIs)
+{
+    nlohmann::json jsonRequest;
+    constexpr std::array invalidRoots{
+        "FakeCollection",           "JsonSchemas",
+        "PowerEquipment",           "TaskService",
+        "TelemetryService/Entries", "UpdateService"};
+
+    constexpr std::array validIDs{"1",
+                                  "1/",
+                                  "Test",
+                                  "Test/",
+                                  "Extra_Test",
+                                  "Extra_Test/",
+                                  "Extra_Test/Sensors",
+                                  "Extra_Test/Sensors/",
+                                  "Extra_Test/Sensors/power_sensor",
+                                  "Extra_Test/Sensors/power_sensor/"};
+
+    // Construct URIs which should NOT have prefix fixing applied
+    for (const auto& root : invalidRoots)
+    {
+        for (const auto& id : validIDs)
+        {
+            std::string initial("/redfish/v1/" + std::string(root) + "/");
+            std::string correct(initial + "asdfjkl_" + std::string(id));
+            initial += id;
+            jsonRequest["@odata.id"] = initial;
+            addPrefixToItem(jsonRequest["@odata.id"], "asdfjkl");
+            EXPECT_EQ(jsonRequest["@odata.id"], initial);
+        }
+    }
+}
+
+TEST(addPrefixToItem, TopLevelCollections)
+{
+    nlohmann::json jsonRequest;
+    constexpr std::array validRoots{"Cables",
+                                    "Chassis/",
+                                    "Fabrics",
+                                    "JsonSchemas",
+                                    "PowerEquipment/FloorPDUs",
+                                    "Systems",
+                                    "TaskService/Tasks",
+                                    "TelemetryService/LogService/Entries",
+                                    "TelemetryService/LogService/Entries/",
+                                    "UpdateService/SoftwareInventory/"};
+
+    // Construct URIs for top level collections.  Prefixes should NOT be
+    // applied to any of the URIs
+    for (const auto& root : validRoots)
+    {
+        std::string initial("/redfish/v1/" + std::string(root));
+        jsonRequest["@odata.id"] = initial;
+        addPrefixToItem(jsonRequest["@odata.id"], "perfix");
+        EXPECT_EQ(jsonRequest["@odata.id"], initial);
+    }
+}
 } // namespace redfish