diff --git a/python/src/github_docs/client.py b/python/src/github_docs/client.py index 1210218..0c54869 100644 --- a/python/src/github_docs/client.py +++ b/python/src/github_docs/client.py @@ -236,7 +236,19 @@ def _request( try: with urllib.request.urlopen(req, timeout=self.config.timeout) as resp: raw = resp.read() - parsed = json.loads(raw) if raw else {} + try: + parsed = json.loads(raw) if raw else {} + except ValueError as e: + # Something in front of GitHub answered 2xx with a page + # rather than JSON. ValueError rather than JSONDecodeError, + # because bytes that are not UTF-8 fail as a + # UnicodeDecodeError before they ever reach the parser. The + # body is not quoted back, for the same reason as on the + # error path: the status is the actionable part. + raise GitHubDocsError( + self._redact(f"GitHub API returned an undecodable response (HTTP {resp.status})"), + status=resp.status, + ) from e return resp.status, parsed except urllib.error.HTTPError as e: raw = e.read() @@ -252,6 +264,12 @@ def _request( ) from e except urllib.error.URLError as e: raise GitHubDocsError(self._redact(f"could not reach GitHub: {e.reason}")) from e + except OSError as e: + # urlopen wraps a failure to connect in URLError, but a timeout or a + # reset while the body is being read arrives bare. Both mean the + # edit did not land, and the README promises a caller that every + # such failure is a GitHubDocsError. + raise GitHubDocsError(self._redact(f"could not reach GitHub: {e}")) from e # -- Paths -------------------------------------------------------------- diff --git a/python/tests/test_client.py b/python/tests/test_client.py index 893ec23..3b71449 100644 --- a/python/tests/test_client.py +++ b/python/tests/test_client.py @@ -9,6 +9,7 @@ import base64 import json +import socket import unittest import urllib.error import urllib.parse @@ -536,6 +537,35 @@ def test_a_network_failure_becomes_a_readable_error(self): self.assertIn("could not reach GitHub", str(ctx.exception)) self.assertIsNone(ctx.exception.status) + def test_a_timeout_while_reading_the_body_becomes_a_readable_error(self): + # urlopen only wraps a connect timeout in URLError; one raised by + # read() arrives as a bare socket.timeout. + resp = _response({}) + resp.read.side_effect = socket.timeout("timed out") + with mock.patch("urllib.request.urlopen", return_value=resp): + with self.assertRaises(GitHubDocsError) as ctx: + self.client.get_default_branch() + self.assertIn("could not reach GitHub", str(ctx.exception)) + self.assertIsNone(ctx.exception.status) + + def test_a_success_body_that_is_not_json_becomes_an_error_with_its_status(self): + # The error path already tolerates a non-JSON body; the success path + # must too, without quoting the body back. + resp = _response(None, raw=b"captive portal") + with mock.patch("urllib.request.urlopen", return_value=resp): + with self.assertRaises(GitHubDocsError) as ctx: + self.client.get_default_branch() + self.assertEqual(ctx.exception.status, 200) + self.assertNotIn("captive portal", str(ctx.exception)) + + def test_a_success_body_that_is_not_utf8_becomes_an_error_with_its_status(self): + # Undecodable bytes fail before the JSON parser, as a UnicodeDecodeError. + resp = _response(None, raw=b"\x80\x81 not text") + with mock.patch("urllib.request.urlopen", return_value=resp): + with self.assertRaises(GitHubDocsError) as ctx: + self.client.get_default_branch() + self.assertEqual(ctx.exception.status, 200) + if __name__ == "__main__": unittest.main()