From 50077af9cf951dffc8daa60d29b5443e8dea3093 Mon Sep 17 00:00:00 2001 From: Andre Luis Anastacio Date: Thu, 8 Aug 2024 05:49:04 -0300 Subject: [PATCH] Deprecate `rest.authorization-url` in favor of `oauth2-server-uri` (#962) * Deprecate rest.authorization-url in favor of oauth2-server-uri * Use deprecation_message instead of deprecated for property deprecation --- mkdocs/docs/configuration.md | 26 +++++++++++++------------- mkdocs/docs/contributing.md | 18 ++++++++++++++++++ mkdocs/docs/how-to-release.md | 10 ++++++++++ pyiceberg/catalog/__init__.py | 6 +++--- pyiceberg/catalog/rest.py | 33 +++++++++++++++++++++++++++++++-- pyiceberg/utils/deprecated.py | 33 +++++++++++++++++++++++++-------- tests/catalog/test_rest.py | 4 ++-- tests/utils/test_deprecated.py | 14 ++++++++++++++ 8 files changed, 116 insertions(+), 28 deletions(-) diff --git a/mkdocs/docs/configuration.md b/mkdocs/docs/configuration.md index 6bfc567135..3445b5a7b9 100644 --- a/mkdocs/docs/configuration.md +++ b/mkdocs/docs/configuration.md @@ -197,19 +197,19 @@ catalog: -| 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') | diff --git a/mkdocs/docs/contributing.md b/mkdocs/docs/contributing.md index beacf0f365..a716f13479 100644 --- 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 index ce38cbfc9c..59ad6977a6 100644 --- 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 index 02b9f9ddb9..6469c0d6f8 100644 --- 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 index 813398faec..639d732bf0 100644 --- 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 index 0de8cbfad8..92051b4fe6 100644 --- 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 index fe9a32fe41..174fff5d9d 100644 --- 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 index 7c44c45859..a77ed66563 100644 --- 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",)