ci(bigtable): handle string vs Cord proto fields (#15562)

Author: scotthart

google/cloud/bigtable/value.cc

@@ -15,6 +15,7 @@
 #include "google/cloud/bigtable/value.h"
 #include "google/cloud/bigtable/timestamp.h"
 #include "google/cloud/internal/throw_delegate.h"
+#include "absl/strings/cord.h"
 #include <google/bigtable/v2/types.pb.h>
 #include <google/protobuf/descriptor.h>
 #include <google/protobuf/message.h>
@@ -25,6 +26,30 @@ namespace cloud {
 namespace bigtable {
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
 namespace {
+
+// Some Bigtable proto fields use Cord internally and string externally.
+template <typename T, typename std::enable_if<
+                          std::is_same<T, std::string>::value>::type* = nullptr>
+std::string AsString(T const& s) {
+  return s;
+}
+template <typename T, typename std::enable_if<
+                          std::is_same<T, std::string>::value>::type* = nullptr>
+std::string AsString(T&& s) {
+  return std::move(s);  // NOLINT(bugprone-move-forwarding-reference)
+}
+template <typename T, typename std::enable_if<
+                          std::is_same<T, absl::Cord>::value>::type* = nullptr>
+std::string AsString(T const& s) {
+  return std::string(s);
+}
+template <typename T, typename std::enable_if<
+                          std::is_same<T, absl::Cord>::value>::type* = nullptr>
+std::string AsString(T&& s) {
+  return std::string(
+      std::move(s));  // NOLINT(bugprone-move-forwarding-reference)
+}
+
 // Compares two sets of Type and Value protos for equality. This method calls
 // itself recursively to compare subtypes and subvalues.
 bool Equal(google::bigtable::v2::Type const& pt1,  // NOLINT(misc-no-recursion)
@@ -91,7 +116,7 @@ std::ostream& StreamHelper(std::ostream& os,  // NOLINT(misc-no-recursion)
     return os << v.string_value();
   }
   if (v.kind_case() == google::bigtable::v2::Value::kBytesValue) {
-    return os << Bytes(v.bytes_value());
+    return os << Bytes(AsString(v.bytes_value()));
   }
   if (v.kind_case() == google::bigtable::v2::Value::kTimestampValue) {
     auto ts = MakeTimestamp(v.timestamp_value());
@@ -326,23 +351,23 @@ StatusOr<std::string> Value::GetValue(std::string const&,
   if (pv.kind_case() != google::bigtable::v2::Value::kStringValue) {
     return internal::UnknownError("missing STRING", GCP_ERROR_INFO());
   }
-  return pv.string_value();
+  return AsString(pv.string_value());
 }
 StatusOr<std::string> Value::GetValue(std::string const&,
                                       google::bigtable::v2::Value&& pv,
                                       google::bigtable::v2::Type const&) {
   if (pv.kind_case() != google::bigtable::v2::Value::kStringValue) {
     return internal::UnknownError("missing STRING", GCP_ERROR_INFO());
   }
-  return std::move(*pv.mutable_string_value());
+  return AsString(std::move(*pv.mutable_string_value()));
 }
 StatusOr<Bytes> Value::GetValue(Bytes const&,
                                 google::bigtable::v2::Value const& pv,
                                 google::bigtable::v2::Type const&) {
   if (pv.kind_case() != google::bigtable::v2::Value::kBytesValue) {
     return internal::UnknownError("missing BYTES", GCP_ERROR_INFO());
   }
-  return Bytes(pv.bytes_value());
+  return Bytes(AsString(pv.bytes_value()));
 }
 StatusOr<Timestamp> Value::GetValue(Timestamp const&,
                                     google::bigtable::v2::Value const& pv,

google/cloud/bigtable/value_test.cc

@@ -16,6 +16,7 @@
 #include "google/cloud/internal/base64_transforms.h"
 #include "google/cloud/testing_util/is_proto_equal.h"
 #include "google/cloud/testing_util/status_matchers.h"
+#include "absl/strings/cord.h"
 #include <google/type/date.pb.h>
 #include <gmock/gmock.h>
 #include <cmath>
@@ -226,6 +227,30 @@ TEST(Value, Equality) {
   }
 }
 
+// The next tests assume std::string is the underlying type of protobuf
+// accessors for string values. In situations where the underlying type is
+// absl::Cord, the assumptions are no longer valid and checking the moved
+// from state of the std::string is even less of a good idea than normal.
+template <typename T,
+          typename U = decltype(std::declval<google::bigtable::v2::Value>()
+                                    .string_value()),
+          typename std::enable_if<
+              std::is_same<std::remove_cv_t<std::remove_reference_t<U>>,
+                           std::string>::value>::type* = nullptr>
+StatusOr<T> MovedFromString(Value const& v) {
+  return v.get<T>();
+}
+
+template <typename T,
+          typename U = decltype(std::declval<google::bigtable::v2::Value>()
+                                    .string_value()),
+          typename std::enable_if<
+              std::is_same<std::remove_cv_t<std::remove_reference_t<U>>,
+                           absl::Cord>::value>::type* = nullptr>
+StatusOr<T> MovedFromString(Value const&) {
+  return T{""};
+}
+
 // NOTE: This test relies on unspecified behavior about the moved-from state
 // of std::string. Specifically, this test relies on the fact that "large"
 // strings, when moved-from, end up empty. And we use this fact to verify that
@@ -246,7 +271,7 @@ TEST(Value, RvalueGetString) {
   EXPECT_EQ(data, *s);
 
   // NOLINTNEXTLINE(bugprone-use-after-move)
-  s = v.get<Type>();
+  s = MovedFromString<Type>(v);
   ASSERT_STATUS_OK(s);
   EXPECT_EQ("", *s);
 }
@@ -271,7 +296,7 @@ TEST(Value, RvalueGetOptionalString) {
   EXPECT_EQ(*data, **s);
 
   // NOLINTNEXTLINE(bugprone-use-after-move)
-  s = v.get<Type>();
+  s = MovedFromString<Type>(v);
   ASSERT_STATUS_OK(s);
   EXPECT_EQ("", **s);
 }