From 17a4100feb60233c3bef358d93912125731857e5 Mon Sep 17 00:00:00 2001 From: Rohit Malhotra Date: Fri, 31 Jan 2025 15:20:28 -0500 Subject: [PATCH] Refactor: Move Github endpoint logic to GithubService class (#6558) Co-authored-by: openhands --- openhands/server/config/server_config.py | 2 + openhands/server/routes/github.py | 122 +++++--------------- openhands/server/routes/settings.py | 8 +- openhands/server/services/github_service.py | 81 ++++++++++++- 4 files changed, 109 insertions(+), 104 deletions(-) diff --git a/openhands/server/config/server_config.py b/openhands/server/config/server_config.py index ae86d8d43ade..456567ef5783 100644 --- a/openhands/server/config/server_config.py +++ b/openhands/server/config/server_config.py @@ -18,6 +18,8 @@ class ServerConfig(ServerConfigInterface): ) conversation_manager_class: str = 'openhands.server.conversation_manager.standalone_conversation_manager.StandaloneConversationManager' + github_service_class: str = 'openhands.server.services.github_service.GitHubService' + def verify_config(self): if self.config_cls: raise ValueError('Unexpected config path provided') diff --git a/openhands/server/routes/github.py b/openhands/server/routes/github.py index e47d34467a9c..ae7e22240609 100644 --- a/openhands/server/routes/github.py +++ b/openhands/server/routes/github.py @@ -1,10 +1,10 @@ -import httpx -import requests from fastapi import APIRouter, Depends, HTTPException, Request, status from fastapi.responses import JSONResponse -from openhands.server.auth import get_github_token -from openhands.utils.async_utils import call_sync_from_async +from openhands.server.auth import get_github_token, get_user_id +from openhands.server.services.github_service import GitHubService +from openhands.server.shared import server_config +from openhands.utils.import_utils import get_impl app = APIRouter(prefix='/api/github') @@ -20,6 +20,9 @@ def require_github_token(request: Request): return github_token +GithubServiceImpl = get_impl(GitHubService, server_config.github_service_class) + + @app.get('/repositories') async def get_github_repositories( page: int = 1, @@ -27,78 +30,32 @@ async def get_github_repositories( sort: str = 'pushed', installation_id: int | None = None, github_token: str = Depends(require_github_token), + github_user_id: str | None = Depends(get_user_id), ): - params: dict[str, str] = { - 'page': str(page), - 'per_page': str(per_page), - } - if installation_id: - github_api_url = ( - f'https://api.github.com/user/installations/{installation_id}/repositories' - ) - else: - github_api_url = 'https://api.github.com/user/repos' - params['sort'] = sort - - headers = generate_github_headers(github_token) - - try: - async with httpx.AsyncClient() as client: - response = await client.get(github_api_url, headers=headers, params=params) - response.raise_for_status() # Raise an error for HTTP codes >= 400 - json_response = JSONResponse(content=response.json()) - - # Forward the Link header if it exists - if 'Link' in response.headers: - json_response.headers['Link'] = response.headers['Link'] - - return json_response - - except requests.exceptions.RequestException as e: - raise HTTPException( - status_code=response.status_code if response else 500, - detail=f'Error fetching repositories: {str(e)}', - ) + print('got user id ', github_user_id) + client = GithubServiceImpl(github_token, github_user_id) + return await client.fetch_response( + 'get_repositories', page, per_page, sort, installation_id + ) @app.get('/user') -async def get_github_user(github_token: str = Depends(require_github_token)): - headers = generate_github_headers(github_token) - try: - async with httpx.AsyncClient() as client: - response = await client.get('https://api.github.com/user', headers=headers) - response.raise_for_status() # Raise an error for HTTP codes >= 400 - json_response = JSONResponse(content=response.json()) - - return json_response - - except requests.exceptions.RequestException as e: - raise HTTPException( - status_code=response.status_code if response else 500, - detail=f'Error fetching user: {str(e)}', - ) +async def get_github_user( + github_token: str = Depends(require_github_token), + github_user_id: str | None = Depends(get_user_id), +): + client = GithubServiceImpl(github_token, github_user_id) + return await client.fetch_response('get_user') @app.get('/installations') async def get_github_installation_ids( github_token: str = Depends(require_github_token), + github_user_id: str | None = Depends(get_user_id), ): - headers = generate_github_headers(github_token) - try: - async with httpx.AsyncClient() as client: - response = await client.get( - 'https://api.github.com/user/installations', headers=headers - ) - response.raise_for_status() - data = response.json() - ids = [installation['id'] for installation in data['installations']] - return JSONResponse(content=ids) - - except httpx.HTTPError as e: - raise HTTPException( - status_code=e.response.status_code if hasattr(e, 'response') else 500, - detail=f'Error fetching installations: {str(e)}', - ) + client = GithubServiceImpl(github_token, github_user_id) + installations = await client.get_installation_ids() + return JSONResponse(content=[i['id'] for i in installations]) @app.get('/search/repositories') @@ -108,37 +65,10 @@ async def search_github_repositories( sort: str = 'stars', order: str = 'desc', github_token: str = Depends(require_github_token), + github_user_id: str | None = Depends(get_user_id), ): - headers = generate_github_headers(github_token) - params = { - 'q': query, - 'per_page': per_page, - 'sort': sort, - 'order': order, - } - - try: - response = await call_sync_from_async( - requests.get, - 'https://api.github.com/search/repositories', - headers=headers, - params=params, - ) - response.raise_for_status() - except requests.exceptions.RequestException as e: - raise HTTPException( - status_code=response.status_code if response else 500, - detail=f'Error searching repositories: {str(e)}', - ) - + client = GithubServiceImpl(github_token, github_user_id) + response = await client.search_repositories(query, per_page, sort, order) json_response = JSONResponse(content=response.json()) response.close() - return json_response - - -def generate_github_headers(token: str) -> dict[str, str]: - return { - 'Authorization': f'Bearer {token}', - 'Accept': 'application/vnd.github.v3+json', - } diff --git a/openhands/server/routes/settings.py b/openhands/server/routes/settings.py index 02cfd28c4d87..40366253ce88 100644 --- a/openhands/server/routes/settings.py +++ b/openhands/server/routes/settings.py @@ -6,7 +6,6 @@ from openhands.server.services.github_service import GitHubService from openhands.server.settings import GETSettingsModel, POSTSettingsModel, Settings from openhands.server.shared import SettingsStoreImpl, config -from openhands.utils.async_utils import call_sync_from_async app = APIRouter(prefix='/api') @@ -51,8 +50,11 @@ async def store_settings( try: # We check if the token is valid by getting the user # If the token is invalid, this will raise an exception - github = GitHubService(settings.github_token) - await call_sync_from_async(github.get_user) + github = GitHubService(settings.github_token, None) + response = await github.get_user() + if response.status_code != status.HTTP_200_OK: + raise Exception('Invalid Github Token') + except Exception as e: logger.warning(f'Invalid GitHub token: {e}') return JSONResponse( diff --git a/openhands/server/services/github_service.py b/openhands/server/services/github_service.py index 2fae3134a0dc..576ed05b6f36 100644 --- a/openhands/server/services/github_service.py +++ b/openhands/server/services/github_service.py @@ -1,16 +1,87 @@ +import httpx import requests +from fastapi.responses import JSONResponse + +from openhands.server.shared import server_config +from openhands.utils.async_utils import call_sync_from_async class GitHubService: - def __init__(self, token: str): + BASE_URL = 'https://api.github.com' + + def __init__(self, token: str, user_id: str | None): self.token = token + self.user_id = user_id self.headers = { 'Authorization': f'Bearer {token}', 'Accept': 'application/vnd.github.v3+json', } - def get_user(self): - response = requests.get('https://api.github.com/user', headers=self.headers) - response.raise_for_status() + def _has_token_expired(self, status_code: int): + return status_code == 401 + + async def _get_latest_token(self): + pass + + async def _fetch_data(self, url: str, params: dict | None = None): + try: + async with httpx.AsyncClient() as client: + response = await client.get(url, headers=self.headers, params=params) + if server_config.app_mode == 'SAAS' and self._has_token_expired( + response.status_code + ): + await self._get_latest_token() + response = await client.get( + url, headers=self.headers, params=params + ) + response.raise_for_status() + return response + + except httpx.HTTPStatusError as e: + status_code = e.response.status_code + error_detail = e.response.text + + return httpx.Response( + status_code=status_code, json=f'GitHub API error: {error_detail}' + ) + except httpx.HTTPError as e: + return httpx.Response(status_code=500, json=f'HTTP error: {str(e)}') + + async def get_user(self): + url = f'{self.BASE_URL}/user' + return await self._fetch_data(url) + + async def get_repositories( + self, page: int, per_page: int, sort: str, installation_id: int | None + ): + params = {'page': str(page), 'per_page': str(per_page)} + if installation_id: + url = f'{self.BASE_URL}/user/installations/{installation_id}/repositories' + else: + url = f'{self.BASE_URL}/user/repos' + params['sort'] = sort + return await self._fetch_data(url, params) + + async def get_installation_ids(self): + url = f'{self.BASE_URL}/user/installations' + response = await self._fetch_data(url) + data = response.json() + return data.get('installations', []) + + async def search_repositories( + self, query: str, per_page: int, sort: str, order: str + ): + url = f'{self.BASE_URL}/search/repositories' + params = {'q': query, 'per_page': per_page, 'sort': sort, 'order': order} + return await call_sync_from_async( + requests.get, url, headers=self.headers, params=params + ) - return response.json() + async def fetch_response(self, method: str, *args, **kwargs): + response = await getattr(self, method)(*args, **kwargs) + json_response = JSONResponse( + content=response.json(), status_code=response.status_code + ) + if 'Link' in response.headers: + json_response.headers['Link'] = response.headers['Link'] + return json_response