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

3 files changed, 39 insertions, 9 deletions

diff --git a/http/http_response.hpp b/http/http_response.hpp
index 71d3377461..38bebb5c8a 100644
--- a/http/http_response.hpp
+++ b/http/http_response.hpp
@@ -40,10 +40,23 @@ struct Response
     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 @@ struct Response
         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 @@ struct Response
     {
         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 @@ struct Response
                          << 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 219d9df796..d7f5819142 100644
--- a/include/async_resp.hpp
+++ b/include/async_resp.hpp
@@ -16,6 +16,8 @@ class AsyncResp
 {
   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 57dee3ce56..48d184b51d 100644
--- a/redfish-core/include/utils/query_param.hpp
+++ b/redfish-core/include/utils/query_param.hpp
@@ -841,14 +841,11 @@ inline void
     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>();
+        auto asyncResp = std::make_shared<bmcweb::AsyncResp>(
+            std::move(intermediateResponse));
+
         asyncResp->res.setCompleteRequestHandler(std::move(completionHandler));
-        asyncResp->res.jsonValue = std::move(intermediateResponse.jsonValue);
         auto multi = std::make_shared<MultiAsyncResp>(app, asyncResp);
-
         multi->startQuery(query);
         return;
     }