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
diff --git a/redfish-core/include/utils/query_param.hpp b/redfish-core/include/utils/query_param.hpp
index 3a70c6c..55942c1 100644
--- a/redfish-core/include/utils/query_param.hpp
+++ b/redfish-core/include/utils/query_param.hpp
@@ -60,10 +60,10 @@
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 @@
}
// 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 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 @@
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 @@
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 @@
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 2d8cfbb..e53bead 100644
--- a/redfish-core/include/utils/query_param_test.cpp
+++ b/redfish-core/include/utils/query_param_test.cpp
@@ -81,7 +81,7 @@
.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 @@
};
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 @@
.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 @@
}
} // 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 6285aa8..3b1da0e 100644
--- a/redfish-core/lib/log_services.hpp
+++ b/redfish-core/lib/log_services.hpp
@@ -1169,6 +1169,10 @@
{
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 @@
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 @@
}
}
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 @@
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 @@
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 @@
}
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 @@
{
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 @@
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 @@
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 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 @@
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
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 @@
"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);
});
}