diff mbox series

api: support filtering patches by hash

Message ID 20191023143342.12300-1-dja@axtens.net
State Accepted
Headers show
Series api: support filtering patches by hash | expand

Commit Message

Daniel Axtens Oct. 23, 2019, 2:33 p.m. UTC 
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

Comments

Konstantin Ryabitsev Oct. 23, 2019, 2:40 p.m. UTC | #1 
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
Daniel Axtens Nov. 6, 2019, 1:44 p.m. UTC | #2 
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
Stephen Finucane Nov. 7, 2019, 1:42 a.m. UTC | #3 
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
Daniel Axtens Nov. 7, 2019, 4:09 a.m. UTC | #4 
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 mbox series

Patch

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.