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.
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?
| "release_level": "stable", | ||
| "transport": "both", | ||
| "language": "java", | ||
| "repo": "googleapis/google-cloud-java", |
There was a problem hiding this comment.
Why editing iam-policy files?
There was a problem hiding this comment.
It is from a git merge. Not a file change made in this PR. Not sure why the file is in the changelist.
There was a problem hiding this comment.
Reverted the file change to remove from PR.
|
|
||
| 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()?
| if ("true" | ||
| .equals( | ||
| authProperties.get( | ||
| BigQueryJdbcUrlUtility.REQUEST_GOOGLE_DRIVE_SCOPE_PROPERTY_NAME))) { | ||
| builder.setScopes(DRIVE_SCOPES); |
There was a problem hiding this comment.
Maybe we can put this logic in a private helper method and use that in all the places unless we can do what @logachev said:
If generic GoogleCredentials allows to createScoped(), can we add scope modification here instead of doing it for every type of auth separately?
There was a problem hiding this comment.
Done. As per Kirill's suggestion.
4 participants
b/500748880