Fix AttributeErrors when using RBTools 0.6 against older versions of RB

Review Request #5838 — Created May 19, 2014 and discarded — Latest diff uploaded

Information

RBTools

Reviewers

Two changes:

1) Fix up the way review URLs are generated in the fallback case where absolute_url is not present (symptom: rbt post crashes with AttributeError)
2) Change attribute accesses to getattr to make get_repository_id more tolerant of missing attributes (symptom: rbt status crashes with AttributeError)

Details:

The rbt post crash is arguably just programmer error, although a bit confusing to diagnose. The ReviewRequestResource.absolute_url property provides fallback behavior for when the absolute_url field is not received in the server response, but that fallback implementation ends up raising because it tries to call self.url, which doesn't exist (and therefore raises an AttributeError for url.) When you raise in a property.fget, Python handles it by swallowing that first exception, and falling back to calling __getattr__ for the name of the property, which then, itself, raises an AttributeError due to the missing absolute_url field. I figured this was worth mentioning since it took me a while in the debugger to figure out what was really going on.

The rbt status crash is similar (missing property) but points to a much larger issue. When doing rbt status the code eventually calls repo.mirror_path, without checking to see if repo has a mirror_path attribute which, in our case (with RB 1.6.14), it doesn't. This is easily worked around with getattr (as is done in this diff), but it points to a larger, systemic problem. That is: RBTools takes data received over the network and effectively merges it into the type system of the Python objects. Put differently, properties on Python objects are defined (or not defined) based on responses from the server. Not only is this causing crashes in this instance (running against older RB installations), but it's a huge potential attack vector for malicious attacks (of the same ilk as SQL injection). The interface of the Python objects in the API should be fixed/static regardless of network responses. If the objects need to vend a varying key/value set dependent on responses from the server, they should use a standard Python dictionary idiom, so it's clear to API consumers that these properties are dynamic and not part of the type system. I've not attempted to address this larger issue in this diff (instead only fixing the minimum necessary to unblock our use of rbt) but I figured it was worth mentioning.

With this patch, I was able to use rbt post, rbt status, and rbt close (for the cases I had at hand) against ReviewBoard 1.6.14.

    Loading...