Fix AttributeErrors when using RBTools 0.6 against older versions of RB
Review Request #5838 — Created May 19, 2014 and discarded
1) Fix up the way review URLs are generated in the fallback case where absolute_url is not present (symptom:
rbt postcrashes with AttributeError)
2) Change attribute accesses to getattr to make get_repository_id more tolerant of missing attributes (symptom:
rbt statuscrashes with AttributeError)
rbt postcrash is arguably just programmer error, although a bit confusing to diagnose. The
ReviewRequestResource.absolute_urlproperty provides fallback behavior for when the
absolute_urlfield 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
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
AttributeErrordue to the missing
absolute_urlfield. I figured this was worth mentioning since it took me a while in the debugger to figure out what was really going on.
rbt statuscrash is similar (missing property) but points to a much larger issue. When doing
rbt statusthe code eventually calls
repo.mirror_path, without checking to see if repo has a
mirror_pathattribute 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 status, and
rbt close(for the cases I had at hand) against ReviewBoard 1.6.14.
This is a review from Review Bot. Tool: Pyflakes Processed Files: rbtools/utils/repository.py rbtools/api/resource.py Ignored Files:
rbtools/api/resource.py (Diff revision 1)
This isn't the same thing. What
absolute_urlis doing is providing the absolute URL to a page (not API) on Review Board. For example,
It's trying to access
self.urlbecause it's looking for the
urlfield from the API resource's payload. Given that, I believe this code is correct.
What errors were you hitting, specifically?
rbtools/utils/repository.py (Diff revision 1)
We should only do the
mirror_path, since that's not in the older APIs. Also, we should keep using parens for line continuation.