Skip to content

Commit 45fe5af

Browse files
mpeddada1mpeddada1
authored
test(bigtable): add conformance test for FailsOnEmptyMetadata and FailsOnExecuteQueryMetadata (#15784)
* test(bigtable): add conformance test for FailsOnEmptyMetadata and FailsOnExecuteQueryMetadata
1 parent 887d7d0 commit 45fe5af

File tree

4 files changed

+33
-74
lines changed

4 files changed

+33
-74
lines changed

‎google/cloud/bigtable/ci/run_conformance_tests_proxy_bazel.sh‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ exit_status=$?
5151
# CloseClient
5252
go test -v \
5353
-run "TestExecuteQuery|TestExecuteQuery_PlanRefresh$|TestExecuteQuery_PlanRefresh_WithMetadataChange|TestExecuteQuery_PlanRefresh_Retries|TestExecuteQuery_FailsOnSuccesfulStreamWithNoToken" \
54-
-skip "CloseClient|FailsOnEmptyMetadata|FailsOnExecuteQueryMetadata|FailsOnInvalidType|FailsOnTypeMismatch|FailsOnTypeMismatchWithinMap|FailsOnTypeMismatchWithinArray|FailsOnTypeMismatchWithinStruct|FailsOnStructMissingField|TestExecuteQuery_PlanRefresh_AfterResumeTokenCausesError|TestExecuteQuery_RetryTest_WithPlanRefresh|TestExecuteQuery_PlanRefresh_RespectsDeadline|TestExecuteQuery_PlanRefresh_RecoversAfterPermanentError" \
54+
-skip "CloseClient|FailsOnInvalidType|FailsOnTypeMismatch|FailsOnTypeMismatchWithinMap|FailsOnTypeMismatchWithinArray|FailsOnTypeMismatchWithinStruct|FailsOnStructMissingField|TestExecuteQuery_PlanRefresh_AfterResumeTokenCausesError|TestExecuteQuery_RetryTest_WithPlanRefresh|TestExecuteQuery_PlanRefresh_RespectsDeadline|TestExecuteQuery_PlanRefresh_RecoversAfterPermanentError" \
5555
-proxy_addr=:9999
5656
exit_status=$?
5757

@@ -61,7 +61,7 @@ exit_status=$?
6161

6262
# Metadata tests b/461232934
6363
#go test -v \
64-
# -run "FailsOnEmptyMetadata|FailsOnExecuteQueryMetadata|FailsOnInvalidType" \
64+
# -run "FailsOnInvalidType" \
6565
# -proxy_addr=:9999
6666
#exit_status=$?
6767

‎google/cloud/bigtable/internal/data_connection_impl.cc‎

Lines changed: 11 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -123,12 +123,10 @@ class DefaultPartialResultSetReader
123123
std::shared_ptr<OperationContext> operation_context,
124124
std::unique_ptr<internal::StreamingReadRpc<
125125
google::bigtable::v2::ExecuteQueryResponse>>
126-
reader,
127-
google::bigtable::v2::ResultSetMetadata initial_metadata)
126+
reader)
128127
: context_(std::move(context)),
129128
operation_context_(std::move(operation_context)),
130-
reader_(std::move(reader)),
131-
initial_metadata_(std::move(initial_metadata)) {}
129+
reader_(std::move(reader)) {}
132130

133131
~DefaultPartialResultSetReader() override = default;
134132

@@ -154,22 +152,13 @@ class DefaultPartialResultSetReader
154152
return true;
155153
}
156154

157-
// Throw an error when there is a schema difference between
158-
// ExecuteQueryResponse and PrepareQueryResponse.
155+
// Throw an error when ExecuteQueryResponse returns metadata.
159156
if (response.has_metadata()) {
160-
std::string initial_metadata_str;
161-
std::string response_metadata_str;
162-
bool metadata_matched =
163-
initial_metadata_.SerializeToString(&initial_metadata_str) &&
164-
response.metadata().SerializeToString(&response_metadata_str) &&
165-
initial_metadata_str == response_metadata_str;
166-
if (response.metadata().ByteSizeLong() > 0 && !metadata_matched) {
167-
final_status_ = internal::AbortedError(
168-
"Schema changed during ExecuteQuery operation", GCP_ERROR_INFO());
169-
operation_context_->PostCall(*context_, final_status_);
170-
return false;
171-
}
172-
continue;
157+
final_status_ = internal::InternalError(
158+
"Expected results response, but received: METADATA",
159+
GCP_ERROR_INFO());
160+
operation_context_->PostCall(*context_, final_status_);
161+
return false;
173162
}
174163

