Merge pull request #1165 from RohanJnr/smart_syncing_users

Smart syncing of Users

2 files changed, 144 insertions, 74 deletions

diff --git a/bot/exts/backend/sync/_syncers.py b/bot/exts/backend/sync/_syncers.py
index 3d4a09df3..38468c2b1 100644
--- a/bot/exts/backend/sync/_syncers.py
+++ b/bot/exts/backend/sync/_syncers.py
@@ -14,7 +14,6 @@ log = logging.getLogger(__name__)
 # These objects are declared as namedtuples because tuples are hashable,
 # something that we make use of when diffing site roles against guild roles.
 _Role = namedtuple('Role', ('id', 'name', 'colour', 'permissions', 'position'))
-_User = namedtuple('User', ('id', 'name', 'discriminator', 'roles', 'in_guild'))
 _Diff = namedtuple('Diff', ('created', 'updated', 'deleted'))
 
 
@@ -134,61 +133,76 @@ class UserSyncer(Syncer):
     async def _get_diff(self, guild: Guild) -> _Diff:
         """Return the difference of users between the cache of `guild` and the database."""
         log.trace("Getting the diff for users.")
-        users = await self.bot.api_client.get('bot/users')
 
-        # Pack DB roles and guild roles into one common, hashable format.
-        # They're hashable so that they're easily comparable with sets later.
-        db_users = {
-            user_dict['id']: _User(
-                roles=tuple(sorted(user_dict.pop('roles'))),
-                **user_dict
-            )
-            for user_dict in users
-        }
-        guild_users = {
-            member.id: _User(
-                id=member.id,
-                name=member.name,
-                discriminator=int(member.discriminator),
-                roles=tuple(sorted(role.id for role in member.roles)),
-                in_guild=True
-            )
-            for member in guild.members
-        }
+        users_to_create = []
+        users_to_update = []
+        seen_guild_users = set()
+
+        async for db_user in self._get_users():
+            # Store user fields which are to be updated.
+            updated_fields = {}
 
-        users_to_create = set()
-        users_to_update = set()
+            def maybe_update(db_field: str, guild_value: t.Union[str, int]) -> None:
+                # Equalize DB user and guild user attributes.
+                if db_user[db_field] != guild_value:
+                    updated_fields[db_field] = guild_value
 
-        for db_user in db_users.values():
-            guild_user = guild_users.get(db_user.id)
-            if guild_user is not None:
-                if db_user != guild_user:
-                    users_to_update.add(guild_user)
+            if guild_user := guild.get_member(db_user["id"]):
+                seen_guild_users.add(guild_user.id)
 
-            elif db_user.in_guild:
+                maybe_update("name", guild_user.name)
+                maybe_update("discriminator", int(guild_user.discriminator))
+                maybe_update("in_guild", True)
+
+                guild_roles = [role.id for role in guild_user.roles]
+                if set(db_user["roles"]) != set(guild_roles):
+                    updated_fields["roles"] = guild_roles
+
+            elif db_user["in_guild"]:
                 # The user is known in the DB but not the guild, and the
                 # DB currently specifies that the user is a member of the guild.
                 # This means that the user has left since the last sync.
                 # Update the `in_guild` attribute of the user on the site
                 # to signify that the user left.
-                new_api_user = db_user._replace(in_guild=False)
-                users_to_update.add(new_api_user)
-
-        new_user_ids = set(guild_users.keys()) - set(db_users.keys())
-        for user_id in new_user_ids:
-            # The user is known on the guild but not on the API. This means
-            # that the user has joined since the last sync. Create it.
-            new_user = guild_users[user_id]
-            users_to_create.add(new_user)
+                updated_fields["in_guild"] = False
+
+            if updated_fields:
+                updated_fields["id"] = db_user["id"]
+                users_to_update.append(updated_fields)
+
+        for member in guild.members:
+            if member.id not in seen_guild_users:
+                # The user is known on the guild but not on the API. This means
+                # that the user has joined since the last sync. Create it.
+                new_user = {
+                    "id": member.id,
+                    "name": member.name,
+                    "discriminator": int(member.discriminator),
+                    "roles": [role.id for role in member.roles],
+                    "in_guild": True
+                }
+                users_to_create.append(new_user)
 
         return _Diff(users_to_create, users_to_update, None)
 
+    async def _get_users(self) -> t.AsyncIterable:
+        """GET users from database."""
+        query_params = {
+            "page": 1
+        }
+        while query_params["page"]:
+            res = await self.bot.api_client.get("bot/users", params=query_params)
+            for user in res["results"]:
+                yield user
+
+            query_params["page"] = res["next_page_no"]
+
     async def _sync(self, diff: _Diff) -> None:
         """Synchronise the database with the user cache of `guild`."""
         log.trace("Syncing created users...")
