[PATCH 4/6] views: Add 'series' parameter to '/mbox' endpoint

[Stephen Finucane]

On Thu, 2017-02-02 at 17:47 +1100, Daniel Axtens wrote:
> Hi Stephen,
> 
> > This allows a user to download dependencies for a patch without
> > having
> > to do it manually.
> 
> I'm not sold on the idea of calling these dependencies and allowing
> downloads of partial series. It just seems to present the user with
> an
> dazzling array of ways to shoot themselves in the foot - for example
> I
> can imagine a lot of users clicking at the start of a series or
> midway
> into a series and downloading an mbox, not realising that if they
> want
> the entire series of patches they need to explicitly find the final
> patch and download the mbox from there... What was your rationale or
> inteded use-case for this? (I'm happier to expose it programatically,
> I
> guess, it's just more complexity we have to carry around...)

While I have this exposed in the UI in a later patch, this feature is
mainly intended for use in testing. I'm of the opinion that when
testing patches, you should test each and every one individually and
preserve bisectability. This is an approach rooted in my experiences
with Gerrit, where individual changes are king and are evaluated on
their own merits. This contrasts with the approach favoured by tools
such as GitHub, where the individual changes are abstracted in favour
of an overall "pull request".

I do think "dependencies" is the correct term to use here, and I would
like to be able to download a patch from the middle of a series with
all patches below it included for use in testing at least. However,
this has been a concern for other folks too and there are clear
benefits to both approaches. Perhaps we could provide a solution to
both problems by also providing a series mbox download URL? For
example:

    /patch/5/?series=1
    /series/1/mbox/

In this case, the first link will download patch 5 plus any
dependencies from series 1 (ignoring patch 6+), while the latter will
download the entirety of series 1. We could expose both in the UI, or
we could expose just the full series link.

The one question here is what do we do if a series isn't complete?
Proceed anyway? Raise an error and fail? Raise an error but provide a
'force' parameter to force the mbox download?

>   
> >  def patch_to_mbox(patch):
> > +    """Get an mbox representation of a single patch.
> > +
> > +    Arguments:
> > +        patch: The Patch object to convert.
> > +
> > +    Returns:
> > +        A string for the mbox file.
> > +    """
> 
> Yay for docstrings, but maybe you should mention them in the
> commit message, or even put them in a patch with other cleanups?

Yes, this should have been a separate patch. Let's do that for v2.

> >      postscript_re = re.compile('\n-{2,3} ?\n')
> >      body = ''
> >  
> > +
> > +def series_patch_to_mbox(patch, series_num):
> > +    """Get an mbox representation of a patch with dependencies.
> > +
> > +    Arguments:
> > +        patch: The Patch object to convert.
> > +        series_num: The series number to retrieve dependencies
> > from.
> > +
> > +    Returns:
> > +        A string for the mbox file.
> > +    """
> > +    try:
> > +        series_num = int(series_num)
> > +    except ValueError:
> > +        # TODO(stephenfin): Perhaps we should raise a HTTP code
> > here?
> 
> Yes, we should - silently failing is not a great way to go.
> I think both this case and the series not found should probably both
> return a 404, preferably with a friendly message of some sort?
> (see
> e.g. http://stackoverflow.com/questions/20922543/correct-http-status-
> code-for-existent-resource-but-non-existent-entity )

Agreed. Let me try this again.

> > +        return ''
> > +
> > +    try:
> > +        series = patch.series.get(id=series_num)
> > +    except Series.DoesNotExist:
> > +        return ''
> > +
> > +    mbox = []
> > +
> > +    # get the series-ified patch
> > +    number = series.seriespatch_set.get(patch=patch).number
> > +    for dep in series.seriespatch_set.filter(number__lt=number):
> > +        mbox.append(patch_to_mbox(dep.patch))
> 
> Are these guaranteed to be ordered even in the event of out of order
> arrival? I honestly can't remember - I think they are?

Yup, series patches are ordered by number [1] and will appear as such.

> > +
> > +    mbox.append(patch_to_mbox(patch))
> > +
> > +    return '\n'.join(mbox)
> 
> 
> >  def patch_mbox(request, patch_id):
> >      patch = get_object_or_404(Patch, id=patch_id)
> > -    response = HttpResponse(content_type="text/plain")
> > -    response.write(patch_to_mbox(patch))
> > +    series_num = request.GET.get('series')
> > +
> > +    response = HttpResponse(content_type='text/plain')
> > +    if series_num:
> > +        response.write(series_patch_to_mbox(patch, series_num))
> > +    else:
> > +        response.write(patch_to_mbox(patch))
> >      response['Content-Disposition'] = 'attachment; filename=' + \
> >          patch.filename.replace(';', '').replace('\n', '')
> 
> I notice we don't sanitise bundle names the same way we sanitise this
> filename (see one of your previous patches). Should we? Are the name
> characters for bundles arbitrary?

I think this is somewhat handled by the BundleForm, which will at least
prevent anyone creating bundles with slashes [2]. However, it could
definitely warrant more attention. I've created a bug [3] to track
investigation here.

As always, thanks for taking the time to look over this :)

Stephen

[1] https://github.com/getpatchwork/patchwork/blob/c1d8a0d/patchwork/mo
dels.py#L668
[2] https://github.com/getpatchwork/patchwork/blob/c1d8a0d/patchwork/fo
rms.py#L72-L74
[3] https://github.com/getpatchwork/patchwork/issues/75