From de90985f668ca59213f2c01079cd190b20882f75 Mon Sep 17 00:00:00 2001 From: Stephen Rosen Date: Wed, 25 Oct 2023 12:46:09 -0500 Subject: [PATCH 1/5] Introduce the MISSING singleton This is not used anywhere yet. --- ...5_122826_sirosen_introduce_missingtype.rst | 6 ++++ src/globus_sdk/__init__.py | 8 +++++ src/globus_sdk/_generate_init.py | 7 ++++ src/globus_sdk/utils.py | 35 +++++++++++++++++++ tests/unit/test_missing_type.py | 28 +++++++++++++++ 5 files changed, 84 insertions(+) create mode 100644 changelog.d/20231025_122826_sirosen_introduce_missingtype.rst create mode 100644 tests/unit/test_missing_type.py diff --git a/changelog.d/20231025_122826_sirosen_introduce_missingtype.rst b/changelog.d/20231025_122826_sirosen_introduce_missingtype.rst new file mode 100644 index 000000000..7b27544bf --- /dev/null +++ b/changelog.d/20231025_122826_sirosen_introduce_missingtype.rst @@ -0,0 +1,6 @@ +Added +~~~~~ + +- A new sentinel value, ``globus_sdk.MISSING``, has been introduced. + It is used for method calls which need to distinguish missing parameters from + an explicit ``None`` used to signify ``null`` (:pr:`NUMBER`) diff --git a/src/globus_sdk/__init__.py b/src/globus_sdk/__init__.py index 8dfc39aaa..968e8fdff 100644 --- a/src/globus_sdk/__init__.py +++ b/src/globus_sdk/__init__.py @@ -126,6 +126,10 @@ def _force_eager_imports() -> None: "TransferClient", "TransferData", }, + "utils": { + "MISSING", + "MissingType", + }, } if t.TYPE_CHECKING: @@ -211,6 +215,8 @@ def _force_eager_imports() -> None: from .services.transfer import TransferAPIError from .services.transfer import TransferClient from .services.transfer import TransferData + from .utils import MISSING + from .utils import MissingType def __dir__() -> t.List[str]: @@ -297,7 +303,9 @@ def __getattr__(name: str) -> t.Any: "IterableTransferResponse", "LocalGlobusConnectPersonal", "LocalGlobusConnectServer", + "MISSING", "MappedCollectionDocument", + "MissingType", "NativeAppAuthClient", "NetworkError", "NullAuthorizer", diff --git a/src/globus_sdk/_generate_init.py b/src/globus_sdk/_generate_init.py index 48cf38f30..1477b2390 100755 --- a/src/globus_sdk/_generate_init.py +++ b/src/globus_sdk/_generate_init.py @@ -200,6 +200,13 @@ def __getattr__(name: str) -> t.Any: "TransferData", ), ), + ( + "utils", + ( + "MISSING", + "MissingType", + ), + ), ] diff --git a/src/globus_sdk/utils.py b/src/globus_sdk/utils.py index 3259da13c..5acae0b8f 100644 --- a/src/globus_sdk/utils.py +++ b/src/globus_sdk/utils.py @@ -22,6 +22,41 @@ PayloadWrapperBase = collections.UserDict +class MissingType: + def __init__(self) -> None: + # disable instantiation, but gated to be able to run once + # when this module is imported + if "MISSING" in globals(): + raise TypeError("MissingType should not be instantiated") + + def __bool__(self) -> bool: + return False + + def __copy__(self) -> MissingType: + return self + + def __deepcopy__(self, memo: dict[int, t.Any]) -> MissingType: + return self + + # unpickling a MissingType should always return the "MISSING" sentinel + def __reduce__(self) -> str: + return "MISSING" + + def __repr__(self) -> str: + return "" + + +# a sentinel value for "missing" values which are distinguished from `None` (null) +# this is the default used to indicate that a parameter was not passed, so that +# method calls passing `None` can be distinguished from those which did not pass any +# value +# users should typically not use this value directly, but it is part of the public SDK +# interfaces along with its type for annotation purposes +# +# *new after v3.29.0* (older methods do not use MISSING) +MISSING = MissingType() + + def sha256_string(s: str) -> str: return hashlib.sha256(s.encode("utf-8")).hexdigest() diff --git a/tests/unit/test_missing_type.py b/tests/unit/test_missing_type.py new file mode 100644 index 000000000..a0db730a1 --- /dev/null +++ b/tests/unit/test_missing_type.py @@ -0,0 +1,28 @@ +import copy +import pickle + +import pytest + +from globus_sdk import utils + + +def test_missing_type_cannot_be_instantiated(): + with pytest.raises(TypeError, match="MissingType should not be instantiated"): + utils.MissingType() + + +def test_missing_sentinel_bools_as_false(): + assert bool(utils.MISSING) is False + + +def test_str_of_missing(): + assert str(utils.MISSING) == "" + + +def test_copy_of_missing_is_self(): + assert copy.copy(utils.MISSING) is utils.MISSING + assert copy.deepcopy(utils.MISSING) is utils.MISSING + + +def test_pickle_of_missing_is_self(): + assert pickle.loads(pickle.dumps(utils.MISSING)) is utils.MISSING From 53009d915f88043a5c6fcbe92ff35e7888598719 Mon Sep 17 00:00:00 2001 From: Stephen Rosen Date: Wed, 25 Oct 2023 14:11:13 -0500 Subject: [PATCH 2/5] Default to MISSING for GroupPolicies ha timeout By defaulting to `MISSING`, we make it possible to treat an explicit `None` as a request for `null` in the payload. --- ...5_125551_sirosen_introduce_missingtype.rst | 5 ++++ src/globus_sdk/services/groups/data.py | 6 ++-- tests/unit/helpers/test_groups.py | 28 +++++++++++++++++++ 3 files changed, 37 insertions(+), 2 deletions(-) create mode 100644 changelog.d/20231025_125551_sirosen_introduce_missingtype.rst create mode 100644 tests/unit/helpers/test_groups.py diff --git a/changelog.d/20231025_125551_sirosen_introduce_missingtype.rst b/changelog.d/20231025_125551_sirosen_introduce_missingtype.rst new file mode 100644 index 000000000..b49eb3a1f --- /dev/null +++ b/changelog.d/20231025_125551_sirosen_introduce_missingtype.rst @@ -0,0 +1,5 @@ +Changed +~~~~~~~ + +- ``GroupPolicies`` objects now treat an explicit instantiation with + ``high_assurance_timeout=None`` as setting the timeout to ``null`` (:pr:`NUMBER`) diff --git a/src/globus_sdk/services/groups/data.py b/src/globus_sdk/services/groups/data.py index 8760c58fd..a3d3dded7 100644 --- a/src/globus_sdk/services/groups/data.py +++ b/src/globus_sdk/services/groups/data.py @@ -263,7 +263,9 @@ def __init__( group_members_visibility: _GROUP_MEMBER_VISIBILITY_T, join_requests: bool, signup_fields: t.Iterable[_GROUP_REQUIRED_SIGNUP_FIELDS_T], - authentication_assurance_timeout: int | None = None, + authentication_assurance_timeout: int + | None + | utils.MissingType = utils.MISSING, ): super().__init__() self["is_high_assurance"] = is_high_assurance @@ -273,5 +275,5 @@ def __init__( ) self["join_requests"] = join_requests self["signup_fields"] = utils.render_enums_for_api(signup_fields) - if authentication_assurance_timeout is not None: + if authentication_assurance_timeout is not utils.MISSING: self["authentication_assurance_timeout"] = authentication_assurance_timeout diff --git a/tests/unit/helpers/test_groups.py b/tests/unit/helpers/test_groups.py new file mode 100644 index 000000000..f76f74021 --- /dev/null +++ b/tests/unit/helpers/test_groups.py @@ -0,0 +1,28 @@ +from globus_sdk import GroupPolicies + + +def test_group_policies_can_explicitly_null_ha_timeout(): + # step 1: confirm that the auth assurance timeout is not in the baseline + # doc which does not specify it + kwargs = { + "is_high_assurance": True, + "group_visibility": "private", + "group_members_visibility": "members", + "join_requests": False, + "signup_fields": [], + } + baseline = GroupPolicies(**kwargs) + assert isinstance(baseline, GroupPolicies) + assert "authentication_assurance_timeout" not in baseline + + # step 2: confirm that the auth assurance timeout is in the doc when + # set to an integer value, even 0 + kwargs["authentication_assurance_timeout"] = 0 + with_falsy_timeout = GroupPolicies(**kwargs) + assert with_falsy_timeout["authentication_assurance_timeout"] == 0 + + # step 3: confirm that the auth assurance timeout is in the doc when + # set to None + kwargs["authentication_assurance_timeout"] = None + with_null_timeout = GroupPolicies(**kwargs) + assert with_null_timeout["authentication_assurance_timeout"] is None From b1e9f58c16a666f5310c2442dc879cb2124f0a18 Mon Sep 17 00:00:00 2001 From: Stephen Rosen Date: Wed, 25 Oct 2023 15:19:40 -0500 Subject: [PATCH 3/5] Automatically strip MISSING from payloads This change updates payload serialization in two significant ways: - MISSING is automatically removed from payloads, and this is applied recursively. This means that it can be included anywhere in a (well-formed) body and the SDK will find and remove it. Custom types inside of the payload could interfere, but native dict/list structures and PayloadWrapper objects are fully supported - PayloadWrapper types are recursively converted to dicts, meaning that it is now valid to construct a helper type based on PayloadWrapper which is nested As a result of this change, simplify the GroupPolicies code to always assign the ha timeout (with a default value of MISSING) and replace the unit test for this behavior with a functional test. The new recursive conversion was benchmarked against a version which was micro-optimized to avoid deep-copying of dict and list data, and found to be nearly 2x *faster* than the optimized version on a typical payload, by virtue of the fact that it did not make unnecessary method calls. --- ...5_122826_sirosen_introduce_missingtype.rst | 3 ++ src/globus_sdk/client.py | 2 +- src/globus_sdk/services/groups/data.py | 3 +- src/globus_sdk/utils.py | 32 +++++++++++ .../groups/test_set_group_policies.py | 18 +++++-- tests/unit/helpers/test_groups.py | 28 ---------- tests/unit/helpers/test_payload_wrapper.py | 53 +++++++++++++++++++ 7 files changed, 105 insertions(+), 34 deletions(-) delete mode 100644 tests/unit/helpers/test_groups.py create mode 100644 tests/unit/helpers/test_payload_wrapper.py diff --git a/changelog.d/20231025_122826_sirosen_introduce_missingtype.rst b/changelog.d/20231025_122826_sirosen_introduce_missingtype.rst index 7b27544bf..5307cebd7 100644 --- a/changelog.d/20231025_122826_sirosen_introduce_missingtype.rst +++ b/changelog.d/20231025_122826_sirosen_introduce_missingtype.rst @@ -4,3 +4,6 @@ Added - A new sentinel value, ``globus_sdk.MISSING``, has been introduced. It is used for method calls which need to distinguish missing parameters from an explicit ``None`` used to signify ``null`` (:pr:`NUMBER`) + + - ``globus_sdk.MISSING`` is now supported in payload data for all methods, and + will be automatically removed from the payload before sending to the server diff --git a/src/globus_sdk/client.py b/src/globus_sdk/client.py index 2cd911b2c..a362cd7ea 100644 --- a/src/globus_sdk/client.py +++ b/src/globus_sdk/client.py @@ -303,7 +303,7 @@ def request( r = self.transport.request( method=method, url=url, - data=data.data if isinstance(data, utils.PayloadWrapper) else data, + data=utils.PayloadWrapper._prepare(data), query_params=query_params, headers=rheaders, encoding=encoding, diff --git a/src/globus_sdk/services/groups/data.py b/src/globus_sdk/services/groups/data.py index a3d3dded7..7ec70823e 100644 --- a/src/globus_sdk/services/groups/data.py +++ b/src/globus_sdk/services/groups/data.py @@ -275,5 +275,4 @@ def __init__( ) self["join_requests"] = join_requests self["signup_fields"] = utils.render_enums_for_api(signup_fields) - if authentication_assurance_timeout is not utils.MISSING: - self["authentication_assurance_timeout"] = authentication_assurance_timeout + self["authentication_assurance_timeout"] = authentication_assurance_timeout diff --git a/src/globus_sdk/utils.py b/src/globus_sdk/utils.py index 5acae0b8f..d7cc62824 100644 --- a/src/globus_sdk/utils.py +++ b/src/globus_sdk/utils.py @@ -189,6 +189,38 @@ def _set_optints(self, **kwargs: t.Any) -> None: for k, v in kwargs.items(): self._set_value(k, v, callback=int) + @classmethod + def _prepare( + cls, data: str | None | dict[str, t.Any] | PayloadWrapper + ) -> str | None | dict[str, t.Any]: + """ + Prepare a payload for serialization. + + It may already be of one of the acceptable input types (str, None, dict), + but if it is a dict it will be recursively converted to + + - remove instances of `MISSING` + - convert any `PayloadWrapper` instances to dicts + """ + if data is None: + return None + if isinstance(data, str): + return data + return t.cast("dict[str, t.Any]", _recursively_prepare_payload(data)) + + +def _recursively_prepare_payload(data: t.Any) -> t.Any: + if isinstance(data, (dict, PayloadWrapper)): + return { + k: _recursively_prepare_payload(v) + for k, v in data.items() + if v is not MISSING + } + elif isinstance(data, (list, tuple)): + return [_recursively_prepare_payload(x) for x in data if x is not MISSING] + else: + return data + def in_sphinx_build() -> bool: # pragma: no cover # check if `sphinx-build` was used to invoke diff --git a/tests/functional/services/groups/test_set_group_policies.py b/tests/functional/services/groups/test_set_group_policies.py index 04bb86709..7406967a2 100644 --- a/tests/functional/services/groups/test_set_group_policies.py +++ b/tests/functional/services/groups/test_set_group_policies.py @@ -7,6 +7,7 @@ GroupPolicies, GroupRequiredSignupFields, GroupVisibility, + utils, ) from globus_sdk._testing import get_last_request, load_response @@ -72,27 +73,30 @@ def test_set_group_policies( @pytest.mark.parametrize( - "group_vis, group_member_vis, signup_fields, signup_fields_str", + "group_vis, group_member_vis, signup_fields, signup_fields_str, auth_timeout", ( ( GroupVisibility.private, GroupMemberVisibility.members, [GroupRequiredSignupFields.address1], ["address1"], + 28800, ), ( GroupVisibility.authenticated, GroupMemberVisibility.managers, ["address1"], ["address1"], + utils.MISSING, ), ( "private", "members", [GroupRequiredSignupFields.address1, "address2"], ["address1", "address2"], + 0, ), - ("authenticated", "managers", ["address1"], ["address1"]), + ("authenticated", "managers", ["address1"], ["address1"], None), ), ) @pytest.mark.parametrize("setter_usage", (False, "enum", "str")) @@ -102,6 +106,7 @@ def test_set_group_policies_explicit_payload( group_member_vis, signup_fields, signup_fields_str, + auth_timeout, setter_usage, ): group_vis_str = group_vis if isinstance(group_vis, str) else group_vis.value @@ -120,7 +125,7 @@ def test_set_group_policies_explicit_payload( group_members_visibility=group_member_vis, join_requests=False, signup_fields=signup_fields, - authentication_assurance_timeout=28800, + authentication_assurance_timeout=auth_timeout, ) if setter_usage: # set a string in the payload directly @@ -139,3 +144,10 @@ def test_set_group_policies_explicit_payload( assert req_body["group_visibility"] == group_vis_str assert req_body["group_members_visibility"] == group_member_vis_str assert req_body["signup_fields"] == signup_fields_str + + # check the authentication_assurance_timeout + # it should be omitted if it's MISSING + if auth_timeout is utils.MISSING: + assert "authentication_assurance_timeout" not in req_body + else: + assert req_body["authentication_assurance_timeout"] == auth_timeout diff --git a/tests/unit/helpers/test_groups.py b/tests/unit/helpers/test_groups.py deleted file mode 100644 index f76f74021..000000000 --- a/tests/unit/helpers/test_groups.py +++ /dev/null @@ -1,28 +0,0 @@ -from globus_sdk import GroupPolicies - - -def test_group_policies_can_explicitly_null_ha_timeout(): - # step 1: confirm that the auth assurance timeout is not in the baseline - # doc which does not specify it - kwargs = { - "is_high_assurance": True, - "group_visibility": "private", - "group_members_visibility": "members", - "join_requests": False, - "signup_fields": [], - } - baseline = GroupPolicies(**kwargs) - assert isinstance(baseline, GroupPolicies) - assert "authentication_assurance_timeout" not in baseline - - # step 2: confirm that the auth assurance timeout is in the doc when - # set to an integer value, even 0 - kwargs["authentication_assurance_timeout"] = 0 - with_falsy_timeout = GroupPolicies(**kwargs) - assert with_falsy_timeout["authentication_assurance_timeout"] == 0 - - # step 3: confirm that the auth assurance timeout is in the doc when - # set to None - kwargs["authentication_assurance_timeout"] = None - with_null_timeout = GroupPolicies(**kwargs) - assert with_null_timeout["authentication_assurance_timeout"] is None diff --git a/tests/unit/helpers/test_payload_wrapper.py b/tests/unit/helpers/test_payload_wrapper.py new file mode 100644 index 000000000..c175e8c96 --- /dev/null +++ b/tests/unit/helpers/test_payload_wrapper.py @@ -0,0 +1,53 @@ +import pytest + +from globus_sdk import utils + + +def test_payload_preparation_strips_missing_dict(): + original = {"foo": None, "bar": utils.MISSING} + prepared = utils.PayloadWrapper._prepare(original) + assert prepared == {"foo": None} + + +# this is a weird case (not really recommended usage), but we have well defined behavior +# for it, so exercise it here +@pytest.mark.parametrize("type_", (list, tuple)) +def test_payload_preparation_strips_missing_list_or_tuple(type_): + original = type_([None, 1, utils.MISSING, 0]) + prepared = utils.PayloadWrapper._prepare(original) + assert prepared == [None, 1, 0] + + +@pytest.mark.parametrize("original", (None, 1, 0, True, False, "foo", object())) +def test_payload_preparation_retains_simple_datatype_identity(original): + prepared = utils.PayloadWrapper._prepare(original) + # check not only that the values are equal, but that they pass the identity test + assert prepared is original + + +# this test makes sense in the context of the identity test above: +# check that the values are equal, although the type may be reconstructed +@pytest.mark.parametrize("original", (["foo", "bar"], {"foo": "bar"})) +def test_payload_preparation_retains_complex_datatype_equality(original): + prepared = utils.PayloadWrapper._prepare(original) + assert prepared == original + + +def test_payload_preparation_dictifies_wrappers(): + x = utils.PayloadWrapper() + x["foo"] = 1 + prepared = utils.PayloadWrapper._prepare(x) + assert prepared == {"foo": 1} + assert isinstance(prepared, dict) + assert prepared is not x + assert not isinstance(prepared, utils.PayloadWrapper) + + +def test_payload_preparation_recursively_dictifies_wrappers(): + x = utils.PayloadWrapper() + x["foo"] = 1 + y = utils.PayloadWrapper() + y["bar"] = x + y["baz"] = [2, x] + prepared = utils.PayloadWrapper._prepare(y) + assert prepared == {"bar": {"foo": 1}, "baz": [2, {"foo": 1}]} From 6ada04df4fec0d4db012221fe1ffc16d7f3c9e68 Mon Sep 17 00:00:00 2001 From: Stephen Rosen Date: Wed, 25 Oct 2023 16:58:58 -0500 Subject: [PATCH 4/5] Minor non-functional changes per review --- src/globus_sdk/services/groups/data.py | 6 +++--- src/globus_sdk/utils.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/globus_sdk/services/groups/data.py b/src/globus_sdk/services/groups/data.py index 7ec70823e..4b5b4d1cc 100644 --- a/src/globus_sdk/services/groups/data.py +++ b/src/globus_sdk/services/groups/data.py @@ -263,9 +263,9 @@ def __init__( group_members_visibility: _GROUP_MEMBER_VISIBILITY_T, join_requests: bool, signup_fields: t.Iterable[_GROUP_REQUIRED_SIGNUP_FIELDS_T], - authentication_assurance_timeout: int - | None - | utils.MissingType = utils.MISSING, + authentication_assurance_timeout: ( + int | None | utils.MissingType + ) = utils.MISSING, ): super().__init__() self["is_high_assurance"] = is_high_assurance diff --git a/src/globus_sdk/utils.py b/src/globus_sdk/utils.py index d7cc62824..7ca96c458 100644 --- a/src/globus_sdk/utils.py +++ b/src/globus_sdk/utils.py @@ -53,7 +53,7 @@ def __repr__(self) -> str: # users should typically not use this value directly, but it is part of the public SDK # interfaces along with its type for annotation purposes # -# *new after v3.29.0* (older methods do not use MISSING) +# *new after v3.29.0* MISSING = MissingType() From 0c28615ae8b666ae00cb3032451eec2286f83ad8 Mon Sep 17 00:00:00 2001 From: Stephen Rosen Date: Wed, 25 Oct 2023 17:07:21 -0500 Subject: [PATCH 5/5] Add support for `MISSING` in params and headers This is an internal feature of the client class. Entirely independently from the payload filtering process (which needs to handle the complex payload types which may be used), a simple helper for `dict|None` data is used to strip out MISSING from any query params and headers before sending. Testing also confirms the behavior of MISSING on simple JSON and Form-POST data. --- src/globus_sdk/client.py | 4 +- src/globus_sdk/utils.py | 6 ++ .../base_client/test_filter_missing.py | 57 +++++++++++++++++++ 3 files changed, 65 insertions(+), 2 deletions(-) create mode 100644 tests/functional/base_client/test_filter_missing.py diff --git a/src/globus_sdk/client.py b/src/globus_sdk/client.py index a362cd7ea..38bdab069 100644 --- a/src/globus_sdk/client.py +++ b/src/globus_sdk/client.py @@ -304,8 +304,8 @@ def request( method=method, url=url, data=utils.PayloadWrapper._prepare(data), - query_params=query_params, - headers=rheaders, + query_params=utils.filter_missing(query_params), + headers=utils.filter_missing(rheaders), encoding=encoding, authorizer=self.authorizer, allow_redirects=allow_redirects, diff --git a/src/globus_sdk/utils.py b/src/globus_sdk/utils.py index 7ca96c458..1891558f4 100644 --- a/src/globus_sdk/utils.py +++ b/src/globus_sdk/utils.py @@ -57,6 +57,12 @@ def __repr__(self) -> str: MISSING = MissingType() +def filter_missing(data: dict[str, t.Any] | None) -> dict[str, t.Any] | None: + if data is None: + return None + return {k: v for k, v in data.items() if v is not MISSING} + + def sha256_string(s: str) -> str: return hashlib.sha256(s.encode("utf-8")).hexdigest() diff --git a/tests/functional/base_client/test_filter_missing.py b/tests/functional/base_client/test_filter_missing.py new file mode 100644 index 000000000..a90b828d6 --- /dev/null +++ b/tests/functional/base_client/test_filter_missing.py @@ -0,0 +1,57 @@ +import json +import urllib.parse + +import pytest + +from globus_sdk import utils +from globus_sdk._testing import RegisteredResponse, get_last_request, load_response + + +@pytest.fixture(autouse=True) +def setup_mock_responses(): + load_response( + RegisteredResponse( + path="https://foo.api.globus.org/bar", + json={"foo": "bar"}, + ) + ) + load_response( + RegisteredResponse( + path="https://foo.api.globus.org/bar", + method="POST", + json={"foo": "bar"}, + ) + ) + + +def test_query_params_can_filter_missing(client): + res = client.get("/bar", query_params={"foo": "bar", "baz": utils.MISSING}) + assert res.http_status == 200 + req = get_last_request() + assert req.params == {"foo": "bar"} + + +def test_headers_can_filter_missing(client): + res = client.get("/bar", headers={"foo": "bar", "baz": utils.MISSING}) + assert res.http_status == 200 + req = get_last_request() + assert req.headers["foo"] == "bar" + assert "baz" not in req.headers + + +def test_json_body_can_filter_missing(client): + res = client.post("/bar", data={"foo": "bar", "baz": utils.MISSING}) + assert res.http_status == 200 + req = get_last_request() + sent = json.loads(req.body) + assert sent == {"foo": "bar"} + + +def test_form_body_can_filter_missing(client): + res = client.post( + "/bar", data={"foo": "bar", "baz": utils.MISSING}, encoding="form" + ) + assert res.http_status == 200 + req = get_last_request() + sent = urllib.parse.parse_qs(req.body) + assert sent == {"foo": ["bar"]}