Convert ReusedGUID to AddonGUID, containing fks for all Addons (#14406)

* Convert ReusedGUID to AddonGUID, containing fks for all Addons

* add index to AddonGUID.guid

* take into account Addon.guid won't be None for deleted addons

src/olympia/addons/migrations/0005_auto_20200527_0513.py

@@ -0,0 +1,42 @@
+# Generated by Django 2.2.12 on 2020-05-27 05:13
+from django.db import migrations
+
+from olympia.amo.utils import chunked
+
+
+def add_all_addon_guids_to_addon_guid(apps, schema_editor):
+    AddonGUID = apps.get_model('addons', 'AddonGUID')
+    Addon = apps.get_model('addons', 'Addon')
+
+    batch_size = 1000
+    existing_ids = AddonGUID.objects.all().values_list('addon_id', flat=True)
+    addon_qs = Addon.unfiltered.exclude(
+        guid=None, id__in=existing_ids).values('id', 'guid')
+    for addons in chunked(addon_qs, batch_size):
+        addonguids = [
+            AddonGUID(
+                addon_id=addon['id'], guid=addon['guid']) for addon in addons]
+        AddonGUID.objects.bulk_create(addonguids, batch_size)
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('addons', '0004_auto_20191126_1712'),
+    ]
+
+    operations = [
+        migrations.AlterModelTable(
+            name='ReusedGUID',
+            table='addons_reusedguid',
+        ),
+        migrations.RenameModel(
+            old_name='ReusedGUID',
+            new_name='AddonGUID',
+        ),
+        migrations.AlterModelOptions(
+            name='addonguid',
+            options={},
+        ),
+        migrations.RunPython(add_all_addon_guids_to_addon_guid),
+    ]

src/olympia/addons/migrations/0006_auto_20200527_0843.py

@@ -0,0 +1,18 @@
+# Generated by Django 2.2.12 on 2020-05-27 08:43
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('addons', '0005_auto_20200527_0513'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='addonguid',
+            name='guid',
+            field=models.CharField(db_index=True, max_length=255),
+        ),
+    ]

src/olympia/addons/models.py

@@ -715,9 +715,10 @@ def initialize_addon_from_upload(cls, data, upload, channel, user):
         addon.save()
         timer.log_interval('6.addon_save')
 
+        if guid:
+            AddonGUID.objects.create(addon=addon, guid=guid)
         if old_guid_addon:
             old_guid_addon.update(guid=GUID_REUSE_FORMAT.format(addon.pk))
-            ReusedGUID.objects.create(addon=old_guid_addon, guid=guid)
             log.info(f'GUID {guid} from addon [{old_guid_addon.pk}] reused '
                      f'by addon [{addon.pk}].')
         if user:
@@ -2109,11 +2110,15 @@ def track_addon_status_change(addon):
                 .format(addon.status))
 
 
-class ReusedGUID(ModelBase):
+class AddonGUID(ModelBase):
     """
-    Addons + guids will be added to this table when a new Add-on has reused
-    the guid from an earlier deleted Add-on.
+    Addons + guids will be added to this table whenever an addon is created.
+    For deleted addons it will contain an fk to the Addon instance even after
+    Addon.guid has been set to null (i.e. when it's been reuploaded).
     """
-    guid = models.CharField(max_length=255, null=False)
+    guid = models.CharField(max_length=255, null=False, db_index=True)
     addon = models.OneToOneField(
         Addon, null=False, on_delete=models.CASCADE, unique=True)
+
+    class Meta:
+        db_table = 'addons_reusedguid'

src/olympia/addons/tests/test_commands.py

@@ -11,7 +11,7 @@
 
 from olympia import amo
 from olympia.addons.management.commands import process_addons
-from olympia.addons.models import Addon, ReusedGUID, GUID_REUSE_FORMAT
+from olympia.addons.models import Addon
 from olympia.abuse.models import AbuseReport
 from olympia.amo.tests import (
     TestCase, addon_factory, user_factory, version_factory)
