Merge pull request #936 from sirosen/fix-create-policy-3

Fix AuthClient.create_policy signature

changelog.d/20231218_161848_sirosen_fix_create_policy.rst

@@ -0,0 +1,6 @@
+Changed
+~~~~~~~
+
+- The argument specification for ``AuthClient.create_policy`` was incorrect.
+  The corrected method will emit deprecation warnings if called with positional
+  arguments, as the corrected version uses keyword-only arguments. (:pr:`NUMBER`)

src/globus_sdk/_testing/data/auth/create_policy.py

@@ -7,8 +7,6 @@
 
 POLICY_REQUEST_ARGS = {
     "project_id": str(uuid.uuid1()),
-    "high_assurance": False,
-    "authentication_assurance_timeout": 35,
     "display_name": "Policy of Foo",
     "description": "Controls access to Foo",
 }
@@ -45,6 +43,7 @@ def make_response_body(request_args: t.Dict[str, t.Any]) -> t.Dict[str, t.Any]:
 
 def register_response(
     args: t.Mapping[str, t.Any],
+    match: t.Any = None,
 ) -> RegisteredResponse:
     request_args = {**POLICY_REQUEST_ARGS, **args}
     request_body = make_request_body(request_args)
@@ -61,15 +60,19 @@ def register_response(
             # Test functions use 'response' to verify response
             "response": response_body,
         },
-        match=[json_params_matcher({"policy": request_body})],
+        match=(
+            [json_params_matcher({"policy": request_body})] if match is None else match
+        ),
     )
 
 
 RESPONSES = ResponseSet(
-    default=register_response({}),
+    default=register_response({}, match=[]),
     project_id_str=register_response({"project_id": str(uuid.uuid1())}),
     project_id_uuid=register_response({"project_id": uuid.uuid1()}),
-    high_assurance=register_response({"high_assurance": True}),
+    high_assurance=register_response(
+        {"high_assurance": True, "authentication_assurance_timeout": 35}
+    ),
     not_high_assurance=register_response({"high_assurance": False}),
     authentication_assurance_timeout=register_response(
         {"authentication_assurance_timeout": 23}

src/globus_sdk/services/auth/client/service_client.py

@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import functools
 import logging
 import sys
 import typing as t
@@ -32,6 +33,45 @@
 
 log = logging.getLogger(__name__)
 
+F = t.TypeVar("F", bound=t.Callable[..., GlobusHTTPResponse])
+
+
+def _create_policy_compat(f: F) -> F:
+    @functools.wraps(f)
+    def wrapper(self: t.Any, *args: t.Any, **kwargs: t.Any) -> t.Any:
+        if args:
+            if len(args) > 5:
+                raise TypeError(
+                    "create_policy() takes 5 positional arguments "
+                    f"but {len(args)} were given"
+                )
+
+            exc.warn_deprecated(
+                "'AuthClient.create_policy' received positional arguments. "
+                "Use only keyword arguments instead."
+            )
+
+            for argname, argvalue in zip(
+                (
+                    "project_id",
+                    "high_assurance",
+                    "authentication_assurance_timeout",
+                    "display_name",
+                    "description",
+                ),
+                args,
+            ):
+                if argname in kwargs:
+                    raise TypeError(
+                        f"create_policy() got multiple values for argument '{argname}'"
+                    )
+                else:
+                    kwargs[argname] = argvalue
+
+        return f(self, **kwargs)
+
+    return t.cast(F, wrapper)
+
 
 class AuthClient(client.BaseClient):
     """
@@ -757,14 +797,15 @@ def get_policies(self) -> IterableResponse:
         """
         return GetPoliciesResponse(self.get("/v2/api/policies"))
 
-    def create_policy(
+    @_create_policy_compat
+    def create_policy(  # pylint: disable=missing-param-doc
         self,
+        *,
         project_id: UUIDLike,
-        high_assurance: bool,
-        authentication_assurance_timeout: int,
         display_name: str,
         description: str,
-        *,
+        high_assurance: bool | utils.MissingType = utils.MISSING,
+        authentication_assurance_timeout: int | utils.MissingType = utils.MISSING,
         domain_constraints_include: (
             t.Iterable[str] | None | utils.MissingType
         ) = utils.MISSING,
@@ -786,6 +827,14 @@ def create_policy(
         :param domain_constraints_exclude: A list of domains that cannot satisfy the
             policy
 
+        .. note:
+
+            ``project_id``, ``display_name``, and ``description`` are all required
+            arguments, although they are not declared as required in the function
+            signature. This is due to a backwards compatible behavior with earlier
+            versions of globus-sdk, and will be changed in a future release which
+            removes the compatible behavior.
+
         .. tab-set::
 
             .. tab-item:: Example Usage

tests/functional/services/auth/service_client/test_create_policy.py

@@ -1,8 +1,11 @@
 from __future__ import annotations
 
+import json
+
 import pytest
 
-from globus_sdk._testing import load_response
+from globus_sdk import exc
+from globus_sdk._testing import get_last_request, load_response
 
 
 @pytest.mark.parametrize(
@@ -32,3 +35,49 @@ def test_create_policy(
     res = service_client.create_policy(**meta["args"])
     for k, v in meta["response"].items():
         assert res["policy"][k] == v
+
+
+def test_compatible_create_policy_usage_rejects_too_many_positionals(service_client):
+    load_response(service_client.create_policy)
+    with pytest.raises(
+        TypeError,
+        match=r"create_policy\(\) takes 5 positional arguments but 6 were given",
+    ):
+        service_client.create_policy(1, 2, 3, 4, 5, 6)
+
+
+def test_valid_compatible_policy_usage_emits_warning(service_client):
+    load_response(service_client.create_policy)
+    with pytest.warns(
+        exc.RemovedInV4Warning,
+        match=r"'AuthClient\.create_policy' received positional arguments",
+    ):
+        service_client.create_policy(
+            "my_project_id",
+            True,
+            101,
+            display_name="my_display_name",
+            description="my_description",
+        )
+
+    lastreq = get_last_request()
+    sent_data = json.loads(lastreq.body)
+    assert sent_data["policy"] == {
+        "project_id": "my_project_id",
+        "high_assurance": True,
+        "authentication_assurance_timeout": 101,
+        "display_name": "my_display_name",
+        "description": "my_description",
+    }
+
+
+def test_policy_usage_warns_and_errors_when_argument_is_supplied_twice(service_client):
+    with pytest.raises(
+        TypeError,
+        match="create_policy\\(\\) got multiple values for argument 'project_id'",
+    ):
+        with pytest.warns(
+            exc.RemovedInV4Warning,
+            match="'AuthClient.create_policy()' received positional arguments",
+        ):
+            service_client.create_policy("my_project_id", project_id="my_project_id2")

tests/non-pytest/mypy-ignore-tests/auth_client_create_policy.py

@@ -0,0 +1,19 @@
+import globus_sdk
+
+ac = globus_sdk.AuthClient()
+
+# create new policy with keyword-only args, as supported
+ac.create_policy(
+    project_id="foo",
+    display_name="My Policy",
+    description="This is a policy",
+)
+
+# create using positional args (deprecated/unsupported)
+ac.create_policy(  # type: ignore[misc]
+    "foo",
+    True,  # type: ignore[arg-type]
+    101,  # type: ignore[arg-type]
+    "My Policy",  # type: ignore[arg-type]
+    "This is a policy",  # type: ignore[arg-type]
+)