From 9d2a697fb9d04f65bb9ba6c7b2b508c263da0503 Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Wed, 11 May 2011 23:26:52 -0400 Subject: [PATCH] codereview: fetch metadata using JSON API, not XML scraping Fixes hg clpatch. R=golang-dev, r, r CC=golang-dev https://golang.org/cl/4524045 --- lib/codereview/codereview.py | 181 ++++++++++++++++------------------- 1 file changed, 82 insertions(+), 99 deletions(-) diff --git a/lib/codereview/codereview.py b/lib/codereview/codereview.py index 36d7df199fe..5fed5efdd0a 100644 --- a/lib/codereview/codereview.py +++ b/lib/codereview/codereview.py @@ -45,10 +45,23 @@ import stat import subprocess import threading from HTMLParser import HTMLParser + +# The standard 'json' package is new in Python 2.6. +# Before that it was an external package named simplejson. try: - from xml.etree import ElementTree as ET -except: - from elementtree import ElementTree as ET + # Standard location in 2.6 and beyond. + import json +except Exception, e: + try: + # Conventional name for earlier package. + import simplejson as json + except: + try: + # Was also bundled with django, which is commonly installed. + from django.utils import simplejson as json + except: + # We give up. + raise e try: hgversion = util.version() @@ -502,14 +515,16 @@ def LoadCL(ui, repo, name, web=True): else: cl = CL(name) if web: - try: - f = GetSettings(name) - except: - return None, "cannot load CL %s from code review server: %s" % (name, ExceptionDetail()) - if 'reviewers' not in f: + set_status("getting issue metadata from web") + d = JSONGet(ui, "/api/" + name + "?messages=true") + set_status(None) + if d is None: + return None, "cannot load CL %s from server" % (name,) + if 'owner_email' not in d or 'issue' not in d or str(d['issue']) != name: return None, "malformed response loading CL data from code review server" - cl.reviewer = SplitCommaSpace(f['reviewers']) - cl.cc = SplitCommaSpace(f['cc']) + cl.dict = d + cl.reviewer = d.get('reviewers', []) + cl.cc = d.get('cc', []) if cl.local and cl.copied_from and cl.desc: # local copy of CL written by someone else # and we saved a description. use that one, @@ -517,9 +532,10 @@ def LoadCL(ui, repo, name, web=True): # before doing hg submit. pass else: - cl.desc = f['description'] + cl.desc = d.get('description', "") cl.url = server_url_base + name cl.web = True + cl.private = d.get('private', False) != False set_status("loaded CL " + name) return cl, '' @@ -1330,7 +1346,9 @@ def clpatch_or_undo(ui, repo, clname, opts, mode): # if version does not match the patch version, # try to update the patch line numbers. - if id != vers: + if vers != "" and id != vers: + if vers not in repo: + return "local repository is out of date; sync to get %s" % (vers) patch, err = portPatch(repo, patch, vers, id) if err != "": return "codereview issue %s is out of date: %s (%s->%s)" % (clname, err, vers, id) @@ -2000,100 +2018,84 @@ class FormParser(HTMLParser): if self.curdata is not None: self.curdata += data -# XML parser -def XMLGet(ui, path): +def JSONGet(ui, path): try: - data = MySend(path, force_auth=False); + data = MySend(path, force_auth=False) + typecheck(data, str) + d = coerce_to_utf8(json.loads(data)) except: - ui.warn("XMLGet %s: %s\n" % (path, ExceptionDetail())) + ui.warn("JSONGet %s: %s\n" % (path, ExceptionDetail())) return None - return ET.XML(data) + return d + +def coerce_to_utf8(x): + if type(x) in [str, int, float, bool, type(None)]: + pass + elif type(x) is unicode: + x = x.encode("utf-8") + elif type(x) is list: + for i in range(len(x)): + x[i] = coerce_to_utf8(x[i]) + elif type(x) is dict: + for k in x: + x[k] = coerce_to_utf8(x[k]) + else: + raise util.Abort("unknown type " + str(type(x)) + " in coerce_to_utf8") + if type(x) is str: + x = x.replace('\r\n', '\n') + return x def IsRietveldSubmitted(ui, clname, hex): - feed = XMLGet(ui, "/rss/issue/" + clname) - if feed is None: + dict = JSONGet(ui, "/api/" + clname + "?messages=true") + if dict is None: return False - for sum in feed.findall("{http://www.w3.org/2005/Atom}entry/{http://www.w3.org/2005/Atom}summary"): - text = sum.text.strip() + for msg in dict.get("messages", []): + text = msg.get("text", "") m = re.match('\*\*\* Submitted as [^*]*?([0-9a-f]+) \*\*\*', text) if m is not None and len(m.group(1)) >= 8 and hex.startswith(m.group(1)): return True return False -def IsRietveldMailed(ui, clname): - feed = XMLGet(ui, "/rss/issue/" + clname) - if feed is None: - return False - for sum in feed.findall("{http://www.w3.org/2005/Atom}entry/{http://www.w3.org/2005/Atom}summary"): - text = sum.text.strip() - if re.match("I'd like you to review this change", text): +def IsRietveldMailed(cl): + for msg in cl.dict.get("messages", []): + if msg.get("text", "").find("I'd like you to review this change") >= 0: return True return False def DownloadCL(ui, repo, clname): set_status("downloading CL " + clname) - cl, err = LoadCL(ui, repo, clname) + cl, err = LoadCL(ui, repo, clname, web=True) if err != "": return None, None, None, "error loading CL %s: %s" % (clname, err) - # Grab RSS feed to learn about CL - feed = XMLGet(ui, "/rss/issue/" + clname) - if feed is None: - return None, None, None, "cannot download CL" - # Find most recent diff - diff = None - prefix = 'http://' + server + '/' + diffs = cl.dict.get("patchsets", []) + if not diffs: + return None, None, None, "CL has no patch sets" + patchid = diffs[-1] + + patchset = JSONGet(ui, "/api/" + clname + "/" + str(patchid)) + if patchset is None: + return None, None, None, "error loading CL patchset %s/%d" % (clname, patchid) + if patchset.get("patchset", 0) != patchid: + return None, None, None, "malformed patchset information" + vers = "" - for entry in feed.findall("{http://www.w3.org/2005/Atom}entry"): - thisVers = "" - for title in entry.findall("{http://www.w3.org/2005/Atom}title"): - m = re.search('diff -r ([0-9a-f]+) ', title.text) - if m: - thisVers = m.group(1) - if thisVers == "": - continue - for link in entry.findall("{http://www.w3.org/2005/Atom}link"): - if link.get('rel') != 'alternate': - continue - text = link.get('href') - if not text.startswith(prefix) or not text.endswith('.diff'): - continue - diff = text[len(prefix)-1:] - vers = thisVers - if diff is None: - return None, None, None, "CL has no diff" + msg = patchset.get("message", "").split() + if len(msg) >= 3 and msg[0] == "diff" and msg[1] == "-r": + vers = msg[2] + diff = "/download/issue" + clname + "_" + str(patchid) + ".diff" + diffdata = MySend(diff, force_auth=False) - - # Find author - first entry will be author who created CL. - nick = None - for author in feed.findall("{http://www.w3.org/2005/Atom}entry/{http://www.w3.org/2005/Atom}author/{http://www.w3.org/2005/Atom}name"): - nick = author.text.strip() - break - if not nick: - return None, None, None, "CL has no author" - - # The author is just a nickname: get the real email address. - try: - # want URL-encoded nick, but without a=, and rietveld rejects + for %20. - url = "/user_popup/" + urllib.urlencode({"a": nick})[2:].replace("+", "%20") - data = MySend(url, force_auth=False) - except: - ui.warn("error looking up %s: %s\n" % (nick, ExceptionDetail())) - cl.copied_from = nick+"@needtofix" - return cl, vers, diffdata, "" - match = re.match(r"(.*) \((.*)\)", data) - if not match: - return None, None, "error looking up %s: cannot parse result %s" % (nick, repr(data)) - if match.group(1) != nick and match.group(2) != nick: - return None, None, "error looking up %s: got info for %s, %s" % (nick, match.group(1), match.group(2)) - email = match.group(1) - + # Print warning if email is not in CONTRIBUTORS file. + email = cl.dict.get("owner_email", "") + if not email: + return None, None, None, "cannot find owner for %s" % (clname) him = FindContributor(ui, repo, email) me = FindContributor(ui, repo, None) if him == me: - cl.mailed = IsRietveldMailed(ui, clname) + cl.mailed = IsRietveldMailed(cl) else: cl.copied_from = email @@ -2192,25 +2194,6 @@ def GetForm(url): m[k.encode("utf-8")] = v.replace("\r\n", "\n").encode("utf-8") return m -# Fetch the settings for the CL, like reviewer and CC list, by -# scraping the Rietveld editing forms. -def GetSettings(issue): - set_status("getting issue metadata from web") - # The /issue/edit page has everything but only the - # CL owner is allowed to fetch it (and submit it). - f = None - try: - f = GetForm("/" + issue + "/edit") - except: - pass - if not f or 'reviewers' not in f: - # Maybe we're not the CL owner. Fall back to the - # /publish page, which has the reviewer and CC lists, - # and then fetch the description separately. - f = GetForm("/" + issue + "/publish") - f['description'] = MySend("/"+issue+"/description", force_auth=False) - return f - def EditDesc(issue, subject=None, desc=None, reviewers=None, cc=None, closed=False, private=False): set_status("uploading change to description") form_fields = GetForm("/" + issue + "/edit")