Deprecate rest.authorization-url in favor of oauth2-server-uri (apache#962)

ndrluis · web-flow · commit 50077af9cf95 · 2024-08-08T10:49:04.000+02:00
* Deprecate rest.authorization-url in favor of oauth2-server-uri

* Use deprecation_message instead of deprecated for property deprecation
diff --git a/mkdocs/docs/configuration.md b/mkdocs/docs/configuration.md
@@ -197,19 +197,19 @@ catalog:
 
 <!-- markdown-link-check-disable -->
 
-| Key                    | Example                          | Description                                                                                        |
-| ---------------------- | -------------------------------- | -------------------------------------------------------------------------------------------------- |
-| uri                    | https://rest-catalog/ws          | URI identifying the REST Server                                                                    |
-| ugi                    | t-1234:secret                    | Hadoop UGI for Hive client.                                                                        |
-| credential             | t-1234:secret                    | Credential to use for OAuth2 credential flow when initializing the catalog                         |
-| token                  | FEW23.DFSDF.FSDF                 | Bearer token value to use for `Authorization` header                                               |
-| scope                  | openid offline corpds:ds:profile | Desired scope of the requested security token (default : catalog)                                  |
-| resource               | rest_catalog.iceberg.com         | URI for the target resource or service                                                             |
-| audience               | rest_catalog                     | Logical name of target resource or service                                                         |
-| rest.sigv4-enabled     | true                             | Sign requests to the REST Server using AWS SigV4 protocol                                          |
-| rest.signing-region    | us-east-1                        | The region to use when SigV4 signing a request                                                     |
-| rest.signing-name      | execute-api                      | The service signing name to use when SigV4 signing a request                                       |
-| rest.authorization-url | https://auth-service/cc          | Authentication URL to use for client credentials authentication (default: uri + 'v1/oauth/tokens') |
+| Key                 | Example                          | Description                                                                                        |
+| ------------------- | -------------------------------- | -------------------------------------------------------------------------------------------------- |
+| uri                 | https://rest-catalog/ws          | URI identifying the REST Server                                                                    |
+| ugi                 | t-1234:secret                    | Hadoop UGI for Hive client.                                                                        |
+| credential          | t-1234:secret                    | Credential to use for OAuth2 credential flow when initializing the catalog                         |
+| token               | FEW23.DFSDF.FSDF                 | Bearer token value to use for `Authorization` header                                               |
+| scope               | openid offline corpds:ds:profile | Desired scope of the requested security token (default : catalog)                                  |
+| resource            | rest_catalog.iceberg.com         | URI for the target resource or service                                                             |
+| audience            | rest_catalog                     | Logical name of target resource or service                                                         |
+| rest.sigv4-enabled  | true                             | Sign requests to the REST Server using AWS SigV4 protocol                                          |
+| rest.signing-region | us-east-1                        | The region to use when SigV4 signing a request                                                     |
+| rest.signing-name   | execute-api                      | The service signing name to use when SigV4 signing a request                                       |
+| oauth2-server-uri   | https://auth-service/cc          | Authentication URL to use for client credentials authentication (default: uri + 'v1/oauth/tokens') |
 
 <!-- markdown-link-check-enable-->
 
diff --git a/mkdocs/docs/contributing.md b/mkdocs/docs/contributing.md
@@ -180,6 +180,24 @@ Which will warn:
 Call to load_something, deprecated in 0.1.0, will be removed in 0.2.0. Please use load_something_else() instead.
 ```
 
+If you want to remove a property or notify about a behavior change, please add a deprecation notice by calling the deprecation_message function:
+
+```python
+from pyiceberg.utils.deprecated import deprecation_message
+
+deprecation_message(
+    deprecated_in="0.1.0",
+    removed_in="0.2.0",
+    help_message="The old_property is deprecated. Please use the something_else property instead.",
+)
+```
+
+Which will warn:
+
+```
+Deprecated in 0.1.0, will be removed in 0.2.0. The old_property is deprecated. Please use the something_else property instead.
+```
+
 ## Type annotations
 
 For the type annotation the types from the `Typing` package are used.
diff --git a/mkdocs/docs/how-to-release.md b/mkdocs/docs/how-to-release.md
@@ -38,6 +38,16 @@ For example, the API with the following deprecation tag should be removed when p
 )
 ```
 
+We also have the `deprecation_message` function. We need to change the behavior according to what is noted in the message of that deprecation.
+
+```python
+deprecation_message(
+    deprecated_in="0.1.0",
+    removed_in="0.2.0",
+    help_message="The old_property is deprecated. Please use the something_else property instead.",
+)
+```
+
 ## Running a release candidate
 
 Make sure that the version is correct in `pyproject.toml` and `pyiceberg/__init__.py`. Correct means that it reflects the version that you want to release.
diff --git a/pyiceberg/catalog/__init__.py b/pyiceberg/catalog/__init__.py
@@ -67,7 +67,7 @@
     RecursiveDict,
 )
 from pyiceberg.utils.config import Config, merge_config
