[PATCH] models: Add Series.is_open
Stephen Finucane
stephen at that.guru
Thu Sep 23 21:40:30 AEST 2021
On Sun, 2021-09-05 at 11:34 +1000, Daniel Axtens wrote:
> Stephen Finucane <stephen at that.guru> writes:
>
> > Start making series more useful by tracking whether the state is open,
> > meaning one or more patches are in a state with action_required=True, or
> > not. This means that the 'git-pw series list' command will only list
> > series that are actually actionable.
> >
> > Signed-off-by: Stephen Finucane <stephen at that.guru>
> > ---
> > Open questions:
> > - Is there a more efficient way to do the migration than I've done?
> > - Should we expose an 'is_open' boolean attribute of a 'state' (open,
> > closed) attribute for the series views?
> > - Is there a race condition possible in how I've implemented the series
> > state checks?
>
> AIUI, series.is_open is a derived property (or I suppose in db terms a
> 'view') of series.patches.
>
> Is the cost of computing it 'live' so bad that we need to cache it in
> the db? (a 'materialised view'[1])
>
> Is there any chance we could just always compute on demand? I'm trying
> to think of all the ways we expose series:
>
> - on the patch list page, we already also fetch all the patches
>
> - on the API /series/ page, we also fetch all the patches even for just
> the list view.
>
> so it seems to me we probably shouldn't suffer a performance hit for
> doing it live.
The biggest impact I see is filtering. This is the main thing I want because
presently 'git pw series list' is pretty much useless without this. I don't
doing this dynamically would work because attempting to fetch, for example, the
first 100 "closed" series will require loading every series and every patch. You
couldn't just load the first 100 since they could be entirely/mostly "open",
meaning you'd need to load another 100, and another, and another, until you get
100 closed. This will need to be stored _somewhere_ fwict.
> [1] https://www.datacamp.com/community/tutorials/materialized-views-postgresql
> and noting that materialised views are not supported natively in mysql -
> but you've basically done https://linuxhint.com/materialized-views-mysql
> with the refresh logic in the application layer rather than a stored
> procedure.
>
> > diff --git patchwork/models.py patchwork/models.py
> > index 58e4c51e..7441510b 100644
> > --- patchwork/models.py
> > +++ patchwork/models.py
> > @@ -14,6 +14,7 @@ from django.conf import settings
> > from django.contrib.auth.models import User
> > from django.core.exceptions import ValidationError
> > from django.db import models
> > +from django.db import transaction
> > from django.urls import reverse
> > from django.utils.functional import cached_property
> > from django.core.validators import validate_unicode_slug
> > @@ -494,6 +495,19 @@ class Patch(SubmissionMixin):
> > for tag in tags:
> > self._set_tag(tag, counter[tag])
> >
> > + def refresh_series_state(self):
> > + if not self.series:
> > + return
> > +
> > + # If any of the patches in the series is in an "action required" state,
> > + # then the series is still open
> > + # TODO(stephenfin): Can this still race?
> > + with transaction.atomic():
> > + self.series.is_open = self.series.patches.filter(
> > + state__action_required=True
> > + ).exists()
> > + self.series.save()
> > +
>
> AIUI based some experimentation and my experience dealing with our
> series race conditions in 2018, this can race and transactions won't
> save you here. The transaction will roll back all the changes if any
> statement fails (e.g. if you had an integrityerror from a foreign key),
> and other users won't see intermediate writes from your
> transaction. What it won't do is preserve the data dependency you're
> creating in the way a traditional mutex would.
>
> Basically, consider a concurrent parsemail and an API client changing a
> patch state and refreshing series state on the same series:
>
>
> | parsemail | API client |
> ----------------------------------------------------------------
> | | series.patch[2].save() |
> | | BEGIN TRANSACTION |
> | | is_open = SELECT(...) = False |
> | series.patch[3].save() | |
> | BEGIN TRANSACTION | |
> | is_open = SELECT(...) = True | |
> | UPDATE series... | |
> | COMMIT TRANSACTION | |
> | | UPDATE series... |
> | | COMMIT TRANSACTION |
>
> both transactions can complete successfully and the end result is
> wrong. (You can play with this sort of thing with two concurrent
> dbshells both updating a commit_ref in a transaction, which I found very
> helpful in getting the details right.)
>
> Not materialising the view avoids persisting a result where we've lost
> the race.
Ack, yeah, this makes sense. I need to investigate these materialized views
things, but assuming they're not an option with MySQL and SQLite then I guess
I'll need to find a way to lock the whole thing in a mutex.
Stephen
PS: Sorry for the delay. Holidays.
> Kind regards,
> Daniel
>
>
> > def save(self, *args, **kwargs):
> > if not hasattr(self, 'state') or not self.state:
> > self.state = get_default_initial_patch_state()
> > @@ -503,6 +517,7 @@ class Patch(SubmissionMixin):
> >
> > super(Patch, self).save(**kwargs)
> >
> > + self.refresh_series_state()
> > self.refresh_tag_counts()
> >
> > def is_editable(self, user):
> > @@ -772,6 +787,11 @@ class Series(FilenameMixin, models.Model):
> > 'by the subject prefix(es)')
> > total = models.IntegerField(help_text='Number of patches in series as '
> > 'indicated by the subject prefix(es)')
> > + is_open = models.BooleanField(
> > + default=True,
> > + null=True,
> > + help_text='Are all patches in this series resolved?',
> > + )
> >
> > @staticmethod
> > def _format_name(obj):
> > diff --git patchwork/tests/api/test_series.py patchwork/tests/api/test_series.py
> > index ef661773..830b2fdb 100644
> > --- patchwork/tests/api/test_series.py
> > +++ patchwork/tests/api/test_series.py
> > @@ -16,6 +16,7 @@ from patchwork.tests.utils import create_patch
> > from patchwork.tests.utils import create_person
> > from patchwork.tests.utils import create_project
> > from patchwork.tests.utils import create_series
> > +from patchwork.tests.utils import create_state
> > from patchwork.tests.utils import create_user
> >
> > if settings.ENABLE_REST_API:
> > @@ -122,6 +123,57 @@ class TestSeriesAPI(utils.APITestCase):
> > 'submitter': 'test at example.org'})
> > self.assertEqual(0, len(resp.data))
> >
> > + def test_list_filter_is_open(self):
> > + """Filter series by status."""
> > + project_obj = create_project(linkname='myproject')
> > + person_obj = create_person(email='test at example.com')
> > +
> > + # create two series with a single patch in each with different states
> > + # for the two patches
> > +
> > + state_a = create_state(name='New', slug='new', action_required=True)
> > + series_a = create_series(project=project_obj, submitter=person_obj)
> > + create_cover(series=series_a)
> > + create_patch(series=series_a, state=state_a)
> > + self.assertTrue(series_a.is_open)
> > +
> > + state_b = create_state(
> > + name='Accepted', slug='Accepted', action_required=False,
> > + )
> > + series_b = create_series(project=project_obj, submitter=person_obj)
> > + create_cover(series=series_b)
> > + create_patch(series=series_b, state=state_b)
> > + self.assertFalse(series_b.is_open)
> > +
> > + resp = self.client.get(self.api_url(), {'is_open': 'True'})
> > + self.assertEqual([series_a.id], [x['id'] for x in resp.data])
> > +
> > + resp = self.client.get(self.api_url(), {'is_open': 'False'})
> > + self.assertEqual([series_b.id], [x['id'] for x in resp.data])
> > +
> > + resp = self.client.get(self.api_url())
> > + self.assertEqual(
> > + [series_a.id, series_b.id], [x['id'] for x in resp.data]
> > + )
> > +
> > + @utils.store_samples('series-list-1-2')
> > + def test_list_version_1_2(self):
> > + """List patches using API v1.2.
> > +
> > + Validate that newer fields are dropped for older API versions.
> > + """
> > + self._create_series()
> > +
> > + resp = self.client.get(self.api_url(version='1.2'))
> > + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> > + self.assertEqual(1, len(resp.data))
> > + self.assertIn('url', resp.data[0])
> > + self.assertIn('web_url', resp.data[0])
> > + self.assertIn('web_url', resp.data[0]['cover_letter'])
> > + self.assertIn('mbox', resp.data[0]['cover_letter'])
> > + self.assertIn('web_url', resp.data[0]['patches'][0])
> > + self.assertNotIn('is_open', resp.data[0])
> > +
> > @utils.store_samples('series-list-1-0')
> > def test_list_version_1_0(self):
> > """List patches using API v1.0.
> > diff --git patchwork/tests/test_models.py patchwork/tests/test_models.py
> > new file mode 100644
> > index 00000000..13db3330
> > --- /dev/null
> > +++ patchwork/tests/test_models.py
> > @@ -0,0 +1,72 @@
> > +# Patchwork - automated patch tracking system
> > +# Copyright (C) 2021 Stephen Finucane <stephen at that.guru>
> > +#
> > +# SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +from django.test import TestCase
> > +
> > +from patchwork.tests.utils import create_patch
> > +from patchwork.tests.utils import create_series
> > +from patchwork.tests.utils import create_state
> > +
> > +
> > +class TestPatch(TestCase):
> > +
> > + def test_save_updates_series_state(self):
> > + state_new = create_state(
> > + name='New',
> > + slug='new',
> > + action_required=True,
> > + )
> > + state_under_review = create_state(
> > + name='Under Review',
> > + slug='under-review',
> > + action_required=True,
> > + )
> > + state_accepted = create_state(
> > + name='Accepted',
> > + slug='accepted',
> > + action_required=False,
> > + )
> > + state_rejected = create_state(
> > + name='Rejected',
> > + slug='rejected',
> > + action_required=False,
> > + )
> > + series = create_series()
> > + patches = []
> > +
> > + # Create four patches - one in each state
> > + for state in (
> > + state_new, state_under_review, state_accepted, state_rejected,
> > + ):
> > + patches.append(create_patch(series=series, state=state))
> > +
> > + # The patches will be in various states so the series should still be
> > + # open
> > + self.assertTrue(patches[0].state.action_required)
> > + self.assertTrue(patches[1].state.action_required)
> > + self.assertFalse(patches[2].state.action_required)
> > + self.assertFalse(patches[3].state.action_required)
> > + self.assertTrue(series.is_open)
> > +
> > + # Now change the first patch to 'accepted'. The series shouldn't change
> > + # state because the second one is still 'under-review'
> > + patches[0].state = state_accepted
> > + patches[0].save()
> > + self.assertFalse(patches[0].state.action_required)
> > + self.assertTrue(patches[1].state.action_required)
> > + self.assertFalse(patches[2].state.action_required)
> > + self.assertFalse(patches[3].state.action_required)
> > + self.assertTrue(series.is_open)
> > +
> > + # Now change the second patch to 'rejected'. The series should finally
> > + # change state because all patches now have a state with
> > + # action_required=False
> > + patches[1].state = state_rejected
> > + patches[1].save()
> > + self.assertFalse(patches[0].state.action_required)
> > + self.assertFalse(patches[1].state.action_required)
> > + self.assertFalse(patches[2].state.action_required)
> > + self.assertFalse(patches[3].state.action_required)
> > + self.assertFalse(series.is_open)
> > --
> > 2.31.1
More information about the Patchwork
mailing list