Merge pull request #60 from adobe-apiplatform/v2

prepare for version 2.9 release

Author: adobeDan

HISTORY.md

@@ -119,3 +119,10 @@ Because the UMAPI functionality around Adobe IDs is now different for migrated o
     * Allow setting attributes on Adobe ID users as long as the server allows it.
 * [Issue 55](https://github.com/adobe-apiplatform/umapi-client.py/issues/55)
     * Don't default the country code when creating new Enterprise ID users.
+
+### Version 2.9
+
+Bug fix release:
+
+* [Issue 58](https://github.com/adobe-apiplatform/umapi-client.py/issues/58)
+    * Error when adding more than 10 groups in a single action step.

tests/test_connections.py

@@ -21,12 +21,14 @@
 import time
 from email.utils import formatdate
 
+import six
 import mock
 import pytest
 import requests
 
 from conftest import mock_connection_params, MockResponse
-from umapi_client import Connection, UnavailableError, ServerError, RequestError
+from umapi_client import Connection, UnavailableError, ServerError, RequestError, UserAction, GroupTypes, \
+    IdentityTypes, RoleTypes
 from umapi_client import __version__ as umapi_version
 
 
@@ -242,3 +244,178 @@ def test_post_request_fail():
         mock_post.return_value = MockResponse(400, text="400 test request failure")
         conn = Connection(**mock_connection_params)
         pytest.raises(RequestError, conn.make_call, "", "[3, 5]")
+
+
+def test_large_group_assignment_split():
+    """
+    Ensure that large group list can be split into multiple commands
+    :return:
+    """
+    group_prefix = "G"
+    add_groups = [group_prefix+six.text_type(n+1) for n in range(0, 15)]
+    user = UserAction(id_type=IdentityTypes.enterpriseID, email="[email protected]")
+    user.add_to_groups(groups=add_groups, group_type=GroupTypes.usergroup)
+    assert user.maybe_split_groups(10) is True
+    assert len(user.commands) == 2
+    assert user.commands[0]["add"][GroupTypes.usergroup.name] == add_groups[0:10]
+    assert user.commands[1]["add"][GroupTypes.usergroup.name] == add_groups[10:]
+
+
+def test_large_group_assignment_split_recursive():
+    """
+    Test group list large enough to trigger recursive split
+    :return:
+    """
+    group_prefix = "G"
+    add_groups = [group_prefix+six.text_type(n+1) for n in range(0, 100)]
+    user = UserAction(id_type=IdentityTypes.enterpriseID, email="[email protected]")
+    user.add_to_groups(groups=add_groups, group_type=GroupTypes.usergroup)
+    assert user.maybe_split_groups(10) is True
+    assert len(user.commands) == 10
+
+
+def test_large_group_mix_split():
+    """
+    Ensure that group split works on add and remove
+    Each "add" and "remove" group should be split into 2 groups each
+    :return:
+    """
+    group_prefix = "G"
+    add_groups = [group_prefix+six.text_type(n+1) for n in range(0, 15)]
+    remove_groups = [group_prefix+six.text_type(n+1) for n in range(15, 30)]
+    user = UserAction(id_type=IdentityTypes.enterpriseID, email="[email protected]")
+    user.add_to_groups(groups=add_groups, group_type=GroupTypes.usergroup) \
+        .remove_from_groups(groups=remove_groups, group_type=GroupTypes.usergroup)
+    assert user.maybe_split_groups(10) is True
+    assert len(user.commands) == 4
+    assert user.commands[0]["add"][GroupTypes.usergroup.name] == add_groups[0:10]
+    assert user.commands[1]["add"][GroupTypes.usergroup.name] == add_groups[10:]
+    assert user.commands[2]["remove"][GroupTypes.usergroup.name] == remove_groups[0:10]
+    assert user.commands[3]["remove"][GroupTypes.usergroup.name] == remove_groups[10:]
+
+
+def test_large_group_action_split():
+    """
+    Ensure that very large group lists (100+) will be handled appropriately
+    Connection.execute_multiple splits commands and splits actions
+    Result should be 2 actions, even though we only created one action
+    :return:
+    """
+    with mock.patch("umapi_client.connection.requests.Session.post") as mock_post:
+        mock_post.return_value = MockResponse(200, {"result": "success"})
+        conn = Connection(**mock_connection_params)
+
+        group_prefix = "G"
+        add_groups = [group_prefix+six.text_type(n+1) for n in range(0, 150)]
+        user = UserAction(id_type=IdentityTypes.enterpriseID, email="[email protected]")
+        user.add_to_groups(groups=add_groups, group_type=GroupTypes.usergroup)
+        assert conn.execute_single(user, immediate=True) == (0, 2, 2)
+
+
+def test_group_size_limit():
+    """
+    Test with different 'throttle_groups' value, which governs the max size of the group list before commands are split
+    :return:
+    """
+    with mock.patch("umapi_client.connection.requests.Session.post") as mock_post:
+        mock_post.return_value = MockResponse(200, {"result": "success"})
+        params = mock_connection_params
+        params['throttle_groups'] = 5
+        conn = Connection(**params)
+
+        group_prefix = "G"
+        add_groups = [group_prefix+six.text_type(n+1) for n in range(0, 150)]
+        user = UserAction(id_type=IdentityTypes.enterpriseID, email="[email protected]")
+        user.add_to_groups(groups=add_groups, group_type=GroupTypes.usergroup)
+        assert conn.execute_single(user, immediate=True) == (0, 3, 3)
+
+
+def test_split_add_user():
+    """
+    Make sure split doesn't do anything when we have a non-add/remove group action
+    :return:
+    """
+    user = UserAction(id_type=IdentityTypes.enterpriseID, email="[email protected]")
+    user.create(first_name="Example", last_name="User", country="US", email="[email protected]")
+    user.update(first_name="EXAMPLE")
+    assert user.maybe_split_groups(10) is False
+    assert len(user.commands) == 2
+    assert user.wire_dict() == {'do': [{'createEnterpriseID': {'country': 'US',
+                                                               'email': '[email protected]',
+                                                               'firstname': 'Example',
+                                                               'lastname': 'User',
+                                                               'option': 'ignoreIfAlreadyExists'}},
+                                       {'update': {'firstname': 'EXAMPLE'}}],
+                                'user': '[email protected]'}
+
+
+def test_split_role_assignment():
+    group_prefix = "G"
+    add_groups = [group_prefix+six.text_type(n+1) for n in range(0, 25)]
+    user = UserAction(id_type=IdentityTypes.enterpriseID, email="[email protected]")
+    user.add_role(groups=add_groups, role_type=RoleTypes.admin)
+    assert user.maybe_split_groups(10) is True
+    assert len(user.commands) == 3
+
+
+def test_no_group_split():
+    """
+    maybe_split should return false if nothing was split
+    :return:
+    """
+    group_prefix = "G"
+    add_groups = [group_prefix+six.text_type(n+1) for n in range(0, 5)]
+    user = UserAction(id_type=IdentityTypes.enterpriseID, email="[email protected]")
+    user.add_to_groups(groups=add_groups, group_type=GroupTypes.usergroup)
+    assert user.maybe_split_groups(10) is False
+    assert len(user.commands) == 1
+
+
+def test_complex_group_split():
+    """
+    Test a complex command with add and remove directive, with multiple group types
+    UserAction's interface doesn't support this, so we build our own command array
+    :return:
+    """
+    group_prefix = "G"
+    add_groups = [group_prefix+six.text_type(n+1) for n in range(0, 150)]
+    add_products = [group_prefix+six.text_type(n+1) for n in range(0, 26)]
+    user = UserAction(id_type=IdentityTypes.enterpriseID, email="[email protected]")
+    user.commands = [{
+        "add": {
+            GroupTypes.usergroup.name: add_groups,
+            GroupTypes.product.name: add_products,
+        },
+    }]
+    assert user.maybe_split_groups(10) is True
+    assert len(user.commands) == 15
+    assert len([c for c in user.commands if 'product' in c['add']]) == 3
+    assert GroupTypes.product.name not in user.commands[3]['add']
+
+
+def test_split_remove_all():
+    """
+    Don't split groups if "remove" is "all" instead of list
+    :return:
+    """
+    group_prefix = "G"
+    add_groups = [group_prefix+six.text_type(n+1) for n in range(0, 11)]
+    user = UserAction(id_type=IdentityTypes.enterpriseID, email="[email protected]")
+    user.remove_from_groups(all_groups=True)
+    assert user.maybe_split_groups(1) is False
+    assert user.wire_dict() == {'do': [{'remove': 'all'}], 'user': '[email protected]'}
+    user.add_to_groups(groups=add_groups)
+    assert user.maybe_split_groups(10) is True
+    assert user.wire_dict() == {'do': [{'remove': 'all'},
+                                       {'add': {'product': ['G1',
+                                                            'G2',
+                                                            'G3',
+                                                            'G4',
+                                                            'G5',
+                                                            'G6',
+                                                            'G7',
+                                                            'G8',
+                                                            'G9',
+                                                            'G10']}},
+                                       {'add': {'product': ['G11']}}],
+                                'user': '[email protected]'}

umapi_client/connection.py

@@ -55,6 +55,7 @@ def __init__(self,
                  timeout_seconds=120.0,
                  throttle_actions=10,
                  throttle_commands=10,
+                 throttle_groups=10,
                  user_agent=None,
                  **kwargs):
         """
@@ -91,6 +92,7 @@ def __init__(self,
         :param timeout_seconds: How many seconds to wait for server response (<= 0 or None means forever)
         :param throttle_actions: Max number of actions to pack into a single call
         :param throttle_commands: Max number of commands allowed in a single action
+        :param throttle_groups: Max number of groups to add/remove to/from a user
         :param user_agent: (optional) string to use as User-Agent header (umapi-client/version data will be added)
 
         Additional keywords are allowed to make it easy to pass a big dictionary with other values
@@ -106,6 +108,7 @@ def __init__(self,
         self.timeout = float(timeout_seconds) if timeout_seconds and float(timeout_seconds) > 0.0 else None
         self.throttle_actions = max(int(throttle_actions), 1)
         self.throttle_commands = max(int(throttle_commands), 1)
+        self.throttle_groups = max(int(throttle_groups), 1)
         self.action_queue = []
         self.local_status = {"multiple-query-count": 0,
                              "single-query-count": 0,
@@ -305,11 +308,20 @@ def execute_multiple(self, actions, immediate=True):
         :return: tuple: the number of actions in the queue, that got sent, and that executed successfully.
         """
         # throttling part 1: split up each action into smaller actions, as needed
+        # optionally split large lists of groups in add/remove commands (if action supports it)
         split_actions = []
         exceptions = []
         for a in actions:
             if len(a.commands) == 0:
                 if self.logger: self.logger.warning("Sending action with no commands: %s", a.frame)
+            # maybe_split_groups is a UserAction attribute, so the call may throw an AttributeError
+            try:
+                if a.maybe_split_groups(self.throttle_groups):
+                    if self.logger: self.logger.debug(
+                        "Throttling actions %s to have a maximum of %d entries in group lists.",
+                        a.frame, self.throttle_groups)
+            except AttributeError:
+                pass
             if len(a.commands) > self.throttle_commands:
                 if self.logger: self.logger.debug("Throttling action %s to have a maximum of %d commands.",
                                                   a.frame, self.throttle_commands)

umapi_client/functional.py

@@ -308,6 +308,39 @@ def delete_account(self):
         self.append(removeFromDomain={})
         return None
 
+    def maybe_split_groups(self, max_groups):
+        """
+        Check if group lists in add/remove directives should be split and split them if needed
+        :param max_groups: Max group list size
+        :return: True if at least one command was split, False if none were split
+        """
+        split_commands = []
+        # return True if we split at least once
+        maybe_split = False
+        valid_step_keys = ['add', 'addRoles', 'remove']
+        for command in self.commands:
+            # commands are assumed to contain a single key
+            step_key, step_args = next(six.iteritems(command))
+            if step_key not in valid_step_keys or not isinstance(step_args, dict):
+                split_commands.append(command)
+                continue
+            new_commands = [command]
+            while True:
+                new_command = {step_key: {}}
+                for group_type, groups in six.iteritems(command[step_key]):
+                    if len(groups) > max_groups:
+                        command[step_key][group_type], new_command[step_key][group_type] = \
+                            groups[0:max_groups], groups[max_groups:]
+                if new_command[step_key]:
+                    new_commands.append(new_command)
+                    command = new_command
+                    maybe_split = True
+                else:
+                    break
+            split_commands += new_commands
+        self.commands = split_commands
+        return maybe_split
+
 
 class UsersQuery(QueryMultiple):
     """

umapi_client/version.py

@@ -18,4 +18,4 @@
 # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 # SOFTWARE.
 
-__version__ = "2.8"
+__version__ = "2.9"