impl(storage): decommission GCS specific OAuth2 API #15883
Conversation
product-auto-label
bot
added
the
api: storage
Summary of ChangesHello @scotthart, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the authentication infrastructure within the Google Cloud C++ client library for Google Cloud Storage. The primary goal is to consolidate authentication logic by removing GCS-specific OAuth2 implementations and fully adopting the unified Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 decommissions the GCS-specific OAuth2 API, replacing it with a unified credentials implementation. This is a significant and positive refactoring that centralizes authentication logic. The changes are extensive, involving the removal of many old credential-related files and updating call sites to use the new APIs. My review found a couple of places where error handling for file operations could be improved to provide better diagnostics. Otherwise, the changes look correct and consistent with the goal of the PR.
| std::ifstream is(*cfg.file_path()); | ||
| std::string contents(std::istreambuf_iterator<char>{is}, {}); | ||
| result = std::make_unique<GrpcServiceAccountAuthentication>( | ||
| std::move(contents), std::move(options)); |
There was a problem hiding this comment.
There's no error handling here if the file specified by cfg.file_path() cannot be opened. If std::ifstream fails to open the file, it will be in a bad state, and reading from it will produce an empty string. This will likely cause a JSON parsing error later, which can be misleading. It would be better to check if the file was opened successfully and return a specific error if it was not.
std::ifstream is(*cfg.file_path());
if (!is) {
result = std::make_unique<GrpcErrorAuthentication>(
google::cloud::internal::UnknownError(
"Cannot open credentials file " + *cfg.file_path(),
GCP_ERROR_INFO()));
return;
}
std::string contents(std::istreambuf_iterator<char>{is}, {});
result = std::make_unique<GrpcServiceAccountAuthentication>(
std::move(contents), std::move(options));| std::ifstream is(path); | ||
| std::string contents(std::istreambuf_iterator<char>{is}, {}); | ||
| return CreateServiceAccountCredentialsFromJsonContents( | ||
| std::move(contents), options, std::move(client_factory)); |
There was a problem hiding this comment.
Similar to another part of this PR, there's no error handling if the file at path cannot be opened. If std::ifstream fails, it enters a bad state, and contents will be an empty string. This will lead to a less specific error message down the line. It's better to check for file opening success and return an error immediately.
std::ifstream is(path);
if (!is) {
return google::cloud::internal::UnknownError(
"Cannot open credentials file " + path, GCP_ERROR_INFO());
}
std::string contents(std::istreambuf_iterator<char>{is}, {});
return CreateServiceAccountCredentialsFromJsonContents(
std::move(contents), options, std::move(client_factory));
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## prepare-for-v3.0.0 #15883 +/- ##
======================================================
- Coverage 92.87% 92.82% -0.05%
======================================================
Files 2391 2360 -31
Lines 218434 216573 -1861
======================================================
- Hits 202866 201041 -1825
+ Misses 15568 15532 -36 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
scotthart
force-pushed
the
v3_storage_use_unified_creds
branch
from
dc982cb to
3e9ad21
Compare
1 participant
No description provided.