From b1e9f58c16a666f5310c2442dc879cb2124f0a18 Mon Sep 17 00:00:00 2001 From: Stephen Rosen Date: Wed, 25 Oct 2023 15:19:40 -0500 Subject: [PATCH] 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}]}