diff --git a/crowdin/approve_strings.py b/crowdin/approve_strings.py index e5613cc..e2b36cb 100644 --- a/crowdin/approve_strings.py +++ b/crowdin/approve_strings.py @@ -34,9 +34,13 @@ import sys import time -import requests +from crowdin_api.api_resources.source_strings.enums import ScopeFilter +from crowdin_api.exceptions import APIException, ValidationError + +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +import crowdin_sdk # noqa: E402 -API = "https://api.crowdin.com/api/v2" DEFAULT_PROJECT = "618696" # libsecret attributes identifying the token in the keyring; must match the @@ -128,14 +132,7 @@ def main(): token = get_token() identifiers = args.identifiers - pid = args.project_id - s = requests.Session() - s.headers.update({"Authorization": f"Bearer {token}"}) - - def get(path, **params): - r = s.get(f"{API}{path}", params=params, timeout=30) - r.raise_for_status() - return r.json() + client = crowdin_sdk.client(token, args.project_id, attempts=6, timeout=30) # 1) Resolve identifiers -> string IDs string_ids = {} @@ -143,8 +140,8 @@ def get(path, **params): found = None offset = 0 while True: - data = get(f"/projects/{pid}/strings", filter=ident, - scope="identifier", limit=500, offset=offset)["data"] + data = client.source_strings.list_strings( + filter=ident, scope=ScopeFilter.IDENTIFIER, limit=500, offset=offset)["data"] for row in data: if row["data"]["identifier"] == ident: found = row["data"]["id"] @@ -158,7 +155,7 @@ def get(path, **params): print(f" string '{ident}' -> id {found}") # 2) Target languages - proj = get(f"/projects/{pid}")["data"] + proj = client.projects.get_project()["data"] target_langs = proj["targetLanguageIds"] print(f"\n{len(target_langs)} target languages: {', '.join(target_langs)}\n") @@ -169,15 +166,15 @@ def get(path, **params): approved, skipped, already, errors = 0, 0, 0, 0 for ident, sid in string_ids.items(): for lang in target_langs: - raw = get(f"/projects/{pid}/translations", - stringId=sid, languageId=lang, limit=500)["data"] + raw = client.string_translations.list_string_translations( + stringId=sid, languageId=lang, limit=500)["data"] trans = [t["data"] for t in raw] if args.list: # Show only unapproved translations (i.e. what a run would approve), # honouring the --by-user filter if one was given. - approvals = get(f"/projects/{pid}/approvals", - stringId=sid, languageId=lang, limit=500)["data"] + approvals = client.string_translations.list_translation_approvals( + stringId=sid, languageId=lang, limit=500)["data"] approved_tids = {a["data"]["translationId"] for a in approvals} pending = [t for t in trans if t["id"] not in approved_tids @@ -209,17 +206,19 @@ def get(path, **params): approved += 1 continue - r = s.post(f"{API}/projects/{pid}/approvals", - json={"translationId": tid}, timeout=30) - if r.status_code == 201: + try: + client.string_translations.add_approval(translationId=tid) + except APIException as exc: + message = crowdin_sdk.error_message(exc) + if isinstance(exc, ValidationError) and "already" in message.lower(): + print(f" [ -- ] {ident} / {lang}: already approved {info}") + already += 1 + else: + print(f" [ERR ] {ident} / {lang}: {exc.http_status} {message}") + errors += 1 + else: print(f" [ ok ] {ident} / {lang}: approved {info}") approved += 1 - elif r.status_code == 400 and "already" in r.text.lower(): - print(f" [ -- ] {ident} / {lang}: already approved {info}") - already += 1 - else: - print(f" [ERR ] {ident} / {lang}: {r.status_code} {r.text}") - errors += 1 time.sleep(0.1) # gentle on rate limits if args.list: diff --git a/crowdin/crowdin_sdk.py b/crowdin/crowdin_sdk.py new file mode 100644 index 0000000..feed87c --- /dev/null +++ b/crowdin/crowdin_sdk.py @@ -0,0 +1,89 @@ +"""crowdin-api-client with this repo's retry underneath it. + +The SDK's own retry is a fixed 100 ms sleep, five times, on 5xx only: a 429 raises +at once whatever Retry-After says, and Crowdin throttles at ~40 req/s. So the SDK +supplies the endpoints, parameter names and error mapping, and shared.retry the +transport, by standing in for the requests.Session its requester talks to. +""" +import functools +import json +import threading + +import requests +from crowdin_api import CrowdinClient +from crowdin_api.requester import APIRequester + +from shared.retry import request_with_retry + + +class RetryingSession: + """The SDK's session, routed through shared.retry. + + One requests.Session per thread: the SDK shares its session across callers, and + requests.Session is not guaranteed thread-safe. `fixed` is one session for every + thread instead, which is how a test hands in a fake. + """ + + def __init__(self, attempts): + self.attempts = attempts + self.headers = {} + self.fixed = None + self._local = threading.local() + + def _session(self): + if self.fixed is not None: + return self.fixed + session = getattr(self._local, "session", None) + if session is None: + session = self._local.session = requests.Session() + session.headers.update(self.headers) + return session + + def request(self, method, url, **kwargs): + return request_with_retry(self._session(), method, url, attempts=self.attempts, + **kwargs) + + def close(self): + session = getattr(self._local, "session", None) + if session is not None: + session.close() + + +class RetryingRequester(APIRequester): + def __init__(self, *args, attempts, **kwargs): + super().__init__(*args, **kwargs) + # super().__init__ has already put the auth headers on the session it created. + plain, self._session = self._session, RetryingSession(attempts) + self._session.headers.update(plain.headers) + plain.close() + + +def client(token, project_id, attempts, timeout): + """A CrowdinClient retrying like shared.retry: `attempts` tries, backing off on + 429 and 5xx. max_retries=1 is the SDK's own loop switched off.""" + crowdin = CrowdinClient(token=token, project_id=int(project_id), timeout=timeout, + max_retries=1) + crowdin.API_REQUESTER_CLASS = functools.partial(RetryingRequester, attempts=attempts) + return crowdin + + +def use_session(crowdin, session): + """Answer every request from `session` (a test fake) instead of the network.""" + crowdin.get_api_requestor().session.fixed = session + + +def error_message(exc): + """Crowdin's error message, or the start of the body when it is not that envelope.""" + body = exc.context or b"" + if isinstance(body, bytes): + body = body.decode("utf-8", "replace") + try: + return json.loads(body).get("error", {}).get("message", "Unknown error") + except (ValueError, AttributeError): + return body[:200] or "Unknown error" + + +def fetch_all(resource, method, **params): + """Every item of a paginated list endpoint, unwrapped from the SDK's envelopes.""" + listing = getattr(resource.with_fetch_all(), method) + return [row["data"] for row in listing(**params)["data"]] diff --git a/crowdin/download_translations_from_crowdin.py b/crowdin/download_translations_from_crowdin.py index f39ce23..4e5f9b7 100644 --- a/crowdin/download_translations_from_crowdin.py +++ b/crowdin/download_translations_from_crowdin.py @@ -14,13 +14,15 @@ from threading import Lock, Semaphore import requests +from crowdin_api.api_resources.enums import ExportProjectTranslationFormat +from crowdin_api.exceptions import APIException sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +import crowdin_sdk # noqa: E402 from generate_shared import print_error, print_progress, print_success, run_main # noqa: E402 from shared.retry import request_with_retry # noqa: E402 -API = "https://api.crowdin.com/api/v2" # Crowdin allows 20 simultaneous requests per account. MAX_CONCURRENT_REQUESTS = 20 REQUEST_TIMEOUT_S = 30 @@ -31,35 +33,25 @@ class CrowdinError(Exception): pass -def error_message(response): - """Crowdin's error message, or the start of the body when it is not that envelope.""" - try: - return response.json().get("error", {}).get("message", "Unknown error") - except ValueError: - return response.text[:200] or "Unknown error" - - class Crowdin: """One project's API, gated to Crowdin's concurrency limit across the worker threads.""" def __init__(self, token, project_id, max_workers): self.project_id = project_id - self.session = requests.Session() - self.session.headers["Authorization"] = f"Bearer {token}" + self.api = crowdin_sdk.client(token, project_id, attempts=MAX_ATTEMPTS, + timeout=REQUEST_TIMEOUT_S) # Exports are served from a signed URL on another host, which must not see the token. self.downloads = requests.Session() self.gate = Semaphore(min(max_workers, MAX_CONCURRENT_REQUESTS)) - def request(self, method, path, context, **kwargs): - """One API response's JSON, or CrowdinError naming `context`.""" + def call(self, context, method, **kwargs): + """One SDK call's JSON, or CrowdinError naming `context`.""" with self.gate: - response = request_with_retry(self.session, method, f"{API}/{path}", - attempts=MAX_ATTEMPTS, timeout=REQUEST_TIMEOUT_S, - **kwargs) - if response.status_code != 200: - raise CrowdinError(f"{context}: {error_message(response)} " - f"(Code: {response.status_code})") - return response.json() + try: + return method(**kwargs) + except APIException as exc: + raise CrowdinError(f"{context}: {crowdin_sdk.error_message(exc)} " + f"(Code: {exc.http_status})") from exc def download(self, url, output_path): response = request_with_retry(self.downloads, "GET", url, attempts=MAX_ATTEMPTS, @@ -84,14 +76,12 @@ def export_and_download_language(client, language, directory, is_source, skip_un allow_unapproved): """Export one language and save it as .xliff. Returns the locale.""" locale = language["locale"] - payload = { - "targetLanguageId": language["id"], - "format": "xliff", - "skipUntranslatedStrings": False if is_source else skip_untranslated, - "exportApprovedOnly": False if is_source else not allow_unapproved, - } - export = client.request("POST", f"projects/{client.project_id}/translations/exports", - f"Export failed for {locale}", json=payload) + export = client.call(f"Export failed for {locale}", + client.api.translations.export_project_translation, + targetLanguageId=language["id"], + format=ExportProjectTranslationFormat.XLIFF, + skipUntranslatedStrings=False if is_source else skip_untranslated, + exportApprovedOnly=False if is_source else not allow_unapproved) client.download(export["data"]["url"], os.path.join(directory, f"{locale}.xliff")) return locale @@ -118,8 +108,7 @@ def main(): client = Crowdin(args.api_token, args.project_id, args.max_workers) print_progress("Retrieving project details...") - project = client.request("GET", f"projects/{args.project_id}", - "Failed to retrieve project details") + project = client.call("Failed to retrieve project details", client.api.projects.get_project) if args.verbose: print(json.dumps(project, indent=2)) source_language = project["data"]["sourceLanguage"] @@ -157,10 +146,9 @@ def main(): if args.glossary_id is not None and args.concept_id is not None: print_progress("Retrieving non-translatable strings...") - terms = client.request( - "GET", f"glossaries/{args.glossary_id}/terms", - "Failed to retrieve non-translatable strings", - params={"conceptId": args.concept_id, "limit": 500}) + terms = client.call("Failed to retrieve non-translatable strings", + client.api.glossaries.list_terms, + glossaryId=args.glossary_id, conceptId=args.concept_id, limit=500) if args.verbose: print(json.dumps(terms, indent=2)) with open(os.path.join(args.download_directory, "_non_translatable_strings.json"), "w", diff --git a/crowdin/report_multiple_translations.py b/crowdin/report_multiple_translations.py index f31e586..47f6b7f 100644 --- a/crowdin/report_multiple_translations.py +++ b/crowdin/report_multiple_translations.py @@ -44,15 +44,15 @@ import os import subprocess import sys -import threading import requests +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) -from shared import discord, retry # noqa: E402 +import crowdin_sdk # noqa: E402 +from shared import discord # noqa: E402 from shared.text import clip # noqa: E402 -API = "https://api.crowdin.com/api/v2" DEFAULT_PROJECT = "618696" KEYRING_ATTRS = ["service", "crowdin", "key", "translation-api-token"] @@ -83,27 +83,10 @@ def get_token(cli_token): "or store it via secret-tool).") -def request_with_retry(session, method, url, **kw): - """A Crowdin API call. A non-retryable 4xx raises, so a caller can read the - body of what it asked for without checking the status first.""" - r = retry.request_with_retry(session, method, url, attempts=10, timeout=60, **kw) - r.raise_for_status() - return r - - -def paged(session, path, **params): - """Yield every item from a paginated Crowdin list endpoint.""" - offset = 0 - limit = 500 - while True: - r = request_with_retry(session, "GET", f"{API}{path}", - params={**params, "limit": limit, "offset": offset}) - data = r.json()["data"] - for row in data: - yield row["data"] - if len(data) < limit: - return - offset += limit +def crowdin_client(token, project_id): + """Ten attempts at sixty seconds: a locale is ~1,400 requests at Crowdin's rate + limit, so a 429 partway through is expected rather than exceptional.""" + return crowdin_sdk.client(token, project_id, attempts=10, timeout=60) def user_label(u): @@ -124,30 +107,18 @@ def user_label(u): # full 80-locale scan is ~1366*80 requests ≈ 40 min. To keep a daily job fast we # scan a ROTATING shard of locales each run (see pick_locales), covering them all # over a cycle. Within a locale we fan strings out across a bounded thread pool. -def scan_locale(session, pid, lang, string_ids, strings, editor_url, max_workers): +def scan_locale(client, lang, string_ids, strings, editor_url, max_workers): """Return the finding dicts for one locale (one slot per 2+-translation group).""" approved_here = {} # stringId -> set(translationId) that are approved - for a in paged(session, f"/projects/{pid}/approvals", languageId=lang): + for a in crowdin_sdk.fetch_all(client.string_translations, "list_translation_approvals", + languageId=lang): approved_here.setdefault(a["stringId"], set()).add(a["translationId"]) print(f"[{lang}] scanning {len(string_ids)} strings " f"({len(approved_here)} with approvals) …", file=sys.stderr) - # requests.Session is not guaranteed thread-safe, so instead of sharing the - # caller's session across the pool, give each worker its own (lazily created, - # carrying the same auth header). - tls = threading.local() - - def worker_session(): - s = getattr(tls, "session", None) - if s is None: - s = requests.Session() - s.headers.update(session.headers) - tls.session = s - return s - def process(sid): - trans = list(paged(worker_session(), f"/projects/{pid}/translations", - stringId=sid, languageId=lang)) + trans = crowdin_sdk.fetch_all(client.string_translations, "list_string_translations", + stringId=sid, languageId=lang) approved_tids = approved_here.get(sid, set()) by_cat = collections.defaultdict(list) for t in trans: @@ -198,13 +169,13 @@ def process(sid): return found -def scan(session, pid, locales, strings, editor_url, max_workers): +def scan(client, locales, strings, editor_url, max_workers): """Return a flat list of finding dicts, one per slot with 2+ translations.""" string_ids = list(strings.keys()) findings = [] for lang in locales: - findings.extend(scan_locale(session, pid, lang, string_ids, strings, - editor_url, max_workers)) + findings.extend(scan_locale(client, lang, string_ids, strings, editor_url, + max_workers)) findings.sort(key=lambda f: (f["locale"], f["identifier"] or "", str(f["pluralCategory"]))) return findings @@ -379,13 +350,12 @@ def main(): sys.exit("No Discord webhook (pass --webhook, set DISCORD_WEBHOOK_URL, or use --dry-run).") pid = args.project_id - session = requests.Session() - session.headers.update({"Authorization": f"Bearer {token}"}) + client = crowdin_client(token, pid) # Project details: identifier + per-language editor codes, used to build # editor URLs that actually point at the right locale. (A string's own webUrl # always targets the first target language, regardless of the locale.) - proj = request_with_retry(session, "GET", f"{API}/projects/{pid}").json()["data"] + proj = client.projects.get_project()["data"] project_slug = proj["identifier"] src_code = proj["sourceLanguage"].get("editorCode") or proj["sourceLanguage"]["id"] editor_code = {l["id"]: (l.get("editorCode") or l["id"]) for l in proj["targetLanguages"]} @@ -412,12 +382,12 @@ def editor_url(lang, sid): print(f"Loading strings for project {pid} …", file=sys.stderr) strings = {} - for s in paged(session, f"/projects/{pid}/strings"): + for s in crowdin_sdk.fetch_all(client.source_strings, "list_strings"): strings[s["id"]] = {"identifier": s.get("identifier"), "text": s.get("text")} print(f" {len(strings)} strings; scanning {len(locales)} locale(s): " f"{', '.join(locales)}", file=sys.stderr) - findings = scan(session, pid, locales, strings, editor_url, args.max_workers) + findings = scan(client, locales, strings, editor_url, args.max_workers) print("\n" + "=" * 80, file=sys.stderr) print(f"Found {len(findings)} slot(s) with 2+ translations across " diff --git a/crowdin/requirements.txt b/crowdin/requirements.txt index 6808702..bb0c329 100644 --- a/crowdin/requirements.txt +++ b/crowdin/requirements.txt @@ -2,6 +2,9 @@ babel==2.17.0 certifi==2024.7.4 charset-normalizer==3.3.2 colorama==0.4.6 +crowdin-api-client==1.29.0 +Deprecated==1.3.1 idna==3.7 requests==2.32.3 -urllib3==2.2.2 \ No newline at end of file +urllib3==2.2.2 +wrapt==2.4.1 diff --git a/crowdin/test_download_translations.py b/crowdin/test_download_translations.py index 3a703c0..343e517 100644 --- a/crowdin/test_download_translations.py +++ b/crowdin/test_download_translations.py @@ -1,4 +1,5 @@ """Run with: cd crowdin && python -m unittest discover""" +import json import os import sys import tempfile @@ -8,9 +9,12 @@ sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +import crowdin_sdk # noqa: E402 import download_translations_from_crowdin as download # noqa: E402 from shared.testing import FakeResponse, FakeSession, NoSleep, NonJsonResponse # noqa: E402 +API = "https://api.crowdin.com/api/v2" + class StreamResponse(FakeResponse): def __init__(self, body): @@ -23,36 +27,46 @@ def iter_content(self, chunk_size): def client(api_responses=(), download_responses=()): crowdin = download.Crowdin("secret", "618696", max_workers=4) - crowdin.session = FakeSession(api_responses) + crowdin_sdk.use_session(crowdin.api, FakeSession(api_responses)) crowdin.downloads = FakeSession(download_responses) return crowdin +def api_session(crowdin): + return crowdin.api.get_api_requestor().session + + +def api_calls(crowdin): + return api_session(crowdin).fixed.calls + + class TestRequests(unittest.TestCase): def test_the_token_goes_to_the_api_and_never_to_the_export_host(self): crowdin = download.Crowdin("secret", "618696", max_workers=4) - self.assertEqual(crowdin.session.headers["Authorization"], "Bearer secret") + self.assertEqual(api_session(crowdin).headers["Authorization"], "Bearer secret") self.assertNotIn("Authorization", crowdin.downloads.headers) def test_a_server_error_is_retried(self): crowdin = client([FakeResponse({}, status_code=503), FakeResponse({"data": {"id": 1}})]) with NoSleep(): - self.assertEqual(crowdin.request("GET", "projects/618696", "project")["data"], {"id": 1}) - self.assertEqual(len(crowdin.session.calls), 2) - self.assertEqual(crowdin.session.calls[0][1], f"{download.API}/projects/618696") + self.assertEqual(crowdin.call("project", crowdin.api.projects.get_project)["data"], + {"id": 1}) + self.assertEqual(len(api_calls(crowdin)), 2) + self.assertEqual(api_calls(crowdin)[0][1], f"{API}/projects/618696") def test_a_client_error_names_the_step_and_crowdins_message(self): crowdin = client([FakeResponse({"error": {"message": "Token invalid"}}, status_code=401)]) with self.assertRaises(download.CrowdinError) as caught: - crowdin.request("GET", "projects/618696", "Failed to retrieve project details") + crowdin.call("Failed to retrieve project details", crowdin.api.projects.get_project) self.assertEqual(str(caught.exception), "Failed to retrieve project details: Token invalid (Code: 401)") def test_a_non_json_error_page_still_reads_as_an_error(self): page = NonJsonResponse() page.status_code = 404 + crowdin = client([page]) with self.assertRaises(download.CrowdinError) as caught: - client([page]).request("GET", "projects/618696", "project") + crowdin.call("project", crowdin.api.projects.get_project) self.assertIn("maintenance", str(caught.exception)) self.assertIn("404", str(caught.exception)) @@ -68,11 +82,12 @@ def test_a_language_is_exported_then_saved_under_its_locale(self): with open(os.path.join(directory, "de-DE.xliff"), "rb") as handle: self.assertEqual(handle.read(), b"") self.assertEqual(locale, "de-DE") - method, url, kwargs = crowdin.session.calls[0] - self.assertEqual((method, url), ("POST", f"{download.API}/projects/618696/translations/exports")) - self.assertEqual(kwargs["json"], {"targetLanguageId": "de", "format": "xliff", - "skipUntranslatedStrings": True, - "exportApprovedOnly": True}) + method, url, kwargs = api_calls(crowdin)[0] + self.assertEqual((method.upper(), url), + ("POST", f"{API}/projects/618696/translations/exports")) + self.assertEqual(json.loads(kwargs["data"]), + {"targetLanguageId": "de", "format": "xliff", + "skipUntranslatedStrings": True, "exportApprovedOnly": True}) self.assertEqual(crowdin.downloads.calls[0][1], "https://storage.example/x.xliff") def test_the_source_language_is_always_exported_whole(self): @@ -82,7 +97,7 @@ def test_the_source_language_is_always_exported_whole(self): download.export_and_download_language( crowdin, {"id": "en", "locale": "en"}, directory, is_source=True, skip_untranslated=True, allow_unapproved=False) - payload = crowdin.session.calls[0][2]["json"] + payload = json.loads(api_calls(crowdin)[0][2]["data"]) self.assertFalse(payload["skipUntranslatedStrings"]) self.assertFalse(payload["exportApprovedOnly"]) diff --git a/crowdin/test_report_multiple_translations.py b/crowdin/test_report_multiple_translations.py index 6f8f347..95f58ff 100644 --- a/crowdin/test_report_multiple_translations.py +++ b/crowdin/test_report_multiple_translations.py @@ -7,28 +7,45 @@ import sys import unittest -import requests +from crowdin_api.exceptions import APIException sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +import crowdin_sdk # noqa: E402 import report_multiple_translations as report # noqa: E402 from shared.testing import FakeResponse, FakeSession, NoSleep, Patched # noqa: E402 -class TestRequestWithRetry(unittest.TestCase): +class TestClient(unittest.TestCase): + def client(self, responses): + client = report.crowdin_client("tok", "618696") + session = FakeSession(responses) + crowdin_sdk.use_session(client, session) + return client, session + def test_a_client_error_raises_rather_than_returning(self): """Callers read the body straight off the response, so a 4xx has to stop them.""" - with self.assertRaises(requests.HTTPError): - report.request_with_retry(FakeSession([FakeResponse({}, status_code=404)]), - "GET", "https://x") + client, _ = self.client([FakeResponse({}, status_code=404)]) + with self.assertRaises(APIException): + client.projects.get_project() def test_crowdins_budget_is_ten_attempts_at_sixty_seconds(self): - session = FakeSession([FakeResponse({}, status_code=503)] * 9 + [FakeResponse({"ok": 1})]) + client, session = self.client([FakeResponse({}, status_code=503)] * 9 + + [FakeResponse({"data": {"ok": 1}})]) with NoSleep(): - self.assertEqual(report.request_with_retry(session, "GET", "https://x").json(), {"ok": 1}) + self.assertEqual(client.projects.get_project()["data"], {"ok": 1}) self.assertEqual(len(session.calls), 10) self.assertEqual({kw["timeout"] for _, _, kw in session.calls}, {60}) + def test_fetch_all_follows_full_pages_and_unwraps_the_envelopes(self): + full = {"data": [{"data": {"id": n}} for n in range(500)]} + client, session = self.client([FakeResponse(full), + FakeResponse({"data": [{"data": {"id": 500}}]})]) + items = crowdin_sdk.fetch_all(client.source_strings, "list_strings") + self.assertEqual(len(items), 501) + self.assertEqual(items[-1], {"id": 500}) + self.assertEqual([kw["params"]["offset"] for _, _, kw in session.calls], [0, 500]) + class TestPostToDiscord(unittest.TestCase): def post(self, responses, messages): diff --git a/github_prs/digest.py b/github_prs/digest.py index 78a0068..3337bb3 100755 --- a/github_prs/digest.py +++ b/github_prs/digest.py @@ -42,18 +42,20 @@ import os import sys from datetime import datetime, timedelta, timezone -from operator import itemgetter +from itertools import islice +from operator import attrgetter import requests +from github import Auth, Github +from github.GithubException import GithubException +from github.GithubRetry import GithubRetry sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) from shared import discord, state as dedup # noqa: E402 from shared.discord import MAX_MESSAGE_TEXT_CHARS # noqa: E402 from shared.env import get_env # noqa: E402 -from shared.retry import request_with_retry # noqa: E402 from shared.text import clip, window_label # noqa: E402 -API = "https://api.github.com" DEFAULT_ORG = "session-foundation" # Three days, because the timer runs on weekdays: Monday's window has to reach back # over the weekend. Overlap between consecutive runs is what --state absorbs. @@ -65,6 +67,7 @@ # https://docs.github.com/en/rest/search#about-search SEARCH_RESULT_LIMIT = 1000 PER_PAGE = 100 +ATTEMPTS = 6 MAINTAINERS_FILE = os.path.join(os.path.dirname(os.path.abspath(__file__)), "maintainers.txt") @@ -80,85 +83,55 @@ def load_maintainers(path): return frozenset(logins) -def github_session(token): - session = requests.Session() - session.headers.update({ - "Authorization": f"Bearer {token}", - "Accept": "application/vnd.github+json", - "X-GitHub-Api-Version": "2022-11-28", - }) - return session +def github_client(token): + """Six attempts with backoff on 429 and 5xx, as shared.retry gives every other + service; GithubRetry adds the secondary rate limit, a 403 GitHub only signals in + the body.""" + retry = GithubRetry(total=ATTEMPTS - 1, backoff_factor=1, backoff_max=30, + retry_after_max=60, status_forcelist=[429, *range(500, 600)]) + return Github(auth=Auth.Token(token), per_page=PER_PAGE, timeout=30, retry=retry) -def fetch_json(session, url, **kwargs): - resp = request_with_retry(session, "GET", url, **kwargs) - if resp.status_code >= 400: - sys.exit(f"GitHub {resp.status_code} on {url}: {resp.text[:300]}") - return resp.json() - - -def fetch_repos(session, org): +def fetch_repos(gh, org): """Names of the org's own, live, public repositories. Private repositories are never reported, whatever the token can see: the digest posts to Discord, and nothing about them belongs there. """ - names, page = set(), 1 - while True: - batch = fetch_json(session, f"{API}/orgs/{org}/repos", - params={"per_page": PER_PAGE, "page": page, "type": "all"}) - for repo in batch: - if repo.get("private") or repo.get("fork") or repo.get("archived"): - continue - names.add(repo["name"]) - if len(batch) < PER_PAGE: - return names - page += 1 - - -def search_open_prs(session, org, max_results=SEARCH_RESULT_LIMIT): + return {repo.name for repo in gh.get_organization(org).get_repos(type="all") + if not (repo.private or repo.fork or repo.archived)} + + +def search_open_prs(gh, org, max_results=SEARCH_RESULT_LIMIT): """Every open PR in the org, newest activity first. Returns (items, truncated). `truncated` is the caller's cue that the counts are - a floor rather than a total. + a floor rather than a total. It comes from the item count, not the search's + totalCount: PyGithub reads that off the last-page link, which GitHub caps at + the same 1000, so it can never exceed what was fetched. """ - items, page = [], 1 + results = gh.search_issues(f"org:{org} is:pr is:open", sort="updated", order="desc") limit = min(max_results, SEARCH_RESULT_LIMIT) - while len(items) < limit: - payload = fetch_json(session, f"{API}/search/issues", params={ - "q": f"org:{org} is:pr is:open", - "sort": "updated", - "order": "desc", - "per_page": PER_PAGE, - "page": page, - # The legacy issue-search syntax is gone; without this the endpoint - # rejects the query rather than falling back to it. - "advanced_search": "true", - }) - batch = payload.get("items", []) - items.extend(batch) - total = payload.get("total_count", len(items)) - if len(batch) < PER_PAGE or len(items) >= total: - return items[:limit], total > len(items[:limit]) - page += 1 - return items[:limit], True + items = list(islice(results, limit)) + return items, len(items) >= limit def repo_name(item): """Repo name for a search result — the API gives only the repository's API URL.""" - return item.get("repository_url", "").rsplit("/", 1)[-1] + return item.repository_url.rsplit("/", 1)[-1] def is_bot(item): - return (item.get("user") or {}).get("type") == "Bot" + return item.user.type == "Bot" def author(item): - return (item.get("user") or {}).get("login") or "?" + return item.user.login or "?" -def parse_time(value): - return datetime.fromisoformat(value) +def timestamp(when): + """GitHub's own form, which is what the state file already holds.""" + return when.strftime("%Y-%m-%dT%H:%M:%SZ") def contributor_prs(items, repos, maintainers): @@ -176,11 +149,11 @@ def contributor_prs(items, repos, maintainers): def in_window(prs, cutoff): """The PRs that have moved at all since the cutoff. A PR nobody has touched in three days is not news, whatever its state here.""" - return [pr for pr in prs if parse_time(pr["updated_at"]) >= cutoff] + return [pr for pr in prs if pr.updated_at >= cutoff] def pr_id(pr): - return str(pr.get("id")) + return str(pr.id) def activity_key(pr): @@ -192,7 +165,7 @@ def activity_key(pr): search result and cost a request per PR that moved; this is the deliberate cheaper half of that trade. """ - return pr.get("updated_at") + return timestamp(pr.updated_at) # ---- Dedup state ----------------------------------------------------------- @@ -200,7 +173,7 @@ def activity_key(pr): # The repo#number is never read back. The file is the first thing anyone opens when # the digest reports the wrong thing, and an id alone identifies nothing. STATE = dedup.Tracker(STATE_VERSION, "PR", pr_id, activity_key, "updated_at", - describe=lambda pr: {"pr": f"{repo_name(pr)}#{pr.get('number')}"}) + describe=lambda pr: {"pr": f"{repo_name(pr)}#{pr.number}"}) DEFAULT_RETENTION_DAYS = STATE.retention_days @@ -232,13 +205,13 @@ def age(then, now): def build_pr_line(pr, now, is_new): marker = NEW_MARKER if is_new else UPDATED_MARKER - stamp = age(parse_time(pr["created_at"] if is_new else pr["updated_at"]), now) - title = clip(pr.get("title"), TITLE_CHARS) - if pr.get("draft"): + stamp = age(pr.created_at if is_new else pr.updated_at, now) + title = clip(pr.title, TITLE_CHARS) + if pr.draft: title = f"[draft] {title}" - comments = pr.get("comments") or 0 + comments = pr.comments or 0 replies = f" · 💬{comments}" if comments else "" - return (f"{marker} [#{pr['number']}]({pr['html_url']}) @{author(pr)} · " + return (f"{marker} [#{pr.number}]({pr.html_url}) @{author(pr)} · " f"{stamp}{replies} · {title}") @@ -274,7 +247,7 @@ def group_by_repo(new, updated, now, max_chars=MAX_MESSAGE_TEXT_CHARS): for prs, field, is_new in ((new, "created_at", True), (updated, "updated_at", False)): group = sorted((pr for pr in prs if repo_name(pr) == repo), - key=itemgetter(field), reverse=True) + key=attrgetter(field), reverse=True) entries += [(build_pr_line(pr, now, is_new), pr_id(pr)) for pr in group] repos.append((split_block(f"**{repo}**", entries, max_chars), len(entries))) repos.sort(key=lambda item: -item[1]) @@ -333,9 +306,14 @@ def main(): required=not args.dry_run) maintainers = load_maintainers(args.maintainers) - session = github_session(token) - repos = fetch_repos(session, org) - items, truncated = search_open_prs(session, org) + gh = github_client(token) + try: + repos = fetch_repos(gh, org) + items, truncated = search_open_prs(gh, org) + except GithubException as exc: + sys.exit(f"GitHub {exc.status}: {str(exc.data)[:300]}") + except requests.RequestException as exc: + sys.exit(f"GitHub unreachable: {exc}") prs = contributor_prs(items, repos, maintainers) now = datetime.now(timezone.utc) diff --git a/github_prs/requirements.txt b/github_prs/requirements.txt index 70429a6..3e75fe2 100644 --- a/github_prs/requirements.txt +++ b/github_prs/requirements.txt @@ -1,2 +1,3 @@ # What the deployment runs. The suite is stdlib unittest and imports nothing else. requests==2.34.2 +PyGithub==2.10.0 diff --git a/github_prs/test_digest.py b/github_prs/test_digest.py index 5e9043d..a553386 100644 --- a/github_prs/test_digest.py +++ b/github_prs/test_digest.py @@ -11,16 +11,22 @@ from datetime import datetime, timedelta, timezone from unittest import mock +from github.Issue import IssueSearchResult +from github.Requester import Requester + sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) import digest # noqa: E402 from shared import discord # noqa: E402 +from shared.testing import NoSleep # noqa: E402 NOW = datetime(2026, 9, 24, 12, 0, tzinfo=timezone.utc) CUTOFF = NOW - timedelta(hours=25) +GH = digest.github_client("token") -def pr(number=1, login="octocat", repo="session-android", created="2026-09-24T08:00:00Z", - updated=None, title="Fix a thing", user_type="User", **extra): +def pr_item(number=1, login="octocat", repo="session-android", created="2026-09-24T08:00:00Z", + updated=None, title="Fix a thing", user_type="User", **extra): + """A search result as the API returns one.""" item = { "id": 10_000 + number, "number": number, @@ -30,11 +36,18 @@ def pr(number=1, login="octocat", repo="session-android", created="2026-09-24T08 "user": {"login": login, "type": user_type}, "created_at": created, "updated_at": updated or created, + "draft": False, + "comments": 0, } item.update(extra) return item +def pr(*args, **kwargs): + """A search result as PyGithub hands it to the digest.""" + return IssueSearchResult(GH.requester, {}, pr_item(*args, **kwargs), completed=True) + + class TestMaintainers(unittest.TestCase): def test_comments_and_blanks_are_ignored(self): with mock.patch("builtins.open", mock.mock_open(read_data= @@ -73,8 +86,7 @@ class TestWindow(unittest.TestCase): def test_only_prs_that_moved_since_the_cutoff_are_considered(self): fresh = pr(1, updated="2026-09-24T09:00:00Z") stale = pr(2, created="2026-08-01T08:00:00Z", updated="2026-08-02T08:00:00Z") - self.assertEqual([p["number"] for p in digest.in_window([fresh, stale], CUTOFF)], - [1]) + self.assertEqual([p.number for p in digest.in_window([fresh, stale], CUTOFF)], [1]) def test_the_cutoff_itself_counts_as_inside_the_window(self): edge = pr(1, updated=CUTOFF.strftime("%Y-%m-%dT%H:%M:%SZ")) @@ -94,21 +106,21 @@ def state(self, *records): def test_a_pr_never_reported_is_new(self): new, changed, unchanged = digest.STATE.partition([pr(1)], digest.STATE.empty()) - self.assertEqual([p["number"] for p in new], [1]) + self.assertEqual([p.number for p in new], [1]) self.assertEqual((changed, unchanged), ([], [])) def test_a_pr_that_moved_since_it_was_reported_is_changed(self): item = pr(1, updated="2026-09-24T09:00:00Z") new, changed, unchanged = digest.STATE.partition( [item], self.state((digest.pr_id(item), "2026-09-20T09:00:00Z"))) - self.assertEqual([p["number"] for p in changed], [1]) + self.assertEqual([p.number for p in changed], [1]) self.assertEqual((new, unchanged), ([], [])) def test_a_pr_that_has_not_moved_is_dropped(self): item = pr(1, updated="2026-09-24T09:00:00Z") new, changed, unchanged = digest.STATE.partition( [item], self.state((digest.pr_id(item), "2026-09-24T09:00:00Z"))) - self.assertEqual([p["number"] for p in unchanged], [1]) + self.assertEqual([p.number for p in unchanged], [1]) self.assertEqual((new, changed), ([], [])) @@ -300,38 +312,91 @@ def test_only_the_first_message_carries_the_header(self): self.assertLess(len(rendered), discord.MAX_MESSAGE_TEXT_CHARS * 2) +class FakeHttpResponse: + def __init__(self, status, headers, payload): + self.status, self.headers, self.payload = status, headers, payload + + def getheaders(self): + return self.headers.items() + + def read(self): + return json.dumps(self.payload) + + +class FakeConnection: + """PyGithub's connection interface, answering from a queue and recording each request.""" + queue, calls = [], [] + + def __init__(self, *args, **kwargs): + pass + + def request(self, verb, url, input, headers, stream=False): + self.calls.append((verb, url)) + + def getresponse(self): + return FakeHttpResponse(*self.queue.pop(0)) + + def close(self): + pass + + +def page(payload, next_page=None): + headers = {"content-type": "application/json"} + if next_page is not None: + headers["link"] = f'; rel="next"' + return 200, headers, payload + + +def repo(name, **flags): + return {"name": name, "fork": False, "archived": False, "private": False, **flags} + + +ORG = {"login": "org", "url": "https://api.github.com/orgs/org"} + + class TestFetching(unittest.TestCase): + def setUp(self): + FakeConnection.queue.clear() + FakeConnection.calls.clear() + Requester.injectConnectionClasses(FakeConnection, FakeConnection) + self.addCleanup(Requester.resetConnectionClasses) + + def fetch(self, *responses, call): + FakeConnection.queue.extend(responses) + with NoSleep(): + return call(digest.github_client("token"), "org") + def test_forks_archived_and_private_repos_are_left_out(self): - page = [{"name": "session-android", "fork": False, "archived": False}, - {"name": "session-pysogs", "fork": True, "archived": False}, - {"name": "retired", "fork": False, "archived": True}, - {"name": "internal", "fork": False, "archived": False, "private": True}] - with mock.patch.object(digest, "fetch_json", return_value=page): - self.assertEqual(digest.fetch_repos(None, "org"), {"session-android"}) + listing = [repo("session-android"), repo("session-pysogs", fork=True), + repo("retired", archived=True), repo("internal", private=True)] + self.assertEqual(self.fetch(page(ORG), page(listing), call=digest.fetch_repos), + {"session-android"}) def test_repo_pagination_follows_full_pages(self): - pages = [[{"name": f"r{n}", "fork": False, "archived": False} - for n in range(digest.PER_PAGE)], - [{"name": "last", "fork": False, "archived": False}]] - with mock.patch.object(digest, "fetch_json", side_effect=pages): - self.assertEqual(len(digest.fetch_repos(None, "org")), digest.PER_PAGE + 1) + first = [repo(f"r{n}") for n in range(digest.PER_PAGE)] + names = self.fetch(page(ORG), page(first, next_page=2), page([repo("last")]), + call=digest.fetch_repos) + self.assertEqual(len(names), digest.PER_PAGE + 1) + self.assertEqual(len(FakeConnection.calls), 3) def test_search_stops_when_the_results_run_out(self): - payload = {"total_count": 2, "items": [pr(1), pr(2)]} - with mock.patch.object(digest, "fetch_json", return_value=payload) as fetch: - items, truncated = digest.search_open_prs(None, "org") - self.assertEqual(len(items), 2) + payload = {"total_count": 2, "incomplete_results": False, + "items": [pr_item(1), pr_item(2)]} + items, truncated = self.fetch(page(payload), call=digest.search_open_prs) + self.assertEqual([p.number for p in items], [1, 2]) self.assertFalse(truncated) - self.assertEqual(fetch.call_count, 1) + self.assertEqual(len(FakeConnection.calls), 1) def test_search_stops_at_the_result_ceiling_rather_than_erroring(self): - full = {"total_count": 1500, - "items": [pr(n) for n in range(digest.PER_PAGE)]} - with mock.patch.object(digest, "fetch_json", return_value=full) as fetch: - items, truncated = digest.search_open_prs(None, "org") + pages = digest.SEARCH_RESULT_LIMIT // digest.PER_PAGE + full = [page({"total_count": 1500, "incomplete_results": False, + "items": [pr_item(n) for n in range(digest.PER_PAGE)]}, + next_page=n + 2) + for n in range(pages)] + items, truncated = self.fetch(*full, call=digest.search_open_prs) self.assertEqual(len(items), digest.SEARCH_RESULT_LIMIT) self.assertTrue(truncated) - self.assertEqual(fetch.call_count, digest.SEARCH_RESULT_LIMIT // digest.PER_PAGE) + self.assertEqual(len(FakeConnection.calls), pages) if __name__ == "__main__": diff --git a/shared/testing.py b/shared/testing.py index da98e4b..dba636a 100644 --- a/shared/testing.py +++ b/shared/testing.py @@ -18,6 +18,10 @@ def __init__(self, payload, status_code=200, retry_after="0"): def json(self): return self._payload + @property + def content(self): + return self.text.encode() + def raise_for_status(self): if self.status_code >= 400: raise requests.HTTPError(f"{self.status_code}", response=self)