Merge pull request #357 from python-discord/joseph/change-roles-field-to-postgres-array

Change roles field on user model to Postgres array

11 files changed, 89 insertions, 25 deletions

diff --git a/pydis_site/apps/api/migrations/0053_user_roles_to_array.py b/pydis_site/apps/api/migrations/0053_user_roles_to_array.py
new file mode 100644
index 00000000..7ff3a548
--- /dev/null
+++ b/pydis_site/apps/api/migrations/0053_user_roles_to_array.py
@@ -0,0 +1,24 @@
+# Generated by Django 2.2.11 on 2020-06-02 13:42
+
+import django.contrib.postgres.fields
+import django.core.validators
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('api', '0052_remove_user_avatar_hash'),
+    ]
+
+    operations = [
+        migrations.RemoveField(
+            model_name='user',
+            name='roles',
+        ),
+        migrations.AddField(
+            model_name='user',
+            name='roles',
+            field=django.contrib.postgres.fields.ArrayField(base_field=models.BigIntegerField(validators=[django.core.validators.MinValueValidator(limit_value=0, message='Role IDs cannot be negative.')]), default=list, help_text='IDs of roles the user has on the server', size=None),
+        ),
+    ]
diff --git a/pydis_site/apps/api/migrations/0054_user_invalidate_unknown_role.py b/pydis_site/apps/api/migrations/0054_user_invalidate_unknown_role.py
new file mode 100644
index 00000000..96230015
--- /dev/null
+++ b/pydis_site/apps/api/migrations/0054_user_invalidate_unknown_role.py
@@ -0,0 +1,21 @@
+# Generated by Django 2.2.11 on 2020-06-02 20:08
+
+import django.contrib.postgres.fields
+import django.core.validators
+from django.db import migrations, models
+import pydis_site.apps.api.models.bot.user
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('api', '0053_user_roles_to_array'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='user',
+            name='roles',
+            field=django.contrib.postgres.fields.ArrayField(base_field=models.BigIntegerField(validators=[django.core.validators.MinValueValidator(limit_value=0, message='Role IDs cannot be negative.'), pydis_site.apps.api.models.bot.user._validate_existing_role]), default=list, help_text='IDs of roles the user has on the server', size=None),
+        ),
+    ]
diff --git a/pydis_site/apps/api/models/bot/user.py b/pydis_site/apps/api/models/bot/user.py
index 65e8751e..bff4d642 100644
--- a/pydis_site/apps/api/models/bot/user.py
+++ b/pydis_site/apps/api/models/bot/user.py
@@ -1,3 +1,5 @@
+from django.contrib.postgres.fields import ArrayField
+from django.core.exceptions import ValidationError
 from django.core.validators import MaxValueValidator, MinValueValidator
 from django.db import models
 
@@ -5,6 +7,14 @@ from pydis_site.apps.api.models.bot.role import Role
 from pydis_site.apps.api.models.utils import ModelReprMixin
 
 
+def _validate_existing_role(value: int) -> None:
+    """Validate that a role exists when given in to the user model."""
+    role = Role.objects.filter(id=value)
+
+    if not role:
+        raise ValidationError(f"Role with ID {value} does not exist")
+
+
 class User(ModelReprMixin, models.Model):
     """A Discord user."""
 
@@ -31,9 +41,18 @@ class User(ModelReprMixin, models.Model):
         ),
         help_text="The discriminator of this user, taken from Discord."
     )
-    roles = models.ManyToManyField(
-        Role,
-        help_text="Any roles this user has on our server."
+    roles = ArrayField(
+        models.BigIntegerField(
+            validators=(
+                MinValueValidator(
+                    limit_value=0,
+                    message="Role IDs cannot be negative."
+                ),
+                _validate_existing_role
+            )
+        ),
+        default=list,
+        help_text="IDs of roles the user has on the server"
     )
     in_guild = models.BooleanField(
         default=True,
@@ -51,7 +70,7 @@ class User(ModelReprMixin, models.Model):
 
         This will fall back to the Developers role if the user does not have any roles.
         """
-        roles = self.roles.all()
+        roles = Role.objects.filter(id__in=self.roles)
         if not roles:
             return Role.objects.get(name="Developers")
-        return max(self.roles.all())
+        return max(roles)
diff --git a/pydis_site/apps/api/serializers.py b/pydis_site/apps/api/serializers.py
index cc3f167d..f2d5144c 100644
--- a/pydis_site/apps/api/serializers.py
+++ b/pydis_site/apps/api/serializers.py
@@ -229,8 +229,6 @@ class TagSerializer(ModelSerializer):
 class UserSerializer(BulkSerializerMixin, ModelSerializer):
     """A class providing (de-)serialization of `User` instances."""
 
-    roles = PrimaryKeyRelatedField(many=True, queryset=Role.objects.all(), required=False)
-
     class Meta:
         """Metadata defined for the Django REST Framework."""
 
diff --git a/pydis_site/apps/api/tests/test_users.py b/pydis_site/apps/api/tests/test_users.py
index 4f563dc6..4c0f6e27 100644
--- a/pydis_site/apps/api/tests/test_users.py
+++ b/pydis_site/apps/api/tests/test_users.py
@@ -146,7 +146,7 @@ class UserModelTests(APISubdomainTestCase):
             discriminator=1111,
             in_guild=True,
         )
-        cls.user_with_roles.roles.add(cls.role_bottom, cls.role_top)
+        cls.user_with_roles.roles.extend([cls.role_bottom.id, cls.role_top.id])
 
         cls.user_without_roles = User.objects.create(
             id=2,
diff --git a/pydis_site/apps/api/viewsets/bot/user.py b/pydis_site/apps/api/viewsets/bot/user.py
index 8f5bccfa..9571b3d7 100644
--- a/pydis_site/apps/api/viewsets/bot/user.py
+++ b/pydis_site/apps/api/viewsets/bot/user.py
@@ -118,4 +118,4 @@ class UserViewSet(BulkCreateModelMixin, ModelViewSet):
     """
 
     serializer_class = UserSerializer
-    queryset = User.objects.prefetch_related('roles')
+    queryset = User.objects
diff --git a/pydis_site/apps/home/signals.py b/pydis_site/apps/home/signals.py
index 4cb4564b..8af48c15 100644
--- a/pydis_site/apps/home/signals.py
+++ b/pydis_site/apps/home/signals.py
@@ -150,7 +150,7 @@ class AllauthSignalListener:
                 social_account = SocialAccount.objects.get(user=user, provider=DiscordProvider.id)
                 discord_user = DiscordUser.objects.get(id=int(social_account.uid))
 
-                mappings = RoleMapping.objects.filter(role__in=discord_user.roles.all()).all()
+                mappings = RoleMapping.objects.filter(role__id__in=discord_user.roles).all()
                 is_staff = any(m.is_staff for m in mappings)
 
                 if user.is_staff != is_staff:
@@ -185,7 +185,7 @@ class AllauthSignalListener:
             self.mapping_model_deleted(RoleMapping, instance=old_instance)
 
         accounts = SocialAccount.objects.filter(
-            uid__in=(u.id for u in instance.role.user_set.all())
+            uid__in=(u.id for u in DiscordUser.objects.filter(roles__contains=[instance.role.id]))
         )
 
         for account in accounts:
@@ -198,7 +198,7 @@ class AllauthSignalListener:
                 discord_user = DiscordUser.objects.get(id=int(account.uid))
 
                 mappings = RoleMapping.objects.filter(
-                    role__in=discord_user.roles.all()
+                    role__id__in=discord_user.roles
                 ).exclude(id=instance.id).all()
                 is_staff = any(m.is_staff for m in mappings)
 
@@ -289,9 +289,9 @@ class AllauthSignalListener:
             new_groups = []
             is_staff = False
 
-            for role in user.roles.all():
+            for role in user.roles:
                 try:
-                    mapping = mappings.get(role=role)
+                    mapping = mappings.get(role__id=role)
                 except RoleMapping.DoesNotExist:
                     continue  # No mapping exists
 
diff --git a/pydis_site/apps/home/tests/test_signal_listener.py b/pydis_site/apps/home/tests/test_signal_listener.py
index fb9a17db..d99d81a5 100644
--- a/pydis_site/apps/home/tests/test_signal_listener.py
+++ b/pydis_site/apps/home/tests/test_signal_listener.py
@@ -89,7 +89,7 @@ class SignalListenerTests(TestCase):
             discriminator=0,
         )
 
-        cls.discord_unmapped.roles.add(cls.unmapped_role)
+        cls.discord_unmapped.roles.append(cls.unmapped_role.id)
         cls.discord_unmapped.save()
 
         cls.discord_not_in_guild = DiscordUser.objects.create(
@@ -105,7 +105,7 @@ class SignalListenerTests(TestCase):
             discriminator=0,
         )
 
-        cls.discord_admin.roles.set([cls.admin_role])
+        cls.discord_admin.roles = [cls.admin_role.id]
         cls.discord_admin.save()
 
         cls.discord_moderator = DiscordUser.objects.create(
@@ -114,7 +114,7 @@ class SignalListenerTests(TestCase):
             discriminator=0,
         )
 
-        cls.discord_moderator.roles.set([cls.moderator_role])
+        cls.discord_moderator.roles = [cls.moderator_role.id]
         cls.discord_moderator.save()
 
         cls.django_user_discordless = DjangoUser.objects.create(username="no-discord")
@@ -333,7 +333,7 @@ class SignalListenerTests(TestCase):
         handler._apply_groups(self.discord_admin, self.social_admin)
         self.assertEqual(self.django_user_discordless.groups.all().count(), 0)
 
-        self.discord_admin.roles.add(self.admin_role)
+        self.discord_admin.roles.append(self.admin_role.id)
         self.discord_admin.save()
 
     def test_apply_groups_moderator(self):
@@ -360,7 +360,7 @@ class SignalListenerTests(TestCase):
         handler._apply_groups(self.discord_moderator, self.social_moderator)
         self.assertEqual(self.django_user_discordless.groups.all().count(), 0)
 
-        self.discord_moderator.roles.add(self.moderator_role)
+        self.discord_moderator.roles.append(self.moderator_role.id)
         self.discord_moderator.save()
 
     def test_apply_groups_other(self):
diff --git a/pydis_site/apps/staff/tests/test_logs_view.py b/pydis_site/apps/staff/tests/test_logs_view.py
index 936d1ad5..17910bb6 100644
--- a/pydis_site/apps/staff/tests/test_logs_view.py
+++ b/pydis_site/apps/staff/tests/test_logs_view.py
@@ -23,7 +23,7 @@ class TestLogsView(TestCase):
             discriminator=1912,
         )
 