-from pyiceberg.utils.deprecated import deprecated
+from pyiceberg.utils.deprecated import deprecation_message
 
 if TYPE_CHECKING:
     import pyarrow as pa
@@ -742,11 +742,11 @@ def __init__(self, name: str, **properties: str):
 
         for property_name in DEPRECATED_PROPERTY_NAMES:
             if self.properties.get(property_name):
-                deprecated(
+                deprecation_message(
                     deprecated_in="0.7.0",
                     removed_in="0.8.0",
                     help_message=f"The property {property_name} is deprecated. Please use properties that start with client., glue., and dynamo. instead",
-                )(lambda: None)()
+                )
 
     def create_table_transaction(
         self,
diff --git a/pyiceberg/catalog/rest.py b/pyiceberg/catalog/rest.py
@@ -71,7 +71,8 @@
 from pyiceberg.table.sorting import UNSORTED_SORT_ORDER, SortOrder, assign_fresh_sort_order_ids
 from pyiceberg.typedef import EMPTY_DICT, UTF8, IcebergBaseModel, Identifier, Properties
 from pyiceberg.types import transform_dict_value_to_str
-from pyiceberg.utils.properties import property_as_bool
+from pyiceberg.utils.deprecated import deprecation_message
+from pyiceberg.utils.properties import get_first_property_value, property_as_bool
 
 if TYPE_CHECKING:
     import pyarrow as pa
@@ -120,6 +121,7 @@ class Endpoints:
 SIGV4_REGION = "rest.signing-region"
 SIGV4_SERVICE = "rest.signing-name"
 AUTH_URL = "rest.authorization-url"
+OAUTH2_SERVER_URI = "oauth2-server-uri"
 HEADER_PREFIX = "header."
 
 NAMESPACE_SEPARATOR = b"\x1f".decode(UTF8)
@@ -291,11 +293,38 @@ def url(self, endpoint: str, prefixed: bool = True, **kwargs: Any) -> str:
 
     @property
     def auth_url(self) -> str:
-        if url := self.properties.get(AUTH_URL):
+        if self.properties.get(AUTH_URL):
+            deprecation_message(
+                deprecated_in="0.8.0",
+                removed_in="0.9.0",
+                help_message=f"The property {AUTH_URL} is deprecated. Please use {OAUTH2_SERVER_URI} instead",
+            )
+
+        self._warn_oauth_tokens_deprecation()
+
+        if url := get_first_property_value(self.properties, AUTH_URL, OAUTH2_SERVER_URI):
             return url
         else:
             return self.url(Endpoints.get_token, prefixed=False)
 
+    def _warn_oauth_tokens_deprecation(self) -> None:
+        has_oauth_server_uri = OAUTH2_SERVER_URI in self.properties
+        has_credential = CREDENTIAL in self.properties
+        has_init_token = TOKEN in self.properties
+        has_sigv4_enabled = property_as_bool(self.properties, SIGV4, False)
+
+        if not has_oauth_server_uri and (has_init_token or has_credential) and not has_sigv4_enabled:
+            deprecation_message(
+                deprecated_in="0.8.0",
+                removed_in="1.0.0",
+                help_message="Iceberg REST client is missing the OAuth2 server URI "
+                f"configuration and defaults to {self.uri}{Endpoints.get_token}. "
+                "This automatic fallback will be removed in a future Iceberg release."
+                f"It is recommended to configure the OAuth2 endpoint using the '{OAUTH2_SERVER_URI}'"
+                "property to be prepared. This warning will disappear if the OAuth2"
+                "endpoint is explicitly configured. See https://github.com/apache/iceberg/issues/10537",
+            )
+
     def _extract_optional_oauth_params(self) -> Dict[str, str]:
         optional_oauth_param = {SCOPE: self.properties.get(SCOPE) or CATALOG_SCOPE}
         set_of_optional_params = {AUDIENCE, RESOURCE}
diff --git a/pyiceberg/utils/deprecated.py b/pyiceberg/utils/deprecated.py
@@ -30,16 +30,33 @@ def deprecated(deprecated_in: str, removed_in: str, help_message: Optional[str]
     def decorator(func: Callable):  # type: ignore
         @functools.wraps(func)
         def new_func(*args: Any, **kwargs: Any) -> Any:
-            warnings.simplefilter("always", DeprecationWarning)  # turn off filter
-
-            warnings.warn(
-                f"Call to {func.__name__}, deprecated in {deprecated_in}, will be removed in {removed_in}.{help_message}",
-                category=DeprecationWarning,
-                stacklevel=2,
-            )
-            warnings.simplefilter("default", DeprecationWarning)  # reset filter
+            message = f"Call to {func.__name__}, deprecated in {deprecated_in}, will be removed in {removed_in}.{help_message}"
+
+            _deprecation_warning(message)
+
             return func(*args, **kwargs)
 
         return new_func
 
     return decorator
+
+
+def deprecation_message(deprecated_in: str, removed_in: str, help_message: Optional[str]) -> None:
+    """Mark properties or behaviors as deprecated.
+
+    Adding this will result in a warning being emitted.
+    """
+    message = f"Deprecated in {deprecated_in}, will be removed in {removed_in}. {help_message}"
+
+    _deprecation_warning(message)
+
+
+def _deprecation_warning(message: str) -> None:
+    warnings.simplefilter("always", DeprecationWarning)  # turn off filter
+
+    warnings.warn(
+        message,
+        category=DeprecationWarning,
+        stacklevel=2,
+    )
+    warnings.simplefilter("default", DeprecationWarning)  # reset filter
diff --git a/tests/catalog/test_rest.py b/tests/catalog/test_rest.py
@@ -24,7 +24,7 @@
 
 import pyiceberg
 from pyiceberg.catalog import PropertiesUpdateSummary, load_catalog
-from pyiceberg.catalog.rest import AUTH_URL, RestCatalog
+from pyiceberg.catalog.rest import OAUTH2_SERVER_URI, RestCatalog
 from pyiceberg.exceptions import (
     AuthorizationExpiredError,
     NamespaceAlreadyExistsError,
@@ -235,7 +235,7 @@ def test_token_200_w_auth_url(rest_mock: Mocker) -> None:
     )
     # pylint: disable=W0212
     assert (
-        RestCatalog("rest", uri=TEST_URI, credential=TEST_CREDENTIALS, **{AUTH_URL: TEST_AUTH_URL})._session.headers[
+        RestCatalog("rest", uri=TEST_URI, credential=TEST_CREDENTIALS, **{OAUTH2_SERVER_URI: TEST_AUTH_URL})._session.headers[
             "Authorization"
         ]
         == f"Bearer {TEST_TOKEN}"
diff --git a/tests/utils/test_deprecated.py b/tests/utils/test_deprecated.py
@@ -35,3 +35,17 @@ def deprecated_method() -> None:
     assert warn.call_args[0] == (
         "Call to deprecated_method, deprecated in 0.1.0, will be removed in 0.2.0. Please use load_something_else() instead.",
     )
+
+
+@patch("warnings.warn")
+def test_deprecation_message(warn: Mock) -> None:
+    from pyiceberg.utils.deprecated import deprecation_message
+
+    deprecation_message(
+        deprecated_in="0.1.0",
+        removed_in="0.2.0",
+        help_message="Please use something_else instead",
+    )
+
+    assert warn.called
+    assert warn.call_args[0] == ("Deprecated in 0.1.0, will be removed in 0.2.0. Please use something_else instead",)
