Sink: add in memory sink, slightly refactor validation/testing.

include/nighthawk/sink/sink.h

@@ -30,7 +30,7 @@ class Sink {
    * @return absl::Status Indicates if the operation succeeded or not.
    */
   virtual absl::Status
-  StoreExecutionResultPiece(const nighthawk::client::ExecutionResponse& response) const PURE;
+  StoreExecutionResultPiece(const nighthawk::client::ExecutionResponse& response) PURE;
 
   /**
    * Attempt to load a vector of ExecutionResponse instances associated to an execution id.

source/sink/sink_impl.cc

@@ -16,40 +16,48 @@ absl::Status verifyCanBeUsedAsDirectoryName(absl::string_view s) {
   const std::string err_template = "'{}' is not a guid: {}";
 
   if (s.size() != reference_value.size()) {
-    return absl::Status(absl::StatusCode::kInvalidArgument,
-                        fmt::format(err_template, s, "bad string length."));
+    return absl::InvalidArgumentError(fmt::format(err_template, s, "bad string length."));
   }
   for (size_t i = 0; i < s.size(); i++) {
     if (reference_value[i] == '-') {
       if (s[i] != '-') {
-        return absl::Status(
-            absl::StatusCode::kInvalidArgument,
+        return absl::InvalidArgumentError(
             fmt::format(err_template, s, "expectations around '-' positions not met."));
       }
       continue;
     }
     if (!std::isxdigit(s[i])) {
-      return absl::Status(absl::StatusCode::kInvalidArgument,
-                          fmt::format(err_template, s, "unexpected character encountered."));
+      return absl::InvalidArgumentError(
+          fmt::format(err_template, s, "unexpected character encountered."));
     }
   }
   return absl::OkStatus();
 }
 
+absl::Status validateKey(absl::string_view key, bool validate_as_directory_name) {
+  absl::Status status =
+      key.empty() ? absl::Status(absl::StatusCode::kInvalidArgument, "empty key is not allowed.")
+                  : absl::OkStatus();
+  if (status.ok() && validate_as_directory_name) {
+    status = verifyCanBeUsedAsDirectoryName(key);
+  }
+  return status;
+}
+
 } // namespace
 
-absl::Status FileSinkImpl::StoreExecutionResultPiece(
-    const nighthawk::client::ExecutionResponse& response) const {
+absl::Status
+FileSinkImpl::StoreExecutionResultPiece(const nighthawk::client::ExecutionResponse& response) {
   const std::string& execution_id = response.execution_id();
-  absl::Status status = verifyCanBeUsedAsDirectoryName(execution_id);
+  absl::Status status = validateKey(execution_id, true);
   if (!status.ok()) {
     return status;
   }
   std::error_code error_code;
   std::filesystem::create_directories("/tmp/nh/" + std::string(execution_id) + "/", error_code);
   // Note error_code will not be set if an existing directory existed.
   if (error_code.value()) {
-    return absl::Status(absl::StatusCode::kInternal, error_code.message());
+    return absl::InternalError(error_code.message());
   }
   // Write to a tmp file, and if that succeeds, we swap it atomically to the target path,
   // to make the completely written file visible to consumers of LoadExecutionResult.
@@ -58,27 +66,26 @@ absl::Status FileSinkImpl::StoreExecutionResultPiece(
   {
     std::ofstream ofs(uid.data(), std::ios_base::out | std::ios_base::binary);
     if (!response.SerializeToOstream(&ofs)) {
-      return absl::Status(absl::StatusCode::kInternal, "Failure writing to temp file");
+      return absl::InternalError("Failure writing to temp file");
     }
   }
   std::filesystem::path filesystem_path(uid.data());
   const std::string new_name =
       "/tmp/nh/" + std::string(execution_id) + "/" + std::string(filesystem_path.filename());
   std::filesystem::rename(uid.data(), new_name, error_code);
   if (error_code.value()) {
-    return absl::Status(absl::StatusCode::kInternal, error_code.message());
+    return absl::InternalError(error_code.message());
   }
   ENVOY_LOG_MISC(trace, "Stored '{}'.", new_name);
-  return absl::Status();
+  return absl::OkStatus();
 }
 
 absl::StatusOr<std::vector<nighthawk::client::ExecutionResponse>>
 FileSinkImpl::LoadExecutionResult(absl::string_view execution_id) const {
-  absl::Status status = verifyCanBeUsedAsDirectoryName(execution_id);
+  absl::Status status = validateKey(execution_id, true);
   if (!status.ok()) {
     return status;
   }
-
   std::filesystem::path filesystem_directory_path("/tmp/nh/" + std::string(execution_id) + "/");
   std::vector<nighthawk::client::ExecutionResponse> responses;
   std::error_code error_code;
@@ -91,17 +98,42 @@ FileSinkImpl::LoadExecutionResult(absl::string_view execution_id) const {
     nighthawk::client::ExecutionResponse response;
     std::ifstream ifs(it.path(), std::ios_base::binary);
     if (!response.ParseFromIstream(&ifs)) {
-      return absl::Status(absl::StatusCode::kInternal,
-                          fmt::format("Failed to parse ExecutionResponse '{}'.", it.path()));
+      return absl::InternalError(fmt::format("Failed to parse ExecutionResponse '{}'.", it.path()));
     } else {
       ENVOY_LOG_MISC(trace, "Loaded '{}'.", it.path());
     }
     responses.push_back(response);
   }
   if (error_code.value()) {
-    return absl::Status(absl::StatusCode::kNotFound, error_code.message());
+    return absl::NotFoundError(error_code.message());
   }
   return responses;
 }
 
+absl::Status
+InMemorySinkImpl::StoreExecutionResultPiece(const nighthawk::client::ExecutionResponse& response) {
+  absl::Status status = validateKey(response.execution_id(), false);
+  if (!status.ok()) {
+    return status;
+  }
+  auto iterator =
+      map_.insert({response.execution_id(), std::vector<nighthawk::client::ExecutionResponse>()})
+          .first;
+  iterator->second.push_back(response);
+  return absl::OkStatus();
+}
+
+absl::StatusOr<std::vector<nighthawk::client::ExecutionResponse>>
+InMemorySinkImpl::LoadExecutionResult(absl::string_view execution_id) const {
+  absl::Status status = validateKey(execution_id, false);
+  if (!status.ok()) {
+    return status;
+  }
+  auto iterator = map_.find(execution_id);
+  if (iterator != map_.end()) {
+    return (*iterator).second;
+  }
+  return absl::NotFoundError(fmt::format("No results found for execution-id: '{}'", execution_id));
+}
+
 } // namespace Nighthawk

source/sink/sink_impl.h

@@ -2,6 +2,8 @@
 
 #include "nighthawk/sink/sink.h"
 
+#include "absl/container/flat_hash_map.h"
+
 namespace Nighthawk {
 
 /**
@@ -11,9 +13,23 @@ namespace Nighthawk {
 class FileSinkImpl : public Sink {
 public:
   absl::Status
-  StoreExecutionResultPiece(const nighthawk::client::ExecutionResponse& response) const override;
+  StoreExecutionResultPiece(const nighthawk::client::ExecutionResponse& response) override;
+  absl::StatusOr<std::vector<nighthawk::client::ExecutionResponse>>
+  LoadExecutionResult(absl::string_view id) const override;
+};
+
+/**
+ * Memory based implementation of Sink.
+ */
+class InMemorySinkImpl : public Sink {
+public:
+  absl::Status
+  StoreExecutionResultPiece(const nighthawk::client::ExecutionResponse& response) override;
   absl::StatusOr<std::vector<nighthawk::client::ExecutionResponse>>
   LoadExecutionResult(absl::string_view id) const override;
+
+private:
+  absl::flat_hash_map<std::string, std::vector<nighthawk::client::ExecutionResponse>> map_;
 };
 
 } // namespace Nighthawk

