Use commit SHA instead of branch id

Use the commit SHA instead of the branch ID to request data for
the file tree and raw files in the IDE.  The SHA is obtained as
the most recent commit in the branch at the moment the branch is
first loaded, then remains the same until the branch is changed
or the window is refreshed.  This is done to prevent unusual file
behavior that could occur if another user edits the files while
the IDE is open.

NOTE:

We opted to build the raw path on demand rather than keep it in
the file object because when a new commit is made, it'd take a
lot of processing to update all the paths.

For this reason, this MR adds a few long `joinPaths` calls
to actions like `getFiles`.

app/assets/javascripts/ide/services/index.js

 import axios from '~/lib/utils/axios_utils';
+import { joinPaths } from '~/lib/utils/url_utility';
+import { escapeFileUrl } from '../stores/utils';
 import Api from '~/api';
 
 export default {
@@ -23,18 +25,25 @@ export default {
       .then(({ data }) => data);
   },
   getBaseRawFileData(file, sha) {
-    if (file.tempFile) {
-      return Promise.resolve(file.baseRaw);
-    }
+    if (file.tempFile || file.baseRaw) return Promise.resolve(file.baseRaw);
 
-    if (file.baseRaw) {
-      return Promise.resolve(file.baseRaw);
-    }
+    // if files are renamed, their base path has changed
+    const filePath =
+      file.mrChange && file.mrChange.renamed_file ? file.mrChange.old_path : file.path;
 
     return axios
-      .get(file.rawPath.replace(`/raw/${file.branchId}/${file.path}`, `/raw/${sha}/${file.path}`), {
-        transformResponse: [f => f],
-      })
+      .get(
+        joinPaths(
+          gon.relative_url_root || '/',
+          file.projectId,
+          'raw',
+          sha,
+          escapeFileUrl(filePath),
+        ),
+        {
+          transformResponse: [f => f],
+        },
+      )
       .then(({ data }) => data);
   },
   getProjectData(namespace, project) {
@@ -58,8 +67,8 @@ export default {
   commit(projectId, payload) {
     return Api.commitMultiple(projectId, payload);
   },
-  getFiles(projectUrl, branchId) {
-    const url = `${projectUrl}/files/${branchId}`;
+  getFiles(projectUrl, ref) {
+    const url = `${projectUrl}/files/${ref}`;
     return axios.get(url, { params: { format: 'json' } });
   },
   lastCommitPipelines({ getters }) {

app/assets/javascripts/ide/stores/actions/file.js

 import { joinPaths } from '~/lib/utils/url_utility';
-import { normalizeHeaders } from '~/lib/utils/common_utils';
 import { __ } from '~/locale';
 import eventHub from '../../eventhub';
 import service from '../../services';
 import * as types from '../mutation_types';
 import router from '../../ide_router';
-import { setPageTitle, replaceFileUrl, addFinalNewlineIfNeeded } from '../utils';
+import { escapeFileUrl, addFinalNewlineIfNeeded, setPageTitleForFile } from '../utils';
 import { viewerTypes, stageKeys } from '../../constants';
 
 export const closeFile = ({ commit, state, dispatch }, file) => {
@@ -58,7 +57,7 @@ export const setFileActive = ({ commit, state, getters, dispatch }, path) => {
 };
 
 export const getFileData = (
-  { state, commit, dispatch },
+  { state, commit, dispatch, getters },
   { path, makeFileActive = true, openFile = makeFileActive },
 ) => {
   const file = state.entries[path];
@@ -67,15 +66,18 @@ export const getFileData = (
 
   commit(types.TOGGLE_LOADING, { entry: file });
 
-  const url = file.prevPath ? replaceFileUrl(file.url, file.path, file.prevPath) : file.url;
+  const url = joinPaths(
+    gon.relative_url_root || '/',
+    state.currentProjectId,
+    file.type,
+    getters.lastCommit && getters.lastCommit.id,
+    escapeFileUrl(file.prevPath || file.path),
+  );
 
   return service
-    .getFileData(joinPaths(gon.relative_url_root || '', url.replace('/-/', '/')))
-    .then(({ data, headers }) => {
-      const normalizedHeaders = normalizeHeaders(headers);
-      let title = normalizedHeaders['PAGE-TITLE'];
-      title = file.prevPath ? title.replace(file.prevPath, file.path) : title;
-      setPageTitle(decodeURI(title));
+    .getFileData(url)
+    .then(({ data }) => {
+      setPageTitleForFile(state, file);
 
       if (data) commit(types.SET_FILE_DATA, { data, file });
       if (openFile) commit(types.TOGGLE_FILE_OPEN, path);

app/assets/javascripts/ide/stores/actions/merge_request.js

@@ -152,15 +152,17 @@ export const openMergeRequest = (
     .then(mr => {
       dispatch('setCurrentBranchId', mr.source_branch);
 
-      dispatch('getBranchData', {
+      // getFiles needs to be called after getting the branch data
+      // since files are fetched using the last commit sha of the branch
+      return dispatch('getBranchData', {
         projectId,
         branchId: mr.source_branch,
-      });
-
-      return dispatch('getFiles', {
-        projectId,
-        branchId: mr.source_branch,
-      });
+      }).then(() =>
+        dispatch('getFiles', {
+          projectId,
+          branchId: mr.source_branch,
+        }),
+      );
     })
     .then(() =>
       dispatch('getMergeRequestVersions', {

app/assets/javascripts/ide/stores/actions/tree.js

@@ -46,7 +46,7 @@ export const setDirectoryData = ({ state, commit }, { projectId, branchId, treeL
   });
 };
 
-export const getFiles = ({ state, commit, dispatch }, { projectId, branchId } = {}) =>
+export const getFiles = ({ state, commit, dispatch, getters }, { projectId, branchId } = {}) =>
   new Promise((resolve, reject) => {
     if (
       !state.trees[`${projectId}/${branchId}`] ||
@@ -54,10 +54,11 @@ export const getFiles = ({ state, commit, dispatch }, { projectId, branchId } =
         state.trees[`${projectId}/${branchId}`].tree.length === 0)
     ) {
       const selectedProject = state.projects[projectId];
-      commit(types.CREATE_TREE, { treePath: `${projectId}/${branchId}` });
+      const selectedBranch = getters.findBranch(projectId, branchId);
 
+      commit(types.CREATE_TREE, { treePath: `${projectId}/${branchId}` });
       service
-        .getFiles(selectedProject.web_url, branchId)
+        .getFiles(selectedProject.web_url, selectedBranch.commit.id)
         .then(({ data }) => {
           const { entries, treeList } = decorateFiles({
             data,

app/assets/javascripts/ide/stores/getters.js

@@ -34,7 +34,9 @@ export const currentMergeRequest = state => {
   return null;
 };
 
-export const currentProject = state => state.projects[state.currentProjectId];
+export const findProject = state => projectId => state.projects[projectId];
+
+export const currentProject = (state, getters) => getters.findProject(state.currentProjectId);
 
 export const emptyRepo = state =>
   state.projects[state.currentProjectId] && state.projects[state.currentProjectId].empty_repo;
@@ -94,8 +96,14 @@ export const lastCommit = (state, getters) => {
   return branch ? branch.commit : null;
 };
 
+export const findBranch = (state, getters) => (projectId, branchId) => {
+  const project = getters.findProject(projectId);
+
+  return project && project.branches[branchId];
+};
+
 export const currentBranch = (state, getters) =>
-  getters.currentProject && getters.currentProject.branches[state.currentBranchId];
+  getters.findBranch(state.currentProjectId, state.currentBranchId);
 
 export const branchName = (_state, getters) => getters.currentBranch && getters.currentBranch.name;
 

app/assets/javascripts/ide/stores/utils.js

@@ -113,6 +113,11 @@ export const setPageTitle = title => {
   document.title = title;
 };
 
+export const setPageTitleForFile = (state, file) => {
+  const title = [file.path, state.currentBranchId, state.currentProjectId, 'GitLab'].join(' · ');
+  setPageTitle(title);
+};
+
 export const commitActionForFile = file => {
   if (file.prevPath) {
     return commitActionTypes.move;

changelogs/unreleased/29451-webide-currentsha.yml

+---
+title: 'Resolve: Web IDE Throws Error When Viewing Diff for Renamed Files'
+merge_request: 19348
+author:
+type: fixed

changelogs/unreleased/issue-63160.yml

+---
+title: Use initial commit SHA instead of branch id to request IDE files and contents
+merge_request: 19348
+author: David Palubin
+type: fixed

spec/frontend/ide/services/index_spec.js

+import axios from 'axios';
+import MockAdapter from 'axios-mock-adapter';
 import services from '~/ide/services';
 import Api from '~/api';
+import { escapeFileUrl } from '~/ide/stores/utils';
 
 jest.mock('~/api');
 
 const TEST_PROJECT_ID = 'alice/wonderland';
 const TEST_BRANCH = 'master-patch-123';
 const TEST_COMMIT_SHA = '123456789';
+const TEST_FILE_PATH = 'README2.md';
+const TEST_FILE_OLD_PATH = 'OLD_README2.md';
+const TEST_FILE_PATH_SPECIAL = 'READM?ME/abc';
+const TEST_FILE_CONTENTS = 'raw file content';
 
 describe('IDE services', () => {
   describe('commit', () => {
@@ -28,4 +35,80 @@ describe('IDE services', () => {
       expect(Api.commitMultiple).toHaveBeenCalledWith(TEST_PROJECT_ID, payload);
     });
   });
+
+  describe('getBaseRawFileData', () => {
+    let file;
+    let mock;
+
+    beforeEach(() => {
+      file = {
+        mrChange: null,
+        projectId: TEST_PROJECT_ID,
+        path: TEST_FILE_PATH,
+      };
+
+      jest.spyOn(axios, 'get');
+
+      mock = new MockAdapter(axios);
+    });
+
+    afterEach(() => {
+      mock.restore();
+    });
+
+    it('gives back file.baseRaw for files with that property present', () => {
+      file.baseRaw = TEST_FILE_CONTENTS;
+
+      return services.getBaseRawFileData(file, TEST_COMMIT_SHA).then(content => {
+        expect(content).toEqual(TEST_FILE_CONTENTS);
+      });
+    });
+
+    it('gives back file.baseRaw for files for temp files', () => {
+      file.tempFile = true;
+      file.baseRaw = TEST_FILE_CONTENTS;
+
+      return services.getBaseRawFileData(file, TEST_COMMIT_SHA).then(content => {
+        expect(content).toEqual(TEST_FILE_CONTENTS);
+      });
+    });
+
+    describe.each`
+      relativeUrlRoot | filePath                  | isRenamed
+      ${''}           | ${TEST_FILE_PATH}         | ${false}
+      ${''}           | ${TEST_FILE_OLD_PATH}     | ${true}
+      ${''}           | ${TEST_FILE_PATH_SPECIAL} | ${false}
+      ${''}           | ${TEST_FILE_PATH_SPECIAL} | ${true}
+      ${'gitlab'}     | ${TEST_FILE_OLD_PATH}     | ${true}
+    `(
+      'with relativeUrlRoot ($relativeUrlRoot) and filePath ($filePath) and isRenamed ($isRenamed)',
+      ({ relativeUrlRoot, filePath, isRenamed }) => {
+        beforeEach(() => {
+          if (isRenamed) {
+            file.mrChange = {
+              renamed_file: true,
+              old_path: filePath,
+            };
+          } else {
+            file.path = filePath;
+          }
+
+          gon.relative_url_root = relativeUrlRoot;
+
+          mock
+            .onGet(
+              `${relativeUrlRoot}/${TEST_PROJECT_ID}/raw/${TEST_COMMIT_SHA}/${escapeFileUrl(
+                filePath,
+              )}`,
+            )
+            .reply(200, TEST_FILE_CONTENTS);
+        });
+
+        it('fetches file content', () =>
+          services.getBaseRawFileData(file, TEST_COMMIT_SHA).then(content => {
+            expect(content).toEqual(TEST_FILE_CONTENTS);
+          }));
+      },
+    );
+  });
 });

spec/javascripts/ide/stores/actions/file_spec.js

@@ -182,13 +182,25 @@ describe('IDE store file actions', () => {
       spyOn(service, 'getFileData').and.callThrough();
 
       localFile = file(`newCreate-${Math.random()}`);
-      localFile.url = `project/getFileDataURL`;
       store.state.entries[localFile.path] = localFile;
+
+      store.state.currentProjectId = 'test/test';
+      store.state.currentBranchId = 'master';
+
+      store.state.projects['test/test'] = {
+        branches: {
+          master: {
+            commit: {
+              id: '7297abc',
+            },
+          },
+        },
+      };
     });
 
     describe('success', () => {
       beforeEach(() => {
-        mock.onGet(`${RELATIVE_URL_ROOT}/project/getFileDataURL`).replyOnce(
+        mock.onGet(`${RELATIVE_URL_ROOT}/test/test/7297abc/${localFile.path}`).replyOnce(
           200,
           {
             blame_path: 'blame_path',
@@ -210,7 +222,7 @@ describe('IDE store file actions', () => {
           .dispatch('getFileData', { path: localFile.path })
           .then(() => {
             expect(service.getFileData).toHaveBeenCalledWith(
-              `${RELATIVE_URL_ROOT}/project/getFileDataURL`,
+              `${RELATIVE_URL_ROOT}/test/test/7297abc/${localFile.path}`,
             );
 
             done();
@@ -229,12 +241,11 @@ describe('IDE store file actions', () => {
           .catch(done.fail);
       });
 
-      it('sets document title', done => {
+      it('sets document title with the branchId', done => {
         store
           .dispatch('getFileData', { path: localFile.path })
           .then(() => {
-            expect(document.title).toBe('testing getFileData');
-
+            expect(document.title).toBe(`${localFile.path} · master · test/test · GitLab`);
             done();
           })
           .catch(done.fail);
@@ -283,7 +294,7 @@ describe('IDE store file actions', () => {
         localFile.path = 'new-shiny-file';
         store.state.entries[localFile.path] = localFile;
 
-        mock.onGet(`${RELATIVE_URL_ROOT}/project/getFileDataURL`).replyOnce(
+        mock.onGet(`${RELATIVE_URL_ROOT}/test/test/7297abc/old-dull-file`).replyOnce(
           200,
           {
             blame_path: 'blame_path',
@@ -304,7 +315,7 @@ describe('IDE store file actions', () => {
         store
           .dispatch('getFileData', { path: localFile.path })
           .then(() => {
-            expect(document.title).toBe('testing new-shiny-file');
+            expect(document.title).toBe(`new-shiny-file · master · test/test · GitLab`);
 
             done();
           })
@@ -314,14 +325,17 @@ describe('IDE store file actions', () => {
 
     describe('error', () => {
       beforeEach(() => {
-        mock.onGet(`project/getFileDataURL`).networkError();
+        mock.onGet(`${RELATIVE_URL_ROOT}/test/test/7297abc/${localFile.path}`).networkError();
       });
 
       it('dispatches error action', done => {
         const dispatch = jasmine.createSpy('dispatch');
 
         actions
-          .getFileData({ state: store.state, commit() {}, dispatch }, { path: localFile.path })
+          .getFileData(
+            { state: store.state, commit() {}, dispatch, getters: store.getters },
+            { path: localFile.path },
+          )
           .then(() => {
             expect(dispatch).toHaveBeenCalledWith('setErrorMessage', {
               text: 'An error occurred whilst loading the file.',

spec/javascripts/ide/stores/actions/merge_request_spec.js

@@ -356,8 +356,30 @@ describe('IDE store merge request actions', () => {
         changes: [],
       };
       store.state.entries = {
-        foo: {},
-        bar: {},
+        foo: {
+          type: 'blob',
+        },
+        bar: {
+          type: 'blob',
+        },
+      };
+
+      store.state.currentProjectId = 'test/test';
+      store.state.currentBranchId = 'master';
+
+      store.state.projects['test/test'] = {
+        branches: {
+          master: {
+            commit: {
+              id: '7297abc',
+            },
+          },
+          abcbranch: {
+            commit: {
+              id: '29020fc',
+            },
+          },
+        },
       };
 
       const originalDispatch = store.dispatch;
@@ -415,9 +437,11 @@ describe('IDE store merge request actions', () => {
     it('updates activity bar view and gets file data, if changes are found', done => {
       store.state.entries.foo = {
         url: 'test',
+        type: 'blob',
       };
       store.state.entries.bar = {
         url: 'test',
+        type: 'blob',
       };
 
       testMergeRequestChanges.changes = [

spec/javascripts/ide/stores/actions/tree_spec.js

@@ -31,7 +31,10 @@ describe('Multi-file store tree actions', () => {
       web_url: '',
       branches: {
         master: {
-          workingReference: '1',
+          workingReference: '12345678',
+          commit: {
+            id: '12345678',
+          },
         },
       },
     };
@@ -61,7 +64,7 @@ describe('Multi-file store tree actions', () => {
         store
           .dispatch('getFiles', basicCallParameters)
           .then(() => {
-            expect(service.getFiles).toHaveBeenCalledWith('', 'master');
+            expect(service.getFiles).toHaveBeenCalledWith('', '12345678');
 
             done();
           })
@@ -99,8 +102,18 @@ describe('Multi-file store tree actions', () => {
         store.state.projects = {
           'abc/def': {
             web_url: `${gl.TEST_HOST}/files`,
+            branches: {
+              'master-testing': {
+                commit: {
+                  id: '12345',
+                },
+              },
+            },
           },
         };
+        const getters = {
+          findBranch: () => store.state.projects['abc/def'].branches['master-testing'],
+        };
 
         mock.onGet(/(.*)/).replyOnce(500);
 
@@ -109,6 +122,7 @@ describe('Multi-file store tree actions', () => {
             commit() {},
             dispatch,
             state: store.state,
+            getters,
           },
           {
             projectId: 'abc/def',

spec/javascripts/ide/stores/getters_spec.js

@@ -163,20 +163,57 @@ describe('IDE store getters', () => {
 
   describe('currentBranch', () => {
     it('returns current projects branch', () => {
-      const localGetters = {
-        currentProject: {
-          branches: {
-            master: {
-              name: 'master',
-            },
+      localState.currentProjectId = 'abcproject';
+      localState.currentBranchId = 'master';
+      localState.projects.abcproject = {
+        name: 'abcproject',
+        branches: {
+          master: {
+            name: 'master',
           },
         },
       };
+      const localGetters = {
+        findBranch: jasmine.createSpy('findBranchSpy'),
+      };
+      getters.currentBranch(localState, localGetters);
+
+      expect(localGetters.findBranch).toHaveBeenCalledWith('abcproject', 'master');
+    });
+  });
+
+  describe('findProject', () => {
+    it('returns the project matching the id', () => {
+      localState.currentProjectId = 'abcproject';
+      localState.projects.abcproject = {
+        name: 'abcproject',
+      };
+
+      expect(getters.findProject(localState)('abcproject').name).toBe('abcproject');
+    });
+  });
+
+  describe('findBranch', () => {
+    let result;
+
+    it('returns the selected branch from a project', () => {
+      localState.currentProjectId = 'abcproject';
       localState.currentBranchId = 'master';
+      localState.projects.abcproject = {
+        name: 'abcproject',
+        branches: {
+          master: {
+            name: 'master',
+          },
+        },
+      };
+      const localGetters = {
+        findProject: () => localState.projects.abcproject,
+      };
 
-      expect(getters.currentBranch(localState, localGetters)).toEqual({
-        name: 'master',
-      });
+      result = getters.findBranch(localState, localGetters)('abcproject', 'master');
+
+      expect(result.name).toBe('master');
     });
   });
 

spec/javascripts/ide/stores/utils_spec.js

@@ -11,6 +11,23 @@ describe('Multi-file store utils', () => {
     });
   });
 
+  describe('setPageTitleForFile', () => {
+    it('sets the document page title for the file passed', () => {
+      const f = {
+        path: 'README.md',
+      };
+
+      const state = {
+        currentBranchId: 'master',
+        currentProjectId: 'test/test',
+      };
+
+      utils.setPageTitleForFile(state, f);
+
+      expect(document.title).toBe('README.md · master · test/test · GitLab');
+    });
+  });
+
   describe('findIndexOfFile', () => {
     let localState;