caching: Improved the tests and coverage of the CacheFilter tree (#12544

)

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

source/extensions/filters/http/cache/cache_filter.cc

@@ -262,6 +262,7 @@ void CacheFilter::processSuccessfulValidation(Http::ResponseHeaderMap& response_
   }
 }
 
+// TODO(yosrym93): Write a test that exercises this when SimpleHttpCache implements updateHeaders
 bool CacheFilter::shouldUpdateCachedEntry(const Http::ResponseHeaderMap& response_headers) const {
   ASSERT(isResponseNotModified(response_headers),
          "shouldUpdateCachedEntry must only be called with 304 responses");

source/extensions/filters/http/cache/cache_headers_utils.cc

@@ -106,31 +106,6 @@ ResponseCacheControl::ResponseCacheControl(absl::string_view cache_control_heade
   }
 }
 
-std::ostream& operator<<(std::ostream& os, const OptionalDuration& duration) {
-  return duration.has_value() ? os << duration.value().count() : os << " ";
-}
-
-std::ostream& operator<<(std::ostream& os, const RequestCacheControl& request_cache_control) {
-  return os << "{"
-            << "must_validate: " << request_cache_control.must_validate_ << ", "
-            << "no_store: " << request_cache_control.no_store_ << ", "
-            << "no_transform: " << request_cache_control.no_transform_ << ", "
-            << "only_if_cached: " << request_cache_control.only_if_cached_ << ", "
-            << "max_age: " << request_cache_control.max_age_ << ", "
-            << "min_fresh: " << request_cache_control.min_fresh_ << ", "
-            << "max_stale: " << request_cache_control.max_stale_ << "}";
-}
-
-std::ostream& operator<<(std::ostream& os, const ResponseCacheControl& response_cache_control) {
-  return os << "{"
-            << "must_validate: " << response_cache_control.must_validate_ << ", "
-            << "no_store: " << response_cache_control.no_store_ << ", "
-            << "no_transform: " << response_cache_control.no_transform_ << ", "
-            << "no_stale: " << response_cache_control.no_stale_ << ", "
-            << "public: " << response_cache_control.is_public_ << ", "
-            << "max_age: " << response_cache_control.max_age_ << "}";
-}
-
 bool operator==(const RequestCacheControl& lhs, const RequestCacheControl& rhs) {
   return (lhs.must_validate_ == rhs.must_validate_) && (lhs.no_store_ == rhs.no_store_) &&
          (lhs.no_transform_ == rhs.no_transform_) && (lhs.only_if_cached_ == rhs.only_if_cached_) &&

source/extensions/filters/http/cache/cache_headers_utils.h

@@ -82,9 +82,6 @@ struct ResponseCacheControl {
   OptionalDuration max_age_;
 };
 
-std::ostream& operator<<(std::ostream& os, const OptionalDuration& duration);
-std::ostream& operator<<(std::ostream& os, const RequestCacheControl& request_cache_control);
-std::ostream& operator<<(std::ostream& os, const ResponseCacheControl& response_cache_control);
 bool operator==(const RequestCacheControl& lhs, const RequestCacheControl& rhs);
 bool operator==(const ResponseCacheControl& lhs, const ResponseCacheControl& rhs);
 

source/extensions/filters/http/cache/cacheability_utils.cc

@@ -61,9 +61,11 @@ bool CacheabilityUtils::isCacheableResponse(const Http::ResponseHeaderMap& heade
   ResponseCacheControl response_cache_control(cache_control);
 
   // Only cache responses with explicit validation data, either:
-  //    max-age or s-maxage cache-control directives with date header.
-  //    expires header.
+  //    "no-cache" cache-control directive
+  //    "max-age" or "s-maxage" cache-control directives with date header
+  //    expires header
   const bool has_validation_data =
+      response_cache_control.must_validate_ ||
       (headers.Date() && response_cache_control.max_age_.has_value()) ||
       headers.get(Http::Headers::get().Expires);
 

source/extensions/filters/http/cache/http_cache.cc

@@ -22,28 +22,6 @@ namespace Extensions {
 namespace HttpFilters {
 namespace Cache {
 
-std::ostream& operator<<(std::ostream& os, CacheEntryStatus status) {
-  switch (status) {
-  case CacheEntryStatus::Ok:
-    return os << "Ok";
-  case CacheEntryStatus::Unusable:
-    return os << "Unusable";
-  case CacheEntryStatus::RequiresValidation:
-    return os << "RequiresValidation";
-  case CacheEntryStatus::FoundNotModified:
-    return os << "FoundNotModified";
-  case CacheEntryStatus::NotSatisfiableRange:
-    return os << "NotSatisfiableRange";
-  case CacheEntryStatus::SatisfiableRange:
-    return os << "SatisfiableRange";
-  }
-  NOT_REACHED_GCOVR_EXCL_LINE;
-}
-
-std::ostream& operator<<(std::ostream& os, const AdjustedByteRange& range) {
-  return os << "[" << range.begin() << "," << range.end() << ")";
-}
-
 LookupRequest::LookupRequest(const Http::RequestHeaderMap& request_headers, SystemTime timestamp)
     : timestamp_(timestamp) {
   // These ASSERTs check prerequisites. A request without these headers can't be looked up in cache;

source/extensions/filters/http/cache/http_cache.h

@@ -41,8 +41,6 @@ enum class CacheEntryStatus {
   SatisfiableRange,
 };
 
-std::ostream& operator<<(std::ostream& os, CacheEntryStatus status);
-
 // Byte range from an HTTP request.
 class RawByteRange {
 public:
@@ -112,8 +110,6 @@ inline bool operator==(const AdjustedByteRange& lhs, const AdjustedByteRange& rh
   return lhs.begin() == rhs.begin() && lhs.end() == rhs.end();
 }
 
-std::ostream& operator<<(std::ostream& os, const AdjustedByteRange& range);
-
 // Adjusts request_range_spec to fit a cached response of size content_length, putting the results
 // in response_ranges. Returns true if response_ranges is satisfiable (empty is considered
 // satisfiable, as it denotes the entire body).

test/extensions/filters/http/cache/BUILD

@@ -1,4 +1,4 @@
-load("//bazel:envoy_build_system.bzl", "envoy_package")
+load("//bazel:envoy_build_system.bzl", "envoy_cc_test_library", "envoy_package")
 load(
     "//test/extensions:extensions_build_system.bzl",
     "envoy_extension_cc_test",
@@ -8,11 +8,22 @@ licenses(["notice"])  # Apache 2
 
 envoy_package()
 
+envoy_cc_test_library(
+    name = "common",
+    hdrs = ["common.h"],
+    deps = [
+        "//source/extensions/filters/http/cache:cache_headers_utils_lib",
+        "//source/extensions/filters/http/cache:http_cache_lib",
+        "//source/extensions/filters/http/cache/simple_http_cache:simple_http_cache_lib",
+    ],
+)
+
 envoy_extension_cc_test(
     name = "cache_headers_utils_test",
     srcs = ["cache_headers_utils_test.cc"],
     extension_name = "envoy.filters.http.cache",
     deps = [
+        ":common",
         "//include/envoy/http:header_map_interface",
         "//source/common/http:header_map_lib",
         "//source/extensions/filters/http/cache:cache_headers_utils_lib",
@@ -25,6 +36,7 @@ envoy_extension_cc_test(
     srcs = ["http_cache_test.cc"],
     extension_name = "envoy.filters.http.cache",
     deps = [
+        ":common",
         "//source/extensions/filters/http/cache:http_cache_lib",
         "//source/extensions/filters/http/cache/simple_http_cache:simple_http_cache_lib",
         "//test/mocks/http:http_mocks",
@@ -38,6 +50,7 @@ envoy_extension_cc_test(
     srcs = ["cache_filter_test.cc"],
     extension_name = "envoy.filters.http.cache",
     deps = [
+        ":common",
         "//source/extensions/filters/http/cache:cache_filter_lib",
         "//source/extensions/filters/http/cache/simple_http_cache:simple_http_cache_lib",
         "//test/mocks/server:factory_context_mocks",
@@ -75,7 +88,6 @@ envoy_extension_cc_test(
         "cache_filter_integration_test.cc",
     ],
     extension_name = "envoy.filters.http.cache",
-    tags = ["fails_on_windows"],
     deps = [
         "//source/extensions/filters/http/cache:config",
         "//source/extensions/filters/http/cache:http_cache_lib",