Python: Cleanup inconsistencies around OAuth responses (apache#5957)

* Python: Cleanup inconsistency in OAuth responses

401 also returns oauth token reponse
https://github.com/apache/iceberg/blob/master/open-api/rest-catalog-open-api.yaml#L170-L178

* Comments

* Switch them around

python/pyiceberg/catalog/__init__.py

@@ -41,6 +41,7 @@
 _ENV_CONFIG = Config()
 
 TYPE = "type"
+URI = "uri"
 
 
 class CatalogType(Enum):
@@ -69,35 +70,57 @@ def load_hive(name: str, conf: Properties) -> Catalog:
 }
 
 
-def infer_catalog_type(catalog_properties: RecursiveDict) -> CatalogType | None:
+def infer_catalog_type(name: str, catalog_properties: RecursiveDict) -> CatalogType | None:
     """Tries to infer the type based on the dict
 
     Args:
         catalog_properties: Catalog properties
 
     Returns:
         The inferred type based on the provided properties
+
+    Raises:
+        ValueError: Raises a ValueError in case properties are missing, or the wrong type
     """
     if uri := catalog_properties.get("uri"):
         if isinstance(uri, str):
             if uri.startswith("http"):
                 return CatalogType.REST
             elif uri.startswith("thrift"):
                 return CatalogType.HIVE
-    return None
+            else:
+                raise ValueError(f"Could not infer the catalog type from the uri: {uri}")
+        else:
+            raise ValueError(f"Expects the URI to be a string, got: {type(uri)}")
+    raise ValueError(
+        f"URI missing, please provide using --uri, the config or environment variable PYICEBERG_CATALOG__{name.upper()}__URI"
+    )
 
 
 def load_catalog(name: str, **properties: str | None) -> Catalog:
+    """Load the catalog based on the properties
+
+    Will look up the properties from the config, based on the name
+
+    Args:
+        name: The name of the catalog
+        properties: The properties that are used next to the configuration
+
+    Returns:
+        An initialized Catalog
+
+    Raises:
+        ValueError: Raises a ValueError in case properties are missing or malformed,
+            or if it could not determine the catalog based on the properties
+    """
     env = _ENV_CONFIG.get_catalog_config(name)
     conf = merge_config(env or {}, properties)
 
+    catalog_type: CatalogType | None
     if provided_catalog_type := conf.get(TYPE):
         catalog_type = CatalogType[provided_catalog_type.upper()]
     else:
-        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}")
+        catalog_type = infer_catalog_type(name, conf)
 
     if catalog_type:
         return AVAILABLE_CATALOGS[catalog_type](name, conf)

python/pyiceberg/catalog/rest.py

@@ -31,14 +31,14 @@
 
 from pyiceberg import __version__
 from pyiceberg.catalog import (
+    URI,
     Catalog,
     Identifier,
     Properties,
     PropertiesUpdateSummary,
 )
 from pyiceberg.exceptions import (
     AuthorizationExpiredError,
-    BadCredentialsError,
     BadRequestError,
     ForbiddenError,
     NamespaceAlreadyExistsError,
@@ -88,7 +88,9 @@ class Endpoints:
 CREDENTIAL = "credential"
 GRANT_TYPE = "grant_type"
 SCOPE = "scope"
+TOKEN = "token"
 TOKEN_EXCHANGE = "urn:ietf:params:oauth:grant-type:token-exchange"
+SEMICOLON = ":"
 
 NAMESPACE_SEPARATOR = b"\x1F".decode("UTF-8")
 
@@ -181,9 +183,10 @@ def __init__(
             properties: Properties that are passed along to the configuration
         """
         self.properties = properties
-        self.uri = properties["uri"]
-        if credential := properties.get("credential"):
-            properties["token"] = self._fetch_access_token(credential)
+        self.uri = properties[URI]
+
+        if credential := properties.get(CREDENTIAL):
+            properties[TOKEN] = self._fetch_access_token(credential)
         super().__init__(name, **self._fetch_config(properties))
 
     def _check_valid_namespace_identifier(self, identifier: Union[str, Identifier]) -> Identifier:
@@ -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 = None, credential
         data = {GRANT_TYPE: CLIENT_CREDENTIALS, CLIENT_ID: client_id, CLIENT_SECRET: client_secret, SCOPE: CATALOG_SCOPE}
         url = self.url(Endpoints.get_token, prefixed=False)
         # Uses application/x-www-form-urlencoded by default
         response = requests.post(url=url, data=data)
         try:
             response.raise_for_status()
         except HTTPError as exc:
-            self._handle_non_200_response(exc, {400: OAuthError, 401: BadCredentialsError})
+            self._handle_non_200_response(exc, {400: OAuthError, 401: OAuthError})
 
         return TokenResponse(**response.json()).access_token
 

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:
-                raise ValueError(
-                    f"URI missing, please provide using --uri, the config or environment variable PYICEBERG_CATALOG__{catalog.upper()}__URI"
-                ) from exc
-            raise exc
+        ctx.obj["catalog"] = load_catalog(catalog, **properties)
     except Exception as e:
         ctx.obj["output"].exception(e)
         ctx.exit(1)

python/pyiceberg/exceptions.py

@@ -44,10 +44,6 @@ class RESTError(Exception):
     """Raises when there is an unknown response from the REST Catalog"""
 
 
-class BadCredentialsError(RESTError):
-    """Raises when providing invalid credentials"""
-
-
 class BadRequestError(RESTError):
     """Raises when an invalid request is being made"""
 

python/tests/catalog/test_rest.py

@@ -23,12 +23,10 @@
 from pyiceberg.catalog import PropertiesUpdateSummary, Table
 from pyiceberg.catalog.rest import RestCatalog
 from pyiceberg.exceptions import (
-    BadCredentialsError,
     NamespaceAlreadyExistsError,
     NoSuchNamespaceError,
     NoSuchTableError,
     OAuthError,
-    RESTError,
     TableAlreadyExistsError,
 )
 from pyiceberg.schema import Schema
@@ -96,34 +94,14 @@ def test_token_400(rest_mock: Mocker):
 
 
 def test_token_401(rest_mock: Mocker):
-    message = "Invalid client ID: abc"
+    message = "invalid_client"
     rest_mock.post(
         f"{TEST_URI}v1/oauth/tokens",
-        json={
-            "error": {
-                "message": message,
-                "type": "BadCredentialsException",
-                "code": 401,
-            }
-        },
-        status_code=401,
-    )
-
-    with pytest.raises(BadCredentialsError) as e:
-        RestCatalog("rest", uri=TEST_URI, credential=TEST_CREDENTIALS)
-    assert message in str(e.value)
-
-
-def test_token_401_oauth_error(rest_mock: Mocker):
-    """This test returns a OAuth error instead of an OpenAPI error"""
-    message = """RESTError 401: Received unexpected JSON Payload: {"error": "invalid_client", "error_description": "Invalid credentials"}, errors: value is not a valid dict"""
-    rest_mock.post(
-        f"{TEST_URI}v1/oauth/tokens",
-        json={"error": "invalid_client", "error_description": "Invalid credentials"},
+        json={"error": "invalid_client", "error_description": "Unknown or invalid client"},
         status_code=401,
     )
 
-    with pytest.raises(RESTError) as e:
+    with pytest.raises(OAuthError) as e:
         RestCatalog("rest", uri=TEST_URI, credential=TEST_CREDENTIALS)
     assert message in str(e.value)