himadripal commented on code in PR #9839:
URL: https://github.com/apache/iceberg/pull/9839#discussion_r1514845097
##########
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
A java class for all optional params is good suggestion, that way, we can
implement individual parameter level default and validation if we need any in
future.
> 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.
Yes, this will need changes in public API and deprecate some methods. I
tried putting `scope` in the map and some of the tests were failing, I'll take
a look again. How about we put all the optional param mentioned
[here](https://datatracker.ietf.org/doc/html/rfc8693#name-token-exchange-request-and-)
in the map and leave the scope handling for later ( I'll create an issue for
consolidating this).
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