@@ -476,54 +476,6 @@ def test_basic(self, sign_file_mock):
         assert sign_file_mock.call_count == 5
 
 
-@pytest.mark.django_db
-def test_backfill_reused_guid():
-    # shouldn't show up in the query but throw them in anyway
-    addon_factory(name='just a deleted addon', status=amo.STATUS_DELETED)
-    addon_factory(name='just a public addon')
-    # simple case - an add-on's guid is reused once.
-    single_reuse_deleted = addon_factory(
-        name='single reuse', status=amo.STATUS_DELETED)
-    single_reuse_addon = addon_factory(
-        name='single reuse', guid='single@reuse')
-    single_reuse_deleted.update(
-        guid=GUID_REUSE_FORMAT.format(single_reuse_addon.id))
-    # more complex case - a guid is reused multiple times.
-    multi_reuse_deleted_a = addon_factory(
-        name='multi reuse', status=amo.STATUS_DELETED)
-    multi_reuse_deleted_b = addon_factory(
-        name='multi reuse', status=amo.STATUS_DELETED)
-    multi_reuse_addon = addon_factory(
-        name='multi reuse', guid='multi@reuse')
-    multi_reuse_deleted_a.update(
-        guid=GUID_REUSE_FORMAT.format(multi_reuse_deleted_b.id))
-    multi_reuse_deleted_b.update(
-        guid=GUID_REUSE_FORMAT.format(multi_reuse_addon.id))
-    # a guid reuse referencing a pk that doesn't exist (addon hard delete?)
-    addon_factory(
-        name='missing reuse', status=amo.STATUS_DELETED,
-        guid=GUID_REUSE_FORMAT.format(999))
-    # reusedguid object already there
-    reused_exists_deleted = addon_factory(
-        name='reused_exists', status=amo.STATUS_DELETED)
-    reused_exists_addon = addon_factory(
-        name='reused_exists', guid='exists@reuse')
-    reused_exists_deleted.update(
-        guid=GUID_REUSE_FORMAT.format(reused_exists_addon.id))
-    ReusedGUID.objects.create(addon=reused_exists_deleted, guid='exists@reuse')
-
-    assert ReusedGUID.objects.count() == 1
-    call_command('backfill_reused_guid')
-    assert ReusedGUID.objects.count() == 4
-    qs_values = ReusedGUID.objects.all().order_by('id').values('addon', 'guid')
-    assert list(qs_values) == [
-        {'addon': reused_exists_deleted.id, 'guid': 'exists@reuse'},
-        {'addon': single_reuse_deleted.id, 'guid': 'single@reuse'},
-        {'addon': multi_reuse_deleted_a.id, 'guid': 'multi@reuse'},
-        {'addon': multi_reuse_deleted_b.id, 'guid': 'multi@reuse'},
-    ]
-
-
 class TestDeleteObsoleteAddons(TestCase):
     def setUp(self):
         # Some add-ons that shouldn't be deleted

src/olympia/addons/tests/test_models.py

@@ -15,7 +15,7 @@
 from olympia.addons.models import (
     Addon, AddonApprovalsCounter, AddonCategory, AddonReviewerFlags, AddonUser,
     AppSupport, Category, DeniedGuid, DeniedSlug, FrozenAddon, MigratedLWT,
-    Preview, ReusedGUID, track_addon_status_change)
+    Preview, AddonGUID, track_addon_status_change)
 from olympia.amo.templatetags.jinja_helpers import absolutify
 from olympia.amo.tests import (
     TestCase, addon_factory, user_factory, version_factory)
@@ -2220,6 +2220,7 @@ def test_existing_guid_same_author(self):
         parsed_data = parse_addon(self.upload, user=self.user)
         deleted = Addon.from_upload(self.upload, [self.selected_app],
                                     parsed_data=parsed_data)
+        assert AddonGUID.objects.filter(guid='guid@xpi').count() == 1
         # Claim the add-on.
         AddonUser(addon=deleted, user=self.user).save()
         deleted.update(status=amo.STATUS_APPROVED)