test/mocks/sink/mock_sink.h

@@ -9,11 +9,10 @@ namespace Nighthawk {
 class MockSink : public Sink {
 public:
   MockSink();
-  MOCK_CONST_METHOD1(StoreExecutionResultPiece,
-                     absl::Status(const nighthawk::client::ExecutionResponse&));
-  MOCK_CONST_METHOD1(
-      LoadExecutionResult,
-      absl::StatusOr<std::vector<nighthawk::client::ExecutionResponse>>(absl::string_view));
+  MOCK_METHOD(absl::Status, StoreExecutionResultPiece,
+              (const nighthawk::client::ExecutionResponse&));
+  MOCK_METHOD(absl::StatusOr<std::vector<nighthawk::client::ExecutionResponse>>,
+              LoadExecutionResult, (absl::string_view), (const));
 };
 
 } // namespace Nighthawk

test/sink/sink_test.cc

@@ -11,7 +11,7 @@ namespace Nighthawk {
 namespace {
 
 // Future sink implementations register here for testing top level generic sink behavior.
-using SinkTypes = testing::Types<FileSinkImpl>;
+using SinkTypes = testing::Types<FileSinkImpl, InMemorySinkImpl>;
 
 template <typename T> class TypedSinkTest : public testing::Test {
 public:
@@ -55,16 +55,15 @@ TYPED_TEST(TypedSinkTest, EmptyKeyStoreFails) {
   const absl::Status status = sink.StoreExecutionResultPiece(result_to_store);
   ASSERT_FALSE(status.ok());
   EXPECT_EQ(status.code(), absl::StatusCode::kInvalidArgument);
-  EXPECT_EQ(status.message(), "'' is not a guid: bad string length.");
+  EXPECT_EQ(status.message(), "empty key is not allowed.");
 }
 
 TYPED_TEST(TypedSinkTest, EmptyKeyLoadFails) {
   TypeParam sink;
   const auto status_or_execution_responses = sink.LoadExecutionResult("");
   ASSERT_EQ(status_or_execution_responses.ok(), false);
   EXPECT_EQ(status_or_execution_responses.status().code(), absl::StatusCode::kInvalidArgument);
-  EXPECT_EQ(status_or_execution_responses.status().message(),
-            "'' is not a guid: bad string length.");
+  EXPECT_EQ(status_or_execution_responses.status().message(), "empty key is not allowed.");
 }
 
 TYPED_TEST(TypedSinkTest, Append) {
@@ -79,13 +78,8 @@ TYPED_TEST(TypedSinkTest, Append) {
   EXPECT_EQ(status_or_execution_responses.value().size(), 2);
 }
 
-// As of today, we constrain execution id to a guid. This way the file sink implementation
-// ensures that it can safely use it to create directories. In the future, other sinks may not
-// have to worry about such things. In that case it makes sense to add a validation call
-// to the sink interface to make this implementation specific, and make the tests below
-// implementation specific too.
-TYPED_TEST(TypedSinkTest, BadGuidShortString) {
-  TypeParam sink;
+TEST(FileSinkTest, BadGuidShortString) {
+  FileSinkImpl sink;
   const auto status_or_execution_responses =
       sink.LoadExecutionResult("14e75b2a-3e31-4a62-9279-add1e54091f");
   ASSERT_EQ(status_or_execution_responses.ok(), false);
@@ -94,8 +88,8 @@ TYPED_TEST(TypedSinkTest, BadGuidShortString) {
             "'14e75b2a-3e31-4a62-9279-add1e54091f' is not a guid: bad string length.");
 }
 
-TYPED_TEST(TypedSinkTest, BadGuidBadDashPlacement) {
-  TypeParam sink;
+TEST(FileSinkTest, BadGuidBadDashPlacement) {
+  FileSinkImpl sink;
   const auto status_or_execution_responses =
       sink.LoadExecutionResult("14e75b2a3-e31-4a62-9279-add1e54091f9");
   ASSERT_EQ(status_or_execution_responses.ok(), false);
@@ -105,8 +99,8 @@ TYPED_TEST(TypedSinkTest, BadGuidBadDashPlacement) {
             "positions not met.");
 }
 
-TYPED_TEST(TypedSinkTest, BadGuidInvalidCharacter) {
-  TypeParam sink;
+TEST(FileSinkTest, BadGuidInvalidCharacter) {
+  FileSinkImpl sink;
   const auto status_or_execution_responses =
       sink.LoadExecutionResult("14e75b2a-3e31-4x62-9279-add1e54091f9");
   ASSERT_EQ(status_or_execution_responses.ok(), false);