175164
final_status_ = internal::InternalError(
@@ -189,7 +178,6 @@ class DefaultPartialResultSetReader
189178
std::unique_ptr<
190179
internal::StreamingReadRpc<google::bigtable::v2::ExecuteQueryResponse>>
191180
reader_;
192-
google::bigtable::v2::ResultSetMetadata initial_metadata_;
193181
Status final_status_;
194182
};
195183

@@ -913,8 +901,8 @@ bigtable::RowStream DataConnectionImpl::ExecuteQuery(
913901
std::shared_ptr<OperationContext> const& operation_context) mutable
914902
-> StatusOr<std::unique_ptr<bigtable::ResultSourceInterface>> {
915903
auto factory =
916-
[stub, request, tracing_enabled, tracing_options, operation_context,
917-
initial_metadata = metadata](std::string const& resume_token) mutable {
904+
[stub, request, tracing_enabled, tracing_options,
905+
operation_context](std::string const& resume_token) mutable {
918906
if (!resume_token.empty()) request.set_resume_token(resume_token);
919907
auto context = std::make_shared<grpc::ClientContext>();
920908
auto const& options = internal::CurrentOptions();
@@ -923,8 +911,7 @@ bigtable::RowStream DataConnectionImpl::ExecuteQuery(
923911
auto stream = stub->ExecuteQuery(context, options, request);
924912
std::unique_ptr<PartialResultSetReader> reader =
925913
std::make_unique<DefaultPartialResultSetReader>(
926-
std::move(context), operation_context, std::move(stream),
927-
std::move(initial_metadata));
914+
std::move(context), operation_context, std::move(stream));
928915
if (tracing_enabled) {
929916
reader = std::make_unique<LoggingResultSetReader>(std::move(reader),
930917
tracing_options);

‎google/cloud/bigtable/internal/data_connection_impl_test.cc‎

Lines changed: 19 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -3054,10 +3054,6 @@ TEST_F(DataConnectionTest, ExecuteQuerySuccessWithTransientErrors) {
30543054

30553055
auto stream = std::make_unique<MockExecuteQueryStream>();
30563056
EXPECT_CALL(*stream, Read)
3057-
.WillOnce([&](google::bigtable::v2::ExecuteQueryResponse* r) {
3058-
*r->mutable_metadata() = pq_response.metadata();
3059-
return absl::nullopt;
3060-
})
30613057
.WillOnce([&](google::bigtable::v2::ExecuteQueryResponse* r) {
30623058
MakeResponse(*r->mutable_results(), {"r1", "v1"}, absl::nullopt,
30633059
false);
@@ -3308,10 +3304,6 @@ TEST_F(DataConnectionTest, ExecuteQuerySuccessWithQueryPlanRefresh) {
33083304

33093305
auto stream = std::make_unique<MockExecuteQueryStream>();
33103306
EXPECT_CALL(*stream, Read)
3311-
.WillOnce([&](google::bigtable::v2::ExecuteQueryResponse* r) {
3312-
*r->mutable_metadata() = refresh_pq_response.metadata();
3313-
return absl::nullopt;
3314-
})
33153307
.WillOnce([&](google::bigtable::v2::ExecuteQueryResponse* r) {
33163308
MakeResponse(*r->mutable_results(), {"r1", "v1"}, absl::nullopt,
33173309
false);
@@ -3479,10 +3471,6 @@ TEST_F(DataConnectionTest, PrepareAndExecuteQuerySuccessWithQueryPlanRefresh) {
34793471

34803472
auto stream = std::make_unique<MockExecuteQueryStream>();
34813473
EXPECT_CALL(*stream, Read)
3482-
.WillOnce([&](ExecuteQueryResponse* r) {
3483-
*r->mutable_metadata() = refresh_pq_response.metadata();
3484-
return absl::nullopt;
3485-
})
34863474
.WillOnce([&](ExecuteQueryResponse* r) {
34873475
MakeResponse(*r->mutable_results(), {"r1", "v1"}, absl::nullopt,
34883476
false);
@@ -3649,10 +3637,6 @@ TEST_F(DataConnectionTest,
36493637

36503638
auto stream = std::make_unique<MockExecuteQueryStream>();
36513639
EXPECT_CALL(*stream, Read)
3652-
.WillOnce([&](ExecuteQueryResponse* r) {
3653-
*r->mutable_metadata() = refresh_pq_response.metadata();
3654-
return absl::nullopt;
3655-
})
36563640
.WillOnce([&](ExecuteQueryResponse* r) {
36573641
MakeResponse(*r->mutable_results(), {"r1", "v1"}, absl::nullopt,
36583642
false);
@@ -3726,44 +3710,29 @@ TEST_F(DataConnectionTest, ExecuteQueryFailureWithSchemaChange) {
37263710
kResultMetadataText, pq_response.mutable_metadata()));
37273711
*pq_response.mutable_valid_until() = internal::ToProtoTimestamp(
37283712
std::chrono::system_clock::now() + std::chrono::seconds(3600));
3729-
3730-
auto constexpr kExecuteQueryResultMetadataText = R"pb(
3731-
proto_schema {
3732-
columns {
3733-
name: "row_key"
3734-
type { string_type {} }
3735-
}
3736-
columns {
3737-
name: "different_value"
3738-
type { string_type {} }
3739-
}
3740-
}
3741-
)pb";
37423713
v2::ExecuteQueryResponse eq_response;
37433714
ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(
3744-
kExecuteQueryResultMetadataText, eq_response.mutable_metadata()));
3715+
kResultMetadataText, eq_response.mutable_metadata()));
37453716

37463717
auto refresh_fn = []() {
37473718
return make_ready_future(
37483719
StatusOr<google::bigtable::v2::PrepareQueryResponse>(
37493720
Status{StatusCode::kUnimplemented, "not implemented"}));
37503721
};
37513722
EXPECT_CALL(*mock, ExecuteQuery)
3752-
.Times(3)
3753-
.WillRepeatedly(
3754-
[&](auto, auto const&,
3755-
google::bigtable::v2::ExecuteQueryRequest const& request) {
3756-
EXPECT_EQ(request.app_profile_id(), kAppProfile);
3757-
EXPECT_EQ(request.instance_name(),
3758-
"projects/test-project/instances/test-instance");
3759-
auto stream = std::make_unique<MockExecuteQueryStream>();
3760-
EXPECT_CALL(*stream, Read)
3761-
.WillOnce([&](google::bigtable::v2::ExecuteQueryResponse* r) {
3762-
*r = eq_response;
3763-
return absl::nullopt;
3764-
});
3765-
return stream;
3766-
});
3723+
.WillOnce([&](auto, auto const&,
3724+
google::bigtable::v2::ExecuteQueryRequest const& request) {
3725+
EXPECT_EQ(request.app_profile_id(), kAppProfile);
3726+
EXPECT_EQ(request.instance_name(),
3727+
"projects/test-project/instances/test-instance");
3728+
auto stream = std::make_unique<MockExecuteQueryStream>();
3729+
EXPECT_CALL(*stream, Read)
3730+
.WillOnce([&](google::bigtable::v2::ExecuteQueryResponse* r) {
3731+
*r = eq_response;
3732+
return absl::nullopt;
3733+
});
3734+
return stream;
3735+
});
37673736

37683737
auto conn = TestConnection(std::move(mock), std::move(factory));
37693738
internal::OptionsSpan span(CallOptions());
@@ -3779,8 +3748,11 @@ TEST_F(DataConnectionTest, ExecuteQueryFailureWithSchemaChange) {
37793748
bigtable::ExecuteQueryParams params{std::move(bq)};
37803749
auto row_stream = conn->ExecuteQuery(std::move(params));
37813750
for (auto const& row : row_stream) {
3782-
EXPECT_THAT(row,
3783-
StatusIs(StatusCode::kAborted, HasSubstr("Schema changed")));
3751+
EXPECT_THAT(
3752+
row,
3753+
StatusIs(
3754+
StatusCode::kInternal,
3755+
HasSubstr("Expected results response, but received: METADATA")));
37843756
}
37853757
fake_cq_impl->SimulateCompletion(false);
37863758
}

‎google/cloud/bigtable/internal/partial_result_set_source.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -212,7 +212,7 @@ Status PartialResultSetSource::BufferProtoRows() {
212212

213213
if (columns_size == 0 && !proto_values.empty()) {
214214
return internal::InternalError(
215-
"ProtoRows has values but the schema has no columns.",
215+
"ProtoRows has values but ResultSetMetadata columns cannot be empty",
216216
GCP_ERROR_INFO());
217217
}
218218
if (proto_values.size() % columns_size != 0) {

0 commit comments

Comments
 (0)