Shared FilterState: List support (#4588)

This is an attempt to solve #4528

I tried adding a setList and getList or other variants to the filter state interface as @curiouserrandy suggested in the issue above. Two problems came up:

1. hasData method becomes complicated. It effectively becomes "check if all elements of the list have same type"
2. getListData also becomes kludgey. Obtain the vector, dynamic cast each element to check if typematches and then return a vector of references. Not sure if its worth the perf overhead to do this over and over again.

Instead, I opted here to return a non-const reference to the underlying object (which is not const either). Short of me doing my own const cast..

Signed-off-by: Shriram Rajagopalan <[email protected]>

include/envoy/stream_info/filter_state.h

@@ -1,5 +1,6 @@
 #pragma once
 
+#include <functional>
 #include <memory>
 
 #include "envoy/common/exception.h"
@@ -30,8 +31,8 @@ class FilterState {
   virtual void setData(absl::string_view data_name, std::unique_ptr<Object>&& data) PURE;
 
   /**
-   * @param data_name the name of the data being set.
-   * @return a reference to the stored data.
+   * @param data_name the name of the data being looked up.
+   * @return a const 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.
@@ -55,15 +56,88 @@ class FilterState {
             (dynamic_cast<const T*>(getDataGeneric(data_name)) != nullptr));
   }
 
+  /**
+   * The addToList, hasListWithName, forEachListItem are variants of the above
+   * functions, that operate on list data. Multiple elements could be added
+   * to the list under the same data_name.
+   * @param data_name the name of the list data being set.
+   * @param data an owning pointer to the data to be appended to the list.
+   * Note that data_names for list elements do not share the same namespace as
+   * the data_names for singleton data objects added through setData. All items
+   * added to the list must be of the same type.
+   */
+  template <typename T>
+  void addToList(absl::string_view data_name, std::unique_ptr<Object>&& data) {
+    const auto* list = getList(data_name);
+    if (list != nullptr) {
+      // Check type of first element in the list
+      const T* cast = dynamic_cast<const T*>(list->at(0).get());
+      if (!cast) {
+        throw EnvoyException(
+            fmt::format("List {} does not conform to the specified type", data_name));
+      }
+    }
+
+    addToListGeneric(data_name, std::move(data));
+  }
+
+  /**
+   * @param data_name the name of the list being probed.
+   * @return Whether a list of the type and name specified exists in the
+   * data store.
+   */
+  template <typename T> bool hasList(absl::string_view data_name) const {
+    const auto* list = getList(data_name);
+    return ((list != nullptr) && (dynamic_cast<const T*>(list->at(0).get()) != nullptr));
+  }
+
+  /**
+   * @param data_name the name of the list data being looked up.
+   * @param operation a lambda function that operates on each element in the list,
+   * if it exists. The iteration will stop if the lambda function returns false or
+   * reaches the end of the list. The iteration order will be the same as the order
+   * in which the elements were added to the list. Note that if the elements in the
+   * list cannot be dynamically type-cast into the requested type, an exception
+   * will be thrown.
+   */
+  template <typename T>
+  void forEachListItem(absl::string_view data_name, std::function<bool(const T&)> op) const {
+    const auto* list = getList(data_name);
+    if (!list) {
+      return;
+    }
+
+    for (const auto& it : *list) {
+      const T* data = dynamic_cast<const T*>(it.get());
+      if (!data) {
+        throw EnvoyException(
+            fmt::format("Element in list {} cannot be coerced to specified type", data_name));
+      }
+      if (!op(*data)) {
+        break;
+      }
+    }
+  }
+
   /**
    * @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.
    */
   virtual bool hasDataWithName(absl::string_view data_name) const PURE;
 
+  /**
+   * @param data_name the name of the list data being probed.
+   * @return Whether a list of any type and the name specified exists in the
+   * data store.
+   */
+  virtual bool hasListWithName(absl::string_view data_name) const PURE;
+
 protected:
   virtual const Object* getDataGeneric(absl::string_view data_name) const PURE;
+  virtual const std::vector<std::unique_ptr<Object>>*
+  getList(absl::string_view data_name) const PURE;
+  virtual void addToListGeneric(absl::string_view data_name, std::unique_ptr<Object>&& data) PURE;
 };
 
 } // namespace StreamInfo

include/envoy/stream_info/stream_info.h

