Fix bugprone-unchecked-optional-access findings

Clang-tidy has the aforementioned check, which shows a few places in the
core where we ignored the required optional checks.  Fix all uses.
Note, we cannot enable the check that this time because of some weird
code in health.hpp that crashes tidy[1].  That will need to be a future
improvement.

There are tests that call something like
ASSERT(optional)
EXPECT(optional->foo())

While this isn't an actual violation, clang-tidy doesn't seem to be
smart enough to deal with it, so add some explicit checks.

[1] https://github.com/llvm/llvm-project/issues/55530

Tested: Redfish service validator passes.

Change-Id: Ied579cd0b957efc81aff5d5d1091a740a7a2d7e3
Signed-off-by: Ed Tanous <edtanous@google.com>

4 files changed, 59 insertions, 35 deletions

diff --git a/http/http2_connection.hpp b/http/http2_connection.hpp
index a63b234575..27df36e82f 100644
--- a/http/http2_connection.hpp
+++ b/http/http2_connection.hpp
@@ -154,8 +154,8 @@ class HTTP2Connection :
         completeResponseFields(thisReq, thisRes);
         thisRes.addHeader(boost::beast::http::field::date, getCachedDateStr());
 
-        boost::beast::http::fields& fields = thisRes.stringResponse->base();
-        std::string code = std::to_string(thisRes.stringResponse->result_int());
+        boost::beast::http::fields& fields = thisRes.stringResponse.base();
+        std::string code = std::to_string(thisRes.stringResponse.result_int());
         hdr.emplace_back(headerFromStringViews(":status", code));
         for (const boost::beast::http::fields::value_type& header : fields)
         {
diff --git a/http/http_client.hpp b/http/http_client.hpp
index d82c566a06..71fb885551 100644
--- a/http/http_client.hpp
+++ b/http/http_client.hpp
@@ -125,6 +125,7 @@ struct PendingRequest
     {}
 };
 
