Stephen Finucane <step...@that.guru> writes: > Oh, the follies of youth. Time to undo the damage of 2.0.0, specifically > commit 86172ccc16, which split Patch into two separate models using > concrete inheritance. As noted previously, this introduced a large > number of unavoidable JOINs across large tables and the performance > impacts these introduce are blocking other features we want, such as > improved tagging functionality. To combine these two models, we must do > the following: > > - Update any references to the 'Patch' model to point to the > 'Submission' model instead > - Move everything from 'Patch' to 'Submission', including both fields > and options > - Delete the 'Patch' model > - Rename the 'Submission' model to 'Patch' > > With this change, our model "hierarchy" goes from: > > Submission > Patch > PatchComment > Cover > CoverComment > > To a nice, flat: > > Patch > PatchComment > Cover > CoverComment > > I expect this migration to be intensive, particularly for MySQL users > who will see their entire tables rewritten. Unfortunately I don't see > any way to resolve this in an easier manner. > > Also note that we have to use two migrations and separate the moving of > fields from the renaming of models, in order to work around a bug in > Django 1.11. This bug has been fixed since Django 2.0 [1][2] but the fix > wasn't backported and Django 1.11 is only accepting security fixes at > this point in time. > > [1] https://code.djangoproject.com/ticket/25530 > [2] https://code.djangoproject.com/ticket/31337 > > Signed-off-by: Stephen Finucane <step...@that.guru> > Cc: Daniel Axtens <d...@axtens.net> > Cc: Stewart Smith <stew...@linux.ibm.com> > --- > TODO: > - Figure out if the indexes are correct (Stewart?). I've just included > everything that's displayed on the '/list' page.
You might have more luck with stew...@flamingspork.com now that Stewart has ascended to the cloud. Regards, Daniel > - If this isn't merged until 3.0, combine the migrations since we will > no longer have to support Django 1.11. > --- > patchwork/api/comment.py | 4 +- > patchwork/management/commands/dumparchive.py | 2 +- > .../0041_merge_patch_submission_a.py | 281 ++++++++++++++++++ > .../0042_merge_patch_submission_b.py | 17 ++ > patchwork/models.py | 94 +++--- > patchwork/parser.py | 16 +- > patchwork/tests/test_mboxviews.py | 26 +- > patchwork/tests/utils.py | 5 +- > patchwork/views/__init__.py | 2 +- > patchwork/views/utils.py | 4 +- > 10 files changed, 376 insertions(+), 75 deletions(-) > create mode 100644 patchwork/migrations/0041_merge_patch_submission_a.py > create mode 100644 patchwork/migrations/0042_merge_patch_submission_b.py > > diff --git a/patchwork/api/comment.py b/patchwork/api/comment.py > index 3802dab9..43b26c61 100644 > --- a/patchwork/api/comment.py > +++ b/patchwork/api/comment.py > @@ -14,7 +14,7 @@ from patchwork.api.base import PatchworkPermission > from patchwork.api.embedded import PersonSerializer > from patchwork.models import Cover > from patchwork.models import CoverComment > -from patchwork.models import Submission > +from patchwork.models import Patch > from patchwork.models import PatchComment > > > @@ -105,7 +105,7 @@ class PatchCommentList(ListAPIView): > lookup_url_kwarg = 'pk' > > def get_queryset(self): > - if not Submission.objects.filter(pk=self.kwargs['pk']).exists(): > + if not Patch.objects.filter(pk=self.kwargs['pk']).exists(): > raise Http404 > > return PatchComment.objects.filter( > diff --git a/patchwork/management/commands/dumparchive.py > b/patchwork/management/commands/dumparchive.py > index 9ee80c8b..e9445eab 100644 > --- a/patchwork/management/commands/dumparchive.py > +++ b/patchwork/management/commands/dumparchive.py > @@ -58,7 +58,7 @@ class Command(BaseCommand): > i + 1, len(projects), project.linkname)) > > with tempfile.NamedTemporaryFile(delete=False) as mbox: > - patches = Patch.objects.filter(patch_project=project) > + patches = Patch.objects.filter(project=project) > count = patches.count() > for j, patch in enumerate(patches): > if not (j % 10): > diff --git a/patchwork/migrations/0041_merge_patch_submission_a.py > b/patchwork/migrations/0041_merge_patch_submission_a.py > new file mode 100644 > index 00000000..80b8dc2f > --- /dev/null > +++ b/patchwork/migrations/0041_merge_patch_submission_a.py > @@ -0,0 +1,281 @@ > +# -*- coding: utf-8 -*- > +from __future__ import unicode_literals > + > +from django.conf import settings > +from django.db import connection, migrations, models > +import django.db.models.deletion > + > +import patchwork.fields > + > + > +def migrate_data(apps, schema_editor): > + if connection.vendor == 'postgresql': > + schema_editor.execute( > + """ > + UPDATE patchwork_submission > + SET archived = patchwork_patch.archived2, > + commit_ref = patchwork_patch.commit_ref2, > + delegate_id = patchwork_patch.delegate2_id, > + diff = patchwork_patch.diff2, > + hash = patchwork_patch.hash2, > + number = patchwork_patch.number2, > + pull_url = patchwork_patch.pull_url2, > + series_id = patchwork_patch.series2_id, > + state_id = patchwork_patch.state2_id, > + FROM patchwork_patch > + WHERE patchwork_submission.id = > patchwork_patch.submission_ptr_id > + """ > + ) > + elif connection.vendor == 'mysql': > + schema_editor.execute( > + """ > + UPDATE patchwork_submission, patchwork_patch > + SET patchwork_submission.archived = patchwork_patch.archived2, > + patchwork_submission.commit_ref = > patchwork_patch.commit_ref2, > + patchwork_submission.delegate_id = > patchwork_patch.delegate2_id, > + patchwork_submission.diff = patchwork_patch.diff2, > + patchwork_submission.hash = patchwork_patch.hash2, > + patchwork_submission.number = patchwork_patch.number2, > + patchwork_submission.pull_url = patchwork_patch.pull_url2, > + patchwork_submission.series_id = > patchwork_patch.series2_id, > + patchwork_submission.state_id = patchwork_patch.state2_id > + WHERE patchwork_submission.id = patchwork_patch.submission_ptr_id > + """ # noqa > + ) > + else: > + raise Exception('DB not supported') > + > + > +class Migration(migrations.Migration): > + > + dependencies = [ > + ('patchwork', '0040_add_cover_model'), > + ] > + > + operations = [ > + # move the 'PatchTag' model to point to 'Submission' > + > + migrations.RemoveField(model_name='patch', name='tags',), > + migrations.AddField( > + model_name='submission', > + name='tags', > + field=models.ManyToManyField( > + through='patchwork.PatchTag', to='patchwork.Tag' > + ), > + ), > + migrations.AlterField( > + model_name='patchtag', > + name='patch', > + field=models.ForeignKey( > + on_delete=django.db.models.deletion.CASCADE, > + to='patchwork.Submission', > + ), > + ), > + > + # do the same for any other field that references 'Patch' > + > + migrations.AlterField( > + model_name='bundle', > + name='patches', > + field=models.ManyToManyField( > + through='patchwork.BundlePatch', to='patchwork.Submission' > + ), > + ), > + migrations.AlterField( > + model_name='bundlepatch', > + name='patch', > + field=models.ForeignKey( > + on_delete=django.db.models.deletion.CASCADE, > + to='patchwork.Submission', > + ), > + ), > + migrations.AlterField( > + model_name='check', > + name='patch', > + field=models.ForeignKey( > + on_delete=django.db.models.deletion.CASCADE, > + to='patchwork.Submission', > + ), > + ), > + migrations.AlterField( > + model_name='event', > + name='patch', > + field=models.ForeignKey( > + blank=True, > + help_text=b'The patch that this event was created for.', > + null=True, > + on_delete=django.db.models.deletion.CASCADE, > + related_name='+', > + to='patchwork.Submission', > + ), > + ), > + migrations.AlterField( > + model_name='patchchangenotification', > + name='patch', > + field=models.OneToOneField( > + on_delete=django.db.models.deletion.CASCADE, > + primary_key=True, > + serialize=False, > + to='patchwork.Submission', > + ), > + ), > + > + # rename all the fields on 'Patch' so we don't have duplicates when > we > + # add them to 'Submission' > + > + migrations.RemoveIndex( > + model_name='patch', name='patch_list_covering_idx', > + ), > + migrations.AlterUniqueTogether(name='patch', > unique_together=set([]),), > + migrations.RenameField( > + model_name='patch', old_name='archived', new_name='archived2', > + ), > + migrations.RenameField( > + model_name='patch', old_name='commit_ref', > new_name='commit_ref2', > + ), > + migrations.RenameField( > + model_name='patch', old_name='delegate', new_name='delegate2', > + ), > + migrations.RenameField( > + model_name='patch', old_name='diff', new_name='diff2', > + ), > + migrations.RenameField( > + model_name='patch', old_name='hash', new_name='hash2', > + ), > + migrations.RenameField( > + model_name='patch', old_name='number', new_name='number2', > + ), > + migrations.RenameField( > + model_name='patch', old_name='pull_url', new_name='pull_url2', > + ), > + migrations.RenameField( > + model_name='patch', old_name='series', new_name='series2', > + ), > + migrations.RenameField( > + model_name='patch', old_name='state', new_name='state2', > + ), > + > + # add the fields found on 'Patch' to 'Submission' > + > + migrations.AddField( > + model_name='submission', > + name='archived', > + field=models.BooleanField(default=False), > + ), > + migrations.AddField( > + model_name='submission', > + name='commit_ref', > + field=models.CharField(blank=True, max_length=255, null=True), > + ), > + migrations.AddField( > + model_name='submission', > + name='delegate', > + field=models.ForeignKey( > + blank=True, > + null=True, > + on_delete=django.db.models.deletion.CASCADE, > + to=settings.AUTH_USER_MODEL, > + ), > + ), > + migrations.AddField( > + model_name='submission', > + name='diff', > + field=models.TextField(blank=True, null=True), > + ), > + migrations.AddField( > + model_name='submission', > + name='hash', > + field=patchwork.fields.HashField( > + blank=True, max_length=40, null=True > + ), > + ), > + migrations.AddField( > + model_name='submission', > + name='number', > + field=models.PositiveSmallIntegerField( > + default=None, > + help_text=b'The number assigned to this patch in the series', > + null=True, > + ), > + ), > + migrations.AddField( > + model_name='submission', > + name='pull_url', > + field=models.CharField(blank=True, max_length=255, null=True), > + ), > + migrations.AddField( > + model_name='submission', > + name='series', > + field=models.ForeignKey( > + blank=True, > + null=True, > + on_delete=django.db.models.deletion.CASCADE, > + related_name='patches', > + related_query_name='patch', > + to='patchwork.Series', > + ), > + ), > + migrations.AddField( > + model_name='submission', > + name='state', > + field=models.ForeignKey( > + null=True, > + on_delete=django.db.models.deletion.CASCADE, > + to='patchwork.State', > + ), > + ), > + > + # copy the data from 'Patch' to 'Submission' > + > + migrations.RunPython(migrate_data, None, atomic=False), > + > + # configure metadata for the 'Submission' model > + > + migrations.AlterModelOptions( > + name='submission', > + options={ > + 'base_manager_name': 'objects', > + 'ordering': ['date'], > + 'verbose_name_plural': 'Patches', > + }, > + ), > + migrations.AlterUniqueTogether( > + name='submission', > + unique_together=set([('series', 'number'), ('msgid', > 'project')]), > + ), > + migrations.RemoveIndex( > + model_name='submission', name='submission_covering_idx', > + ), > + migrations.AddIndex( > + model_name='submission', > + index=models.Index( > + fields=[ > + b'archived', > + b'state', > + b'delegate', > + b'date', > + b'project', > + b'submitter', > + b'name', > + ], > + name=b'patch_covering_idx', > + ), > + ), > + > + # remove the foreign key fields from the 'Patch' model > + > + migrations.RemoveField(model_name='patch', name='delegate2',), > + migrations.RemoveField(model_name='patch', name='patch_project',), > + migrations.RemoveField(model_name='patch', name='series2',), > + migrations.RemoveField(model_name='patch', name='state2',), > + migrations.RemoveField(model_name='patch', name='submission_ptr',), > + > + # drop the 'Patch' model and rename 'Submission' to 'Patch'; this is > + # done in a seperate migration to work around bug #31337 for anyone > + # still using Django 1.11 > + # > + # https://code.djangoproject.com/ticket/3133 > + > + # migrations.DeleteModel(name='Patch',), > + # migrations.RenameModel(old_name='Submission', new_name='Patch',), > + ] > diff --git a/patchwork/migrations/0042_merge_patch_submission_b.py > b/patchwork/migrations/0042_merge_patch_submission_b.py > new file mode 100644 > index 00000000..0051ce87 > --- /dev/null > +++ b/patchwork/migrations/0042_merge_patch_submission_b.py > @@ -0,0 +1,17 @@ > +# -*- coding: utf-8 -*- > + > +from __future__ import unicode_literals > + > +from django.db import migrations > + > + > +class Migration(migrations.Migration): > + > + dependencies = [ > + ('patchwork', '0041_merge_patch_submission_a'), > + ] > + > + operations = [ > + migrations.DeleteModel(name='Patch',), > + migrations.RenameModel(old_name='Submission', new_name='Patch',), > + ] > diff --git a/patchwork/models.py b/patchwork/models.py > index 37cbce32..c7b95de4 100644 > --- a/patchwork/models.py > +++ b/patchwork/models.py > @@ -169,7 +169,7 @@ class UserProfile(models.Model): > @property > def contributor_projects(self): > submitters = Person.objects.filter(user=self.user) > - return Project.objects.filter(id__in=Submission.objects.filter( > + return Project.objects.filter(id__in=Patch.objects.filter( > submitter__in=submitters).values('project_id').query) > > @property > @@ -292,8 +292,7 @@ class PatchQuerySet(models.query.QuerySet): > select[tag.attr_name] = ( > "coalesce(" > "(SELECT count FROM patchwork_patchtag" > - " WHERE patchwork_patchtag.patch_id=" > - "patchwork_patch.submission_ptr_id" > + " WHERE patchwork_patchtag.patch_id=patchwork_patch.id" > " AND patchwork_patchtag.tag_id=%s), 0)") > select_params.append(tag.id) > > @@ -424,24 +423,8 @@ class Cover(FilenameMixin, EmailMixin, SubmissionMixin): > ] > > > -class Submission(SubmissionMixin, FilenameMixin, EmailMixin): > - > - class Meta: > - ordering = ['date'] > - unique_together = [('msgid', 'project')] > - indexes = [ > - # This is a covering index for the /list/ query > - # Like what we have for Patch, but used for displaying what we > want > - # rather than for working out the count (of course, this all > - # depends on the SQL optimiser of your db engine) > - models.Index(fields=['date', 'project', 'submitter', 'name'], > - name='submission_covering_idx'), > - ] > - > - > @python_2_unicode_compatible > -class Patch(Submission): > - # patch metadata > +class Patch(FilenameMixin, EmailMixin, SubmissionMixin): > > diff = models.TextField(null=True, blank=True) > commit_ref = models.CharField(max_length=255, null=True, blank=True) > @@ -450,24 +433,31 @@ class Patch(Submission): > > # patchwork metadata > > - delegate = models.ForeignKey(User, blank=True, null=True, > - on_delete=models.CASCADE) > + delegate = models.ForeignKey( > + User, > + blank=True, > + null=True, > + on_delete=models.CASCADE, > + ) > state = models.ForeignKey(State, null=True, on_delete=models.CASCADE) > archived = models.BooleanField(default=False) > hash = HashField(null=True, blank=True) > > - # duplicate project from submission in subclass so we can count the > - # patches in a project without needing to do a JOIN. > - patch_project = models.ForeignKey(Project, on_delete=models.CASCADE) > - > # series metadata > > series = models.ForeignKey( > - 'Series', null=True, blank=True, on_delete=models.CASCADE, > - related_name='patches', related_query_name='patch') > + 'Series', > + null=True, > + blank=True, > + on_delete=models.CASCADE, > + related_name='patches', > + related_query_name='patch', > + ) > number = models.PositiveSmallIntegerField( > - default=None, null=True, > - help_text='The number assigned to this patch in the series') > + default=None, > + null=True, > + help_text='The number assigned to this patch in the series', > + ) > > objects = PatchManager() > > @@ -632,14 +622,23 @@ class Patch(Submission): > > class Meta: > verbose_name_plural = 'Patches' > + ordering = ['date'] > base_manager_name = 'objects' > - unique_together = [('series', 'number')] > - > + unique_together = [('msgid', 'project'), ('series', 'number')] > indexes = [ > # This is a covering index for the /list/ query > - models.Index(fields=['archived', 'patch_project', 'state', > - 'delegate'], > - name='patch_list_covering_idx'), > + models.Index( > + fields=[ > + 'archived', > + 'state', > + 'delegate', > + 'date', > + 'project', > + 'submitter', > + 'name', > + ], > + name='patch_covering_idx', > + ), > ] > > > @@ -678,7 +677,7 @@ class PatchComment(EmailMixin, models.Model): > # parent > > patch = models.ForeignKey( > - Submission, > + Patch, > related_name='comments', > related_query_name='comment', > on_delete=models.CASCADE, > @@ -698,15 +697,11 @@ class PatchComment(EmailMixin, models.Model): > > def save(self, *args, **kwargs): > super(PatchComment, self).save(*args, **kwargs) > - # TODO(stephenfin): Update this once patch is flattened > - if hasattr(self.patch, 'patch'): > - self.patch.patch.refresh_tag_counts() > + self.patch.refresh_tag_counts() > > def delete(self, *args, **kwargs): > super(PatchComment, self).delete(*args, **kwargs) > - # TODO(stephenfin): Update this once patch is flattened > - if hasattr(self.patch, 'patch'): > - self.patch.patch.refresh_tag_counts() > + self.patch.refresh_tag_counts() > > def is_editable(self, user): > return False > @@ -749,10 +744,10 @@ class Series(FilenameMixin, models.Model): > > @staticmethod > def _format_name(obj): > - # The parser ensure 'Submission.name' will always take the form > - # 'subject' or '[prefix_a,prefix_b,...] subject'. There will never be > - # multiple prefixes (text inside brackets), thus, we don't need to > - # account for multiple prefixes here. > + # The parser ensure 'Cover.name' will always take the form 'subject' > or > + # '[prefix_a,prefix_b,...] subject'. There will never be multiple > + # prefixes (text inside brackets), thus, we don't need to account for > + # multiple prefixes here. > prefix_re = re.compile(r'^\[([^\]]*)\]\s*(.*)$') > match = prefix_re.match(obj.name) > if match: > @@ -1118,7 +1113,10 @@ class EmailOptout(models.Model): > > > class PatchChangeNotification(models.Model): > - patch = models.OneToOneField(Patch, primary_key=True, > - on_delete=models.CASCADE) > + patch = models.OneToOneField( > + Patch, > + primary_key=True, > + on_delete=models.CASCADE, > + ) > last_modified = models.DateTimeField(default=datetime.datetime.utcnow) > orig_state = models.ForeignKey(State, on_delete=models.CASCADE) > diff --git a/patchwork/parser.py b/patchwork/parser.py > index cd82f3be..69cfb63a 100644 > --- a/patchwork/parser.py > +++ b/patchwork/parser.py > @@ -30,7 +30,6 @@ from patchwork.models import Project > from patchwork.models import Series > from patchwork.models import SeriesReference > from patchwork.models import State > -from patchwork.models import Submission > > > _hunk_re = re.compile(r'^\@\@ -\d+(?:,(\d+))? \+\d+(?:,(\d+))? \@\@') > @@ -647,14 +646,14 @@ def find_comment_content(mail): > return None, commentbuf > > > -def find_submission_for_comment(project, refs): > +def find_patch_for_comment(project, refs): > for ref in refs: > ref = ref[:255] > # first, check for a direct reply > try: > - submission = Submission.objects.get(project=project, msgid=ref) > - return submission > - except Submission.DoesNotExist: > + patch = Patch.objects.get(project=project, msgid=ref) > + return patch > + except Patch.DoesNotExist: > pass > > # see if we have comments that refer to a patch > @@ -1099,7 +1098,6 @@ def parse_mail(mail, list_id=None): > patch = Patch.objects.create( > msgid=msgid, > project=project, > - patch_project=project, > name=name[:255], > date=date, > headers=headers, > @@ -1273,13 +1271,13 @@ def parse_mail(mail, list_id=None): > # comments > > # we only save comments if we have the parent email > - submission = find_submission_for_comment(project, refs) > - if submission: > + patch = find_patch_for_comment(project, refs) > + if patch: > author = get_or_create_author(mail, project) > > try: > comment = PatchComment.objects.create( > - patch=submission, > + patch=patch, > msgid=msgid, > date=date, > headers=headers, > diff --git a/patchwork/tests/test_mboxviews.py > b/patchwork/tests/test_mboxviews.py > index a7b0186a..1535c5cb 100644 > --- a/patchwork/tests/test_mboxviews.py > +++ b/patchwork/tests/test_mboxviews.py > @@ -268,9 +268,12 @@ class MboxSeriesDependencies(TestCase): > def test_patch_with_wildcard_series(self): > _, patch_a, patch_b = self._create_patches() > > - response = self.client.get('%s?series=*' % reverse( > - 'patch-mbox', args=[patch_b.patch.project.linkname, > - patch_b.patch.url_msgid])) > + response = self.client.get( > + '%s?series=*' % reverse( > + 'patch-mbox', > + args=[patch_b.project.linkname, patch_b.url_msgid], > + ), > + ) > > self.assertContains(response, patch_a.content) > self.assertContains(response, patch_b.content) > @@ -279,9 +282,12 @@ class MboxSeriesDependencies(TestCase): > series, patch_a, patch_b = self._create_patches() > > response = self.client.get('%s?series=%d' % ( > - reverse('patch-mbox', args=[patch_b.patch.project.linkname, > - patch_b.patch.url_msgid]), > - series.id)) > + reverse( > + 'patch-mbox', > + args=[patch_b.project.linkname, patch_b.url_msgid], > + ), > + series.id, > + )) > > self.assertContains(response, patch_a.content) > self.assertContains(response, patch_b.content) > @@ -291,8 +297,12 @@ class MboxSeriesDependencies(TestCase): > > for value in ('foo', str(series.id + 1)): > response = self.client.get('%s?series=%s' % ( > - reverse('patch-mbox', args=[patch_b.patch.project.linkname, > - patch_b.patch.url_msgid]), > value)) > + reverse( > + 'patch-mbox', > + args=[patch_b.project.linkname, patch_b.url_msgid] > + ), > + value, > + )) > > self.assertEqual(response.status_code, 404) > > diff --git a/patchwork/tests/utils.py b/patchwork/tests/utils.py > index aeeb14d7..b24a992e 100644 > --- a/patchwork/tests/utils.py > +++ b/patchwork/tests/utils.py > @@ -190,9 +190,6 @@ def create_patch(**kwargs): > } > values.update(kwargs) > > - if 'patch_project' not in values: > - values['patch_project'] = values['project'] > - > patch = Patch.objects.create(**values) > > if series: > @@ -311,7 +308,7 @@ def create_series_reference(**kwargs): > > > def _create_submissions(create_func, count=1, **kwargs): > - """Create 'count' Submission-based objects. > + """Create 'count' SubmissionMixin-based objects. > > Args: > count (int): Number of patches to create > diff --git a/patchwork/views/__init__.py b/patchwork/views/__init__.py > index ad17a070..3efe90cd 100644 > --- a/patchwork/views/__init__.py > +++ b/patchwork/views/__init__.py > @@ -257,7 +257,7 @@ def generic_list(request, project, view, view_args=None, > filter_settings=None, > context['filters'].set_status(filterclass, setting) > > if patches is None: > - patches = Patch.objects.filter(patch_project=project) > + patches = Patch.objects.filter(project=project) > > # annotate with tag counts > patches = patches.with_tag_counts(project) > diff --git a/patchwork/views/utils.py b/patchwork/views/utils.py > index 875edf45..b301fba9 100644 > --- a/patchwork/views/utils.py > +++ b/patchwork/views/utils.py > @@ -183,8 +183,8 @@ def series_to_mbox(series): > """ > mbox = [] > > - for dep in series.patches.all().order_by('number'): > - mbox.append(patch_to_mbox(dep.patch)) > + for patch in series.patches.all().order_by('number'): > + mbox.append(patch_to_mbox(patch)) > > return '\n'.join(mbox) > > -- > 2.24.1 _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork