fix review_types argument of the API's Branch.createMergeProposal method

[Markus Korn]

The reviewers and the review_types argument of the Branch.createMergeProposal method must have the same length as zip() is used to generate the review_requests argument for addLandingTarget. Right now a ValueError is raised if this is not the case. However this ValueError is not well exposed over the API, it just appears as a 500 Internal Error with no additional explaination. On the other hand this behavior is different to the behavior of the web interface, where the review type argument is optional.

To fix this bug there are tow possible solutions:
 * make the types optional too, by appending emtpy strings to the review_types list like
      review_types += [""]*(len(reviewers) - len(review_types))
 * or expose the ValueError as a 400 Bad Request with some verbose information

Current error representation:

In [7]: b.createMergeProposal (target_branch=trunk, reviewers=[launchpad.me.self_link,])
---------------------------------------------------------------------------
HTTPError Traceback (most recent call last)

/media/devel/ilaunchpad-shell/trunk/<ipython console> in <module>()

/usr/lib/python2.6/dist-packages/lazr/restfulclient/resource.pyc in __call__(self, *args, **kwargs)
    458 extra_headers = { 'Content-type' : media_type }
    459 response, content = self.root._browser._request(
--> 460 url, in_representation, http_method, extra_headers=extra_headers)
    461
    462 if response.status == 201:

/usr/lib/python2.6/dist-packages/lazr/restfulclient/_browser.pyc in _request(self, url, data, method, media_type, extra_headers)
    202 # Turn non-2xx responses into exceptions.

    203 if response.status // 100 != 2:
--> 204 raise HTTPError(response, content)
    205 return response, content
    206

HTTPError: HTTP Error 500: Internal Server Error
---------------------------------------------------------------------------
Response: {'connection': 'close',
 'content-length': '10',
 'content-type': 'text/plain',
 'date': 'Thu, 11 Feb 2010 11:38:19 GMT',
 'server': 'zope.server.http (HTTP)',
 'status': '500',
 'via': '1.1 wildcard.staging.launchpad.net',
 'x-lazr-oopsid': 'OOPS-1503S125',
 'x-powered-by': 'Zope (www.zope.org), Python (www.python.org)'}
Content: 'ValueError'
---------------------------------------------------------------------------

I'm happy to fix this bug, if someone tells me what the "correct" solutions would look like

[Tim Penhey (thumper)]

Markus,

I think that the API exposed method should do detailed checking, and give a verbose error message if it is bad. The only thing I'm not sure on is how to make it not OOPS, but instead to return a known error.

[Markus Korn (thekorn)]

I started working on it in the attached branch.

@Tim: as far as I understand you last comment the solution would be to just expose the ValueError as a "HTTP 400 Bad Request"-error with some verbose information. So there won't be an empty string as default for each review_type

However there is still an issue with the attached solution, the testcase is failing, the error is still handled as an internal server error, so I need to investigate further.

[Leonard Richardson (leonardr)]

Hey, Markus, are you still working on this? If not, I may be able to take it off your hands.

[Robert Collins (lifeless)]

Unassigning : the branch from @markus may be useful, but its been a year since they took it on - its clearly stalled.

[Robert Collins (lifeless)]

@Markus - sorry we didn't get back to you about this --much-- earlier.

[Leonard Richardson (leonardr)]

The code in Markus' branch was correct, but it didn't work due to a bug in lazr.restful that prevented webservice_error from working. The bug was fixed recently. I've ported Markus' branch to a modern Launchpad branch, and changed his doctest to a unit test.

[Andrew Bennetts (spiv)]

For some reason this bug is now affecting the UDD package importer, e.g. OOPS-1910A1064. The list of failing imports is: <http://package-import.ubuntu.com/status/7ae30b44f1f904e04bbae6700b7181dd.html>. Currently 51 imports are affected.

[Robert Collins (lifeless)]

n Mon, Mar 28, 2011 at 12:41 PM, Andrew Bennetts
<email address hidden> wrote:
> For some reason this bug is now affecting the UDD package importer, e.g.
> OOPS-1910A1064.  The list of failing imports is: <http://package-
> import.ubuntu.com/status/7ae30b44f1f904e04bbae6700b7181dd.html>.
> Currently 51 imports are affected.

My first guess would be a bug in the package importer, based on the
defect fixed here.

[Launchpad QA Bot (lpqabot)]

Fixed in stable r13031 <http://bazaar.launchpad.net/~launchpad-pqm/launchpad/stable/revision/13031>.