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
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)
…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. Add metadata decorator test file to CMake unit tests list.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #16089 +/- ##
=======================================
Coverage 92.70% 92.70%
=======================================
Files 2353 2354 +1
Lines 218352 218393 +41
=======================================
+ Hits 202425 202468 +43
+ Misses 15927 15925 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mutianf
left a comment
There was a problem hiding this comment.
Undo approve. bigtable_metadata_decorator is a autogenerated file
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.