query: Fix default expand level with delegated
With delegate expand, the default expand level is -=
`queryCapabilities.canDelegateExpandLevel`. This creates an overlap of
expand process between delegate expand vs. default expand.
With
query.expandLevel = 2 ->
query.expandLevel = 1 and delegated.expandLevel = 1.
Both delegated and default expand will try to only expand of level one
instead of level 2 for the default.
The code in
https://github.com/openbmc/bmcweb/blob/479e899d5f57a67647f83b7f615d2c8565290bcf/redfish-core/include/utils/query_param.hpp#L583-L597
stated that the level with "@odata.id" + other property is treated as a
seperate level. So with `query.expandLevel = 1` it just loop through the
id that was already expanded and is noop.
Tested:
Before:
/redfish/v1/Chassis/BMC/Sensors?$expand=.($levels=2) returns
the same result as level=1. Needs level=3 to expand to the next level.
The RelatedItem in here doesn't get expanded with level=2.
```
wget -qO- 'http://localhost:80/redfish/v1/Chassis/BMC/Sensors?$expand=.($levels=1)'
...
{
"@odata.id": "/redfish/v1/Chassis/BMC/Sensors/temperature_DIMMXX",
"@odata.type": "#Sensor.v1_2_0.Sensor",
"Id": "temperature_DIMMXX",
"Name": "DIMMXX",
"Reading": 30.0,
"ReadingRangeMax": 127.0,
"ReadingRangeMin": -128.0,
"ReadingType": "Temperature",
"ReadingUnits": "Cel",
"RelatedItem": [
{
"@odata.id": "/redfish/v1/Systems/system/Memory/dimmXX"
}
],
"Status": {
"Health": "OK",
"State": "Enabled"
},
"Thresholds": {
"LowerCaution": {
"Reading": null
},
"LowerCritical": {
"Reading": null
},
"UpperCaution": {
"Reading": 93.0
},
"UpperCritical": {
"Reading": 95.0
}
}
}
],
"Members@odata.count": 242,
"Name": "Sensors"
}
```
After:
level=2 was able to expand to the next level.
Change-Id: I542177a94a33f8df7afbb68837f3a53b86140c86
Signed-off-by: Willy Tu <wltu@google.com>
diff --git a/redfish-core/include/query.hpp b/redfish-core/include/query.hpp
index 10a337b..c1ca38e 100644
--- a/redfish-core/include/query.hpp
+++ b/redfish-core/include/query.hpp
@@ -166,9 +166,9 @@
asyncResp->res.releaseCompleteRequestHandler();
asyncResp->res.setCompleteRequestHandler(
- [&app, handler(std::move(handler)),
- query{std::move(*queryOpt)}](crow::Response& resIn) mutable {
- processAllParams(app, query, handler, resIn);
+ [&app, handler(std::move(handler)), query{std::move(*queryOpt)},
+ delegated{delegated}](crow::Response& resIn) mutable {
+ processAllParams(app, query, delegated, handler, resIn);
});
return needToCallHandlers;
diff --git a/redfish-core/include/utils/query_param.hpp b/redfish-core/include/utils/query_param.hpp
index 71fcbd7..e5402d5 100644
--- a/redfish-core/include/utils/query_param.hpp
+++ b/redfish-core/include/utils/query_param.hpp
@@ -221,7 +221,6 @@
}
else
{
- query.expandLevel -= queryCapabilities.canDelegateExpandLevel;
delegated.expandLevel = queryCapabilities.canDelegateExpandLevel;
}
}
@@ -534,8 +533,8 @@
// with the keys from the jsonResponse object
inline void findNavigationReferencesRecursive(
ExpandType eType, nlohmann::json& jsonResponse,
- const nlohmann::json::json_pointer& p, int depth, bool inLinks,
- std::vector<ExpandNode>& out)
+ const nlohmann::json::json_pointer& p, int depth, int skipDepth,
+ bool inLinks, std::vector<ExpandNode>& out)
{
// If no expand is needed, return early
if (eType == ExpandType::None)
@@ -554,7 +553,7 @@
nlohmann::json::json_pointer newPtr = p / index;
BMCWEB_LOG_DEBUG << "Traversing response at " << newPtr.to_string();
findNavigationReferencesRecursive(eType, element, newPtr, depth,
- inLinks, out);
+ skipDepth, inLinks, out);
index++;
}
}
@@ -574,7 +573,10 @@
if (uri != nullptr)
{
BMCWEB_LOG_DEBUG << "Found " << *uri << " at " << p.to_string();
- out.push_back({p, *uri});
+ if (skipDepth == 0)
+ {
+ out.push_back({p, *uri});
+ }
return;
}
}
@@ -589,11 +591,22 @@
// "@odata.id" then that means we have entered a new level / expanded
// resource. We need to stop traversing if we're already at the desired
// depth
- if ((obj->size() > 1) && (depth == 0))
+ if (obj->size() > 1)
{
- return;
+ if (depth == 0)
+ {
+ return;
+ }
+ if (skipDepth > 0)
+ {
+ skipDepth--;
+ }
}
- newDepth--;
+
+ if (skipDepth == 0)
+ {
+ newDepth--;
+ }
}
// Loop the object and look for links
@@ -619,7 +632,8 @@
BMCWEB_LOG_DEBUG << "Traversing response at " << newPtr;
findNavigationReferencesRecursive(eType, element.second, newPtr,
- newDepth, localInLinks, out);
+ newDepth, skipDepth, localInLinks,
+ out);
}
}
@@ -630,13 +644,14 @@
// lands. May want to avoid forwarding query params when request is uptree from
// a top level collection.
inline std::vector<ExpandNode>
- findNavigationReferences(ExpandType eType, int depth,
+ findNavigationReferences(ExpandType eType, int depth, int skipDepth,
nlohmann::json& jsonResponse)
{
std::vector<ExpandNode> ret;
const nlohmann::json::json_pointer root = nlohmann::json::json_pointer("");
- findNavigationReferencesRecursive(eType, jsonResponse, root, depth, false,
- ret);
+ // SkipDepth +1 since we are skipping the root by default.
+ findNavigationReferencesRecursive(eType, jsonResponse, root, depth,
+ skipDepth + 1, false, ret);
return ret;
}
@@ -795,10 +810,11 @@
// Handles the very first level of Expand, and starts a chain of sub-queries
// for deeper levels.
- void startQuery(const Query& query)
+ void startQuery(const Query& query, const Query& delegated)
{
std::vector<ExpandNode> nodes = findNavigationReferences(
- query.expandType, query.expandLevel, finalRes->res.jsonValue);
+ query.expandType, query.expandLevel, delegated.expandLevel,
+ finalRes->res.jsonValue);
BMCWEB_LOG_DEBUG << nodes.size() << " nodes to traverse";
const std::optional<std::string> queryStr = formatQueryForExpand(query);
if (!queryStr)
@@ -960,7 +976,7 @@
}
inline void
- processAllParams(crow::App& app, const Query& query,
+ processAllParams(crow::App& app, const Query& query, const Query& delegated,
std::function<void(crow::Response&)>& completionHandler,
crow::Response& intermediateResponse)
{
@@ -998,7 +1014,7 @@
asyncResp->res.setCompleteRequestHandler(std::move(completionHandler));
auto multi = std::make_shared<MultiAsyncResp>(app, asyncResp);
- multi->startQuery(query);
+ multi->startQuery(query, delegated);
return;
}
diff --git a/test/redfish-core/include/utils/query_param_test.cpp b/test/redfish-core/include/utils/query_param_test.cpp
index 8c75241..7aa4ad5 100644
--- a/test/redfish-core/include/utils/query_param_test.cpp
+++ b/test/redfish-core/include/utils/query_param_test.cpp
@@ -53,7 +53,7 @@
EXPECT_FALSE(delegated.isOnly);
EXPECT_EQ(delegated.expandLevel, capabilities.canDelegateExpandLevel);
EXPECT_EQ(delegated.expandType, ExpandType::Both);
- EXPECT_EQ(query.expandLevel, 2);
+ EXPECT_EQ(query.expandLevel, 5);
}
TEST(Delegate, OnlyNegative)
@@ -659,23 +659,25 @@
"Foo" : {"@odata.id": "/foobar"}})"_json;
// Parsing as the root should net one entry
- EXPECT_THAT(findNavigationReferences(ExpandType::Both, 1, singleTreeNode),
- UnorderedElementsAre(
- ExpandNode{json::json_pointer("/Foo"), "/foobar"}));
+ EXPECT_THAT(
+ findNavigationReferences(ExpandType::Both, 1, 0, singleTreeNode),
+ UnorderedElementsAre(
+ ExpandNode{json::json_pointer("/Foo"), "/foobar"}));
// Parsing in Non-hyperlinks mode should net one entry
EXPECT_THAT(
- findNavigationReferences(ExpandType::NotLinks, 1, singleTreeNode),
+ findNavigationReferences(ExpandType::NotLinks, 1, 0, singleTreeNode),
UnorderedElementsAre(
ExpandNode{json::json_pointer("/Foo"), "/foobar"}));
// Searching for not types should return empty set
- EXPECT_TRUE(
- findNavigationReferences(ExpandType::None, 1, singleTreeNode).empty());
+ EXPECT_TRUE(findNavigationReferences(ExpandType::None, 1, 0, singleTreeNode)
+ .empty());
// Searching for hyperlinks only should return empty set
EXPECT_TRUE(
- findNavigationReferences(ExpandType::Links, 1, singleTreeNode).empty());
+ findNavigationReferences(ExpandType::Links, 1, 0, singleTreeNode)
+ .empty());
// Responses must include their "@odata.id" property for $expand to work
// correctly
@@ -686,7 +688,7 @@
// Should still find Foo
EXPECT_THAT(
- findNavigationReferences(ExpandType::NotLinks, 1, multiTreeNodes),
+ findNavigationReferences(ExpandType::NotLinks, 1, 0, multiTreeNodes),
UnorderedElementsAre(
ExpandNode{json::json_pointer("/Foo"), "/foobar"}));
}
@@ -702,21 +704,23 @@
"Links" : {"Sessions": {"@odata.id": "/foobar"}}})"_json;
// Parsing as the root should net one entry
- EXPECT_THAT(findNavigationReferences(ExpandType::Both, 1, singleLinkNode),
- UnorderedElementsAre(ExpandNode{
- json::json_pointer("/Links/Sessions"), "/foobar"}));
+ EXPECT_THAT(
+ findNavigationReferences(ExpandType::Both, 1, 0, singleLinkNode),
+ UnorderedElementsAre(
+ ExpandNode{json::json_pointer("/Links/Sessions"), "/foobar"}));
// Parsing in hyperlinks mode should net one entry
- EXPECT_THAT(findNavigationReferences(ExpandType::Links, 1, singleLinkNode),
- UnorderedElementsAre(ExpandNode{
- json::json_pointer("/Links/Sessions"), "/foobar"}));
+ EXPECT_THAT(
+ findNavigationReferences(ExpandType::Links, 1, 0, singleLinkNode),
+ UnorderedElementsAre(
+ ExpandNode{json::json_pointer("/Links/Sessions"), "/foobar"}));
// Searching for not types should return empty set
- EXPECT_TRUE(
- findNavigationReferences(ExpandType::None, 1, singleLinkNode).empty());
+ EXPECT_TRUE(findNavigationReferences(ExpandType::None, 1, 0, singleLinkNode)
+ .empty());
// Searching for non-hyperlinks only should return empty set
EXPECT_TRUE(
- findNavigationReferences(ExpandType::NotLinks, 1, singleLinkNode)
+ findNavigationReferences(ExpandType::NotLinks, 1, 0, singleLinkNode)
.empty());
}
@@ -754,10 +758,10 @@
// Expand has already occurred so we should not do anything
EXPECT_TRUE(
- findNavigationReferences(ExpandType::NotLinks, 1, expNode).empty());
+ findNavigationReferences(ExpandType::NotLinks, 1, 0, expNode).empty());
// Previous expand was only a single level so we should further expand
- EXPECT_THAT(findNavigationReferences(ExpandType::NotLinks, 2, expNode),
+ EXPECT_THAT(findNavigationReferences(ExpandType::NotLinks, 2, 0, expNode),
UnorderedElementsAre(
ExpandNode{json::json_pointer("/Members/0/Sensors"),
"/redfish/v1/Chassis/5B247A_Sat1/Sensors"},
@@ -768,12 +772,12 @@
json expNode2 = R"({"@odata.id" : "/redfish/v1"})"_json;
expNode2["Chassis"] = std::move(expNode);
EXPECT_TRUE(
- findNavigationReferences(ExpandType::NotLinks, 1, expNode2).empty());
+ findNavigationReferences(ExpandType::NotLinks, 1, 0, expNode2).empty());
EXPECT_TRUE(
- findNavigationReferences(ExpandType::NotLinks, 2, expNode2).empty());
+ findNavigationReferences(ExpandType::NotLinks, 2, 0, expNode2).empty());
// Previous expand was two levels so we should further expand
- EXPECT_THAT(findNavigationReferences(ExpandType::NotLinks, 3, expNode2),
+ EXPECT_THAT(findNavigationReferences(ExpandType::NotLinks, 3, 0, expNode2),
UnorderedElementsAre(
ExpandNode{json::json_pointer("/Chassis/Members/0/Sensors"),
"/redfish/v1/Chassis/5B247A_Sat1/Sensors"},
@@ -781,6 +785,39 @@
"/redfish/v1/Chassis/5B247A_Sat2/Sensors"}));
}
+TEST(QueryParams, DelegatedSkipExpanded)
+{
+ using nlohmann::json;
+
+ // Responses must include their "@odata.id" property for $expand to work
+ // correctly
+ json expNode = json::parse(R"(
+{
+ "@odata.id": "/redfish/v1",
+ "Foo": {
+ "@odata.id": "/foo"
+ },
+ "Bar": {
+ "@odata.id": "/bar",
+ "Foo": {
+ "@odata.id": "/barfoo"
+ }
+ }
+}
+)",
+ nullptr, false);
+
+ EXPECT_THAT(findNavigationReferences(ExpandType::NotLinks, 2, 0, expNode),
+ UnorderedElementsAre(
+ ExpandNode{json::json_pointer("/Foo"), "/foo"},
+ ExpandNode{json::json_pointer("/Bar/Foo"), "/barfoo"}));
+
+ // Skip the first expand level
+ EXPECT_THAT(findNavigationReferences(ExpandType::NotLinks, 1, 1, expNode),
+ UnorderedElementsAre(
+ ExpandNode{json::json_pointer("/Bar/Foo"), "/barfoo"}));
+}
+
TEST(QueryParams, PartiallyPreviouslyExpanded)
{
using nlohmann::json;
@@ -812,13 +849,13 @@
// The 5B247A_Sat1 Chassis was already expanded a single level so we should
// only want to expand the Local Chassis
EXPECT_THAT(
- findNavigationReferences(ExpandType::NotLinks, 1, expNode),
+ findNavigationReferences(ExpandType::NotLinks, 1, 0, expNode),
UnorderedElementsAre(ExpandNode{json::json_pointer("/Members/0"),
"/redfish/v1/Chassis/Local"}));
// The 5B247A_Sat1 Chassis was already expanded a single level so we should
// further expand it as well as the Local Chassis
- EXPECT_THAT(findNavigationReferences(ExpandType::NotLinks, 2, expNode),
+ EXPECT_THAT(findNavigationReferences(ExpandType::NotLinks, 2, 0, expNode),
UnorderedElementsAre(
ExpandNode{json::json_pointer("/Members/0"),
"/redfish/v1/Chassis/Local"},
@@ -830,19 +867,19 @@
"Systems": {"@odata.id": "/redfish/v1/Systems"}})"_json;
expNode2["Chassis"] = std::move(expNode);
- EXPECT_THAT(findNavigationReferences(ExpandType::NotLinks, 1, expNode2),
+ EXPECT_THAT(findNavigationReferences(ExpandType::NotLinks, 1, 0, expNode2),
UnorderedElementsAre(ExpandNode{json::json_pointer("/Systems"),
"/redfish/v1/Systems"}));
EXPECT_THAT(
- findNavigationReferences(ExpandType::NotLinks, 2, expNode2),
+ findNavigationReferences(ExpandType::NotLinks, 2, 0, expNode2),
UnorderedElementsAre(
ExpandNode{json::json_pointer("/Systems"), "/redfish/v1/Systems"},
ExpandNode{json::json_pointer("/Chassis/Members/0"),
"/redfish/v1/Chassis/Local"}));
EXPECT_THAT(
- findNavigationReferences(ExpandType::NotLinks, 3, expNode2),
+ findNavigationReferences(ExpandType::NotLinks, 3, 0, expNode2),
UnorderedElementsAre(
ExpandNode{json::json_pointer("/Systems"), "/redfish/v1/Systems"},
ExpandNode{json::json_pointer("/Chassis/Members/0"),