You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/10/11 11:26:05 UTC
[GitHub] [iceberg] Fokko opened a new pull request, #5957: Python: Cleanup inconsistency in OAuth responses
Fokko opened a new pull request, #5957:
URL: https://github.com/apache/iceberg/pull/5957
401 also returns oauth token reponse
https://github.com/apache/iceberg/blob/master/open-api/rest-catalog-open-api.yaml#L170-L178
- This also removes the `BadCredentialsError` and replaces it with the `OAuthError`.
- Checks if there is a semicolon in the credential, and throws a nice error if not.
--
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
[GitHub] [iceberg] Fokko commented on a diff in pull request #5957: Python: Cleanup inconsistency in OAuth responses
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #5957:
URL: https://github.com/apache/iceberg/pull/5957#discussion_r993032719
##########
python/pyiceberg/cli/console.py:
##########
@@ -65,14 +65,7 @@ def run(ctx: Context, catalog: str, verbose: bool, output: str, uri: Optional[st
ctx.obj["output"] = JsonOutput(verbose=verbose)
try:
- try:
- ctx.obj["catalog"] = load_catalog(catalog, **properties)
- except ValueError as exc:
- if not uri:
Review Comment:
Mostly because it is at a very high level, and the `ValueError` is very broad. For example, if you would provide a credential without a semicolon, it would throw a ValueError:
```
>>> client, secret = "abc".split(":")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ValueError: not enough values to unpack (expected 2, got 1)
```
Next to that we currently only check if the `uri` is set from the CLI (and ignore if it is passed in from the config), it will just complain that the URI is not set (while your credential is not having a `:`).
--
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
[GitHub] [iceberg] Fokko commented on a diff in pull request #5957: Python: Cleanup inconsistency in OAuth responses
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #5957:
URL: https://github.com/apache/iceberg/pull/5957#discussion_r993032719
##########
python/pyiceberg/cli/console.py:
##########
@@ -65,14 +65,7 @@ def run(ctx: Context, catalog: str, verbose: bool, output: str, uri: Optional[st
ctx.obj["output"] = JsonOutput(verbose=verbose)
try:
- try:
- ctx.obj["catalog"] = load_catalog(catalog, **properties)
- except ValueError as exc:
- if not uri:
Review Comment:
Mostly because it is at a very high level, and the `ValueError` is very broad. For example, if you would provide a credential without a semicolon, it would throw a ValueError:
```
>>> client, secret = "abc".split(":")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ValueError: not enough values to unpack (expected 2, got 1)
```
Next to that we only check if the `uri` is passed in from the CLI `--credential abc:def` (and ignore if it is passed in from the config). This results in the error that the URI is not set (while your credential does not have a `:`).
--
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
[GitHub] [iceberg] Fokko merged pull request #5957: Python: Cleanup inconsistency in OAuth responses
Posted by GitBox <gi...@apache.org>.
Fokko merged PR #5957:
URL: https://github.com/apache/iceberg/pull/5957
--
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
[GitHub] [iceberg] Fokko commented on a diff in pull request #5957: Python: Cleanup inconsistency in OAuth responses
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #5957:
URL: https://github.com/apache/iceberg/pull/5957#discussion_r996856028
##########
python/pyiceberg/catalog/rest.py:
##########
@@ -225,15 +228,18 @@ def url(self, endpoint: str, prefixed: bool = True, **kwargs) -> str:
return url + endpoint.format(**kwargs)
def _fetch_access_token(self, credential: str) -> str:
- client_id, client_secret = credential.split(":")
+ if SEMICOLON in credential:
+ client_id, client_secret = credential.split(SEMICOLON)
+ else:
+ client_id, client_secret = credential, None
Review Comment:
Ah, that was my intention, fixed!
--
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
[GitHub] [iceberg] Fokko commented on a diff in pull request #5957: Python: Cleanup inconsistency in OAuth responses
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #5957:
URL: https://github.com/apache/iceberg/pull/5957#discussion_r993032719
##########
python/pyiceberg/cli/console.py:
##########
@@ -65,14 +65,7 @@ def run(ctx: Context, catalog: str, verbose: bool, output: str, uri: Optional[st
ctx.obj["output"] = JsonOutput(verbose=verbose)
try:
- try:
- ctx.obj["catalog"] = load_catalog(catalog, **properties)
- except ValueError as exc:
- if not uri:
Review Comment:
Mostly because it is at a very high level, and the `ValueError` is very broad. For example, if you would provide a credential without a semicolon, it would throw a ValueError:
```
>>> client, secret = "abc".split(":")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ValueError: not enough values to unpack (expected 2, got 1)
```
Next to that we only check if the `uri` is passed in from the CLI `--credential abc:def` (and ignore if it is passed in from the config). This results in the error that the URI is not set (while your credential does not have a `:`). Therefore I prefer pushing this exception down.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5957: Python: Cleanup inconsistency in OAuth responses
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5957:
URL: https://github.com/apache/iceberg/pull/5957#discussion_r994985959
##########
python/pyiceberg/catalog/rest.py:
##########
@@ -225,15 +228,18 @@ def url(self, endpoint: str, prefixed: bool = True, **kwargs) -> str:
return url + endpoint.format(**kwargs)
def _fetch_access_token(self, credential: str) -> str:
- client_id, client_secret = credential.split(":")
+ if SEMICOLON in credential:
+ client_id, client_secret = credential.split(SEMICOLON)
+ else:
+ client_id, client_secret = credential, None
Review Comment:
In Java, we do the opposite. If there is no `:`, then the entire credential is used as the client secret.
--
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
[GitHub] [iceberg] rdblue commented on pull request #5957: Python: Cleanup inconsistency in OAuth responses
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5957:
URL: https://github.com/apache/iceberg/pull/5957#issuecomment-1278018129
I think this is ready once the handling for a credential with no `:` is fixed.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5957: Python: Cleanup inconsistency in OAuth responses
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5957:
URL: https://github.com/apache/iceberg/pull/5957#discussion_r992779568
##########
python/pyiceberg/cli/console.py:
##########
@@ -65,14 +65,7 @@ def run(ctx: Context, catalog: str, verbose: bool, output: str, uri: Optional[st
ctx.obj["output"] = JsonOutput(verbose=verbose)
try:
- try:
- ctx.obj["catalog"] = load_catalog(catalog, **properties)
- except ValueError as exc:
- if not uri:
Review Comment:
Why remove this handling? It seems more correct to have this in.
--
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
[GitHub] [iceberg] Fokko commented on a diff in pull request #5957: Python: Cleanup inconsistency in OAuth responses
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #5957:
URL: https://github.com/apache/iceberg/pull/5957#discussion_r993039124
##########
python/pyiceberg/catalog/__init__.py:
##########
@@ -97,7 +98,9 @@ def load_catalog(name: str, **properties: str | None) -> Catalog:
if inferred_catalog_type := infer_catalog_type(conf):
catalog_type = inferred_catalog_type
else:
- raise ValueError(f"Invalid configuration. Could not determine the catalog type: {properties}")
+ raise ValueError(
+ f"URI missing, please provide using --uri, the config or environment variable PYICEBERG_CATALOG__{name.upper()}__URI"
Review Comment:
I've moved them to the `infer_catalog_type`, I think they make more sense there.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5957: Python: Cleanup inconsistency in OAuth responses
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5957:
URL: https://github.com/apache/iceberg/pull/5957#discussion_r992778853
##########
python/pyiceberg/catalog/rest.py:
##########
@@ -225,15 +228,18 @@ def url(self, endpoint: str, prefixed: bool = True, **kwargs) -> str:
return url + endpoint.format(**kwargs)
def _fetch_access_token(self, credential: str) -> str:
- client_id, client_secret = credential.split(":")
+ if SEMICOLON not in credential:
+ raise ValueError("Expected semicolon in the credential to separate the client id and secret.")
Review Comment:
In Java, we just use the whole string as the client secret and set the client ID to empty string.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5957: Python: Cleanup inconsistency in OAuth responses
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5957:
URL: https://github.com/apache/iceberg/pull/5957#discussion_r992778219
##########
python/pyiceberg/catalog/__init__.py:
##########
@@ -97,7 +98,9 @@ def load_catalog(name: str, **properties: str | None) -> Catalog:
if inferred_catalog_type := infer_catalog_type(conf):
catalog_type = inferred_catalog_type
else:
- raise ValueError(f"Invalid configuration. Could not determine the catalog type: {properties}")
+ raise ValueError(
+ f"URI missing, please provide using --uri, the config or environment variable PYICEBERG_CATALOG__{name.upper()}__URI"
Review Comment:
I don't think this is necessarily true. Isn't the old error message more accurate?
--
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
[GitHub] [iceberg] Fokko commented on a diff in pull request #5957: Python: Cleanup inconsistency in OAuth responses
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #5957:
URL: https://github.com/apache/iceberg/pull/5957#discussion_r993034931
##########
python/pyiceberg/catalog/rest.py:
##########
@@ -225,15 +228,18 @@ def url(self, endpoint: str, prefixed: bool = True, **kwargs) -> str:
return url + endpoint.format(**kwargs)
def _fetch_access_token(self, credential: str) -> str:
- client_id, client_secret = credential.split(":")
+ if SEMICOLON not in credential:
+ raise ValueError("Expected semicolon in the credential to separate the client id and secret.")
Review Comment:
I like that, updated! 👍🏻
--
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
[GitHub] [iceberg] Fokko commented on pull request #5957: Python: Cleanup inconsistency in OAuth responses
Posted by GitBox <gi...@apache.org>.
Fokko commented on PR #5957:
URL: https://github.com/apache/iceberg/pull/5957#issuecomment-1280692517
Thanks for the review @rdblue !
--
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