-        for user in diff.created:
-            await self.bot.api_client.post('bot/users', json=user._asdict())
+        if diff.created:
+            await self.bot.api_client.post("bot/users", json=diff.created)
 
         log.trace("Syncing updated users...")
-        for user in diff.updated:
-            await self.bot.api_client.put(f'bot/users/{user.id}', json=user._asdict())
+        if diff.updated:
+            await self.bot.api_client.patch("bot/users/bulk_patch", json=diff.updated)
diff --git a/tests/bot/exts/backend/sync/test_users.py b/tests/bot/exts/backend/sync/test_users.py
index c0a1da35c..9f380a15d 100644
--- a/tests/bot/exts/backend/sync/test_users.py
+++ b/tests/bot/exts/backend/sync/test_users.py
@@ -1,7 +1,6 @@
 import unittest
-from unittest import mock
 
-from bot.exts.backend.sync._syncers import UserSyncer, _Diff, _User
+from bot.exts.backend.sync._syncers import UserSyncer, _Diff
 from tests import helpers
 
 
@@ -10,7 +9,7 @@ def fake_user(**kwargs):
     kwargs.setdefault("id", 43)
     kwargs.setdefault("name", "bob the test man")
     kwargs.setdefault("discriminator", 1337)
-    kwargs.setdefault("roles", (666,))
+    kwargs.setdefault("roles", [666])
     kwargs.setdefault("in_guild", True)
 
     return kwargs
@@ -40,22 +39,42 @@ class UserSyncerDiffTests(unittest.IsolatedAsyncioTestCase):
 
         return guild
 
+    @staticmethod
+    def get_mock_member(member: dict):
+        member = member.copy()
+        del member["in_guild"]
+        mock_member = helpers.MockMember(**member)
+        mock_member.roles = [helpers.MockRole(id=role_id) for role_id in member["roles"]]
+        return mock_member
+
     async def test_empty_diff_for_no_users(self):
         """When no users are given, an empty diff should be returned."""
+        self.bot.api_client.get.return_value = {
+            "count": 3,
+            "next_page_no": None,
+            "previous_page_no": None,
+            "results": []
+        }
         guild = self.get_guild()
 
         actual_diff = await self.syncer._get_diff(guild)
-        expected_diff = (set(), set(), None)
+        expected_diff = ([], [], None)
 
         self.assertEqual(actual_diff, expected_diff)
 
     async def test_empty_diff_for_identical_users(self):
         """No differences should be found if the users in the guild and DB are identical."""
-        self.bot.api_client.get.return_value = [fake_user()]
+        self.bot.api_client.get.return_value = {
+            "count": 3,
+            "next_page_no": None,
+            "previous_page_no": None,
+            "results": [fake_user()]
+        }
         guild = self.get_guild(fake_user())
 
+        guild.get_member.return_value = self.get_mock_member(fake_user())
         actual_diff = await self.syncer._get_diff(guild)
-        expected_diff = (set(), set(), None)
+        expected_diff = ([], [], None)
 
         self.assertEqual(actual_diff, expected_diff)
 
@@ -63,59 +82,102 @@ class UserSyncerDiffTests(unittest.IsolatedAsyncioTestCase):
         """Only updated users should be added to the 'updated' set of the diff."""
         updated_user = fake_user(id=99, name="new")
 
-        self.bot.api_client.get.return_value = [fake_user(id=99, name="old"), fake_user()]
+        self.bot.api_client.get.return_value = {
+            "count": 3,
+            "next_page_no": None,
+            "previous_page_no": None,
+            "results": [fake_user(id=99, name="old"), fake_user()]
+        }
         guild = self.get_guild(updated_user, fake_user())
+        guild.get_member.side_effect = [
+            self.get_mock_member(updated_user),
+            self.get_mock_member(fake_user())
+        ]
 
         actual_diff = await self.syncer._get_diff(guild)
-        expected_diff = (set(), {_User(**updated_user)}, None)
+        expected_diff = ([], [{"id": 99, "name": "new"}], None)
 
         self.assertEqual(actual_diff, expected_diff)
 
     async def test_diff_for_new_users(self):
-        """Only new users should be added to the 'created' set of the diff."""
+        """Only new users should be added to the 'created' list of the diff."""
         new_user = fake_user(id=99, name="new")
 
-        self.bot.api_client.get.return_value = [fake_user()]
+        self.bot.api_client.get.return_value = {
+            "count": 3,
+            "next_page_no": None,
+            "previous_page_no": None,
+            "results": [fake_user()]
+        }
         guild = self.get_guild(fake_user(), new_user)
-
+        guild.get_member.side_effect = [
+            self.get_mock_member(fake_user()),
+            self.get_mock_member(new_user)
+        ]
         actual_diff = await self.syncer._get_diff(guild)
