From de5288a659040b3ec6c8078bde880f0ce71f28c3 Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Thu, 19 Jul 2018 16:27:55 -0400 Subject: [PATCH 01/30] Initial implementation of DynamicMetadata & StringAccessor Signed-off-by: Randy Smith --- include/envoy/request_info/BUILD | 17 ++++++ include/envoy/request_info/dynamic_metadata.h | 56 +++++++++++++++++++ include/envoy/request_info/string_accessor.h | 17 ++++++ source/common/request_info/BUILD | 17 ++++++ .../request_info/dynamic_metadata_impl.cc | 49 ++++++++++++++++ .../request_info/dynamic_metadata_impl.h | 39 +++++++++++++ .../request_info/string_accessor_impl.h | 23 ++++++++ 7 files changed, 218 insertions(+) create mode 100644 include/envoy/request_info/dynamic_metadata.h create mode 100644 include/envoy/request_info/string_accessor.h create mode 100644 source/common/request_info/dynamic_metadata_impl.cc create mode 100644 source/common/request_info/dynamic_metadata_impl.h create mode 100644 source/common/request_info/string_accessor_impl.h diff --git a/include/envoy/request_info/BUILD b/include/envoy/request_info/BUILD index 57bc0ae18b780..b7b02090e3198 100644 --- a/include/envoy/request_info/BUILD +++ b/include/envoy/request_info/BUILD @@ -18,3 +18,20 @@ envoy_cc_library( "//include/envoy/upstream:upstream_interface", ], ) + +envoy_cc_library( + name = "dynamic_metadata_interface", + hdrs = ["dynamic_metadata.h"], + external_deps = ["abseil_optional"], + deps = [ + ], +) + +envoy_cc_library( + name = "string_accessor_interface", + hdrs = ["string_accessor.h"], + external_deps = ["abseil_optional"], + deps = [ + ], +) + diff --git a/include/envoy/request_info/dynamic_metadata.h b/include/envoy/request_info/dynamic_metadata.h new file mode 100644 index 0000000000000..4011b082cb5b1 --- /dev/null +++ b/include/envoy/request_info/dynamic_metadata.h @@ -0,0 +1,56 @@ +#pragma once + +#include "envoy/common/pure.h" + +#include "absl/strings/string_view.h" + +namespace Envoy { +namespace RequestInfo { + +class DynamicMetadata { + public: + virtual ~DynamicMetadata() PURE; + + // May only be called once for a particular |data_name| + template + void setData( + absl::string_view data_name, + std::unique_ptr&& data) { + setDataGeneric(data_name, Traits::getTypeId(), static_cast(data.release()), + Traits::destructor); + } + + template + const T& getData(absl::string_view data_name) { + return *static_cast(getDataGeneric(data_name, Traits::getTypeId())); + } + + protected: + virtual void setDataGeneric( + absl::string_view data_name, + size_t type_id, + void* data, // Implementation must take ownership + void (*destructor)(void *)) PURE; + + virtual void* getDataGeneric( + absl::string_view data_name, + size_t type_id) PURE; + + private: + // TODO(rdsmith): Is it ok to default this to presumably zero this way? + static size_t type_id_index_; + + template + class Traits { + static size_t getTypeId() { + static const size_t type_id = ++type_id_index_; + return type_id; + } + void destructor(void *ptr) { + delete static_cast(ptr); + } + }; +}; + +} // namespace RequestInfo +} // namespace Envoy diff --git a/include/envoy/request_info/string_accessor.h b/include/envoy/request_info/string_accessor.h new file mode 100644 index 0000000000000..eeb112a913f93 --- /dev/null +++ b/include/envoy/request_info/string_accessor.h @@ -0,0 +1,17 @@ +#pragma once + +#include "envoy/common/pure.h" + +#include "absl/strings/string_view.h" + +namespace Envoy { +namespace RequestInfo { + +class StringAccessor { + public: + virtual ~StringAccessor() PURE; + absl::string_view asString() const PURE; +}; + +} // namespace RequestInfo +} // namespace Envoy diff --git a/source/common/request_info/BUILD b/source/common/request_info/BUILD index 77d0e7d0774f7..95bade2468de4 100644 --- a/source/common/request_info/BUILD +++ b/source/common/request_info/BUILD @@ -17,6 +17,23 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "dynamic_metadata_lib", + srcs = ["dynamic_metadata_impl.cc"], + hdrs = ["dynamic_metadata_impl.h"], + deps = [ + "//include/envoy/request_info:dynamic_metadata_interface", + ], +) + +envoy_cc_library( + name = "string_accessor_lib", + hdrs = ["string_accessor_impl.h"], + deps = [ + "//include/envoy/request_info:string_accessor_interface", + ], +) + envoy_cc_library( name = "utility_lib", srcs = ["utility.cc"], diff --git a/source/common/request_info/dynamic_metadata_impl.cc b/source/common/request_info/dynamic_metadata_impl.cc new file mode 100644 index 0000000000000..7275bc962d640 --- /dev/null +++ b/source/common/request_info/dynamic_metadata_impl.cc @@ -0,0 +1,49 @@ +#include "common/request_info/dynamic_metadata_impl.h" + +#include "envoy/common/exception.h" + +namespace Envoy { +namespace RequestInfo { + +DynamicMetadataImpl::~DynamicMetadataImpl() { + for (auto& it : data_storage_) { + if (it.second.destructor_) { + it.second.destructor_(it.second.ptr_); + } + } + data_storage_.clear(); +} + +void DynamicMetadataImpl::setDataGeneric( + absl::string_view data_name, + size_t type_id, + void* data, + void (*destructor)(void*)) { + if (data_storage_.find(data_name) != data_storage_.end()) { + throw EnvoyException("DynamicMetadata::setData called twice with same name."); + } + + data_storage_[static_cast(data_name)] = { type_id, data, destructor }; +} + +void* DynamicMetadataImpl::getDataGeneric( + absl::string_view data_name, + size_t type_id) { + const auto& it = data_storage_.find(data_name); + + if (it == data_storage_.end()) { + throw EnvoyException("DynamicMetadata::getData called for unknown data name."); + } + + if (it->second.typeid_ != type_id) { + throw EnvoyException( + "DynamicMetadata::getData called with different type than name originally set with."); + } + + return it->second.ptr_; +} + +} // namespace RequestInfo +} // namespace Envoy + + diff --git a/source/common/request_info/dynamic_metadata_impl.h b/source/common/request_info/dynamic_metadata_impl.h new file mode 100644 index 0000000000000..4591cf0403e12 --- /dev/null +++ b/source/common/request_info/dynamic_metadata_impl.h @@ -0,0 +1,39 @@ +#pragma once + +#include "envoy/request_info/dynamic_metadata.h" + +#include + +#include "absl/strings/string_view.h" + +namespace Envoy { +namespace RequestInfo { + +class DynamicMetadataImpl : public DynamicMetadata { + public: + DynamicMetadataImpl() {} + ~DynamicMetadataImpl() override; + + // DynamicMetadata + void setDataGeneric( + absl::string_view data_name, + size_t type_id, + void* data, + void (*destructor)(void*)) override; + + void* getDataGeneric( + absl::string_view data_name, + size_t type_id) override; + + private: + struct Data { + size_t typeid_; + void* ptr_; + void (*destructor_)(void *); + }; + + std::map> data_storage_; +}; + +} // namespace RequestInfo +} // namespace Envoy diff --git a/source/common/request_info/string_accessor_impl.h b/source/common/request_info/string_accessor_impl.h new file mode 100644 index 0000000000000..6ea9396671250 --- /dev/null +++ b/source/common/request_info/string_accessor_impl.h @@ -0,0 +1,23 @@ +#pragma once + +#include "include/envoy/request_info/string_accessor.h" + +namespace Envoy { +namespace RequestInfo { + +class StringAccessorImpl : public StringAccessor { + public: + StringAccessorImpl(absl::string_view value) : value_(value) {} + + // StringAccessor + ~StringAccessorImpl() override {} + absl::string_view asString() const override { + return value_; + } + + private: + std::string value_; +}; + +} // namespace RequestInfo +} // namespace Envoy From 3b1250e815f81171c177758e0718a02a6c2e0c9e Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Thu, 19 Jul 2018 17:02:36 -0400 Subject: [PATCH 02/30] Integrated (I hope) with RequestInfo. Signed-off-by: Randy Smith --- include/envoy/request_info/BUILD | 1 + include/envoy/request_info/request_info.h | 7 +++++++ source/common/request_info/BUILD | 1 + source/common/request_info/request_info_impl.h | 6 ++++++ test/mocks/request_info/mocks.h | 1 + 5 files changed, 16 insertions(+) diff --git a/include/envoy/request_info/BUILD b/include/envoy/request_info/BUILD index b7b02090e3198..d3335f5f03676 100644 --- a/include/envoy/request_info/BUILD +++ b/include/envoy/request_info/BUILD @@ -13,6 +13,7 @@ envoy_cc_library( hdrs = ["request_info.h"], external_deps = ["abseil_optional"], deps = [ + ":dynamic_metadata_interface", "//include/envoy/common:time_interface", "//include/envoy/http:protocol_interface", "//include/envoy/upstream:upstream_interface", diff --git a/include/envoy/request_info/request_info.h b/include/envoy/request_info/request_info.h index ee21a3ae7e8b1..f309406b17faf 100644 --- a/include/envoy/request_info/request_info.h +++ b/include/envoy/request_info/request_info.h @@ -8,6 +8,7 @@ #include "envoy/common/pure.h" #include "envoy/common/time.h" #include "envoy/http/protocol.h" +#include "envoy/request_info/dynamic_metadata.h" #include "envoy/upstream/upstream.h" #include "absl/types/optional.h" @@ -300,6 +301,12 @@ class RequestInfo { * the same key overriding existing. */ virtual void setDynamicMetadata(const std::string& name, const ProtobufWkt::Struct& value) PURE; + + /** + * More general dynamic metadata object. + * TODO(rdsmith): Replace uses of above with this object. + */ + virtual DynamicMetadata& dynamicMetadata2() PURE; }; } // namespace RequestInfo diff --git a/source/common/request_info/BUILD b/source/common/request_info/BUILD index 95bade2468de4..1418c975d84f0 100644 --- a/source/common/request_info/BUILD +++ b/source/common/request_info/BUILD @@ -12,6 +12,7 @@ envoy_cc_library( name = "request_info_lib", hdrs = ["request_info_impl.h"], deps = [ + ":dynamic_metadata_lib", "//include/envoy/request_info:request_info_interface", "//source/common/common:assert_lib", ], diff --git a/source/common/request_info/request_info_impl.h b/source/common/request_info/request_info_impl.h index f56f34ebc4e52..862d8147d6926 100644 --- a/source/common/request_info/request_info_impl.h +++ b/source/common/request_info/request_info_impl.h @@ -6,6 +6,7 @@ #include "envoy/request_info/request_info.h" #include "common/common/assert.h" +#include "common/request_info/dynamic_metadata_impl.h" namespace Envoy { namespace RequestInfo { @@ -178,6 +179,10 @@ struct RequestInfoImpl : public RequestInfo { (*metadata_.mutable_filter_metadata())[name].MergeFrom(value); }; + DynamicMetadata& dynamicMetadata2() override { + return metadata2_; + } + const SystemTime start_time_; const MonotonicTime start_time_monotonic_; @@ -197,6 +202,7 @@ struct RequestInfoImpl : public RequestInfo { bool hc_request_{}; const Router::RouteEntry* route_entry_{}; envoy::api::v2::core::Metadata metadata_{}; + DynamicMetadataImpl metadata2_{}; private: uint64_t bytes_received_{}; diff --git a/test/mocks/request_info/mocks.h b/test/mocks/request_info/mocks.h index cafe6815e5bcc..474fc2bb44314 100644 --- a/test/mocks/request_info/mocks.h +++ b/test/mocks/request_info/mocks.h @@ -60,6 +60,7 @@ class MockRequestInfo : public RequestInfo { MOCK_METHOD2(setDynamicMetadata, void(const std::string&, const ProtobufWkt::Struct&)); MOCK_METHOD3(setDynamicMetadata, void(const std::string&, const std::string&, const std::string&)); + MOCK_METHOD0(dynamicMetadata2, DynamicMetadata&()); std::shared_ptr> host_{ new testing::NiceMock()}; From ee73e1fdde66b29ec7b2adcb641d7cc8a549c2e4 Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Fri, 20 Jul 2018 16:03:58 -0400 Subject: [PATCH 03/30] Initial dynamic metadata test passes. Signed-off-by: Randy Smith --- include/envoy/request_info/BUILD | 1 + .../envoy/request_info/dynamic_metadata.cc | 9 +++ include/envoy/request_info/dynamic_metadata.h | 7 ++- include/envoy/request_info/string_accessor.h | 4 +- .../request_info/string_accessor_impl.h | 2 +- test/common/request_info/BUILD | 17 +++++ .../dynamic_metadata_impl_test.cc | 63 +++++++++++++++++++ .../request_info/string_accessor_impl_test.cc | 18 ++++++ 8 files changed, 115 insertions(+), 6 deletions(-) create mode 100644 include/envoy/request_info/dynamic_metadata.cc create mode 100644 test/common/request_info/dynamic_metadata_impl_test.cc create mode 100644 test/common/request_info/string_accessor_impl_test.cc diff --git a/include/envoy/request_info/BUILD b/include/envoy/request_info/BUILD index d3335f5f03676..d06fc09a4c6aa 100644 --- a/include/envoy/request_info/BUILD +++ b/include/envoy/request_info/BUILD @@ -23,6 +23,7 @@ envoy_cc_library( envoy_cc_library( name = "dynamic_metadata_interface", hdrs = ["dynamic_metadata.h"], + srcs = ["dynamic_metadata.cc"], external_deps = ["abseil_optional"], deps = [ ], diff --git a/include/envoy/request_info/dynamic_metadata.cc b/include/envoy/request_info/dynamic_metadata.cc new file mode 100644 index 0000000000000..a49de3017cd92 --- /dev/null +++ b/include/envoy/request_info/dynamic_metadata.cc @@ -0,0 +1,9 @@ +#include "envoy/request_info/dynamic_metadata.h" + +namespace Envoy { +namespace RequestInfo { + +size_t DynamicMetadata::type_id_index_ = 0; + +} // namespace RequestInfo +} // namespace Envoy diff --git a/include/envoy/request_info/dynamic_metadata.h b/include/envoy/request_info/dynamic_metadata.h index 4011b082cb5b1..f8e117e546587 100644 --- a/include/envoy/request_info/dynamic_metadata.h +++ b/include/envoy/request_info/dynamic_metadata.h @@ -9,7 +9,7 @@ namespace RequestInfo { class DynamicMetadata { public: - virtual ~DynamicMetadata() PURE; + virtual ~DynamicMetadata() {}; // May only be called once for a particular |data_name| template @@ -17,7 +17,7 @@ class DynamicMetadata { absl::string_view data_name, std::unique_ptr&& data) { setDataGeneric(data_name, Traits::getTypeId(), static_cast(data.release()), - Traits::destructor); + &Traits::destructor); } template @@ -42,11 +42,12 @@ class DynamicMetadata { template class Traits { + public: static size_t getTypeId() { static const size_t type_id = ++type_id_index_; return type_id; } - void destructor(void *ptr) { + static void destructor(void *ptr) { delete static_cast(ptr); } }; diff --git a/include/envoy/request_info/string_accessor.h b/include/envoy/request_info/string_accessor.h index eeb112a913f93..97f8d8b1e4daa 100644 --- a/include/envoy/request_info/string_accessor.h +++ b/include/envoy/request_info/string_accessor.h @@ -9,8 +9,8 @@ namespace RequestInfo { class StringAccessor { public: - virtual ~StringAccessor() PURE; - absl::string_view asString() const PURE; + virtual ~StringAccessor() {}; + virtual absl::string_view asString() const PURE; }; } // namespace RequestInfo diff --git a/source/common/request_info/string_accessor_impl.h b/source/common/request_info/string_accessor_impl.h index 6ea9396671250..f70b6905ed6db 100644 --- a/source/common/request_info/string_accessor_impl.h +++ b/source/common/request_info/string_accessor_impl.h @@ -1,6 +1,6 @@ #pragma once -#include "include/envoy/request_info/string_accessor.h" +#include "envoy/request_info/string_accessor.h" namespace Envoy { namespace RequestInfo { diff --git a/test/common/request_info/BUILD b/test/common/request_info/BUILD index 5dde76ff832db..51fc9c577d302 100644 --- a/test/common/request_info/BUILD +++ b/test/common/request_info/BUILD @@ -8,6 +8,14 @@ load( envoy_package() +envoy_cc_test( + name = "dynamic_metadata_impl_test", + srcs = ["dynamic_metadata_impl_test.cc"], + deps = [ + "//source/common/request_info:dynamic_metadata_lib", + ], +) + envoy_cc_test( name = "request_info_impl_test", srcs = ["request_info_impl_test.cc"], @@ -20,6 +28,14 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "string_accessor_impl_test", + srcs = ["string_accessor_impl_test.cc"], + deps = [ + "//source/common/request_info:string_accessor_lib" + ], +) + envoy_cc_test( name = "utility_test", srcs = ["utility_test.cc"], @@ -28,3 +44,4 @@ envoy_cc_test( "//test/mocks/request_info:request_info_mocks", ], ) + diff --git a/test/common/request_info/dynamic_metadata_impl_test.cc b/test/common/request_info/dynamic_metadata_impl_test.cc new file mode 100644 index 0000000000000..9f641e3c93860 --- /dev/null +++ b/test/common/request_info/dynamic_metadata_impl_test.cc @@ -0,0 +1,63 @@ +#include "common/request_info/dynamic_metadata_impl.h" + +#include "gtest/gtest.h" +// #include "gmock/gmock.h" + +namespace Envoy { +namespace RequestInfo { +namespace { + +class TestStoredType { + public: + TestStoredType(int value, size_t* access_count, size_t* destruction_count) + : value_(value), access_count_(access_count), destruction_count_(destruction_count) {} + ~TestStoredType() { ++*destruction_count_; } + + int Access() const { + ++*access_count_; + return value_; + } + + private: + int value_; + size_t* access_count_; + size_t* destruction_count_; +}; + +class DynamicMetadataImplTest : public testing::Test { + public: + DynamicMetadataImplTest() { + ResetDynamicMetadata(); + } + + void ResetDynamicMetadata() { + dynamic_metadata_ = std::make_unique(); + } + DynamicMetadata& dynamic_metadata() { return *dynamic_metadata_; } + + private: + std::unique_ptr dynamic_metadata_; +}; + +} // namespace + +TEST_F(DynamicMetadataImplTest, Simple) { + size_t access_count; + size_t destruction_count; + dynamic_metadata().setData("test_name", + std::make_unique( + 5, &access_count, &destruction_count)); + EXPECT_EQ(0u, access_count); + EXPECT_EQ(0u, destruction_count); + + EXPECT_EQ(5, dynamic_metadata().getData("test_name").Access()); + EXPECT_EQ(1u, access_count); + EXPECT_EQ(0u, destruction_count); + + ResetDynamicMetadata(); + EXPECT_EQ(1u, access_count); + EXPECT_EQ(1u, destruction_count); +} + +} // namespace RequestInfo +} // namespace Envoy diff --git a/test/common/request_info/string_accessor_impl_test.cc b/test/common/request_info/string_accessor_impl_test.cc new file mode 100644 index 0000000000000..e07773a7d1df6 --- /dev/null +++ b/test/common/request_info/string_accessor_impl_test.cc @@ -0,0 +1,18 @@ +#include "common/request_info/string_accessor_impl.h" + +#include "absl/strings/string_view.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace RequestInfo { + +TEST(StringAccessorImplTest, Storage) { + const char* const TestString = "test string 1"; + StringAccessorImpl accessor(TestString); + + EXPECT_EQ(TestString, accessor.asString()); +} + +} // namespace RequestInfo +} // namespace Envoy From c8066df9dd91d9da411bba834beeddc6df409a57 Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Fri, 20 Jul 2018 16:15:30 -0400 Subject: [PATCH 04/30] All DM tests. Signed-off-by: Randy Smith --- .../dynamic_metadata_impl_test.cc | 65 +++++++++++++++++-- 1 file changed, 60 insertions(+), 5 deletions(-) diff --git a/test/common/request_info/dynamic_metadata_impl_test.cc b/test/common/request_info/dynamic_metadata_impl_test.cc index 9f641e3c93860..36700ffc8b1c6 100644 --- a/test/common/request_info/dynamic_metadata_impl_test.cc +++ b/test/common/request_info/dynamic_metadata_impl_test.cc @@ -1,7 +1,8 @@ #include "common/request_info/dynamic_metadata_impl.h" +#include "envoy/common/exception.h" + #include "gtest/gtest.h" -// #include "gmock/gmock.h" namespace Envoy { namespace RequestInfo { @@ -11,10 +12,16 @@ class TestStoredType { public: TestStoredType(int value, size_t* access_count, size_t* destruction_count) : value_(value), access_count_(access_count), destruction_count_(destruction_count) {} - ~TestStoredType() { ++*destruction_count_; } + ~TestStoredType() { + if (destruction_count_) { + ++*destruction_count_; + } + } int Access() const { - ++*access_count_; + if (access_count_) { + ++*access_count_; + } return value_; } @@ -42,8 +49,8 @@ class DynamicMetadataImplTest : public testing::Test { } // namespace TEST_F(DynamicMetadataImplTest, Simple) { - size_t access_count; - size_t destruction_count; + size_t access_count = 0u; + size_t destruction_count = 0u; dynamic_metadata().setData("test_name", std::make_unique( 5, &access_count, &destruction_count)); @@ -59,5 +66,53 @@ TEST_F(DynamicMetadataImplTest, Simple) { EXPECT_EQ(1u, destruction_count); } +TEST_F(DynamicMetadataImplTest, SameTypes) { + size_t access_count_1 = 0u; + size_t access_count_2 = 0u; + size_t destruction_count = 0u; + const int ValueOne = 5; + const int ValueTwo = 6; + + dynamic_metadata().setData( + "test_1", std::make_unique( + ValueOne, &access_count_1, &destruction_count)); + dynamic_metadata().setData( + "test_2", std::make_unique( + ValueTwo, &access_count_2, &destruction_count)); + EXPECT_EQ(0u, access_count_1); + EXPECT_EQ(0u, access_count_2); + EXPECT_EQ(0u, destruction_count); + + EXPECT_EQ(ValueOne, + dynamic_metadata().getData("test_1").Access()); + EXPECT_EQ(1u, access_count_1); + EXPECT_EQ(0u, access_count_2); + EXPECT_EQ(ValueTwo, + dynamic_metadata().getData("test_2").Access()); + EXPECT_EQ(1u, access_count_1); + EXPECT_EQ(1u, access_count_2); + ResetDynamicMetadata(); + EXPECT_EQ(2u, destruction_count); +} + +TEST_F(DynamicMetadataImplTest, SimpleType) { + dynamic_metadata().setData("test_1", std::make_unique(1)); + dynamic_metadata().setData("test_2", std::make_unique(2)); + + EXPECT_EQ(1, dynamic_metadata().getData("test_1")); + EXPECT_EQ(2, dynamic_metadata().getData("test_2")); +} + +TEST_F(DynamicMetadataImplTest, NameConflict) { + dynamic_metadata().setData("test_1", std::make_unique(1)); + EXPECT_THROW(dynamic_metadata().setData("test_1", std::make_unique(2)), EnvoyException); +} + +TEST_F(DynamicMetadataImplTest, NameConflictDifferentTypes) { + dynamic_metadata().setData("test_1", std::make_unique(1)); + EXPECT_THROW(dynamic_metadata().setData("test_1", std::make_unique(2, nullptr, nullptr)), EnvoyException); +} + + } // namespace RequestInfo } // namespace Envoy From 653fdd738f3d04b74ba3a6120cce5f7f6ab05c68 Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Fri, 20 Jul 2018 16:21:38 -0400 Subject: [PATCH 05/30] Trivial request_info test. Signed-off-by: Randy Smith --- test/common/request_info/request_info_impl_test.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/common/request_info/request_info_impl_test.cc b/test/common/request_info/request_info_impl_test.cc index 687a6264a3c86..3f76498902568 100644 --- a/test/common/request_info/request_info_impl_test.cc +++ b/test/common/request_info/request_info_impl_test.cc @@ -140,6 +140,9 @@ TEST(RequestInfoImplTest, MiscSettersAndGetters) { NiceMock route_entry; request_info.route_entry_ = &route_entry; EXPECT_EQ(&route_entry, request_info.routeEntry()); + + request_info.dynamicMetadata2().setData("test", std::make_unique(1)); + EXPECT_EQ(1, request_info.dynamicMetadata2().getData("test")); } } From 36a35941366fc3e47999fd382d8934e8d525d444 Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Fri, 20 Jul 2018 17:10:25 -0400 Subject: [PATCH 06/30] Format fixed. Signed-off-by: Randy Smith --- include/envoy/request_info/BUILD | 5 +- include/envoy/request_info/dynamic_metadata.h | 37 +++++--------- include/envoy/request_info/string_accessor.h | 4 +- .../request_info/dynamic_metadata_impl.cc | 19 +++----- .../request_info/dynamic_metadata_impl.h | 25 ++++------ .../common/request_info/request_info_impl.h | 4 +- .../request_info/string_accessor_impl.h | 10 ++-- test/common/request_info/BUILD | 3 +- .../dynamic_metadata_impl_test.cc | 48 ++++++++----------- .../request_info/string_accessor_impl_test.cc | 1 - 10 files changed, 59 insertions(+), 97 deletions(-) diff --git a/include/envoy/request_info/BUILD b/include/envoy/request_info/BUILD index d06fc09a4c6aa..b520b87e77f63 100644 --- a/include/envoy/request_info/BUILD +++ b/include/envoy/request_info/BUILD @@ -13,7 +13,7 @@ envoy_cc_library( hdrs = ["request_info.h"], external_deps = ["abseil_optional"], deps = [ - ":dynamic_metadata_interface", + ":dynamic_metadata_interface", "//include/envoy/common:time_interface", "//include/envoy/http:protocol_interface", "//include/envoy/upstream:upstream_interface", @@ -22,8 +22,8 @@ envoy_cc_library( envoy_cc_library( name = "dynamic_metadata_interface", - hdrs = ["dynamic_metadata.h"], srcs = ["dynamic_metadata.cc"], + hdrs = ["dynamic_metadata.h"], external_deps = ["abseil_optional"], deps = [ ], @@ -36,4 +36,3 @@ envoy_cc_library( deps = [ ], ) - diff --git a/include/envoy/request_info/dynamic_metadata.h b/include/envoy/request_info/dynamic_metadata.h index f8e117e546587..ee968f9c84fb1 100644 --- a/include/envoy/request_info/dynamic_metadata.h +++ b/include/envoy/request_info/dynamic_metadata.h @@ -8,48 +8,37 @@ namespace Envoy { namespace RequestInfo { class DynamicMetadata { - public: - virtual ~DynamicMetadata() {}; +public: + virtual ~DynamicMetadata(){}; // May only be called once for a particular |data_name| - template - void setData( - absl::string_view data_name, - std::unique_ptr&& data) { + template void setData(absl::string_view data_name, std::unique_ptr&& data) { setDataGeneric(data_name, Traits::getTypeId(), static_cast(data.release()), &Traits::destructor); } - template - const T& getData(absl::string_view data_name) { + template const T& getData(absl::string_view data_name) { return *static_cast(getDataGeneric(data_name, Traits::getTypeId())); } - protected: - virtual void setDataGeneric( - absl::string_view data_name, - size_t type_id, - void* data, // Implementation must take ownership - void (*destructor)(void *)) PURE; +protected: + virtual void setDataGeneric(absl::string_view data_name, size_t type_id, + void* data, // Implementation must take ownership + void (*destructor)(void*)) PURE; - virtual void* getDataGeneric( - absl::string_view data_name, - size_t type_id) PURE; + virtual void* getDataGeneric(absl::string_view data_name, size_t type_id) PURE; - private: +private: // TODO(rdsmith): Is it ok to default this to presumably zero this way? static size_t type_id_index_; - template - class Traits { - public: + template class Traits { + public: static size_t getTypeId() { static const size_t type_id = ++type_id_index_; return type_id; } - static void destructor(void *ptr) { - delete static_cast(ptr); - } + static void destructor(void* ptr) { delete static_cast(ptr); } }; }; diff --git a/include/envoy/request_info/string_accessor.h b/include/envoy/request_info/string_accessor.h index 97f8d8b1e4daa..641dfbb142461 100644 --- a/include/envoy/request_info/string_accessor.h +++ b/include/envoy/request_info/string_accessor.h @@ -8,8 +8,8 @@ namespace Envoy { namespace RequestInfo { class StringAccessor { - public: - virtual ~StringAccessor() {}; +public: + virtual ~StringAccessor(){}; virtual absl::string_view asString() const PURE; }; diff --git a/source/common/request_info/dynamic_metadata_impl.cc b/source/common/request_info/dynamic_metadata_impl.cc index 7275bc962d640..1a781f6b89a8b 100644 --- a/source/common/request_info/dynamic_metadata_impl.cc +++ b/source/common/request_info/dynamic_metadata_impl.cc @@ -14,23 +14,18 @@ DynamicMetadataImpl::~DynamicMetadataImpl() { data_storage_.clear(); } -void DynamicMetadataImpl::setDataGeneric( - absl::string_view data_name, - size_t type_id, - void* data, - void (*destructor)(void*)) { +void DynamicMetadataImpl::setDataGeneric(absl::string_view data_name, size_t type_id, void* data, + void (*destructor)(void*)) { if (data_storage_.find(data_name) != data_storage_.end()) { throw EnvoyException("DynamicMetadata::setData called twice with same name."); } - data_storage_[static_cast(data_name)] = { type_id, data, destructor }; + data_storage_[static_cast(data_name)] = {type_id, data, destructor}; } -void* DynamicMetadataImpl::getDataGeneric( - absl::string_view data_name, - size_t type_id) { +void* DynamicMetadataImpl::getDataGeneric(absl::string_view data_name, size_t type_id) { const auto& it = data_storage_.find(data_name); - + if (it == data_storage_.end()) { throw EnvoyException("DynamicMetadata::getData called for unknown data name."); } @@ -39,11 +34,9 @@ void* DynamicMetadataImpl::getDataGeneric( throw EnvoyException( "DynamicMetadata::getData called with different type than name originally set with."); } - + return it->second.ptr_; } } // namespace RequestInfo } // namespace Envoy - - diff --git a/source/common/request_info/dynamic_metadata_impl.h b/source/common/request_info/dynamic_metadata_impl.h index 4591cf0403e12..af2798a3417a3 100644 --- a/source/common/request_info/dynamic_metadata_impl.h +++ b/source/common/request_info/dynamic_metadata_impl.h @@ -1,39 +1,34 @@ #pragma once -#include "envoy/request_info/dynamic_metadata.h" - #include +#include "envoy/request_info/dynamic_metadata.h" + #include "absl/strings/string_view.h" namespace Envoy { namespace RequestInfo { class DynamicMetadataImpl : public DynamicMetadata { - public: +public: DynamicMetadataImpl() {} ~DynamicMetadataImpl() override; // DynamicMetadata - void setDataGeneric( - absl::string_view data_name, - size_t type_id, - void* data, - void (*destructor)(void*)) override; + void setDataGeneric(absl::string_view data_name, size_t type_id, void* data, + void (*destructor)(void*)) override; - void* getDataGeneric( - absl::string_view data_name, - size_t type_id) override; + void* getDataGeneric(absl::string_view data_name, size_t type_id) override; - private: +private: struct Data { size_t typeid_; void* ptr_; - void (*destructor_)(void *); + void (*destructor_)(void*); }; std::map> data_storage_; -}; - +}; + } // namespace RequestInfo } // namespace Envoy diff --git a/source/common/request_info/request_info_impl.h b/source/common/request_info/request_info_impl.h index 862d8147d6926..8710012a7eef3 100644 --- a/source/common/request_info/request_info_impl.h +++ b/source/common/request_info/request_info_impl.h @@ -179,9 +179,7 @@ struct RequestInfoImpl : public RequestInfo { (*metadata_.mutable_filter_metadata())[name].MergeFrom(value); }; - DynamicMetadata& dynamicMetadata2() override { - return metadata2_; - } + DynamicMetadata& dynamicMetadata2() override { return metadata2_; } const SystemTime start_time_; const MonotonicTime start_time_monotonic_; diff --git a/source/common/request_info/string_accessor_impl.h b/source/common/request_info/string_accessor_impl.h index f70b6905ed6db..7ce837336985b 100644 --- a/source/common/request_info/string_accessor_impl.h +++ b/source/common/request_info/string_accessor_impl.h @@ -5,17 +5,15 @@ namespace Envoy { namespace RequestInfo { -class StringAccessorImpl : public StringAccessor { - public: +class StringAccessorImpl : public StringAccessor { +public: StringAccessorImpl(absl::string_view value) : value_(value) {} // StringAccessor ~StringAccessorImpl() override {} - absl::string_view asString() const override { - return value_; - } + absl::string_view asString() const override { return value_; } - private: +private: std::string value_; }; diff --git a/test/common/request_info/BUILD b/test/common/request_info/BUILD index 51fc9c577d302..99e4a6f21b28f 100644 --- a/test/common/request_info/BUILD +++ b/test/common/request_info/BUILD @@ -32,7 +32,7 @@ envoy_cc_test( name = "string_accessor_impl_test", srcs = ["string_accessor_impl_test.cc"], deps = [ - "//source/common/request_info:string_accessor_lib" + "//source/common/request_info:string_accessor_lib", ], ) @@ -44,4 +44,3 @@ envoy_cc_test( "//test/mocks/request_info:request_info_mocks", ], ) - diff --git a/test/common/request_info/dynamic_metadata_impl_test.cc b/test/common/request_info/dynamic_metadata_impl_test.cc index 36700ffc8b1c6..447c473b7f691 100644 --- a/test/common/request_info/dynamic_metadata_impl_test.cc +++ b/test/common/request_info/dynamic_metadata_impl_test.cc @@ -1,7 +1,7 @@ -#include "common/request_info/dynamic_metadata_impl.h" - #include "envoy/common/exception.h" +#include "common/request_info/dynamic_metadata_impl.h" + #include "gtest/gtest.h" namespace Envoy { @@ -9,7 +9,7 @@ namespace RequestInfo { namespace { class TestStoredType { - public: +public: TestStoredType(int value, size_t* access_count, size_t* destruction_count) : value_(value), access_count_(access_count), destruction_count_(destruction_count) {} ~TestStoredType() { @@ -25,24 +25,20 @@ class TestStoredType { return value_; } - private: +private: int value_; size_t* access_count_; size_t* destruction_count_; }; -class DynamicMetadataImplTest : public testing::Test { - public: - DynamicMetadataImplTest() { - ResetDynamicMetadata(); - } +class DynamicMetadataImplTest : public testing::Test { +public: + DynamicMetadataImplTest() { ResetDynamicMetadata(); } - void ResetDynamicMetadata() { - dynamic_metadata_ = std::make_unique(); - } + void ResetDynamicMetadata() { dynamic_metadata_ = std::make_unique(); } DynamicMetadata& dynamic_metadata() { return *dynamic_metadata_; } - private: +private: std::unique_ptr dynamic_metadata_; }; @@ -51,9 +47,8 @@ class DynamicMetadataImplTest : public testing::Test { TEST_F(DynamicMetadataImplTest, Simple) { size_t access_count = 0u; size_t destruction_count = 0u; - dynamic_metadata().setData("test_name", - std::make_unique( - 5, &access_count, &destruction_count)); + dynamic_metadata().setData( + "test_name", std::make_unique(5, &access_count, &destruction_count)); EXPECT_EQ(0u, access_count); EXPECT_EQ(0u, destruction_count); @@ -72,23 +67,19 @@ TEST_F(DynamicMetadataImplTest, SameTypes) { size_t destruction_count = 0u; const int ValueOne = 5; const int ValueTwo = 6; - + dynamic_metadata().setData( - "test_1", std::make_unique( - ValueOne, &access_count_1, &destruction_count)); + "test_1", std::make_unique(ValueOne, &access_count_1, &destruction_count)); dynamic_metadata().setData( - "test_2", std::make_unique( - ValueTwo, &access_count_2, &destruction_count)); + "test_2", std::make_unique(ValueTwo, &access_count_2, &destruction_count)); EXPECT_EQ(0u, access_count_1); EXPECT_EQ(0u, access_count_2); EXPECT_EQ(0u, destruction_count); - EXPECT_EQ(ValueOne, - dynamic_metadata().getData("test_1").Access()); + EXPECT_EQ(ValueOne, dynamic_metadata().getData("test_1").Access()); EXPECT_EQ(1u, access_count_1); EXPECT_EQ(0u, access_count_2); - EXPECT_EQ(ValueTwo, - dynamic_metadata().getData("test_2").Access()); + EXPECT_EQ(ValueTwo, dynamic_metadata().getData("test_2").Access()); EXPECT_EQ(1u, access_count_1); EXPECT_EQ(1u, access_count_2); ResetDynamicMetadata(); @@ -98,7 +89,7 @@ TEST_F(DynamicMetadataImplTest, SameTypes) { TEST_F(DynamicMetadataImplTest, SimpleType) { dynamic_metadata().setData("test_1", std::make_unique(1)); dynamic_metadata().setData("test_2", std::make_unique(2)); - + EXPECT_EQ(1, dynamic_metadata().getData("test_1")); EXPECT_EQ(2, dynamic_metadata().getData("test_2")); } @@ -110,9 +101,10 @@ TEST_F(DynamicMetadataImplTest, NameConflict) { TEST_F(DynamicMetadataImplTest, NameConflictDifferentTypes) { dynamic_metadata().setData("test_1", std::make_unique(1)); - EXPECT_THROW(dynamic_metadata().setData("test_1", std::make_unique(2, nullptr, nullptr)), EnvoyException); + EXPECT_THROW( + dynamic_metadata().setData("test_1", std::make_unique(2, nullptr, nullptr)), + EnvoyException); } - } // namespace RequestInfo } // namespace Envoy diff --git a/test/common/request_info/string_accessor_impl_test.cc b/test/common/request_info/string_accessor_impl_test.cc index e07773a7d1df6..3d58dda86a81a 100644 --- a/test/common/request_info/string_accessor_impl_test.cc +++ b/test/common/request_info/string_accessor_impl_test.cc @@ -1,7 +1,6 @@ #include "common/request_info/string_accessor_impl.h" #include "absl/strings/string_view.h" - #include "gtest/gtest.h" namespace Envoy { From 1bfe99ca436720bc26b5d6f0fc05e48964a53b0c Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Fri, 20 Jul 2018 18:22:23 -0400 Subject: [PATCH 07/30] Changes forced by asan testing. Signed-off-by: Randy Smith --- include/envoy/request_info/dynamic_metadata.h | 3 ++- source/common/request_info/dynamic_metadata_impl.cc | 1 + test/common/access_log/test_util.h | 3 +++ 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/include/envoy/request_info/dynamic_metadata.h b/include/envoy/request_info/dynamic_metadata.h index ee968f9c84fb1..4f8cf7d1501f1 100644 --- a/include/envoy/request_info/dynamic_metadata.h +++ b/include/envoy/request_info/dynamic_metadata.h @@ -4,6 +4,8 @@ #include "absl/strings/string_view.h" +#include + namespace Envoy { namespace RequestInfo { @@ -29,7 +31,6 @@ class DynamicMetadata { virtual void* getDataGeneric(absl::string_view data_name, size_t type_id) PURE; private: - // TODO(rdsmith): Is it ok to default this to presumably zero this way? static size_t type_id_index_; template class Traits { diff --git a/source/common/request_info/dynamic_metadata_impl.cc b/source/common/request_info/dynamic_metadata_impl.cc index 1a781f6b89a8b..aed6c4528708b 100644 --- a/source/common/request_info/dynamic_metadata_impl.cc +++ b/source/common/request_info/dynamic_metadata_impl.cc @@ -17,6 +17,7 @@ DynamicMetadataImpl::~DynamicMetadataImpl() { void DynamicMetadataImpl::setDataGeneric(absl::string_view data_name, size_t type_id, void* data, void (*destructor)(void*)) { if (data_storage_.find(data_name) != data_storage_.end()) { + (*destructor)(data); throw EnvoyException("DynamicMetadata::setData called twice with same name."); } diff --git a/test/common/access_log/test_util.h b/test/common/access_log/test_util.h index dc750cc900d22..5f33bf02b4b4c 100644 --- a/test/common/access_log/test_util.h +++ b/test/common/access_log/test_util.h @@ -154,6 +154,8 @@ class TestRequestInfo : public RequestInfo::RequestInfo { (*metadata_.mutable_filter_metadata())[name].MergeFrom(value); }; + ::Envoy::RequestInfo::DynamicMetadata& dynamicMetadata2() override { return metadata2_; } + SystemTime start_time_; MonotonicTime start_time_monotonic_; @@ -176,6 +178,7 @@ class TestRequestInfo : public RequestInfo::RequestInfo { Network::Address::InstanceConstSharedPtr downstream_remote_address_; const Router::RouteEntry* route_entry_{}; envoy::api::v2::core::Metadata metadata_{}; + ::Envoy::RequestInfo::DynamicMetadataImpl metadata2_{}; }; } // namespace Envoy From 603c58805aebb9d624cb4447457965e31c94f66d Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Fri, 20 Jul 2018 18:28:31 -0400 Subject: [PATCH 08/30] Got test to build. Signed-off-by: Randy Smith --- test/common/access_log/test_util.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/common/access_log/test_util.h b/test/common/access_log/test_util.h index 5f33bf02b4b4c..0308695669d52 100644 --- a/test/common/access_log/test_util.h +++ b/test/common/access_log/test_util.h @@ -2,6 +2,8 @@ #include "envoy/request_info/request_info.h" +#include "common/request_info/dynamic_metadata_impl.h" + namespace Envoy { class TestRequestInfo : public RequestInfo::RequestInfo { From 199a5c810076b8fc52742097e99785c065474bac Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Fri, 20 Jul 2018 18:57:26 -0400 Subject: [PATCH 09/30] Fixed format Signed-off-by: Randy Smith --- include/envoy/request_info/dynamic_metadata.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/envoy/request_info/dynamic_metadata.h b/include/envoy/request_info/dynamic_metadata.h index 4f8cf7d1501f1..0321b1436bcc8 100644 --- a/include/envoy/request_info/dynamic_metadata.h +++ b/include/envoy/request_info/dynamic_metadata.h @@ -1,11 +1,11 @@ #pragma once +#include + #include "envoy/common/pure.h" #include "absl/strings/string_view.h" -#include - namespace Envoy { namespace RequestInfo { From 229414daf10fc39b58ce1334f5ba6ade0e193e75 Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Tue, 24 Jul 2018 18:18:26 -0400 Subject: [PATCH 10/30] Incorporated most comments. Signed-off-by: Randy Smith --- include/envoy/request_info/dynamic_metadata.h | 16 ++++++++++++++- include/envoy/request_info/string_accessor.h | 8 ++++++++ .../request_info/dynamic_metadata_impl.cc | 6 ++++++ .../request_info/dynamic_metadata_impl.h | 1 + .../dynamic_metadata_impl_test.cc | 20 +++++++++++++++++++ 5 files changed, 50 insertions(+), 1 deletion(-) diff --git a/include/envoy/request_info/dynamic_metadata.h b/include/envoy/request_info/dynamic_metadata.h index 0321b1436bcc8..f351e5d821031 100644 --- a/include/envoy/request_info/dynamic_metadata.h +++ b/include/envoy/request_info/dynamic_metadata.h @@ -13,22 +13,36 @@ class DynamicMetadata { public: virtual ~DynamicMetadata(){}; - // May only be called once for a particular |data_name| + /** + * @param data_name the name of the data being set. + * @param data an owning pointer to the data to be stored. + * Note that it is an error to call setData() twice with the same data_name. + */ template void setData(absl::string_view data_name, std::unique_ptr&& data) { setDataGeneric(data_name, Traits::getTypeId(), static_cast(data.release()), &Traits::destructor); } + /** + * @param data_name the name of the data being set. + * @return a reference to the stored data. + * Note that it is an error to access data that has not previously been set. + */ template const T& getData(absl::string_view data_name) { return *static_cast(getDataGeneric(data_name, Traits::getTypeId())); } + template bool hasData(absl::string_view data_name) { + return hasDataGeneric(data_name, Traits::getTypeId()); + } + protected: virtual void setDataGeneric(absl::string_view data_name, size_t type_id, void* data, // Implementation must take ownership void (*destructor)(void*)) PURE; virtual void* getDataGeneric(absl::string_view data_name, size_t type_id) PURE; + virtual bool hasDataGeneric(absl::string_view data_name, size_t type_id) PURE; private: static size_t type_id_index_; diff --git a/include/envoy/request_info/string_accessor.h b/include/envoy/request_info/string_accessor.h index 641dfbb142461..22876a189e81a 100644 --- a/include/envoy/request_info/string_accessor.h +++ b/include/envoy/request_info/string_accessor.h @@ -7,9 +7,17 @@ namespace Envoy { namespace RequestInfo { +/** + * Contains a string in a form which is usable with DynamicMetadata and + * allows lazy evaluation if needed. + */ class StringAccessor { public: virtual ~StringAccessor(){}; + + /** + * @return the string the accessor represents. + */ virtual absl::string_view asString() const PURE; }; diff --git a/source/common/request_info/dynamic_metadata_impl.cc b/source/common/request_info/dynamic_metadata_impl.cc index aed6c4528708b..df9c7818ddbe8 100644 --- a/source/common/request_info/dynamic_metadata_impl.cc +++ b/source/common/request_info/dynamic_metadata_impl.cc @@ -39,5 +39,11 @@ void* DynamicMetadataImpl::getDataGeneric(absl::string_view data_name, size_t ty return it->second.ptr_; } +bool DynamicMetadataImpl::hasDataGeneric(absl::string_view data_name, size_t type_id) { + const auto& it = data_storage_.find(data_name); + + return (it != data_storage_.end() && it->second.typeid_ == type_id); +} + } // namespace RequestInfo } // namespace Envoy diff --git a/source/common/request_info/dynamic_metadata_impl.h b/source/common/request_info/dynamic_metadata_impl.h index af2798a3417a3..1b7aa37a410cc 100644 --- a/source/common/request_info/dynamic_metadata_impl.h +++ b/source/common/request_info/dynamic_metadata_impl.h @@ -19,6 +19,7 @@ class DynamicMetadataImpl : public DynamicMetadata { void (*destructor)(void*)) override; void* getDataGeneric(absl::string_view data_name, size_t type_id) override; + bool hasDataGeneric(absl::string_view data_name, size_t type_id) override; private: struct Data { diff --git a/test/common/request_info/dynamic_metadata_impl_test.cc b/test/common/request_info/dynamic_metadata_impl_test.cc index 447c473b7f691..3ffe15dc98bc5 100644 --- a/test/common/request_info/dynamic_metadata_impl_test.cc +++ b/test/common/request_info/dynamic_metadata_impl_test.cc @@ -97,6 +97,7 @@ TEST_F(DynamicMetadataImplTest, SimpleType) { TEST_F(DynamicMetadataImplTest, NameConflict) { dynamic_metadata().setData("test_1", std::make_unique(1)); EXPECT_THROW(dynamic_metadata().setData("test_1", std::make_unique(2)), EnvoyException); + EXPECT_EQ(1, dynamic_metadata().getData("test_1")); } TEST_F(DynamicMetadataImplTest, NameConflictDifferentTypes) { @@ -106,5 +107,24 @@ TEST_F(DynamicMetadataImplTest, NameConflictDifferentTypes) { EnvoyException); } +TEST_F(DynamicMetadataImplTest, UnknownName) { + EXPECT_THROW(dynamic_metadata().getData("test_1"), EnvoyException); +} + +TEST_F(DynamicMetadataImplTest, WrongTypeGet) { + dynamic_metadata().setData( + "test_name", std::make_unique(5, nullptr, nullptr)); + EXPECT_EQ(5, dynamic_metadata().getData("test_name").Access()); + EXPECT_THROW(dynamic_metadata().getData("test_name"), EnvoyException); +} + +TEST_F(DynamicMetadataImplTest, HasData) { + dynamic_metadata().setData("test_1", std::make_unique(1)); + EXPECT_TRUE(dynamic_metadata().hasData("test_1")); + EXPECT_FALSE(dynamic_metadata().hasData("test_2")); + EXPECT_FALSE(dynamic_metadata().hasData("test_1")); + EXPECT_FALSE(dynamic_metadata().hasData("test_2")); +} + } // namespace RequestInfo } // namespace Envoy From 48f2246761eef662c3b9c7734b182fada3c3e18a Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Tue, 24 Jul 2018 18:22:48 -0400 Subject: [PATCH 11/30] Made type id thread safe. Signed-off-by: Randy Smith --- include/envoy/request_info/dynamic_metadata.cc | 2 +- include/envoy/request_info/dynamic_metadata.h | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/include/envoy/request_info/dynamic_metadata.cc b/include/envoy/request_info/dynamic_metadata.cc index a49de3017cd92..b0af97a3bdefb 100644 --- a/include/envoy/request_info/dynamic_metadata.cc +++ b/include/envoy/request_info/dynamic_metadata.cc @@ -3,7 +3,7 @@ namespace Envoy { namespace RequestInfo { -size_t DynamicMetadata::type_id_index_ = 0; +std::atomic DynamicMetadata::type_id_index_(0u); } // namespace RequestInfo } // namespace Envoy diff --git a/include/envoy/request_info/dynamic_metadata.h b/include/envoy/request_info/dynamic_metadata.h index f351e5d821031..5ca3e01019a5a 100644 --- a/include/envoy/request_info/dynamic_metadata.h +++ b/include/envoy/request_info/dynamic_metadata.h @@ -1,5 +1,6 @@ #pragma once +#include #include #include "envoy/common/pure.h" @@ -45,12 +46,12 @@ class DynamicMetadata { virtual bool hasDataGeneric(absl::string_view data_name, size_t type_id) PURE; private: - static size_t type_id_index_; + static std::atomic type_id_index_; template class Traits { public: static size_t getTypeId() { - static const size_t type_id = ++type_id_index_; + static const size_t type_id = type_id_index_.fetch_add(1); return type_id; } static void destructor(void* ptr) { delete static_cast(ptr); } From ed497c8a365ade9b934a1b01be03b90120d104e3 Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Wed, 25 Jul 2018 08:31:50 -0400 Subject: [PATCH 12/30] Removed insertion into RequestInfo Signed-off-by: Randy Smith --- include/envoy/request_info/request_info.h | 6 ------ source/common/request_info/request_info_impl.h | 3 --- test/common/access_log/test_util.h | 2 -- test/common/request_info/request_info_impl_test.cc | 3 --- test/mocks/request_info/mocks.h | 1 - 5 files changed, 15 deletions(-) diff --git a/include/envoy/request_info/request_info.h b/include/envoy/request_info/request_info.h index f309406b17faf..8d8a011196746 100644 --- a/include/envoy/request_info/request_info.h +++ b/include/envoy/request_info/request_info.h @@ -301,12 +301,6 @@ class RequestInfo { * the same key overriding existing. */ virtual void setDynamicMetadata(const std::string& name, const ProtobufWkt::Struct& value) PURE; - - /** - * More general dynamic metadata object. - * TODO(rdsmith): Replace uses of above with this object. - */ - virtual DynamicMetadata& dynamicMetadata2() PURE; }; } // namespace RequestInfo diff --git a/source/common/request_info/request_info_impl.h b/source/common/request_info/request_info_impl.h index 8710012a7eef3..7729d7c417471 100644 --- a/source/common/request_info/request_info_impl.h +++ b/source/common/request_info/request_info_impl.h @@ -179,8 +179,6 @@ struct RequestInfoImpl : public RequestInfo { (*metadata_.mutable_filter_metadata())[name].MergeFrom(value); }; - DynamicMetadata& dynamicMetadata2() override { return metadata2_; } - const SystemTime start_time_; const MonotonicTime start_time_monotonic_; @@ -200,7 +198,6 @@ struct RequestInfoImpl : public RequestInfo { bool hc_request_{}; const Router::RouteEntry* route_entry_{}; envoy::api::v2::core::Metadata metadata_{}; - DynamicMetadataImpl metadata2_{}; private: uint64_t bytes_received_{}; diff --git a/test/common/access_log/test_util.h b/test/common/access_log/test_util.h index 0308695669d52..f5a20225e2053 100644 --- a/test/common/access_log/test_util.h +++ b/test/common/access_log/test_util.h @@ -156,8 +156,6 @@ class TestRequestInfo : public RequestInfo::RequestInfo { (*metadata_.mutable_filter_metadata())[name].MergeFrom(value); }; - ::Envoy::RequestInfo::DynamicMetadata& dynamicMetadata2() override { return metadata2_; } - SystemTime start_time_; MonotonicTime start_time_monotonic_; diff --git a/test/common/request_info/request_info_impl_test.cc b/test/common/request_info/request_info_impl_test.cc index 3f76498902568..687a6264a3c86 100644 --- a/test/common/request_info/request_info_impl_test.cc +++ b/test/common/request_info/request_info_impl_test.cc @@ -140,9 +140,6 @@ TEST(RequestInfoImplTest, MiscSettersAndGetters) { NiceMock route_entry; request_info.route_entry_ = &route_entry; EXPECT_EQ(&route_entry, request_info.routeEntry()); - - request_info.dynamicMetadata2().setData("test", std::make_unique(1)); - EXPECT_EQ(1, request_info.dynamicMetadata2().getData("test")); } } diff --git a/test/mocks/request_info/mocks.h b/test/mocks/request_info/mocks.h index 474fc2bb44314..cafe6815e5bcc 100644 --- a/test/mocks/request_info/mocks.h +++ b/test/mocks/request_info/mocks.h @@ -60,7 +60,6 @@ class MockRequestInfo : public RequestInfo { MOCK_METHOD2(setDynamicMetadata, void(const std::string&, const ProtobufWkt::Struct&)); MOCK_METHOD3(setDynamicMetadata, void(const std::string&, const std::string&, const std::string&)); - MOCK_METHOD0(dynamicMetadata2, DynamicMetadata&()); std::shared_ptr> host_{ new testing::NiceMock()}; From dbbaec50fdece4633b6e51e4a7f47bd396c6f94d Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Wed, 25 Jul 2018 08:33:47 -0400 Subject: [PATCH 13/30] fit fix format Signed-off-by: Randy Smith --- include/envoy/request_info/dynamic_metadata.h | 4 ++-- include/envoy/request_info/string_accessor.h | 2 +- test/common/request_info/dynamic_metadata_impl_test.cc | 3 +-- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/include/envoy/request_info/dynamic_metadata.h b/include/envoy/request_info/dynamic_metadata.h index 5ca3e01019a5a..7d29f9d0b9a51 100644 --- a/include/envoy/request_info/dynamic_metadata.h +++ b/include/envoy/request_info/dynamic_metadata.h @@ -15,7 +15,7 @@ class DynamicMetadata { virtual ~DynamicMetadata(){}; /** - * @param data_name the name of the data being set. + * @param data_name the name of the data being set. * @param data an owning pointer to the data to be stored. * Note that it is an error to call setData() twice with the same data_name. */ @@ -25,7 +25,7 @@ class DynamicMetadata { } /** - * @param data_name the name of the data being set. + * @param data_name the name of the data being set. * @return a reference to the stored data. * Note that it is an error to access data that has not previously been set. */ diff --git a/include/envoy/request_info/string_accessor.h b/include/envoy/request_info/string_accessor.h index 22876a189e81a..fac65c36f2f75 100644 --- a/include/envoy/request_info/string_accessor.h +++ b/include/envoy/request_info/string_accessor.h @@ -8,7 +8,7 @@ namespace Envoy { namespace RequestInfo { /** - * Contains a string in a form which is usable with DynamicMetadata and + * Contains a string in a form which is usable with DynamicMetadata and * allows lazy evaluation if needed. */ class StringAccessor { diff --git a/test/common/request_info/dynamic_metadata_impl_test.cc b/test/common/request_info/dynamic_metadata_impl_test.cc index 3ffe15dc98bc5..0cf840643dafa 100644 --- a/test/common/request_info/dynamic_metadata_impl_test.cc +++ b/test/common/request_info/dynamic_metadata_impl_test.cc @@ -112,8 +112,7 @@ TEST_F(DynamicMetadataImplTest, UnknownName) { } TEST_F(DynamicMetadataImplTest, WrongTypeGet) { - dynamic_metadata().setData( - "test_name", std::make_unique(5, nullptr, nullptr)); + dynamic_metadata().setData("test_name", std::make_unique(5, nullptr, nullptr)); EXPECT_EQ(5, dynamic_metadata().getData("test_name").Access()); EXPECT_THROW(dynamic_metadata().getData("test_name"), EnvoyException); } From cedbac4959bf9fb44d40acadf517b16f14e84764 Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Wed, 25 Jul 2018 10:31:58 -0400 Subject: [PATCH 14/30] Cleanup Signed-off-by: Randy Smith --- source/common/request_info/BUILD | 1 - source/common/request_info/dynamic_metadata_impl.h | 1 - source/common/request_info/request_info_impl.h | 1 - test/common/access_log/test_util.h | 3 --- 4 files changed, 6 deletions(-) diff --git a/source/common/request_info/BUILD b/source/common/request_info/BUILD index 1418c975d84f0..95bade2468de4 100644 --- a/source/common/request_info/BUILD +++ b/source/common/request_info/BUILD @@ -12,7 +12,6 @@ envoy_cc_library( name = "request_info_lib", hdrs = ["request_info_impl.h"], deps = [ - ":dynamic_metadata_lib", "//include/envoy/request_info:request_info_interface", "//source/common/common:assert_lib", ], diff --git a/source/common/request_info/dynamic_metadata_impl.h b/source/common/request_info/dynamic_metadata_impl.h index 1b7aa37a410cc..f86c7e29a047e 100644 --- a/source/common/request_info/dynamic_metadata_impl.h +++ b/source/common/request_info/dynamic_metadata_impl.h @@ -17,7 +17,6 @@ class DynamicMetadataImpl : public DynamicMetadata { // DynamicMetadata void setDataGeneric(absl::string_view data_name, size_t type_id, void* data, void (*destructor)(void*)) override; - void* getDataGeneric(absl::string_view data_name, size_t type_id) override; bool hasDataGeneric(absl::string_view data_name, size_t type_id) override; diff --git a/source/common/request_info/request_info_impl.h b/source/common/request_info/request_info_impl.h index 7729d7c417471..f56f34ebc4e52 100644 --- a/source/common/request_info/request_info_impl.h +++ b/source/common/request_info/request_info_impl.h @@ -6,7 +6,6 @@ #include "envoy/request_info/request_info.h" #include "common/common/assert.h" -#include "common/request_info/dynamic_metadata_impl.h" namespace Envoy { namespace RequestInfo { diff --git a/test/common/access_log/test_util.h b/test/common/access_log/test_util.h index f5a20225e2053..dc750cc900d22 100644 --- a/test/common/access_log/test_util.h +++ b/test/common/access_log/test_util.h @@ -2,8 +2,6 @@ #include "envoy/request_info/request_info.h" -#include "common/request_info/dynamic_metadata_impl.h" - namespace Envoy { class TestRequestInfo : public RequestInfo::RequestInfo { @@ -178,7 +176,6 @@ class TestRequestInfo : public RequestInfo::RequestInfo { Network::Address::InstanceConstSharedPtr downstream_remote_address_; const Router::RouteEntry* route_entry_{}; envoy::api::v2::core::Metadata metadata_{}; - ::Envoy::RequestInfo::DynamicMetadataImpl metadata2_{}; }; } // namespace Envoy From 6864aa48064cc3ffec13e288e4d07d484412f44e Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Thu, 26 Jul 2018 14:31:05 -0400 Subject: [PATCH 15/30] Added doxygen comments and finer grained probe function Signed-off-by: Randy Smith --- include/envoy/request_info/dynamic_metadata.h | 14 +++++++++++++- .../common/request_info/dynamic_metadata_impl.cc | 8 ++++++-- source/common/request_info/dynamic_metadata_impl.h | 5 +++-- .../request_info/dynamic_metadata_impl_test.cc | 2 ++ 4 files changed, 24 insertions(+), 5 deletions(-) diff --git a/include/envoy/request_info/dynamic_metadata.h b/include/envoy/request_info/dynamic_metadata.h index 7d29f9d0b9a51..5debb04e2f8de 100644 --- a/include/envoy/request_info/dynamic_metadata.h +++ b/include/envoy/request_info/dynamic_metadata.h @@ -33,10 +33,22 @@ class DynamicMetadata { return *static_cast(getDataGeneric(data_name, Traits::getTypeId())); } - template bool hasData(absl::string_view data_name) { + /** + * @param data_name the name of the data being set. + * @return Whether data of the type and name specified exists in the + * data store. + */ + template bool hasData(absl::string_view data_name) const { return hasDataGeneric(data_name, Traits::getTypeId()); } + /** + * @param data_name the name of the data being set. + * @return Whether data of any type and the name specified exists in the + * data store. + */ + virtual bool hasDataWithName(absl::string_view data_name) const PURE; + protected: virtual void setDataGeneric(absl::string_view data_name, size_t type_id, void* data, // Implementation must take ownership diff --git a/source/common/request_info/dynamic_metadata_impl.cc b/source/common/request_info/dynamic_metadata_impl.cc index df9c7818ddbe8..7ede1863215f8 100644 --- a/source/common/request_info/dynamic_metadata_impl.cc +++ b/source/common/request_info/dynamic_metadata_impl.cc @@ -24,7 +24,7 @@ void DynamicMetadataImpl::setDataGeneric(absl::string_view data_name, size_t typ data_storage_[static_cast(data_name)] = {type_id, data, destructor}; } -void* DynamicMetadataImpl::getDataGeneric(absl::string_view data_name, size_t type_id) { +void* DynamicMetadataImpl::getDataGeneric(absl::string_view data_name, size_t type_id) const { const auto& it = data_storage_.find(data_name); if (it == data_storage_.end()) { @@ -39,11 +39,15 @@ void* DynamicMetadataImpl::getDataGeneric(absl::string_view data_name, size_t ty return it->second.ptr_; } -bool DynamicMetadataImpl::hasDataGeneric(absl::string_view data_name, size_t type_id) { +bool DynamicMetadataImpl::hasDataGeneric(absl::string_view data_name, size_t type_id) const { const auto& it = data_storage_.find(data_name); return (it != data_storage_.end() && it->second.typeid_ == type_id); } +bool DynamicMetadataImpl::hasDataWithName(absl::string_view data_name) const { + return (data_storage_.find(data_name) != data_storage_.end()); +} + } // namespace RequestInfo } // namespace Envoy diff --git a/source/common/request_info/dynamic_metadata_impl.h b/source/common/request_info/dynamic_metadata_impl.h index f86c7e29a047e..39727bb2c26f8 100644 --- a/source/common/request_info/dynamic_metadata_impl.h +++ b/source/common/request_info/dynamic_metadata_impl.h @@ -17,8 +17,9 @@ class DynamicMetadataImpl : public DynamicMetadata { // DynamicMetadata void setDataGeneric(absl::string_view data_name, size_t type_id, void* data, void (*destructor)(void*)) override; - void* getDataGeneric(absl::string_view data_name, size_t type_id) override; - bool hasDataGeneric(absl::string_view data_name, size_t type_id) override; + void* getDataGeneric(absl::string_view data_name, size_t type_id) const override; + bool hasDataGeneric(absl::string_view data_name, size_t type_id) const override; + bool hasDataWithName(absl::string_view data_name) const override; private: struct Data { diff --git a/test/common/request_info/dynamic_metadata_impl_test.cc b/test/common/request_info/dynamic_metadata_impl_test.cc index 0cf840643dafa..33cf7beb7eb1c 100644 --- a/test/common/request_info/dynamic_metadata_impl_test.cc +++ b/test/common/request_info/dynamic_metadata_impl_test.cc @@ -123,6 +123,8 @@ TEST_F(DynamicMetadataImplTest, HasData) { EXPECT_FALSE(dynamic_metadata().hasData("test_2")); EXPECT_FALSE(dynamic_metadata().hasData("test_1")); EXPECT_FALSE(dynamic_metadata().hasData("test_2")); + EXPECT_TRUE(dynamic_metadata().hasDataWithName("test_1")); + EXPECT_FALSE(dynamic_metadata().hasDataWithName("test_2")); } } // namespace RequestInfo From 18b3b57410102f8d8f1125ee6b67fc144857f7a8 Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Thu, 26 Jul 2018 14:48:26 -0400 Subject: [PATCH 16/30] Added missing const markers Signed-off-by: Randy Smith --- include/envoy/request_info/dynamic_metadata.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/envoy/request_info/dynamic_metadata.h b/include/envoy/request_info/dynamic_metadata.h index 5debb04e2f8de..53a60ee4a87da 100644 --- a/include/envoy/request_info/dynamic_metadata.h +++ b/include/envoy/request_info/dynamic_metadata.h @@ -29,7 +29,7 @@ class DynamicMetadata { * @return a reference to the stored data. * Note that it is an error to access data that has not previously been set. */ - template const T& getData(absl::string_view data_name) { + template const T& getData(absl::string_view data_name) const { return *static_cast(getDataGeneric(data_name, Traits::getTypeId())); } @@ -54,8 +54,8 @@ class DynamicMetadata { void* data, // Implementation must take ownership void (*destructor)(void*)) PURE; - virtual void* getDataGeneric(absl::string_view data_name, size_t type_id) PURE; - virtual bool hasDataGeneric(absl::string_view data_name, size_t type_id) PURE; + virtual void* getDataGeneric(absl::string_view data_name, size_t type_id) const PURE; + virtual bool hasDataGeneric(absl::string_view data_name, size_t type_id) const PURE; private: static std::atomic type_id_index_; From 0df9cfa073fbfb4ba6d92f65c63c6e1f28c0a970 Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Thu, 26 Jul 2018 14:51:00 -0400 Subject: [PATCH 17/30] dock fix Signed-off-by: Randy Smith --- include/envoy/request_info/dynamic_metadata.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/envoy/request_info/dynamic_metadata.h b/include/envoy/request_info/dynamic_metadata.h index 53a60ee4a87da..4ec4d210971ea 100644 --- a/include/envoy/request_info/dynamic_metadata.h +++ b/include/envoy/request_info/dynamic_metadata.h @@ -35,7 +35,7 @@ class DynamicMetadata { /** * @param data_name the name of the data being set. - * @return Whether data of the type and name specified exists in the + * @return Whether data of the type and name specified exists in the * data store. */ template bool hasData(absl::string_view data_name) const { @@ -44,7 +44,7 @@ class DynamicMetadata { /** * @param data_name the name of the data being set. - * @return Whether data of any type and the name specified exists in the + * @return Whether data of any type and the name specified exists in the * data store. */ virtual bool hasDataWithName(absl::string_view data_name) const PURE; From 510368d1c097e275b707820e081e7deb2d07501e Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Fri, 27 Jul 2018 13:16:08 -0400 Subject: [PATCH 18/30] Incoporated comments. Signed-off-by: Randy Smith --- include/envoy/request_info/BUILD | 4 ---- include/envoy/request_info/dynamic_metadata.h | 7 ++++--- source/common/request_info/dynamic_metadata_impl.cc | 1 - 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/include/envoy/request_info/BUILD b/include/envoy/request_info/BUILD index b520b87e77f63..2edefa8e316f0 100644 --- a/include/envoy/request_info/BUILD +++ b/include/envoy/request_info/BUILD @@ -25,14 +25,10 @@ envoy_cc_library( srcs = ["dynamic_metadata.cc"], hdrs = ["dynamic_metadata.h"], external_deps = ["abseil_optional"], - deps = [ - ], ) envoy_cc_library( name = "string_accessor_interface", hdrs = ["string_accessor.h"], external_deps = ["abseil_optional"], - deps = [ - ], ) diff --git a/include/envoy/request_info/dynamic_metadata.h b/include/envoy/request_info/dynamic_metadata.h index 4ec4d210971ea..b19577486c678 100644 --- a/include/envoy/request_info/dynamic_metadata.h +++ b/include/envoy/request_info/dynamic_metadata.h @@ -17,7 +17,8 @@ class DynamicMetadata { /** * @param data_name the name of the data being set. * @param data an owning pointer to the data to be stored. - * Note that it is an error to call setData() twice with the same data_name. + * Note that it is an error to call setData() twice with the same data_name; this is to + * enforce a single authoritative source for each piece of data stored in DynamicMetadata. */ template void setData(absl::string_view data_name, std::unique_ptr&& data) { setDataGeneric(data_name, Traits::getTypeId(), static_cast(data.release()), @@ -34,7 +35,7 @@ class DynamicMetadata { } /** - * @param data_name the name of the data being set. + * @param data_name the name of the data being probed. * @return Whether data of the type and name specified exists in the * data store. */ @@ -43,7 +44,7 @@ class DynamicMetadata { } /** - * @param data_name the name of the data being set. + * @param data_name the name of the data being probed. * @return Whether data of any type and the name specified exists in the * data store. */ diff --git a/source/common/request_info/dynamic_metadata_impl.cc b/source/common/request_info/dynamic_metadata_impl.cc index 7ede1863215f8..4e2c2d1b1a3e0 100644 --- a/source/common/request_info/dynamic_metadata_impl.cc +++ b/source/common/request_info/dynamic_metadata_impl.cc @@ -11,7 +11,6 @@ DynamicMetadataImpl::~DynamicMetadataImpl() { it.second.destructor_(it.second.ptr_); } } - data_storage_.clear(); } void DynamicMetadataImpl::setDataGeneric(absl::string_view data_name, size_t type_id, void* data, From 88fade49a7ed82000015bcba0cb335e2cf7e4921 Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Fri, 27 Jul 2018 13:19:46 -0400 Subject: [PATCH 19/30] dock fix Signed-off-by: Randy Smith --- include/envoy/request_info/dynamic_metadata.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/envoy/request_info/dynamic_metadata.h b/include/envoy/request_info/dynamic_metadata.h index b19577486c678..8ef60482df6cd 100644 --- a/include/envoy/request_info/dynamic_metadata.h +++ b/include/envoy/request_info/dynamic_metadata.h @@ -18,7 +18,7 @@ class DynamicMetadata { * @param data_name the name of the data being set. * @param data an owning pointer to the data to be stored. * Note that it is an error to call setData() twice with the same data_name; this is to - * enforce a single authoritative source for each piece of data stored in DynamicMetadata. + * enforce a single authoritative source for each piece of data stored in DynamicMetadata. */ template void setData(absl::string_view data_name, std::unique_ptr&& data) { setDataGeneric(data_name, Traits::getTypeId(), static_cast(data.release()), From 66d640ea5b034238ace059fa604475b742050fdf Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Fri, 27 Jul 2018 13:30:20 -0400 Subject: [PATCH 20/30] Removed StringAccessor. Signed-off-by: Randy Smith --- include/envoy/request_info/BUILD | 6 ----- include/envoy/request_info/string_accessor.h | 25 ------------------- source/common/request_info/BUILD | 8 ------ .../request_info/string_accessor_impl.h | 21 ---------------- test/common/request_info/BUILD | 8 ------ .../request_info/string_accessor_impl_test.cc | 17 ------------- 6 files changed, 85 deletions(-) delete mode 100644 include/envoy/request_info/string_accessor.h delete mode 100644 source/common/request_info/string_accessor_impl.h delete mode 100644 test/common/request_info/string_accessor_impl_test.cc diff --git a/include/envoy/request_info/BUILD b/include/envoy/request_info/BUILD index 2edefa8e316f0..fbd6cb8f51f37 100644 --- a/include/envoy/request_info/BUILD +++ b/include/envoy/request_info/BUILD @@ -26,9 +26,3 @@ envoy_cc_library( hdrs = ["dynamic_metadata.h"], external_deps = ["abseil_optional"], ) - -envoy_cc_library( - name = "string_accessor_interface", - hdrs = ["string_accessor.h"], - external_deps = ["abseil_optional"], -) diff --git a/include/envoy/request_info/string_accessor.h b/include/envoy/request_info/string_accessor.h deleted file mode 100644 index fac65c36f2f75..0000000000000 --- a/include/envoy/request_info/string_accessor.h +++ /dev/null @@ -1,25 +0,0 @@ -#pragma once - -#include "envoy/common/pure.h" - -#include "absl/strings/string_view.h" - -namespace Envoy { -namespace RequestInfo { - -/** - * Contains a string in a form which is usable with DynamicMetadata and - * allows lazy evaluation if needed. - */ -class StringAccessor { -public: - virtual ~StringAccessor(){}; - - /** - * @return the string the accessor represents. - */ - virtual absl::string_view asString() const PURE; -}; - -} // namespace RequestInfo -} // namespace Envoy diff --git a/source/common/request_info/BUILD b/source/common/request_info/BUILD index 95bade2468de4..f94d5bcf65da2 100644 --- a/source/common/request_info/BUILD +++ b/source/common/request_info/BUILD @@ -26,14 +26,6 @@ envoy_cc_library( ], ) -envoy_cc_library( - name = "string_accessor_lib", - hdrs = ["string_accessor_impl.h"], - deps = [ - "//include/envoy/request_info:string_accessor_interface", - ], -) - envoy_cc_library( name = "utility_lib", srcs = ["utility.cc"], diff --git a/source/common/request_info/string_accessor_impl.h b/source/common/request_info/string_accessor_impl.h deleted file mode 100644 index 7ce837336985b..0000000000000 --- a/source/common/request_info/string_accessor_impl.h +++ /dev/null @@ -1,21 +0,0 @@ -#pragma once - -#include "envoy/request_info/string_accessor.h" - -namespace Envoy { -namespace RequestInfo { - -class StringAccessorImpl : public StringAccessor { -public: - StringAccessorImpl(absl::string_view value) : value_(value) {} - - // StringAccessor - ~StringAccessorImpl() override {} - absl::string_view asString() const override { return value_; } - -private: - std::string value_; -}; - -} // namespace RequestInfo -} // namespace Envoy diff --git a/test/common/request_info/BUILD b/test/common/request_info/BUILD index 99e4a6f21b28f..5a78017e37665 100644 --- a/test/common/request_info/BUILD +++ b/test/common/request_info/BUILD @@ -28,14 +28,6 @@ envoy_cc_test( ], ) -envoy_cc_test( - name = "string_accessor_impl_test", - srcs = ["string_accessor_impl_test.cc"], - deps = [ - "//source/common/request_info:string_accessor_lib", - ], -) - envoy_cc_test( name = "utility_test", srcs = ["utility_test.cc"], diff --git a/test/common/request_info/string_accessor_impl_test.cc b/test/common/request_info/string_accessor_impl_test.cc deleted file mode 100644 index 3d58dda86a81a..0000000000000 --- a/test/common/request_info/string_accessor_impl_test.cc +++ /dev/null @@ -1,17 +0,0 @@ -#include "common/request_info/string_accessor_impl.h" - -#include "absl/strings/string_view.h" -#include "gtest/gtest.h" - -namespace Envoy { -namespace RequestInfo { - -TEST(StringAccessorImplTest, Storage) { - const char* const TestString = "test string 1"; - StringAccessorImpl accessor(TestString); - - EXPECT_EQ(TestString, accessor.asString()); -} - -} // namespace RequestInfo -} // namespace Envoy From c5f3775142b1c6f1d29fb1599d9cf43fc19f712b Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Tue, 31 Jul 2018 13:07:46 -0400 Subject: [PATCH 21/30] Shifted source associated with template MP to source/common Signed-off-by: Randy Smith --- include/envoy/request_info/BUILD | 9 ++++++++- source/common/request_info/BUILD | 8 ++++++++ .../common}/request_info/dynamic_metadata.cc | 0 3 files changed, 16 insertions(+), 1 deletion(-) rename {include/envoy => source/common}/request_info/dynamic_metadata.cc (100%) diff --git a/include/envoy/request_info/BUILD b/include/envoy/request_info/BUILD index fbd6cb8f51f37..19324e921725e 100644 --- a/include/envoy/request_info/BUILD +++ b/include/envoy/request_info/BUILD @@ -22,7 +22,14 @@ envoy_cc_library( envoy_cc_library( name = "dynamic_metadata_interface", - srcs = ["dynamic_metadata.cc"], + deps = [ + ":dynamic_metadata_interface_hdr", + "//source/common/request_info:dynamic_metadata_interface_lib", + ], +) + +envoy_cc_library( + name = "dynamic_metadata_interface_hdr", hdrs = ["dynamic_metadata.h"], external_deps = ["abseil_optional"], ) diff --git a/source/common/request_info/BUILD b/source/common/request_info/BUILD index f94d5bcf65da2..ce6131a1d4914 100644 --- a/source/common/request_info/BUILD +++ b/source/common/request_info/BUILD @@ -17,6 +17,14 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "dynamic_metadata_interface_lib", + srcs = ["dynamic_metadata.cc"], + deps = [ + "//include/envoy/request_info:dynamic_metadata_interface_hdr", + ], +) + envoy_cc_library( name = "dynamic_metadata_lib", srcs = ["dynamic_metadata_impl.cc"], diff --git a/include/envoy/request_info/dynamic_metadata.cc b/source/common/request_info/dynamic_metadata.cc similarity index 100% rename from include/envoy/request_info/dynamic_metadata.cc rename to source/common/request_info/dynamic_metadata.cc From 4a89207a2e5cac54e8b2fdb33cf9d084a9864f55 Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Wed, 1 Aug 2018 14:02:31 -0400 Subject: [PATCH 22/30] Switched implementation over to dynamic_cast<> with all that represents. Signed-off-by: Randy Smith --- include/envoy/request_info/BUILD | 8 ---- include/envoy/request_info/dynamic_metadata.h | 45 +++++++++---------- source/common/request_info/BUILD | 8 ---- .../common/request_info/dynamic_metadata.cc | 9 ---- .../request_info/dynamic_metadata_impl.cc | 36 +++------------ .../request_info/dynamic_metadata_impl.h | 18 +++----- 6 files changed, 30 insertions(+), 94 deletions(-) delete mode 100644 source/common/request_info/dynamic_metadata.cc diff --git a/include/envoy/request_info/BUILD b/include/envoy/request_info/BUILD index 19324e921725e..09a30c66a8672 100644 --- a/include/envoy/request_info/BUILD +++ b/include/envoy/request_info/BUILD @@ -22,14 +22,6 @@ envoy_cc_library( envoy_cc_library( name = "dynamic_metadata_interface", - deps = [ - ":dynamic_metadata_interface_hdr", - "//source/common/request_info:dynamic_metadata_interface_lib", - ], -) - -envoy_cc_library( - name = "dynamic_metadata_interface_hdr", hdrs = ["dynamic_metadata.h"], external_deps = ["abseil_optional"], ) diff --git a/include/envoy/request_info/dynamic_metadata.h b/include/envoy/request_info/dynamic_metadata.h index b19577486c678..59bd8aa31ab3a 100644 --- a/include/envoy/request_info/dynamic_metadata.h +++ b/include/envoy/request_info/dynamic_metadata.h @@ -3,6 +3,7 @@ #include #include +#include "envoy/common/exception.h" #include "envoy/common/pure.h" #include "absl/strings/string_view.h" @@ -12,6 +13,11 @@ namespace RequestInfo { class DynamicMetadata { public: + class DynamicMetadataObject { + public: + virtual ~DynamicMetadataObject() {}; + }; + virtual ~DynamicMetadata(){}; /** @@ -20,18 +26,22 @@ class DynamicMetadata { * Note that it is an error to call setData() twice with the same data_name; this is to * enforce a single authoritative source for each piece of data stored in DynamicMetadata. */ - template void setData(absl::string_view data_name, std::unique_ptr&& data) { - setDataGeneric(data_name, Traits::getTypeId(), static_cast(data.release()), - &Traits::destructor); - } + virtual void setData(absl::string_view data_name, + std::unique_ptr&& data) PURE; /** * @param data_name the name of the data being set. * @return a reference to the stored data. * Note that it is an error to access data that has not previously been set. + * This function will fail if the data stored under |data_name| cannot be + * dynamically cast to the type specified. */ template const T& getData(absl::string_view data_name) const { - return *static_cast(getDataGeneric(data_name, Traits::getTypeId())); + const T* result = dynamic_cast(getDataGeneric(data_name)); + if (!result) { + throw EnvoyException("Data stored under {} cannot be coerced to specified type", data_name); + } + return *result; } /** @@ -40,7 +50,7 @@ class DynamicMetadata { * data store. */ template bool hasData(absl::string_view data_name) const { - return hasDataGeneric(data_name, Traits::getTypeId()); + return (dynamic_cast(getDataGeneric(data_name)) != nullptr); } /** @@ -48,27 +58,12 @@ class DynamicMetadata { * @return Whether data of any type and the name specified exists in the * data store. */ - virtual bool hasDataWithName(absl::string_view data_name) const PURE; + bool hasDataWithName(absl::string_view data_name) const { + return (getDataGeneric(data_name) != nullptr); + } protected: - virtual void setDataGeneric(absl::string_view data_name, size_t type_id, - void* data, // Implementation must take ownership - void (*destructor)(void*)) PURE; - - virtual void* getDataGeneric(absl::string_view data_name, size_t type_id) const PURE; - virtual bool hasDataGeneric(absl::string_view data_name, size_t type_id) const PURE; - -private: - static std::atomic type_id_index_; - - template class Traits { - public: - static size_t getTypeId() { - static const size_t type_id = type_id_index_.fetch_add(1); - return type_id; - } - static void destructor(void* ptr) { delete static_cast(ptr); } - }; + virtual const DynamicMetadataObject* getDataGeneric(absl::string_view data_name) const PURE; }; } // namespace RequestInfo diff --git a/source/common/request_info/BUILD b/source/common/request_info/BUILD index ce6131a1d4914..f94d5bcf65da2 100644 --- a/source/common/request_info/BUILD +++ b/source/common/request_info/BUILD @@ -17,14 +17,6 @@ envoy_cc_library( ], ) -envoy_cc_library( - name = "dynamic_metadata_interface_lib", - srcs = ["dynamic_metadata.cc"], - deps = [ - "//include/envoy/request_info:dynamic_metadata_interface_hdr", - ], -) - envoy_cc_library( name = "dynamic_metadata_lib", srcs = ["dynamic_metadata_impl.cc"], diff --git a/source/common/request_info/dynamic_metadata.cc b/source/common/request_info/dynamic_metadata.cc deleted file mode 100644 index b0af97a3bdefb..0000000000000 --- a/source/common/request_info/dynamic_metadata.cc +++ /dev/null @@ -1,9 +0,0 @@ -#include "envoy/request_info/dynamic_metadata.h" - -namespace Envoy { -namespace RequestInfo { - -std::atomic DynamicMetadata::type_id_index_(0u); - -} // namespace RequestInfo -} // namespace Envoy diff --git a/source/common/request_info/dynamic_metadata_impl.cc b/source/common/request_info/dynamic_metadata_impl.cc index 4e2c2d1b1a3e0..42807d1a4cbb2 100644 --- a/source/common/request_info/dynamic_metadata_impl.cc +++ b/source/common/request_info/dynamic_metadata_impl.cc @@ -5,47 +5,21 @@ namespace Envoy { namespace RequestInfo { -DynamicMetadataImpl::~DynamicMetadataImpl() { - for (auto& it : data_storage_) { - if (it.second.destructor_) { - it.second.destructor_(it.second.ptr_); - } - } -} - -void DynamicMetadataImpl::setDataGeneric(absl::string_view data_name, size_t type_id, void* data, - void (*destructor)(void*)) { +void DynamicMetadataImpl::setData(absl::string_view data_name, + std::unique_ptr&& data) { if (data_storage_.find(data_name) != data_storage_.end()) { - (*destructor)(data); throw EnvoyException("DynamicMetadata::setData called twice with same name."); } - - data_storage_[static_cast(data_name)] = {type_id, data, destructor}; + data_storage_[static_cast(data_name)] = std::move(data); } -void* DynamicMetadataImpl::getDataGeneric(absl::string_view data_name, size_t type_id) const { +const DynamicMetadata::DynamicMetadataObject* DynamicMetadataImpl::getDataGeneric(absl::string_view data_name) const { const auto& it = data_storage_.find(data_name); if (it == data_storage_.end()) { throw EnvoyException("DynamicMetadata::getData called for unknown data name."); } - - if (it->second.typeid_ != type_id) { - throw EnvoyException( - "DynamicMetadata::getData called with different type than name originally set with."); - } - - return it->second.ptr_; -} - -bool DynamicMetadataImpl::hasDataGeneric(absl::string_view data_name, size_t type_id) const { - const auto& it = data_storage_.find(data_name); - - return (it != data_storage_.end() && it->second.typeid_ == type_id); -} - -bool DynamicMetadataImpl::hasDataWithName(absl::string_view data_name) const { - return (data_storage_.find(data_name) != data_storage_.end()); + return it->second.get(); } } // namespace RequestInfo diff --git a/source/common/request_info/dynamic_metadata_impl.h b/source/common/request_info/dynamic_metadata_impl.h index 39727bb2c26f8..d11591ce0a42e 100644 --- a/source/common/request_info/dynamic_metadata_impl.h +++ b/source/common/request_info/dynamic_metadata_impl.h @@ -12,23 +12,15 @@ namespace RequestInfo { class DynamicMetadataImpl : public DynamicMetadata { public: DynamicMetadataImpl() {} - ~DynamicMetadataImpl() override; + ~DynamicMetadataImpl() override {}; // DynamicMetadata - void setDataGeneric(absl::string_view data_name, size_t type_id, void* data, - void (*destructor)(void*)) override; - void* getDataGeneric(absl::string_view data_name, size_t type_id) const override; - bool hasDataGeneric(absl::string_view data_name, size_t type_id) const override; - bool hasDataWithName(absl::string_view data_name) const override; + void setData(absl::string_view data_name, + std::unique_ptr&& data) override; + const DynamicMetadataObject* getDataGeneric(absl::string_view data_name) const override; private: - struct Data { - size_t typeid_; - void* ptr_; - void (*destructor_)(void*); - }; - - std::map> data_storage_; + std::map, std::less<>> data_storage_; }; } // namespace RequestInfo From d76a5fe0787f8c3903ccc2ff55ffd63a067ff549 Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Wed, 1 Aug 2018 15:19:04 -0400 Subject: [PATCH 23/30] ... and finished tests. Signed-off-by: Randy Smith --- include/envoy/request_info/dynamic_metadata.h | 10 +-- .../request_info/dynamic_metadata_impl.cc | 4 + .../request_info/dynamic_metadata_impl.h | 1 + .../dynamic_metadata_impl_test.cc | 89 +++++++++++++------ 4 files changed, 71 insertions(+), 33 deletions(-) diff --git a/include/envoy/request_info/dynamic_metadata.h b/include/envoy/request_info/dynamic_metadata.h index 59bd8aa31ab3a..ab37920b59ece 100644 --- a/include/envoy/request_info/dynamic_metadata.h +++ b/include/envoy/request_info/dynamic_metadata.h @@ -4,6 +4,7 @@ #include #include "envoy/common/exception.h" +#include "common/common/fmt.h" #include "envoy/common/pure.h" #include "absl/strings/string_view.h" @@ -39,7 +40,7 @@ class DynamicMetadata { template const T& getData(absl::string_view data_name) const { const T* result = dynamic_cast(getDataGeneric(data_name)); if (!result) { - throw EnvoyException("Data stored under {} cannot be coerced to specified type", data_name); + throw EnvoyException(fmt::format("Data stored under {} cannot be coerced to specified type", data_name)); } return *result; } @@ -50,7 +51,8 @@ class DynamicMetadata { * data store. */ template bool hasData(absl::string_view data_name) const { - return (dynamic_cast(getDataGeneric(data_name)) != nullptr); + return (hasDataWithName(data_name) && + (dynamic_cast(getDataGeneric(data_name)) != nullptr)); } /** @@ -58,9 +60,7 @@ class DynamicMetadata { * @return Whether data of any type and the name specified exists in the * data store. */ - bool hasDataWithName(absl::string_view data_name) const { - return (getDataGeneric(data_name) != nullptr); - } + virtual bool hasDataWithName(absl::string_view data_name) const PURE; protected: virtual const DynamicMetadataObject* getDataGeneric(absl::string_view data_name) const PURE; diff --git a/source/common/request_info/dynamic_metadata_impl.cc b/source/common/request_info/dynamic_metadata_impl.cc index 42807d1a4cbb2..b9b6431d31fa0 100644 --- a/source/common/request_info/dynamic_metadata_impl.cc +++ b/source/common/request_info/dynamic_metadata_impl.cc @@ -13,6 +13,10 @@ void DynamicMetadataImpl::setData(absl::string_view data_name, data_storage_[static_cast(data_name)] = std::move(data); } +bool DynamicMetadataImpl::hasDataWithName(absl::string_view data_name) const { + return (data_storage_.count(data_name) > 0); +} + const DynamicMetadata::DynamicMetadataObject* DynamicMetadataImpl::getDataGeneric(absl::string_view data_name) const { const auto& it = data_storage_.find(data_name); diff --git a/source/common/request_info/dynamic_metadata_impl.h b/source/common/request_info/dynamic_metadata_impl.h index d11591ce0a42e..468d2f31ac925 100644 --- a/source/common/request_info/dynamic_metadata_impl.h +++ b/source/common/request_info/dynamic_metadata_impl.h @@ -17,6 +17,7 @@ class DynamicMetadataImpl : public DynamicMetadata { // DynamicMetadata void setData(absl::string_view data_name, std::unique_ptr&& data) override; + bool hasDataWithName(absl::string_view) const override; const DynamicMetadataObject* getDataGeneric(absl::string_view data_name) const override; private: diff --git a/test/common/request_info/dynamic_metadata_impl_test.cc b/test/common/request_info/dynamic_metadata_impl_test.cc index 33cf7beb7eb1c..bbe1be5f4e992 100644 --- a/test/common/request_info/dynamic_metadata_impl_test.cc +++ b/test/common/request_info/dynamic_metadata_impl_test.cc @@ -8,11 +8,11 @@ namespace Envoy { namespace RequestInfo { namespace { -class TestStoredType { -public: - TestStoredType(int value, size_t* access_count, size_t* destruction_count) +class TestStoredTypeTracking : public DynamicMetadata::DynamicMetadataObject { + public: + TestStoredTypeTracking(int value, size_t* access_count, size_t* destruction_count) : value_(value), access_count_(access_count), destruction_count_(destruction_count) {} - ~TestStoredType() { + ~TestStoredTypeTracking() { if (destruction_count_) { ++*destruction_count_; } @@ -31,6 +31,16 @@ class TestStoredType { size_t* destruction_count_; }; +class SimpleType : public DynamicMetadata::DynamicMetadataObject { + public: + SimpleType(int value) : value_(value) {} + + int Access() const { return value_; } + + private: + int value_; +}; + class DynamicMetadataImplTest : public testing::Test { public: DynamicMetadataImplTest() { ResetDynamicMetadata(); } @@ -48,11 +58,11 @@ TEST_F(DynamicMetadataImplTest, Simple) { size_t access_count = 0u; size_t destruction_count = 0u; dynamic_metadata().setData( - "test_name", std::make_unique(5, &access_count, &destruction_count)); + "test_name", std::make_unique(5, &access_count, &destruction_count)); EXPECT_EQ(0u, access_count); EXPECT_EQ(0u, destruction_count); - EXPECT_EQ(5, dynamic_metadata().getData("test_name").Access()); + EXPECT_EQ(5, dynamic_metadata().getData("test_name").Access()); EXPECT_EQ(1u, access_count); EXPECT_EQ(0u, destruction_count); @@ -69,17 +79,17 @@ TEST_F(DynamicMetadataImplTest, SameTypes) { const int ValueTwo = 6; dynamic_metadata().setData( - "test_1", std::make_unique(ValueOne, &access_count_1, &destruction_count)); + "test_1", std::make_unique(ValueOne, &access_count_1, &destruction_count)); dynamic_metadata().setData( - "test_2", std::make_unique(ValueTwo, &access_count_2, &destruction_count)); + "test_2", std::make_unique(ValueTwo, &access_count_2, &destruction_count)); EXPECT_EQ(0u, access_count_1); EXPECT_EQ(0u, access_count_2); EXPECT_EQ(0u, destruction_count); - EXPECT_EQ(ValueOne, dynamic_metadata().getData("test_1").Access()); + EXPECT_EQ(ValueOne, dynamic_metadata().getData("test_1").Access()); EXPECT_EQ(1u, access_count_1); EXPECT_EQ(0u, access_count_2); - EXPECT_EQ(ValueTwo, dynamic_metadata().getData("test_2").Access()); + EXPECT_EQ(ValueTwo, dynamic_metadata().getData("test_2").Access()); EXPECT_EQ(1u, access_count_1); EXPECT_EQ(1u, access_count_2); ResetDynamicMetadata(); @@ -87,42 +97,65 @@ TEST_F(DynamicMetadataImplTest, SameTypes) { } TEST_F(DynamicMetadataImplTest, SimpleType) { - dynamic_metadata().setData("test_1", std::make_unique(1)); - dynamic_metadata().setData("test_2", std::make_unique(2)); + dynamic_metadata().setData("test_1", std::make_unique(1)); + dynamic_metadata().setData("test_2", std::make_unique(2)); - EXPECT_EQ(1, dynamic_metadata().getData("test_1")); - EXPECT_EQ(2, dynamic_metadata().getData("test_2")); + EXPECT_EQ(1, dynamic_metadata().getData("test_1").Access()); + EXPECT_EQ(2, dynamic_metadata().getData("test_2").Access()); } TEST_F(DynamicMetadataImplTest, NameConflict) { - dynamic_metadata().setData("test_1", std::make_unique(1)); - EXPECT_THROW(dynamic_metadata().setData("test_1", std::make_unique(2)), EnvoyException); - EXPECT_EQ(1, dynamic_metadata().getData("test_1")); + dynamic_metadata().setData("test_1", std::make_unique(1)); + EXPECT_THROW(dynamic_metadata().setData("test_1", std::make_unique(2)), EnvoyException); + EXPECT_EQ(1, dynamic_metadata().getData("test_1").Access()); } TEST_F(DynamicMetadataImplTest, NameConflictDifferentTypes) { - dynamic_metadata().setData("test_1", std::make_unique(1)); + dynamic_metadata().setData("test_1", std::make_unique(1)); EXPECT_THROW( - dynamic_metadata().setData("test_1", std::make_unique(2, nullptr, nullptr)), + dynamic_metadata().setData("test_1", std::make_unique(2, nullptr, nullptr)), EnvoyException); } TEST_F(DynamicMetadataImplTest, UnknownName) { - EXPECT_THROW(dynamic_metadata().getData("test_1"), EnvoyException); + EXPECT_THROW(dynamic_metadata().getData("test_1"), EnvoyException); } TEST_F(DynamicMetadataImplTest, WrongTypeGet) { - dynamic_metadata().setData("test_name", std::make_unique(5, nullptr, nullptr)); - EXPECT_EQ(5, dynamic_metadata().getData("test_name").Access()); - EXPECT_THROW(dynamic_metadata().getData("test_name"), EnvoyException); + dynamic_metadata().setData("test_name", std::make_unique(5, nullptr, nullptr)); + EXPECT_EQ(5, dynamic_metadata().getData("test_name").Access()); + EXPECT_THROW(dynamic_metadata().getData("test_name"), EnvoyException); +} + +namespace { + +class A : public DynamicMetadata::DynamicMetadataObject { +}; + +class B: public A {}; + +class C : public B {}; + +} // namespace + +TEST_F(DynamicMetadataImplTest, FungibleInheritance) { + dynamic_metadata().setData("testB", std::make_unique()); + EXPECT_TRUE(dynamic_metadata().hasData("testB")); + EXPECT_TRUE(dynamic_metadata().hasData("testB")); + EXPECT_FALSE(dynamic_metadata().hasData("testB")); + + dynamic_metadata().setData("testC", std::make_unique()); + EXPECT_TRUE(dynamic_metadata().hasData("testC")); + EXPECT_TRUE(dynamic_metadata().hasData("testC")); + EXPECT_TRUE(dynamic_metadata().hasData("testC")); } TEST_F(DynamicMetadataImplTest, HasData) { - dynamic_metadata().setData("test_1", std::make_unique(1)); - EXPECT_TRUE(dynamic_metadata().hasData("test_1")); - EXPECT_FALSE(dynamic_metadata().hasData("test_2")); - EXPECT_FALSE(dynamic_metadata().hasData("test_1")); - EXPECT_FALSE(dynamic_metadata().hasData("test_2")); + dynamic_metadata().setData("test_1", std::make_unique(1)); + EXPECT_TRUE(dynamic_metadata().hasData("test_1")); + EXPECT_FALSE(dynamic_metadata().hasData("test_2")); + EXPECT_FALSE(dynamic_metadata().hasData("test_1")); + EXPECT_FALSE(dynamic_metadata().hasData("test_2")); EXPECT_TRUE(dynamic_metadata().hasDataWithName("test_1")); EXPECT_FALSE(dynamic_metadata().hasDataWithName("test_2")); } From e59f4be95a4952b0aae614c53967c8a048c66293 Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Wed, 1 Aug 2018 15:32:18 -0400 Subject: [PATCH 24/30] Incorporated rest of comments. Signed-off-by: Randy Smith --- .../request_info/dynamic_metadata_impl.cc | 2 +- .../dynamic_metadata_impl_test.cc | 26 +++++++++---------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/source/common/request_info/dynamic_metadata_impl.cc b/source/common/request_info/dynamic_metadata_impl.cc index b9b6431d31fa0..5a54288cbdb1e 100644 --- a/source/common/request_info/dynamic_metadata_impl.cc +++ b/source/common/request_info/dynamic_metadata_impl.cc @@ -14,7 +14,7 @@ void DynamicMetadataImpl::setData(absl::string_view data_name, } bool DynamicMetadataImpl::hasDataWithName(absl::string_view data_name) const { - return (data_storage_.count(data_name) > 0); + return data_storage_.count(data_name) > 0; } const DynamicMetadata::DynamicMetadataObject* DynamicMetadataImpl::getDataGeneric(absl::string_view data_name) const { diff --git a/test/common/request_info/dynamic_metadata_impl_test.cc b/test/common/request_info/dynamic_metadata_impl_test.cc index bbe1be5f4e992..5539bf822cdb9 100644 --- a/test/common/request_info/dynamic_metadata_impl_test.cc +++ b/test/common/request_info/dynamic_metadata_impl_test.cc @@ -18,7 +18,7 @@ class TestStoredTypeTracking : public DynamicMetadata::DynamicMetadataObject { } } - int Access() const { + int access() const { if (access_count_) { ++*access_count_; } @@ -35,7 +35,7 @@ class SimpleType : public DynamicMetadata::DynamicMetadataObject { public: SimpleType(int value) : value_(value) {} - int Access() const { return value_; } + int access() const { return value_; } private: int value_; @@ -43,9 +43,9 @@ class SimpleType : public DynamicMetadata::DynamicMetadataObject { class DynamicMetadataImplTest : public testing::Test { public: - DynamicMetadataImplTest() { ResetDynamicMetadata(); } + DynamicMetadataImplTest() { resetDynamicMetadata(); } - void ResetDynamicMetadata() { dynamic_metadata_ = std::make_unique(); } + void resetDynamicMetadata() { dynamic_metadata_ = std::make_unique(); } DynamicMetadata& dynamic_metadata() { return *dynamic_metadata_; } private: @@ -62,11 +62,11 @@ TEST_F(DynamicMetadataImplTest, Simple) { EXPECT_EQ(0u, access_count); EXPECT_EQ(0u, destruction_count); - EXPECT_EQ(5, dynamic_metadata().getData("test_name").Access()); + EXPECT_EQ(5, dynamic_metadata().getData("test_name").access()); EXPECT_EQ(1u, access_count); EXPECT_EQ(0u, destruction_count); - ResetDynamicMetadata(); + resetDynamicMetadata(); EXPECT_EQ(1u, access_count); EXPECT_EQ(1u, destruction_count); } @@ -86,13 +86,13 @@ TEST_F(DynamicMetadataImplTest, SameTypes) { EXPECT_EQ(0u, access_count_2); EXPECT_EQ(0u, destruction_count); - EXPECT_EQ(ValueOne, dynamic_metadata().getData("test_1").Access()); + EXPECT_EQ(ValueOne, dynamic_metadata().getData("test_1").access()); EXPECT_EQ(1u, access_count_1); EXPECT_EQ(0u, access_count_2); - EXPECT_EQ(ValueTwo, dynamic_metadata().getData("test_2").Access()); + EXPECT_EQ(ValueTwo, dynamic_metadata().getData("test_2").access()); EXPECT_EQ(1u, access_count_1); EXPECT_EQ(1u, access_count_2); - ResetDynamicMetadata(); + resetDynamicMetadata(); EXPECT_EQ(2u, destruction_count); } @@ -100,14 +100,14 @@ TEST_F(DynamicMetadataImplTest, SimpleType) { dynamic_metadata().setData("test_1", std::make_unique(1)); dynamic_metadata().setData("test_2", std::make_unique(2)); - EXPECT_EQ(1, dynamic_metadata().getData("test_1").Access()); - EXPECT_EQ(2, dynamic_metadata().getData("test_2").Access()); + EXPECT_EQ(1, dynamic_metadata().getData("test_1").access()); + EXPECT_EQ(2, dynamic_metadata().getData("test_2").access()); } TEST_F(DynamicMetadataImplTest, NameConflict) { dynamic_metadata().setData("test_1", std::make_unique(1)); EXPECT_THROW(dynamic_metadata().setData("test_1", std::make_unique(2)), EnvoyException); - EXPECT_EQ(1, dynamic_metadata().getData("test_1").Access()); + EXPECT_EQ(1, dynamic_metadata().getData("test_1").access()); } TEST_F(DynamicMetadataImplTest, NameConflictDifferentTypes) { @@ -123,7 +123,7 @@ TEST_F(DynamicMetadataImplTest, UnknownName) { TEST_F(DynamicMetadataImplTest, WrongTypeGet) { dynamic_metadata().setData("test_name", std::make_unique(5, nullptr, nullptr)); - EXPECT_EQ(5, dynamic_metadata().getData("test_name").Access()); + EXPECT_EQ(5, dynamic_metadata().getData("test_name").access()); EXPECT_THROW(dynamic_metadata().getData("test_name"), EnvoyException); } From c6ba24ca29e94653de685e7173ef02d44a2ef670 Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Wed, 1 Aug 2018 15:32:49 -0400 Subject: [PATCH 25/30] Fix format Signed-off-by: Randy Smith --- include/envoy/request_info/dynamic_metadata.h | 10 +++--- .../request_info/dynamic_metadata_impl.cc | 3 +- .../request_info/dynamic_metadata_impl.h | 5 ++- .../dynamic_metadata_impl_test.cc | 31 ++++++++++--------- 4 files changed, 26 insertions(+), 23 deletions(-) diff --git a/include/envoy/request_info/dynamic_metadata.h b/include/envoy/request_info/dynamic_metadata.h index ab37920b59ece..13f8d7c9b80fe 100644 --- a/include/envoy/request_info/dynamic_metadata.h +++ b/include/envoy/request_info/dynamic_metadata.h @@ -4,9 +4,10 @@ #include #include "envoy/common/exception.h" -#include "common/common/fmt.h" #include "envoy/common/pure.h" +#include "common/common/fmt.h" + #include "absl/strings/string_view.h" namespace Envoy { @@ -15,8 +16,8 @@ namespace RequestInfo { class DynamicMetadata { public: class DynamicMetadataObject { - public: - virtual ~DynamicMetadataObject() {}; + public: + virtual ~DynamicMetadataObject(){}; }; virtual ~DynamicMetadata(){}; @@ -40,7 +41,8 @@ class DynamicMetadata { template const T& getData(absl::string_view data_name) const { const T* result = dynamic_cast(getDataGeneric(data_name)); if (!result) { - throw EnvoyException(fmt::format("Data stored under {} cannot be coerced to specified type", data_name)); + throw EnvoyException( + fmt::format("Data stored under {} cannot be coerced to specified type", data_name)); } return *result; } diff --git a/source/common/request_info/dynamic_metadata_impl.cc b/source/common/request_info/dynamic_metadata_impl.cc index 5a54288cbdb1e..670882013977e 100644 --- a/source/common/request_info/dynamic_metadata_impl.cc +++ b/source/common/request_info/dynamic_metadata_impl.cc @@ -17,7 +17,8 @@ bool DynamicMetadataImpl::hasDataWithName(absl::string_view data_name) const { return data_storage_.count(data_name) > 0; } -const DynamicMetadata::DynamicMetadataObject* DynamicMetadataImpl::getDataGeneric(absl::string_view data_name) const { +const DynamicMetadata::DynamicMetadataObject* +DynamicMetadataImpl::getDataGeneric(absl::string_view data_name) const { const auto& it = data_storage_.find(data_name); if (it == data_storage_.end()) { diff --git a/source/common/request_info/dynamic_metadata_impl.h b/source/common/request_info/dynamic_metadata_impl.h index 468d2f31ac925..4767997bdb7cf 100644 --- a/source/common/request_info/dynamic_metadata_impl.h +++ b/source/common/request_info/dynamic_metadata_impl.h @@ -12,11 +12,10 @@ namespace RequestInfo { class DynamicMetadataImpl : public DynamicMetadata { public: DynamicMetadataImpl() {} - ~DynamicMetadataImpl() override {}; + ~DynamicMetadataImpl() override{}; // DynamicMetadata - void setData(absl::string_view data_name, - std::unique_ptr&& data) override; + void setData(absl::string_view data_name, std::unique_ptr&& data) override; bool hasDataWithName(absl::string_view) const override; const DynamicMetadataObject* getDataGeneric(absl::string_view data_name) const override; diff --git a/test/common/request_info/dynamic_metadata_impl_test.cc b/test/common/request_info/dynamic_metadata_impl_test.cc index 5539bf822cdb9..c331b06408583 100644 --- a/test/common/request_info/dynamic_metadata_impl_test.cc +++ b/test/common/request_info/dynamic_metadata_impl_test.cc @@ -9,7 +9,7 @@ namespace RequestInfo { namespace { class TestStoredTypeTracking : public DynamicMetadata::DynamicMetadataObject { - public: +public: TestStoredTypeTracking(int value, size_t* access_count, size_t* destruction_count) : value_(value), access_count_(access_count), destruction_count_(destruction_count) {} ~TestStoredTypeTracking() { @@ -32,12 +32,12 @@ class TestStoredTypeTracking : public DynamicMetadata::DynamicMetadataObject { }; class SimpleType : public DynamicMetadata::DynamicMetadataObject { - public: +public: SimpleType(int value) : value_(value) {} int access() const { return value_; } - private: +private: int value_; }; @@ -78,10 +78,10 @@ TEST_F(DynamicMetadataImplTest, SameTypes) { const int ValueOne = 5; const int ValueTwo = 6; - dynamic_metadata().setData( - "test_1", std::make_unique(ValueOne, &access_count_1, &destruction_count)); - dynamic_metadata().setData( - "test_2", std::make_unique(ValueTwo, &access_count_2, &destruction_count)); + dynamic_metadata().setData("test_1", std::make_unique( + ValueOne, &access_count_1, &destruction_count)); + dynamic_metadata().setData("test_2", std::make_unique( + ValueTwo, &access_count_2, &destruction_count)); EXPECT_EQ(0u, access_count_1); EXPECT_EQ(0u, access_count_2); EXPECT_EQ(0u, destruction_count); @@ -106,15 +106,16 @@ TEST_F(DynamicMetadataImplTest, SimpleType) { TEST_F(DynamicMetadataImplTest, NameConflict) { dynamic_metadata().setData("test_1", std::make_unique(1)); - EXPECT_THROW(dynamic_metadata().setData("test_1", std::make_unique(2)), EnvoyException); + EXPECT_THROW(dynamic_metadata().setData("test_1", std::make_unique(2)), + EnvoyException); EXPECT_EQ(1, dynamic_metadata().getData("test_1").access()); } TEST_F(DynamicMetadataImplTest, NameConflictDifferentTypes) { dynamic_metadata().setData("test_1", std::make_unique(1)); - EXPECT_THROW( - dynamic_metadata().setData("test_1", std::make_unique(2, nullptr, nullptr)), - EnvoyException); + EXPECT_THROW(dynamic_metadata().setData( + "test_1", std::make_unique(2, nullptr, nullptr)), + EnvoyException); } TEST_F(DynamicMetadataImplTest, UnknownName) { @@ -122,17 +123,17 @@ TEST_F(DynamicMetadataImplTest, UnknownName) { } TEST_F(DynamicMetadataImplTest, WrongTypeGet) { - dynamic_metadata().setData("test_name", std::make_unique(5, nullptr, nullptr)); + dynamic_metadata().setData("test_name", + std::make_unique(5, nullptr, nullptr)); EXPECT_EQ(5, dynamic_metadata().getData("test_name").access()); EXPECT_THROW(dynamic_metadata().getData("test_name"), EnvoyException); } namespace { -class A : public DynamicMetadata::DynamicMetadataObject { -}; +class A : public DynamicMetadata::DynamicMetadataObject {}; -class B: public A {}; +class B : public A {}; class C : public B {}; From 9a5ba4f4da0a6d14a58f2174c0d2d6bf32a1f05f Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Wed, 1 Aug 2018 16:22:48 -0400 Subject: [PATCH 26/30] Remove unneeded header. Signed-off-by: Randy Smith --- include/envoy/request_info/dynamic_metadata.h | 1 - 1 file changed, 1 deletion(-) diff --git a/include/envoy/request_info/dynamic_metadata.h b/include/envoy/request_info/dynamic_metadata.h index d61b81833fc08..50f78feddf09d 100644 --- a/include/envoy/request_info/dynamic_metadata.h +++ b/include/envoy/request_info/dynamic_metadata.h @@ -1,6 +1,5 @@ #pragma once -#include #include #include "envoy/common/exception.h" From 9cb16ccbeae5a2e6e3b4b37fbe0b172050501944 Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Thu, 2 Aug 2018 11:20:36 -0400 Subject: [PATCH 27/30] Changed DynamicMetadataObject to Object Signed-off-by: Randy Smith --- include/envoy/request_info/dynamic_metadata.h | 8 ++++---- source/common/request_info/dynamic_metadata_impl.cc | 4 ++-- source/common/request_info/dynamic_metadata_impl.h | 6 +++--- test/common/request_info/dynamic_metadata_impl_test.cc | 6 +++--- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/include/envoy/request_info/dynamic_metadata.h b/include/envoy/request_info/dynamic_metadata.h index 50f78feddf09d..8e035a6a5dc00 100644 --- a/include/envoy/request_info/dynamic_metadata.h +++ b/include/envoy/request_info/dynamic_metadata.h @@ -14,9 +14,9 @@ namespace RequestInfo { class DynamicMetadata { public: - class DynamicMetadataObject { + class Object { public: - virtual ~DynamicMetadataObject(){}; + virtual ~Object(){}; }; virtual ~DynamicMetadata(){}; @@ -28,7 +28,7 @@ class DynamicMetadata { * enforce a single authoritative source for each piece of data stored in DynamicMetadata. */ virtual void setData(absl::string_view data_name, - std::unique_ptr&& data) PURE; + std::unique_ptr&& data) PURE; /** * @param data_name the name of the data being set. @@ -64,7 +64,7 @@ class DynamicMetadata { virtual bool hasDataWithName(absl::string_view data_name) const PURE; protected: - virtual const DynamicMetadataObject* getDataGeneric(absl::string_view data_name) const PURE; + virtual const Object* getDataGeneric(absl::string_view data_name) const PURE; }; } // namespace RequestInfo diff --git a/source/common/request_info/dynamic_metadata_impl.cc b/source/common/request_info/dynamic_metadata_impl.cc index 670882013977e..dc50197a9712a 100644 --- a/source/common/request_info/dynamic_metadata_impl.cc +++ b/source/common/request_info/dynamic_metadata_impl.cc @@ -6,7 +6,7 @@ namespace Envoy { namespace RequestInfo { void DynamicMetadataImpl::setData(absl::string_view data_name, - std::unique_ptr&& data) { + std::unique_ptr&& data) { if (data_storage_.find(data_name) != data_storage_.end()) { throw EnvoyException("DynamicMetadata::setData called twice with same name."); } @@ -17,7 +17,7 @@ bool DynamicMetadataImpl::hasDataWithName(absl::string_view data_name) const { return data_storage_.count(data_name) > 0; } -const DynamicMetadata::DynamicMetadataObject* +const DynamicMetadata::Object* DynamicMetadataImpl::getDataGeneric(absl::string_view data_name) const { const auto& it = data_storage_.find(data_name); diff --git a/source/common/request_info/dynamic_metadata_impl.h b/source/common/request_info/dynamic_metadata_impl.h index 4767997bdb7cf..054b22d743fce 100644 --- a/source/common/request_info/dynamic_metadata_impl.h +++ b/source/common/request_info/dynamic_metadata_impl.h @@ -15,12 +15,12 @@ class DynamicMetadataImpl : public DynamicMetadata { ~DynamicMetadataImpl() override{}; // DynamicMetadata - void setData(absl::string_view data_name, std::unique_ptr&& data) override; + void setData(absl::string_view data_name, std::unique_ptr&& data) override; bool hasDataWithName(absl::string_view) const override; - const DynamicMetadataObject* getDataGeneric(absl::string_view data_name) const override; + const Object* getDataGeneric(absl::string_view data_name) const override; private: - std::map, std::less<>> data_storage_; + std::map, std::less<>> data_storage_; }; } // namespace RequestInfo diff --git a/test/common/request_info/dynamic_metadata_impl_test.cc b/test/common/request_info/dynamic_metadata_impl_test.cc index c331b06408583..d10b734dbf21a 100644 --- a/test/common/request_info/dynamic_metadata_impl_test.cc +++ b/test/common/request_info/dynamic_metadata_impl_test.cc @@ -8,7 +8,7 @@ namespace Envoy { namespace RequestInfo { namespace { -class TestStoredTypeTracking : public DynamicMetadata::DynamicMetadataObject { +class TestStoredTypeTracking : public DynamicMetadata::Object { public: TestStoredTypeTracking(int value, size_t* access_count, size_t* destruction_count) : value_(value), access_count_(access_count), destruction_count_(destruction_count) {} @@ -31,7 +31,7 @@ class TestStoredTypeTracking : public DynamicMetadata::DynamicMetadataObject { size_t* destruction_count_; }; -class SimpleType : public DynamicMetadata::DynamicMetadataObject { +class SimpleType : public DynamicMetadata::Object { public: SimpleType(int value) : value_(value) {} @@ -131,7 +131,7 @@ TEST_F(DynamicMetadataImplTest, WrongTypeGet) { namespace { -class A : public DynamicMetadata::DynamicMetadataObject {}; +class A : public DynamicMetadata::Object {}; class B : public A {}; From 65d8e16299f1481b95c7958661e705edd655b28a Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Thu, 2 Aug 2018 11:22:46 -0400 Subject: [PATCH 28/30] Incorporated comments Signed-off-by: Randy Smith --- source/common/request_info/dynamic_metadata_impl.h | 3 --- test/common/request_info/dynamic_metadata_impl_test.cc | 4 ++-- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/source/common/request_info/dynamic_metadata_impl.h b/source/common/request_info/dynamic_metadata_impl.h index 054b22d743fce..4df9cee4ec05e 100644 --- a/source/common/request_info/dynamic_metadata_impl.h +++ b/source/common/request_info/dynamic_metadata_impl.h @@ -11,9 +11,6 @@ namespace RequestInfo { class DynamicMetadataImpl : public DynamicMetadata { public: - DynamicMetadataImpl() {} - ~DynamicMetadataImpl() override{}; - // DynamicMetadata void setData(absl::string_view data_name, std::unique_ptr&& data) override; bool hasDataWithName(absl::string_view) const override; diff --git a/test/common/request_info/dynamic_metadata_impl_test.cc b/test/common/request_info/dynamic_metadata_impl_test.cc index d10b734dbf21a..1ad500a8cad9f 100644 --- a/test/common/request_info/dynamic_metadata_impl_test.cc +++ b/test/common/request_info/dynamic_metadata_impl_test.cc @@ -75,8 +75,8 @@ TEST_F(DynamicMetadataImplTest, SameTypes) { size_t access_count_1 = 0u; size_t access_count_2 = 0u; size_t destruction_count = 0u; - const int ValueOne = 5; - const int ValueTwo = 6; + static const int ValueOne = 5; + static const int ValueTwo = 6; dynamic_metadata().setData("test_1", std::make_unique( ValueOne, &access_count_1, &destruction_count)); From e62097f53ecf2fd042428c0be5fb98b7fa2fffb6 Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Thu, 2 Aug 2018 11:24:21 -0400 Subject: [PATCH 29/30] fix format Signed-off-by: Randy Smith --- include/envoy/request_info/dynamic_metadata.h | 3 +-- source/common/request_info/dynamic_metadata_impl.cc | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/include/envoy/request_info/dynamic_metadata.h b/include/envoy/request_info/dynamic_metadata.h index 8e035a6a5dc00..145ee08a5debe 100644 --- a/include/envoy/request_info/dynamic_metadata.h +++ b/include/envoy/request_info/dynamic_metadata.h @@ -27,8 +27,7 @@ class DynamicMetadata { * Note that it is an error to call setData() twice with the same data_name; this is to * enforce a single authoritative source for each piece of data stored in DynamicMetadata. */ - virtual void setData(absl::string_view data_name, - std::unique_ptr&& data) PURE; + virtual void setData(absl::string_view data_name, std::unique_ptr&& data) PURE; /** * @param data_name the name of the data being set. diff --git a/source/common/request_info/dynamic_metadata_impl.cc b/source/common/request_info/dynamic_metadata_impl.cc index dc50197a9712a..0d64713ff7ff4 100644 --- a/source/common/request_info/dynamic_metadata_impl.cc +++ b/source/common/request_info/dynamic_metadata_impl.cc @@ -5,8 +5,7 @@ namespace Envoy { namespace RequestInfo { -void DynamicMetadataImpl::setData(absl::string_view data_name, - std::unique_ptr&& data) { +void DynamicMetadataImpl::setData(absl::string_view data_name, std::unique_ptr&& data) { if (data_storage_.find(data_name) != data_storage_.end()) { throw EnvoyException("DynamicMetadata::setData called twice with same name."); } From d22118132267f26f1695e8887fb6f8949efb3322 Mon Sep 17 00:00:00 2001 From: Randy Smith Date: Fri, 3 Aug 2018 14:10:20 -0400 Subject: [PATCH 30/30] Incorporated comments. Signed-off-by: Randy Smith --- .../request_info/dynamic_metadata_impl.cc | 2 ++ .../request_info/dynamic_metadata_impl.h | 3 +++ test/common/request_info/BUILD | 1 + .../dynamic_metadata_impl_test.cc | 20 ++++++++++++------- 4 files changed, 19 insertions(+), 7 deletions(-) diff --git a/source/common/request_info/dynamic_metadata_impl.cc b/source/common/request_info/dynamic_metadata_impl.cc index 0d64713ff7ff4..bf99a6cfb795c 100644 --- a/source/common/request_info/dynamic_metadata_impl.cc +++ b/source/common/request_info/dynamic_metadata_impl.cc @@ -9,6 +9,8 @@ void DynamicMetadataImpl::setData(absl::string_view data_name, std::unique_ptr called twice with same name."); } + // absl::string_view will not convert to std::string without an explicit case; see + // https://github.com/abseil/abseil-cpp/blob/master/absl/strings/string_view.h#L328 data_storage_[static_cast(data_name)] = std::move(data); } diff --git a/source/common/request_info/dynamic_metadata_impl.h b/source/common/request_info/dynamic_metadata_impl.h index 4df9cee4ec05e..51a18472aa79c 100644 --- a/source/common/request_info/dynamic_metadata_impl.h +++ b/source/common/request_info/dynamic_metadata_impl.h @@ -17,6 +17,9 @@ class DynamicMetadataImpl : public DynamicMetadata { const Object* getDataGeneric(absl::string_view data_name) const override; private: + // The explicit non-type-specific comparator is necessary to allow use of find() method + // with absl::string_view. See + // https://stackoverflow.com/questions/20317413/what-are-transparent-comparators. std::map, std::less<>> data_storage_; }; diff --git a/test/common/request_info/BUILD b/test/common/request_info/BUILD index 5a78017e37665..b7cdfa87f6482 100644 --- a/test/common/request_info/BUILD +++ b/test/common/request_info/BUILD @@ -13,6 +13,7 @@ envoy_cc_test( srcs = ["dynamic_metadata_impl_test.cc"], deps = [ "//source/common/request_info:dynamic_metadata_lib", + "//test/test_common:utility_lib", ], ) diff --git a/test/common/request_info/dynamic_metadata_impl_test.cc b/test/common/request_info/dynamic_metadata_impl_test.cc index 1ad500a8cad9f..a2245fdf4b8ef 100644 --- a/test/common/request_info/dynamic_metadata_impl_test.cc +++ b/test/common/request_info/dynamic_metadata_impl_test.cc @@ -2,6 +2,8 @@ #include "common/request_info/dynamic_metadata_impl.h" +#include "test/test_common/utility.h" + #include "gtest/gtest.h" namespace Envoy { @@ -106,27 +108,31 @@ TEST_F(DynamicMetadataImplTest, SimpleType) { TEST_F(DynamicMetadataImplTest, NameConflict) { dynamic_metadata().setData("test_1", std::make_unique(1)); - EXPECT_THROW(dynamic_metadata().setData("test_1", std::make_unique(2)), - EnvoyException); + EXPECT_THROW_WITH_MESSAGE(dynamic_metadata().setData("test_1", std::make_unique(2)), + EnvoyException, + "DynamicMetadata::setData called twice with same name."); EXPECT_EQ(1, dynamic_metadata().getData("test_1").access()); } TEST_F(DynamicMetadataImplTest, NameConflictDifferentTypes) { dynamic_metadata().setData("test_1", std::make_unique(1)); - EXPECT_THROW(dynamic_metadata().setData( - "test_1", std::make_unique(2, nullptr, nullptr)), - EnvoyException); + EXPECT_THROW_WITH_MESSAGE( + dynamic_metadata().setData("test_1", + std::make_unique(2, nullptr, nullptr)), + EnvoyException, "DynamicMetadata::setData called twice with same name."); } TEST_F(DynamicMetadataImplTest, UnknownName) { - EXPECT_THROW(dynamic_metadata().getData("test_1"), EnvoyException); + EXPECT_THROW_WITH_MESSAGE(dynamic_metadata().getData("test_1"), EnvoyException, + "DynamicMetadata::getData called for unknown data name."); } TEST_F(DynamicMetadataImplTest, WrongTypeGet) { dynamic_metadata().setData("test_name", std::make_unique(5, nullptr, nullptr)); EXPECT_EQ(5, dynamic_metadata().getData("test_name").access()); - EXPECT_THROW(dynamic_metadata().getData("test_name"), EnvoyException); + EXPECT_THROW_WITH_MESSAGE(dynamic_metadata().getData("test_name"), EnvoyException, + "Data stored under test_name cannot be coerced to specified type"); } namespace {