Query param: fix regression in top parameters

With the inclusion of ce8ea743055f1b82c60790db40aa3295e03bdf9c it looks
like we're now returning 400 error codes, with a response error of
QueryNotSupportedOnResource for resources which don't support top and
skip (like RegistryFile).  This would imply that the Query object NEEDS
a way to represent "user didn't provide us a skip/top parameter" which
arguably means this needs to go back to a std::optional<size_t>.

The error gets added from:
https://github.com/openbmc/bmcweb/blob/d5c80ad9c07b94465d8ea62d2b6f87c30cac765e/redfish-core/include/utils/query_param.hpp#L304
and appears to be a basic logic error in that now all queries assume
that the user provided top and skip, which fails for non-collections.

This commit moves that direction, changing the Query object back to
std::optional<size_t>.  This has the unintended consequence of now
putting the idea of "defaults" back into the per-delegated handlers. This
seems reasonable, as arguably the defaults for each individual collection
are going to be different, and at some point we might want to take advant
age of that.

Tested:
1. Tested on Romulus QEMU. All passed.
2. Tested on s7106, Validator passed.

Signed-off-by: Ed Tanous <edtanous@google.com>
Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: I9f912957d130694b281c6e391602de158eaddcb3

3 files changed, 68 insertions, 45 deletions

diff --git a/redfish-core/include/utils/query_param.hpp b/redfish-core/include/utils/query_param.hpp
index 3a70c6c0b4..55942c1880 100644
--- a/redfish-core/include/utils/query_param.hpp
+++ b/redfish-core/include/utils/query_param.hpp
@@ -60,10 +60,10 @@ struct Query
     ExpandType expandType = ExpandType::None;
 
     // Skip
-    size_t skip = 0;
+    std::optional<size_t> skip = std::nullopt;
 
     // Top
-    size_t top = maxEntriesPerPage;
+    std::optional<size_t> top = std::nullopt;
 };
 
 // The struct defines how resource handlers in redfish-core/lib/ can handle
@@ -109,14 +109,14 @@ inline Query delegate(const QueryCapabilities& queryCapabilities, Query& query)
     }
 
     // delegate top
-    if (queryCapabilities.canDelegateTop)
+    if (query.top && queryCapabilities.canDelegateTop)
     {
         delegated.top = query.top;
-        query.top = maxEntriesPerPage;
+        query.top = std::nullopt;
     }
 
     // delegate skip
-    if (queryCapabilities.canDelegateSkip)
+    if (query.skip && queryCapabilities.canDelegateSkip)
     {
         delegated.skip = query.skip;
         query.skip = 0;
@@ -196,12 +196,12 @@ inline QueryError getNumericParam(std::string_view value, size_t& param)
 
 inline QueryError getSkipParam(std::string_view value, Query& query)
 {
-    return getNumericParam(value, query.skip);
+    return getNumericParam(value, query.skip.emplace());
 }
 
 inline QueryError getTopParam(std::string_view value, Query& query)
 {
-    QueryError ret = getNumericParam(value, query.top);
+    QueryError ret = getNumericParam(value, query.top.emplace());
     if (ret != QueryError::Ok)
     {
         return ret;
@@ -566,6 +566,11 @@ class MultiAsyncResp : public std::enable_shared_from_this<MultiAsyncResp>
 
 inline void processTopAndSkip(const Query& query, crow::Response& res)
 {
+    if (!query.skip && !query.top)
+    {
+        // No work to do.
+        return;
+    }
     nlohmann::json::object_t* obj =
         res.jsonValue.get_ptr<nlohmann::json::object_t*>();
     if (obj == nullptr)
@@ -596,13 +601,18 @@ inline void processTopAndSkip(const Query& query, crow::Response& res)
         return;
     }
 
-    // Per section 7.3.1 of the Redfish specification, $skip is run before $top
-    // Can only skip as many values as we have
-    size_t skip = std::min(arr->size(), query.skip);
-    arr->erase(arr->begin(), arr->begin() + static_cast<ssize_t>(skip));
-
-    size_t top = std::min(arr->size(), query.top);
-    arr->erase(arr->begin() + static_cast<ssize_t>(top), arr->end());
+    if (query.skip)
+    {
+        // Per section 7.3.1 of the Redfish specification, $skip is run before
+        // $top Can only skip as many values as we have
+        size_t skip = std::min(arr->size(), *query.skip);
+        arr->erase(arr->begin(), arr->begin() + static_cast<ssize_t>(skip));
+    }
+    if (query.top)
+    {
+        size_t top = std::min(arr->size(), *query.top);
+        arr->erase(arr->begin() + static_cast<ssize_t>(top), arr->end());
+    }
 }
 
 inline void
@@ -631,7 +641,7 @@ inline void
         return;
     }
 
-    if (query.top <= maxEntriesPerPage || query.skip != 0)
+    if (query.top || query.skip)
     {
         processTopAndSkip(query, intermediateResponse);
     }
diff --git a/redfish-core/include/utils/query_param_test.cpp b/redfish-core/include/utils/query_param_test.cpp
index 2d8cfbb990..e53beade71 100644
--- a/redfish-core/include/utils/query_param_test.cpp
+++ b/redfish-core/include/utils/query_param_test.cpp
@@ -81,7 +81,7 @@ TEST(Delegate, TopNegative)
         .top = 42,
     };
     Query delegated = delegate(QueryCapabilities{}, query);
