From 0a93ffea16187163bd6b8b39981ae53fa66ee3df Mon Sep 17 00:00:00 2001 From: "sp.wack" <83104063+amanape@users.noreply.github.com> Date: Mon, 13 Jan 2025 23:12:50 +0400 Subject: [PATCH] chore: Move GH requests to the server (#6217) --- frontend/__tests__/api/github.test.ts | 47 --------- .../github/github-repo-selector.test.tsx | 5 +- frontend/src/api/github.ts | 82 --------------- frontend/src/api/open-hands.ts | 39 ++++++++ .../github-repositories-suggestion-box.tsx | 2 +- .../src/hooks/query/use-app-installations.ts | 7 +- frontend/src/hooks/query/use-github-user.ts | 4 +- .../src/hooks/query/use-latest-repo-commit.ts | 17 ---- .../hooks/query/use-search-repositories.ts | 5 +- openhands/server/app.py | 2 + openhands/server/middleware.py | 13 +++ openhands/server/routes/github.py | 99 ++++++++++++++++--- 12 files changed, 151 insertions(+), 171 deletions(-) delete mode 100644 frontend/__tests__/api/github.test.ts delete mode 100644 frontend/src/hooks/query/use-latest-repo-commit.ts diff --git a/frontend/__tests__/api/github.test.ts b/frontend/__tests__/api/github.test.ts deleted file mode 100644 index 5a659d4b71e6a..0000000000000 --- a/frontend/__tests__/api/github.test.ts +++ /dev/null @@ -1,47 +0,0 @@ -import { describe, expect, it, vi } from "vitest"; -import { retrieveLatestGitHubCommit } from "../../src/api/github"; - -describe("retrieveLatestGitHubCommit", () => { - const { githubGetMock } = vi.hoisted(() => ({ - githubGetMock: vi.fn(), - })); - - vi.mock("../../src/api/github-axios-instance", () => ({ - github: { - get: githubGetMock, - }, - })); - - it("should return the latest commit when repository has commits", async () => { - const mockCommit = { - sha: "123abc", - commit: { - message: "Initial commit", - }, - }; - - githubGetMock.mockResolvedValueOnce({ - data: [mockCommit], - }); - - const result = await retrieveLatestGitHubCommit("user/repo"); - expect(result).toEqual(mockCommit); - }); - - it("should return null when repository is empty", async () => { - const error = new Error("Repository is empty"); - (error as any).response = { status: 409 }; - githubGetMock.mockRejectedValueOnce(error); - - const result = await retrieveLatestGitHubCommit("user/empty-repo"); - expect(result).toBeNull(); - }); - - it("should throw error for other error cases", async () => { - const error = new Error("Network error"); - (error as any).response = { status: 500 }; - githubGetMock.mockRejectedValueOnce(error); - - await expect(retrieveLatestGitHubCommit("user/repo")).rejects.toThrow(); - }); -}); diff --git a/frontend/__tests__/components/features/github/github-repo-selector.test.tsx b/frontend/__tests__/components/features/github/github-repo-selector.test.tsx index 8d507294d9c16..5ef98b251af98 100644 --- a/frontend/__tests__/components/features/github/github-repo-selector.test.tsx +++ b/frontend/__tests__/components/features/github/github-repo-selector.test.tsx @@ -3,7 +3,6 @@ import { describe, expect, it, vi } from "vitest"; import { renderWithProviders } from "test-utils"; import { GitHubRepositorySelector } from "#/components/features/github/github-repo-selector"; import OpenHands from "#/api/open-hands"; -import * as GitHubAPI from "#/api/github"; describe("GitHubRepositorySelector", () => { const onInputChangeMock = vi.fn(); @@ -60,8 +59,8 @@ describe("GitHubRepositorySelector", () => { ]; const searchPublicRepositoriesSpy = vi.spyOn( - GitHubAPI, - "searchPublicRepositories", + OpenHands, + "searchGitHubRepositories", ); searchPublicRepositoriesSpy.mockResolvedValue(mockSearchedRepos); diff --git a/frontend/src/api/github.ts b/frontend/src/api/github.ts index 256968536c9bf..c69ca870d751d 100644 --- a/frontend/src/api/github.ts +++ b/frontend/src/api/github.ts @@ -1,19 +1,6 @@ import { extractNextPageFromLink } from "#/utils/extract-next-page-from-link"; -import { github } from "./github-axios-instance"; import { openHands } from "./open-hands-axios"; -/** - * Given the user, retrieves app installations IDs for OpenHands Github App - * Uses user access token for Github App - */ -export const retrieveGitHubAppInstallations = async (): Promise => { - const response = await github.get( - "/user/installations", - ); - - return response.data.installations.map((installation) => installation.id); -}; - /** * Retrieves repositories where OpenHands Github App has been installed * @param installationIndex Pagination cursor position for app installation IDs @@ -82,72 +69,3 @@ export const retrieveGitHubUserRepositories = async ( return { data: response.data, nextPage }; }; - -/** - * Given a GitHub token, retrieves the authenticated user - * @returns The authenticated user or an error response - */ -export const retrieveGitHubUser = async () => { - const response = await github.get("/user"); - - const { data } = response; - - const user: GitHubUser = { - id: data.id, - login: data.login, - avatar_url: data.avatar_url, - company: data.company, - name: data.name, - email: data.email, - }; - - return user; -}; - -export const searchPublicRepositories = async ( - query: string, - per_page = 5, - sort: "" | "updated" | "stars" | "forks" = "stars", - order: "desc" | "asc" = "desc", -): Promise => { - const response = await github.get<{ items: GitHubRepository[] }>( - "/search/repositories", - { - params: { - q: query, - per_page, - sort, - order, - }, - }, - ); - return response.data.items; -}; - -export const retrieveLatestGitHubCommit = async ( - repository: string, -): Promise => { - try { - const response = await github.get( - `/repos/${repository}/commits`, - { - params: { - per_page: 1, - }, - }, - ); - return response.data[0] || null; - } catch (error) { - if (!error || typeof error !== "object") { - throw new Error("Unknown error occurred"); - } - const axiosError = error as { response?: { status: number } }; - if (axiosError.response?.status === 409) { - // Repository is empty, no commits yet - return null; - } - throw new Error( - error instanceof Error ? error.message : "Unknown error occurred", - ); - } -}; diff --git a/frontend/src/api/open-hands.ts b/frontend/src/api/open-hands.ts index 08dec1ebe258e..bcb183a106a76 100644 --- a/frontend/src/api/open-hands.ts +++ b/frontend/src/api/open-hands.ts @@ -315,6 +315,45 @@ class OpenHands { const data = await openHands.post("/api/settings", settings); return data.status === 200; } + + static async getGitHubUser(): Promise { + const response = await openHands.get("/api/github/user"); + + const { data } = response; + + const user: GitHubUser = { + id: data.id, + login: data.login, + avatar_url: data.avatar_url, + company: data.company, + name: data.name, + email: data.email, + }; + + return user; + } + + static async getGitHubUserInstallationIds(): Promise { + const response = await openHands.get("/api/github/installations"); + return response.data; + } + + static async searchGitHubRepositories( + query: string, + per_page = 5, + ): Promise { + const response = await openHands.get<{ items: GitHubRepository[] }>( + "/api/github/search/repositories", + { + params: { + query, + per_page, + }, + }, + ); + + return response.data.items; + } } export default OpenHands; diff --git a/frontend/src/components/features/github/github-repositories-suggestion-box.tsx b/frontend/src/components/features/github/github-repositories-suggestion-box.tsx index 3ad3f65a21960..d2a246dbab695 100644 --- a/frontend/src/components/features/github/github-repositories-suggestion-box.tsx +++ b/frontend/src/components/features/github/github-repositories-suggestion-box.tsx @@ -59,7 +59,7 @@ export function GitHubRepositoriesSuggestionBox({ ) : ( diff --git a/frontend/src/hooks/query/use-app-installations.ts b/frontend/src/hooks/query/use-app-installations.ts index 861ed4bd27a1b..e22bd7805e0c4 100644 --- a/frontend/src/hooks/query/use-app-installations.ts +++ b/frontend/src/hooks/query/use-app-installations.ts @@ -1,7 +1,7 @@ import { useQuery } from "@tanstack/react-query"; import { useAuth } from "#/context/auth-context"; import { useConfig } from "./use-config"; -import { retrieveGitHubAppInstallations } from "#/api/github"; +import OpenHands from "#/api/open-hands"; export const useAppInstallations = () => { const { data: config } = useConfig(); @@ -9,10 +9,7 @@ export const useAppInstallations = () => { return useQuery({ queryKey: ["installations", gitHubToken, config?.GITHUB_CLIENT_ID], - queryFn: async () => { - const data = await retrieveGitHubAppInstallations(); - return data; - }, + queryFn: OpenHands.getGitHubUserInstallationIds, enabled: !!gitHubToken && !!config?.GITHUB_CLIENT_ID && diff --git a/frontend/src/hooks/query/use-github-user.ts b/frontend/src/hooks/query/use-github-user.ts index ff7cd18457fc2..9841f6524cda1 100644 --- a/frontend/src/hooks/query/use-github-user.ts +++ b/frontend/src/hooks/query/use-github-user.ts @@ -1,9 +1,9 @@ import { useQuery } from "@tanstack/react-query"; import React from "react"; import posthog from "posthog-js"; -import { retrieveGitHubUser } from "#/api/github"; import { useAuth } from "#/context/auth-context"; import { useConfig } from "./use-config"; +import OpenHands from "#/api/open-hands"; export const useGitHubUser = () => { const { gitHubToken, setUserId } = useAuth(); @@ -11,7 +11,7 @@ export const useGitHubUser = () => { const user = useQuery({ queryKey: ["user", gitHubToken], - queryFn: retrieveGitHubUser, + queryFn: OpenHands.getGitHubUser, enabled: !!gitHubToken && !!config?.APP_MODE, retry: false, }); diff --git a/frontend/src/hooks/query/use-latest-repo-commit.ts b/frontend/src/hooks/query/use-latest-repo-commit.ts deleted file mode 100644 index 9a9b8a9611578..0000000000000 --- a/frontend/src/hooks/query/use-latest-repo-commit.ts +++ /dev/null @@ -1,17 +0,0 @@ -import { useQuery } from "@tanstack/react-query"; -import { retrieveLatestGitHubCommit } from "#/api/github"; -import { useAuth } from "#/context/auth-context"; - -interface UseLatestRepoCommitConfig { - repository: string | null; -} - -export const useLatestRepoCommit = (config: UseLatestRepoCommitConfig) => { - const { gitHubToken } = useAuth(); - - return useQuery({ - queryKey: ["latest_commit", gitHubToken, config.repository], - queryFn: () => retrieveLatestGitHubCommit(config.repository!), - enabled: !!gitHubToken && !!config.repository, - }); -}; diff --git a/frontend/src/hooks/query/use-search-repositories.ts b/frontend/src/hooks/query/use-search-repositories.ts index 4cd343c0a55ab..27d6e2be9d1ea 100644 --- a/frontend/src/hooks/query/use-search-repositories.ts +++ b/frontend/src/hooks/query/use-search-repositories.ts @@ -1,12 +1,11 @@ import { useQuery } from "@tanstack/react-query"; -import { searchPublicRepositories } from "#/api/github"; +import OpenHands from "#/api/open-hands"; export function useSearchRepositories(query: string) { return useQuery({ queryKey: ["repositories", query], - queryFn: () => searchPublicRepositories(query, 3), + queryFn: () => OpenHands.searchGitHubRepositories(query, 3), enabled: !!query, select: (data) => data.map((repo) => ({ ...repo, is_public: true })), - initialData: [], }); } diff --git a/openhands/server/app.py b/openhands/server/app.py index 1ece8476151a0..7387892d08dc4 100644 --- a/openhands/server/app.py +++ b/openhands/server/app.py @@ -11,6 +11,7 @@ import openhands.agenthub # noqa F401 (we import this to get the agents registered) from openhands.server.middleware import ( AttachConversationMiddleware, + GitHubTokenMiddleware, InMemoryRateLimiter, LocalhostCORSMiddleware, NoCacheMiddleware, @@ -44,6 +45,7 @@ async def _lifespan(app: FastAPI): allow_headers=['*'], ) +app.add_middleware(GitHubTokenMiddleware) app.add_middleware(NoCacheMiddleware) app.add_middleware( RateLimitMiddleware, rate_limiter=InMemoryRateLimiter(requests=10, seconds=1) diff --git a/openhands/server/middleware.py b/openhands/server/middleware.py index a7b30e479bbc4..4b50344e33cee 100644 --- a/openhands/server/middleware.py +++ b/openhands/server/middleware.py @@ -166,3 +166,16 @@ async def __call__(self, request: Request, call_next: Callable): await self._detach_session(request) return response + + +class GitHubTokenMiddleware(BaseHTTPMiddleware): + async def dispatch(self, request: Request, call_next): + if request.url.path.startswith('/api/github'): + github_token = request.headers.get('X-GitHub-Token') + if not github_token: + return JSONResponse( + status_code=400, + content={'error': 'Missing X-GitHub-Token header'}, + ) + request.state.github_token = github_token + return await call_next(request) diff --git a/openhands/server/routes/github.py b/openhands/server/routes/github.py index 7ec225767b447..7e7accde40a57 100644 --- a/openhands/server/routes/github.py +++ b/openhands/server/routes/github.py @@ -5,10 +5,10 @@ from openhands.server.shared import openhands_config from openhands.utils.async_utils import call_sync_from_async -app = APIRouter(prefix='/api') +app = APIRouter(prefix='/api/github') -@app.get('/github/repositories') +@app.get('/repositories') async def get_github_repositories( request: Request, page: int = 1, @@ -16,11 +16,6 @@ async def get_github_repositories( sort: str = 'pushed', installation_id: int | None = None, ): - # Extract the GitHub token from the headers - github_token = request.headers.get('X-GitHub-Token') - if not github_token: - raise HTTPException(status_code=400, detail='Missing X-GitHub-Token header') - openhands_config.verify_github_repo_list(installation_id) # Add query parameters @@ -38,10 +33,7 @@ async def get_github_repositories( params['sort'] = sort # Set the authorization header with the GitHub token - headers = { - 'Authorization': f'Bearer {github_token}', - 'Accept': 'application/vnd.github.v3+json', - } + headers = generate_github_headers(request.state.github_token) # Fetch repositories from GitHub try: @@ -64,3 +56,88 @@ async def get_github_repositories( json_response.headers['Link'] = response.headers['Link'] return json_response + + +@app.get('/user') +async def get_github_user(request: Request): + headers = generate_github_headers(request.state.github_token) + try: + response = await call_sync_from_async( + requests.get, 'https://api.github.com/user', headers=headers + ) + response.raise_for_status() + except requests.exceptions.RequestException as e: + raise HTTPException( + status_code=response.status_code if response else 500, + detail=f'Error fetching user: {str(e)}', + ) + + json_response = JSONResponse(content=response.json()) + response.close() + + return json_response + + +@app.get('/installations') +async def get_github_installation_ids(request: Request): + headers = generate_github_headers(request.state.github_token) + try: + response = await call_sync_from_async( + requests.get, 'https://api.github.com/user/installations', headers=headers + ) + response.raise_for_status() + except requests.exceptions.RequestException as e: + raise HTTPException( + status_code=response.status_code if response else 500, + detail=f'Error fetching installations: {str(e)}', + ) + + data = response.json() + ids = [installation['id'] for installation in data['installations']] + json_response = JSONResponse(content=ids) + response.close() + + return json_response + + +@app.get('/search/repositories') +async def search_github_repositories( + request: Request, + query: str, + per_page: int = 5, + sort: str = 'stars', + order: str = 'desc', +): + headers = generate_github_headers(request.state.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)}', + ) + + 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', + }