Stephen Finucane <step...@that.guru> writes:
> On Sat, 2018-09-22 at 01:34 +1000, Daniel Axtens wrote:
>> Stephen Finucane <step...@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 <step...@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@lists.ozlabs.org
>> > https://lists.ozlabs.org/listinfo/patchwork
_______________________________________________
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
