fix(bqjdbc): add Google Driver scope to all credential types#12847
fix(bqjdbc): add Google Driver scope to all credential types#12847
Conversation
![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 refactors the handling of the RequestGoogleDriveScope property in BigQueryJdbcOAuthUtility. The property is now processed globally and applied across all credential types, including Service Account, User Account, Pre-generated tokens, Application Default Credentials, and External Account authentication. The implementation standardizes the logic for adding the Google Drive read-only scope using boolean string checks and includes corresponding test updates. Feedback was provided regarding the duplication of hardcoded scope lists across multiple methods, suggesting they be moved to a shared utility class for better maintainability.
| throw new IllegalStateException(OAUTH_TYPE_ERROR_MESSAGE); | ||
| } | ||
|
|
||
| return getServiceAccountImpersonatedCredentials(credentials, authProperties); |
There was a problem hiding this comment.
If generic GoogleCredentials allows to createScoped(), can we add scope modification here instead of doing it for every type of auth separately?
| static final String DRIVE_READONLY_SCOPE = "https://www.googleapis.com/auth/drive.readonly"; | ||
|
|
||
| static final List<String> DEFAULT_SCOPES = Arrays.asList(BIGQUERY_SCOPE); | ||
| static final List<String> DRIVE_SCOPES = Arrays.asList(BIGQUERY_SCOPE, DRIVE_READONLY_SCOPE); |
There was a problem hiding this comment.
nit: BIGQUERY_WITH_DRIVE_SCOPES?
| @@ -10,7 +10,6 @@ | |||
| "repo": "googleapis/google-cloud-java", | |||
There was a problem hiding this comment.
Why editing iam-policy files?
|
|
||
| Integer reqGoogleDriveScope = ds.getRequestGoogleDriveScope(); | ||
| if (reqGoogleDriveScope != null) { | ||
| Boolean reqGoogleDriveScopeBool = |
There was a problem hiding this comment.
I also don't like using magic string & comparing it for bool property.. Should we use retrieve bool at Connection layer from ds & pass bool directly to getCredentials()?
3 participants
b/500748880