You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "flyrain (via GitHub)" <gi...@apache.org> on 2024/02/23 23:51:14 UTC
[PR] Make issued_token_type optional to support OAuth2 Client Credential Flow [iceberg-python]
flyrain opened a new pull request, #466:
URL: https://github.com/apache/iceberg-python/pull/466
To fix issue(https://github.com/apache/iceberg-python/issues/463). In short, Client Credential Flow's http response doesn't have the field `issued_token_type`. Make it optional to avoid validation error like this:
```
ValidationError: 1 validation error for TokenResponse
issued_token_type
Field required [type=missing, input_value={'access_token': 'eyJhbGc... 'token_type': 'bearer'},
input_type=dict]
```
cc @danielcweeks @Fokko @syun64 @RussellSpitzer
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make issued_token_type optional to support OAuth2 Client Credential Flow [iceberg-python]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #466:
URL: https://github.com/apache/iceberg-python/pull/466#issuecomment-1971472894
Thanks @Fokko for the review!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make issued_token_type optional to support OAuth2 Client Credential Flow [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #466:
URL: https://github.com/apache/iceberg-python/pull/466#discussion_r1504877454
##########
pyiceberg/catalog/rest.py:
##########
@@ -156,8 +156,10 @@ class RegisterTableRequest(IcebergBaseModel):
class TokenResponse(IcebergBaseModel):
access_token: str = Field()
token_type: str = Field()
- expires_in: int = Field()
- issued_token_type: str = Field()
+ expires_in: Optional[int] = None
+ issued_token_type: Optional[str] = None
+ refresh_token: Optional[str] = None
+ scope: Optional[str] = None
Review Comment:
From my perspective, just styling - it makes it consistent with how we are labeling the other Models in the repository
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make issued_token_type optional to support OAuth2 Client Credential Flow [iceberg-python]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #466:
URL: https://github.com/apache/iceberg-python/pull/466#issuecomment-1967655744
Thanks @syun64 for the review. We will need at least an approval from committers. cc @Fokko @danielcweeks
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make issued_token_type optional to support OAuth2 Client Credential Flow [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #466:
URL: https://github.com/apache/iceberg-python/pull/466#discussion_r1507438784
##########
pyiceberg/catalog/rest.py:
##########
@@ -156,8 +156,10 @@ class RegisterTableRequest(IcebergBaseModel):
class TokenResponse(IcebergBaseModel):
access_token: str = Field()
token_type: str = Field()
- expires_in: int = Field()
- issued_token_type: str = Field()
+ expires_in: Optional[int] = Field(default=None)
+ issued_token_type: Optional[str] = Field(default=None)
Review Comment:
We could also convert this into a literal, since it is an enum in the spec:
```suggestion
issued_token_type: Optional[Literal['urn:ietf:params:oauth:token-type:access_token', 'urn:ietf:params:oauth:token-type:refresh_token', 'urn:ietf:params:oauth:token-type:id_token', 'urn:ietf:params:oauth:token-type:saml1', 'urn:ietf:params:oauth:token-type:saml2', 'urn:ietf:params:oauth:token-type:jwt'] = Field(default=None)
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make issued_token_type optional to support OAuth2 Client Credential Flow [iceberg-python]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #466:
URL: https://github.com/apache/iceberg-python/pull/466#discussion_r1503212827
##########
pyiceberg/catalog/rest.py:
##########
@@ -157,7 +157,7 @@ class TokenResponse(IcebergBaseModel):
access_token: str = Field()
token_type: str = Field()
expires_in: int = Field()
- issued_token_type: str = Field()
+ issued_token_type: Optional[str] = None
Review Comment:
That's good ideas. Thanks for the review, @syun64. Fixed in a new commit.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make issued_token_type optional to support OAuth2 Client Credential Flow [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #466:
URL: https://github.com/apache/iceberg-python/pull/466#discussion_r1501297005
##########
pyiceberg/catalog/rest.py:
##########
@@ -157,7 +157,7 @@ class TokenResponse(IcebergBaseModel):
access_token: str = Field()
token_type: str = Field()
expires_in: int = Field()
- issued_token_type: str = Field()
+ issued_token_type: Optional[str] = None
Review Comment:
Could we use this opportunity to align the OAuthTokenResponse exactly with the [model specified in the Iceberg Rest Catalog Open API Spec](https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml#L3120)?
- we want to verify within the model that the **issued_token_type** is one of accepted types, if it is provided
- the current model is missing Optional **scope** and **refresh_token**
- also update **expires_in** to be Optional (expires_in is a recommended property in [OAuth](https://www.oauth.com/oauth2-servers/access-tokens/access-token-response/) and if it wasn't provided in the TokenResponse, it will fail for similar reasons in the existing model)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make issued_token_type optional to support OAuth2 Client Credential Flow [iceberg-python]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #466:
URL: https://github.com/apache/iceberg-python/pull/466#discussion_r1503212827
##########
pyiceberg/catalog/rest.py:
##########
@@ -157,7 +157,7 @@ class TokenResponse(IcebergBaseModel):
access_token: str = Field()
token_type: str = Field()
expires_in: int = Field()
- issued_token_type: str = Field()
+ issued_token_type: Optional[str] = None
Review Comment:
That's good idea. Thanks for the review, @syun64. Fixed in a new commit.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make issued_token_type optional to support OAuth2 Client Credential Flow [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #466:
URL: https://github.com/apache/iceberg-python/pull/466#discussion_r1504619419
##########
pyiceberg/catalog/rest.py:
##########
@@ -156,8 +156,10 @@ class RegisterTableRequest(IcebergBaseModel):
class TokenResponse(IcebergBaseModel):
access_token: str = Field()
token_type: str = Field()
- expires_in: int = Field()
- issued_token_type: str = Field()
+ expires_in: Optional[int] = None
+ issued_token_type: Optional[str] = None
+ refresh_token: Optional[str] = None
+ scope: Optional[str] = None
Review Comment:
This is a nit, could we use Field annotation like the attributes above?
[Example](https://github.com/apache/iceberg-python/blob/5209874cd4685427520b67b64188f5e9919f2816/pyiceberg/table/snapshots.py#L141)
```suggestion
expires_in: Optional[int] = Field(default=None)
issued_token_type: Optional[str] = Field(default=None)
refresh_token: Optional[str] = Field(default=None)
scope: Optional[str] = Field(default=None)
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make issued_token_type optional to support OAuth2 Client Credential Flow [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #466:
URL: https://github.com/apache/iceberg-python/pull/466
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make issued_token_type optional to support OAuth2 Client Credential Flow [iceberg-python]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #466:
URL: https://github.com/apache/iceberg-python/pull/466#discussion_r1504837924
##########
pyiceberg/catalog/rest.py:
##########
@@ -156,8 +156,10 @@ class RegisterTableRequest(IcebergBaseModel):
class TokenResponse(IcebergBaseModel):
access_token: str = Field()
token_type: str = Field()
- expires_in: int = Field()
- issued_token_type: str = Field()
+ expires_in: Optional[int] = None
+ issued_token_type: Optional[str] = None
+ refresh_token: Optional[str] = None
+ scope: Optional[str] = None
Review Comment:
Fixed. Thanks for the suggestion. A curious question: what do we prefer a Field annotation here?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org