Skip to content

Commit

Permalink
Automatically strip MISSING from payloads
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
sirosen committed Oct 25, 2023
1 parent 53009d9 commit b1e9f58
Show file tree
Hide file tree
Showing 7 changed files with 105 additions and 34 deletions.
3 changes: 3 additions & 0 deletions changelog.d/20231025_122826_sirosen_introduce_missingtype.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion src/globus_sdk/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
3 changes: 1 addition & 2 deletions src/globus_sdk/services/groups/data.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
32 changes: 32 additions & 0 deletions src/globus_sdk/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 15 additions & 3 deletions tests/functional/services/groups/test_set_group_policies.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
GroupPolicies,
GroupRequiredSignupFields,
GroupVisibility,
utils,
)
from globus_sdk._testing import get_last_request, load_response

Expand Down Expand Up @@ -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"))
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
28 changes: 0 additions & 28 deletions tests/unit/helpers/test_groups.py

This file was deleted.

53 changes: 53 additions & 0 deletions tests/unit/helpers/test_payload_wrapper.py
Original file line number Diff line number Diff line change
@@ -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}]}

0 comments on commit b1e9f58

Please sign in to comment.