[PATCH v2 5/5] views: Add support for boolean 'series' parameters

[Daniel Axtens]

Stephen Finucane <stephen at that.guru> writes:

> On Sat, 2018-09-22 at 01:34 +1000, Daniel Axtens wrote:
>> Stephen Finucane <stephen at that.guru> writes:
>> 
>> > Previously, we allowed users to download patch mboxes with dependencies
>> > included using a 'series' parameter. This accepted either a numeric ID,
>> > corresponding to the ID of the patch series that dependencies should be
>> > included from, or a wildcard value ('*').
>> > 
>> >     /patch/{patchID}/mbox/?series=123
>> >     /patch/{patchID}/mbox/?series=*
>> > 
>> > With switch to a 1:N series-patch relationship, this is clearly
>> > unnecessary now but must be retained to avoid breaking users. However,
>> > that doesn't mean we can't things a little clearer. Add support for
>> > boolean parameters, which make more sense for this kind of relationship:
>> > 
>> >   /patch/{patchID}/mbox/?series=true
>> >   /patch/{patchID}/mbox/?series=1
>> >   /patch/{patchID}/mbox/?series=false
>> >   /patch/{patchID}/mbox/?series=0
>> > 
>> > Signed-off-by: Stephen Finucane <stephen at that.guru>
>> > ---
>> > TODO: I'm not sure if I should do this or introduce a new parameter
>> > (maybe 'dependencies'?). Thoughts?
>> 
>> So my question is:
>> 
>>   "Previously, specifying a wrong series number would lead to getting
>>    no patches. Is it a problem if specifying a wrong series number now
>>    leads to you getting the series patches?"
>
> "No patches" might be a bit misleading: it would lead you to getting a
> HTTP 404.
>
>> I would say that the answer to that question is "No, if you specify
>> ?series=<literally anything>", you should get the whole series.
>
> The concern I have here is that we'd encode bad behaviour in clients.
> If '?series=6' is used on a Patchwork 2.0 or 2.1 instance where the
> given patch is associated with series 8, they would see an error on the
> older versions but not the newer ones. As a result, I've continued to
> check this behaviour. It's also the reason why I'm thinking it would be
> best to move away from this parameter altogether.
>
>> Therefore, I'd make the whole thing simpler by dropping the 0/false
>> thing and just returning the series if ?series is present.
>
> Hmm, that makes sense. Looking at various RFCs, it seems there's no
> reason arguments need to be key-value pairs. That being said, given
> that behaviour has changed, do we want to use a new parameter to
> highlight this fact?
>
If the feature had existed for a little longer or if there were a
multiplicity of tools that used it, sure, but I think we can bend the
rules here and get away with it.

But I certainly wouldn't NAK a patch that did otherwise if you'd prefer
to keep things a bit more rigorously backwards compatible.

Regards,
Daniel

> Stephen
>
>> Regards,
>> Daniel
>> 
>> > ---
>> >  patchwork/tests/test_mboxviews.py | 17 +++++++++++++++++
>> >  patchwork/views/patch.py          |  2 +-
>> >  patchwork/views/utils.py          |  2 +-
>> >  3 files changed, 19 insertions(+), 2 deletions(-)
>> > 
>> > diff --git a/patchwork/tests/test_mboxviews.py b/patchwork/tests/test_mboxviews.py
>> > index 2add3950..9d941bf8 100644
>> > --- a/patchwork/tests/test_mboxviews.py
>> > +++ b/patchwork/tests/test_mboxviews.py
>> > @@ -237,6 +237,23 @@ class MboxSeriesDependencies(TestCase):
>> >          self.assertContains(response, patch_a.content)
>> >          self.assertContains(response, patch_b.content)
>> >  
>> > +    def test_patch_with_boolean_series(self):
>> > +        _, patch_a, patch_b = self._create_patches()
>> > +
>> > +        for value in ('true', '1'):
>> > +            response = self.client.get('%s?series=%s' % (
>> > +                reverse('patch-mbox', args=[patch_b.id]), value))
>> > +
>> > +            self.assertContains(response, patch_a.content)
>> > +            self.assertContains(response, patch_b.content)
>> > +
>> > +        for value in ('false', '0'):
>> > +            response = self.client.get('%s?series=%s' % (
>> > +                reverse('patch-mbox', args=[patch_b.id]), value))
>> > +
>> > +            self.assertNotContains(response, patch_a.content)
>> > +            self.assertContains(response, patch_b.content)
>> > +
>> >      def test_patch_with_invalid_series(self):
>> >          series, patch_a, patch_b = self._create_patches()
>> >  
>> > diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py
>> > index 9fed4312..91b98e38 100644
>> > --- a/patchwork/views/patch.py
>> > +++ b/patchwork/views/patch.py
>> > @@ -144,7 +144,7 @@ def patch_mbox(request, patch_id):
>> >      series_id = request.GET.get('series')
>> >  
>> >      response = HttpResponse(content_type='text/plain')
>> > -    if series_id:
>> > +    if series_id and series_id.lower() not in ('false', '0'):
>> >          if not patch.series:
>> >              raise Http404('Patch does not have an associated series. This is '
>> >                            'because the patch was processed with an older '
>> > diff --git a/patchwork/views/utils.py b/patchwork/views/utils.py
>> > index bfcf6aba..4644c621 100644
>> > --- a/patchwork/views/utils.py
>> > +++ b/patchwork/views/utils.py
>> > @@ -141,7 +141,7 @@ def series_patch_to_mbox(patch, series_id):
>> >      Returns:
>> >          A string for the mbox file.
>> >      """
>> > -    if series_id != '*':
>> > +    if series_id.lower() not in ('*', 'true', '1'):
>> >          try:
>> >              series_id = int(series_id)
>> >          except ValueError:
>> > -- 
>> > 2.17.1
>> > 
>> > _______________________________________________
>> > Patchwork mailing list
>> > Patchwork at lists.ozlabs.org
>> > https://lists.ozlabs.org/listinfo/patchwork