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

Fri Feb 3 08:42:52 AEDT 2017

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.

OK, between you and some more testing-oriented co-workers I'm now
convinced. Lets provide an API to get patch + dependencies (and lets go
with that nomenclature), and a separate endpoint to get an entire
series. I suspect from a UI point of view we need only expose the series
download, but we can bikeshed that later on in the series. :)

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

Excellent question. (Also one we're going to have to answer for
dependencies!)

I think silently returning a partial series is probably not wise - I can
imagine some tools (and people) getting very confused. I think maybe
raise an error and fail: if people want to download a known-incomplete
series the existing bundle functionality should work nicely. 

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

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

>> > +        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.
>
Excellent!

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

Cool, [2] is a good start and thanks for tracking it at [3].

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

You're welcome - thanks for writing it!

Regards,
Daniel
>
> 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