diff --git a/package.json b/package.json index bcec031..2513fab 100644 --- a/package.json +++ b/package.json @@ -28,8 +28,10 @@ "devDependencies": { "@jscpd/finder": "^3.5.5", "@octokit/types": "^6.34.0", + "@types/glob": "^7.2.0", "@types/jest": "^26.0.18", "@types/js-yaml": "^4.0.0", + "@types/minimatch": "^3.0.5", "@types/node": "^14.14.11", "@types/rimraf": "^3.0.0", "@typescript-eslint/eslint-plugin": "^4.9.1", diff --git a/src/Parser/SarifParser.ts b/src/Parser/SarifParser.ts index 3e69e34..0224ab7 100644 --- a/src/Parser/SarifParser.ts +++ b/src/Parser/SarifParser.ts @@ -88,23 +88,28 @@ export class SarifParser extends Parser { private processResults(run: SarifRun, lintItems: LintItem[]): void { const results = run.results || []; for (const result of results) { - const lintItem = this.toLintItem(result, run); + const lintItem = this.toLintItem(result); if (lintItem) { lintItems.push(lintItem); } } } - private isValidSarifLog(log: any): log is SarifLog { + private isValidSarifLog(log: unknown): log is SarifLog { + if (log == null || typeof log !== 'object') { + return false; + } + + const obj = log as Record; return ( - log && - typeof log === 'object' && - typeof log.version === 'string' && - Array.isArray(log.runs) + 'version' in obj && + typeof obj.version === 'string' && + 'runs' in obj && + Array.isArray(obj.runs) ); } - private toLintItem(result: SarifResult, run: SarifRun): LintItem | null { + private toLintItem(result: SarifResult): LintItem | null { if (!result.locations?.[0]) { Log.warn('SarifParser Warning: Result has no location information', { result }); return null; @@ -141,8 +146,4 @@ export class SarifParser extends Parser { return mapSeverity(severityMap[level ?? 'warning']); } - - private findRuleDetails(ruleId: string, run: SarifRun): SarifRule | undefined { - return run.tool.driver.rules?.find((rule) => rule.id === ruleId); - } } diff --git a/src/Provider/GitLab/GitLab.spec.ts b/src/Provider/GitLab/GitLab.spec.ts index 5311f4a..bef142c 100644 --- a/src/Provider/GitLab/GitLab.spec.ts +++ b/src/Provider/GitLab/GitLab.spec.ts @@ -14,10 +14,6 @@ import { AnalyzerBot } from '../../AnalyzerBot/AnalyzerBot'; const mockCurrentUserId = 123456; -// Create helper function to generate consistent comment keys -const generateCommentKey = (file: string, line: number | undefined, text: string) => - `${file}:${line}:${text}`; - const mockNotes = [ { id: 1, @@ -163,6 +159,201 @@ describe('VCS: GitLab', () => { // deleteNote and deleteDiscussion will be called for outdated comments }); + describe('Error Handling for Comment Deletion', () => { + it('should handle 404 errors gracefully when deleting notes', async () => { + const service = new MrServiceMock(); + const configsWithRemoveOldComment = { ...configs, removeOldComment: true }; + + // Mock a 404 error for note deletion + const notFoundError = new Error('Not Found'); + (notFoundError as Error & { status: number }).status = 404; + service.deleteNote.mockRejectedValueOnce(notFoundError); + + // Add an existing comment that should be deleted + service.listAllNotes.mockResolvedValue([ + { + id: 999, + author: { id: mockCurrentUserId }, + system: false, + body: 'outdated comment', + }, + ]); + + const gitLab = createGitLab(service, configsWithRemoveOldComment); + + // Should not throw error even when note deletion fails with 404 + await expect(gitLab.report([touchFileWarning])).resolves.toBe(true); + expect(service.deleteNote).toHaveBeenCalled(); + }); + + it('should handle 404 errors gracefully when deleting discussions', async () => { + const service = new MrServiceMock(); + const configsWithRemoveOldComment = { ...configs, removeOldComment: true }; + + // Mock a 404 error for discussion deletion + const notFoundError = new Error('Not Found'); + (notFoundError as Error & { status: number }).status = 404; + service.deleteDiscussion.mockRejectedValueOnce(notFoundError); + + // Add an existing discussion that should be deleted + service.listAllDiscussions.mockResolvedValue([ + { + id: 'discussion-123', + notes: [ + { + id: 456, + author: { id: mockCurrentUserId }, + system: false, + body: 'outdated discussion comment', + }, + ], + }, + ]); + + const gitLab = createGitLab(service, configsWithRemoveOldComment); + + // Should not throw error even when discussion deletion fails with 404 + await expect(gitLab.report([touchFileWarning])).resolves.toBe(true); + expect(service.deleteDiscussion).toHaveBeenCalled(); + }); + + it('should continue processing when some comment deletions fail', async () => { + const service = new MrServiceMock(); + const configsWithRemoveOldComment = { ...configs, removeOldComment: true }; + + // Mock multiple notes, some will fail to delete + service.listAllNotes.mockResolvedValue([ + { + id: 100, + author: { id: mockCurrentUserId }, + system: false, + body: 'comment that will fail', + }, + { + id: 200, + author: { id: mockCurrentUserId }, + system: false, + body: 'comment that will succeed', + }, + ]); + + // First deletion fails, second succeeds + const notFoundError = new Error('Not Found'); + (notFoundError as Error & { status: number }).status = 404; + service.deleteNote + .mockRejectedValueOnce(notFoundError) + .mockResolvedValueOnce(undefined); + + const gitLab = createGitLab(service, configsWithRemoveOldComment); + + // Should complete successfully despite partial failures + await expect(gitLab.report([touchFileWarning])).resolves.toBe(true); + expect(service.deleteNote).toHaveBeenCalledTimes(2); + }); + + it('should handle invalid comment IDs gracefully', async () => { + const service = new MrServiceMock(); + const configsWithRemoveOldComment = { ...configs, removeOldComment: true }; + + // Mock notes with invalid IDs + service.listAllNotes.mockResolvedValue([ + { + id: null, // Invalid ID + author: { id: mockCurrentUserId }, + system: false, + body: 'comment with null id', + }, + { + id: 'invalid-string-id', // Invalid ID type + author: { id: mockCurrentUserId }, + system: false, + body: 'comment with string id', + }, + ]); + + const gitLab = createGitLab(service, configsWithRemoveOldComment); + + // Should handle invalid IDs without attempting deletion + await expect(gitLab.report([touchFileWarning])).resolves.toBe(true); + // deleteNote should not be called for invalid IDs + expect(service.deleteNote).not.toHaveBeenCalled(); + }); + + it('should handle discussion deletion with 404 errors on individual notes', async () => { + const service = new MrServiceMock(); + const configsWithRemoveOldComment = { ...configs, removeOldComment: true }; + + // Mock a discussion with notes + const mockDiscussion = { + id: 'discussion-456', + notes: [ + { id: 10, author: { id: mockCurrentUserId } }, + { id: 20, author: { id: mockCurrentUserId } }, + ], + }; + + service.listAllDiscussions.mockResolvedValue([mockDiscussion]); + + // Mock discussion show to return the discussion + const mockMRService = service as MrServiceMock & { + api: { + MergeRequestDiscussions: { + show: jest.Mock; + }; + MergeRequestNotes: { + remove: jest.Mock; + }; + }; + }; + mockMRService.api = { + MergeRequestDiscussions: { + show: jest.fn().mockResolvedValue(mockDiscussion), + }, + MergeRequestNotes: { + remove: jest + .fn() + .mockRejectedValueOnce(new Error('Not Found')) // First note fails + .mockResolvedValueOnce(undefined), // Second note succeeds + }, + }; + + const gitLab = createGitLab(service, configsWithRemoveOldComment); + + // Should complete successfully despite partial note deletion failures + await expect(gitLab.report([touchFileWarning])).resolves.toBe(true); + }); + + it('should log appropriate warnings for failed deletions', async () => { + const service = new MrServiceMock(); + const configsWithRemoveOldComment = { ...configs, removeOldComment: true }; + + // Spy on console to capture log messages + const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(); + + // Mock a 404 error + const notFoundError = new Error('Not Found'); + (notFoundError as Error & { status: number }).status = 404; + service.deleteNote.mockRejectedValueOnce(notFoundError); + + service.listAllNotes.mockResolvedValue([ + { + id: 123, + author: { id: mockCurrentUserId }, + system: false, + body: 'comment to delete', + }, + ]); + + const gitLab = createGitLab(service, configsWithRemoveOldComment); + await gitLab.report([touchFileWarning]); + + // Should log warning about failed deletion + // Note: The actual log message will depend on the Winston logger configuration + + consoleSpy.mockRestore(); + }); + }); + describe('when failOnWarnings is true', () => { const warningConfigs = { ...configs, failOnWarnings: true }; diff --git a/src/Provider/GitLab/GitLabAdapter.ts b/src/Provider/GitLab/GitLabAdapter.ts index c5cc836..68c15f6 100644 --- a/src/Provider/GitLab/GitLabAdapter.ts +++ b/src/Provider/GitLab/GitLabAdapter.ts @@ -2,7 +2,7 @@ import { VCSAdapter } from '../@interfaces/VCSAdapter'; import { Diff } from '../../Git/@types/PatchTypes'; import { Log } from '../../Logger'; import { IGitLabMRService } from './IGitLabMRService'; -import { MergeRequestDiffVersionsSchema, MergeRequestNoteSchema } from '@gitbeaker/core'; +import { MergeRequestDiffVersionsSchema } from '@gitbeaker/core'; import { IAnalyzerBot } from '../../AnalyzerBot/@interfaces/IAnalyzerBot'; import { Comment } from '../../AnalyzerBot/@types/CommentTypes'; @@ -27,7 +27,7 @@ export class GitLabAdapter implements VCSAdapter { // Store existing bot comments and their IDs notes .filter( - (note: { author: { id: any }; system: any }) => + (note: { author: { id: number }; system: boolean }) => note.author.id === userId && !note.system, ) .forEach((note: { id: number; body: string }) => { @@ -38,21 +38,28 @@ export class GitLabAdapter implements VCSAdapter { // Store existing discussions and their IDs discussions .filter( - (discussion: { notes: any[] }) => + (discussion: { notes?: Array<{ author: { id: number } }> }) => discussion.notes && discussion.notes.some( - (note: { author: { id: any } }) => note.author.id === userId, + (note: { author: { id: number } }) => note.author.id === userId, ), ) - .forEach((discussion: { id: string; notes: any[] }) => { - discussion.notes - .filter((note: { author: { id: any } }) => note.author.id === userId) - .forEach((note: { body: string }) => { - const commentKey = this.generateCommentKey('', 0, note.body); - this.existingComments.add(commentKey); - this.existingDiscussionIds.set(commentKey, discussion.id); - }); - }); + .forEach( + (discussion: { + id: string; + notes?: Array<{ author: { id: number }; body: string }>; + }) => { + if (discussion.notes) { + discussion.notes + .filter((note: { author: { id: number } }) => note.author.id === userId) + .forEach((note: { body: string }) => { + const commentKey = this.generateCommentKey('', 0, note.body); + this.existingComments.add(commentKey); + this.existingDiscussionIds.set(commentKey, discussion.id); + }); + } + }, + ); Log.debug(`Found ${this.existingComments.size} existing CodeCoach comments`); } @@ -71,12 +78,7 @@ export class GitLabAdapter implements VCSAdapter { } } - async createReviewComment( - text: string, - file: string, - line: number, - nLines?: number, - ): Promise { + async createReviewComment(text: string, file: string, line: number): Promise { const commentKey = this.generateCommentKey(file, line, text); if (!this.existingComments.has(commentKey)) { @@ -105,39 +107,111 @@ export class GitLabAdapter implements VCSAdapter { } async removeExistingComments(currentComments: Comment[]): Promise { - // Create a set of current issue keys + const currentIssueKeys = this.createCurrentIssueKeys(currentComments); + const commentsToDelete = this.prepareCommentsForDeletion(currentIssueKeys); + + if (commentsToDelete.length > 0) { + await this.executeCommentDeletions(commentsToDelete); + } else { + Log.debug('No outdated comments to delete'); + } + } + + private createCurrentIssueKeys(currentComments: Comment[]): Set { const currentIssueKeys = new Set(); currentComments.forEach((comment) => { const key = this.generateCommentKey(comment.file, comment.line, comment.text); currentIssueKeys.add(key); }); + return currentIssueKeys; + } - // Delete comments that are no longer relevant + private prepareCommentsForDeletion(currentIssueKeys: Set): Promise[] { const commentsToDelete: Promise[] = []; // Check regular comments this.existingCommentIds.forEach((commentId, commentText) => { if (!currentIssueKeys.has(commentText)) { - commentsToDelete.push(this.mrService.deleteNote(commentId)); - this.existingComments.delete(commentText); - this.existingCommentIds.delete(commentText); + this.addCommentDeletion(commentId, commentText, commentsToDelete); } }); // Check discussion comments this.existingDiscussionIds.forEach((discussionId, commentKey) => { if (!currentIssueKeys.has(commentKey)) { - commentsToDelete.push(this.mrService.deleteDiscussion(discussionId)); - this.existingComments.delete(commentKey); - this.existingDiscussionIds.delete(commentKey); + this.addDiscussionDeletion(discussionId, commentKey, commentsToDelete); } }); - if (commentsToDelete.length > 0) { - await Promise.all(commentsToDelete); - Log.debug(`Deleted ${commentsToDelete.length} outdated comments`); + return commentsToDelete; + } + + private addCommentDeletion( + commentId: number, + commentText: string, + commentsToDelete: Promise[], + ): void { + if (commentId && typeof commentId === 'number') { + commentsToDelete.push(this.mrService.deleteNote(commentId)); } else { - Log.debug('No outdated comments to delete'); + Log.warn(`Invalid comment ID found: ${commentId}, skipping deletion`); + } + this.existingComments.delete(commentText); + this.existingCommentIds.delete(commentText); + } + + private addDiscussionDeletion( + discussionId: string, + commentKey: string, + commentsToDelete: Promise[], + ): void { + if (discussionId && typeof discussionId === 'string') { + commentsToDelete.push(this.mrService.deleteDiscussion(discussionId)); + } else { + Log.warn(`Invalid discussion ID found: ${discussionId}, skipping deletion`); + } + this.existingComments.delete(commentKey); + this.existingDiscussionIds.delete(commentKey); + } + + private async executeCommentDeletions( + commentsToDelete: Promise[], + ): Promise { + const results = await this.performDeletions(commentsToDelete); + this.logDeletionResults(results, commentsToDelete.length); + } + + private async performDeletions( + commentsToDelete: Promise[], + ): Promise<{ succeeded: number; failed: number }> { + let succeeded = 0; + let failed = 0; + + for (const deletePromise of commentsToDelete) { + try { + await deletePromise; + succeeded++; + } catch (error: unknown) { + failed++; + Log.debug('Failed to delete individual comment', { + error: error instanceof Error ? error.message : String(error), + }); + } + } + + return { succeeded, failed }; + } + + private logDeletionResults( + results: { succeeded: number; failed: number }, + totalComments: number, + ): void { + if (results.failed > 0) { + Log.warn( + `Deleted ${results.succeeded}/${totalComments} outdated comments, ${results.failed} failed`, + ); + } else { + Log.debug(`Deleted ${results.succeeded} outdated comments`); } } } diff --git a/src/Provider/GitLab/GitLabMRService.ts b/src/Provider/GitLab/GitLabMRService.ts index 82ec48e..8aec746 100644 --- a/src/Provider/GitLab/GitLabMRService.ts +++ b/src/Provider/GitLab/GitLabMRService.ts @@ -59,29 +59,99 @@ export class GitLabMRService implements IGitLabMRService { } async deleteNote(noteId: number): Promise { - await this.api.MergeRequestNotes.remove(this.projectId, this.mrIid, noteId); + await this.deleteNoteWithErrorHandling(noteId); } - async listAllDiscussions(): Promise { + private async deleteNoteWithErrorHandling(noteId: number): Promise { + try { + await this.api.MergeRequestNotes.remove(this.projectId, this.mrIid, noteId); + } catch (error: unknown) { + const errorObj = error as { message?: string; status?: number }; + if (errorObj.message === 'Not Found' || errorObj.status === 404) { + Log.warn(`Note ${noteId} not found, skipping deletion`); + return; + } + Log.error(`Failed to delete note ${noteId}`, { error: errorObj.message }); + throw error; + } + } + + async listAllDiscussions(): Promise< + Array<{ + id: string; + notes?: Array<{ id: number; author: { id: number }; body: string }>; + }> + > { return await this.api.MergeRequestDiscussions.all(this.projectId, this.mrIid); } async deleteDiscussion(discussionId: string): Promise { - // GitLab discussions are deleted by removing all notes in the discussion - // We need to get the discussion and remove its notes - const discussion = await this.api.MergeRequestDiscussions.show( + try { + const discussion = await this.getDiscussion(discussionId); + await this.deleteDiscussionNotes(discussion, discussionId); + } catch (error: unknown) { + this.handleDiscussionDeletionError(error, discussionId); + } + } + + private async getDiscussion( + discussionId: string, + ): Promise<{ notes?: Array<{ id: number }> }> { + return await this.api.MergeRequestDiscussions.show( this.projectId, this.mrIid, discussionId, ); - if (discussion.notes && discussion.notes.length > 0) { - const deletePromises = discussion.notes.map((note: any) => - this.api.MergeRequestNotes.remove(this.projectId, this.mrIid, note.id), + } + + private async deleteDiscussionNotes( + discussion: { notes?: Array<{ id: number }> }, + discussionId: string, + ): Promise { + if (!discussion.notes || discussion.notes.length === 0) { + return; + } + + const deletePromises = discussion.notes.map(async (note: { id: number }) => { + try { + await this.deleteNoteWithErrorHandling(note.id); + return { success: true }; + } catch (error) { + return { success: false }; + } + }); + + const results = await Promise.all(deletePromises); + this.logDeletionResults(results, discussion.notes.length, discussionId); + } + + private logDeletionResults( + results: Array<{ success: boolean }>, + totalNotes: number, + discussionId: string, + ): void { + const failed = results.filter((r) => !r.success).length; + if (failed > 0) { + Log.warn( + `Failed to delete ${failed}/${totalNotes} notes in discussion ${discussionId}`, ); - await Promise.all(deletePromises); } } + private handleDiscussionDeletionError(error: unknown, discussionId: string): void { + const errorObj = error as { message?: string; status?: number }; + + if (errorObj.message === 'Not Found' || errorObj.status === 404) { + Log.warn(`Discussion ${discussionId} not found, skipping deletion`); + return; + } + + Log.error(`Failed to delete discussion ${discussionId}`, { + error: errorObj.message || String(error), + }); + throw error; + } + // github can do someone fancy shit here we cant async createNote(note: string): Promise { await this.api.MergeRequestNotes.create(this.projectId, this.mrIid, note); @@ -93,7 +163,7 @@ export class GitLabMRService implements IGitLabMRService { if (!changes) { return []; } else { - return changes.map((d) => ({ + return changes.map((d: { new_path: string; diff: string }) => ({ file: d.new_path, patch: getPatch(d.diff), })); @@ -105,7 +175,7 @@ export class GitLabMRService implements IGitLabMRService { this.projectId, this.mrIid, ); - const collected = versions.filter((v) => v.state === 'collected'); + const collected = versions.filter((v: { state: string }) => v.state === 'collected'); if (collected.length === 0) { Log.warn( diff --git a/src/Provider/GitLab/IGitLabMRService.ts b/src/Provider/GitLab/IGitLabMRService.ts index ee4d1f7..b72149d 100644 --- a/src/Provider/GitLab/IGitLabMRService.ts +++ b/src/Provider/GitLab/IGitLabMRService.ts @@ -5,7 +5,12 @@ export interface IGitLabMRService { createNote(note: string): Promise; listAllNotes(): Promise; deleteNote(noteId: number): Promise; - listAllDiscussions(): Promise; + listAllDiscussions(): Promise< + Array<{ + id: string; + notes?: Array<{ id: number; author: { id: number }; body: string }>; + }> + >; deleteDiscussion(discussionId: string): Promise; getCurrentUserId(): Promise; diff(): Promise; diff --git a/yarn.lock b/yarn.lock index b9f854d..a461740 100644 --- a/yarn.lock +++ b/yarn.lock @@ -822,6 +822,14 @@ "@types/minimatch" "*" "@types/node" "*" +"@types/glob@^7.2.0": + version "7.2.0" + resolved "https://registry.yarnpkg.com/@types/glob/-/glob-7.2.0.tgz#bc1b5bf3aa92f25bd5dd39f35c57361bdce5b2eb" + integrity sha512-ZUxbzKl0IfJILTS6t7ip5fQQM/J3TJYubDm3nMbgubNNYS62eXeUpoLUC8/7fJNiFYHTrGPQn7hspDUzIHX3UA== + dependencies: + "@types/minimatch" "*" + "@types/node" "*" + "@types/graceful-fs@^4.1.2": version "4.1.3" resolved "https://registry.yarnpkg.com/@types/graceful-fs/-/graceful-fs-4.1.3.tgz#039af35fe26bec35003e8d86d2ee9c586354348f" @@ -871,6 +879,11 @@ resolved "https://registry.yarnpkg.com/@types/minimatch/-/minimatch-3.0.3.tgz#3dca0e3f33b200fc7d1139c0cd96c1268cadfd9d" integrity sha512-tHq6qdbT9U1IRSGf14CL0pUlULksvY9OZ+5eEgl1N7t+OA3tGvNpxJCzuKQlsNgCVwbAs670L1vcVQi8j9HjnA== +"@types/minimatch@^3.0.5": + version "3.0.5" + resolved "https://registry.yarnpkg.com/@types/minimatch/-/minimatch-3.0.5.tgz#1001cc5e6a3704b83c236027e77f2f58ea010f40" + integrity sha512-Klz949h02Gz2uZCMGwDUSDS1YBlTdDDgbWHi+81l29tQALUtvz4rAYi5uoVhE5Lagoq6DeqAUlbrHvW/mXDgdQ== + "@types/node@*": version "14.6.0" resolved "https://registry.yarnpkg.com/@types/node/-/node-14.6.0.tgz#7d4411bf5157339337d7cff864d9ff45f177b499"