Copilot commented on code in PR #9618:
URL: https://github.com/apache/gravitino/pull/9618#discussion_r2660498197
##########
flink-connector/flink/src/main/java/org/apache/gravitino/flink/connector/catalog/GravitinoCatalogManager.java:
##########
@@ -247,14 +245,19 @@ private static GravitinoAdminClient buildOAuthClient(
String credential =
config.get(GravitinoCatalogStoreFactoryOptions.OAUTH2_CREDENTIAL);
String path =
config.get(GravitinoCatalogStoreFactoryOptions.OAUTH2_TOKEN_PATH);
String scope =
config.get(GravitinoCatalogStoreFactoryOptions.OAUTH2_SCOPE);
- Preconditions.checkArgument(
- StringUtils.isNoneBlank(serverUri, credential, path, scope),
- String.format(
- "OAuth2 authentication requires: %s, %s, %s, and %s",
+
+ // Remove OAuth config from client config map, Because
+ // GravitinoAdminClient.builder().withClientConfig() does not recognize
these keys.
Review Comment:
The comment should explain why removing these keys is necessary. Consider
clarifying that these OAuth2-specific keys would cause validation errors or
unexpected behavior if passed to the GravitinoAdminClient builder.
```suggestion
// Remove OAuth-specific config entries from the client config map.
These keys are only
// used to construct the OAuth2 token provider and are not valid
GravitinoAdminClient
// client configuration options; passing them to withClientConfig()
could cause validation
// errors or other unexpected behavior.
```
##########
flink-connector/flink/src/main/java/org/apache/gravitino/flink/connector/catalog/GravitinoCatalogManager.java:
##########
@@ -247,14 +245,19 @@ private static GravitinoAdminClient buildOAuthClient(
String credential =
config.get(GravitinoCatalogStoreFactoryOptions.OAUTH2_CREDENTIAL);
String path =
config.get(GravitinoCatalogStoreFactoryOptions.OAUTH2_TOKEN_PATH);
String scope =
config.get(GravitinoCatalogStoreFactoryOptions.OAUTH2_SCOPE);
- Preconditions.checkArgument(
- StringUtils.isNoneBlank(serverUri, credential, path, scope),
- String.format(
- "OAuth2 authentication requires: %s, %s, %s, and %s",
+
+ // Remove OAuth config from client config map, Because
+ // GravitinoAdminClient.builder().withClientConfig() does not recognize
these keys.
+ Set<String> oauthConfigKeys =
+ Sets.newHashSet(
+ GravitinoCatalogStoreFactoryOptions.AUTH_TYPE,
GravitinoCatalogStoreFactoryOptions.OAUTH2_SERVER_URI,
GravitinoCatalogStoreFactoryOptions.OAUTH2_CREDENTIAL,
GravitinoCatalogStoreFactoryOptions.OAUTH2_TOKEN_PATH,
- GravitinoCatalogStoreFactoryOptions.OAUTH2_SCOPE));
+ GravitinoCatalogStoreFactoryOptions.OAUTH2_SCOPE);
+ for (String key : oauthConfigKeys) {
+ config.remove(key);
+ }
Review Comment:
The new logic for removing OAuth config keys from the client config map
lacks test coverage. Add a unit test to verify that these specific keys are
properly removed from the config before it's passed to the GravitinoAdminClient
builder.
##########
flink-connector/flink/src/main/java/org/apache/gravitino/flink/connector/catalog/GravitinoCatalogManager.java:
##########
@@ -247,14 +245,19 @@ private static GravitinoAdminClient buildOAuthClient(
String credential =
config.get(GravitinoCatalogStoreFactoryOptions.OAUTH2_CREDENTIAL);
String path =
config.get(GravitinoCatalogStoreFactoryOptions.OAUTH2_TOKEN_PATH);
String scope =
config.get(GravitinoCatalogStoreFactoryOptions.OAUTH2_SCOPE);
- Preconditions.checkArgument(
- StringUtils.isNoneBlank(serverUri, credential, path, scope),
- String.format(
- "OAuth2 authentication requires: %s, %s, %s, and %s",
+
+ // Remove OAuth config from client config map, Because
Review Comment:
The word 'Because' should not be capitalized mid-sentence. Change 'Because'
to 'because'.
```suggestion
// Remove OAuth config from client config map, because
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