@@ -2235,8 +2236,8 @@ def test_existing_guid_same_author(self):
         deleted.reload()
         assert addon.guid == 'guid@xpi'
         assert deleted.guid == 'guid-reused-by-pk-%s' % addon.pk
-        assert ReusedGUID.objects.filter(guid='guid@xpi').count() == 1
-        assert ReusedGUID.objects.filter(guid='guid@xpi').last().addon == (
+        assert AddonGUID.objects.filter(guid='guid@xpi').count() == 2
+        assert AddonGUID.objects.filter(guid='guid@xpi').first().addon == (
             deleted)
 
     def test_old_soft_deleted_addons_and_upload_non_extension(self):

src/olympia/amo/tests/__init__.py

@@ -43,7 +43,7 @@
 from olympia.accounts.utils import fxa_login_url
 from olympia.addons.indexers import AddonIndexer
 from olympia.addons.models import (
-    Addon, AddonCategory, Category,
+    Addon, AddonCategory, AddonGUID, Category,
     update_search_index as addon_update_search_index)
 from olympia.amo.urlresolvers import get_url_prefix, Prefixer, set_url_prefix
 from olympia.amo.storage_utils import copy_stored_file
@@ -736,6 +736,8 @@ def addon_factory(
 
     # Save 4.
     addon.save()
+    if addon.guid:
+        AddonGUID.objects.create(addon=addon, guid=addon.guid)
 
     # Potentially update is_public on authors
     [user.update_is_public() for user in users]

src/olympia/blocklist/mlbf.py

@@ -5,7 +5,6 @@
 
 from django.conf import settings
 from django.core.files.storage import default_storage as storage
-from django.db.models import Q
 from django.utils.functional import cached_property
 
 from filtercascade import FilterCascade
@@ -34,20 +33,16 @@ def fetch_blocked_from_db(cls):
         blocks_guids = [block.guid for block in blocks]
 
         file_qs = File.objects.filter(
-            Q(version__addon__guid__in=blocks_guids) |
-            Q(version__addon__reusedguid__guid__in=blocks_guids),
+            version__addon__addonguid__guid__in=blocks_guids,
             is_signed=True,
             is_webextension=True,
         ).order_by('version_id').values(
-            'version__addon__guid',
-            'version__addon__reusedguid__guid',
+            'version__addon__addonguid__guid',
             'version__version',
             'version_id')
         addons_versions = defaultdict(list)
         for file_ in file_qs:
-            addon_key = (
-                file_['version__addon__reusedguid__guid'] or
-                file_['version__addon__guid'])
+            addon_key = file_['version__addon__addonguid__guid']
             addons_versions[addon_key].append(
                 (file_['version__version'], file_['version_id']))
 
@@ -70,12 +65,8 @@ def fetch_all_versions_from_db(cls, excluding_version_ids=None):
 
         qs = (
             Version.unfiltered.exclude(id__in=excluding_version_ids or ())
-                   .values(
-                       'addon__guid', 'addon__reusedguid__guid', 'version'))
-        return list(
-            (version['addon__reusedguid__guid'] or version['addon__guid'],
-             version['version'])
-            for version in qs)
+                   .values_list('addon__addonguid__guid', 'version'))
+        return list(qs)
 
     @classmethod
     def hash_filter_inputs(cls, input_list):

src/olympia/blocklist/tests/test_mlbf.py

@@ -6,7 +6,7 @@
 from filtercascade import FilterCascade
 
 from olympia import amo
-from olympia.addons.models import GUID_REUSE_FORMAT, ReusedGUID
+from olympia.addons.models import GUID_REUSE_FORMAT
 from olympia.amo.tests import (
     addon_factory, TestCase, user_factory, version_factory)
 from olympia.blocklist.models import Block
@@ -71,6 +71,18 @@ def setup_data(self):
             min_version='9999')
 
         # A blocked addon has been uploaded and deleted before
