[2/2] 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 <mpe@ellerman.id.au>
Signed-off-by: Daniel Axtens <dja@axtens.net>
---
 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

Hi Daniel,

> 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.

LGTM: https://patchwork.ozlabs.org/api/patches/?msgid=20200414062102.6798-3-dja@axtens.net

Tested-by: Jeremy Kerr <jk@ozlabs.org>

Cheers,


Jeremy

On 14/4/20 4:21 pm, Daniel Axtens wrote:
> 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 <mpe@ellerman.id.au>
> Signed-off-by: Daniel Axtens <dja@axtens.net>

This is indeed something we would expect to be able to do in the API now 
that we support lookup by message ID for the web.

Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>

> ---
>   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 13cdc9cd78fd..cc0d97e696b6 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 bd714d5e7a2a..f5618d41faa0 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 db2ed122eec2..7bdbe66997c0 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 deb5ace11880..93e6281bf5e6 100644
> --- a/patchwork/api/filters.py
> +++ b/patchwork/api/filters.py
> @@ -184,6 +184,10 @@ class SeriesFilterSet(TimestampMixin, BaseFilterSet):
>           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)
> @@ -192,6 +196,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
> @@ -210,17 +215,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 5eeb1902e1d1..1b19ded1b4d5 100644
> --- a/patchwork/tests/api/test_cover.py
> +++ b/patchwork/tests/api/test_cover.py
> @@ -111,6 +111,18 @@ class TestCoverLetterAPI(utils.APITestCase):
>               '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 b24c5ab28947..da2dd6e9084b 100644
> --- a/patchwork/tests/api/test_patch.py
> +++ b/patchwork/tests/api/test_patch.py
> @@ -199,6 +199,18 @@ class TestPatchAPI(utils.APITestCase):
>                                  {'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 000000000000..0fcbbeb8a736
> --- /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.
>

On Tue, 2020-04-14 at 16:21 +1000, Daniel Axtens wrote:
> 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 <mpe@ellerman.id.au>
> Signed-off-by: Daniel Axtens <dja@axtens.net>

I meant to do this before cutting v2.2.0 and forgot. Probably should
have had a list.

It's not ideal putting this into an already released API version, but I
agree it's a huge oversight. Let's make sure we get v2.2.1 out nice and
quickly.

Reviewed-by: Stephen Finucane <stephen@that.guru>

Stephen

Stephen Finucane <stephen@that.guru> writes:

> On Tue, 2020-04-14 at 16:21 +1000, Daniel Axtens wrote:
>> 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 <mpe@ellerman.id.au>
>> Signed-off-by: Daniel Axtens <dja@axtens.net>
>
> I meant to do this before cutting v2.2.0 and forgot. Probably should
> have had a list.
>
> It's not ideal putting this into an already released API version, but I
> agree it's a huge oversight. Let's make sure we get v2.2.1 out nice and
> quickly.

Indeed, doing this now.

Thanks!

Regards,
Daniel

>
> Reviewed-by: Stephen Finucane <stephen@that.guru>
>
> Stephen

Daniel Axtens <dja@axtens.net> writes:
> 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 <mpe@ellerman.id.au>
> Signed-off-by: Daniel Axtens <dja@axtens.net>

Thanks, this is awesome.

For reference here's my updated terrible script that I use to apply a
series from a patch id or message id.

cheers

#!/usr/bin/python3

import sys
import requests
from subprocess import check_call

session = requests.Session()

arg = sys.argv[1]

try:
    pid = int(arg)
except ValueError:
    pid = None

if pid is None:
    arg = arg.replace('id:', '')
    url = f'https://patchwork.ozlabs.org/api/patches/?msgid={arg}&project=linuxppc-dev'
    json = session.get(url).json()[0]
else:
    url = f'https://patchwork.ozlabs.org/api/patches/{pid}/'
    json = session.get(url).json()

sid = json['series'][0]['id']

cmd = f'git pw series apply {sid}'
check_call(cmd.split())
print("OK")