Add shim objects for looked-up entries, similar to top-level collections

Actually all entry objects would benefit from starting out as empty shims. If you write bug.owner.someNamedOperation() there's no reason to actually get a representation of bug.owner.

On Thu, 20 May 2010 17:59:32 -0000, Leonard Richardson <email address hidden> wrote:
> Actually all entry objects would benefit from starting out as empty
> shims. If you write bug.owner.someNamedOperation() there's no reason to
> actually get a representation of bug.owner.

One thing to be careful of is that

  lp.me.someNamedOperation()

still works.

If you POST to

  /+me/

then you get a redirect (302 I believe), check that the http library
doesn't turn this in to a GET of /~your-id/. The HTTP rfc says that
anything but GET or HEAD shouldn't automatically follow a 302.

Also, txrestfulclient uses shims, but for a different reason, because
making __getattr__ return a Deferred would be both ugly and really hard
to use. It still does the intermediate GET requests, but I should see
about short circuiting that.

What it does have to have is an explicit way to resolve (lp_resolve()
was picked), so that you control when you get a deferred. This means
that you don't have to trigger a GET when anything is done to the
object. If you use shims for every attribute then I guess you have to
have the shim check for a representation for during __getattr__, __eq__,
__str__, etc.

Thanks,

James

Actually, someone yesterday had a problem that lp.me.someNamedOperation() *wasn't* working. launchpadlib would GET /+me, follow the redirect to /~leonardr, and set up a representation. Then they'd invoke someNamedOperation() and it would POST to /+me instead of /~leonardr.

Not sure what this means, but I'm going to try to deal with this at the same time as the shim objects.

I wrote some lazr.restfulclient tests. The plot thickens!