+        reused_2_1_addon = addon_factory(
+            # this a previous, superceeded, addon that should be included
+            status=amo.STATUS_DELETED,  # they should all be deleted
+            version_kw={'version': '2.1'},
+            file_kw={'is_signed': True, 'is_webextension': True})
+        self.addon_deleted_before_2_1_ver = reused_2_1_addon.versions.all()[0]
+        reused_2_5_addon = addon_factory(
+            # And this is an earlier addon that should also be included
+            status=amo.STATUS_DELETED,  # they should all be deleted
+            version_kw={'version': '2.5'},
+            file_kw={'is_signed': True, 'is_webextension': True})
+        self.addon_deleted_before_2_5_ver = reused_2_5_addon.versions.all()[0]
         current_addon = addon_factory(
             version_kw={'version': '2'},
             file_kw={'is_signed': True, 'is_webextension': True})
@@ -82,22 +94,12 @@ def setup_data(self):
         version_factory(
             addon=current_addon, version='3.0',
             file_kw={'is_signed': True, 'is_webextension': True})
-        reused_2_1 = ReusedGUID.objects.create(
-            # this a previous, superceeded, addon that should be included
-            guid=current_addon.guid, addon=addon_factory(
-                guid=GUID_REUSE_FORMAT.format(current_addon.id),
-                version_kw={'version': '2.1'},
-                file_kw={'is_signed': True, 'is_webextension': True}))
-        self.addon_deleted_before_2_1_ver = reused_2_1.addon.versions.all()[0]
-        reused_2_5 = ReusedGUID.objects.create(
-            # And this is an earlier addon that should also be included
-            guid=current_addon.guid, addon=addon_factory(
-                guid=GUID_REUSE_FORMAT.format(
-                    self.addon_deleted_before_2_1_ver.addon_id),
-                status=amo.STATUS_DELETED,  # they should all be deleted tbf
-                version_kw={'version': '2.5'},
-                file_kw={'is_signed': True, 'is_webextension': True}))
-        self.addon_deleted_before_2_5_ver = reused_2_5.addon.versions.all()[0]
+        reused_2_1_addon.update(
+            guid=GUID_REUSE_FORMAT.format(current_addon.id))
+        reused_2_5_addon.update(
+            guid=GUID_REUSE_FORMAT.format(reused_2_1_addon.id))
+        reused_2_1_addon.addonguid.update(guid=current_addon.guid)
+        reused_2_5_addon.addonguid.update(guid=current_addon.guid)
         self.addon_deleted_before_block = Block.objects.create(
             guid=current_addon.guid, min_version='2.0.1', updated_by=user)
 

src/olympia/reviewers/tests/test_views.py

@@ -31,8 +31,7 @@
 from olympia.accounts.serializers import BaseUserSerializer
 from olympia.activity.models import ActivityLog, DraftComment
 from olympia.addons.models import (
-    Addon, AddonApprovalsCounter, AddonReviewerFlags, AddonUser, DeniedGuid,
-    ReusedGUID)
+    Addon, AddonApprovalsCounter, AddonReviewerFlags, AddonUser, DeniedGuid)
 from olympia.amo.storage_utils import copy_stored_file
 from olympia.amo.templatetags.jinja_helpers import (
     absolutify, format_date, format_datetime)
@@ -5188,10 +5187,10 @@ def test_reused_guid_from_previous_deleted_addon(self):
         old_two = addon_factory(status=amo.STATUS_DELETED)
         old_other = addon_factory(status=amo.STATUS_DELETED)
         old_noguid = addon_factory(status=amo.STATUS_DELETED)
-        ReusedGUID.objects.create(addon=old_one, guid='reuse@')
-        ReusedGUID.objects.create(addon=old_two, guid='reuse@')
-        ReusedGUID.objects.create(addon=old_other, guid='other@')
-        ReusedGUID.objects.create(addon=old_noguid, guid='')
+        old_one.addonguid.update(guid='reuse@')
+        old_two.addonguid.update(guid='reuse@')
+        old_other.addonguid.update(guid='other@')
+        old_noguid.addonguid.update(guid='')
         self.addon.update(guid='reuse@')
 
         response = self.client.get(self.url)