flyrain commented on code in PR #9839:
URL: https://github.com/apache/iceberg/pull/9839#discussion_r1511897858
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -178,15 +178,18 @@ public void initialize(String name, Map<String, String>
unresolved) {
ConfigResponse config;
OAuthTokenResponse authResponse;
String credential = props.get(OAuth2Properties.CREDENTIAL);
+ // CONSIDER : putting scope in optional param map - reduce wiring on scope
Review Comment:
Ideally, we should have a Java class synced with the
[OAuthClientCredentialsRequest](https://github.com/apache/iceberg/blob/bcbcbb263ea7e13ab22d0feb918e207c7e42dbbd/open-api/rest-catalog-open-api.yaml#L2919)
in Rest spec. It'd be nice to pass around an object of the class. The class
itself can even be autogenerated. Also by doing it this way, the Java code is
always synced with spec. And every time we made a change like this, only the
spec needs to be changed. cc @nastra
However, that's fairly a big change. We will potentially need to deprecate
some methods. Another option is to put every optional parameter(including
`scope`, `token type`) in a map at this PR, then refactor it later.
WDYT? Also cc @danielcweeks @nastra @syun64
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -178,15 +178,18 @@ public void initialize(String name, Map<String, String>
unresolved) {
ConfigResponse config;
OAuthTokenResponse authResponse;
String credential = props.get(OAuth2Properties.CREDENTIAL);
+ // CONSIDER : putting scope in optional param map - reduce wiring on scope
Review Comment:
We will also need an update on spec for these optional parameters, but I'm
OK with another PR.
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -207,7 +212,13 @@ private AuthSession authSession() {
token,
expiresAtMillis(properties()),
new AuthSession(
- ImmutableMap.of(), token, null, credential(), SCOPE,
oauth2ServerUri())));
+ ImmutableMap.of(),
+ token,
+ null,
+ credential(),
+ SCOPE,
+ oauth2ServerUri(),
+ optionalOAuthParams())));
Review Comment:
Can we extract this part so that the following one at line 231 can reuse it?
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
