launchpad-dev team mailing list archive

["Francis J. Lacoste" <francis.lacoste@xxxxxxxxxxxxx>]

On 11-07-14 09:23 AM, Graham Binns wrote:
> Hi folks,
> 
> A Request-For-Answers here. I'm currently working on fixing bug 657004
> ("can't read bug.linked_branches via REST API when it contains a
> private branch"). The most efficient solution I've been able to come
> up with (with lots of help and advice from Rob) adds a getter,
> getVisibleLinkedBranches(user) to IBug, which returns all the linked
> branches visible to `user`. This is now exposed in the API (in my
> branch [1]) as IBug.linked_branches, replacing the existing collection
> attribute.
> 
> I didn't expect this to work, but with the following declaration:
> 
>     @call_with(user=REQUEST_USER)
>     @export_operation_as('linked_branches')
>     @export_read_operation()
>     @operation_for_version('devel')
>     def getVisibleLinkedBranches(user):
>         """Rertun the linked to this bug that are visible by `user`."""
> 
> The following launchpadlib call works perfectly:
> 
>     >>> linked_branches = lp.bug[12345].linked_branches
> 
> (I'd expected that I'd need to add parens after the call because it's
> become a named get instead of a property on the bug. Launchpadlib
> appears to do magic, which is slightly worrying).

I wouldn't have expected this to work either. Did you remove the
exported() around the IBug.linked_branches collection field? If not,
it's possible that we end up with two different declaration for
linked_branches in the WADL (or an arbitrary one). In all cases, that's
not a good situation to be in.

> 
> The problem that I now have is that although for launchpadlib users
> the API won't appear to have changed, at the RESTful level it *has*
> changed:
> 
>  - There's no longer a linked_branches_collection_link in
>    the Bug JSON.
>  - To get linked_branches you now have to make a named GET
>    to linked_branches.
> 
> Basically, if I'm right, any API client that uses the RESTful API
> directly - including our own JS API client - is now going to break
> when it tries to do anything with linked_branches.
> 
> I can see a couple of solutions to this:

I had hoped for another solution.

Annotate your accessor method with @accessor_for (like we have
@mutator_for).

But it seems we never implemented that one! :-/ Using the @mutator_for
annotation as a template, it wouldn't probably be too complicated to
add. If give a shot, it would give you a chance of mucking within the
guts of lazr.restful. Sounds like fun, no? If you are game, let me know
and I can help you walk through them (you could also ask Benji for some
help).

> 
>  1. Make linked_branches a property again. This means using
>     the LaunchBag to get the current user. Avoiding the Bag
>     was the main reason for adding a getter instead of using
>     a property here.

If you wimp away from enhancing lazr.restful, I'd suggest you go with
that wart. It has a nice transition path to @accessor_for.

@property
def list_branches(self):
   # XXX reference bug about missing @accessor_for
   user = ...
   return self.getVisibleLinkedBranches(user)

>  2. Leave the current linked_branches attribute exposed for
>     the stable versions of the API and expose the new getter
>     for the devel API (and subsequent stable versions).

That's has the downside of introducing another one of these named
operation which makes our web API so inconsistent. So I'd rather have a
LaunchBag reference.

>  3. Change the API and contact all the people who use the WS
>     API to let them know about the change.
> 

That's really a PITA. We don't really have a way to get that list.
Don't do it :-)

-- 
Francis J. Lacoste
francis.lacoste@xxxxxxxxxxxxx

Attachment: signature.asc
Description: OpenPGP digital signature