-    EXPECT_EQ(delegated.top, maxEntriesPerPage);
+    EXPECT_EQ(delegated.top, std::nullopt);
     EXPECT_EQ(query.top, 42);
 }
 
@@ -95,7 +95,7 @@ TEST(Delegate, TopPositive)
     };
     Query delegated = delegate(capabilities, query);
     EXPECT_EQ(delegated.top, 42);
-    EXPECT_EQ(query.top, maxEntriesPerPage);
+    EXPECT_EQ(query.top, std::nullopt);
 }
 
 TEST(Delegate, SkipNegative)
@@ -104,7 +104,7 @@ TEST(Delegate, SkipNegative)
         .skip = 42,
     };
     Query delegated = delegate(QueryCapabilities{}, query);
-    EXPECT_EQ(delegated.skip, 0);
+    EXPECT_EQ(delegated.skip, std::nullopt);
     EXPECT_EQ(query.skip, 42);
 }
 
@@ -358,4 +358,4 @@ TEST(QueryParams, FindNavigationReferencesLink)
 }
 
 } // namespace
-} // namespace redfish::query_param
\ No newline at end of file
+} // namespace redfish::query_param
diff --git a/redfish-core/lib/log_services.hpp b/redfish-core/lib/log_services.hpp
index 6285aa8603..3b1da0ecef 100644
--- a/redfish-core/lib/log_services.hpp
+++ b/redfish-core/lib/log_services.hpp
@@ -1169,6 +1169,10 @@ inline void requestRoutesJournalEventLogEntryCollection(App& app)
         {
             return;
         }
+        size_t top =
+            delegatedQuery.top.value_or(query_param::maxEntriesPerPage);
+        size_t skip = delegatedQuery.skip.value_or(0);
+
         // Collections don't include the static data added by SubRoute
         // because it has a duplicate entry for members
         asyncResp->res.jsonValue["@odata.type"] =
@@ -1226,8 +1230,7 @@ inline void requestRoutesJournalEventLogEntryCollection(App& app)
                 entryCount++;
                 // Handle paging using skip (number of entries to skip from the
                 // start) and top (number of entries to display)
-                if (entryCount <= delegatedQuery.skip ||
-                    entryCount > delegatedQuery.skip + delegatedQuery.top)
+                if (entryCount <= skip || entryCount > skip + top)
                 {
                     continue;
                 }
@@ -1236,11 +1239,11 @@ inline void requestRoutesJournalEventLogEntryCollection(App& app)
             }
         }
         asyncResp->res.jsonValue["Members@odata.count"] = entryCount;
-        if (delegatedQuery.skip + delegatedQuery.top < entryCount)
+        if (skip + top < entryCount)
         {
             asyncResp->res.jsonValue["Members@odata.nextLink"] =
                 "/redfish/v1/Systems/system/LogServices/EventLog/Entries?$skip=" +
-                std::to_string(delegatedQuery.skip + delegatedQuery.top);
+                std::to_string(skip + top);
         }
         });
 }
@@ -1892,13 +1895,16 @@ inline void requestRoutesSystemHostLoggerCollection(App& app)
             BMCWEB_LOG_ERROR << "fail to get host log file path";
             return;
         }
-
+        // If we weren't provided top and skip limits, use the defaults.
+        size_t skip = delegatedQuery.skip.value_or(0);
+        size_t top =
+            delegatedQuery.top.value_or(query_param::maxEntriesPerPage);
         size_t logCount = 0;
         // This vector only store the entries we want to expose that
         // control by skip and top.
         std::vector<std::string> logEntries;
-        if (!getHostLoggerEntries(hostLoggerFiles, delegatedQuery.skip,
-                                  delegatedQuery.top, logEntries, logCount))
+        if (!getHostLoggerEntries(hostLoggerFiles, skip, top, logEntries,
+                                  logCount))
         {
             messages::internalError(asyncResp->res);
             return;
@@ -1915,17 +1921,17 @@ inline void requestRoutesSystemHostLoggerCollection(App& app)
             for (size_t i = 0; i < logEntries.size(); i++)
             {
                 nlohmann::json::object_t hostLogEntry;
-                fillHostLoggerEntryJson(std::to_string(delegatedQuery.skip + i),
-                                        logEntries[i], hostLogEntry);
+                fillHostLoggerEntryJson(std::to_string(skip + i), logEntries[i],
+                                        hostLogEntry);
                 logEntryArray.push_back(std::move(hostLogEntry));
             }
 
             asyncResp->res.jsonValue["Members@odata.count"] = logCount;
-            if (delegatedQuery.skip + delegatedQuery.top < logCount)
+            if (skip + top < logCount)
             {
                 asyncResp->res.jsonValue["Members@odata.nextLink"] =
                     "/redfish/v1/Systems/system/LogServices/HostLogger/Entries?$skip=" +
-                    std::to_string(delegatedQuery.skip + delegatedQuery.top);
+                    std::to_string(skip + top);
             }
         }
         });
