fix(bigtable): always include app_profile_id in metadata params#16089
fix(bigtable): always include app_profile_id in metadata params#16089elinorli wants to merge 1 commit intogoogleapis:mainfrom
Conversation
…meters 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.
product-auto-label
Bot
added
the
api: bigtable
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the BigtableMetadata decorator to ensure that app_profile_id is always included in the routing parameters, even when the ID is an empty string. This change is applied across 17 different RPC methods. Additionally, a new unit test file, bigtable_metadata_decorator_test.cc, has been introduced to verify this behavior. The review feedback correctly identifies that the logic for adding this parameter is duplicated many times and should be factored out into a helper function to comply with the repository's style guide regarding code duplication.
| params.push_back(absl::StrCat("app_profile_id=", | ||
| internal::UrlEncode(request.app_profile_id()))); |
There was a problem hiding this comment.
The logic to add app_profile_id to the params vector is duplicated 17 times across this file. According to the repository style guide (lines 13-14), duplicated code appearing 3 or more times should be factored out into a helper function to improve maintainability and ensure consistency across all RPC methods.
References
- Prefers to factor out duplicated code if it appears 3 or more times in non-test files. (link)
1 participant
Unconditionally include
app_profile_idinx-goog-request-paramsrouting headers across all 17BigtableMetadatadecorator 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_idvalues.