1. The reason launchpad.projects['foo'].getMergeProposals() makes a request for /projects/foo is that the ability to do .projects['foo'] in the first place is a launchpadlib hack. There's no indication in the WADL that this is possible, and no information in the WADL or in the launchpadlib hack about what kind of object .projects['foo'] is going to be. If we know it's a project (and it shouldn't be that hard to figure it out or add it to the hack), we can avoid making that request.

2. Here's some lazr.restfulclient code I use in my test.

recipe1 = services.recipes[1]
recipes = recipe1.cookbook.find_recipes(search="bar")

The first LOC makes a request for /recipes/1 for the reason given above--the service-specific hack.

The second LOC makes one and only one request, to /cookbooks/Cookbook%20Name?ws.op=find_recipes. It does NOT make a request to /cookbooks/Cookbook%20Name.

Now, here's some launchpadlib code.

bug = launchpad.bugs[1]
branches = bug.owner.getBranches()

The first LOC makes a request for /bugs/1.

The second LOC makes what appears to be TWO requests for /~sabdfl, and only then makes a request for /~sabdfl?ws.op=getBranches. 'bug.owner' all on its own makes TWO requests for /~sabdfl.

Clearly something funny is going on. Here are the two requests for /~safbdl:

>>> bug.owner
send: 'GET /1.0/~sabdfl HTTP/1.1\r\nHost: api.staging.launchpad.net\r\nAccept-Encoding: identity\r\nte: deflate, gzip\r\nAuthorization: OAuth realm="OAuth", oauth_nonce="50222755", oauth_timestamp="1276634241", oauth_consumer_key="foo", oauth_signature_method="PLAINTEXT", oauth_version="1.0", oauth_token="", oauth_signature="%26"\r\naccept: application/json\r\nuser-agent: lazr.restfulclient 0.9.14; oauth_consumer="foo"\r\n\r\n'
reply: ''
send: 'GET /1.0/~sabdfl HTTP/1.1\r\nHost: api.staging.launchpad.net\r\nAccept-Encoding: identity\r\nte: deflate, gzip\r\nAuthorization: OAuth realm="OAuth", oauth_nonce="50222755", oauth_timestamp="1276634241", oauth_consumer_key="foo", oauth_signature_method="PLAINTEXT", oauth_version="1.0", oauth_token="", oauth_signature="%26"\r\naccept: application/json\r\nuser-agent: lazr.restfulclient 0.9.14; oauth_consumer="foo"\r\n\r\n'
reply: 'HTTP/1.1 200 Ok\r\n'
...
<person at https://api.staging.launchpad.net/1.0/~sabdfl>

The first request gets no reply, and the two requests have the same nonce, so it's likely something weird is happening and that first request is not really going over the wire. But according to the lazr.restfulclient example, there should be NO request happening here. I don't know why launchpadlib feels the need to make this request at all.

To sum up, we have a problem that can be fixed in lazr.restfulclient + launchpadlib, and a problem that seems specific to launchpadlib about which I have no clue. But we're actually in better shape than I thought. I wrote a whole bunch of tests against lazr.restfulclient to test the features I thought I'd be adding, and all but one of the tests pass already.

I'm fairly sure the extra 'sabdfl' request was due to bug 568301, which has already been fixed. I ran the test against lazr.restfulclient trunk and that request no longer happens. So we're back where we started: the only problem is to do with the hacky top-level-collection lookup.

And here's why CollectionWithKeyBasedLookup always fetches a representation:

        # We don't know what kind of resource this is. Even the
        # subclass doesn't necessarily know, because some resources
        # (the person list) are gateways to more than one kind of
        # resource (people, and teams). The only way to know for sure
        # is to retrieve a representation of the resource and see how
        # the resource describes itself.

For most CollectionWithKeyBasedLookup classes, we can avoid this by specifying the equivalent of resource_type_link in the subclass. But this won't work for PersonCollection, since launchpad.people['leonardr'] has a different resource_type_link than launchpad.people['beta-testers'], and the only way to know this is to make that HTTP request.

So I can fix this easily for projects, bugs, branches, etc. but not for people. I could work up some extra solution for people where something like launchpad.people['team:beta-testers'] would work, but I think that's too much work for the benefit, and I'm not sure how the design would look--obviously 'team:beta-testers' is pretty bad.

I've released a lazr.restfulclient that fixes this problem, so I've made the lazr.restfulclient task Fix Released.

I've released a launchpadlib that takes advantage of the fix for all but the collection of users. I have a couple ideas to fix this but nothing great. One idea is to define separate launchpad.persons and launchpad.teams collections.

Another idea is to simply not care and make all objects looked up from launchpad.users into 'team' objects. I'm pretty sure one of the following is true: either there's no non-cosmetic difference between 'person' and 'team' objects, or 'person' is a proper subset of team. If the former, this should work fine. If the latter, the worst that will happen is that lp_operations and other methods will show some team-specific capabilities on launchpad.people['salgado'] that will error out if you actually try them.

Right now, 'person' is a proper subset of 'team'. We could eliminate the HTTP request by having launchpad.people['foo'] always return a 'team' object.

Here's the problem. If I release a version of launchpadlib that makes this assumption, people will use it approximately forever. If we ever add something to 'person' that's not in 'team', then 1) we'll immediately have to release a new version of Launchpad that goes back to the current behavior, and 2) older versions of launchpadlib will not be able to access the new features of 'person'. We're not breaking backwards compatibility, exactly, but we are violating the rule that users shouldn't have to upgrade their client when the web service changes. And I'm probably not the one adding that new feature to 'person', but I am going to be the one releasing the new version of launchpadlib, so at some random point in the future someone will complain and I'll have to drop everything and scramble to revert the fix I'm about to make. Not good.

A better solution might be to define UserSet.collection_of = ['person', 'team'], and have launchpad.people['foo'] return an object that's in a quantum superposition of 'person' and 'team'. An object whose lp_attributes shows the union of a person's lp_attributes and a team's, whatever they are according to the current WADL. Similarly for lp_operations and so on. Once you tried to use the object (accessing an attribute or invoking a named operation), launchpadlib would make an HTTP request to the object, and if it turned out to not support the attribute or named operation you were trying to use, you'd get an error at this point. (Just as you can assign launchpad.projects['no-such-project'] to a Python object and you only get an error when you try to do something with it.) But, that's a lot of implementation work, and not something I want to tackle now. (Or after people start complaining that launchpadlib is broken.)

Leaonard, a team is a person, it extends that interface. Unless we change that (which is a pretty complex LP-side change) there won't be any way to add an operation to a person that wouldn't be available on a team. (It might not work, but that's a different story.)

So saying that collection-of 'team' does seem safe for now.

I assume that the correct WADL type is set when the object is retrieved for real (because the user tries to access a data attributes)?

See bug 605731 for a problem with this.

Thanks,

James