Preserve headers from the root object on expand
There is a bug where, when running an expand query, headers from the
response object get dropped. These headers include OData.type, and the
newly minted Link header, as well as possible others.
This was actually noted in a TODO, although the author of the TODO,
didn't fully understand the consequences at the time, and thought there
was no functional impact.
To resolve this, this commit resolves the TODO, and allows the Response
object to be moved out, instead of having to create a new one, which
preserves all the response state. To do this, it creates a move
constructor on the Response object for this use. The move constructor
is relatively benign, with one caveat, that we might be moving while in
a completion handler (as is the most common use). So both the existing
operator= and Response() move constructor are amended to handle this
case, and simply null out the response object in the copied object,
which would be correct behavior, given that each callback handler should
only be called once per Response object.
Tested:
curl --insecure --user root:0penBmc -vvvv
https://192.168.7.2/redfish/v1\?\$expand\=\*\(\$levels\=2\)
returns the same body as previously, now with the included:
OData-Version: 4.0
Allow: Get
headers in the response.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I221364dd4304903b37cacb1386f621b073a0a891
diff --git a/http/http_response.hpp b/http/http_response.hpp
index 71d3377..38bebb5 100644
--- a/http/http_response.hpp
+++ b/http/http_response.hpp
@@ -40,10 +40,23 @@
Response() : stringResponse(response_type{})
{}
+ Response(Response&& res) noexcept :
+ stringResponse(std::move(res.stringResponse)), completed(res.completed)
+ {
+ jsonValue = std::move(res.jsonValue);
+ // See note in operator= move handler for why this is needed.
+ if (!res.completed)
+ {
+ completeRequestHandler = std::move(res.completeRequestHandler);
+ res.completeRequestHandler = nullptr;
+ }
+ isAliveHelper = res.isAliveHelper;
+ res.isAliveHelper = nullptr;
+ }
+
~Response() = default;
Response(const Response&) = delete;
- Response(Response&&) = delete;
Response& operator=(const Response& r) = delete;
@@ -58,10 +71,23 @@
stringResponse = std::move(r.stringResponse);
r.stringResponse.emplace(response_type{});
jsonValue = std::move(r.jsonValue);
+
+ // Only need to move completion handler if not already completed
+ // Note, there are cases where we might move out of a Response object
+ // while in a completion handler for that response object. This check
+ // is intended to prevent destructing the functor we are currently
+ // executing from in that case.
+ if (!r.completed)
+ {
+ completeRequestHandler = std::move(r.completeRequestHandler);
+ r.completeRequestHandler = nullptr;
+ }
+ else
+ {
+ completeRequestHandler = nullptr;
+ }
completed = r.completed;
- completeRequestHandler = std::move(r.completeRequestHandler);
isAliveHelper = std::move(r.isAliveHelper);
- r.completeRequestHandler = nullptr;
r.isAliveHelper = nullptr;
return *this;
}
@@ -160,6 +186,10 @@
{
BMCWEB_LOG_DEBUG << this << " setting completion handler";
completeRequestHandler = std::move(handler);
+
+ // Now that we have a new completion handler attached, we're no longer
+ // complete
+ completed = false;
}
std::function<void(Response&)> releaseCompleteRequestHandler()
@@ -168,6 +198,7 @@
<< static_cast<bool>(completeRequestHandler);
std::function<void(Response&)> ret = completeRequestHandler;
completeRequestHandler = nullptr;
+ completed = true;
return ret;
}
diff --git a/include/async_resp.hpp b/include/async_resp.hpp
index 219d9df..d7f5819 100644
--- a/include/async_resp.hpp
+++ b/include/async_resp.hpp
@@ -16,6 +16,8 @@
{
public:
AsyncResp() = default;
+ explicit AsyncResp(crow::Response&& resIn) : res(std::move(resIn))
+ {}
AsyncResp(const AsyncResp&) = delete;
AsyncResp(AsyncResp&&) = delete;
diff --git a/redfish-core/include/utils/query_param.hpp b/redfish-core/include/utils/query_param.hpp
index 57dee3c..48d184b 100644
--- a/redfish-core/include/utils/query_param.hpp
+++ b/redfish-core/include/utils/query_param.hpp
@@ -841,14 +841,11 @@
if (query.expandType != ExpandType::None)
{
BMCWEB_LOG_DEBUG << "Executing expand query";
- // TODO(ed) this is a copy of the response object. Admittedly,
- // we're inherently doing something inefficient, but we shouldn't
- // have to do a full copy
- auto asyncResp = std::make_shared<bmcweb::AsyncResp>();
- asyncResp->res.setCompleteRequestHandler(std::move(completionHandler));
- asyncResp->res.jsonValue = std::move(intermediateResponse.jsonValue);
- auto multi = std::make_shared<MultiAsyncResp>(app, asyncResp);
+ auto asyncResp = std::make_shared<bmcweb::AsyncResp>(
+ std::move(intermediateResponse));
+ asyncResp->res.setCompleteRequestHandler(std::move(completionHandler));
+ auto multi = std::make_shared<MultiAsyncResp>(app, asyncResp);
multi->startQuery(query);
return;
}