+namespace http = boost::beast::http;
 class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
 {
   private:
@@ -137,10 +138,9 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
     uint32_t connId;
 
     // Data buffers
-    boost::beast::http::request<boost::beast::http::string_body> req;
-    std::optional<
-        boost::beast::http::response_parser<boost::beast::http::string_body>>
-        parser;
+    http::request<http::string_body> req;
+    using parser_type = http::response_parser<http::string_body>;
+    std::optional<parser_type> parser;
     boost::beast::flat_static_buffer<httpReadBufferSize> buffer;
     Response res;
 
@@ -329,9 +329,10 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
     {
         state = ConnState::recvInProgress;
 
-        parser.emplace(std::piecewise_construct, std::make_tuple());
+        parser_type& thisParser = parser.emplace(std::piecewise_construct,
+                                                 std::make_tuple());
 
-        parser->body_limit(connPolicy->requestByteLimit);
+        thisParser.body_limit(connPolicy->requestByteLimit);
 
         timer.expires_after(std::chrono::seconds(30));
         timer.async_wait(std::bind_front(onTimeout, weak_from_this()));
@@ -340,14 +341,14 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
         if (sslConn)
         {
             boost::beast::http::async_read(
-                *sslConn, buffer, *parser,
+                *sslConn, buffer, thisParser,
                 std::bind_front(&ConnectionInfo::afterRead, this,
                                 shared_from_this()));
         }
         else
         {
             boost::beast::http::async_read(
-                conn, buffer, *parser,
+                conn, buffer, thisParser,
                 std::bind_front(&ConnectionInfo::afterRead, this,
                                 shared_from_this()));
         }
@@ -375,6 +376,10 @@ class ConnectionInfo : public std::enable_shared_from_this<ConnectionInfo>
         }
         BMCWEB_LOG_DEBUG("recvMessage() bytes transferred: {}",
                          bytesTransferred);
+        if (!parser)
+        {
+            return;
+        }
         BMCWEB_LOG_DEBUG("recvMessage() data: {}", parser->get().body());
 
         unsigned int respCode = parser->get().result_int();
diff --git a/http/http_connection.hpp b/http/http_connection.hpp
index b5d0d2e59e..ba4af3f747 100644
--- a/http/http_connection.hpp
+++ b/http/http_connection.hpp
@@ -94,6 +94,10 @@ class Connection :
         // don't require auth
         if (preverified)
         {
+            if (!req)
+            {
+                return false;
+            }
             mtlsSession = verifyMtlsUser(req->ipAddress, ctx);
             if (mtlsSession)
             {
@@ -202,6 +206,10 @@ class Connection :
     void handle()
     {
         std::error_code reqEc;
+        if (!parser)
+        {
+            return;
+        }
         crow::Request& thisReq = req.emplace(parser->release(), reqEc);
         if (reqEc)
         {
@@ -363,6 +371,10 @@ class Connection :
         {
             return;
         }
+        if (!req)
+        {
+            return;
+        }
         req->ipAddress = ip;
     }
 
@@ -389,7 +401,10 @@ class Connection :
     void doReadHeaders()
     {
         BMCWEB_LOG_DEBUG("{} doReadHeaders", logPtr(this));
-
+        if (!parser)
+        {
+            return;
+        }
         // Clean up any previous Connection.
         boost::beast::http::async_read_header(
             adaptor, buffer, *parser,
@@ -475,6 +490,10 @@ class Connection :
     void doRead()
     {
         BMCWEB_LOG_DEBUG("{} doRead", logPtr(this));
+        if (!parser)
+        {
+            return;
+        }
         startDeadline();
         boost::beast::http::async_read_some(
             adaptor, buffer, *parser,
@@ -515,7 +534,7 @@ class Connection :
     {
         BMCWEB_LOG_DEBUG("{} doWrite", logPtr(this));
         thisRes.preparePayload();
-        serializer.emplace(*thisRes.stringResponse);
+        serializer.emplace(thisRes.stringResponse);
         startDeadline();
         boost::beast::http::async_write(adaptor, *serializer,
                                         [this, self(shared_from_this())](
diff --git a/http/http_response.hpp b/http/http_response.hpp
index c4f9366ed3..cb07a83635 100644
--- a/http/http_response.hpp
+++ b/http/http_response.hpp
@@ -23,26 +23,26 @@ struct Response
     using response_type =
         boost::beast::http::response<boost::beast::http::string_body>;
 
-    std::optional<response_type> stringResponse;
+    response_type stringResponse;
 
     nlohmann::json jsonValue;
 
     void addHeader(std::string_view key, std::string_view value)
     {
-        stringResponse->insert(key, value);
+        stringResponse.insert(key, value);
     }
 
     void addHeader(boost::beast::http::field key, std::string_view value)
     {
-        stringResponse->insert(key, value);
+        stringResponse.insert(key, value);
     }
 
     void clearHeader(boost::beast::http::field key)
     {
-        stringResponse->erase(key);
+        stringResponse.erase(key);
     }
 
-    Response() : stringResponse(response_type{}) {}
+    Response() = default;
 
     Response(Response&& res) noexcept :
         stringResponse(std::move(res.stringResponse)),
@@ -73,7 +73,7 @@ struct Response
             return *this;
         }
         stringResponse = std::move(r.stringResponse);
-        r.stringResponse.emplace(response_type{});
+        r.stringResponse.clear();
         jsonValue = std::move(r.jsonValue);
 
         // Only need to move completion handler if not already completed
@@ -98,27 +98,27 @@ struct Response
 
     void result(unsigned v)
     {
-        stringResponse->result(v);
+        stringResponse.result(v);
     }
 
     void result(boost::beast::http::status v)
     {
-        stringResponse->result(v);
+        stringResponse.result(v);
     }
 
     boost::beast::http::status result() const
     {
-        return stringResponse->result();
+        return stringResponse.result();
     }
 
     unsigned resultInt() const
     {
-        return stringResponse->result_int();
+        return stringResponse.result_int();
     }
 
     std::string_view reason() const
     {
-        return stringResponse->reason();
+        return stringResponse.reason();
     }
 
     bool isCompleted() const noexcept
@@ -128,29 +128,29 @@ struct Response
 
     std::string& body()
     {
-        return stringResponse->body();
+        return stringResponse.body();
     }
 
     std::string_view getHeaderValue(std::string_view key) const
     {
-        return stringResponse->base()[key];
+        return stringResponse.base()[key];
     }
 
     void keepAlive(bool k)
     {
-        stringResponse->keep_alive(k);
+        stringResponse.keep_alive(k);
     }
 
     bool keepAlive() const
     {
-        return stringResponse->keep_alive();
+        return stringResponse.keep_alive();
     }
 
     void preparePayload()
     {
         // This code is a throw-free equivalent to
         // beast::http::message::prepare_payload
-        boost::optional<uint64_t> pSize = stringResponse->payload_size();
+        boost::optional<uint64_t> pSize = stringResponse.payload_size();
         using boost::beast::http::status;
         using boost::beast::http::status_class;
         using boost::beast::http::to_status_class;
@@ -160,12 +160,11 @@ struct Response
         }
         else
         {
-            bool is1XXReturn = to_status_class(stringResponse->result()) ==
+            bool is1XXReturn = to_status_class(stringResponse.result()) ==
                                status_class::informational;
             if (*pSize > 0 &&
-                (is1XXReturn ||
-                 stringResponse->result() == status::no_content ||
-                 stringResponse->result() == status::not_modified))
+                (is1XXReturn || stringResponse.result() == status::no_content ||
+                 stringResponse.result() == status::not_modified))
             {
                 BMCWEB_LOG_CRITICAL(
                     "{} Response content provided but code was no-content or not_modified, which aren't allowed to have a body",
@@ -174,13 +173,14 @@ struct Response
                 body().clear();
             }
         }
-        stringResponse->content_length(*pSize);
+        stringResponse.content_length(*pSize);
     }
 
     void clear()
     {
         BMCWEB_LOG_DEBUG("{} Clearing response containers", logPtr(this));
-        stringResponse.emplace(response_type{});
+        stringResponse.clear();
+        stringResponse.body().shrink_to_fit();
         jsonValue = nullptr;
         completed = false;
         expectedHash = std::nullopt;
@@ -188,7 +188,7 @@ struct Response
 
     void write(std::string_view bodyPart)
     {
-        stringResponse->body() += std::string(bodyPart);
+        stringResponse.body() += std::string(bodyPart);
     }
 
     std::string computeEtag() const