diff --git a/src/services/git-service-base.ts b/src/services/git-service-base.ts index b33e57c..9c1368b 100644 --- a/src/services/git-service-base.ts +++ b/src/services/git-service-base.ts @@ -281,16 +281,25 @@ export abstract class BaseGitService { } /** - * Resolves a branch to its latest commit sha and that commit's base tree - * sha, via GitHub's `git/ref/heads/{branch}` endpoint. Gitea's older - * versions require a different branch-resolution endpoint, so it provides - * its own override rather than using this helper. + * Resolves a branch to its latest commit sha via GitHub's + * `git/ref/heads/{branch}` endpoint. Gitea's older versions require a + * different branch-resolution endpoint, so it provides its own override + * rather than using this helper. */ - protected async resolveGitHubStyleBaseTree(branch: string): Promise<{ latestCommitSha: string; baseTreeSha: string }> { + protected async getLatestCommitSha(branch: string): Promise { const base = this.getGitDataApiBase(); const refResp = await this.safeRequest(`${base}/git/ref/heads/${branch}`, 'GET'); - const latestCommitSha = this.parseJson<{ object: { sha: string } }>(refResp).object.sha; + return this.parseJson<{ object: { sha: string } }>(refResp).object.sha; + } + /** + * Resolves a branch to its latest commit sha and that commit's base tree + * sha. Only needed by the REST Git Data API flow (pushSymlink, and + * Gitea's pushBatch/deleteBatch which lack a GraphQL alternative). + */ + protected async resolveGitHubStyleBaseTree(branch: string): Promise<{ latestCommitSha: string; baseTreeSha: string }> { + const latestCommitSha = await this.getLatestCommitSha(branch); + const base = this.getGitDataApiBase(); const commitResp = await this.safeRequest(`${base}/git/commits/${latestCommitSha}`, 'GET'); const baseTreeSha = this.parseJson<{ tree: { sha: string } }>(commitResp).tree.sha; diff --git a/src/services/github-service.ts b/src/services/github-service.ts index 53d4ae5..0c79fc9 100644 --- a/src/services/github-service.ts +++ b/src/services/github-service.ts @@ -1,7 +1,22 @@ import { GitServiceInterface, GitTreeEntry, BatchPushItem, BatchPushResult } from './git-service-interface'; -import { BaseGitService, ConnectionTestResult, GitFile, GitHubContentResponse, GitHubTreeResponse, GIT_SYMLINK_MODE, BLOB_CREATE_CONCURRENCY } from './git-service-base'; +import { BaseGitService, ConnectionTestResult, GitFile, GitHubContentResponse, GitHubTreeResponse, GIT_SYMLINK_MODE } from './git-service-base'; import { logger } from '../utils/logger'; +/** + * Commits any mix of file additions/deletions in one request. Used instead of + * the REST Git Data API's blob -> tree -> commit -> ref sequence for + * pushBatch/deleteBatch: those need one HTTP round trip per blob to upload + * content, while this mutation carries file content directly in the request + * body, so an N-file batch is one call instead of N+~5. + */ +const CREATE_COMMIT_MUTATION = ` + mutation ($input: CreateCommitOnBranchInput!) { + createCommitOnBranch(input: $input) { + commit { oid } + } + } +`; + export class GitHubService extends BaseGitService implements GitServiceInterface { private owner: string = ''; private repo: string = ''; @@ -87,29 +102,48 @@ export class GitHubService extends BaseGitService implements GitServiceInterface return { path: fullPath, sha: blobSha }; } + /** + * Calls a GitHub GraphQL mutation. GraphQL reports mutation-level failures + * (e.g. a stale expectedHeadOid) as a 200 response with an `errors` array + * rather than an HTTP error status, so this checks for that on top of + * safeRequest's status-code check. + */ + private async githubGraphQL(query: string, variables: Record): Promise { + const response = await this.safeRequest('https://api.github.com/graphql', 'POST', { query, variables }); + const body = this.parseJson<{ data?: T; errors?: Array<{ message: string }> }>(response); + if (body.errors && body.errors.length > 0) { + throw new Error(`GitHub GraphQL error: ${body.errors.map(e => e.message).join('; ')}`); + } + if (!body.data) { + throw new Error('GitHub GraphQL response returned no data'); + } + return body.data; + } + async pushBatch(items: BatchPushItem[], branch: string, message: string): Promise { if (items.length === 0) return []; - const base = this.getGitDataApiBase(); - const { latestCommitSha, baseTreeSha } = await this.resolveGitHubStyleBaseTree(branch); + const expectedHeadOid = await this.getLatestCommitSha(branch); - const blobShas = await this.mapWithConcurrency(items, BLOB_CREATE_CONCURRENCY, async item => { - const blobResp = await this.safeRequest(`${base}/git/blobs`, 'POST', { - content: this.encodeContent(item.content), - encoding: 'base64', - }); - return this.parseJson<{ sha: string }>(blobResp).sha; + await this.githubGraphQL(CREATE_COMMIT_MUTATION, { + input: { + branch: { repositoryNameWithOwner: `${this.owner}/${this.repo}`, branchName: branch }, + message: { headline: message }, + expectedHeadOid, + fileChanges: { + additions: items.map(item => ({ + path: this.getFullPath(item.path), + contents: this.encodeContent(item.content), + })), + }, + }, }); - const treeItems = items.map((item, i) => ({ - path: this.getFullPath(item.path), - mode: '100644', - type: 'blob' as const, - sha: blobShas[i] as string, - })); - - await this.commitGitHubStyleTree(base, branch, baseTreeSha, latestCommitSha, treeItems, message); - - return items.map((item, i) => ({ path: item.path, sha: blobShas[i] })); + // createCommitOnBranch only returns the new commit's oid, not each + // file's blob sha, so read them back with one follow-up tree fetch + // (mirrors GitLab's pushBatch, which has the same limitation). + const freshTree = await this.listFilesDetailed(branch, false); + const shaByPath = new Map(freshTree.map(e => [e.path, e.sha])); + return items.map(item => ({ path: item.path, sha: shaByPath.get(this.getFullPath(item.path)) })); } async listFilesDetailed(branch: string, useFilter = true): Promise { @@ -160,17 +194,18 @@ export class GitHubService extends BaseGitService implements GitServiceInterface async deleteBatch(paths: string[], branch: string, message: string): Promise { if (paths.length === 0) return; - const base = this.getGitDataApiBase(); - const { latestCommitSha, baseTreeSha } = await this.resolveGitHubStyleBaseTree(branch); + const expectedHeadOid = await this.getLatestCommitSha(branch); - const treeItems = paths.map(path => ({ - path: this.getFullPath(path), - mode: '100644', - type: 'blob' as const, - sha: null, - })); - - await this.commitGitHubStyleTree(base, branch, baseTreeSha, latestCommitSha, treeItems, message); + await this.githubGraphQL(CREATE_COMMIT_MUTATION, { + input: { + branch: { repositoryNameWithOwner: `${this.owner}/${this.repo}`, branchName: branch }, + message: { headline: message }, + expectedHeadOid, + fileChanges: { + deletions: paths.map(path => ({ path: this.getFullPath(path) })), + }, + }, + }); } async testConnection(branch: string): Promise { diff --git a/tests/services/github-service.test.ts b/tests/services/github-service.test.ts index 2757d07..d800e8a 100644 --- a/tests/services/github-service.test.ts +++ b/tests/services/github-service.test.ts @@ -105,15 +105,11 @@ describe('GitHubService', () => { expect(requestUrl).not.toHaveBeenCalled(); }); - it('commits N files in one commit via ref -> commit -> blobs -> tree -> commit -> ref', async () => { + it('commits N files in one GraphQL mutation via ref -> createCommitOnBranch -> tree', async () => { vi.mocked(requestUrl) .mockResolvedValueOnce({ status: 200, json: { object: { sha: 'commit1' } } } as unknown as RequestUrlResponse) // get ref - .mockResolvedValueOnce({ status: 200, json: { tree: { sha: 'tree1' } } } as unknown as RequestUrlResponse) // get commit - .mockResolvedValueOnce({ status: 201, json: { sha: 'blob-a' } } as unknown as RequestUrlResponse) // blob a - .mockResolvedValueOnce({ status: 201, json: { sha: 'blob-b' } } as unknown as RequestUrlResponse) // blob b - .mockResolvedValueOnce({ status: 201, json: { sha: 'tree2' } } as unknown as RequestUrlResponse) // create tree - .mockResolvedValueOnce({ status: 201, json: { sha: 'commit2' } } as unknown as RequestUrlResponse) // create commit - .mockResolvedValueOnce({ status: 200, json: {} } as unknown as RequestUrlResponse); // update ref + .mockResolvedValueOnce({ status: 200, json: { data: { createCommitOnBranch: { commit: { oid: 'commit2' } } } } } as unknown as RequestUrlResponse) // mutation + .mockResolvedValueOnce({ status: 200, json: { tree: [{ path: 'a.md', type: 'blob', sha: 'blob-a' }, { path: 'b.md', type: 'blob', sha: 'blob-b' }], truncated: false } } as unknown as RequestUrlResponse); // fresh tree const result = await service.pushBatch( [{ path: 'a.md', content: 'hello' }, { path: 'b.md', content: 'world' }], @@ -124,69 +120,37 @@ describe('GitHubService', () => { expect(result).toEqual([{ path: 'a.md', sha: 'blob-a' }, { path: 'b.md', sha: 'blob-b' }]); const calls = vi.mocked(requestUrl).mock.calls.map(c => c[0] as RequestUrlParam); - expect(calls).toHaveLength(7); + expect(calls).toHaveLength(3); + expect(calls[1]?.url).toBe('https://api.github.com/graphql'); + expect(calls[1]?.method).toBe('POST'); - // blobs are base64-encoded (not pushSymlink's raw utf-8 path), so binary content works too. - const blobABody = JSON.parse(calls[2]?.body as string) as { content: string; encoding: string }; - expect(blobABody.encoding).toBe('base64'); - expect(atob(blobABody.content)).toBe('hello'); - - const treeBody = JSON.parse(calls[4]?.body as string) as { base_tree: string; tree: Array<{ path: string; mode: string; type: string; sha: string }> }; - expect(treeBody.base_tree).toBe('tree1'); - expect(treeBody.tree).toEqual([ - { path: 'a.md', mode: '100644', type: 'blob', sha: 'blob-a' }, - { path: 'b.md', mode: '100644', type: 'blob', sha: 'blob-b' }, + const mutationBody = JSON.parse(calls[1]?.body as string) as { + variables: { input: { branch: { repositoryNameWithOwner: string; branchName: string }; message: { headline: string }; expectedHeadOid: string; fileChanges: { additions: Array<{ path: string; contents: string }> } } }; + }; + const input = mutationBody.variables.input; + expect(input.branch).toEqual({ repositoryNameWithOwner: `${owner}/${repo}`, branchName: 'main' }); + expect(input.message).toEqual({ headline: 'Push 2 file(s) from Obsidian' }); + expect(input.expectedHeadOid).toBe('commit1'); + // contents are base64-encoded (not pushSymlink's raw utf-8 path), so binary content works too. + expect(atob(input.fileChanges.additions[0]!.contents)).toBe('hello'); + expect(input.fileChanges.additions).toEqual([ + { path: 'a.md', contents: btoa('hello') }, + { path: 'b.md', contents: btoa('world') }, ]); - - const commitBody = JSON.parse(calls[5]?.body as string) as { message: string; tree: string; parents: string[] }; - expect(commitBody).toEqual({ message: 'Push 2 file(s) from Obsidian', tree: 'tree2', parents: ['commit1'] }); - - expect(calls[6]?.method).toBe('PATCH'); - expect(JSON.parse(calls[6]?.body as string)).toEqual({ sha: 'commit2' }); }); - it('creates blobs concurrently, not one at a time (regression guard against a sequential loop)', async () => { - // Deferred blob responses: none of them resolve until every blob - // POST has already been dispatched. A sequential `for` loop would - // deadlock here since call N+1 never fires until call N resolves. - const blobDeferreds = Array.from({ length: 4 }, () => { - let resolve!: (v: RequestUrlResponse) => void; - const promise = new Promise(r => { resolve = r; }); - return { promise, resolve }; - }); - let blobCallCount = 0; + it('throws when the GraphQL response reports errors on an HTTP 200', async () => { + // GraphQL reports mutation failures (e.g. a stale expectedHeadOid) as a + // 200 response with an `errors` array, not an HTTP error status. + vi.mocked(requestUrl) + .mockResolvedValueOnce({ status: 200, json: { object: { sha: 'commit1' } } } as unknown as RequestUrlResponse) // get ref + .mockResolvedValueOnce({ status: 200, json: { errors: [{ message: 'Head sha was modified' }] } } as unknown as RequestUrlResponse); // mutation failure - vi.mocked(requestUrl).mockImplementation(((param: string | RequestUrlParam) => { - const url = (param as RequestUrlParam).url; - if (url.endsWith('/git/ref/heads/main')) { - return Promise.resolve({ status: 200, json: { object: { sha: 'commit1' } } } as unknown as RequestUrlResponse); - } - if (url.includes('/git/commits/commit1')) { - return Promise.resolve({ status: 200, json: { tree: { sha: 'tree1' } } } as unknown as RequestUrlResponse); - } - if (url.endsWith('/git/blobs')) { - return blobDeferreds[blobCallCount++]!.promise; - } - if (url.endsWith('/git/trees')) { - return Promise.resolve({ status: 201, json: { sha: 'tree2' } } as unknown as RequestUrlResponse); - } - if (url.endsWith('/git/commits')) { - return Promise.resolve({ status: 201, json: { sha: 'commit2' } } as unknown as RequestUrlResponse); - } - return Promise.resolve({ status: 200, json: {} } as unknown as RequestUrlResponse); - }) as unknown as typeof requestUrl); - - const items = Array.from({ length: 4 }, (_, i) => ({ path: `f${i}.md`, content: `content${i}` })); - const resultPromise = service.pushBatch(items, 'main', 'push'); - - // Wait for every blob POST to have been dispatched — a sequential - // loop would never get past the first one, since its promise never resolves. - await vi.waitFor(() => expect(blobCallCount).toBe(4)); - - blobDeferreds.forEach((d, i) => d.resolve({ status: 201, json: { sha: `blob-${i}` } } as unknown as RequestUrlResponse)); - const result = await resultPromise; - - expect(result).toEqual(items.map((item, i) => ({ path: item.path, sha: `blob-${i}` }))); + await expect(service.pushBatch( + [{ path: 'a.md', content: 'hello' }], + 'main', + 'Push 1 file(s) from Obsidian' + )).rejects.toThrow('Head sha was modified'); }); }); @@ -362,31 +326,33 @@ describe('GitHubService', () => { expect(requestUrl).not.toHaveBeenCalled(); }); - it('deletes N files in one commit via ref -> commit -> tree(sha:null) -> commit -> ref', async () => { + it('deletes N files in one GraphQL mutation via ref -> createCommitOnBranch', async () => { vi.mocked(requestUrl) .mockResolvedValueOnce({ status: 200, json: { object: { sha: 'commit1' } } } as unknown as RequestUrlResponse) // get ref - .mockResolvedValueOnce({ status: 200, json: { tree: { sha: 'tree1' } } } as unknown as RequestUrlResponse) // get commit - .mockResolvedValueOnce({ status: 201, json: { sha: 'tree2' } } as unknown as RequestUrlResponse) // create tree - .mockResolvedValueOnce({ status: 201, json: { sha: 'commit2' } } as unknown as RequestUrlResponse) // create commit - .mockResolvedValueOnce({ status: 200, json: {} } as unknown as RequestUrlResponse); // update ref + .mockResolvedValueOnce({ status: 200, json: { data: { createCommitOnBranch: { commit: { oid: 'commit2' } } } } } as unknown as RequestUrlResponse); // mutation await service.deleteBatch(['a.md', 'b.md'], 'main', 'Delete 2 file(s) from Obsidian'); const calls = vi.mocked(requestUrl).mock.calls.map(c => c[0] as RequestUrlParam); - expect(calls).toHaveLength(5); + expect(calls).toHaveLength(2); + expect(calls[1]?.url).toBe('https://api.github.com/graphql'); - const treeBody = JSON.parse(calls[2]?.body as string) as { base_tree: string; tree: Array<{ path: string; mode: string; type: string; sha: string | null }> }; - expect(treeBody.base_tree).toBe('tree1'); - expect(treeBody.tree).toEqual([ - { path: 'a.md', mode: '100644', type: 'blob', sha: null }, - { path: 'b.md', mode: '100644', type: 'blob', sha: null }, - ]); + const mutationBody = JSON.parse(calls[1]?.body as string) as { + variables: { input: { expectedHeadOid: string; message: { headline: string }; fileChanges: { deletions: Array<{ path: string }> } } }; + }; + const input = mutationBody.variables.input; + expect(input.expectedHeadOid).toBe('commit1'); + expect(input.message).toEqual({ headline: 'Delete 2 file(s) from Obsidian' }); + expect(input.fileChanges.deletions).toEqual([{ path: 'a.md' }, { path: 'b.md' }]); + }); - const commitBody = JSON.parse(calls[3]?.body as string) as { message: string; tree: string; parents: string[] }; - expect(commitBody).toEqual({ message: 'Delete 2 file(s) from Obsidian', tree: 'tree2', parents: ['commit1'] }); + it('throws when the GraphQL response reports errors on an HTTP 200', async () => { + vi.mocked(requestUrl) + .mockResolvedValueOnce({ status: 200, json: { object: { sha: 'commit1' } } } as unknown as RequestUrlResponse) // get ref + .mockResolvedValueOnce({ status: 200, json: { errors: [{ message: 'Head sha was modified' }] } } as unknown as RequestUrlResponse); // mutation failure - expect(calls[4]?.method).toBe('PATCH'); - expect(JSON.parse(calls[4]?.body as string)).toEqual({ sha: 'commit2' }); + await expect(service.deleteBatch(['a.md'], 'main', 'Delete 1 file(s) from Obsidian')) + .rejects.toThrow('Head sha was modified'); }); });