@@ -1971,7 +1977,7 @@ inline void requestRoutesSystemHostLoggerLogEntry(App& app)
         }
 
         size_t logCount = 0;
-        uint64_t top = 1;
+        size_t top = 1;
         std::vector<std::string> logEntries;
         // We can get specific entry by skip and top. For example, if we
         // want to get nth entry, we can set skip = n-1 and top = 1 to
@@ -2190,6 +2196,11 @@ inline void requestRoutesBMCJournalLogEntryCollection(App& app)
         {
             return;
         }
+
+        size_t skip = delegatedQuery.skip.value_or(0);
+        size_t top =
+            delegatedQuery.top.value_or(query_param::maxEntriesPerPage);
+
         // Collections don't include the static data added by SubRoute
         // because it has a duplicate entry for members
         asyncResp->res.jsonValue["@odata.type"] =
@@ -2223,8 +2234,7 @@ inline void requestRoutesBMCJournalLogEntryCollection(App& app)
             entryCount++;
             // Handle paging using skip (number of entries to skip from
             // the start) and top (number of entries to display)
-            if (entryCount <= delegatedQuery.skip ||
-                entryCount > delegatedQuery.skip + delegatedQuery.top)
+            if (entryCount <= skip || entryCount > skip + top)
             {
                 continue;
             }
@@ -2246,11 +2256,11 @@ inline void requestRoutesBMCJournalLogEntryCollection(App& app)
             logEntryArray.push_back(std::move(bmcJournalLogEntry));
         }
         asyncResp->res.jsonValue["Members@odata.count"] = entryCount;
-        if (delegatedQuery.skip + delegatedQuery.top < entryCount)
+        if (skip + top < entryCount)
         {
             asyncResp->res.jsonValue["Members@odata.nextLink"] =
                 "/redfish/v1/Managers/bmc/LogServices/Journal/Entries?$skip=" +
-                std::to_string(delegatedQuery.skip + delegatedQuery.top);
+                std::to_string(skip + top);
         }
         });
 }
@@ -3395,8 +3405,8 @@ static void getPostCodeForEntry(const std::shared_ptr<bmcweb::AsyncResp>& aResp,
 static void getPostCodeForBoot(const std::shared_ptr<bmcweb::AsyncResp>& aResp,
                                const uint16_t bootIndex,
                                const uint16_t bootCount,
-                               const uint64_t entryCount, const uint64_t skip,
-                               const uint64_t top)
+                               const uint64_t entryCount, size_t skip,
+                               size_t top)
 {
     crow::connections::systemBus->async_method_call(
         [aResp, bootIndex, bootCount, entryCount, skip,
@@ -3415,12 +3425,14 @@ static void getPostCodeForBoot(const std::shared_ptr<bmcweb::AsyncResp>& aResp,
         if (!postcode.empty())
         {
             endCount = entryCount + postcode.size();
-
-            if ((skip < endCount) && ((top + skip) > entryCount))
+            if (skip < endCount && (top + skip) > entryCount)
             {
-                uint64_t thisBootSkip = std::max(skip, entryCount) - entryCount;
+                uint64_t thisBootSkip =
+                    std::max(static_cast<uint64_t>(skip), entryCount) -
+                    entryCount;
                 uint64_t thisBootTop =
-                    std::min(top + skip, endCount) - entryCount;
+                    std::min(static_cast<uint64_t>(top + skip), endCount) -
+                    entryCount;
 
                 fillPostCodeEntry(aResp, postcode, bootIndex, 0, thisBootSkip,
                                   thisBootTop);
@@ -3449,7 +3461,7 @@ static void getPostCodeForBoot(const std::shared_ptr<bmcweb::AsyncResp>& aResp,
 
 static void
     getCurrentBootNumber(const std::shared_ptr<bmcweb::AsyncResp>& aResp,
-                         const uint64_t skip, const uint64_t top)
+                         size_t skip, size_t top)
 {
     uint64_t entryCount = 0;
     sdbusplus::asio::getProperty<uint16_t>(
@@ -3496,9 +3508,10 @@ inline void requestRoutesPostCodesEntryCollection(App& app)
             "Collection of POST Code Log Entries";
         asyncResp->res.jsonValue["Members"] = nlohmann::json::array();
         asyncResp->res.jsonValue["Members@odata.count"] = 0;
-
-        getCurrentBootNumber(asyncResp, delegatedQuery.skip,
-                             delegatedQuery.top);
+        size_t skip = delegatedQuery.skip.value_or(0);
+        size_t top =
+            delegatedQuery.top.value_or(query_param::maxEntriesPerPage);
+        getCurrentBootNumber(asyncResp, skip, top);
         });
 }