From 591dd6e82a064db2949ca525d83aa90b1e1b42c9 Mon Sep 17 00:00:00 2001 From: ClaudiaFang Date: Sun, 26 Apr 2026 10:46:44 +0800 Subject: [PATCH] refactor: consolidate duplicated methods into BaseGitService (#17) --- package-lock.json | 40 ++------- src/services/git-service-base.ts | 35 +++++++- src/services/github-service.ts | 31 +------ src/services/gitlab-service.ts | 31 +------ tests/logic/gitignore-manager.test.ts | 28 ++++++ tests/logic/sync-manager-batch.test.ts | 46 +++++++++- tests/services/git-service-base.test.ts | 68 +++++++++++++++ tests/services/github-service.test.ts | 110 +++++++++++++++++++++++- tests/services/gitlab-service.test.ts | 92 ++++++++++++++++++++ 9 files changed, 381 insertions(+), 100 deletions(-) create mode 100644 tests/services/git-service-base.test.ts diff --git a/package-lock.json b/package-lock.json index 302acae..43e6bdf 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,13 +1,13 @@ { - "name": "git-file-push", - "version": "1.0.0", + "name": "git-file-sync", + "version": "1.1.0", "lockfileVersion": 3, "requires": true, "packages": { "": { - "name": "git-file-push", - "version": "1.0.0", - "license": "0-BSD", + "name": "git-file-sync", + "version": "1.1.0", + "license": "MIT", "dependencies": { "ignore": "^7.0.5", "obsidian": "latest" @@ -1556,9 +1556,6 @@ "arm64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -1576,9 +1573,6 @@ "arm64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -1596,9 +1590,6 @@ "ppc64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -1616,9 +1607,6 @@ "s390x" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -1636,9 +1624,6 @@ "x64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -1656,9 +1641,6 @@ "x64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -7320,9 +7302,6 @@ "arm64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MPL-2.0", "optional": true, "os": [ @@ -7344,9 +7323,6 @@ "arm64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MPL-2.0", "optional": true, "os": [ @@ -7368,9 +7344,6 @@ "x64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MPL-2.0", "optional": true, "os": [ @@ -7392,9 +7365,6 @@ "x64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MPL-2.0", "optional": true, "os": [ diff --git a/src/services/git-service-base.ts b/src/services/git-service-base.ts index 72133e7..fbc0546 100644 --- a/src/services/git-service-base.ts +++ b/src/services/git-service-base.ts @@ -88,10 +88,43 @@ export abstract class BaseGitService { return cleanRoot + cleanPath; } + protected encodeContent(content: string): string { + const bytes = new TextEncoder().encode(content); + let binary = ''; + for (let i = 0; i < bytes.byteLength; i++) { + const byte = bytes[i]; + if (byte !== undefined) { + binary += String.fromCodePoint(byte); + } + } + return btoa(binary); + } + + protected decodeContent(base64: string): string { + const binary = atob(base64.replace(/\s/g, '')); + const bytes = new Uint8Array(binary.length); + for (let i = 0; i < binary.length; i++) { + const cp = binary.codePointAt(i); + bytes[i] = cp !== undefined ? cp : 0; + } + return new TextDecoder().decode(bytes); + } + + protected handleFileNotFound(e: unknown): GitFile { + if (e instanceof Error && e.message.includes('404')) { + return { content: '', sha: '' }; + } + throw e; + } + + async getRepoGitignores(branch: string): Promise { + const allFiles = await this.listFiles(branch); + return allFiles.filter(p => p.endsWith('.gitignore')); + } + abstract getFile(path: string, branch: string): Promise; abstract pushFile(path: string, content: string, branch: string, message: string, sha?: string): Promise; abstract listFiles(branch: string): Promise; abstract deleteFile(path: string, branch: string, message: string): Promise; abstract testConnection(): Promise; - abstract getRepoGitignores(branch: string): Promise; } diff --git a/src/services/github-service.ts b/src/services/github-service.ts index ea606f0..c1d613c 100644 --- a/src/services/github-service.ts +++ b/src/services/github-service.ts @@ -32,10 +32,7 @@ export class GitHubService extends BaseGitService implements GitServiceInterface sha: data.sha }; } catch (e) { - if (e instanceof Error && e.message.includes('404')) { - return { content: '', sha: '' }; - } - throw e; + return this.handleFileNotFound(e); } } @@ -86,30 +83,4 @@ export class GitHubService extends BaseGitService implements GitServiceInterface } } - async getRepoGitignores(branch: string): Promise { - const allFiles = await this.listFiles(branch); - return allFiles.filter(p => p.endsWith('.gitignore')); - } - - private encodeContent(content: string): string { - const bytes = new TextEncoder().encode(content); - let binary = ''; - for (let i = 0; i < bytes.byteLength; i++) { - const byte = bytes[i]; - if (byte !== undefined) { - binary += String.fromCodePoint(byte); - } - } - return btoa(binary); - } - - private decodeContent(base64: string): string { - const binary = atob(base64.replace(/\s/g, '')); - const bytes = new Uint8Array(binary.length); - for (let i = 0; i < binary.length; i++) { - const cp = binary.codePointAt(i); - bytes[i] = cp !== undefined ? cp : 0; - } - return new TextDecoder().decode(bytes); - } } diff --git a/src/services/gitlab-service.ts b/src/services/gitlab-service.ts index 9ac1dfd..680feb5 100644 --- a/src/services/gitlab-service.ts +++ b/src/services/gitlab-service.ts @@ -34,10 +34,7 @@ export class GitLabService extends BaseGitService implements GitServiceInterface sha: data.last_commit_id }; } catch (e) { - if (e instanceof Error && e.message.includes('404')) { - return { content: '', sha: '' }; - } - throw e; + return this.handleFileNotFound(e); } } @@ -90,30 +87,4 @@ export class GitLabService extends BaseGitService implements GitServiceInterface } } - async getRepoGitignores(branch: string): Promise { - const allFiles = await this.listFiles(branch); - return allFiles.filter(p => p.endsWith('.gitignore')); - } - - private encodeContent(content: string): string { - const bytes = new TextEncoder().encode(content); - let binary = ''; - for (let i = 0; i < bytes.byteLength; i++) { - const byte = bytes[i]; - if (byte !== undefined) { - binary += String.fromCodePoint(byte); - } - } - return btoa(binary); - } - - private decodeContent(base64: string): string { - const binary = atob(base64.replace(/\s/g, '')); - const bytes = new Uint8Array(binary.length); - for (let i = 0; i < binary.length; i++) { - const cp = binary.codePointAt(i); - bytes[i] = cp !== undefined ? cp : 0; - } - return new TextDecoder().decode(bytes); - } } diff --git a/tests/logic/gitignore-manager.test.ts b/tests/logic/gitignore-manager.test.ts index 3a51fe5..8e320f5 100644 --- a/tests/logic/gitignore-manager.test.ts +++ b/tests/logic/gitignore-manager.test.ts @@ -62,6 +62,34 @@ describe('GitignoreManager', () => { expect(manager.isIgnored('secret.txt')).toBe(true); }); + it('should fall back to [".gitignore"] when getRepoGitignores throws', async () => { + vi.mocked(mockGitService.getRepoGitignores).mockRejectedValue(new Error('Network error')); + const adapter = mockApp.vault.adapter as Mocked; + vi.mocked(adapter.exists).mockResolvedValue(true); + vi.mocked(adapter.read).mockResolvedValue('node_modules/'); + + const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + await manager.loadGitignores(); + + expect(consoleSpy).toHaveBeenCalled(); + expect(manager.isIgnored('node_modules/test.js')).toBe(true); + }); + + it('should fall back to remote when local .gitignore read throws', async () => { + vi.mocked(mockGitService.getRepoGitignores).mockResolvedValue(['.gitignore']); + const adapter = mockApp.vault.adapter as Mocked; + vi.mocked(adapter.exists).mockResolvedValue(true); + vi.mocked(adapter.read).mockRejectedValue(new Error('Permission denied')); + vi.mocked(mockGitService.getFile).mockResolvedValue({ content: 'secret.txt', sha: 'sha' }); + + const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + await manager.loadGitignores(); + + expect(consoleSpy).toHaveBeenCalled(); + expect(mockGitService.getFile).toHaveBeenCalledWith('/.gitignore', branch); + expect(manager.isIgnored('secret.txt')).toBe(true); + }); + it('should handle subdirectory gitignores correctly', async () => { // Repo structure: // .gitignore diff --git a/tests/logic/sync-manager-batch.test.ts b/tests/logic/sync-manager-batch.test.ts index 6ff205f..39a50ec 100644 --- a/tests/logic/sync-manager-batch.test.ts +++ b/tests/logic/sync-manager-batch.test.ts @@ -118,7 +118,7 @@ describe('SyncManager Batch Operations', () => { it('should handle missing remote files during batch pull', async () => { const files = ['exists.md', 'missing.md']; - + vi.mocked(mockGitService.getFile) .mockResolvedValueOnce({ content: 'content', sha: 'sha' }) .mockResolvedValueOnce({ content: '', sha: '' }); @@ -130,4 +130,48 @@ describe('SyncManager Batch Operations', () => { expect(results.errors[0]!.error).toContain('File not found in remote'); }); }); + + describe('onProgress callback', () => { + it('should call onProgress for each file processed', async () => { + const files = ['file1.md', 'file2.md', 'file3.md']; + const adapter = mockApp.vault.adapter as Mocked; + + vi.mocked(adapter.exists).mockResolvedValue(true); + vi.mocked(adapter.read).mockResolvedValue('content'); + vi.mocked(mockGitService.getFile).mockResolvedValue({ content: '', sha: 'sha' }); + vi.mocked(mockGitService.pushFile).mockResolvedValue('path'); + + const onProgress = vi.fn(); + await manager.pushAllFiles(files, onProgress); + + expect(onProgress).toHaveBeenCalledTimes(3); + expect(onProgress).toHaveBeenCalledWith(1, 3, 'file1.md'); + expect(onProgress).toHaveBeenCalledWith(2, 3, 'file2.md'); + expect(onProgress).toHaveBeenCalledWith(3, 3, 'file3.md'); + }); + }); + + describe('batch push with rename detection', () => { + it('should detect and handle rename during batch push', async () => { + const oldPath = 'old.md'; + const newPath = 'new.md'; + const mockFile = Object.assign(new TFile(), { path: newPath, name: 'new.md' }); + mockSettings.syncMetadata = { + [oldPath]: { lastSyncedSha: 'sha', lastSyncedAt: 0, lastKnownPath: oldPath } + }; + + vi.mocked(mockApp.vault.getFileByPath).mockImplementation(p => p === oldPath ? null : mockFile); + vi.mocked(mockApp.vault.read).mockResolvedValue('content'); + vi.mocked(mockApp.vault.adapter.exists as ReturnType).mockResolvedValue(true); + vi.mocked(mockGitService.pushFile).mockResolvedValue(newPath); + vi.mocked(mockGitService.getFile).mockResolvedValue({ content: 'content', sha: 'new-sha' }); + + const results = await manager.pushAllFiles([mockFile]); + + expect(results.success).toBe(1); + expect(mockGitService.pushFile).toHaveBeenCalledWith( + newPath, 'content', 'main', `Rename ${oldPath} to ${newPath}`, undefined + ); + }); + }); }); diff --git a/tests/services/git-service-base.test.ts b/tests/services/git-service-base.test.ts new file mode 100644 index 0000000..8cf6a52 --- /dev/null +++ b/tests/services/git-service-base.test.ts @@ -0,0 +1,68 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { GitHubService } from '../../src/services/github-service'; +import { requestUrl, RequestUrlResponse } from 'obsidian'; + +// Tests for BaseGitService protected methods exercised via GitHubService +describe('BaseGitService', () => { + let service: GitHubService; + + beforeEach(() => { + vi.clearAllMocks(); + service = new GitHubService(); + service.updateConfig('token', 'owner', 'repo'); + }); + + describe('getFullPath with rootPath ending in /', () => { + it('should not double-add slash when rootPath already ends with /', async () => { + service.updateConfig('token', 'owner', 'repo', 'vault/'); + vi.mocked(requestUrl).mockResolvedValue({ + status: 200, + json: { content: btoa('hello'), sha: 'abc' } + } as unknown as RequestUrlResponse); + + await service.getFile('notes/test.md', 'main'); + + const calls = vi.mocked(requestUrl).mock.calls; + const call = calls[0]?.[0] as { url: string }; + // rootPath 'vault/' + 'notes/test.md' should produce 'vault/notes/test.md' + expect(call.url).toContain('vault/notes/test.md'); + expect(call.url).not.toContain('vault//notes/test.md'); + }); + }); + + describe('safeRequest with non-Error exception', () => { + it('should wrap non-Error throws in a new Error', async () => { + // Throw a plain string (not an Error instance) from requestUrl + vi.mocked(requestUrl).mockRejectedValue('plain string error'); + + await expect(service.getFile('test.md', 'main')).rejects.toThrow( + 'Network error or unexpected failure: plain string error' + ); + }); + }); + + describe('encodeContent / decodeContent round-trip', () => { + it('should correctly encode and decode UTF-8 content', async () => { + const original = 'Hello, δΈ–η•Œ! 🌍'; + vi.mocked(requestUrl).mockResolvedValue({ + status: 200, + json: { + content: { path: 'test.md' } + } + } as unknown as RequestUrlResponse); + + // Push encodes content; we verify the encoded body decodes back to original + await service.pushFile('test.md', original, 'main', 'test'); + + const calls = vi.mocked(requestUrl).mock.calls; + const body = JSON.parse((calls[0]?.[0] as { body: string }).body) as { content: string }; + // Decode what was sent and confirm round-trip + const decoded = atob(body.content.replace(/\s/g, '')); + const bytes = new Uint8Array(decoded.length); + for (let i = 0; i < decoded.length; i++) { + bytes[i] = decoded.codePointAt(i) ?? 0; + } + expect(new TextDecoder().decode(bytes)).toBe(original); + }); + }); +}); diff --git a/tests/services/github-service.test.ts b/tests/services/github-service.test.ts index c403630..fcc1e5e 100644 --- a/tests/services/github-service.test.ts +++ b/tests/services/github-service.test.ts @@ -69,9 +69,9 @@ describe('GitHubService', () => { }); it('should update existing file correctly (sha provided)', async () => { - vi.mocked(requestUrl).mockResolvedValue({ - status: 200, - json: { content: { path: 'existing.md' } } + vi.mocked(requestUrl).mockResolvedValue({ + status: 200, + json: { content: { path: 'existing.md' } } } as unknown as RequestUrlResponse); const result = await service.pushFile('existing.md', 'updated content', 'main', 'update', 'old-sha'); @@ -85,4 +85,108 @@ describe('GitHubService', () => { expect(lastCall.body).toContain('"sha":"old-sha"'); }); }); + + describe('listFiles', () => { + it('should list blob files from tree API', async () => { + vi.mocked(requestUrl).mockResolvedValue({ + status: 200, + json: { + tree: [ + { path: 'file1.md', type: 'blob' }, + { path: 'dir/file2.md', type: 'blob' }, + { path: 'subdir', type: 'tree' }, + ] + } + } as unknown as RequestUrlResponse); + + const result = await service.listFiles('main'); + expect(result).toEqual(['file1.md', 'dir/file2.md']); + }); + + it('should filter by rootPath when set', async () => { + service.updateConfig(token, owner, repo, 'vault'); + vi.mocked(requestUrl).mockResolvedValue({ + status: 200, + json: { + tree: [ + { path: 'vault/file1.md', type: 'blob' }, + { path: 'other/file2.md', type: 'blob' }, + ] + } + } as unknown as RequestUrlResponse); + + const result = await service.listFiles('main'); + expect(result).toEqual(['vault/file1.md']); + }); + }); + + describe('deleteFile', () => { + it('should delete file using its sha', async () => { + vi.mocked(requestUrl) + .mockResolvedValueOnce({ + status: 200, + json: { content: btoa('content'), sha: 'file-sha' } + } as unknown as RequestUrlResponse) + .mockResolvedValueOnce({ + status: 200, + json: {} + } as unknown as RequestUrlResponse); + + await service.deleteFile('test.md', 'main', 'delete test.md'); + + const calls = vi.mocked(requestUrl).mock.calls; + expect(calls).toHaveLength(2); + const deleteCall = calls[1]?.[0] as RequestUrlParam; + expect(deleteCall.method).toBe('DELETE'); + expect(deleteCall.body).toContain('"sha":"file-sha"'); + }); + }); + + describe('testConnection', () => { + it('should return true on successful connection', async () => { + vi.mocked(requestUrl).mockResolvedValue({ status: 200, json: {} } as unknown as RequestUrlResponse); + const result = await service.testConnection(); + expect(result).toBe(true); + }); + + it('should return false on failed connection', async () => { + vi.mocked(requestUrl).mockResolvedValue({ + status: 401, + json: { message: 'Unauthorized' }, + text: 'Unauthorized' + } as unknown as RequestUrlResponse); + const result = await service.testConnection(); + expect(result).toBe(false); + }); + }); + + describe('getRepoGitignores', () => { + it('should return only .gitignore paths from file list', async () => { + vi.mocked(requestUrl).mockResolvedValue({ + status: 200, + json: { + tree: [ + { path: '.gitignore', type: 'blob' }, + { path: 'src/main.ts', type: 'blob' }, + { path: 'sub/.gitignore', type: 'blob' }, + ] + } + } as unknown as RequestUrlResponse); + + const result = await service.getRepoGitignores('main'); + expect(result).toEqual(['.gitignore', 'sub/.gitignore']); + }); + }); + + describe('getFile error handling', () => { + it('should rethrow non-404 errors', async () => { + vi.mocked(requestUrl).mockResolvedValue({ + status: 500, + json: { message: 'Internal Server Error' }, + text: 'Internal Server Error' + } as unknown as RequestUrlResponse); + + await expect(service.getFile('test.md', 'main')).rejects.toThrow('500'); + }); + }); }); diff --git a/tests/services/gitlab-service.test.ts b/tests/services/gitlab-service.test.ts index f1901da..38aaa28 100644 --- a/tests/services/gitlab-service.test.ts +++ b/tests/services/gitlab-service.test.ts @@ -88,4 +88,96 @@ describe('GitLabService', () => { expect(lastCall.body).toContain(btoa('updated content')); }); }); + + describe('listFiles', () => { + it('should list blob files from tree API', async () => { + vi.mocked(requestUrl).mockResolvedValue({ + status: 200, + json: [ + { path: 'file1.md', type: 'blob' }, + { path: 'dir/file2.md', type: 'blob' }, + { path: 'subdir', type: 'tree' }, + ] + } as unknown as RequestUrlResponse); + + const result = await service.listFiles('main'); + expect(result).toEqual(['file1.md', 'dir/file2.md']); + }); + + it('should filter by rootPath when set', async () => { + service.updateConfig(baseUrl, token, projectId, 'vault'); + vi.mocked(requestUrl).mockResolvedValue({ + status: 200, + json: [ + { path: 'vault/file1.md', type: 'blob' }, + { path: 'other/file2.md', type: 'blob' }, + ] + } as unknown as RequestUrlResponse); + + const result = await service.listFiles('main'); + expect(result).toEqual(['vault/file1.md']); + }); + }); + + describe('deleteFile', () => { + it('should delete file with commit message', async () => { + vi.mocked(requestUrl).mockResolvedValue({ + status: 200, + json: {} + } as unknown as RequestUrlResponse); + + await service.deleteFile('test.md', 'main', 'delete test.md'); + + const calls = vi.mocked(requestUrl).mock.calls; + const deleteCall = calls[0]?.[0] as RequestUrlParam; + expect(deleteCall.method).toBe('DELETE'); + expect(deleteCall.body).toContain('"commit_message":"delete test.md"'); + }); + }); + + describe('testConnection', () => { + it('should return true on successful connection', async () => { + vi.mocked(requestUrl).mockResolvedValue({ status: 200, json: {} } as unknown as RequestUrlResponse); + const result = await service.testConnection(); + expect(result).toBe(true); + }); + + it('should return false on failed connection', async () => { + vi.mocked(requestUrl).mockResolvedValue({ + status: 401, + json: { message: 'Unauthorized' }, + text: 'Unauthorized' + } as unknown as RequestUrlResponse); + const result = await service.testConnection(); + expect(result).toBe(false); + }); + }); + + describe('getRepoGitignores', () => { + it('should return only .gitignore paths from file list', async () => { + vi.mocked(requestUrl).mockResolvedValue({ + status: 200, + json: [ + { path: '.gitignore', type: 'blob' }, + { path: 'src/main.ts', type: 'blob' }, + { path: 'sub/.gitignore', type: 'blob' }, + ] + } as unknown as RequestUrlResponse); + + const result = await service.getRepoGitignores('main'); + expect(result).toEqual(['.gitignore', 'sub/.gitignore']); + }); + }); + + describe('getFile error handling', () => { + it('should rethrow non-404 errors', async () => { + vi.mocked(requestUrl).mockResolvedValue({ + status: 500, + json: { message: 'Internal Server Error' }, + text: 'Internal Server Error' + } as unknown as RequestUrlResponse); + + await expect(service.getFile('test.md', 'main')).rejects.toThrow('500'); + }); + }); });