ref(models): Remove file fk from avatar base model (#26629)

migrations_lockfile.txt

@@ -7,5 +7,5 @@ will then be regenerated, and you should be able to merge without conflicts.
 
 jira_ac: 0001_initial
 nodestore: 0002_nodestore_no_dictfield
-sentry: 0208_add_team_scope
+sentry: 0209_avatar_remove_file_fk
 social_auth: 0001_initial

src/sentry/api/bases/avatar.py

@@ -16,7 +16,7 @@ class AvatarSerializer(serializers.Serializer):
         if attrs.get("avatar_type") == "upload":
             model_type = self.context["type"]
             has_existing_file = model_type.objects.filter(
-                file__isnull=False, **self.context["kwargs"]
+                file_id__isnull=False, **self.context["kwargs"]
             ).exists()
             if not has_existing_file and not attrs.get("avatar_photo"):
                 raise serializers.ValidationError(

src/sentry/api/endpoints/organization_details.py

@@ -270,7 +270,7 @@ class OrganizationSerializer(serializers.Serializer):
         attrs = super().validate(attrs)
         if attrs.get("avatarType") == "upload":
             has_existing_file = OrganizationAvatar.objects.filter(
-                organization=self.context["organization"], file__isnull=False
+                organization=self.context["organization"], file_id__isnull=False
             ).exists()
             if not has_existing_file and not attrs.get("avatar"):
                 raise serializers.ValidationError(

src/sentry/migrations/0209_avatar_remove_file_fk.py

@@ -0,0 +1,78 @@
+# Generated by Django 1.11.29 on 2021-06-17 07:50
+
+import django.db.models.deletion
+from django.db import migrations
+
+import sentry.db.models.fields.bounded
+
+
+class Migration(migrations.Migration):
+    # This flag is used to mark that a migration shouldn't be automatically run in
+    # production. We set this to True for operations that we think are risky and want
+    # someone from ops to run manually and monitor.
+    # General advice is that if in doubt, mark your migration as `is_dangerous`.
+    # Some things you should always mark as dangerous:
+    # - Large data migrations. Typically we want these to be run manually by ops so that
+    #   they can be monitored. Since data migrations will now hold a transaction open
+    #   this is even more important.
+    # - Adding columns to highly active tables, even ones that are NULL.
+    is_dangerous = True
+
+    # This flag is used to decide whether to run this migration in a transaction or not.
+    # By default we prefer to run in a transaction, but for migrations where you want
+    # to `CREATE INDEX CONCURRENTLY` this needs to be set to False. Typically you'll
+    # want to create an index concurrently when adding one to an existing table.
+    # You'll also usually want to set this to `False` if you're writing a data
+    # migration, since we don't want the entire migration to run in one long-running
+    # transaction.
+    atomic = False
+
+    dependencies = [
+        ("sentry", "0208_add_team_scope"),
+    ]
+
+    avatar_models = (
+        "organizationavatar",
+        "projectavatar",
+        "sentryappavatar",
+        "useravatar",
+        "teamavatar",
+    )
+
+    operations = [
+        migrations.SeparateDatabaseAndState(
+            database_operations=[
+                migrations.AlterField(
+                    model_name=model_name,
+                    name="file",
+                    field=sentry.db.models.fields.foreignkey.FlexibleForeignKey(
+                        db_constraint=False,
+                        unique=True,
+                        null=True,
+                        on_delete=django.db.models.deletion.SET_NULL,
+                        to="sentry.File",
+                    ),
+                )
+                for model_name in avatar_models
+            ],
+            state_operations=[
+                *[
+                    migrations.RemoveField(
+                        model_name=model_name,
+                        name="file",
+                    )
+                    for model_name in avatar_models
+                ],
+                *[
+                    migrations.AddField(
+                        model_name=model_name,
+                        name="file_id",
+                        field=sentry.db.models.fields.bounded.BoundedBigIntegerField(
+                            null=True, unique=True
+                        ),
+                    )
+                    for model_name in avatar_models
+                ],
+            ],
+        ),
+    ]

src/sentry/models/avatar.py

@@ -5,7 +5,7 @@ from django.db import models, transaction
 from django.utils.encoding import force_bytes
 from PIL import Image
 
-from sentry.db.models import FlexibleForeignKey, Model
+from sentry.db.models import BoundedBigIntegerField, Model
 from sentry.utils.cache import cache
 
 
@@ -22,7 +22,7 @@ class AvatarBase(Model):
 
     FILE_TYPE = None
 
-    file = FlexibleForeignKey("sentry.File", unique=True, null=True, on_delete=models.SET_NULL)
+    file_id = BoundedBigIntegerField(unique=True, null=True)
     ident = models.CharField(max_length=32, unique=True, db_index=True)
 
     class Meta:
@@ -33,9 +33,24 @@ class AvatarBase(Model):
             self.ident = uuid4().hex
         return super().save(*args, **kwargs)
 
+    def get_file(self):
+        from sentry.models import File
+
+        if self.file_id is None:
+            return None
+
+        try:
+            return File.objects.get(pk=self.file_id)
+        except File.DoesNotExist:
+            # Best effort replication of previous behaviour with foreign key
+            # which was set with on_delete=models.SET_NULL
+            self.update(file_id=None)
+            return None
+
     def delete(self, *args, **kwargs):
-        if self.file:
-            self.file.delete()
+        file = self.get_file()
+        if file:
+            file.delete()
         return super().delete(*args, **kwargs)
 
     def get_cache_key(self, size):
@@ -45,14 +60,15 @@ class AvatarBase(Model):
         cache.delete_many([self.get_cache_key(x) for x in self.ALLOWED_SIZES])
 
     def get_cached_photo(self, size):
-        if not self.file:
+        file = self.get_file()
+        if not file:
             return
         if size not in self.ALLOWED_SIZES:
             size = min(self.ALLOWED_SIZES, key=lambda x: abs(x - size))
         cache_key = self.get_cache_key(size)
         photo = cache.get(cache_key)
         if photo is None:
-            photo_file = self.file.getfile()
+            photo_file = file.getfile()
             with Image.open(photo_file) as image:
                 image = image.resize((size, size), Image.LANCZOS)
                 image_file = BytesIO()
@@ -78,11 +94,12 @@ class AvatarBase(Model):
 
         with transaction.atomic():
             instance, created = cls.objects.get_or_create(**relation)
-            if instance.file and photo:
-                instance.file.delete()
+            file = instance.get_file()
+            if file and photo:
+                file.delete()
 
             if photo:
-                instance.file = photo
+                instance.file_id = photo.id
                 instance.ident = uuid4().hex
 
             instance.avatar_type = [i for i, n in cls.AVATAR_TYPES if n == type][0]

src/sentry/web/frontend/base.py

@@ -11,7 +11,6 @@ from django.template.context_processors import csrf
 from django.urls import reverse
 from django.views.decorators.csrf import csrf_exempt
 from django.views.generic import View
-from sudo.views import redirect_to_sudo
 
 from sentry import roles
 from sentry.api.serializers import serialize
@@ -32,6 +31,7 @@ from sentry.utils import auth
 from sentry.utils.audit import create_audit_entry
 from sentry.web.frontend.generic import FOREVER_CACHE
 from sentry.web.helpers import render_to_response
+from sudo.views import redirect_to_sudo
 
 logger = logging.getLogger(__name__)
 audit_logger = logging.getLogger("sentry.audit.ui")
@@ -548,7 +548,7 @@ class AvatarPhotoView(View):
         except self.model.DoesNotExist:
             return HttpResponseNotFound()
 
-        photo = avatar.file
+        photo = avatar.get_file()
         if not photo:
             return HttpResponseNotFound()
 

tests/sentry/api/endpoints/test_organization_avatar.py

@@ -29,7 +29,7 @@ class OrganizationAvatarPutTest(OrganizationAvatarTestBase):
 
         avatar = OrganizationAvatar.objects.get(organization=self.organization)
         assert avatar.get_avatar_type_display() == "upload"
-        assert avatar.file
+        assert avatar.file_id
 
     def test_put_bad(self):
         OrganizationAvatar.objects.create(organization=self.organization)

tests/sentry/api/endpoints/test_organization_details.py

@@ -214,7 +214,7 @@ class OrganizationUpdateTest(OrganizationDetailsTestBase):
 
         avatar = OrganizationAvatar.objects.get(organization=self.organization)
         assert avatar.get_avatar_type_display() == "upload"
-        assert avatar.file
+        assert avatar.file_id
 
     def test_various_options(self):
         initial = self.organization.get_audit_log_data()

tests/sentry/api/endpoints/test_project_avatar.py

@@ -39,7 +39,7 @@ class ProjectAvatarTest(APITestCase):
         avatar = ProjectAvatar.objects.get(project=project)
         assert response.status_code == 200, response.content
         assert avatar.get_avatar_type_display() == "upload"
-        assert avatar.file
+        assert avatar.file_id
 
     def test_put_bad(self):
         project = self.project  # force creation

tests/sentry/api/endpoints/test_team_avatar.py

@@ -39,7 +39,7 @@ class TeamAvatarTest(APITestCase):
         avatar = TeamAvatar.objects.get(team=team)
         assert response.status_code == 200, response.content
         assert avatar.get_avatar_type_display() == "upload"
-        assert avatar.file
+        assert avatar.file_id
 
     def test_put_bad(self):
         team = self.team  # force creation