-        cls.author.roles.add(cls.developers_role)
+        cls.author.roles.append(cls.developers_role.id)
 
         cls.deletion_context = MessageDeletionContext.objects.create(
             actor=cls.actor,
diff --git a/pydis_site/tests/test_utils_account.py b/pydis_site/tests/test_utils_account.py
index d2946368..e8db7b64 100644
--- a/pydis_site/tests/test_utils_account.py
+++ b/pydis_site/tests/test_utils_account.py
@@ -61,16 +61,18 @@ class AccountUtilsTests(TestCase):
             position=0
         )
 
-        self.discord_user_role.roles.add(everyone_role)
-        self.discord_user_two_roles.roles.add(everyone_role)
+        self.discord_user_role.roles.append(everyone_role.id)
+        self.discord_user_two_roles.roles.append(everyone_role.id)
 
-        self.discord_user_two_roles.roles.add(Role.objects.create(
+        developers_role = Role.objects.create(
             id=1,
             name="Developers",
             colour=0,
             permissions=0,
             position=1
-        ))
+        )
+
+        self.discord_user_two_roles.roles.append(developers_role.id)
 
     def test_account_adapter(self):
         """Test that our Allauth account adapter functions correctly."""
diff --git a/pydis_site/utils/account.py b/pydis_site/utils/account.py
index 2d699c88..b4e41198 100644
--- a/pydis_site/utils/account.py
+++ b/pydis_site/utils/account.py
@@ -54,7 +54,7 @@ class SocialAccountAdapter(DefaultSocialAccountAdapter):
 
             raise ImmediateHttpResponse(redirect(reverse("home")))
 
-        if user.roles.count() <= 1:
+        if len(user.roles) <= 1:
             add_message(request, ERROR, ERROR_JOIN_DISCORD)
 
             raise ImmediateHttpResponse(redirect(reverse("home")))