@@ -306,8 +306,9 @@ class StreamInfo {
 
   /**
    * Object on which filters can share data on a per-request basis.
-   * Only one filter can produce a named data object, but it may be
-   * consumed by many other objects.
+   * For singleton data objects, only one filter can produce a named data object.
+   * List data objects can be updated by multiple filters (append only). Both object
+   * types can be consumed by multiple filters.
    * @return the per-request state associated with this request.
    */
   virtual FilterState& perRequestState() PURE;

source/common/stream_info/filter_state_impl.cc

@@ -6,13 +6,14 @@ namespace Envoy {
 namespace StreamInfo {
 
 void FilterStateImpl::setData(absl::string_view data_name, std::unique_ptr<Object>&& data) {
-  // TODO(Google): Remove string conversion when fixed internally.
+  // TODO(Google): Remove string conversion when fixed internally. Fixing
+  // this TODO will also require an explicit cast from absl::string_view to
+  // std::string in the data_storage_ index below; see
+  // https://github.com/abseil/abseil-cpp/blob/master/absl/strings/string_view.h#L328
   const std::string name(data_name);
   if (data_storage_.find(name) != data_storage_.end()) {
     throw EnvoyException("FilterState::setData<T> 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_[name] = std::move(data);
 }
 
@@ -31,5 +32,29 @@ const FilterState::Object* FilterStateImpl::getDataGeneric(absl::string_view dat
   return it->second.get();
 }
 
+void FilterStateImpl::addToListGeneric(absl::string_view data_name,
+                                       std::unique_ptr<Object>&& data) {
+  const std::string name(data_name);
+  if (list_storage_.find(name) == list_storage_.end()) {
+    list_storage_[name] = std::vector<std::unique_ptr<FilterState::Object>>();
+  }
+
+  list_storage_[name].push_back(std::move(data));
+}
+
+const std::vector<std::unique_ptr<FilterState::Object>>*
+FilterStateImpl::getList(absl::string_view data_name) const {
+  const auto& it = list_storage_.find(std::string(data_name));
+
+  if (it == list_storage_.end()) {
+    return nullptr;
+  }
+  return &it->second;
+}
+
+bool FilterStateImpl::hasListWithName(absl::string_view data_name) const {
+  return list_storage_.count(std::string(data_name)) > 0;
+}
+
 } // namespace StreamInfo
 } // namespace Envoy

source/common/stream_info/filter_state_impl.h

@@ -15,12 +15,16 @@ class FilterStateImpl : public FilterState {
   void setData(absl::string_view data_name, std::unique_ptr<Object>&& data) override;
   bool hasDataWithName(absl::string_view) const override;
   const Object* getDataGeneric(absl::string_view data_name) const override;
+  void addToListGeneric(absl::string_view data_name, std::unique_ptr<Object>&& data) override;
+  const std::vector<std::unique_ptr<Object>>* getList(absl::string_view data_name) const override;
+  bool hasListWithName(absl::string_view) 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::string, std::unique_ptr<Object>, std::less<>> data_storage_;
+  std::map<std::string, std::vector<std::unique_ptr<Object>>, std::less<>> list_storage_;
 };
 
 } // namespace StreamInfo

test/common/stream_info/filter_state_impl_test.cc

@@ -134,6 +134,89 @@ TEST_F(FilterStateImplTest, WrongTypeGet) {
                             "Data stored under test_name cannot be coerced to specified type");
 }
 
+// Add elements to filter state list and simulate a consumer iterating over
+// all elements.
+TEST_F(FilterStateImplTest, IterateThroughListTillEnd) {
+  size_t access_count = 0;
+  size_t destruction_count = 0;
+  filter_state().addToList<TestStoredTypeTracking>(
+      "test_name", std::make_unique<TestStoredTypeTracking>(5, &access_count, &destruction_count));
+  filter_state().addToList<TestStoredTypeTracking>(
+      "test_name", std::make_unique<TestStoredTypeTracking>(5, &access_count, &destruction_count));
+  EXPECT_EQ(0, access_count);
+  EXPECT_EQ(0, destruction_count);
+
+  filter_state().forEachListItem<TestStoredTypeTracking>("test_name",
+                                                         [&](const TestStoredTypeTracking& t) {
+                                                           EXPECT_EQ(5, t.access());
+                                                           return true;
+                                                         });
+
+  EXPECT_EQ(2u, access_count);
+  EXPECT_EQ(0, destruction_count);
+
+  resetFilterState();
+  EXPECT_EQ(2u, access_count);
+  EXPECT_EQ(2u, destruction_count);
+}
+
+// Add elements to filter state list and simulate a consumer iterating over
+// elements and breaking out of the loop by returning false.
+TEST_F(FilterStateImplTest, IterateThroughListAndBreak) {
+  size_t access_count = 0;
+  size_t destruction_count = 0;
+  filter_state().addToList<TestStoredTypeTracking>(
+      "test_name", std::make_unique<TestStoredTypeTracking>(5, &access_count, &destruction_count));
+  filter_state().addToList<TestStoredTypeTracking>(
+      "test_name", std::make_unique<TestStoredTypeTracking>(5, &access_count, &destruction_count));
+  EXPECT_EQ(0, access_count);
+  EXPECT_EQ(0, destruction_count);
+
+  filter_state().forEachListItem<TestStoredTypeTracking>("test_name",
+                                                         [&](const TestStoredTypeTracking& t) {
+                                                           EXPECT_EQ(5, t.access());
+                                                           return false;
+                                                         });
+
+  EXPECT_EQ(1u, access_count);
+  EXPECT_EQ(0, destruction_count);
+
+  resetFilterState();
+  EXPECT_EQ(1u, access_count);
+  EXPECT_EQ(2u, destruction_count);
+}
+
+// Check that list and (unary) data elements have no namespace conflicts by
+// adding a list element and a data element with same key.
+TEST_F(FilterStateImplTest, NoNameConflictBetweenDataAndList) {
+  filter_state().setData("test_1", std::make_unique<SimpleType>(1));
+  filter_state().addToList<SimpleType>("test_1", std::make_unique<SimpleType>(2));
+  EXPECT_EQ(1, filter_state().getData<SimpleType>("test_1").access());
+  filter_state().forEachListItem<SimpleType>("test_1", [&](const SimpleType& t) {
+    EXPECT_EQ(2, t.access());
+    return true;
+  });
+}
+
+// Check that adding different types to the same list causes exception.
+TEST_F(FilterStateImplTest, ErrorAddingDifferentTypesToSameList) {
+  filter_state().addToList<SimpleType>("test_1", std::make_unique<SimpleType>(1));
+  EXPECT_THROW_WITH_MESSAGE(
+      filter_state().addToList<TestStoredTypeTracking>(
+          "test_1", std::make_unique<TestStoredTypeTracking>(2, nullptr, nullptr)),
+      EnvoyException, "List test_1 does not conform to the specified type");
+}
+
+// Check that adding ForEachListItem throws error when types don't match.
+TEST_F(FilterStateImplTest, WrongTypeInForEachListItem) {
+  filter_state().addToList<TestStoredTypeTracking>(
+      "test_name", std::make_unique<TestStoredTypeTracking>(5, nullptr, nullptr));
+  EXPECT_THROW_WITH_MESSAGE(filter_state().forEachListItem<SimpleType>(
+                                "test_name", [&](const SimpleType&) { return true; }),
+                            EnvoyException,
+                            "Element in list test_name cannot be coerced to specified type");
+}
+
 namespace {
 
 class A : public FilterState::Object {};
@@ -150,10 +233,20 @@ TEST_F(FilterStateImplTest, FungibleInheritance) {
   EXPECT_TRUE(filter_state().hasData<A>("testB"));
   EXPECT_FALSE(filter_state().hasData<C>("testB"));
 
+  filter_state().addToList<B>("testB", std::make_unique<B>());
+  EXPECT_TRUE(filter_state().hasList<B>("testB"));
+  EXPECT_TRUE(filter_state().hasList<A>("testB"));
+  EXPECT_FALSE(filter_state().hasList<C>("testB"));
+
   filter_state().setData("testC", std::make_unique<C>());
   EXPECT_TRUE(filter_state().hasData<B>("testC"));
   EXPECT_TRUE(filter_state().hasData<A>("testC"));
   EXPECT_TRUE(filter_state().hasData<C>("testC"));
+
+  filter_state().addToList<C>("testC", std::make_unique<C>());
+  EXPECT_TRUE(filter_state().hasList<B>("testC"));
+  EXPECT_TRUE(filter_state().hasList<A>("testC"));
+  EXPECT_TRUE(filter_state().hasList<C>("testC"));
 }
 
 TEST_F(FilterStateImplTest, HasData) {
@@ -166,5 +259,13 @@ TEST_F(FilterStateImplTest, HasData) {
   EXPECT_FALSE(filter_state().hasDataWithName("test_2"));
 }
 
+TEST_F(FilterStateImplTest, HasList) {
+  filter_state().addToList<SimpleType>("test_1", std::make_unique<SimpleType>(1));
+  EXPECT_TRUE(filter_state().hasList<SimpleType>("test_1"));
+  EXPECT_FALSE(filter_state().hasList<SimpleType>("test_2"));
+  EXPECT_FALSE(filter_state().hasList<TestStoredTypeTracking>("test_1"));
+  EXPECT_FALSE(filter_state().hasList<TestStoredTypeTracking>("test_2"));
+}
+
 } // namespace StreamInfo
 } // namespace Envoy