http: use OptRef helper to reduce some boilerplate (#14361)

Makes use of OptRef and adds some helpers to make it easier to go from OptRef to T* and vice versa.

Risk Level: Low
Testing: Existing tests
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

Signed-off-by: Snow Pettersen <[email protected]>

include/envoy/common/optref.h

@@ -34,6 +34,54 @@ template <class T> struct OptRef : public absl::optional<std::reference_wrapper<
     const T& ref = **this;
     return &ref;
   }
+
+  /**
+   * Helper to convert a OptRef into a pointer. If the optional is not set, returns a nullptr.
+   */
+  T* ptr() {
+    if (this->has_value()) {
+      T& ref = **this;
+      return &ref;
+    }
+
+    return nullptr;
+  }
+
+  /**
+   * Helper to convert a OptRef into a pointer. If the optional is not set, returns a nullptr.
+   */
+  const T* ptr() const {
+    if (this->has_value()) {
+      const T& ref = **this;
+      return &ref;
+    }
+
+    return nullptr;
+  }
+
+  T& ref() { return **this; }
+
+  const T& ref() const { return **this; }
 };
 
+/**
+ * Constructs an OptRef<T> from the provided reference.
+ * @param ref the reference to wrap
+ * @return OptRef<T> the wrapped reference
+ */
+template <class T> OptRef<T> makeOptRef(T& ref) { return {ref}; }
+
+/**
+ * Constructs an OptRef<T> from the provided pointer.
+ * @param ptr the pointer to wrap
+ * @return OptRef<T> the wrapped pointer, or absl::nullopt if the pointer is nullptr
+ */
+template <class T> OptRef<T> makeOptRefFromPtr(T* ptr) {
+  if (ptr == nullptr) {
+    return {};
+  }
+
+  return {*ptr};
+}
+
 } // namespace Envoy

include/envoy/http/header_map.h

@@ -8,6 +8,7 @@
 #include <string>
 #include <vector>
 
+#include "envoy/common/optref.h"
 #include "envoy/common/pure.h"
 
 #include "common/common/assert.h"
@@ -748,16 +749,15 @@ class RequestHeaderMap
   INLINE_REQ_HEADERS(DEFINE_INLINE_HEADER)
 };
 using RequestHeaderMapPtr = std::unique_ptr<RequestHeaderMap>;
-using RequestHeaderMapOptRef = absl::optional<std::reference_wrapper<RequestHeaderMap>>;
-using RequestHeaderMapOptConstRef = absl::optional<std::reference_wrapper<const RequestHeaderMap>>;
-using RequestHeaderMapOptRef = absl::optional<std::reference_wrapper<RequestHeaderMap>>;
+using RequestHeaderMapOptRef = OptRef<RequestHeaderMap>;
+using RequestHeaderMapOptConstRef = OptRef<const RequestHeaderMap>;
 
 // Request trailers.
 class RequestTrailerMap
     : public HeaderMap,
       public CustomInlineHeaderBase<CustomInlineHeaderRegistry::Type::RequestTrailers> {};
 using RequestTrailerMapPtr = std::unique_ptr<RequestTrailerMap>;
-using RequestTrailerMapOptRef = absl::optional<std::reference_wrapper<RequestTrailerMap>>;
+using RequestTrailerMapOptRef = OptRef<RequestTrailerMap>;
 
 // Base class for both response headers and trailers.
 class ResponseHeaderOrTrailerMap {
@@ -776,17 +776,16 @@ class ResponseHeaderMap
   INLINE_RESP_HEADERS(DEFINE_INLINE_HEADER)
 };
 using ResponseHeaderMapPtr = std::unique_ptr<ResponseHeaderMap>;
-using ResponseHeaderMapOptRef = absl::optional<std::reference_wrapper<ResponseHeaderMap>>;
-using ResponseHeaderMapOptConstRef =
-    absl::optional<std::reference_wrapper<const ResponseHeaderMap>>;
+using ResponseHeaderMapOptRef = OptRef<ResponseHeaderMap>;
+using ResponseHeaderMapOptConstRef = OptRef<const ResponseHeaderMap>;
 
 // Response trailers.
 class ResponseTrailerMap
     : public ResponseHeaderOrTrailerMap,
       public HeaderMap,
       public CustomInlineHeaderBase<CustomInlineHeaderRegistry::Type::ResponseTrailers> {};
 using ResponseTrailerMapPtr = std::unique_ptr<ResponseTrailerMap>;
-using ResponseTrailerMapOptRef = absl::optional<std::reference_wrapper<ResponseTrailerMap>>;
+using ResponseTrailerMapOptRef = OptRef<ResponseTrailerMap>;
 
 /**
  * Convenient container type for storing Http::LowerCaseString and std::string key/value pairs.

source/common/common/dump_state_utils.h

@@ -20,7 +20,7 @@ namespace Envoy {
 #define DUMP_DETAILS(member)                                                                       \
   do {                                                                                             \
     os << spaces << #member ": ";                                                                  \
-    if ((member) != nullptr) {                                                                     \
+    if (member) {                                                                                  \
       os << "\n";                                                                                  \
       (member)->dumpState(os, indent_level + 1);                                                   \
     } else {                                                                                       \

source/common/http/conn_manager_impl.h

@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "envoy/access_log/access_log.h"
+#include "envoy/common/optref.h"
 #include "envoy/common/random_generator.h"
 #include "envoy/common/scope_tracker.h"
 #include "envoy/common/time.h"
@@ -229,22 +230,20 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
     }
     void chargeStats(const ResponseHeaderMap& headers) override;
 
-    // TODO(snowp): Create shared OptRef/OptConstRef helpers
     Http::RequestHeaderMapOptRef requestHeaders() override {
-      return request_headers_ ? absl::make_optional(std::ref(*request_headers_)) : absl::nullopt;
+      return makeOptRefFromPtr(request_headers_.get());
     }
     Http::RequestTrailerMapOptRef requestTrailers() override {
-      return request_trailers_ ? absl::make_optional(std::ref(*request_trailers_)) : absl::nullopt;
+      return makeOptRefFromPtr(request_trailers_.get());
     }
     Http::ResponseHeaderMapOptRef continueHeaders() override {
-      return continue_headers_ ? absl::make_optional(std::ref(*continue_headers_)) : absl::nullopt;
+      return makeOptRefFromPtr(continue_headers_.get());
     }
     Http::ResponseHeaderMapOptRef responseHeaders() override {
-      return response_headers_ ? absl::make_optional(std::ref(*response_headers_)) : absl::nullopt;
+      return makeOptRefFromPtr(response_headers_.get());
     }
     Http::ResponseTrailerMapOptRef responseTrailers() override {
-      return response_trailers_ ? absl::make_optional(std::ref(*response_trailers_))
-                                : absl::nullopt;
+      return makeOptRefFromPtr(response_trailers_.get());
     }
 
     void endStream() override {

source/common/http/filter_manager.cc

@@ -844,16 +844,14 @@ void FilterManager::sendLocalReplyViaFilterChain(
                  absl::string_view& content_type) -> void {
             // TODO(snowp): This &get() business isn't nice, rework LocalReply and others to accept
             // opt refs.
-            local_reply_.rewrite(filter_manager_callbacks_.requestHeaders().has_value()
-                                     ? &filter_manager_callbacks_.requestHeaders()->get()
-                                     : nullptr,
-                                 response_headers, stream_info_, code, body, content_type);
+            local_reply_.rewrite(filter_manager_callbacks_.requestHeaders().ptr(), response_headers,
+                                 stream_info_, code, body, content_type);
           },
           [this, modify_headers](ResponseHeaderMapPtr&& headers, bool end_stream) -> void {
             filter_manager_callbacks_.setResponseHeaders(std::move(headers));
             // TODO: Start encoding from the last decoder filter that saw the
             // request instead.
-            encodeHeaders(nullptr, filter_manager_callbacks_.responseHeaders()->get(), end_stream);
+            encodeHeaders(nullptr, filter_manager_callbacks_.responseHeaders().ref(), end_stream);
           },
           [this](Buffer::Instance& data, bool end_stream) -> void {
             // TODO: Start encoding from the last decoder filter that saw the
@@ -885,10 +883,8 @@ void FilterManager::sendDirectLocalReply(
           },
           [&](ResponseHeaderMap& response_headers, Code& code, std::string& body,
               absl::string_view& content_type) -> void {
-            local_reply_.rewrite(filter_manager_callbacks_.requestHeaders().has_value()
-                                     ? &filter_manager_callbacks_.requestHeaders()->get()
-                                     : nullptr,
-                                 response_headers, stream_info_, code, body, content_type);
+            local_reply_.rewrite(filter_manager_callbacks_.requestHeaders().ptr(), response_headers,
+                                 stream_info_, code, body, content_type);
           },
           [&](ResponseHeaderMapPtr&& response_headers, bool end_stream) -> void {
             // Move the response headers into the FilterManager to make sure they're visible to
@@ -1248,11 +1244,11 @@ bool FilterManager::createFilterChain() {
   bool upgrade_rejected = false;
   const HeaderEntry* upgrade = nullptr;
   if (filter_manager_callbacks_.requestHeaders()) {
-    upgrade = filter_manager_callbacks_.requestHeaders()->get().Upgrade();
+    upgrade = filter_manager_callbacks_.requestHeaders()->Upgrade();
 
     // Treat CONNECT requests as a special upgrade case.
     if (!upgrade && HeaderUtility::isConnect(*filter_manager_callbacks_.requestHeaders())) {
-      upgrade = filter_manager_callbacks_.requestHeaders()->get().Method();
+      upgrade = filter_manager_callbacks_.requestHeaders()->Method();
     }
   }
 

source/common/http/filter_manager.h

@@ -2,6 +2,7 @@
 
 #include <memory>
 
+#include "envoy/common/optref.h"
 #include "envoy/extensions/filters/common/matcher/action/v3/skip_action.pb.h"
 #include "envoy/http/filter.h"
 #include "envoy/http/header_map.h"
@@ -35,19 +36,11 @@ class HttpMatchingDataImpl : public HttpMatchingData {
   }
 
   Http::RequestHeaderMapOptConstRef requestHeaders() const override {
-    if (request_headers_) {
-      return absl::make_optional(std::cref(*request_headers_));
-    }
-
-    return absl::nullopt;
+    return makeOptRefFromPtr(request_headers_);
   }
 
   Http::ResponseHeaderMapOptConstRef responseHeaders() const override {
-    if (response_headers_) {
-      return absl::make_optional(std::cref(*response_headers_));
-    }
-
-    return absl::nullopt;
+    return makeOptRefFromPtr(response_headers_);
   }
 
 private:
@@ -630,10 +623,10 @@ class FilterManager : public ScopeTrackedObject,
     const char* spaces = spacesForLevel(indent_level);
     os << spaces << "FilterManager " << this << DUMP_MEMBER(state_.has_continue_headers_) << "\n";
 
-    DUMP_OPT_REF_DETAILS(filter_manager_callbacks_.requestHeaders());
-    DUMP_OPT_REF_DETAILS(filter_manager_callbacks_.requestTrailers());
-    DUMP_OPT_REF_DETAILS(filter_manager_callbacks_.responseHeaders());
-    DUMP_OPT_REF_DETAILS(filter_manager_callbacks_.responseTrailers());
+    DUMP_DETAILS(filter_manager_callbacks_.requestHeaders());
+    DUMP_DETAILS(filter_manager_callbacks_.requestTrailers());
+    DUMP_DETAILS(filter_manager_callbacks_.responseHeaders());
+    DUMP_DETAILS(filter_manager_callbacks_.responseTrailers());
     DUMP_DETAILS(&stream_info_);
   }
 
@@ -689,15 +682,15 @@ class FilterManager : public ScopeTrackedObject,
   void log() {
     RequestHeaderMap* request_headers = nullptr;
     if (filter_manager_callbacks_.requestHeaders()) {
-      request_headers = &filter_manager_callbacks_.requestHeaders()->get();
+      request_headers = filter_manager_callbacks_.requestHeaders().ptr();
     }
     ResponseHeaderMap* response_headers = nullptr;
     if (filter_manager_callbacks_.responseHeaders()) {
-      response_headers = &filter_manager_callbacks_.responseHeaders()->get();
+      response_headers = filter_manager_callbacks_.responseHeaders().ptr();
     }
     ResponseTrailerMap* response_trailers = nullptr;
     if (filter_manager_callbacks_.responseTrailers()) {
-      response_trailers = &filter_manager_callbacks_.responseTrailers()->get();
+      response_trailers = filter_manager_callbacks_.responseTrailers().ptr();
     }
 
     for (const auto& log_handler : access_log_handlers_) {
@@ -822,11 +815,11 @@ class FilterManager : public ScopeTrackedObject,
 
   void requestHeadersInitialized() {
     if (Http::Headers::get().MethodValues.Head ==
-        filter_manager_callbacks_.requestHeaders()->get().getMethodValue()) {
+        filter_manager_callbacks_.requestHeaders()->getMethodValue()) {
       state_.is_head_request_ = true;
     }
     state_.is_grpc_request_ =
-        Grpc::Common::isGrpcRequestHeaders(filter_manager_callbacks_.requestHeaders()->get());
+        Grpc::Common::isGrpcRequestHeaders(filter_manager_callbacks_.requestHeaders().ref());
   }
 
   /**

test/common/http/conn_manager_impl_test_2.cc

@@ -2356,7 +2356,7 @@ TEST_F(HttpConnectionManagerImplTest, TestSessionTrace) {
           object->dumpState(out);
           std::string state = out.str();
           EXPECT_THAT(state,
-                      testing::HasSubstr("filter_manager_callbacks_.requestHeaders():   empty"));
+                      testing::HasSubstr("filter_manager_callbacks_.requestHeaders():   null"));
           EXPECT_THAT(state, testing::HasSubstr("protocol_: 1"));
           return nullptr;
         }))

test/common/http/filter_manager_test.cc

@@ -1,3 +1,4 @@
+#include "envoy/common/optref.h"
 #include "envoy/http/filter.h"
 #include "envoy/http/header_map.h"
 #include "envoy/matcher/matcher.h"
@@ -65,7 +66,7 @@ TEST_F(FilterManagerTest, SendLocalReplyDuringDecodingGrpcClassiciation) {
                                    {"content-type", "application/grpc"}}};
 
   ON_CALL(filter_manager_callbacks_, requestHeaders())
-      .WillByDefault(Return(absl::make_optional(std::ref(*grpc_headers))));
+      .WillByDefault(Return(makeOptRef(*grpc_headers)));
 
   EXPECT_CALL(filter_factory_, createFilterChain(_))
       .WillRepeatedly(Invoke([&](FilterChainFactoryCallbacks& callbacks) -> void {
@@ -127,7 +128,7 @@ TEST_F(FilterManagerTest, SendLocalReplyDuringEncodingGrpcClassiciation) {
                                    {"content-type", "application/grpc"}}};
 
   ON_CALL(filter_manager_callbacks_, requestHeaders())
-      .WillByDefault(Return(absl::make_optional(std::ref(*grpc_headers))));
+      .WillByDefault(Return(makeOptRef(*grpc_headers)));
   filter_manager_->createFilterChain();
 
   filter_manager_->requestHeadersInitialized();
@@ -186,7 +187,7 @@ TEST_F(FilterManagerTest, MatchTreeSkipActionDecodingHeaders) {
                                    {"content-type", "application/grpc"}}};
 
   ON_CALL(filter_manager_callbacks_, requestHeaders())
-      .WillByDefault(Return(absl::make_optional(std::ref(*grpc_headers))));
+      .WillByDefault(Return(makeOptRef(*grpc_headers)));
   filter_manager_->createFilterChain();
 
   filter_manager_->requestHeadersInitialized();
@@ -237,7 +238,7 @@ TEST_F(FilterManagerTest, MatchTreeSkipActionRequestAndResponseHeaders) {
   Buffer::OwnedImpl data("data");
 
   ON_CALL(filter_manager_callbacks_, requestHeaders())
-      .WillByDefault(Return(absl::make_optional(std::ref(*headers))));
+      .WillByDefault(Return((makeOptRef(*headers))));
   filter_manager_->createFilterChain();
 
   EXPECT_CALL(filter_manager_callbacks_, encodeHeaders(_, _));

test/mocks/http/mocks.cc

@@ -1,6 +1,7 @@
 #include "mocks.h"
 
 #include "envoy/buffer/buffer.h"
+#include "envoy/common/optref.h"
 #include "envoy/event/dispatcher.h"
 #include "envoy/http/header_map.h"
 
@@ -23,10 +24,7 @@ MockServerConnectionCallbacks::~MockServerConnectionCallbacks() = default;
 
 MockFilterManagerCallbacks::MockFilterManagerCallbacks() {
   ON_CALL(*this, responseHeaders()).WillByDefault(Invoke([this]() -> ResponseHeaderMapOptRef {
-    if (response_headers_) {
-      return absl::make_optional(std::ref(*response_headers_));
-    }
-    return absl::nullopt;
+    return makeOptRefFromPtr(response_headers_.get());
   }));
 }
 MockFilterManagerCallbacks::~MockFilterManagerCallbacks() = default;