From 6d959769f3a0ac893bf32b0f3b4be9e78031b778 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Sun, 5 Apr 2020 19:46:58 +0100 Subject: [PATCH 01/39] Post-release version bump Signed-off-by: Stephen Finucane --- patchwork/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patchwork/__init__.py b/patchwork/__init__.py index b55d4b3b0..8d85ae904 100644 --- a/patchwork/__init__.py +++ b/patchwork/__init__.py @@ -5,7 +5,7 @@ from patchwork.version import get_latest_version -VERSION = (2, 2, 0) +VERSION = (2, 2, 1, 'alpha', 0) __version__ = get_latest_version(VERSION) From 2111f726e173b09f607b2a1cc44e4236c0030c69 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 8 Apr 2020 23:27:52 +0100 Subject: [PATCH 02/39] docs: Reference Patchwork 2.2 tarballs This is what users will want right now. Signed-off-by: Stephen Finucane (cherry picked from commit 3d4eebde23b68daeba1b0852152243e5a6019fb8) --- docs/deployment/installation.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/deployment/installation.rst b/docs/deployment/installation.rst index 667a6a69d..12801a794 100644 --- a/docs/deployment/installation.rst +++ b/docs/deployment/installation.rst @@ -139,14 +139,14 @@ The first requirement is Patchwork itself. It can be downloaded like so: .. code-block:: shell - $ wget https://github.com/getpatchwork/patchwork/archive/v2.1.0.tar.gz + $ wget https://github.com/getpatchwork/patchwork/archive/v2.2.0.tar.gz We will install this under ``/opt``, though this is only a suggestion: .. code-block:: shell - $ tar -xvzf v2.1.0.tar.gz - $ sudo mv v2.1.0 /opt/patchwork + $ tar -xvzf v2.2.0.tar.gz + $ sudo mv v2.2.0 /opt/patchwork .. important:: From 9c7bc7f8506dccc4bdc2692ae55401dd898aad97 Mon Sep 17 00:00:00 2001 From: Jeremy Kerr Date: Tue, 14 Apr 2020 13:57:53 +0800 Subject: [PATCH 03/39] lib/sql: Update grant script for recent schema changes This change fixes a few omissions in the grant scripts: - patchrelation is missing from both mysql and postgres scripts; it's only needed for web user access. - event is missing from the web grants on postgres, and the mail grants on mysql. Tested on postgres only. Fixes: 27c2acf56c ("models, templates: Add patch relations") Fixes: 34e3c9c493 ("sql: Update 'grant-all.mysql' script with missing tables") Fixes: 234bc7c316 ("lib/sql: fix permissions for v2.0.0 on postgres") Signed-off-by: Jeremy Kerr Signed-off-by: Daniel Axtens --- lib/sql/grant-all.mysql.sql | 2 ++ lib/sql/grant-all.postgres.sql | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/lib/sql/grant-all.mysql.sql b/lib/sql/grant-all.mysql.sql index 02770777c..100cd387f 100644 --- a/lib/sql/grant-all.mysql.sql +++ b/lib/sql/grant-all.mysql.sql @@ -23,6 +23,7 @@ GRANT SELECT, UPDATE, INSERT, DELETE ON patchwork_emailoptout TO 'www-data'@loca GRANT SELECT, UPDATE, INSERT, DELETE ON patchwork_event TO 'www-data'@localhost; GRANT SELECT, UPDATE, INSERT, DELETE ON patchwork_patch TO 'www-data'@localhost; GRANT SELECT, UPDATE, INSERT, DELETE ON patchwork_patchchangenotification TO 'www-data'@localhost; +GRANT SELECT, UPDATE, INSERT, DELETE ON patchwork_patchrelation TO 'www-data'@localhost; GRANT SELECT, UPDATE, INSERT, DELETE ON patchwork_patchtag TO 'www-data'@localhost; GRANT SELECT, UPDATE, INSERT, DELETE ON patchwork_person TO 'www-data'@localhost; GRANT SELECT, UPDATE, INSERT, DELETE ON patchwork_project TO 'www-data'@localhost; @@ -38,6 +39,7 @@ GRANT SELECT, UPDATE, INSERT, DELETE ON patchwork_userprofile_maintainer_project -- cover letters) and series GRANT INSERT, SELECT ON patchwork_comment TO 'nobody'@localhost; GRANT INSERT, SELECT ON patchwork_coverletter TO 'nobody'@localhost; +GRANT INSERT, SELECT ON patchwork_event TO 'nobody'@localhost; GRANT INSERT, SELECT ON patchwork_patch TO 'nobody'@localhost; GRANT INSERT, SELECT ON patchwork_person TO 'nobody'@localhost; GRANT INSERT, SELECT ON patchwork_series TO 'nobody'@localhost; diff --git a/lib/sql/grant-all.postgres.sql b/lib/sql/grant-all.postgres.sql index 10ec8d2ca..56a24864d 100644 --- a/lib/sql/grant-all.postgres.sql +++ b/lib/sql/grant-all.postgres.sql @@ -21,8 +21,10 @@ GRANT SELECT, UPDATE, INSERT, DELETE ON patchwork_delegationrule, patchwork_emailconfirmation, patchwork_emailoptout, + patchwork_event, patchwork_patch, patchwork_patchchangenotification, + patchwork_patchrelation, patchwork_patchtag, patchwork_person, patchwork_project, @@ -50,7 +52,9 @@ GRANT SELECT, UPDATE ON patchwork_comment_id_seq, patchwork_delegationrule_id_seq, patchwork_emailconfirmation_id_seq, + patchwork_event_id_seq, patchwork_patch_id_seq, + patchwork_patchrelation_id_seq, patchwork_patchtag_id_seq, patchwork_person_id_seq, patchwork_project_id_seq, From 40f1fa9e9316115b6ec71ef84737b1df6480e6f0 Mon Sep 17 00:00:00 2001 From: Daniel Axtens Date: Tue, 14 Apr 2020 14:26:58 +1000 Subject: [PATCH 04/39] api: do not fetch every patch in a patch detail view 404 mpe and jk and sfr found that the OzLabs server was melting due to some queries downloading every patch. Turns out if you 404 the patch detail view in the API, d-r-f attempts to render a listbox with every single patch to fill in the 'related' field. The bundle API also has a similar field. Replace the multiple selection box with a text field. You can still (AIUI) populate the relevant patch IDs manually. This is the recommended approach per https://www.django-rest-framework.org/topics/browsable-api/#handling-choicefield-with-large-numbers-of-items Reported-by: Jeremy Kerr Reported-by: Michael Ellerman Reported-by: Stephen Rothwell Tested-by: Jeremy Kerr Server-no-longer-on-fire-by: Jeremy Kerr Reviewed-by: Stephen Finucane Signed-off-by: Daniel Axtens (cherry picked from commit 08c5856444bc2e100c4acbbea0a244cd46083c4b) --- patchwork/api/bundle.py | 3 ++- patchwork/api/embedded.py | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/patchwork/api/bundle.py b/patchwork/api/bundle.py index b8c0f1781..54a9266e7 100644 --- a/patchwork/api/bundle.py +++ b/patchwork/api/bundle.py @@ -62,7 +62,8 @@ class BundleSerializer(BaseHyperlinkedModelSerializer): project = ProjectSerializer(read_only=True) mbox = SerializerMethodField() owner = UserSerializer(read_only=True) - patches = PatchSerializer(many=True, required=True) + patches = PatchSerializer(many=True, required=True, + style={'base_template': 'input.html'}) def get_web_url(self, instance): request = self.context.get('request') diff --git a/patchwork/api/embedded.py b/patchwork/api/embedded.py index 85a30cae1..cb3f07e6b 100644 --- a/patchwork/api/embedded.py +++ b/patchwork/api/embedded.py @@ -141,7 +141,8 @@ class Meta: class PatchRelationSerializer(BaseHyperlinkedModelSerializer): """Hide the PatchRelation model, just show the list""" - patches = PatchSerializer(many=True) + patches = PatchSerializer(many=True, + style={'base_template': 'input.html'}) def to_internal_value(self, data): if not isinstance(data, type([])): From 0a7a74a59bf9371af03ba31613f9fe8a821030d9 Mon Sep 17 00:00:00 2001 From: Daniel Axtens Date: Tue, 14 Apr 2020 15:34:40 +1000 Subject: [PATCH 05/39] api: allow filtering patches and covers by msgid In the process of fixing the previous bug, I realised that: a) /api/patches/msgid is a perfectly reasonable thing to attempt b) We have no way of finding a patch by message id in the API We can't actualy make /api/patches/msgid work because it may not be unique, but we can add a filter. I'm shoehorning this into stable/2.2, even though it's technically an API change: it's minor, not incompatible and in hindsight a glaring hole. Cc: Michael Ellerman Tested-by: Jeremy Kerr Reviewed-by: Andrew Donnellan Reviewed-by: Stephen Finucane Signed-off-by: Daniel Axtens (cherry picked from commit d08b6c72964898c9997a62e4ab6a721f166a56ca) --- docs/api/schemas/latest/patchwork.yaml | 16 ++++++++++++++++ docs/api/schemas/patchwork.j2 | 18 ++++++++++++++++++ docs/api/schemas/v1.2/patchwork.yaml | 16 ++++++++++++++++ patchwork/api/filters.py | 14 ++++++++++---- patchwork/tests/api/test_cover.py | 12 ++++++++++++ patchwork/tests/api/test_patch.py | 12 ++++++++++++ .../rest-filter-msgid-41f693cd4e53cf93.yaml | 6 ++++++ 7 files changed, 90 insertions(+), 4 deletions(-) create mode 100644 releasenotes/notes/rest-filter-msgid-41f693cd4e53cf93.yaml diff --git a/docs/api/schemas/latest/patchwork.yaml b/docs/api/schemas/latest/patchwork.yaml index 13cdc9cd7..cc0d97e69 100644 --- a/docs/api/schemas/latest/patchwork.yaml +++ b/docs/api/schemas/latest/patchwork.yaml @@ -246,6 +246,14 @@ paths: schema: title: '' type: string + - in: query + name: msgid + description: > + The cover message-id as a case-sensitive string, without leading or + trailing angle brackets, to filter by. + schema: + title: '' + type: string responses: '200': description: '' @@ -474,6 +482,14 @@ paths: schema: title: '' type: string + - in: query + name: msgid + description: > + The patch message-id as a case-sensitive string, without leading or + trailing angle brackets, to filter by. + schema: + title: '' + type: string responses: '200': description: '' diff --git a/docs/api/schemas/patchwork.j2 b/docs/api/schemas/patchwork.j2 index bd714d5e7..f5618d41f 100644 --- a/docs/api/schemas/patchwork.j2 +++ b/docs/api/schemas/patchwork.j2 @@ -251,6 +251,16 @@ paths: schema: title: '' type: string +{% if version >= (1, 2) %} + - in: query + name: msgid + description: > + The cover message-id as a case-sensitive string, without leading or + trailing angle brackets, to filter by. + schema: + title: '' + type: string +{% endif %} responses: '200': description: '' @@ -488,6 +498,14 @@ paths: schema: title: '' type: string + - in: query + name: msgid + description: > + The patch message-id as a case-sensitive string, without leading or + trailing angle brackets, to filter by. + schema: + title: '' + type: string {% endif %} responses: '200': diff --git a/docs/api/schemas/v1.2/patchwork.yaml b/docs/api/schemas/v1.2/patchwork.yaml index db2ed122e..7bdbe6699 100644 --- a/docs/api/schemas/v1.2/patchwork.yaml +++ b/docs/api/schemas/v1.2/patchwork.yaml @@ -246,6 +246,14 @@ paths: schema: title: '' type: string + - in: query + name: msgid + description: > + The cover message-id as a case-sensitive string, without leading or + trailing angle brackets, to filter by. + schema: + title: '' + type: string responses: '200': description: '' @@ -474,6 +482,14 @@ paths: schema: title: '' type: string + - in: query + name: msgid + description: > + The patch message-id as a case-sensitive string, without leading or + trailing angle brackets, to filter by. + schema: + title: '' + type: string responses: '200': description: '' diff --git a/patchwork/api/filters.py b/patchwork/api/filters.py index f7b6a6f66..1f02cfa05 100644 --- a/patchwork/api/filters.py +++ b/patchwork/api/filters.py @@ -172,6 +172,10 @@ class Meta: fields = ('submitter', 'project') +def msgid_filter(queryset, name, value): + return queryset.filter(**{name: '<' + value + '>'}) + + class CoverLetterFilterSet(TimestampMixin, BaseFilterSet): project = ProjectFilter(queryset=Project.objects.all(), distinct=False) @@ -180,6 +184,7 @@ class CoverLetterFilterSet(TimestampMixin, BaseFilterSet): series = BaseFilter(queryset=Project.objects.all(), widget=MultipleHiddenInput, distinct=False) submitter = PersonFilter(queryset=Person.objects.all(), distinct=False) + msgid = CharFilter(method=msgid_filter) class Meta: model = CoverLetter @@ -198,17 +203,18 @@ class PatchFilterSet(TimestampMixin, BaseFilterSet): delegate = UserFilter(queryset=User.objects.all(), distinct=False) state = StateFilter(queryset=State.objects.all(), distinct=False) hash = CharFilter(lookup_expr='iexact') + msgid = CharFilter(method=msgid_filter) class Meta: model = Patch - # NOTE(dja): ideally we want to version the hash field, but I cannot - # find a way to do that which is reliable and not extremely ugly. + # NOTE(dja): ideally we want to version the hash/msgid field, but I + # can't find a way to do that which is reliable and not extremely ugly. # The best I can come up with is manually working with request.GET # which seems to rather defeat the point of using django-filters. fields = ('project', 'series', 'submitter', 'delegate', - 'state', 'archived', 'hash') + 'state', 'archived', 'hash', 'msgid') versioned_fields = { - '1.2': ('hash', ), + '1.2': ('hash', 'msgid'), } diff --git a/patchwork/tests/api/test_cover.py b/patchwork/tests/api/test_cover.py index 5eeb1902e..1b19ded1b 100644 --- a/patchwork/tests/api/test_cover.py +++ b/patchwork/tests/api/test_cover.py @@ -111,6 +111,18 @@ def test_list_filter_submitter(self): 'submitter': 'test@example.org'}) self.assertEqual(0, len(resp.data)) + def test_list_filter_msgid(self): + """Filter covers by msgid.""" + cover = create_cover() + + resp = self.client.get(self.api_url(), {'msgid': cover.url_msgid}) + self.assertEqual([cover.id], [x['id'] for x in resp.data]) + + # empty response if nothing matches + resp = self.client.get(self.api_url(), { + 'msgid': 'fishfish@fish.fish'}) + self.assertEqual(0, len(resp.data)) + @utils.store_samples('cover-list-1-0') def test_list_version_1_0(self): create_cover() diff --git a/patchwork/tests/api/test_patch.py b/patchwork/tests/api/test_patch.py index b24c5ab28..da2dd6e90 100644 --- a/patchwork/tests/api/test_patch.py +++ b/patchwork/tests/api/test_patch.py @@ -199,6 +199,18 @@ def test_list_filter_hash_version_1_1(self): {'hash': 'garbagevalue'}) self.assertEqual(1, len(resp.data)) + def test_list_filter_msgid(self): + """Filter patches by msgid.""" + patch = self._create_patch() + + resp = self.client.get(self.api_url(), {'msgid': patch.url_msgid}) + self.assertEqual([patch.id], [x['id'] for x in resp.data]) + + # empty response if nothing matches + resp = self.client.get(self.api_url(), { + 'msgid': 'fishfish@fish.fish'}) + self.assertEqual(0, len(resp.data)) + @utils.store_samples('patch-list-1-0') def test_list_version_1_0(self): """List patches using API v1.0.""" diff --git a/releasenotes/notes/rest-filter-msgid-41f693cd4e53cf93.yaml b/releasenotes/notes/rest-filter-msgid-41f693cd4e53cf93.yaml new file mode 100644 index 000000000..0fcbbeb8a --- /dev/null +++ b/releasenotes/notes/rest-filter-msgid-41f693cd4e53cf93.yaml @@ -0,0 +1,6 @@ +--- +api: + - | + The REST API now supports filtering patches and cover letters by message + ID, using the ``msgid`` query parameter. Don't include leading or trailing + angle brackets. From 123bfffe5187089aaadb4829bf67af7dea092ce9 Mon Sep 17 00:00:00 2001 From: Daniel Axtens Date: Tue, 14 Apr 2020 16:43:43 +1000 Subject: [PATCH 06/39] Release 2.2.1 Signed-off-by: Daniel Axtens --- patchwork/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patchwork/__init__.py b/patchwork/__init__.py index 8d85ae904..963cb914c 100644 --- a/patchwork/__init__.py +++ b/patchwork/__init__.py @@ -5,7 +5,7 @@ from patchwork.version import get_latest_version -VERSION = (2, 2, 1, 'alpha', 0) +VERSION = (2, 2, 1) __version__ = get_latest_version(VERSION) From 965dc6946c77884abe01f76309c75c6b0cbe03ae Mon Sep 17 00:00:00 2001 From: Daniel Axtens Date: Tue, 14 Apr 2020 16:43:51 +1000 Subject: [PATCH 07/39] Post-release version bump Signed-off-by: Daniel Axtens --- patchwork/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patchwork/__init__.py b/patchwork/__init__.py index 963cb914c..b4cb728ce 100644 --- a/patchwork/__init__.py +++ b/patchwork/__init__.py @@ -5,7 +5,7 @@ from patchwork.version import get_latest_version -VERSION = (2, 2, 1) +VERSION = (2, 2, 2, 'alpha', 0) __version__ = get_latest_version(VERSION) From bf2aee679d185f585844d7afecc4145ff3b5b06f Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Fri, 17 Apr 2020 23:07:27 +0100 Subject: [PATCH 08/39] REST: Allow update of bundle without patches Presently, when updating a patch we assume that patches are provided. This isn't necessary - you might just want to make it public - and isn't enforced by the API itself. However, because we make this assumption, we see a HTTP 500. Resolve the issue and add tests to prevent a regression. Signed-off-by: Stephen Finucane Resolves: #357 (cherry picked from commit 4fd7a739bbed62230d4166509929a35f63f6892e) --- patchwork/api/bundle.py | 8 +++--- patchwork/tests/api/test_bundle.py | 25 +++++++++++++++++++ .../notes/issue-357-1bef23dbfda2722d.yaml | 6 +++++ 3 files changed, 36 insertions(+), 3 deletions(-) create mode 100644 releasenotes/notes/issue-357-1bef23dbfda2722d.yaml diff --git a/patchwork/api/bundle.py b/patchwork/api/bundle.py index 54a9266e7..93e323166 100644 --- a/patchwork/api/bundle.py +++ b/patchwork/api/bundle.py @@ -80,10 +80,11 @@ def create(self, validated_data): return instance def update(self, instance, validated_data): - patches = validated_data.pop('patches') + patches = validated_data.pop('patches', None) instance = super(BundleSerializer, self).update( instance, validated_data) - instance.overwrite_patches(patches) + if patches: + instance.overwrite_patches(patches) return instance def validate_patches(self, value): @@ -97,7 +98,8 @@ def validate_patches(self, value): return value def validate(self, data): - data['project'] = data['patches'][0].project + if data.get('patches'): + data['project'] = data['patches'][0].project return super(BundleSerializer, self).validate(data) diff --git a/patchwork/tests/api/test_bundle.py b/patchwork/tests/api/test_bundle.py index d03f26f15..799244861 100644 --- a/patchwork/tests/api/test_bundle.py +++ b/patchwork/tests/api/test_bundle.py @@ -288,9 +288,34 @@ def test_update(self): self.assertEqual(status.HTTP_200_OK, resp.status_code) self.assertEqual(2, len(resp.data['patches'])) self.assertEqual('hello-bundle', resp.data['name']) + self.assertFalse(resp.data['public']) self.assertEqual(1, Bundle.objects.all().count()) self.assertEqual(2, len(Bundle.objects.first().patches.all())) self.assertEqual('hello-bundle', Bundle.objects.first().name) + self.assertFalse(Bundle.objects.first().public) + + def test_update_no_patches(self): + """Validate we handle updating only the name.""" + user, project, patch_a, patch_b = self._test_create_update() + bundle = create_bundle(owner=user, project=project) + + bundle.append_patch(patch_a) + bundle.append_patch(patch_b) + + self.assertEqual(1, Bundle.objects.all().count()) + self.assertEqual(2, len(Bundle.objects.first().patches.all())) + + resp = self.client.patch(self.api_url(bundle.id), { + 'name': 'hello-bundle', 'public': True, + }) + self.assertEqual(status.HTTP_200_OK, resp.status_code) + self.assertEqual(2, len(resp.data['patches'])) + self.assertEqual('hello-bundle', resp.data['name']) + self.assertTrue(resp.data['public']) + self.assertEqual(1, Bundle.objects.all().count()) + self.assertEqual(2, len(Bundle.objects.first().patches.all())) + self.assertEqual('hello-bundle', Bundle.objects.first().name) + self.assertTrue(Bundle.objects.first().public) @utils.store_samples('bundle-delete-not-found') def test_delete_anonymous(self): diff --git a/releasenotes/notes/issue-357-1bef23dbfda2722d.yaml b/releasenotes/notes/issue-357-1bef23dbfda2722d.yaml new file mode 100644 index 000000000..1f337c726 --- /dev/null +++ b/releasenotes/notes/issue-357-1bef23dbfda2722d.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + An issue that preventing updating bundles via the REST API without + updating the included patches has been resolved. + (`#357 `__) From 77b8191ae6c0c08565f139da9ff94f5a2f21cc6b Mon Sep 17 00:00:00 2001 From: Andrew Donnellan Date: Wed, 15 Apr 2020 19:06:56 +1000 Subject: [PATCH 09/39] parser: Don't crash when From: is list email but has weird mangle format get_original_sender() tries to demangle DMARC-mangled From headers, in the case where the email's From address is the list address. It knows how to handle Google Groups and Mailman style mangling, where the original submitter's name will be turned into e.g. "Andrew Donnellan via linuxppc-dev". If an email has the From header set to the list address but has a name that doesn't include " via ", we'll throw an exception because stripped_name hasn't been set. Sometimes this is because the list name is seemingly empty, resulting in a mangled name like "Andrew Donnellan via" without the space after "via" that we detect. Handle this as well as we can instead, and add a test. Fixes: 8279a84238c10 ("parser: Unmangle From: headers that have been mangled for DMARC purposes") Reported-by: Jeremy Kerr Signed-off-by: Andrew Donnellan Reviewed-by: Stephen Finucane (cherry picked from commit 4698499b9704b29244af4a873efc6c8ae6ca3ff8) --- patchwork/parser.py | 7 +++++++ patchwork/tests/test_parser.py | 23 +++++++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/patchwork/parser.py b/patchwork/parser.py index 45930b454..cf3e3cf0a 100644 --- a/patchwork/parser.py +++ b/patchwork/parser.py @@ -392,6 +392,13 @@ def get_original_sender(mail, name, email): # Mailman uses the format " via " # Google Groups uses "'' via " stripped_name = name[:name.rfind(' via ')].strip().strip("'") + elif name.endswith(' via'): + # Sometimes this seems to happen (perhaps if Mailman isn't set up with + # any list name) + stripped_name = name[:name.rfind(' via')].strip().strip("'") + else: + # We've hit a format that we don't expect + stripped_name = None original_from = clean_header(mail.get('X-Original-From', '')) if original_from: diff --git a/patchwork/tests/test_parser.py b/patchwork/tests/test_parser.py index 6fbc9da9f..d5c98091b 100644 --- a/patchwork/tests/test_parser.py +++ b/patchwork/tests/test_parser.py @@ -371,6 +371,29 @@ def test_google_dmarc_munging(self): self.assertEqual(person_b._state.adding, False) self.assertEqual(person_b.id, person_a.id) + def test_weird_dmarc_munging(self): + project = create_project() + real_sender = 'Existing Sender ' + munged_sender1 = "'Existing Sender' via <{}>".format(project.listemail) + munged_sender2 = "'Existing Sender' <{}>".format(project.listemail) + + # Unmunged author + mail = self._create_email(real_sender) + person_a = get_or_create_author(mail, project) + person_a.save() + + # Munged with no list name + mail = self._create_email(munged_sender1, None, None, real_sender) + person_b = get_or_create_author(mail, project) + self.assertEqual(person_b._state.adding, False) + self.assertEqual(person_b.id, person_a.id) + + # Munged with no 'via' + mail = self._create_email(munged_sender2, None, None, real_sender) + person_b = get_or_create_author(mail, project) + self.assertEqual(person_b._state.adding, False) + self.assertEqual(person_b.id, person_a.id) + class SeriesCorrelationTest(TestCase): """Validate correct behavior of find_series.""" From 1493073e38c0c57e85778d2fdbf55cdb415522ca Mon Sep 17 00:00:00 2001 From: Jeremy Kerr Date: Thu, 16 Apr 2020 09:29:24 +0800 Subject: [PATCH 10/39] tests: Add duplicate mail test Test that we get the correct DuplicateMailError from parsing the same mail twice. Conflicts: patchwork/tests/test_parser.py NOTE(stephenfin): Conflicts are due to the removal of 'six' imports on master. Signed-off-by: Jeremy Kerr Reviewed-by: Stephen Finucane (cherry picked from commit d4b5fbd1d0c743003d59cef025f54b8d97586ddf) --- patchwork/tests/test_parser.py | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/patchwork/tests/test_parser.py b/patchwork/tests/test_parser.py index d5c98091b..7fe3608fa 100644 --- a/patchwork/tests/test_parser.py +++ b/patchwork/tests/test_parser.py @@ -12,6 +12,7 @@ import sys import unittest +from django.db.transaction import atomic from django.test import TestCase from django.test import TransactionTestCase from django.utils import six @@ -32,6 +33,7 @@ from patchwork.parser import parse_version from patchwork.parser import split_prefixes from patchwork.parser import subject_check +from patchwork.parser import DuplicateMailError from patchwork.tests import TEST_MAIL_DIR from patchwork.tests import TEST_FUZZ_DIR from patchwork.tests.utils import create_project @@ -1121,3 +1123,28 @@ def test_hdr(self): def test_x_face(self): self._test_patch('x-face.mbox') + + +class DuplicateMailTest(TestCase): + def setUp(self): + self.listid = 'patchwork.ozlabs.org' + create_project(listid=self.listid) + create_state() + + def _test_duplicate_mail(self, mail): + _parse_mail(mail) + with self.assertRaises(DuplicateMailError): + # If we see any database errors from the duplicate insert + # (typically an IntegrityError), the insert will abort the current + # transaction. This atomic() ensures that we can recover, and + # perform subsequent queries. + with atomic(): + _parse_mail(mail) + + def test_duplicate_patch(self): + diff = read_patch('0001-add-line.patch') + m = create_email(diff, listid=self.listid, msgid='1@example.com') + + self._test_duplicate_mail(m) + + self.assertEqual(Patch.objects.count(), 1) From 9463c2b3038fd278ef1a7de4a370d40862379483 Mon Sep 17 00:00:00 2001 From: Jeremy Kerr Date: Thu, 16 Apr 2020 09:29:25 +0800 Subject: [PATCH 11/39] tests: ensure we don't see database errors during duplicate insert Currently, the parser causes IntegrityErrors while inserting duplicate patches; these tend to pollute database logs. This change adds a check, which currently fails, to ensure we do not cause errors during a duplicate patch parse. Conflicts: patchwork/tests/test_parser.py NOTE(stephenfin): Conflicts are once again due to import reordering. In addition, we have to skip these tests on Django 1.11 since the 'connection.execute_wrapper' context manager was first added in Django 2.0 [1]. [1] https://docs.djangoproject.com/en/dev/releases/2.0/#models Signed-off-by: Jeremy Kerr Signed-off-by: Stephen Finucane [stephenfin: Add 'expectedFailure' marker to keep all tests green] (cherry picked from commit a60e75e2c6897fd262ec95a35e0e94b9027c11d4) --- patchwork/tests/test_parser.py | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/patchwork/tests/test_parser.py b/patchwork/tests/test_parser.py index 7fe3608fa..11dcb21ff 100644 --- a/patchwork/tests/test_parser.py +++ b/patchwork/tests/test_parser.py @@ -12,7 +12,9 @@ import sys import unittest +import django from django.db.transaction import atomic +from django.db import connection from django.test import TestCase from django.test import TransactionTestCase from django.utils import six @@ -1125,6 +1127,10 @@ def test_x_face(self): self._test_patch('x-face.mbox') +@unittest.skipIf( + django.VERSION < (2, 0), + 'Django 1.11 does not provide an easy DB query introspection API' +) class DuplicateMailTest(TestCase): def setUp(self): self.listid = 'patchwork.ozlabs.org' @@ -1132,15 +1138,30 @@ def setUp(self): create_state() def _test_duplicate_mail(self, mail): + errors = [] + + def log_query_errors(execute, sql, params, many, context): + try: + result = execute(sql, params, many, context) + except Exception as e: + errors.append(e) + raise + return result + _parse_mail(mail) + with self.assertRaises(DuplicateMailError): - # If we see any database errors from the duplicate insert - # (typically an IntegrityError), the insert will abort the current - # transaction. This atomic() ensures that we can recover, and - # perform subsequent queries. - with atomic(): - _parse_mail(mail) + with connection.execute_wrapper(log_query_errors): + # If we see any database errors from the duplicate insert + # (typically an IntegrityError), the insert will abort the + # current transaction. This atomic() ensures that we can + # recover, and perform subsequent queries. + with atomic(): + _parse_mail(mail) + + self.assertEqual(errors, []) + @unittest.expectedFailure def test_duplicate_patch(self): diff = read_patch('0001-add-line.patch') m = create_email(diff, listid=self.listid, msgid='1@example.com') From 91dce3e077ebdef833afd06a8407e1012929d929 Mon Sep 17 00:00:00 2001 From: Jeremy Kerr Date: Thu, 16 Apr 2020 09:29:26 +0800 Subject: [PATCH 12/39] parser: prevent IntegrityErrors Currently, the parser relies on causing (and catching) IntegrityErrors on patch insert to catch duplicate (msgid,project) mails. This change performs an atomic select -> insert instead. Signed-off-by: Jeremy Kerr Signed-off-by: Stephen Finucane [stephenfin: Remove 'expectedFailure' marker again] (cherry picked from commit 947c6aae94b7b554ca701c1d7e5baf000759ed2d) --- patchwork/parser.py | 7 ++++--- patchwork/tests/test_parser.py | 1 - 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/patchwork/parser.py b/patchwork/parser.py index cf3e3cf0a..76b89d369 100644 --- a/patchwork/parser.py +++ b/patchwork/parser.py @@ -1092,7 +1092,10 @@ def parse_mail(mail, list_id=None): filenames = find_filenames(diff) delegate = find_delegate_by_filename(project, filenames) - try: + with transaction.atomic(): + if Patch.objects.filter(project=project, msgid=msgid): + raise DuplicateMailError(msgid=msgid) + patch = Patch.objects.create( msgid=msgid, project=project, @@ -1107,8 +1110,6 @@ def parse_mail(mail, list_id=None): delegate=delegate, state=find_state(mail)) logger.debug('Patch saved') - except IntegrityError: - raise DuplicateMailError(msgid=msgid) for attempt in range(1, 11): # arbitrary retry count try: diff --git a/patchwork/tests/test_parser.py b/patchwork/tests/test_parser.py index 11dcb21ff..4041ef360 100644 --- a/patchwork/tests/test_parser.py +++ b/patchwork/tests/test_parser.py @@ -1161,7 +1161,6 @@ def log_query_errors(execute, sql, params, many, context): self.assertEqual(errors, []) - @unittest.expectedFailure def test_duplicate_patch(self): diff = read_patch('0001-add-line.patch') m = create_email(diff, listid=self.listid, msgid='1@example.com') From 69cd420b4ec3c95a47bb80eed2415d8ba01d84fb Mon Sep 17 00:00:00 2001 From: Jeremy Kerr Date: Thu, 16 Apr 2020 09:29:27 +0800 Subject: [PATCH 13/39] parser: don't trigger database IntegrityErrors on duplicate comments As we've done for the Patch model, this change prevents database errors from duplicate Comments. Signed-off-by: Jeremy Kerr Reviewed-by: Stephen Finucane (cherry picked from commit 55aa9cd749f3ff0de430c8f04c687d691c3a703a) --- patchwork/parser.py | 6 +++--- patchwork/tests/test_parser.py | 12 ++++++++++++ 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/patchwork/parser.py b/patchwork/parser.py index 76b89d369..0f21e0f7d 100644 --- a/patchwork/parser.py +++ b/patchwork/parser.py @@ -1277,7 +1277,9 @@ def parse_mail(mail, list_id=None): author = get_or_create_author(mail, project) - try: + with transaction.atomic(): + if Comment.objects.filter(submission=submission, msgid=msgid): + raise DuplicateMailError(msgid=msgid) comment = Comment.objects.create( submission=submission, msgid=msgid, @@ -1285,8 +1287,6 @@ def parse_mail(mail, list_id=None): headers=headers, submitter=author, content=message) - except IntegrityError: - raise DuplicateMailError(msgid=msgid) logger.debug('Comment saved') diff --git a/patchwork/tests/test_parser.py b/patchwork/tests/test_parser.py index 4041ef360..f06a6f8eb 100644 --- a/patchwork/tests/test_parser.py +++ b/patchwork/tests/test_parser.py @@ -1168,3 +1168,15 @@ def test_duplicate_patch(self): self._test_duplicate_mail(m) self.assertEqual(Patch.objects.count(), 1) + + def test_duplicate_comment(self): + diff = read_patch('0001-add-line.patch') + m1 = create_email(diff, listid=self.listid, msgid='1@example.com') + _parse_mail(m1) + + m2 = create_email('test', listid=self.listid, msgid='2@example.com', + in_reply_to='1@example.com') + self._test_duplicate_mail(m2) + + self.assertEqual(Patch.objects.count(), 1) + self.assertEqual(Comment.objects.count(), 1) From 0c74c92576a68401710fb0bbcbbf6094d9a9175c Mon Sep 17 00:00:00 2001 From: Jeremy Kerr Date: Thu, 16 Apr 2020 09:29:28 +0800 Subject: [PATCH 14/39] parser: don't trigger database IntegrityErrors on duplicate coverletters As we've done for the Patch and Comment models, this change prevents database errors from duplicate CoverLetters. Signed-off-by: Jeremy Kerr Signed-off-by: Stephen Finucane [stephenfin: Add release note] (cherry picked from commit 55fb26bf5bb3ca81ae35426efa9b2410e206c8b2) --- patchwork/parser.py | 7 ++++--- patchwork/tests/test_parser.py | 10 ++++++++++ releasenotes/notes/issue-358-7d664318a19385fa.yaml | 7 +++++++ 3 files changed, 21 insertions(+), 3 deletions(-) create mode 100644 releasenotes/notes/issue-358-7d664318a19385fa.yaml diff --git a/patchwork/parser.py b/patchwork/parser.py index 0f21e0f7d..a1c4a8b40 100644 --- a/patchwork/parser.py +++ b/patchwork/parser.py @@ -1250,7 +1250,10 @@ def parse_mail(mail, list_id=None): SeriesReference.objects.create( msgid=msgid, project=project, series=series) - try: + with transaction.atomic(): + if CoverLetter.objects.filter(project=project, msgid=msgid): + raise DuplicateMailError(msgid=msgid) + cover_letter = CoverLetter.objects.create( msgid=msgid, project=project, @@ -1259,8 +1262,6 @@ def parse_mail(mail, list_id=None): headers=headers, submitter=author, content=message) - except IntegrityError: - raise DuplicateMailError(msgid=msgid) logger.debug('Cover letter saved') diff --git a/patchwork/tests/test_parser.py b/patchwork/tests/test_parser.py index f06a6f8eb..adefac491 100644 --- a/patchwork/tests/test_parser.py +++ b/patchwork/tests/test_parser.py @@ -23,6 +23,7 @@ from patchwork.models import Patch from patchwork.models import Person from patchwork.models import State +from patchwork.models import CoverLetter from patchwork.parser import clean_subject from patchwork.parser import get_or_create_author from patchwork.parser import find_patch_content as find_content @@ -1180,3 +1181,12 @@ def test_duplicate_comment(self): self.assertEqual(Patch.objects.count(), 1) self.assertEqual(Comment.objects.count(), 1) + + def test_duplicate_coverletter(self): + m = create_email('test', listid=self.listid, msgid='1@example.com') + del m['Subject'] + m['Subject'] = '[PATCH 0/1] test cover letter' + + self._test_duplicate_mail(m) + + self.assertEqual(CoverLetter.objects.count(), 1) diff --git a/releasenotes/notes/issue-358-7d664318a19385fa.yaml b/releasenotes/notes/issue-358-7d664318a19385fa.yaml new file mode 100644 index 000000000..a0eeaa010 --- /dev/null +++ b/releasenotes/notes/issue-358-7d664318a19385fa.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + The parser module now uses an atomic select-insert when creating new patch, + cover letter and comment entries. This prevents the integrity errors from + being logged in the DB logs. + (`#358 `__) From 85dc4da5bd5e7769e43b5167e54e56a0efac3f52 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Sun, 26 Apr 2020 15:28:20 +0100 Subject: [PATCH 15/39] docker: Ignore postgres data file Signed-off-by: Stephen Finucane (cherry picked from commit 10b6162807ed76b1b56c0a57c9e55e02e88daad1) --- tools/docker/db/.gitignore | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/docker/db/.gitignore b/tools/docker/db/.gitignore index 60baa9cb8..c39b72534 100644 --- a/tools/docker/db/.gitignore +++ b/tools/docker/db/.gitignore @@ -1 +1,2 @@ -data/* +data +postdata From 4491c864bacd1bce30482a99f052b3578e7af673 Mon Sep 17 00:00:00 2001 From: Jan Remmet Date: Mon, 25 May 2020 09:03:10 +0200 Subject: [PATCH 16/39] admin: fix series query remove typo from search_fields. Signed-off-by: Jan Remmet Signed-off-by: Stephen Finucane (cherry picked from commit 7e5d2e64e2a51a7c4b789888e93788d5ad8875c8) --- patchwork/admin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patchwork/admin.py b/patchwork/admin.py index c3d45240f..b55eb431c 100644 --- a/patchwork/admin.py +++ b/patchwork/admin.py @@ -126,7 +126,7 @@ class SeriesAdmin(admin.ModelAdmin): list_filter = ('project', 'submitter') list_select_related = ('submitter', 'project') readonly_fields = ('received_total', 'received_all') - search_fields = ('submitter_name', 'submitter_email') + search_fields = ('submitter__name', 'submitter__email') exclude = ('patches', ) inlines = (PatchInline, ) From 5a84eb14dd14f0ccf1df3c2f0cf221cc43a7a2d0 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Mon, 25 May 2020 11:31:14 +0100 Subject: [PATCH 17/39] Release 2.2.2 Signed-off-by: Stephen Finucane --- patchwork/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patchwork/__init__.py b/patchwork/__init__.py index b4cb728ce..a88295926 100644 --- a/patchwork/__init__.py +++ b/patchwork/__init__.py @@ -5,7 +5,7 @@ from patchwork.version import get_latest_version -VERSION = (2, 2, 2, 'alpha', 0) +VERSION = (2, 2, 2) __version__ = get_latest_version(VERSION) From 935c1852877fcf3401c1fc325caee79d86770482 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Mon, 25 May 2020 11:33:26 +0100 Subject: [PATCH 18/39] Post-release version bump Signed-off-by: Stephen Finucane --- patchwork/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patchwork/__init__.py b/patchwork/__init__.py index a88295926..0691cf54b 100644 --- a/patchwork/__init__.py +++ b/patchwork/__init__.py @@ -5,7 +5,7 @@ from patchwork.version import get_latest_version -VERSION = (2, 2, 2) +VERSION = (2, 2, 3, 'alpha', 0) __version__ = get_latest_version(VERSION) From 128a99d3368aa61c1e4fef3373227f7ab644f2b8 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Sat, 28 Nov 2020 17:09:45 +0000 Subject: [PATCH 19/39] lib: Add update permissions to 'nobody' postgres user This user needs the ability to change some attributes of 'Patch' and 'CoverLetter' instances that are stored in the 'patchwork_submission' table, since both are are concrete subclasses of 'Submission'. Stable-only since the 'Submission model has been removed on 'master'. Signed-off-by: Stephen Finucane Suggested-by: Ali Alnubani Closes: #364 Stable-Only --- lib/sql/grant-all.postgres.sql | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/sql/grant-all.postgres.sql b/lib/sql/grant-all.postgres.sql index 56a24864d..48acf1b0b 100644 --- a/lib/sql/grant-all.postgres.sql +++ b/lib/sql/grant-all.postgres.sql @@ -73,6 +73,8 @@ GRANT INSERT, SELECT ON patchwork_coverletter, patchwork_event, patchwork_seriesreference, +TO "nobody"; +GRANT INSERT, SELECT, UPDATE ON patchwork_submission TO "nobody"; GRANT INSERT, SELECT, UPDATE, DELETE ON From 0922c8afbba59697255a4aa4f8c754120b0dbc5e Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Sat, 28 Nov 2020 17:28:33 +0000 Subject: [PATCH 20/39] lib: Drop errant comma Signed-off-by: Stephen Finucane --- lib/sql/grant-all.postgres.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/sql/grant-all.postgres.sql b/lib/sql/grant-all.postgres.sql index 48acf1b0b..427c3e8d8 100644 --- a/lib/sql/grant-all.postgres.sql +++ b/lib/sql/grant-all.postgres.sql @@ -72,7 +72,7 @@ GRANT INSERT, SELECT ON patchwork_comment, patchwork_coverletter, patchwork_event, - patchwork_seriesreference, + patchwork_seriesreference TO "nobody"; GRANT INSERT, SELECT, UPDATE ON patchwork_submission From 289f93784e320576a46006ff2641bfcc262b04f8 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Sat, 28 Nov 2020 17:32:32 +0000 Subject: [PATCH 21/39] lib: Grant SELECT on auth_user If a mail arrives with the 'X-Patchwork-Delegate' hint header, the 'patchwork.parser' script will need to index the users table to find the appropriate user. This should be okay from a security perspective since passwords are hashed and salted and the rest of the information is mostly accessible publicly via the web UI and REST API. Signed-off-by: Stephen Finucane Suggested-by: Ali Alnubani Closes: #365 (cherry picked from commit e69a2adcf50b57980d5eb0074cc72698d5cac31a) --- lib/sql/grant-all.mysql.sql | 1 + lib/sql/grant-all.postgres.sql | 1 + 2 files changed, 2 insertions(+) diff --git a/lib/sql/grant-all.mysql.sql b/lib/sql/grant-all.mysql.sql index 100cd387f..c8044c689 100644 --- a/lib/sql/grant-all.mysql.sql +++ b/lib/sql/grant-all.mysql.sql @@ -46,6 +46,7 @@ GRANT INSERT, SELECT ON patchwork_series TO 'nobody'@localhost; GRANT INSERT, SELECT ON patchwork_seriesreference TO 'nobody'@localhost; GRANT INSERT, SELECT ON patchwork_submission TO 'nobody'@localhost; GRANT INSERT, SELECT, UPDATE, DELETE ON patchwork_patchtag TO 'nobody'@localhost; +GRANT SELECT ON auth_user TO 'nobody'@localhost; GRANT SELECT ON patchwork_delegationrule TO 'nobody'@localhost; GRANT SELECT ON patchwork_project TO 'nobody'@localhost; GRANT SELECT ON patchwork_state TO 'nobody'@localhost; diff --git a/lib/sql/grant-all.postgres.sql b/lib/sql/grant-all.postgres.sql index 427c3e8d8..cac70a87b 100644 --- a/lib/sql/grant-all.postgres.sql +++ b/lib/sql/grant-all.postgres.sql @@ -84,6 +84,7 @@ GRANT INSERT, SELECT, UPDATE, DELETE ON patchwork_series TO "nobody"; GRANT SELECT ON + auth_user, patchwork_delegationrule, patchwork_project, patchwork_state, From dd19914b99171b73cd2b852cb157274613afe23b Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Sun, 4 Oct 2020 11:42:42 +0100 Subject: [PATCH 22/39] tests: Rework 'create_relation' helper This wasn't actually creating just a patch relation object - it was also creating patches, which is something we already have an explicit helper for. Clean this thing up. Signed-off-by: Stephen Finucane (cherry picked from commit 87e9f510b2670d199ab2930900c357c3ecda761d) --- patchwork/tests/api/test_relation.py | 54 ++++++++++++++++------------ patchwork/tests/utils.py | 15 +++----- 2 files changed, 36 insertions(+), 33 deletions(-) diff --git a/patchwork/tests/api/test_relation.py b/patchwork/tests/api/test_relation.py index d48c62bc5..5f8048f25 100644 --- a/patchwork/tests/api/test_relation.py +++ b/patchwork/tests/api/test_relation.py @@ -48,8 +48,8 @@ def test_no_relation(self): @utils.store_samples('relation-list') def test_list_two_patch_relation(self): - relation = create_relation(2, project=self.project) - patches = relation.patches.all() + relation = create_relation() + patches = create_patches(2, project=self.project, related=relation) # nobody resp = self.client.get(self.api_url(item=patches[0].pk)) @@ -101,8 +101,8 @@ def test_create_two_patch_relation_maintainer(self): self.assertEqual(patches[1].related, patches[0].related) def test_delete_two_patch_relation_nobody(self): - relation = create_relation(project=self.project) - patch = relation.patches.all()[0] + relation = create_relation() + patch = create_patches(2, project=self.project, related=relation)[0] self.assertEqual(PatchRelation.objects.count(), 1) @@ -112,8 +112,8 @@ def test_delete_two_patch_relation_nobody(self): @utils.store_samples('relation-delete') def test_delete_two_patch_relation_maintainer(self): - relation = create_relation(project=self.project) - patch = relation.patches.all()[0] + relation = create_relation() + patch = create_patches(2, project=self.project, related=relation)[0] self.assertEqual(PatchRelation.objects.count(), 1) @@ -145,8 +145,8 @@ def test_create_three_patch_relation(self): self.assertEqual(patches[1].related, patches[2].related) def test_delete_from_three_patch_relation(self): - relation = create_relation(3, project=self.project) - patch = relation.patches.all()[0] + relation = create_relation() + patch = create_patches(3, project=self.project, related=relation)[0] self.assertEqual(PatchRelation.objects.count(), 1) @@ -159,8 +159,9 @@ def test_delete_from_three_patch_relation(self): @utils.store_samples('relation-extend-through-new') def test_extend_relation_through_new(self): - relation = create_relation(project=self.project) - existing_patch_a = relation.patches.first() + relation = create_relation() + existing_patch_a = create_patches( + 2, project=self.project, related=relation)[0] new_patch = create_patch(project=self.project) @@ -173,8 +174,9 @@ def test_extend_relation_through_new(self): self.assertEqual(relation.patches.count(), 3) def test_extend_relation_through_old(self): - relation = create_relation(project=self.project) - existing_patch_a = relation.patches.first() + relation = create_relation() + existing_patch_a = create_patches( + 2, project=self.project, related=relation)[0] new_patch = create_patch(project=self.project) @@ -188,8 +190,9 @@ def test_extend_relation_through_old(self): self.assertEqual(relation.patches.count(), 3) def test_extend_relation_through_new_two(self): - relation = create_relation(project=self.project) - existing_patch_a = relation.patches.first() + relation = create_relation() + existing_patch_a = create_patches( + 2, project=self.project, related=relation)[0] new_patch_a = create_patch(project=self.project) new_patch_b = create_patch(project=self.project) @@ -210,8 +213,9 @@ def test_extend_relation_through_new_two(self): @utils.store_samples('relation-extend-through-old') def test_extend_relation_through_old_two(self): - relation = create_relation(project=self.project) - existing_patch_a = relation.patches.first() + relation = create_relation() + existing_patch_a = create_patches( + 2, project=self.project, related=relation)[0] new_patch_a = create_patch(project=self.project) new_patch_b = create_patch(project=self.project) @@ -232,9 +236,10 @@ def test_extend_relation_through_old_two(self): self.assertEqual(relation.patches.count(), 4) def test_remove_one_patch_from_relation_bad(self): - relation = create_relation(3, project=self.project) - keep_patch_a = relation.patches.all()[1] - keep_patch_b = relation.patches.all()[2] + relation = create_relation() + patches = create_patches(3, project=self.project, related=relation) + keep_patch_a = patches[1] + keep_patch_b = patches[1] # this should do nothing - it is interpreted as # _adding_ keep_patch_b again which is a no-op. @@ -248,8 +253,9 @@ def test_remove_one_patch_from_relation_bad(self): self.assertEqual(relation.patches.count(), 3) def test_remove_one_patch_from_relation_good(self): - relation = create_relation(3, project=self.project) - target_patch = relation.patches.all()[0] + relation = create_relation() + target_patch = create_patches( + 3, project=self.project, related=relation)[0] # maintainer self.client.force_authenticate(user=self.maintainer) @@ -263,8 +269,10 @@ def test_remove_one_patch_from_relation_good(self): @utils.store_samples('relation-forbid-moving-between-relations') def test_forbid_moving_patch_between_relations(self): """Test the break-before-make logic""" - relation_a = create_relation(project=self.project) - relation_b = create_relation(project=self.project) + relation_a = create_relation() + create_patches(2, project=self.project, related=relation_a) + relation_b = create_relation() + create_patches(2, project=self.project, related=relation_b) patch_a = relation_a.patches.first() patch_b = relation_b.patches.first() diff --git a/patchwork/tests/utils.py b/patchwork/tests/utils.py index 7759c8f37..a7151e177 100644 --- a/patchwork/tests/utils.py +++ b/patchwork/tests/utils.py @@ -297,6 +297,11 @@ def create_series_reference(**kwargs): return SeriesReference.objects.create(**values) +def create_relation(**kwargs): + """Create 'PatchRelation' object.""" + return PatchRelation.objects.create(**kwargs) + + def _create_submissions(create_func, count=1, **kwargs): """Create 'count' Submission-based objects. @@ -353,13 +358,3 @@ def create_covers(count=1, **kwargs): kwargs (dict): Overrides for various cover letter fields """ return _create_submissions(create_cover, count, **kwargs) - - -def create_relation(count_patches=2, **kwargs): - relation = PatchRelation.objects.create() - values = { - 'related': relation - } - values.update(kwargs) - create_patches(count_patches, **values) - return relation From 6d37dcf1f0c53fec8e3b943dc3dcf368099cb4fa Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Sun, 4 Oct 2020 12:03:19 +0100 Subject: [PATCH 23/39] tests: Add tests for 'patch-relation-changed' events This event is rather odd. If you have two patches then the way a relation is created is by creating a 'PatchRelation' instance and then setting the 'related' attribute on the first patch followed by the second patch. Because the event uses the 'Patch' model's 'pre_save' signal, we'll only see events for the patch being currently saved. This means no event will be raised for the first patch and only one event, the one for the second patch, will be raised when the second patch is being added to the relationship. In hindsight, the structure of the event is off. We should have had something like a 'patch-added-to-relationship' and a 'patch-removed-from-relationship' event, both with the same fields: 'project', 'actor', 'patch' and 'related', the latter of which would have listed all of the _other_ patches in the relationship. Sadly, this is an API change which means we can't do it now. We may well wish to do so in the future though. Signed-off-by: Stephen Finucane (cherry picked from commit 8092f8f4f59ea5581180a13994eb090d24394206) --- patchwork/tests/test_events.py | 47 ++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/patchwork/tests/test_events.py b/patchwork/tests/test_events.py index 415237f97..5bac77890 100644 --- a/patchwork/tests/test_events.py +++ b/patchwork/tests/test_events.py @@ -172,6 +172,53 @@ def test_patch_delegated(self): Event.CATEGORY_PATCH_DELEGATED) self.assertEventFields(events[3], previous_delegate=delegate_b) + def test_patch_relations_changed(self): + # purposefully setting series to None to minimize additional events + relation = utils.create_relation() + patches = utils.create_patches(3, series=None) + + # mark the first two patches as related; the second patch should be the + # one that the event is raised for + + patches[0].related = relation + patches[0].save() + patches[1].related = relation + patches[1].save() + + events = _get_events(patch=patches[1]) + self.assertEqual(events.count(), 2) + self.assertEqual( + events[1].category, Event.CATEGORY_PATCH_RELATION_CHANGED) + self.assertEqual(events[1].project, patches[1].project) + self.assertEqual(events[1].previous_relation, None) + self.assertEqual(events[1].current_relation, relation) + + # add the third patch + + patches[2].related = relation + patches[2].save() + + events = _get_events(patch=patches[2]) + self.assertEqual(events.count(), 2) + self.assertEqual( + events[1].category, Event.CATEGORY_PATCH_RELATION_CHANGED) + self.assertEqual(events[1].project, patches[1].project) + self.assertEqual(events[1].previous_relation, None) + self.assertEqual(events[1].current_relation, relation) + + # drop the third patch + + patches[2].related = None + patches[2].save() + + events = _get_events(patch=patches[2]) + self.assertEqual(events.count(), 3) + self.assertEqual( + events[2].category, Event.CATEGORY_PATCH_RELATION_CHANGED) + self.assertEqual(events[2].project, patches[1].project) + self.assertEqual(events[2].previous_relation, relation) + self.assertEqual(events[2].current_relation, None) + class CheckCreatedTest(_BaseTestCase): From a8492f0e2ba3ca783c1b7748ad3d9b204372acf4 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Thu, 1 Oct 2020 14:38:31 +0100 Subject: [PATCH 24/39] Remove 'PatchRelationSerializer' This wasn't writeable for reasons I haven't been able to figure out. However, it's not actually needed: the 'PatchSerializer' can do the job just fine, given enough information. This exposes a bug in DRF, which has been reported upstream [1]. While we wait for that fix, or some variant of it, to be merged, we must monkey patch the library. Conflicts: patchwork/api/patch.py NOTE(stephenfin): Conflicts are due to the absence of commit d3d4f9f36 ("Add Django 3.0 support") which we do not want to backport here. [1] https://github.com/encode/django-rest-framework/issues/7550 [2] https://github.com/encode/django-rest-framework/pull/7574 Signed-off-by: Stephen Finucane Reported-by: Ralf Ramsauer Closes: #379 Cc: Daniel Axtens Cc: Rohit Sarkar (cherry picked from commit fe07f30df6fe263938b1f898ffffc354c4ff470c) --- patchwork/api/__init__.py | 50 +++++++++++++++++++++++++++++++++++++++ patchwork/api/embedded.py | 28 +--------------------- patchwork/api/event.py | 7 +++--- patchwork/api/patch.py | 22 ++++++++--------- 4 files changed, 65 insertions(+), 42 deletions(-) diff --git a/patchwork/api/__init__.py b/patchwork/api/__init__.py index e69de29bb..efc0dd890 100644 --- a/patchwork/api/__init__.py +++ b/patchwork/api/__init__.py @@ -0,0 +1,50 @@ +# Patchwork - automated patch tracking system +# Copyright (C) 2020, Stephen Finucane +# +# SPDX-License-Identifier: GPL-2.0-or-later + +from rest_framework.fields import empty +from rest_framework.fields import get_attribute +from rest_framework.fields import SkipField +from rest_framework.relations import ManyRelatedField + + +# monkey patch django-rest-framework to work around issue #7550 [1] until #7574 +# [2] or some other variant lands +# +# [1] https://github.com/encode/django-rest-framework/issues/7550 +# [2] https://github.com/encode/django-rest-framework/pull/7574 + +def _get_attribute(self, instance): + # Can't have any relationships if not created + if hasattr(instance, 'pk') and instance.pk is None: + return [] + + try: + relationship = get_attribute(instance, self.source_attrs) + except (KeyError, AttributeError) as exc: + if self.default is not empty: + return self.get_default() + if self.allow_null: + return None + if not self.required: + raise SkipField() + msg = ( + 'Got {exc_type} when attempting to get a value for field ' + '`{field}` on serializer `{serializer}`.\nThe serializer ' + 'field might be named incorrectly and not match ' + 'any attribute or key on the `{instance}` instance.\n' + 'Original exception text was: {exc}.'.format( + exc_type=type(exc).__name__, + field=self.field_name, + serializer=self.parent.__class__.__name__, + instance=instance.__class__.__name__, + exc=exc + ) + ) + raise type(exc)(msg) + + return relationship.all() if hasattr(relationship, 'all') else relationship + + +ManyRelatedField.get_attribute = _get_attribute diff --git a/patchwork/api/embedded.py b/patchwork/api/embedded.py index cb3f07e6b..ca799efe7 100644 --- a/patchwork/api/embedded.py +++ b/patchwork/api/embedded.py @@ -12,9 +12,8 @@ from collections import OrderedDict from rest_framework.serializers import CharField -from rest_framework.serializers import SerializerMethodField from rest_framework.serializers import PrimaryKeyRelatedField -from rest_framework.serializers import ValidationError +from rest_framework.serializers import SerializerMethodField from patchwork.api.base import BaseHyperlinkedModelSerializer from patchwork.api.base import CheckHyperlinkedIdentityField @@ -139,31 +138,6 @@ class Meta: } -class PatchRelationSerializer(BaseHyperlinkedModelSerializer): - """Hide the PatchRelation model, just show the list""" - patches = PatchSerializer(many=True, - style={'base_template': 'input.html'}) - - def to_internal_value(self, data): - if not isinstance(data, type([])): - raise ValidationError( - "Patch relations must be specified as a list of patch IDs" - ) - result = super(PatchRelationSerializer, self).to_internal_value( - {'patches': data} - ) - return result - - def to_representation(self, instance): - data = super(PatchRelationSerializer, self).to_representation(instance) - data = data['patches'] - return data - - class Meta: - model = models.PatchRelation - fields = ('patches',) - - class PersonSerializer(SerializedRelatedField): class _Serializer(BaseHyperlinkedModelSerializer): diff --git a/patchwork/api/event.py b/patchwork/api/event.py index d7a99c7df..951786767 100644 --- a/patchwork/api/event.py +++ b/patchwork/api/event.py @@ -13,7 +13,6 @@ from patchwork.api.embedded import CheckSerializer from patchwork.api.embedded import CoverLetterSerializer from patchwork.api.embedded import PatchSerializer -from patchwork.api.embedded import PatchRelationSerializer from patchwork.api.embedded import ProjectSerializer from patchwork.api.embedded import SeriesSerializer from patchwork.api.embedded import UserSerializer @@ -34,8 +33,10 @@ class EventSerializer(ModelSerializer): current_delegate = UserSerializer() created_check = SerializerMethodField() created_check = CheckSerializer() - previous_relation = PatchRelationSerializer(read_only=True) - current_relation = PatchRelationSerializer(read_only=True) + previous_relation = PatchSerializer( + source='previous_relation.patches', many=True, default=None) + current_relation = PatchSerializer( + source='current_relation.patches', many=True, default=None) _category_map = { Event.CATEGORY_COVER_CREATED: ['cover'], diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py index 15fce8efd..548d2c262 100644 --- a/patchwork/api/patch.py +++ b/patchwork/api/patch.py @@ -10,7 +10,6 @@ from django.core.exceptions import ValidationError from django.utils.text import slugify from django.utils.translation import ugettext_lazy as _ -from rest_framework import status from rest_framework.exceptions import APIException from rest_framework.exceptions import PermissionDenied from rest_framework.generics import ListAPIView @@ -18,15 +17,16 @@ from rest_framework.relations import RelatedField from rest_framework.reverse import reverse from rest_framework.serializers import SerializerMethodField +from rest_framework import status from patchwork.api.base import BaseHyperlinkedModelSerializer from patchwork.api.base import PatchworkPermission -from patchwork.api.filters import PatchFilterSet -from patchwork.api.embedded import PatchRelationSerializer +from patchwork.api.embedded import PatchSerializer from patchwork.api.embedded import PersonSerializer from patchwork.api.embedded import ProjectSerializer from patchwork.api.embedded import SeriesSerializer from patchwork.api.embedded import UserSerializer +from patchwork.api.filters import PatchFilterSet from patchwork.models import Patch from patchwork.models import PatchRelation from patchwork.models import State @@ -83,7 +83,8 @@ class PatchListSerializer(BaseHyperlinkedModelSerializer): check = SerializerMethodField() checks = SerializerMethodField() tags = SerializerMethodField() - related = PatchRelationSerializer() + related = PatchSerializer( + source='related.patches', many=True, default=[]) def get_web_url(self, instance): request = self.context.get('request') @@ -127,14 +128,11 @@ def to_representation(self, instance): data = super(PatchListSerializer, self).to_representation(instance) data['series'] = [data['series']] if data['series'] else [] - # stop the related serializer returning this patch in the list of - # related patches. Also make it return an empty list, not null/None - if 'related' in data: - if data['related']: - data['related'] = [p for p in data['related'] - if p['id'] != instance.id] - else: - data['related'] = [] + # Remove this patch from 'related' + if 'related' in data and data['related']: + data['related'] = [ + x for x in data['related'] if x['id'] != data['id'] + ] return data From 1ea8b10264f66833da921055947f8431cf831179 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Sun, 29 Nov 2020 12:50:22 +0000 Subject: [PATCH 25/39] REST: Null out previous, current relation info These fields don't work like we expect them to. Because we're linking to a non-idempotent entity, an instance of 'relation', what we're storing in either of these fields is subject to change as patches are added and removed. This makes the information pretty much useless after the fact. It's best to just state the patch and request that people query the information themselves if necessary. We don't want to remove the field entirely from the API - that would be a truly breaking change - so instead we null it out like we do for patch tags. In a v2 API (i.e. a major version bump) we can remove this entirely. A small bug with the schema generation is corrected. Signed-off-by: Stephen Finucane Related: #379 (cherry picked from commit 646a2f2c80056a33c70efd760446832f90899247) --- docs/api/schemas/patchwork.j2 | 4 +++- docs/api/schemas/v1.1/patchwork.yaml | 18 ------------------ docs/api/schemas/v1.2/patchwork.yaml | 2 ++ patchwork/api/event.py | 22 ++++++++++++++-------- patchwork/signals.py | 8 +++----- patchwork/tests/test_events.py | 12 ++++++------ 6 files changed, 28 insertions(+), 38 deletions(-) diff --git a/docs/api/schemas/patchwork.j2 b/docs/api/schemas/patchwork.j2 index f5618d41f..af15df1ce 100644 --- a/docs/api/schemas/patchwork.j2 +++ b/docs/api/schemas/patchwork.j2 @@ -1821,7 +1821,7 @@ components: current_state: title: Current state type: string -{% if version >= (1, 1) %} +{% if version >= (1, 2) %} EventPatchRelationChanged: allOf: - $ref: '#/components/schemas/EventBase' @@ -1837,9 +1837,11 @@ components: previous_relation: title: Previous relation type: string + nullable: true current_relation: title: Current relation type: string + nullable: true {% endif %} EventPatchDelegated: allOf: diff --git a/docs/api/schemas/v1.1/patchwork.yaml b/docs/api/schemas/v1.1/patchwork.yaml index 6b497abaf..babc972ac 100644 --- a/docs/api/schemas/v1.1/patchwork.yaml +++ b/docs/api/schemas/v1.1/patchwork.yaml @@ -1551,24 +1551,6 @@ components: current_state: title: Current state type: string - EventPatchRelationChanged: - allOf: - - $ref: '#/components/schemas/EventBase' - - type: object - properties: - category: - enum: - - patch-relation-changed - payload: - properties: - patch: - $ref: '#/components/schemas/PatchEmbedded' - previous_relation: - title: Previous relation - type: string - current_relation: - title: Current relation - type: string EventPatchDelegated: allOf: - $ref: '#/components/schemas/EventBase' diff --git a/docs/api/schemas/v1.2/patchwork.yaml b/docs/api/schemas/v1.2/patchwork.yaml index 7bdbe6699..db60c789c 100644 --- a/docs/api/schemas/v1.2/patchwork.yaml +++ b/docs/api/schemas/v1.2/patchwork.yaml @@ -1768,9 +1768,11 @@ components: previous_relation: title: Previous relation type: string + nullable: true current_relation: title: Current relation type: string + nullable: true EventPatchDelegated: allOf: - $ref: '#/components/schemas/EventBase' diff --git a/patchwork/api/event.py b/patchwork/api/event.py index 951786767..bc1bda463 100644 --- a/patchwork/api/event.py +++ b/patchwork/api/event.py @@ -33,10 +33,8 @@ class EventSerializer(ModelSerializer): current_delegate = UserSerializer() created_check = SerializerMethodField() created_check = CheckSerializer() - previous_relation = PatchSerializer( - source='previous_relation.patches', many=True, default=None) - current_relation = PatchSerializer( - source='current_relation.patches', many=True, default=None) + previous_relation = SerializerMethodField() + current_relation = SerializerMethodField() _category_map = { Event.CATEGORY_COVER_CREATED: ['cover'], @@ -53,6 +51,12 @@ class EventSerializer(ModelSerializer): Event.CATEGORY_SERIES_COMPLETED: ['series'], } + def get_previous_relation(self, instance): + return None + + def get_current_relation(self, instance): + return None + def to_representation(self, instance): data = super(EventSerializer, self).to_representation(instance) payload = OrderedDict() @@ -72,10 +76,12 @@ def to_representation(self, instance): class Meta: model = Event - fields = ('id', 'category', 'project', 'date', 'actor', 'patch', - 'series', 'cover', 'previous_state', 'current_state', - 'previous_delegate', 'current_delegate', 'created_check', - 'previous_relation', 'current_relation',) + fields = ( + 'id', 'category', 'project', 'date', 'actor', 'patch', + 'series', 'cover', 'previous_state', 'current_state', + 'previous_delegate', 'current_delegate', 'created_check', + 'previous_relation', 'current_relation', + ) read_only_fields = fields versioned_fields = { '1.2': ('actor', ), diff --git a/patchwork/signals.py b/patchwork/signals.py index 3a2f0fbdd..0771104a4 100644 --- a/patchwork/signals.py +++ b/patchwork/signals.py @@ -137,14 +137,12 @@ def create_event(patch, before, after): @receiver(pre_save, sender=Patch) def create_patch_relation_changed_event(sender, instance, raw, **kwargs): - def create_event(patch, before, after): + def create_event(patch): return Event.objects.create( category=Event.CATEGORY_PATCH_RELATION_CHANGED, project=patch.project, actor=getattr(patch, '_edited_by', None), - patch=patch, - previous_relation=before, - current_relation=after) + patch=patch) # don't trigger for items loaded from fixtures or new items if raw or not instance.pk: @@ -155,7 +153,7 @@ def create_event(patch, before, after): if orig_patch.related == instance.related: return - create_event(instance, orig_patch.related, instance.related) + create_event(instance) @receiver(pre_save, sender=Patch) diff --git a/patchwork/tests/test_events.py b/patchwork/tests/test_events.py index 5bac77890..090b6dc01 100644 --- a/patchwork/tests/test_events.py +++ b/patchwork/tests/test_events.py @@ -190,8 +190,8 @@ def test_patch_relations_changed(self): self.assertEqual( events[1].category, Event.CATEGORY_PATCH_RELATION_CHANGED) self.assertEqual(events[1].project, patches[1].project) - self.assertEqual(events[1].previous_relation, None) - self.assertEqual(events[1].current_relation, relation) + self.assertIsNone(events[1].previous_relation) + self.assertIsNone(events[1].current_relation) # add the third patch @@ -203,8 +203,8 @@ def test_patch_relations_changed(self): self.assertEqual( events[1].category, Event.CATEGORY_PATCH_RELATION_CHANGED) self.assertEqual(events[1].project, patches[1].project) - self.assertEqual(events[1].previous_relation, None) - self.assertEqual(events[1].current_relation, relation) + self.assertIsNone(events[1].previous_relation) + self.assertIsNone(events[1].current_relation) # drop the third patch @@ -216,8 +216,8 @@ def test_patch_relations_changed(self): self.assertEqual( events[2].category, Event.CATEGORY_PATCH_RELATION_CHANGED) self.assertEqual(events[2].project, patches[1].project) - self.assertEqual(events[2].previous_relation, relation) - self.assertEqual(events[2].current_relation, None) + self.assertIsNone(events[2].previous_relation) + self.assertIsNone(events[2].current_relation) class CheckCreatedTest(_BaseTestCase): From 8dc0a75f7be28ce7aa0a4f5fab5c0af689a9003a Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Sun, 13 Dec 2020 18:34:47 +0000 Subject: [PATCH 26/39] Add release note for #379 Signed-off-by: Stephen Finucane (cherry picked from commit 4c8154d178b63a77048638323b0171116426112e) --- releasenotes/notes/issue-379-7b518d15eb0276c1.yaml | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 releasenotes/notes/issue-379-7b518d15eb0276c1.yaml diff --git a/releasenotes/notes/issue-379-7b518d15eb0276c1.yaml b/releasenotes/notes/issue-379-7b518d15eb0276c1.yaml new file mode 100644 index 000000000..6d92e9512 --- /dev/null +++ b/releasenotes/notes/issue-379-7b518d15eb0276c1.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Resolve a bug that would prevent listing patches for a project via the + browseable API view when logged in with admin permissions (`issue #379`__) + + __ https://github.com/getpatchwork/patchwork/issues/379 From e3a3b4703d157a38504b82d64551749cc939cf62 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Sun, 13 Dec 2020 19:05:14 +0000 Subject: [PATCH 27/39] Release 2.2.3 Signed-off-by: Stephen Finucane --- patchwork/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patchwork/__init__.py b/patchwork/__init__.py index 0691cf54b..2514fe730 100644 --- a/patchwork/__init__.py +++ b/patchwork/__init__.py @@ -5,7 +5,7 @@ from patchwork.version import get_latest_version -VERSION = (2, 2, 3, 'alpha', 0) +VERSION = (2, 2, 3) __version__ = get_latest_version(VERSION) From 5c03cbe0de1b7805089d54c961c165abafe9e5f5 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Sun, 13 Dec 2020 19:06:03 +0000 Subject: [PATCH 28/39] Post-release version bump Signed-off-by: Stephen Finucane --- patchwork/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patchwork/__init__.py b/patchwork/__init__.py index 2514fe730..ac6af88f4 100644 --- a/patchwork/__init__.py +++ b/patchwork/__init__.py @@ -5,7 +5,7 @@ from patchwork.version import get_latest_version -VERSION = (2, 2, 3) +VERSION = (2, 2, 4, 'alpha', 0) __version__ = get_latest_version(VERSION) From b170a825993b2a840e3477c1470811d563746f2c Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Sat, 20 Feb 2021 14:35:07 +0000 Subject: [PATCH 29/39] models: Use parent model to get comment's 'list_archive_url' We were attempting to retrieve the 'list_archive_url' attribute from the 'PatchComment' or 'CoverComment' instances, rather than the parent 'Patch' and 'Cover' object, respectively. Correct this and add plenty of tests to prevent this regressing. Conflicts: patchwork/models.py Changes: patchwork/tests/api/test_comment.py NOTE(stephenfin): Conflicts and changes are necessary to deal with the fact we have a single comment model instead of two comment models as in stable/3.0. Signed-off-by: Stephen Finucane Fixes: 02ffb1315 ("models: Add list archive lookup") Closes: #391 (cherry picked from commit 93ff4db29262c0560122f61eadf78d9626def238) (cherry picked from commit 6c73a55c52ecd6d756901c9e9647fae57aceda01) --- patchwork/models.py | 12 ++++-- patchwork/tests/api/test_comment.py | 24 +++++++++++ patchwork/tests/api/test_project.py | 42 ++++++++++++++++++- patchwork/tests/utils.py | 2 + .../notes/issue-391-4088c856247f228e.yaml | 6 +++ 5 files changed, 82 insertions(+), 4 deletions(-) create mode 100644 releasenotes/notes/issue-391-4088c856247f228e.yaml diff --git a/patchwork/models.py b/patchwork/models.py index e295e1736..a3c34da7e 100644 --- a/patchwork/models.py +++ b/patchwork/models.py @@ -380,10 +380,13 @@ class Submission(FilenameMixin, EmailMixin, models.Model): def list_archive_url(self): if not self.project.list_archive_url_format: return None + if not self.msgid: return None + return self.project.list_archive_url_format.format( - self.url_msgid) + self.url_msgid, + ) # patchwork metadata @@ -640,10 +643,13 @@ class Comment(EmailMixin, models.Model): def list_archive_url(self): if not self.submission.project.list_archive_url_format: return None + if not self.msgid: return None - return self.project.list_archive_url_format.format( - self.url_msgid) + + return self.submission.project.list_archive_url_format.format( + self.url_msgid, + ) def get_absolute_url(self): return reverse('comment-redirect', kwargs={'comment_id': self.id}) diff --git a/patchwork/tests/api/test_comment.py b/patchwork/tests/api/test_comment.py index f48bfce1a..e95205629 100644 --- a/patchwork/tests/api/test_comment.py +++ b/patchwork/tests/api/test_comment.py @@ -53,6 +53,18 @@ def test_list(self): self.assertEqual(status.HTTP_200_OK, resp.status_code) self.assertEqual(1, len(resp.data)) self.assertSerialized(comment, resp.data[0]) + self.assertIn('list_archive_url', resp.data[0]) + + def test_list_version_1_1(self): + """List cover letter comments using API v1.1.""" + cover = create_cover() + comment = create_comment(submission=cover) + + resp = self.client.get(self.api_url(cover, version='1.1')) + self.assertEqual(status.HTTP_200_OK, resp.status_code) + self.assertEqual(1, len(resp.data)) + self.assertSerialized(comment, resp.data[0]) + self.assertNotIn('list_archive_url', resp.data[0]) def test_list_version_1_0(self): """List cover letter comments using API v1.0.""" @@ -104,6 +116,18 @@ def test_list(self): self.assertEqual(status.HTTP_200_OK, resp.status_code) self.assertEqual(1, len(resp.data)) self.assertSerialized(comment, resp.data[0]) + self.assertIn('list_archive_url', resp.data[0]) + + def test_list_version_1_1(self): + """List patch comments using API v1.1.""" + patch = create_patch() + comment = create_comment(submission=patch) + + resp = self.client.get(self.api_url(patch, version='1.1')) + self.assertEqual(status.HTTP_200_OK, resp.status_code) + self.assertEqual(1, len(resp.data)) + self.assertSerialized(comment, resp.data[0]) + self.assertNotIn('list_archive_url', resp.data[0]) def test_list_version_1_0(self): """List patch comments using API v1.0.""" diff --git a/patchwork/tests/api/test_project.py b/patchwork/tests/api/test_project.py index 5a7676740..701a4ed04 100644 --- a/patchwork/tests/api/test_project.py +++ b/patchwork/tests/api/test_project.py @@ -71,6 +71,26 @@ def test_list_authenticated(self): self.assertEqual(status.HTTP_200_OK, resp.status_code) self.assertEqual(1, len(resp.data)) self.assertSerialized(project, resp.data[0]) + self.assertIn('subject_match', resp.data[0]) + self.assertIn('list_archive_url', resp.data[0]) + self.assertIn('list_archive_url_format', resp.data[0]) + self.assertIn('commit_url_format', resp.data[0]) + + @utils.store_samples('project-list-1.1') + def test_list_version_1_1(self): + """List projects using API v1.1. + + Validate that newer fields are dropped for older API versions. + """ + create_project() + + resp = self.client.get(self.api_url(version='1.1')) + self.assertEqual(status.HTTP_200_OK, resp.status_code) + self.assertEqual(1, len(resp.data)) + self.assertIn('subject_match', resp.data[0]) + self.assertNotIn('list_archive_url', resp.data[0]) + self.assertNotIn('list_archive_url_format', resp.data[0]) + self.assertNotIn('commit_url_format', resp.data[0]) @utils.store_samples('project-list-1.0') def test_list_version_1_0(self): @@ -86,7 +106,7 @@ def test_list_version_1_0(self): self.assertNotIn('subject_match', resp.data[0]) @utils.store_samples('project-detail') - def test_detail_by_id(self): + def test_detail(self): """Show project using ID lookup. Validate that it's possible to filter by pk. @@ -96,6 +116,10 @@ def test_detail_by_id(self): resp = self.client.get(self.api_url(project.pk)) self.assertEqual(status.HTTP_200_OK, resp.status_code) self.assertSerialized(project, resp.data) + self.assertIn('subject_match', resp.data) + self.assertIn('list_archive_url', resp.data) + self.assertIn('list_archive_url_format', resp.data) + self.assertIn('commit_url_format', resp.data) def test_detail_by_linkname(self): """Show project using linkname lookup. @@ -119,6 +143,22 @@ def test_detail_by_numeric_linkname(self): self.assertEqual(status.HTTP_200_OK, resp.status_code) self.assertSerialized(project, resp.data) + @utils.store_samples('project-detail-1.1') + def test_detail_version_1_1(self): + """Show project using API v1.1. + + Validate that newer fields are dropped for older API versions. + """ + project = create_project() + + resp = self.client.get(self.api_url(project.pk, version='1.1')) + self.assertEqual(status.HTTP_200_OK, resp.status_code) + self.assertIn('name', resp.data) + self.assertIn('subject_match', resp.data) + self.assertNotIn('list_archive_url', resp.data) + self.assertNotIn('list_archive_url_format', resp.data) + self.assertNotIn('commit_url_format', resp.data) + @utils.store_samples('project-detail-1.0') def test_detail_version_1_0(self): """Show project using API v1.0. diff --git a/patchwork/tests/utils.py b/patchwork/tests/utils.py index a7151e177..2cb0d8aa2 100644 --- a/patchwork/tests/utils.py +++ b/patchwork/tests/utils.py @@ -60,6 +60,8 @@ def create_project(**kwargs): 'listid': 'test%d.example.com' % num, 'listemail': 'test%d@example.com' % num, 'subject_match': '', + 'list_archive_url': 'https://lists.example.com/', + 'list_archive_url_format': 'https://lists.example.com/mail/{}', } values.update(kwargs) diff --git a/releasenotes/notes/issue-391-4088c856247f228e.yaml b/releasenotes/notes/issue-391-4088c856247f228e.yaml new file mode 100644 index 000000000..597902b63 --- /dev/null +++ b/releasenotes/notes/issue-391-4088c856247f228e.yaml @@ -0,0 +1,6 @@ +--- +api: + - | + The ``list_archive_url`` field will now be correctly shown for patch + comments and cover letter comments. + (`#391 `__) From 65a392dff4df9e9538fdfe7bee2e4c63e3da113f Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Mon, 22 Feb 2021 09:23:34 +0000 Subject: [PATCH 30/39] requirements: Cap pyrsistent on Python 2 I can't reproduce this locally, but it's breaking Travis. Signed-off-by: Stephen Finucane --- requirements-test.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/requirements-test.txt b/requirements-test.txt index bddc37d3b..ed2e5aa68 100644 --- a/requirements-test.txt +++ b/requirements-test.txt @@ -2,4 +2,5 @@ mysqlclient~=1.4.4 # pyup: >=1.4.4,<1.5.0 psycopg2-binary~=2.8.0 # pyup: >=2.8.0,<2.9.0 sqlparse~=0.3.0 # pyup: >=0.3.0,<0.4.0 python-dateutil~=2.8.0 # pyup: >=2.8.0,<2.9.0 +pyrsistent<0.16.0; python_version < '3.0' # pyup: ignore openapi-core~=0.8.0 # pyup: >=0.8.0,<0.9.0 From 864c71a735ca82dc2e9c9c2c6f9eab0fae4a3d1b Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 24 Feb 2021 15:08:45 +0000 Subject: [PATCH 31/39] api: do not fetch every patch in a patch detail view 404 (v2) Commit 08c5856 fixed an issue whereby a 404 on the aforementioned URL could result in a large DB query due to DRF attempting to populate the 'related' list box with all patches on the instance. That was accidentally reverted in commit fe07f30. "Unrevert" this change. Signed-off-by: Stephen Finucane Fixes: fe07f30 ("Remove 'PatchRelationSerializer'") Closes: #397 (cherry picked from commit 79700f321335a2d7c649eb03932797af521942a0) --- patchwork/api/patch.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py index 548d2c262..c94e2a7df 100644 --- a/patchwork/api/patch.py +++ b/patchwork/api/patch.py @@ -84,7 +84,8 @@ class PatchListSerializer(BaseHyperlinkedModelSerializer): checks = SerializerMethodField() tags = SerializerMethodField() related = PatchSerializer( - source='related.patches', many=True, default=[]) + source='related.patches', many=True, default=[], + style={'base_template': 'input.html'}) def get_web_url(self, instance): request = self.context.get('request') From 4e4f74b221f4d7b75841e1a28cab5a6b3da993ec Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 24 Feb 2021 15:32:53 +0000 Subject: [PATCH 32/39] Release 2.2.4 Signed-off-by: Stephen Finucane --- patchwork/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patchwork/__init__.py b/patchwork/__init__.py index ac6af88f4..d24e8835e 100644 --- a/patchwork/__init__.py +++ b/patchwork/__init__.py @@ -5,7 +5,7 @@ from patchwork.version import get_latest_version -VERSION = (2, 2, 4, 'alpha', 0) +VERSION = (2, 2, 4) __version__ = get_latest_version(VERSION) From c99a933d6f13b6d97583609a8d90d581f6880957 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 24 Feb 2021 15:33:26 +0000 Subject: [PATCH 33/39] Post-release version bump Signed-off-by: Stephen Finucane --- patchwork/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patchwork/__init__.py b/patchwork/__init__.py index d24e8835e..9459f54a2 100644 --- a/patchwork/__init__.py +++ b/patchwork/__init__.py @@ -5,7 +5,7 @@ from patchwork.version import get_latest_version -VERSION = (2, 2, 4) +VERSION = (2, 2, 5, 'alpha', 0) __version__ = get_latest_version(VERSION) From 49d0acc1e228d88cca68c78f7dce25c1d247732c Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Sat, 6 Mar 2021 14:54:59 +0000 Subject: [PATCH 34/39] Only show unique checks Commit e5c641fc4 optimized fetching of checks when displaying a patch by prefetching these checks ahead of time. Unfortunately we missed that this should exclude older versions of checks for a given context. Make the code used to generate the unique checks generic and allow us to use that as a filter to the checks provided to the template, restoring the correct behavior. Conflicts: patchwork/tests/test_detail.py NOTE(stephenfin): Conflicts are due to some differences in imports, however, we also need to drop some usages of f-strings since Patchwork 2.x supported Python 2.x also. We also need to specify an explicit decode value for some strings since Python 2.x defaults to 'ascii', not 'utf-8'. Signed-off-by: Stephen Finucane Closes: #398 Fixes: e5c641fc4 ("Optimise fetching checks when displaying a patch") (cherry picked from commit a43d6acda0939c14e8135549968815876680d497) (cherry picked from commit f4ff605eb449ea56b265ee7a5a516b48d83a5eb0) --- patchwork/models.py | 90 ++++++++++++++++++---------------- patchwork/tests/test_detail.py | 38 ++++++++++++++ patchwork/views/patch.py | 4 +- 3 files changed, 89 insertions(+), 43 deletions(-) diff --git a/patchwork/models.py b/patchwork/models.py index a3c34da7e..028beb3c1 100644 --- a/patchwork/models.py +++ b/patchwork/models.py @@ -516,50 +516,13 @@ def is_editable(self, user): return True return False - @property - def combined_check_state(self): - """Return the combined state for all checks. - - Generate the combined check's state for this patch. This check - is one of the following, based on the value of each unique - check: - - * failure, if any context's latest check reports as failure - * warning, if any context's latest check reports as warning - * pending, if there are no checks, or a context's latest - Check reports as pending - * success, if latest checks for all contexts reports as - success - """ - state_names = dict(Check.STATE_CHOICES) - states = [check.state for check in self.checks] - - if not states: - return state_names[Check.STATE_PENDING] - - for state in [Check.STATE_FAIL, Check.STATE_WARNING, - Check.STATE_PENDING]: # order sensitive - if state in states: - return state_names[state] - - return state_names[Check.STATE_SUCCESS] - - @property - def checks(self): - """Return the list of unique checks. - - Generate a list of checks associated with this patch for each - type of Check. Only "unique" checks are considered, - identified by their 'context' field. This means, given n - checks with the same 'context', the newest check is the only - one counted regardless of its value. The end result will be a - association of types to number of unique checks for said - type. - """ + @staticmethod + def filter_unique_checks(checks): + """Filter the provided checks to generate the unique list.""" unique = {} duplicates = [] - for check in self.check_set.all(): + for check in checks: ctx = check.context user = check.user_id @@ -586,7 +549,50 @@ def checks(self): # prefetch_related.) So, do it 'by hand' in Python. We can # also be confident that this won't be worse, seeing as we've # just iterated over self.check_set.all() *anyway*. - return [c for c in self.check_set.all() if c.id not in duplicates] + return [c for c in checks if c.id not in duplicates] + + @property + def checks(self): + """Return the list of unique checks. + + Generate a list of checks associated with this patch for each + type of Check. Only "unique" checks are considered, + identified by their 'context' field. This means, given n + checks with the same 'context', the newest check is the only + one counted regardless of its value. The end result will be a + association of types to number of unique checks for said + type. + """ + return self.filter_unique_checks(self.check_set.all()) + + @property + def combined_check_state(self): + """Return the combined state for all checks. + + Generate the combined check's state for this patch. This check + is one of the following, based on the value of each unique + check: + + * failure, if any context's latest check reports as failure + * warning, if any context's latest check reports as warning + * pending, if there are no checks, or a context's latest check reports + as pending + * success, if latest checks for all contexts reports as success + """ + state_names = dict(Check.STATE_CHOICES) + states = [check.state for check in self.checks] + + if not states: + return state_names[Check.STATE_PENDING] + + # order sensitive + for state in ( + Check.STATE_FAIL, Check.STATE_WARNING, Check.STATE_PENDING, + ): + if state in states: + return state_names[state] + + return state_names[Check.STATE_SUCCESS] @property def check_count(self): diff --git a/patchwork/tests/test_detail.py b/patchwork/tests/test_detail.py index 92fe2d7e6..326e27bca 100644 --- a/patchwork/tests/test_detail.py +++ b/patchwork/tests/test_detail.py @@ -3,13 +3,19 @@ # # SPDX-License-Identifier: GPL-2.0-or-later +from datetime import datetime as dt +from datetime import timedelta + from django.test import TestCase from django.urls import reverse +from patchwork.models import Check +from patchwork.tests.utils import create_check from patchwork.tests.utils import create_comment from patchwork.tests.utils import create_cover from patchwork.tests.utils import create_patch from patchwork.tests.utils import create_project +from patchwork.tests.utils import create_user class CoverLetterViewTest(TestCase): @@ -156,6 +162,38 @@ def test_invalid_patch_id(self): response = self.client.get(requested_url) self.assertEqual(response.status_code, 404) + def test_patch_with_checks(self): + user = create_user() + patch = create_patch() + check_a = create_check( + patch=patch, user=user, context='foo', state=Check.STATE_FAIL, + date=(dt.utcnow() - timedelta(days=1))) + create_check( + patch=patch, user=user, context='foo', state=Check.STATE_SUCCESS) + check_b = create_check( + patch=patch, user=user, context='bar', state=Check.STATE_PENDING) + requested_url = reverse( + 'patch-detail', + kwargs={ + 'project_id': patch.project.linkname, + 'msgid': patch.url_msgid, + }, + ) + response = self.client.get(requested_url) + + # the response should contain checks + self.assertContains(response, '

