Message ID | 20191023143342.12300-1-dja@axtens.net |
---|---|
State | Accepted |
Headers | show |
Series | api: support filtering patches by hash | expand |
On Thu, Oct 24, 2019 at 01:33:42AM +1100, Daniel Axtens wrote: > This is a feature that the XML-RPC API has, and which is used in > the wild [1], so support it in the REST API. Nice! Thank you for this. > I tried to version the new filter field, but it's not at all clear > how to do this with django-filters. The best way I could find > requires manually manipulating request.GET, which seems to defeat > the point of django-filters. So document it for 1.2, and have it > work on older versions as an undocumented feature. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/mricon/korg-helpers.git/tree/git-patchwork-bot.py?id=104e7374e1be8458e6d2e82478625a7bf8c822ff > > Cc: Konstantin Ryabitsev <konstantin@linuxfoundation.org> > Signed-off-by: Daniel Axtens <dja@axtens.net> Acked-by: Konstantin Ryabitsev <konstantin@linuxfoundation.org> -K
Hi Stephen, > 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. > + # 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') > + 'state', 'archived', 'hash') > Just hoping to get your Ack on this part of the patch, in case you had some Deep Magic that could achieve your goals of API purity. Regards, Daniel > > class CheckFilterSet(TimestampMixin, FilterSet): > diff --git patchwork/tests/api/test_patch.py patchwork/tests/api/test_patch.py > index edae9851c422..4afc15a9ba1c 100644 > --- patchwork/tests/api/test_patch.py > +++ patchwork/tests/api/test_patch.py > @@ -22,6 +22,13 @@ from patchwork.tests.utils import create_user > if settings.ENABLE_REST_API: > from rest_framework import status > > +# a diff different from the default, required to test hash filtering > +SAMPLE_DIFF = """--- /dev/null\t2019-01-01 00:00:00.000000000 +0800 > ++++ a\t2019-01-01 00:00:00.000000000 +0800 > +@@ -0,0 +1 @@ > ++b > +""" > + > > @unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API') > class TestPatchAPI(utils.APITestCase): > @@ -152,6 +159,35 @@ class TestPatchAPI(utils.APITestCase): > 'submitter': 'test@example.org'}) > self.assertEqual(0, len(resp.data)) > > + def test_list_filter_hash(self): > + """Filter patches by hash.""" > + patch = self._create_patch() > + patch_new_diff = create_patch(state=patch.state, project=patch.project, > + submitter=patch.submitter, > + diff=SAMPLE_DIFF) > + > + # check regular filtering > + resp = self.client.get(self.api_url(), {'hash': patch.hash}) > + self.assertEqual([patch.id], [x['id'] for x in resp.data]) > + > + # 2 patches with identical diffs > + patch_same_diff = create_patch(state=patch.state, > + project=patch.project, > + submitter=patch.submitter) > + resp = self.client.get(self.api_url(), {'hash': patch.hash}) > + self.assertEqual([patch.id, patch_same_diff.id], > + [x['id'] for x in resp.data]) > + > + # case insensitive matching > + resp = self.client.get(self.api_url(), > + {'hash': patch_new_diff.hash.upper()}) > + self.assertEqual([patch_new_diff.id], [x['id'] for x in resp.data]) > + > + # empty response if nothing matches > + resp = self.client.get(self.api_url(), { > + 'hash': 'da638d0746a115000bf890fada1f02679aa282e8'}) > + 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 releasenotes/notes/rest-filter-hash-e031bd3db42eb540.yaml releasenotes/notes/rest-filter-hash-e031bd3db42eb540.yaml > new file mode 100644 > index 000000000000..b805931d4d80 > --- /dev/null > +++ releasenotes/notes/rest-filter-hash-e031bd3db42eb540.yaml > @@ -0,0 +1,5 @@ > +--- > +api: > + - | > + The REST API now supports filtering patches by their hashes, using the > + ``hash`` query parameter. > -- > 2.20.1
On Thu, 2019-11-07 at 00:44 +1100, Daniel Axtens wrote: > Hi Stephen, > > > 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. > > + # 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') > > + 'state', 'archived', 'hash') > > > > Just hoping to get your Ack on this part of the patch, in case you had > some Deep Magic that could achieve your goals of API purity. It's definitely possible, but I'm at the OpenInfra Summit this week so I won't have time to complete it. Is next week okay? You could merge this as-is and I could fix before we cut v2.2, if you'd like? Stephen > Regards, > Daniel
Stephen Finucane <stephen@that.guru> writes: > On Thu, 2019-11-07 at 00:44 +1100, Daniel Axtens wrote: >> Hi Stephen, >> >> > 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. >> > + # 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') >> > + 'state', 'archived', 'hash') >> > >> >> Just hoping to get your Ack on this part of the patch, in case you had >> some Deep Magic that could achieve your goals of API purity. > > It's definitely possible, but I'm at the OpenInfra Summit this week so > I won't have time to complete it. Is next week okay? You could merge > this as-is and I could fix before we cut v2.2, if you'd like? I'm in no rush. I'd like to get 2.2 out sometime in November, but beyond that I'm not fussy. I think there might be some shared logic between this and versioning the order-by-date stuff, so why don't we wait and handle them both when you're back on deck fully. Regards, Daniel > > Stephen > >> Regards, >> Daniel
diff --git docs/api/schemas/latest/patchwork.yaml docs/api/schemas/latest/patchwork.yaml index 46969000a65b..6c7564baf81b 100644 --- docs/api/schemas/latest/patchwork.yaml +++ docs/api/schemas/latest/patchwork.yaml @@ -463,6 +463,13 @@ paths: enum: - 'true' - 'false' + - in: query + name: hash + description: > + The patch hash as a case-insensitive hexadecimal string, to filter by. + schema: + title: '' + type: string responses: '200': description: '' diff --git docs/api/schemas/patchwork.j2 docs/api/schemas/patchwork.j2 index 4fc100eb4a92..12a6f67de32b 100644 --- docs/api/schemas/patchwork.j2 +++ docs/api/schemas/patchwork.j2 @@ -468,6 +468,15 @@ paths: enum: - 'true' - 'false' +{% if version >= (1, 2) %} + - in: query + name: hash + description: > + The patch hash as a case-insensitive hexadecimal string, to filter by. + schema: + title: '' + type: string +{% endif %} responses: '200': description: '' diff --git docs/api/schemas/v1.2/patchwork.yaml docs/api/schemas/v1.2/patchwork.yaml index 2ced470b7dc0..7dc95793faa3 100644 --- docs/api/schemas/v1.2/patchwork.yaml +++ docs/api/schemas/v1.2/patchwork.yaml @@ -463,6 +463,13 @@ paths: enum: - 'true' - 'false' + - in: query + name: hash + description: > + The patch hash as a case-insensitive hexadecimal string, to filter by. + schema: + title: '' + type: string responses: '200': description: '' diff --git patchwork/api/filters.py patchwork/api/filters.py index 37aca82d9ab2..4184ee8253f0 100644 --- patchwork/api/filters.py +++ patchwork/api/filters.py @@ -7,6 +7,7 @@ from django.contrib.auth.models import User from django.core.exceptions import ValidationError from django.db.models import Q from django_filters.rest_framework import FilterSet +from django_filters import CharFilter from django_filters import IsoDateTimeFilter from django_filters import ModelMultipleChoiceFilter from django.forms import ModelMultipleChoiceField as BaseMultipleChoiceField @@ -176,11 +177,16 @@ class PatchFilterSet(TimestampMixin, FilterSet): submitter = PersonFilter(queryset=Person.objects.all()) delegate = UserFilter(queryset=User.objects.all()) state = StateFilter(queryset=State.objects.all()) + hash = CharFilter(lookup_expr='iexact') 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. + # 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') + 'state', 'archived', 'hash') class CheckFilterSet(TimestampMixin, FilterSet): diff --git patchwork/tests/api/test_patch.py patchwork/tests/api/test_patch.py index edae9851c422..4afc15a9ba1c 100644 --- patchwork/tests/api/test_patch.py +++ patchwork/tests/api/test_patch.py @@ -22,6 +22,13 @@ from patchwork.tests.utils import create_user if settings.ENABLE_REST_API: from rest_framework import status +# a diff different from the default, required to test hash filtering +SAMPLE_DIFF = """--- /dev/null\t2019-01-01 00:00:00.000000000 +0800 ++++ a\t2019-01-01 00:00:00.000000000 +0800 +@@ -0,0 +1 @@ ++b +""" + @unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API') class TestPatchAPI(utils.APITestCase): @@ -152,6 +159,35 @@ class TestPatchAPI(utils.APITestCase): 'submitter': 'test@example.org'}) self.assertEqual(0, len(resp.data)) + def test_list_filter_hash(self): + """Filter patches by hash.""" + patch = self._create_patch() + patch_new_diff = create_patch(state=patch.state, project=patch.project, + submitter=patch.submitter, + diff=SAMPLE_DIFF) + + # check regular filtering + resp = self.client.get(self.api_url(), {'hash': patch.hash}) + self.assertEqual([patch.id], [x['id'] for x in resp.data]) + + # 2 patches with identical diffs + patch_same_diff = create_patch(state=patch.state, + project=patch.project, + submitter=patch.submitter) + resp = self.client.get(self.api_url(), {'hash': patch.hash}) + self.assertEqual([patch.id, patch_same_diff.id], + [x['id'] for x in resp.data]) + + # case insensitive matching + resp = self.client.get(self.api_url(), + {'hash': patch_new_diff.hash.upper()}) + self.assertEqual([patch_new_diff.id], [x['id'] for x in resp.data]) + + # empty response if nothing matches + resp = self.client.get(self.api_url(), { + 'hash': 'da638d0746a115000bf890fada1f02679aa282e8'}) + 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 releasenotes/notes/rest-filter-hash-e031bd3db42eb540.yaml releasenotes/notes/rest-filter-hash-e031bd3db42eb540.yaml new file mode 100644 index 000000000000..b805931d4d80 --- /dev/null +++ releasenotes/notes/rest-filter-hash-e031bd3db42eb540.yaml @@ -0,0 +1,5 @@ +--- +api: + - | + The REST API now supports filtering patches by their hashes, using the + ``hash`` query parameter.
This is a feature that the XML-RPC API has, and which is used in the wild [1], so support it in the REST API. I tried to version the new filter field, but it's not at all clear how to do this with django-filters. The best way I could find requires manually manipulating request.GET, which seems to defeat the point of django-filters. So document it for 1.2, and have it work on older versions as an undocumented feature. [1] https://git.kernel.org/pub/scm/linux/kernel/git/mricon/korg-helpers.git/tree/git-patchwork-bot.py?id=104e7374e1be8458e6d2e82478625a7bf8c822ff Cc: Konstantin Ryabitsev <konstantin@linuxfoundation.org> Signed-off-by: Daniel Axtens <dja@axtens.net> --- docs/api/schemas/latest/patchwork.yaml | 7 ++++ docs/api/schemas/patchwork.j2 | 9 +++++ docs/api/schemas/v1.2/patchwork.yaml | 7 ++++ patchwork/api/filters.py | 8 ++++- patchwork/tests/api/test_patch.py | 36 +++++++++++++++++++ .../rest-filter-hash-e031bd3db42eb540.yaml | 5 +++ 6 files changed, 71 insertions(+), 1 deletion(-) create mode 100644 releasenotes/notes/rest-filter-hash-e031bd3db42eb540.yaml