Fix AttributeErrors when using RBTools 0.6 against older versions of RB
Review Request #5838 — Created May 19, 2014 and discarded
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. TheReviewRequestResource.absolute_url
property provides fallback behavior for when theabsolute_url
field is not received in the server response, but that fallback implementation ends up raising because it tries to callself.url
, which doesn't exist (and therefore raises anAttributeError
forurl
.) When you raise in aproperty.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 anAttributeError
due to the missingabsolute_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 doingrbt status
the code eventually callsrepo.mirror_path
, without checking to see if repo has amirror_path
attribute which, in our case (with RB 1.6.14), it doesn't. This is easily worked around withgetattr
(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
, andrbt 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:
-
-
This isn't the same thing. What
absolute_url
is doing is providing the absolute URL to a page (not API) on Review Board. For example,/users/chipx86/
.It's trying to access
self.url
because it's looking for theurl
field from the API resource's payload. Given that, I believe this code is correct.What errors were you hitting, specifically?
-
We should only do the
getattr
formirror_path
, since that's not in the older APIs. Also, we should keep using parens for line continuation.