From 79f6bb46f0f390c167480ee807b5eacd2729656a Mon Sep 17 00:00:00 2001 From: dbolduc Date: Wed, 24 May 2023 14:02:00 -0400 Subject: [PATCH 1/3] refactor(bigtable): future-proof ReadRows mocking --- google/cloud/bigtable/data_connection.cc | 14 +++++-- google/cloud/bigtable/data_connection.h | 12 ++++++ .../bigtable/mocks/mock_data_connection.h | 18 ++++++++ google/cloud/bigtable/table_test.cc | 42 ++++++++++++++----- 4 files changed, 72 insertions(+), 14 deletions(-) diff --git a/google/cloud/bigtable/data_connection.cc b/google/cloud/bigtable/data_connection.cc index 8a724884d3788..1c3f0bc7b9b7f 100644 --- a/google/cloud/bigtable/data_connection.cc +++ b/google/cloud/bigtable/data_connection.cc @@ -74,9 +74,17 @@ future> DataConnection::AsyncBulkApply( Status(StatusCode::kUnimplemented, "not-implemented"), mut.size())); } -RowReader DataConnection::ReadRows( - // NOLINTNEXTLINE(performance-unnecessary-value-param) - std::string const&, RowSet, std::int64_t, Filter) { +RowReader DataConnection::ReadRows(std::string const& table_name, + RowSet row_set, std::int64_t rows_limit, + Filter filter) { + return ReadRowsFull(ReadRowsParams{ + std::move(table_name), + google::cloud::internal::CurrentOptions().get(), + std::move(row_set), rows_limit, std::move(filter)}); +} + +// NOLINTNEXTLINE(performance-unnecessary-value-param) +RowReader DataConnection::ReadRowsFull(ReadRowsParams) { return MakeRowReader(std::make_shared( Status(StatusCode::kUnimplemented, "not implemented"))); } diff --git a/google/cloud/bigtable/data_connection.h b/google/cloud/bigtable/data_connection.h index 5b427807c860e..17c87dc846a3e 100644 --- a/google/cloud/bigtable/data_connection.h +++ b/google/cloud/bigtable/data_connection.h @@ -42,6 +42,15 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END namespace bigtable { GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN +/// Wrap the arguments to `ReadRows()`. +struct ReadRowsParams { + std::string table_name; + std::string app_profile_id; + RowSet row_set; + std::int64_t rows_limit; + Filter filter = Filter::PassAllFilter(); +}; + /** * A connection to the Cloud Bigtable Data API. * @@ -93,9 +102,12 @@ class DataConnection { virtual future> AsyncBulkApply( std::string const& table_name, BulkMutation mut); + /// Prefer to use `ReadRowsFull()` in mocks. virtual RowReader ReadRows(std::string const& table_name, RowSet row_set, std::int64_t rows_limit, Filter filter); + virtual RowReader ReadRowsFull(ReadRowsParams params); + virtual StatusOr> ReadRow(std::string const& table_name, std::string row_key, Filter filter); diff --git a/google/cloud/bigtable/mocks/mock_data_connection.h b/google/cloud/bigtable/mocks/mock_data_connection.h index b6aaf61880d88..84aadbb245c47 100644 --- a/google/cloud/bigtable/mocks/mock_data_connection.h +++ b/google/cloud/bigtable/mocks/mock_data_connection.h @@ -36,6 +36,20 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN */ class MockDataConnection : public bigtable::DataConnection { public: + MockDataConnection() { + ON_CALL(*this, ReadRows) + .WillByDefault([this](std::string const& table_name, + bigtable::RowSet const& row_set, + std::int64_t rows_limit, + bigtable::Filter const& filter) { + bigtable::ReadRowsParams params{ + table_name, + internal::CurrentOptions().get(), + row_set, rows_limit, filter}; + return this->ReadRowsFull(params); + }); + } + MOCK_METHOD(Options, options, (), (override)); MOCK_METHOD(Status, Apply, @@ -54,11 +68,15 @@ class MockDataConnection : public bigtable::DataConnection { (std::string const& table_name, bigtable::BulkMutation mut), (override)); + /// Prefer to use `ReadRowsFull()`. MOCK_METHOD(bigtable::RowReader, ReadRows, (std::string const& table_name, bigtable::RowSet row_set, std::int64_t rows_limit, bigtable::Filter filter), (override)); + MOCK_METHOD(bigtable::RowReader, ReadRowsFull, (bigtable::ReadRowsParams), + (override)); + MOCK_METHOD((StatusOr>), ReadRow, (std::string const& table_name, std::string row_key, bigtable::Filter filter), diff --git a/google/cloud/bigtable/table_test.cc b/google/cloud/bigtable/table_test.cc index b9ae62e35c320..19854c239c272 100644 --- a/google/cloud/bigtable/table_test.cc +++ b/google/cloud/bigtable/table_test.cc @@ -252,17 +252,15 @@ TEST(TableTest, AsyncBulkApply) { TEST(TableTest, ReadRows) { auto mock = std::make_shared(); - EXPECT_CALL(*mock, ReadRows) - .WillOnce([](std::string const& table_name, - bigtable::RowSet const& row_set, std::int64_t rows_limit, - bigtable::Filter const& filter) { - CheckCurrentOptions(); - EXPECT_EQ(kTableName, table_name); - EXPECT_THAT(row_set, IsTestRowSet()); - EXPECT_EQ(rows_limit, RowReader::NO_ROWS_LIMIT); - EXPECT_THAT(filter, IsTestFilter()); - return bigtable_mocks::MakeRowReader({}, PermanentError()); - }); + EXPECT_CALL(*mock, ReadRowsFull).WillOnce([](ReadRowsParams const& params) { + CheckCurrentOptions(); + EXPECT_EQ(params.table_name, kTableName); + EXPECT_EQ(params.app_profile_id, kAppProfileId); + EXPECT_THAT(params.row_set, IsTestRowSet()); + EXPECT_EQ(params.rows_limit, RowReader::NO_ROWS_LIMIT); + EXPECT_THAT(params.filter, IsTestFilter()); + return bigtable_mocks::MakeRowReader({}, PermanentError()); + }); auto table = TestTable(std::move(mock)); auto reader = table.ReadRows(TestRowSet(), TestFilter(), CallOptions()); @@ -273,6 +271,28 @@ TEST(TableTest, ReadRows) { TEST(TableTest, ReadRowsWithRowLimit) { auto mock = std::make_shared(); + EXPECT_CALL(*mock, ReadRowsFull).WillOnce([](ReadRowsParams const& params) { + CheckCurrentOptions(); + EXPECT_EQ(params.table_name, kTableName); + EXPECT_EQ(params.app_profile_id, kAppProfileId); + EXPECT_THAT(params.row_set, IsTestRowSet()); + EXPECT_EQ(params.rows_limit, 42); + EXPECT_THAT(params.filter, IsTestFilter()); + return bigtable_mocks::MakeRowReader({}, PermanentError()); + }); + + auto table = TestTable(std::move(mock)); + auto reader = table.ReadRows(TestRowSet(), 42, TestFilter(), CallOptions()); + auto it = reader.begin(); + EXPECT_THAT(*it, StatusIs(StatusCode::kPermissionDenied)); + EXPECT_EQ(++it, reader.end()); +} + +TEST(TableTest, ReadRowsMockBackwardsCompatibility) { + auto mock = std::make_shared(); + // Ensure that existing mocks which set expectations on the legacy `ReadRows` + // method continue to work. This is more a test of `MockDataConnection` than + // of `Table`. EXPECT_CALL(*mock, ReadRows) .WillOnce([](std::string const& table_name, bigtable::RowSet const& row_set, std::int64_t rows_limit, From fdf3fa781f14b03d8a90a3d364757403b7e4c12d Mon Sep 17 00:00:00 2001 From: dbolduc Date: Wed, 24 May 2023 14:03:21 -0400 Subject: [PATCH 2/3] derived connections --- .../bigtable/internal/data_connection_impl.cc | 16 +++++----- .../bigtable/internal/data_connection_impl.h | 5 +--- .../internal/data_connection_impl_test.cc | 24 +++++++++++++++ .../internal/data_tracing_connection.cc | 8 ++--- .../internal/data_tracing_connection_test.cc | 29 ++++++++++++++++++- 5 files changed, 63 insertions(+), 19 deletions(-) diff --git a/google/cloud/bigtable/internal/data_connection_impl.cc b/google/cloud/bigtable/internal/data_connection_impl.cc index 4acd998c9c284..09e1578d2182d 100644 --- a/google/cloud/bigtable/internal/data_connection_impl.cc +++ b/google/cloud/bigtable/internal/data_connection_impl.cc @@ -173,13 +173,12 @@ DataConnectionImpl::AsyncBulkApply(std::string const& table_name, app_profile_id(), table_name, std::move(mut)); } -bigtable::RowReader DataConnectionImpl::ReadRows(std::string const& table_name, - bigtable::RowSet row_set, - std::int64_t rows_limit, - bigtable::Filter filter) { +bigtable::RowReader DataConnectionImpl::ReadRowsFull( + bigtable::ReadRowsParams params) { auto impl = std::make_shared( - stub_, app_profile_id(), table_name, std::move(row_set), rows_limit, - std::move(filter), retry_policy(), backoff_policy()); + stub_, std::move(params.app_profile_id), std::move(params.table_name), + std::move(params.row_set), params.rows_limit, std::move(params.filter), + retry_policy(), backoff_policy()); return MakeRowReader(std::move(impl)); } @@ -188,8 +187,9 @@ StatusOr> DataConnectionImpl::ReadRow( bigtable::Filter filter) { bigtable::RowSet row_set(std::move(row_key)); std::int64_t const rows_limit = 1; - auto reader = - ReadRows(table_name, std::move(row_set), rows_limit, std::move(filter)); + auto reader = ReadRowsFull( + bigtable::ReadRowsParams{table_name, app_profile_id(), std::move(row_set), + rows_limit, std::move(filter)}); auto it = reader.begin(); if (it == reader.end()) return std::make_pair(false, bigtable::Row("", {})); diff --git a/google/cloud/bigtable/internal/data_connection_impl.h b/google/cloud/bigtable/internal/data_connection_impl.h index f8025b071d1f9..41e7f4542b236 100644 --- a/google/cloud/bigtable/internal/data_connection_impl.h +++ b/google/cloud/bigtable/internal/data_connection_impl.h @@ -52,10 +52,7 @@ class DataConnectionImpl : public bigtable::DataConnection { future> AsyncBulkApply( std::string const& table_name, bigtable::BulkMutation mut) override; - bigtable::RowReader ReadRows(std::string const& table_name, - bigtable::RowSet row_set, - std::int64_t rows_limit, - bigtable::Filter filter) override; + bigtable::RowReader ReadRowsFull(bigtable::ReadRowsParams params) override; StatusOr> ReadRow( std::string const& table_name, std::string row_key, diff --git a/google/cloud/bigtable/internal/data_connection_impl_test.cc b/google/cloud/bigtable/internal/data_connection_impl_test.cc index 9de15ca582416..1f16e39a3a7af 100644 --- a/google/cloud/bigtable/internal/data_connection_impl_test.cc +++ b/google/cloud/bigtable/internal/data_connection_impl_test.cc @@ -704,6 +704,30 @@ TEST(DataConnectionTest, ReadRows) { EXPECT_EQ(reader.begin(), reader.end()); } +// The DefaultRowReader is tested extensively in `default_row_reader_test.cc`. +// In this test, we just verify that the configuration is passed along. +TEST(DataConnectionTest, ReadRowsFull) { + auto mock = std::make_shared(); + EXPECT_CALL(*mock, ReadRows) + .WillOnce([](auto, google::bigtable::v2::ReadRowsRequest const& request) { + EXPECT_EQ(kAppProfile, request.app_profile_id()); + EXPECT_EQ(kTableName, request.table_name()); + EXPECT_EQ(42, request.rows_limit()); + EXPECT_THAT(request, HasTestRowSet()); + EXPECT_THAT(request.filter(), IsTestFilter()); + + auto stream = std::make_unique(); + EXPECT_CALL(*stream, Read).WillOnce(Return(Status())); + return stream; + }); + + auto conn = TestConnection(std::move(mock)); + internal::OptionsSpan span(CallOptions()); + auto reader = conn->ReadRowsFull(bigtable::ReadRowsParams{ + kTableName, kAppProfile, TestRowSet(), 42, TestFilter()}); + EXPECT_EQ(reader.begin(), reader.end()); +} + TEST(DataConnectionTest, ReadRowEmpty) { auto mock = std::make_shared(); EXPECT_CALL(*mock, ReadRows) diff --git a/google/cloud/bigtable/internal/data_tracing_connection.cc b/google/cloud/bigtable/internal/data_tracing_connection.cc index 458b06adbd707..2b3683f93eb63 100644 --- a/google/cloud/bigtable/internal/data_tracing_connection.cc +++ b/google/cloud/bigtable/internal/data_tracing_connection.cc @@ -90,14 +90,10 @@ class DataTracingConnection : public bigtable::DataConnection { }); } - bigtable::RowReader ReadRows(std::string const& table_name, - bigtable::RowSet row_set, - std::int64_t rows_limit, - bigtable::Filter filter) override { + bigtable::RowReader ReadRowsFull(bigtable::ReadRowsParams params) override { auto span = internal::MakeSpan("bigtable::Table::ReadRows"); auto scope = opentelemetry::trace::Scope(span); - auto reader = child_->ReadRows(table_name, std::move(row_set), rows_limit, - std::move(filter)); + auto reader = child_->ReadRowsFull(std::move(params)); return MakeTracedRowReader(std::move(span), std::move(reader)); } diff --git a/google/cloud/bigtable/internal/data_tracing_connection_test.cc b/google/cloud/bigtable/internal/data_tracing_connection_test.cc index be557af1d6bab..43833860efb21 100644 --- a/google/cloud/bigtable/internal/data_tracing_connection_test.cc +++ b/google/cloud/bigtable/internal/data_tracing_connection_test.cc @@ -227,7 +227,7 @@ TEST(DataTracingConnection, ReadRows) { auto span_catcher = InstallSpanCatcher(); auto mock = std::make_shared(); - EXPECT_CALL(*mock, ReadRows).WillOnce([] { + EXPECT_CALL(*mock, ReadRowsFull).WillOnce([] { EXPECT_TRUE(ThereIsAnActiveSpan()); return bigtable_mocks::MakeRowReader({}, internal::AbortedError("fail")); }); @@ -249,6 +249,33 @@ TEST(DataTracingConnection, ReadRows) { SpanAttribute("gcloud.status_code", kErrorCode))))); } +TEST(DataTracingConnection, ReadRowsFull) { + auto span_catcher = InstallSpanCatcher(); + + auto mock = std::make_shared(); + EXPECT_CALL(*mock, ReadRowsFull).WillOnce([] { + EXPECT_TRUE(ThereIsAnActiveSpan()); + return bigtable_mocks::MakeRowReader({}, internal::AbortedError("fail")); + }); + + auto under_test = MakeDataTracingConnection(mock); + auto reader = under_test->ReadRowsFull( + bigtable::ReadRowsParams{kTableName, "app-profile-id", bigtable::RowSet(), + 0, bigtable::Filter::PassAllFilter()}); + auto it = reader.begin(); + EXPECT_THAT(*it, StatusIs(StatusCode::kAborted)); + EXPECT_EQ(++it, reader.end()); + + EXPECT_THAT( + span_catcher->GetSpans(), + ElementsAre(AllOf( + SpanHasInstrumentationScope(), SpanKindIsClient(), + SpanNamed("bigtable::Table::ReadRows"), + SpanWithStatus(opentelemetry::trace::StatusCode::kError, "fail"), + SpanHasAttributes( + SpanAttribute("gcloud.status_code", kErrorCode))))); +} + TEST(DataTracingConnection, ReadRowFound) { auto span_catcher = InstallSpanCatcher(); From 85ecbbda64a63004b3c01e40386e3fc9ebf19c2e Mon Sep 17 00:00:00 2001 From: dbolduc Date: Wed, 24 May 2023 14:03:31 -0400 Subject: [PATCH 3/3] update docs --- google/cloud/bigtable/examples/howto_mock_data_api.cc | 4 ++-- google/cloud/bigtable/mocks/mock_row_reader.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/google/cloud/bigtable/examples/howto_mock_data_api.cc b/google/cloud/bigtable/examples/howto_mock_data_api.cc index 68d029a235bd7..535df7b395a81 100644 --- a/google/cloud/bigtable/examples/howto_mock_data_api.cc +++ b/google/cloud/bigtable/examples/howto_mock_data_api.cc @@ -42,7 +42,7 @@ TEST(MockTableTest, ReadRowsSuccess) { // yield "r1" then "r2": //! [simulate-call] std::vector rows = {cbt::Row("r1", {}), cbt::Row("r2", {})}; - EXPECT_CALL(*mock, ReadRows) + EXPECT_CALL(*mock, ReadRowsFull) .WillOnce(Return(ByMove(cbtm::MakeRowReader(rows)))); //! [simulate-call] @@ -72,7 +72,7 @@ TEST(MockTableTest, ReadRowsFailure) { // Return a `RowReader` that yields only a failing status (no rows). gc::Status final_status(gc::StatusCode::kPermissionDenied, "fail"); - EXPECT_CALL(*mock, ReadRows) + EXPECT_CALL(*mock, ReadRowsFull) .WillOnce(Return(ByMove(cbtm::MakeRowReader({}, final_status)))); cbt::Table table(mock, cbt::TableResource("project", "instance", "table")); diff --git a/google/cloud/bigtable/mocks/mock_row_reader.h b/google/cloud/bigtable/mocks/mock_row_reader.h index a6d50cb50ecdf..c8a7dc97590c8 100644 --- a/google/cloud/bigtable/mocks/mock_row_reader.h +++ b/google/cloud/bigtable/mocks/mock_row_reader.h @@ -46,7 +46,7 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN * std::vector rows = {cbt::Row("r1", {}), cbt::Row("r2", {})}; * * auto mock = std::shared_ptr(); - * EXPECT_CALL(*mock, ReadRows) + * EXPECT_CALL(*mock, ReadRowsFull) * .WillOnce(Return(cbtm::MakeRowReader(rows))); * * auto table = cbt::Table(mock);