| Message ID | 20180910223712.5100-6-stephen@that.guru |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Convert Series-Patch relationship to 1:N | expand |
Stephen Finucane <stephen@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@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?" I would say that the answer to that question is "No, if you specify ?series=<literally anything>", you should get the whole series. Therefore, I'd make the whole thing simpler by dropping the 0/false thing and just returning the series if ?series is present. 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
On Sat, 2018-09-22 at 01:34 +1000, Daniel Axtens wrote: > Stephen Finucane <stephen@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@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? 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
Stephen Finucane <stephen@that.guru> writes: > On Sat, 2018-09-22 at 01:34 +1000, Daniel Axtens wrote: >> Stephen Finucane <stephen@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@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
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:
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@that.guru> --- TODO: I'm not sure if I should do this or introduce a new parameter (maybe 'dependencies'?). Thoughts? --- patchwork/tests/test_mboxviews.py | 17 +++++++++++++++++ patchwork/views/patch.py | 2 +- patchwork/views/utils.py | 2 +- 3 files changed, 19 insertions(+), 2 deletions(-)