diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 8c72971682dd1cecda1a12b3143aaf6b0c585919..5b41005170588746fe7f9a722b24a4fbc28dc1a0 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -191,7 +191,13 @@ export const fetchDiffFilesMeta = ({ commit, state }) => { commit(types.SET_MERGE_REQUEST_DIFFS, data.merge_request_diffs || []); commit(types.SET_DIFF_METADATA, strippedData); - worker.postMessage(prepareDiffData(data, state.diffFiles)); + worker.postMessage( + prepareDiffData({ + diff: data, + priorFiles: state.diffFiles, + meta: true, + }), + ); return data; }) diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index 90940d8222637f3dc477d40952f161fd9abb67c3..19122c3096f0c3fbb968dd82269a1e0498f2cae4 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/app/assets/javascripts/diffs/store/mutations.js @@ -73,7 +73,10 @@ export default { }, [types.SET_DIFF_DATA_BATCH](state, data) { - const files = prepareDiffData(data, state.diffFiles); + const files = prepareDiffData({ + diff: data, + priorFiles: state.diffFiles, + }); Object.assign(state, { ...convertObjectPropsToCamelCase(data), @@ -143,7 +146,7 @@ export default { }, [types.ADD_COLLAPSED_DIFFS](state, { file, data }) { - const files = prepareDiffData(data); + const files = prepareDiffData({ diff: data }); const [newFileData] = files.filter(f => f.file_hash === file.file_hash); const selectedFile = state.diffFiles.find(f => f.file_hash === file.file_hash); Object.assign(selectedFile, { ...newFileData }); diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index 8949c8cd23e31389d7fcfe432f600a4dd3d44dd2..1839df12c967a08ae35ad5a4eeea3c75b9d22643 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -386,9 +386,9 @@ function deduplicateFilesList(files) { return Object.values(dedupedFiles); } -export function prepareDiffData(diff, priorFiles = []) { +export function prepareDiffData({ diff, priorFiles = [], meta = false }) { const cleanedFiles = (diff.diff_files || []) - .map((file, index, allFiles) => prepareRawDiffFile({ file, allFiles })) + .map((file, index, allFiles) => prepareRawDiffFile({ file, allFiles, meta })) .map(ensureBasicDiffFileLines) .map(prepareDiffFileLines) .map(finalizeDiffFile); diff --git a/app/assets/javascripts/diffs/utils/diff_file.js b/app/assets/javascripts/diffs/utils/diff_file.js index aa801783c57476a08e9cb87568d945fc889b855a..67f117bc4d342c483cf7d60e1bcd33d7ab6af72f 100644 --- a/app/assets/javascripts/diffs/utils/diff_file.js +++ b/app/assets/javascripts/diffs/utils/diff_file.js @@ -4,6 +4,7 @@ import { DIFF_FILE_MANUAL_COLLAPSE, DIFF_FILE_AUTOMATIC_COLLAPSE, } from '../constants'; +import { uuids } from './uuids'; function fileSymlinkInformation(file, fileList) { const duplicates = fileList.filter(iteratedFile => iteratedFile.file_hash === file.file_hash); @@ -32,16 +33,29 @@ function collapsed(file) { }; } -export function prepareRawDiffFile({ file, allFiles }) { - Object.assign(file, { +function identifier(file) { + return uuids({ + seeds: [file.file_identifier_hash, file.content_sha], + })[0]; +} + +export function prepareRawDiffFile({ file, allFiles, meta = false }) { + const additionalProperties = { brokenSymlink: fileSymlinkInformation(file, allFiles), viewer: { ...file.viewer, ...collapsed(file), }, - }); + }; + + // It's possible, but not confirmed, that `content_sha` isn't available sometimes + // See: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/49506#note_464692057 + // We don't want duplicate IDs if that's the case, so we just don't assign an ID + if (!meta && file.content_sha) { + additionalProperties.id = identifier(file); + } - return file; + return Object.assign(file, additionalProperties); } export function collapsedType(file) { diff --git a/spec/frontend/diffs/store/utils_spec.js b/spec/frontend/diffs/store/utils_spec.js index a78c4c2d06532771e01ec360e722290ed0d4d911..7ee97224707c0c7af7093f4af6921fa13fdc5a54 100644 --- a/spec/frontend/diffs/store/utils_spec.js +++ b/spec/frontend/diffs/store/utils_spec.js @@ -409,10 +409,13 @@ describe('DiffsStoreUtils', () => { diff_files: [{ ...mock, [INLINE_DIFF_LINES_KEY]: undefined }], }; - preparedDiff.diff_files = utils.prepareDiffData(preparedDiff); - splitInlineDiff.diff_files = utils.prepareDiffData(splitInlineDiff); - splitParallelDiff.diff_files = utils.prepareDiffData(splitParallelDiff); - completedDiff.diff_files = utils.prepareDiffData(completedDiff, [mock]); + preparedDiff.diff_files = utils.prepareDiffData({ diff: preparedDiff }); + splitInlineDiff.diff_files = utils.prepareDiffData({ diff: splitInlineDiff }); + splitParallelDiff.diff_files = utils.prepareDiffData({ diff: splitParallelDiff }); + completedDiff.diff_files = utils.prepareDiffData({ + diff: completedDiff, + priorFiles: [mock], + }); }); it('sets the renderIt and collapsed attribute on files', () => { @@ -447,7 +450,10 @@ describe('DiffsStoreUtils', () => { content_sha: 'ABC', file_hash: 'DEF', }; - const updatedFilesList = utils.prepareDiffData({ diff_files: [fakeNewFile] }, priorFiles); + const updatedFilesList = utils.prepareDiffData({ + diff: { diff_files: [fakeNewFile] }, + priorFiles, + }); expect(updatedFilesList).toEqual([mock, fakeNewFile]); }); @@ -460,7 +466,10 @@ describe('DiffsStoreUtils', () => { { ...mock, [INLINE_DIFF_LINES_KEY]: undefined }, { ...mock, [INLINE_DIFF_LINES_KEY]: undefined, content_sha: 'ABC', file_hash: 'DEF' }, ]; - const updatedFilesList = utils.prepareDiffData({ diff_files: fakeBatch }, priorFiles); + const updatedFilesList = utils.prepareDiffData({ + diff: { diff_files: fakeBatch }, + priorFiles, + }); expect(updatedFilesList).toEqual([ mock, @@ -498,7 +507,7 @@ describe('DiffsStoreUtils', () => { beforeEach(() => { mock = getDiffMetadataMock(); - preparedDiffFiles = utils.prepareDiffData(mock); + preparedDiffFiles = utils.prepareDiffData({ diff: mock, meta: true }); }); it('sets the renderIt and collapsed attribute on files', () => { @@ -514,7 +523,7 @@ describe('DiffsStoreUtils', () => { const fileMock = getDiffFileMock(); const metaData = getDiffMetadataMock(); const priorFiles = [fileMock]; - const updatedFilesList = utils.prepareDiffData(metaData, priorFiles); + const updatedFilesList = utils.prepareDiffData({ diff: metaData, priorFiles, meta: true }); expect(updatedFilesList.length).toEqual(2); expect(updatedFilesList[0]).toEqual(fileMock); @@ -539,7 +548,7 @@ describe('DiffsStoreUtils', () => { const fileMock = getDiffFileMock(); const metaMock = getDiffMetadataMock(); const priorFiles = [{ ...fileMock }]; - const updatedFilesList = utils.prepareDiffData(metaMock, priorFiles); + const updatedFilesList = utils.prepareDiffData({ diff: metaMock, priorFiles, meta: true }); expect(updatedFilesList).toEqual([ fileMock, diff --git a/spec/frontend/diffs/utils/diff_file_spec.js b/spec/frontend/diffs/utils/diff_file_spec.js index dfa63e5177871b64a626e4c7702df90cee59167f..815975f45cf87ae8c9a908437b7d1e76b260ac82 100644 --- a/spec/frontend/diffs/utils/diff_file_spec.js +++ b/spec/frontend/diffs/utils/diff_file_spec.js @@ -1,31 +1,42 @@ import { prepareRawDiffFile } from '~/diffs/utils/diff_file'; -const DIFF_FILES = [ - { - file_hash: 'ABC', // This file is just a normal file - }, - { - file_hash: 'DEF', // This file replaces a symlink - a_mode: '0', - b_mode: '0755', - }, - { - file_hash: 'DEF', // This symlink is replaced by a file - a_mode: '120000', - b_mode: '0', - }, - { - file_hash: 'GHI', // This symlink replaces a file - a_mode: '0', - b_mode: '120000', - }, - { - file_hash: 'GHI', // This file is replaced by a symlink - a_mode: '0755', - b_mode: '0', - }, -]; - +function getDiffFiles() { + return [ + { + file_hash: 'ABC', // This file is just a normal file + file_identifier_hash: 'ABC1', + content_sha: 'C047347', + }, + { + file_hash: 'DEF', // This file replaces a symlink + file_identifier_hash: 'DEF1', + content_sha: 'C047347', + a_mode: '0', + b_mode: '0755', + }, + { + file_hash: 'DEF', // This symlink is replaced by a file + file_identifier_hash: 'DEF2', + content_sha: 'C047347', + a_mode: '120000', + b_mode: '0', + }, + { + file_hash: 'GHI', // This symlink replaces a file + file_identifier_hash: 'GHI1', + content_sha: 'C047347', + a_mode: '0', + b_mode: '120000', + }, + { + file_hash: 'GHI', // This file is replaced by a symlink + file_identifier_hash: 'GHI2', + content_sha: 'C047347', + a_mode: '0755', + b_mode: '0', + }, + ]; +} function makeBrokenSymlinkObject(replaced, wasSymbolic, isSymbolic, wasReal, isReal) { return { replaced, @@ -38,6 +49,12 @@ function makeBrokenSymlinkObject(replaced, wasSymbolic, isSymbolic, wasReal, isR describe('diff_file utilities', () => { describe('prepareRawDiffFile', () => { + let files; + + beforeEach(() => { + files = getDiffFiles(); + }); + it.each` fileIndex | description | brokenSymlink ${0} | ${'a file that is not symlink-adjacent'} | ${false} @@ -49,12 +66,51 @@ describe('diff_file utilities', () => { 'properly marks $description with the correct .brokenSymlink value', ({ fileIndex, brokenSymlink }) => { const preppedRaw = prepareRawDiffFile({ - file: DIFF_FILES[fileIndex], - allFiles: DIFF_FILES, + file: files[fileIndex], + allFiles: files, }); expect(preppedRaw.brokenSymlink).toStrictEqual(brokenSymlink); }, ); + + it.each` + fileIndex | id + ${0} | ${'e075da30-4ec7-4e1c-a505-fe0fb0efe2d8'} + ${1} | ${'5ab05419-123e-4d18-8454-0b8c3d9f3f91'} + ${2} | ${'94eb6bba-575c-4504-bd8e-5d302364d31e'} + ${3} | ${'06d669b2-29b7-4f47-9731-33fc38a8db61'} + ${4} | ${'edd3e8f9-07f9-4647-8171-544c72e5a175'} + `('sets the file id properly { id: $id } on normal diff files', ({ fileIndex, id }) => { + const preppedFile = prepareRawDiffFile({ + file: files[fileIndex], + allFiles: files, + }); + + expect(preppedFile.id).toBe(id); + }); + + it('does not set the `id` property for metadata diff files', () => { + const preppedFile = prepareRawDiffFile({ + file: files[0], + allFiles: files, + meta: true, + }); + + expect(preppedFile).not.toHaveProp('id'); + }); + + it('does not set the id property if the file is missing a `content_sha`', () => { + const fileMissingContentSha = { ...files[0] }; + + delete fileMissingContentSha.content_sha; + + const preppedFile = prepareRawDiffFile({ + file: fileMissingContentSha, + allFiles: files, + }); + + expect(preppedFile).not.toHaveProp('id'); + }); }); });