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 postcrashes with AttributeError)
2) Change attribute accesses to getattr to make get_repository_id more tolerant of missing attributes (symptom:rbt statuscrashes with AttributeError)Details:
The
rbt postcrash is arguably just programmer error, although a bit confusing to diagnose. TheReviewRequestResource.absolute_urlproperty provides fallback behavior for when theabsolute_urlfield 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 anAttributeErrorforurl.) 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 anAttributeErrordue to the missingabsolute_urlfield. 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 statuscrash is similar (missing property) but points to a much larger issue. When doingrbt statusthe code eventually callsrepo.mirror_path, without checking to see if repo has amirror_pathattribute 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_urlis doing is providing the absolute URL to a page (not API) on Review Board. For example,/users/chipx86/.It's trying to access
self.urlbecause it's looking for theurlfield 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
getattrformirror_path, since that's not in the older APIs. Also, we should keep using parens for line continuation.
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: rbtools/utils/repository.py rbtools/api/resource.py Ignored Files: