[PATCH] models: Add Series.is_open
Daniel Axtens
dja at axtens.net
Sun Sep 5 11:34:21 AEST 2021
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.
[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.
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