Checks

') + + # and it should only show the unique checks + self.assertEqual( + 1, response.content.decode('utf-8').count( + '%s/%s' % (check_a.user, check_a.context) + )) + self.assertEqual( + 1, response.content.decode('utf-8').count( + '%s/%s' % (check_b.user, check_b.context) + )) + class CommentRedirectTest(TestCase): diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py index 470ad1973..34934dea6 100644 --- a/patchwork/views/patch.py +++ b/patchwork/views/patch.py @@ -123,7 +123,9 @@ def patch_detail(request, project_id, msgid): related_different_project = [] context['comments'] = comments - context['checks'] = patch.check_set.all().select_related('user') + context['checks'] = Patch.filter_unique_checks( + patch.check_set.all().select_related('user'), + ) context['submission'] = patch context['patchform'] = form context['createbundleform'] = createbundleform From 6beb3107c0c4e54eaec31d5a0c102a29d2b8d718 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Mon, 26 Jul 2021 17:57:01 +0100 Subject: [PATCH 35/39] Release 2.2.5 Signed-off-by: Stephen Finucane --- patchwork/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patchwork/__init__.py b/patchwork/__init__.py index 9459f54a2..8071bb090 100644 --- a/patchwork/__init__.py +++ b/patchwork/__init__.py @@ -5,7 +5,7 @@ from patchwork.version import get_latest_version -VERSION = (2, 2, 5, 'alpha', 0) +VERSION = (2, 2, 5) __version__ = get_latest_version(VERSION) From d1c6ea117e375600744ed61332b0d1dbf50a9610 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Mon, 26 Jul 2021 17:57:18 +0100 Subject: [PATCH 36/39] Post-release version bump Signed-off-by: Stephen Finucane --- patchwork/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patchwork/__init__.py b/patchwork/__init__.py index 8071bb090..601756c37 100644 --- a/patchwork/__init__.py +++ b/patchwork/__init__.py @@ -5,7 +5,7 @@ from patchwork.version import get_latest_version -VERSION = (2, 2, 5) +VERSION = (2, 2, 6, 'alpha', 0) __version__ = get_latest_version(VERSION) From 1d860982a170e7c7218375f812fe3e23032759e4 Mon Sep 17 00:00:00 2001 From: Daniel Axtens Date: Sat, 21 Aug 2021 00:57:58 +1000 Subject: [PATCH 37/39] REST: Don't error if a versioned field we would remove is absent We remove fields that shouldn't be seen on old versions of the API. This was done with `pop(field name)`, which will throw an exception if the named field is absent from the data. However, sometimes if a patch request is via an old API version, we hit this line without ever having the field present. This is odd, but not harmful and we definitely shouldn't 500. Signed-off-by: Daniel Axtens [stephenfin: Merge test into bug fix] Signed-off-by: Stephen Finucane Tested-by: Konstantin Ryabitsev Fixes: d944f17ec059 ("REST: Use versioning for modified responses") Closes: #423 (cherry picked from commit 5c22f9d8fcc72deec7d8beaefc97207ea8fc646d) (cherry picked from commit 956f988a47c9c10c5141bc0209dc58916e983ca2) --- patchwork/api/base.py | 5 ++++- patchwork/tests/api/test_patch.py | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/patchwork/api/base.py b/patchwork/api/base.py index 89a43114f..6cb703b12 100644 --- a/patchwork/api/base.py +++ b/patchwork/api/base.py @@ -96,6 +96,9 @@ def to_representation(self, instance): # field was added, we drop it if not utils.has_version(request, version): for field in self.Meta.versioned_fields[version]: - data.pop(field) + # After a PATCH with an older API version, we may not see + # these fields. If they don't exist, don't panic, return + # (and then discard) None. + data.pop(field, None) return data diff --git a/patchwork/tests/api/test_patch.py b/patchwork/tests/api/test_patch.py index da2dd6e90..b94ad2290 100644 --- a/patchwork/tests/api/test_patch.py +++ b/patchwork/tests/api/test_patch.py @@ -334,6 +334,20 @@ def test_update_maintainer(self): self.assertEqual(status.HTTP_200_OK, resp.status_code, resp) self.assertIsNone(Patch.objects.get(id=patch.id).delegate) + def test_update_maintainer_version_1_0(self): + """Update patch as maintainer on v1.1.""" + project = create_project() + patch = create_patch(project=project) + state = create_state() + user = create_maintainer(project) + + self.client.force_authenticate(user=user) + resp = self.client.patch(self.api_url(patch.id, version="1.1"), + {'state': state.slug, 'delegate': user.id}) + self.assertEqual(status.HTTP_200_OK, resp.status_code, resp) + self.assertEqual(Patch.objects.get(id=patch.id).state, state) + self.assertEqual(Patch.objects.get(id=patch.id).delegate, user) + @utils.store_samples('patch-update-error-bad-request') def test_update_invalid_state(self): """Update patch with invalid fields. From 97f7adee9d5c1e0ff5c57e73ae7f82479f7d8f58 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 27 Oct 2021 11:55:54 +0100 Subject: [PATCH 38/39] Release 2.2.6 Signed-off-by: Stephen Finucane --- patchwork/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patchwork/__init__.py b/patchwork/__init__.py index 601756c37..51d808168 100644 --- a/patchwork/__init__.py +++ b/patchwork/__init__.py @@ -5,7 +5,7 @@ from patchwork.version import get_latest_version -VERSION = (2, 2, 6, 'alpha', 0) +VERSION = (2, 2, 6) __version__ = get_latest_version(VERSION) From d357c267637a4d9858f95ff815d54bdb0b9417ad Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 27 Oct 2021 11:56:54 +0100 Subject: [PATCH 39/39] Post-release version bump Signed-off-by: Stephen Finucane --- patchwork/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patchwork/__init__.py b/patchwork/__init__.py index 51d808168..0ed62d3ca 100644 --- a/patchwork/__init__.py +++ b/patchwork/__init__.py @@ -5,7 +5,7 @@ from patchwork.version import get_latest_version -VERSION = (2, 2, 6) +VERSION = (2, 2, 7, 'alpha', 0) __version__ = get_latest_version(VERSION)