-        expected_diff = ({_User(**new_user)}, set(), None)
+        expected_diff = ([new_user], [], None)
 
         self.assertEqual(actual_diff, expected_diff)
 
     async def test_diff_sets_in_guild_false_for_leaving_users(self):
         """When a user leaves the guild, the `in_guild` flag is updated to `False`."""
-        leaving_user = fake_user(id=63, in_guild=False)
-
-        self.bot.api_client.get.return_value = [fake_user(), fake_user(id=63)]
+        self.bot.api_client.get.return_value = {
+            "count": 3,
+            "next_page_no": None,
+            "previous_page_no": None,
+            "results": [fake_user(), fake_user(id=63)]
+        }
         guild = self.get_guild(fake_user())
+        guild.get_member.side_effect = [
+            self.get_mock_member(fake_user()),
+            None
+        ]
 
         actual_diff = await self.syncer._get_diff(guild)
-        expected_diff = (set(), {_User(**leaving_user)}, None)
+        expected_diff = ([], [{"id": 63, "in_guild": False}], None)
 
         self.assertEqual(actual_diff, expected_diff)
 
     async def test_diff_for_new_updated_and_leaving_users(self):
         """When users are added, updated, and removed, all of them are returned properly."""
         new_user = fake_user(id=99, name="new")
+
         updated_user = fake_user(id=55, name="updated")
-        leaving_user = fake_user(id=63, in_guild=False)
 
-        self.bot.api_client.get.return_value = [fake_user(), fake_user(id=55), fake_user(id=63)]
+        self.bot.api_client.get.return_value = {
+            "count": 3,
+            "next_page_no": None,
+            "previous_page_no": None,
+            "results": [fake_user(), fake_user(id=55), fake_user(id=63)]
+        }
         guild = self.get_guild(fake_user(), new_user, updated_user)
+        guild.get_member.side_effect = [
+            self.get_mock_member(fake_user()),
+            self.get_mock_member(updated_user),
+            None
+        ]
 
         actual_diff = await self.syncer._get_diff(guild)
-        expected_diff = ({_User(**new_user)}, {_User(**updated_user), _User(**leaving_user)}, None)
+        expected_diff = ([new_user], [{"id": 55, "name": "updated"}, {"id": 63, "in_guild": False}], None)
 
         self.assertEqual(actual_diff, expected_diff)
 
     async def test_empty_diff_for_db_users_not_in_guild(self):
-        """When the DB knows a user the guild doesn't, no difference is found."""
-        self.bot.api_client.get.return_value = [fake_user(), fake_user(id=63, in_guild=False)]
+        """When the DB knows a user, but the guild doesn't, no difference is found."""
+        self.bot.api_client.get.return_value = {
+            "count": 3,
+            "next_page_no": None,
+            "previous_page_no": None,
+            "results": [fake_user(), fake_user(id=63, in_guild=False)]
+        }
         guild = self.get_guild(fake_user())
+        guild.get_member.side_effect = [
+            self.get_mock_member(fake_user()),
+            None
+        ]
 
         actual_diff = await self.syncer._get_diff(guild)
-        expected_diff = (set(), set(), None)
+        expected_diff = ([], [], None)
 
         self.assertEqual(actual_diff, expected_diff)
 
@@ -131,13 +193,10 @@ class UserSyncerSyncTests(unittest.IsolatedAsyncioTestCase):
         """Only POST requests should be made with the correct payload."""
         users = [fake_user(id=111), fake_user(id=222)]
 
-        user_tuples = {_User(**user) for user in users}
-        diff = _Diff(user_tuples, set(), None)
+        diff = _Diff(users, [], None)
         await self.syncer._sync(diff)
 
-        calls = [mock.call("bot/users", json=user) for user in users]
-        self.bot.api_client.post.assert_has_calls(calls, any_order=True)
-        self.assertEqual(self.bot.api_client.post.call_count, len(users))
+        self.bot.api_client.post.assert_called_once_with("bot/users", json=diff.created)
 
         self.bot.api_client.put.assert_not_called()
         self.bot.api_client.delete.assert_not_called()
@@ -146,13 +205,10 @@ class UserSyncerSyncTests(unittest.IsolatedAsyncioTestCase):
         """Only PUT requests should be made with the correct payload."""
         users = [fake_user(id=111), fake_user(id=222)]
 
-        user_tuples = {_User(**user) for user in users}
-        diff = _Diff(set(), user_tuples, None)
+        diff = _Diff([], users, None)
         await self.syncer._sync(diff)
 
-        calls = [mock.call(f"bot/users/{user['id']}", json=user) for user in users]
-        self.bot.api_client.put.assert_has_calls(calls, any_order=True)
-        self.assertEqual(self.bot.api_client.put.call_count, len(users))
+        self.bot.api_client.patch.assert_called_once_with("bot/users/bulk_patch", json=diff.updated)
 
         self.bot.api_client.post.assert_not_called()
         self.bot.api_client.delete.assert_not_called()