From 560f95e351cba5a5145b1cc7b6c6cb223d6eeab4 Mon Sep 17 00:00:00 2001 From: Andrew Halberstadt Date: Mon, 9 Sep 2024 09:48:45 -0400 Subject: [PATCH] feat!: make `client.request` return response object rather than content There's lots of things you might want to do with the response object other than read the comment. As `simple-github` is meant to just be very basic middleware, we shoudn't make assumptions about what users need from the response. Issue: #40 --- README.md | 26 ++++++++++++++++----- src/simple_github/auth.py | 8 +++++-- src/simple_github/client.py | 46 ++++++++++++++----------------------- test/test_client.py | 40 +++++++++++++++++++++----------- test/test_simple_github.py | 9 +++++--- 5 files changed, 75 insertions(+), 54 deletions(-) diff --git a/README.md b/README.md index 7b970a6..7cebeea 100644 --- a/README.md +++ b/README.md @@ -43,9 +43,14 @@ In the simplest case, you can provide an access token to use: from simple_github import TokenClient token = "" async with TokenClient(token) as session: - await session.get("/octocat") + resp = await session.get("/octocat") + resp.raise_for_status() + data = await resp.json() + await resp.close() ``` +The return value is an [aiohttp.ClientResponse][0] object. + If calling synchronously, simply remove the `async` / `await` from the examples: @@ -53,9 +58,16 @@ examples: from simple_github import TokenClient token = "" with TokenClient(token) as session: - session.get("/octocat") + resp = session.get("/octocat") + resp.raise_for_status() + data = resp.json() ``` +In this case the return value is a [requests.Response][1] object. + +[0]: https://docs.aiohttp.org/en/stable/client_reference.html#aiohttp.ClientResponse +[1]: https://requests.readthedocs.io/en/latest/api/#requests.Response + ### Authenticate as a Github App installation To authenticate as an app installation you'll need: @@ -73,14 +85,14 @@ privkey = "" owner = "mozilla-releng" async with AppClient(app_id, privkey, owner=owner) as session: - await session.get("/octocat") + resp = await session.get("/octocat") ``` You can also specify repositories if you want to restrict access. ```python async with AppClient(app_id, privkey, owner=owner, repositories=["simple-github"]) as session: - await session.get("/octocat") + resp = await session.get("/octocat") ``` ### Authenticate as a Github App @@ -90,7 +102,7 @@ administering the app. To do this, simply omit the `owner` argument. ```python async with AppClient(app_id, privkey) as session: - await session.get("/octocat") + resp = await session.get("/octocat") ``` ### Query the REST API @@ -101,7 +113,9 @@ query it by passing in the path fragment to `session.get` or `session.post`. For example, you can list pull requests with a `GET` request: ```python -pulls = await session.get("/repos/mozilla-releng/simple-github/pulls") +resp = await session.get("/repos/mozilla-releng/simple-github/pulls") +pulls = await resp.json() +await resp.close() open_pulls = [p for p in pulls if p["state"] == "open"] ``` diff --git a/src/simple_github/auth.py b/src/simple_github/auth.py index 2f61e8d..2336b15 100644 --- a/src/simple_github/auth.py +++ b/src/simple_github/auth.py @@ -129,7 +129,9 @@ async def _get_installation_id(self) -> str: Returns: str: The app's installation id. """ - installations = await self._client.get("/app/installations") + async with await self._client.get("/app/installations") as response: + response.raise_for_status() + installations = await response.json() assert isinstance(installations, list) for installation in installations: @@ -164,7 +166,9 @@ async def _gen_installation_token(self) -> AsyncGenerator[str, None]: data["repositories"] = self.repositories async def _gentoken() -> str: - result = await self._client.post(query, data=data) + response = await self._client.post(query, data=data) + response.raise_for_status() + result = await response.json() assert isinstance(result, dict) return result["token"] diff --git a/src/simple_github/client.py b/src/simple_github/client.py index b5dff3a..582ef0c 100644 --- a/src/simple_github/client.py +++ b/src/simple_github/client.py @@ -1,16 +1,16 @@ import asyncio import json from abc import abstractmethod -from typing import TYPE_CHECKING, Any, Coroutine, Dict, List, Optional, Union +from typing import TYPE_CHECKING, Any, Coroutine, Dict, Optional, Union -from aiohttp import ClientSession, ContentTypeError +from aiohttp import ClientResponse, ClientSession from gql import Client as GqlClient from gql import gql from gql.client import ReconnectingAsyncClientSession, SyncClientSession from gql.transport.aiohttp import AIOHTTPTransport from gql.transport.requests import RequestsHTTPTransport +from requests import Response as RequestsResponse from requests import Session -from requests.exceptions import JSONDecodeError if TYPE_CHECKING: from simple_github.auth import Auth @@ -18,7 +18,7 @@ GITHUB_API_ENDPOINT = "https://api.github.com" GITHUB_GRAPHQL_ENDPOINT = "https://api.github.com/graphql" -Response = Union[Dict[str, Any], List[Any], str] +Response = Union[RequestsResponse, ClientResponse] RequestData = Optional[Dict[str, Any]] # Implementations of the base class can be either sync or async. @@ -120,7 +120,7 @@ def _get_requests_session(self) -> Session: assert session.transport.session return session.transport.session - def request(self, method: str, query: str, **kwargs) -> Response: + def request(self, method: str, query: str, **kwargs) -> RequestsResponse: """Make a request to Github's REST API. Args: @@ -136,14 +136,9 @@ def request(self, method: str, query: str, **kwargs) -> Response: session = self._get_requests_session() with session.request(method, url, **kwargs) as resp: - if not resp.ok: - resp.raise_for_status() - try: - return resp.json() - except JSONDecodeError: - return resp.text.strip('"') - - def get(self, query: str) -> Response: + return resp + + def get(self, query: str) -> RequestsResponse: """Make a GET request to Github's REST API. Args: @@ -154,7 +149,7 @@ def get(self, query: str) -> Response: """ return self.request("GET", query) - def post(self, query: str, data: RequestData = None) -> Response: + def post(self, query: str, data: RequestData = None) -> RequestsResponse: """Make a POST request to Github's REST API. Args: @@ -166,7 +161,7 @@ def post(self, query: str, data: RequestData = None) -> Response: """ return self.request("POST", query, data=json.dumps(data)) - def put(self, query: str, data: RequestData = None) -> Response: + def put(self, query: str, data: RequestData = None) -> RequestsResponse: """Make a PUT request to Github's REST API. Args: @@ -178,7 +173,7 @@ def put(self, query: str, data: RequestData = None) -> Response: """ return self.request("PUT", query, data=json.dumps(data)) - def patch(self, query: str, data: RequestData = None) -> Response: + def patch(self, query: str, data: RequestData = None) -> RequestsResponse: """Make a PATCH request to Github's REST API. Args: @@ -263,7 +258,7 @@ async def _get_aiohttp_session(self) -> ClientSession: assert session.transport.session return session.transport.session - async def request(self, method: str, query: str, **kwargs: Any) -> Response: + async def request(self, method: str, query: str, **kwargs: Any) -> ClientResponse: """Make a request to Github's REST API. Args: @@ -277,16 +272,9 @@ async def request(self, method: str, query: str, **kwargs: Any) -> Response: """ url = f"{GITHUB_API_ENDPOINT}/{query.lstrip('/')}" session = await self._get_aiohttp_session() + return await session.request(method, url, **kwargs) - async with session.request(method, url, **kwargs) as resp: - if not resp.ok: - resp.raise_for_status() - try: - return await resp.json() - except ContentTypeError: - return (await resp.text()).strip('"') - - async def get(self, query: str) -> Response: + async def get(self, query: str) -> ClientResponse: """Make a GET request to Github's REST API. Args: @@ -297,7 +285,7 @@ async def get(self, query: str) -> Response: """ return await self.request("GET", query) - async def post(self, query: str, data: RequestData = None) -> Response: + async def post(self, query: str, data: RequestData = None) -> ClientResponse: """Make a POST request to Github's REST API. Args: @@ -309,7 +297,7 @@ async def post(self, query: str, data: RequestData = None) -> Response: """ return await self.request("POST", query, data=json.dumps(data)) - async def put(self, query: str, data: RequestData = None) -> Response: + async def put(self, query: str, data: RequestData = None) -> ClientResponse: """Make a PUT request to Github's REST API. Args: @@ -321,7 +309,7 @@ async def put(self, query: str, data: RequestData = None) -> Response: """ return await self.request("PUT", query, data=json.dumps(data)) - async def patch(self, query: str, data: RequestData = None) -> Response: + async def patch(self, query: str, data: RequestData = None) -> ClientResponse: """Make a PATCH request to Github's REST API. Args: diff --git a/test/test_client.py b/test/test_client.py index 703857c..0a18cd9 100644 --- a/test/test_client.py +++ b/test/test_client.py @@ -93,28 +93,33 @@ async def test_async_client_rest(aioresponses, async_client): url = f"{GITHUB_API_ENDPOINT}/octocat" aioresponses.get(url, status=200, payload={"answer": 42}) - result = await client.get("/octocat") + resp = await client.get("/octocat") + result = await resp.json() assert result == {"answer": 42} aioresponses.post(url, status=200, payload={"answer": 42}) - result = await client.post("/octocat", data={"foo": "bar"}) + resp = await client.post("/octocat", data={"foo": "bar"}) + result = await resp.json() assert result == {"answer": 42} aioresponses.put(url, status=200, payload={"answer": 42}) - result = await client.put("/octocat", data={"foo": "bar"}) + resp = await client.put("/octocat", data={"foo": "bar"}) + result = await resp.json() assert result == {"answer": 42} aioresponses.patch(url, status=200, payload={"answer": 42}) - result = await client.patch("/octocat", data={"foo": "bar"}) + resp = await client.patch("/octocat", data={"foo": "bar"}) + result = await resp.json() assert result == {"answer": 42} aioresponses.delete(url, status=200) - result = await client.delete("/octocat") + await client.delete("/octocat") aioresponses.assert_called_with(url, "DELETE", data="null") aioresponses.get(url, status=401) with pytest.raises(ClientResponseError): - await client.get("/octocat") + resp = await client.get("/octocat") + resp.raise_for_status() def test_sync_client_rest(responses, sync_client): @@ -122,23 +127,27 @@ def test_sync_client_rest(responses, sync_client): url = f"{GITHUB_API_ENDPOINT}/octocat" responses.get(url, status=200, json={"answer": 42}) - result = client.get("/octocat") + resp = client.get("/octocat") + result = resp.json() assert result == {"answer": 42} responses.post(url, status=200, json={"answer": 42}) - result = client.post("/octocat", data={"foo": "bar"}) + resp = client.post("/octocat", data={"foo": "bar"}) + result = resp.json() assert result == {"answer": 42} responses.put(url, status=200, json={"answer": 42}) - result = client.put("/octocat", data={"foo": "bar"}) + resp = client.put("/octocat", data={"foo": "bar"}) + result = resp.json() assert result == {"answer": 42} responses.patch(url, status=200, json={"answer": 42}) - result = client.patch("/octocat", data={"foo": "bar"}) + resp = client.patch("/octocat", data={"foo": "bar"}) + result = resp.json() assert result == {"answer": 42} responses.delete(url, status=200) - result = client.delete("/octocat") + client.delete("/octocat") resp = responses.calls[-1].response assert resp.url == url assert resp.request.method == "DELETE" @@ -146,7 +155,8 @@ def test_sync_client_rest(responses, sync_client): responses.get(url, status=401) with pytest.raises(HTTPError): - client.get("/octocat") + resp = client.get("/octocat") + resp.raise_for_status() @pytest.mark.asyncio @@ -159,7 +169,8 @@ async def test_async_client_rest_with_text(aioresponses, async_client): status=200, payload=text, ) - result = await client.get("/octocat") + resp = await client.get("/octocat") + result = (await resp.text()).strip('"') assert result == text @@ -172,7 +183,8 @@ def test_sync_client_rest_with_text(responses, sync_client): status=200, json=text, ) - result = client.get("/octocat") + resp = client.get("/octocat") + result = resp.json() assert result == text diff --git a/test/test_simple_github.py b/test/test_simple_github.py index 74911fc..95268fb 100644 --- a/test/test_simple_github.py +++ b/test/test_simple_github.py @@ -11,7 +11,8 @@ async def test_token_client(aioresponses): ) async with TokenClient("abc") as client: - result = await client.get("/octocat") + resp = await client.get("/octocat") + result = await resp.json() assert result == {"foo": "bar"} @@ -22,7 +23,8 @@ async def test_app_client(aioresponses, privkey): ) async with AppClient(id=123, privkey=privkey) as client: - result = await client.get("/octocat") + resp = await client.get("/octocat") + result = await resp.json() assert result == {"foo": "bar"} @@ -46,5 +48,6 @@ async def test_app_installation_client(aioresponses, privkey): ) async with AppClient(id=123, privkey=privkey, owner=owner) as client: - result = await client.get("/octocat") + resp = await client.get("/octocat") + result = await resp.json() assert result == {"foo": "bar"}