From edc5bedf412088a066f07acbae87374bf7b17cc5 Mon Sep 17 00:00:00 2001 From: Zijie Li Date: Tue, 5 May 2026 16:43:08 -0400 Subject: [PATCH] fix(bigtable): always include app_profile_id in metadata routing parameters Unconditionally include app_profile_id in x-goog-request-params routing headers across all 17 BigtableMetadata decorator methods, even when it is empty because an empty app profile is treated as the default app profile. Also, add a new unit test file to verify the metadata formatting behavior for both empty and non-empty app_profile_id values. Add metadata decorator test file to CMake unit tests list. --- google/cloud/bigtable/CMakeLists.txt | 1 + .../bigtable/bigtable_client_unit_tests.bzl | 1 + .../internal/bigtable_metadata_decorator.cc | 102 ++++++----------- .../bigtable_metadata_decorator_test.cc | 107 ++++++++++++++++++ .../cloud/testing_util/validate_metadata.cc | 8 +- 5 files changed, 150 insertions(+), 69 deletions(-) create mode 100644 google/cloud/bigtable/internal/bigtable_metadata_decorator_test.cc diff --git a/google/cloud/bigtable/CMakeLists.txt b/google/cloud/bigtable/CMakeLists.txt index 2be88e34d3e0c..b8c6cb8673bdc 100644 --- a/google/cloud/bigtable/CMakeLists.txt +++ b/google/cloud/bigtable/CMakeLists.txt @@ -447,6 +447,7 @@ if (BUILD_TESTING) internal/async_row_sampler_test.cc internal/async_streaming_read_test.cc internal/bigtable_channel_refresh_test.cc + internal/bigtable_metadata_decorator_test.cc internal/bigtable_random_two_least_used_decorator_test.cc internal/bigtable_stub_factory_test.cc internal/bulk_mutator_test.cc diff --git a/google/cloud/bigtable/bigtable_client_unit_tests.bzl b/google/cloud/bigtable/bigtable_client_unit_tests.bzl index 99c7af16cbaba..bd64ebcefba91 100644 --- a/google/cloud/bigtable/bigtable_client_unit_tests.bzl +++ b/google/cloud/bigtable/bigtable_client_unit_tests.bzl @@ -42,6 +42,7 @@ bigtable_client_unit_tests = [ "internal/async_row_sampler_test.cc", "internal/async_streaming_read_test.cc", "internal/bigtable_channel_refresh_test.cc", + "internal/bigtable_metadata_decorator_test.cc", "internal/bigtable_random_two_least_used_decorator_test.cc", "internal/bigtable_stub_factory_test.cc", "internal/bulk_mutator_test.cc", diff --git a/google/cloud/bigtable/internal/bigtable_metadata_decorator.cc b/google/cloud/bigtable/internal/bigtable_metadata_decorator.cc index 1ffca73ff9c70..85828caf62406 100644 --- a/google/cloud/bigtable/internal/bigtable_metadata_decorator.cc +++ b/google/cloud/bigtable/internal/bigtable_metadata_decorator.cc @@ -76,10 +76,8 @@ BigtableMetadata::ReadRows( }(); table_name_matcher->AppendParam(request, params); - if (!request.app_profile_id().empty()) { - params.push_back(absl::StrCat( - "app_profile_id=", internal::UrlEncode(request.app_profile_id()))); - } + params.push_back(absl::StrCat("app_profile_id=", + internal::UrlEncode(request.app_profile_id()))); static auto* name_matcher = [] { return new google::cloud::internal::RoutingMatcher< @@ -131,10 +129,8 @@ BigtableMetadata::SampleRowKeys( }(); table_name_matcher->AppendParam(request, params); - if (!request.app_profile_id().empty()) { - params.push_back(absl::StrCat( - "app_profile_id=", internal::UrlEncode(request.app_profile_id()))); - } + params.push_back(absl::StrCat("app_profile_id=", + internal::UrlEncode(request.app_profile_id()))); static auto* name_matcher = [] { return new google::cloud::internal::RoutingMatcher< @@ -165,10 +161,8 @@ StatusOr BigtableMetadata::MutateRow( std::vector params; params.reserve(2); - if (!request.app_profile_id().empty()) { - params.push_back(absl::StrCat( - "app_profile_id=", internal::UrlEncode(request.app_profile_id()))); - } + params.push_back(absl::StrCat("app_profile_id=", + internal::UrlEncode(request.app_profile_id()))); static auto* table_name_matcher = [] { return new google::cloud::internal::RoutingMatcher< @@ -205,10 +199,8 @@ BigtableMetadata::MutateRows( std::vector params; params.reserve(2); - if (!request.app_profile_id().empty()) { - params.push_back(absl::StrCat( - "app_profile_id=", internal::UrlEncode(request.app_profile_id()))); - } + params.push_back(absl::StrCat("app_profile_id=", + internal::UrlEncode(request.app_profile_id()))); static auto* table_name_matcher = [] { return new google::cloud::internal::RoutingMatcher< @@ -244,10 +236,8 @@ BigtableMetadata::CheckAndMutateRow( std::vector params; params.reserve(2); - if (!request.app_profile_id().empty()) { - params.push_back(absl::StrCat( - "app_profile_id=", internal::UrlEncode(request.app_profile_id()))); - } + params.push_back(absl::StrCat("app_profile_id=", + internal::UrlEncode(request.app_profile_id()))); static auto* table_name_matcher = [] { return new google::cloud::internal::RoutingMatcher< @@ -296,10 +286,8 @@ BigtableMetadata::PingAndWarm( }(); name_matcher->AppendParam(request, params); - if (!request.app_profile_id().empty()) { - params.push_back(absl::StrCat( - "app_profile_id=", internal::UrlEncode(request.app_profile_id()))); - } + params.push_back(absl::StrCat("app_profile_id=", + internal::UrlEncode(request.app_profile_id()))); if (params.empty()) { SetMetadata(context, options); @@ -316,10 +304,8 @@ BigtableMetadata::ReadModifyWriteRow( std::vector params; params.reserve(2); - if (!request.app_profile_id().empty()) { - params.push_back(absl::StrCat( - "app_profile_id=", internal::UrlEncode(request.app_profile_id()))); - } + params.push_back(absl::StrCat("app_profile_id=", + internal::UrlEncode(request.app_profile_id()))); static auto* table_name_matcher = [] { return new google::cloud::internal::RoutingMatcher< @@ -368,10 +354,8 @@ BigtableMetadata::PrepareQuery( }(); name_matcher->AppendParam(request, params); - if (!request.app_profile_id().empty()) { - params.push_back(absl::StrCat( - "app_profile_id=", internal::UrlEncode(request.app_profile_id()))); - } + params.push_back(absl::StrCat("app_profile_id=", + internal::UrlEncode(request.app_profile_id()))); if (params.empty()) { SetMetadata(context, options); @@ -402,10 +386,8 @@ BigtableMetadata::ExecuteQuery( }(); name_matcher->AppendParam(request, params); - if (!request.app_profile_id().empty()) { - params.push_back(absl::StrCat( - "app_profile_id=", internal::UrlEncode(request.app_profile_id()))); - } + params.push_back(absl::StrCat("app_profile_id=", + internal::UrlEncode(request.app_profile_id()))); if (params.empty()) { SetMetadata(*context, options); @@ -487,10 +469,8 @@ BigtableMetadata::AsyncReadRows( }(); table_name_matcher->AppendParam(request, params); - if (!request.app_profile_id().empty()) { - params.push_back(absl::StrCat( - "app_profile_id=", internal::UrlEncode(request.app_profile_id()))); - } + params.push_back(absl::StrCat("app_profile_id=", + internal::UrlEncode(request.app_profile_id()))); static auto* name_matcher = [] { return new google::cloud::internal::RoutingMatcher< @@ -545,10 +525,8 @@ BigtableMetadata::AsyncSampleRowKeys( }(); table_name_matcher->AppendParam(request, params); - if (!request.app_profile_id().empty()) { - params.push_back(absl::StrCat( - "app_profile_id=", internal::UrlEncode(request.app_profile_id()))); - } + params.push_back(absl::StrCat("app_profile_id=", + internal::UrlEncode(request.app_profile_id()))); static auto* name_matcher = [] { return new google::cloud::internal::RoutingMatcher< @@ -583,10 +561,8 @@ BigtableMetadata::AsyncMutateRow( std::vector params; params.reserve(2); - if (!request.app_profile_id().empty()) { - params.push_back(absl::StrCat( - "app_profile_id=", internal::UrlEncode(request.app_profile_id()))); - } + params.push_back(absl::StrCat("app_profile_id=", + internal::UrlEncode(request.app_profile_id()))); static auto* table_name_matcher = [] { return new google::cloud::internal::RoutingMatcher< @@ -626,10 +602,8 @@ BigtableMetadata::AsyncMutateRows( std::vector params; params.reserve(2); - if (!request.app_profile_id().empty()) { - params.push_back(absl::StrCat( - "app_profile_id=", internal::UrlEncode(request.app_profile_id()))); - } + params.push_back(absl::StrCat("app_profile_id=", + internal::UrlEncode(request.app_profile_id()))); static auto* table_name_matcher = [] { return new google::cloud::internal::RoutingMatcher< @@ -668,10 +642,8 @@ BigtableMetadata::AsyncCheckAndMutateRow( std::vector params; params.reserve(2); - if (!request.app_profile_id().empty()) { - params.push_back(absl::StrCat( - "app_profile_id=", internal::UrlEncode(request.app_profile_id()))); - } + params.push_back(absl::StrCat("app_profile_id=", + internal::UrlEncode(request.app_profile_id()))); static auto* table_name_matcher = [] { return new google::cloud::internal::RoutingMatcher< @@ -723,10 +695,8 @@ BigtableMetadata::AsyncPingAndWarm( }(); name_matcher->AppendParam(request, params); - if (!request.app_profile_id().empty()) { - params.push_back(absl::StrCat( - "app_profile_id=", internal::UrlEncode(request.app_profile_id()))); - } + params.push_back(absl::StrCat("app_profile_id=", + internal::UrlEncode(request.app_profile_id()))); if (params.empty()) { SetMetadata(*context, *options); @@ -746,10 +716,8 @@ BigtableMetadata::AsyncReadModifyWriteRow( std::vector params; params.reserve(2); - if (!request.app_profile_id().empty()) { - params.push_back(absl::StrCat( - "app_profile_id=", internal::UrlEncode(request.app_profile_id()))); - } + params.push_back(absl::StrCat("app_profile_id=", + internal::UrlEncode(request.app_profile_id()))); static auto* table_name_matcher = [] { return new google::cloud::internal::RoutingMatcher< @@ -801,10 +769,8 @@ BigtableMetadata::AsyncPrepareQuery( }(); name_matcher->AppendParam(request, params); - if (!request.app_profile_id().empty()) { - params.push_back(absl::StrCat( - "app_profile_id=", internal::UrlEncode(request.app_profile_id()))); - } + params.push_back(absl::StrCat("app_profile_id=", + internal::UrlEncode(request.app_profile_id()))); if (params.empty()) { SetMetadata(*context, *options); diff --git a/google/cloud/bigtable/internal/bigtable_metadata_decorator_test.cc b/google/cloud/bigtable/internal/bigtable_metadata_decorator_test.cc new file mode 100644 index 0000000000000..81e05b4548c11 --- /dev/null +++ b/google/cloud/bigtable/internal/bigtable_metadata_decorator_test.cc @@ -0,0 +1,107 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "google/cloud/bigtable/internal/bigtable_metadata_decorator.h" +#include "google/cloud/bigtable/testing/mock_bigtable_stub.h" +#include "google/cloud/internal/api_client_header.h" +#include "google/cloud/testing_util/status_matchers.h" +#include "google/cloud/testing_util/validate_metadata.h" +#include +#include + +namespace google { +namespace cloud { +namespace bigtable_internal { +GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN +namespace { + +using ::google::cloud::bigtable::testing::MockBigtableStub; +using ::google::cloud::testing_util::ValidateMetadataFixture; +using ::testing::Contains; +using ::testing::Pair; + +class BigtableMetadataTest : public ::testing::Test { + protected: + void SetUp() override { mock_ = std::make_shared(); } + + std::multimap GetMetadata( + grpc::ClientContext& client_context) { + return validate_metadata_fixture_.GetMetadata(client_context); + } + + std::shared_ptr mock_; + + private: + ValidateMetadataFixture validate_metadata_fixture_; +}; + +TEST_F(BigtableMetadataTest, MutateRowAppProfileIdEmpty) { + EXPECT_CALL(*mock_, MutateRow) + .WillOnce(::testing::Return(google::bigtable::v2::MutateRowResponse{})); + + BigtableMetadata stub(mock_, {}); + grpc::ClientContext context; + google::bigtable::v2::MutateRowRequest request; + request.set_table_name("projects/p/instances/i/tables/t"); + + auto status = stub.MutateRow(context, Options{}, request); + auto metadata = GetMetadata(context); + EXPECT_THAT(metadata, Contains(Pair("x-goog-request-params", + "app_profile_id=&table_name=projects%2Fp%" + "2Finstances%2Fi%2Ftables%2Ft"))); +} + +TEST_F(BigtableMetadataTest, MutateRowAppProfileIdNotEmpty) { + EXPECT_CALL(*mock_, MutateRow) + .WillOnce(::testing::Return(google::bigtable::v2::MutateRowResponse{})); + + BigtableMetadata stub(mock_, {}); + grpc::ClientContext context; + google::bigtable::v2::MutateRowRequest request; + request.set_table_name("projects/p/instances/i/tables/t"); + request.set_app_profile_id("my-profile"); + + auto status = stub.MutateRow(context, Options{}, request); + auto metadata = GetMetadata(context); + EXPECT_THAT(metadata, + Contains(Pair("x-goog-request-params", + "app_profile_id=my-profile&table_name=projects%2Fp%" + "2Finstances%2Fi%2Ftables%2Ft"))); +} + +TEST_F(BigtableMetadataTest, MutateRowIsContextMDValidWithEmptyAppProfileId) { + EXPECT_CALL(*mock_, MutateRow) + .WillOnce([](grpc::ClientContext& context, Options const&, + google::bigtable::v2::MutateRowRequest const& request) { + ValidateMetadataFixture fixture; + fixture.IsContextMDValid( + context, "google.bigtable.v2.Bigtable.MutateRow", request, + google::cloud::internal::GeneratedLibClientHeader()); + return google::bigtable::v2::MutateRowResponse{}; + }); + + BigtableMetadata stub(mock_, {}); + grpc::ClientContext context; + google::bigtable::v2::MutateRowRequest request; + request.set_table_name("projects/p/instances/i/tables/t"); + + auto status = stub.MutateRow(context, Options{}, request); + EXPECT_STATUS_OK(status); +} + +} // namespace +GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END +} // namespace bigtable_internal +} // namespace cloud +} // namespace google diff --git a/google/cloud/testing_util/validate_metadata.cc b/google/cloud/testing_util/validate_metadata.cc index 229089b752535..0f42c80a67bbd 100644 --- a/google/cloud/testing_util/validate_metadata.cc +++ b/google/cloud/testing_util/validate_metadata.cc @@ -69,7 +69,13 @@ RoutingHeaders ExtractMDFromHeader(std::string header) { std::regex assign_re("([^=]+)=([^=]+)"); std::smatch match_res; std::string s = i->str(); - bool const matched = std::regex_match(s, match_res, assign_re); + bool matched = std::regex_match(s, match_res, assign_re); + // A special case for app_profile_id because an empty app profile is treated + // as the default app profile. We allow it structurally, but skip inserting + // it into the map to avoid mismatches against empty expectations. + if (!matched && s == "app_profile_id=") { + continue; + } EXPECT_TRUE(matched) << "Bad header format. The header should be a series of \"a=b\" " "delimited with \"&\", but is \"" +