Launchpad itself

Merge lp:~wgrant/launchpad/gh-rate-limit-auth into lp:launchpad

  1. gh-rate-limit-auth
  2. Merge into devel
Proposed by William Grant
Status: Merged
Merged at revision: 18295
Proposed branch: lp:~wgrant/launchpad/gh-rate-limit-auth
Merge into: lp:launchpad
Diff against target: 113 lines (+52/-9)
2 files modified
lib/lp/bugs/externalbugtracker/github.py (+18/-9)
lib/lp/bugs/externalbugtracker/tests/test_github.py (+34/-0)
To merge this branch: bzr merge lp:~wgrant/launchpad/gh-rate-limit-auth
Reviewer Review Type Date Requested Status
Colin Watson (community) Approve
Review via email: mp+313309@code.launchpad.net

Commit message

Fix GitHub bug sync rate limit check to not use the anonymous limit.

Description of the change

Pass the GitHub bug sync auth down to GitHubRateLimit.

Previously all requests except the rate limit check were authenticated,
but the rate limit check omitted the token, so requests would be
cancelled when they would have succeeded.

To post a comment you must log in.
Revision history for this message
Colin Watson (cjwatson) wrote :

I thought it might be something like this. Thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/bugs/externalbugtracker/github.py'
2--- lib/lp/bugs/externalbugtracker/github.py 2016-07-04 17:11:29 +0000
3+++ lib/lp/bugs/externalbugtracker/github.py 2016-12-15 07:27:15 +0000
4@@ -77,28 +77,39 @@
5 def __init__(self):
6 self.clearCache()
7
8- def _update(self, host, token=None):
9+ def _update(self, host, auth_header=None):
10 headers = {
11 "User-Agent": LP_USER_AGENT,
12 "Host": host,
13 "Accept": "application/vnd.github.v3+json",
14 }
15- if token is not None:
16- headers["Authorization"] = "token %s" % token
17+ if auth_header is not None:
18+ headers["Authorization"] = auth_header
19 url = "https://%s/rate_limit" % host
20 try:
21 response = requests.get(url, headers=headers)
22 response.raise_for_status()
23- self._limits[(host, token)] = response.json()["resources"]["core"]
24+ return response.json()["resources"]["core"]
25 except requests.RequestException as e:
26 raise BugTrackerConnectError(url, e)
27
28 @ensure_no_transaction
29 def makeRequest(self, method, url, token=None, **kwargs):
30 """See `IGitHubRateLimit`."""
31+ if token is not None:
32+ auth_header = "token %s" % token
33+ if "headers" in kwargs:
34+ kwargs["headers"] = kwargs["headers"].copy()
35+ else:
36+ kwargs["headers"] = {}
37+ kwargs["headers"]["Authorization"] = auth_header
38+ else:
39+ auth_header = None
40+
41 host = urlsplit(url).netloc
42 if (host, token) not in self._limits:
43- self._update(host, token=token)
44+ self._limits[(host, token)] = self._update(
45+ host, auth_header=auth_header)
46 limit = self._limits[(host, token)]
47 if not limit["remaining"]:
48 raise GitHubExceededRateLimit(host, limit["reset"])
49@@ -216,9 +227,6 @@
50 def _getHeaders(self, last_accessed=None):
51 """See `ExternalBugTracker`."""
52 headers = super(GitHub, self)._getHeaders()
53- token = self.credentials["token"]
54- if token is not None:
55- headers["Authorization"] = "token %s" % token
56 headers["Accept"] = "application/vnd.github.v3+json"
57 if last_accessed is not None:
58 headers["If-Modified-Since"] = (
59@@ -234,7 +242,8 @@
60 try:
61 response = getUtility(IGitHubRateLimit).makeRequest(
62 "GET", urljoin(self.baseurl + "/", page),
63- headers=self._getHeaders(last_accessed=last_accessed))
64+ headers=self._getHeaders(last_accessed=last_accessed),
65+ token=self.credentials["token"])
66 response.raise_for_status()
67 return response
68 except requests.RequestException as e:
69
70=== modified file 'lib/lp/bugs/externalbugtracker/tests/test_github.py'
71--- lib/lp/bugs/externalbugtracker/tests/test_github.py 2016-07-04 17:11:29 +0000
72+++ lib/lp/bugs/externalbugtracker/tests/test_github.py 2016-12-15 07:27:15 +0000
73@@ -165,6 +165,40 @@
74 "content": {"resources": {"core": rate_limit}},
75 }
76
77+ def test__getPage_authenticated(self):
78+ @urlmatch(path=r".*/test$")
79+ def handler(url, request):
80+ self.request = request
81+ return {"status_code": 200, "content": json.dumps("success")}
82+
83+ self.pushConfig(
84+ "checkwatches.credentials", **{"api.github.com.token": "sosekrit"})
85+ tracker = GitHub("https://github.com/user/repository/issues")
86+ with HTTMock(self._rate_limit_handler, handler):
87+ self.assertEqual("success", tracker._getPage("test").json())
88+ self.assertEqual(
89+ "https://api.github.com/repos/user/repository/test",
90+ self.request.url)
91+ self.assertEqual(
92+ "token sosekrit", self.request.headers["Authorization"])
93+ self.assertEqual(
94+ "token sosekrit", self.rate_limit_request.headers["Authorization"])
95+
96+ def test__getPage_unauthenticated(self):
97+ @urlmatch(path=r".*/test$")
98+ def handler(url, request):
99+ self.request = request
100+ return {"status_code": 200, "content": json.dumps("success")}
101+
102+ tracker = GitHub("https://github.com/user/repository/issues")
103+ with HTTMock(self._rate_limit_handler, handler):
104+ self.assertEqual("success", tracker._getPage("test").json())
105+ self.assertEqual(
106+ "https://api.github.com/repos/user/repository/test",
107+ self.request.url)
108+ self.assertNotIn("Authorization", self.request.headers)
109+ self.assertNotIn("Authorization", self.rate_limit_request.headers)
110+
111 def test_getRemoteBug(self):
112 @urlmatch(path=r".*/issues/1$")
113 def handler(url, request):