diff --git a/include/envoy/common/optref.h b/include/envoy/common/optref.h index cf51cdaa52ea1..f96654f5938af 100644 --- a/include/envoy/common/optref.h +++ b/include/envoy/common/optref.h @@ -34,6 +34,54 @@ template struct OptRef : public absl::optionalhas_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 from the provided reference. + * @param ref the reference to wrap + * @return OptRef the wrapped reference + */ +template OptRef makeOptRef(T& ref) { return {ref}; } + +/** + * Constructs an OptRef from the provided pointer. + * @param ptr the pointer to wrap + * @return OptRef the wrapped pointer, or absl::nullopt if the pointer is nullptr + */ +template OptRef makeOptRefFromPtr(T* ptr) { + if (ptr == nullptr) { + return {}; + } + + return {*ptr}; +} + } // namespace Envoy diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 06bbe4f7a91b4..8df40a404e829 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -8,6 +8,7 @@ #include #include +#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; -using RequestHeaderMapOptRef = absl::optional>; -using RequestHeaderMapOptConstRef = absl::optional>; -using RequestHeaderMapOptRef = absl::optional>; +using RequestHeaderMapOptRef = OptRef; +using RequestHeaderMapOptConstRef = OptRef; // Request trailers. class RequestTrailerMap : public HeaderMap, public CustomInlineHeaderBase {}; using RequestTrailerMapPtr = std::unique_ptr; -using RequestTrailerMapOptRef = absl::optional>; +using RequestTrailerMapOptRef = OptRef; // Base class for both response headers and trailers. class ResponseHeaderOrTrailerMap { @@ -776,9 +776,8 @@ class ResponseHeaderMap INLINE_RESP_HEADERS(DEFINE_INLINE_HEADER) }; using ResponseHeaderMapPtr = std::unique_ptr; -using ResponseHeaderMapOptRef = absl::optional>; -using ResponseHeaderMapOptConstRef = - absl::optional>; +using ResponseHeaderMapOptRef = OptRef; +using ResponseHeaderMapOptConstRef = OptRef; // Response trailers. class ResponseTrailerMap @@ -786,7 +785,7 @@ class ResponseTrailerMap public HeaderMap, public CustomInlineHeaderBase {}; using ResponseTrailerMapPtr = std::unique_ptr; -using ResponseTrailerMapOptRef = absl::optional>; +using ResponseTrailerMapOptRef = OptRef; /** * Convenient container type for storing Http::LowerCaseString and std::string key/value pairs. diff --git a/source/common/common/dump_state_utils.h b/source/common/common/dump_state_utils.h index be6c24885813c..704ea271a5c4d 100644 --- a/source/common/common/dump_state_utils.h +++ b/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 { \ diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 828f8ceb30e45..4872cd1a75515 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -10,6 +10,7 @@ #include #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, } 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 { diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index f0b7618794f41..5f27ecacf0e4c 100644 --- a/source/common/http/filter_manager.cc +++ b/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(); } } diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index 21109a82d7740..2493bd2edaff1 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -2,6 +2,7 @@ #include +#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()); } /** diff --git a/test/common/http/conn_manager_impl_test_2.cc b/test/common/http/conn_manager_impl_test_2.cc index 8fa74b347d0d1..5a057f083801e 100644 --- a/test/common/http/conn_manager_impl_test_2.cc +++ b/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; })) diff --git a/test/common/http/filter_manager_test.cc b/test/common/http/filter_manager_test.cc index 08a34e7054f3e..85d755e864cb4 100644 --- a/test/common/http/filter_manager_test.cc +++ b/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(_, _)); diff --git a/test/mocks/http/mocks.cc b/test/mocks/http/mocks.cc index 639295ae16d3b..81f6bcd093ff6 100644 --- a/test/mocks/http/mocks.cc +++ b/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;