Merge "Revise getFromProjectLookup" into stable-3.8

4 files changed, 79 insertions, 58 deletions

diff --git a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts
index f19326455f..e08e213939 100644
--- a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts
+++ b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts
@@ -3083,37 +3083,48 @@ export class GrRestApiServiceImpl implements RestApiService, Finalizable {
       });
   }
 
-  async setInProjectLookup(changeNum: NumericChangeId, project: RepoName) {
-    const lookupProject = await this._projectLookup[changeNum];
-    if (lookupProject && lookupProject !== project) {
-      console.warn(
-        'Change set with multiple project nums.' +
-          'One of them must be invalid.'
-      );
-    }
+  /**
+   * This can be called by the router, if the project can be determined from
+   * the URL. Or when handling a dashabord or a search response.
+   *
+   * Then we don't need to make a dedicated REST API call or have a fallback,
+   * if that fails.
+   */
+  setInProjectLookup(changeNum: NumericChangeId, project: RepoName) {
     this._projectLookup[changeNum] = Promise.resolve(project);
   }
 
   getFromProjectLookup(
     changeNum: NumericChangeId
   ): Promise<RepoName | undefined> {
-    const project = this._projectLookup[`${changeNum}`];
-    if (project) {
-      return project;
-    }
-
-    const onError = (response?: Response | null) => firePageError(response);
-
-    const projectPromise = this.getChange(changeNum, onError).then(change => {
-      if (!change || !change.project) {
-        return;
+    // Hopefully setInProjectLookup() has already been called. Then we don't
+    // have to make a dedicated REST API call to look up the project.
+    let projectPromise = this._projectLookup[changeNum];
+    if (projectPromise) return projectPromise;
+
+    // Ignore errors, because we have some dedicated fallback logic, see below.
+    const onError = () => {};
+    projectPromise = this.getChange(changeNum, onError).then(change => {
+      if (change?.project) return change.project;
+
+      // In the very rare case that the change index cannot provide an answer
+      // (e.g. stale index) we should check, if the router has called
+      // setInProjectLookup() in the meantime. Then we can fall back to that.
+      const currentProjectPromise = this._projectLookup[changeNum];
+      if (currentProjectPromise !== projectPromise) {
+        return currentProjectPromise;
       }
-      this.setInProjectLookup(changeNum, change.project);
-      return change.project;
-    });
 
+      // No luck. Without knowing the project we cannot proceed at all.
+      firePageError(
+        new Response(
+          `Failed to lookup the repo for change number ${changeNum}`,
+          {status: 404}
+        )
+      );
+      return undefined;
+    });
     this._projectLookup[changeNum] = projectPromise;
-
     return projectPromise;
   }
 
@@ -3303,10 +3314,6 @@ export class GrRestApiServiceImpl implements RestApiService, Finalizable {
     return this.getDocsBaseUrlCachedPromise;
   }
 
-  testOnly_clearDocsBaseUrlCache() {
-    this.getDocsBaseUrlCachedPromise = undefined;
-  }
-
   getDocumentationSearches(filter: string): Promise<DocResult[] | undefined> {
     filter = filter.trim();
     const encodedFilter = encodeURIComponent(filter);
diff --git a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl_test.ts b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl_test.ts
index 3ab5c54ba2..696e142336 100644
--- a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl_test.ts
+++ b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl_test.ts
@@ -38,6 +38,7 @@ import {
 } from '../../constants/constants';
 import {
   BasePatchSetNum,
+  ChangeInfo,
   ChangeMessageId,
   CommentInfo,
   DashboardId,
@@ -62,6 +63,7 @@ import {
 import {assert} from '@open-wc/testing';
 import {AuthService} from '../gr-auth/gr-auth';
 import {GrAuthMock} from '../gr-auth/gr-auth_mock';
+import {getBaseUrl} from '../../utils/url-util';
 
 const EXPECTED_QUERY_OPTIONS = listChangesOptionsToHex(
   ListChangesOption.CHANGE_ACTIONS,
@@ -358,7 +360,7 @@ suite('gr-rest-api-service-impl tests', () => {
       assert.isTrue(fetchStub.calledOnce);
       assert.equal(
         fetchStub.firstCall.args[0].url,
-        'test52/accounts/?o=DETAILS&q=%22bro%22'
+        `${getBaseUrl()}/accounts/?o=DETAILS&q=%22bro%22`
       );
     });
 
@@ -367,7 +369,7 @@ suite('gr-rest-api-service-impl tests', () => {
       assert.isTrue(fetchStub.calledOnce);
       assert.equal(
         fetchStub.firstCall.args[0].url,
-        'test53/accounts/?o=DETAILS&q=%22bro%22%20and%20cansee%3A341682'
+        `${getBaseUrl()}/accounts/?o=DETAILS&q=%22bro%22%20and%20cansee%3A341682`
       );
     });
 
@@ -381,8 +383,7 @@ suite('gr-rest-api-service-impl tests', () => {
       assert.isTrue(fetchStub.calledOnce);
       assert.equal(
         fetchStub.firstCall.args[0].url,
-        'test54/accounts/?o=DETAILS&q=%22bro%22%20and%20' +
-          'cansee%3A341682%20and%20is%3Aactive'
+        `${getBaseUrl()}/accounts/?o=DETAILS&q=%22bro%22%20and%20cansee%3A341682%20and%20is%3Aactive`
       );
     });
   });
@@ -1156,32 +1157,46 @@ suite('gr-rest-api-service-impl tests', () => {
   });
 
   test('setInProjectLookup', async () => {
-    await element.setInProjectLookup(
-      555 as NumericChangeId,
-      'project' as RepoName
-    );
+    element.setInProjectLookup(555 as NumericChangeId, 'project' as RepoName);
     const project = await element.getFromProjectLookup(555 as NumericChangeId);
     assert.deepEqual(project, 'project' as RepoName);
   });
 
   suite('getFromProjectLookup', () => {
-    test('getChange succeeds, no project', async () => {
-      sinon.stub(element, 'getChange').resolves(null);
-      const val = await element.getFromProjectLookup(555 as NumericChangeId);
-      assert.strictEqual(val, undefined);
+    const changeNum = 555 as NumericChangeId;
+    const repo = 'test-repo' as RepoName;
+
+    test('getChange fails to yield a project', async () => {
+      const promise = mockPromise<null>();
+      sinon.stub(element, 'getChange').returns(promise);
+
+      const projectLookup = element.getFromProjectLookup(changeNum);
+      promise.resolve(null);
+
+      assert.isUndefined(await projectLookup);
     });
 
     test('getChange succeeds with project', async () => {
-      sinon
-        .stub(element, 'getChange')
-        .resolves({...createChange(), project: 'project' as RepoName});
-      const projectLookup = element.getFromProjectLookup(
-        555 as NumericChangeId
-      );
-      const val = await projectLookup;
-      assert.equal(val, 'project' as RepoName);
+      const promise = mockPromise<null | ChangeInfo>();
+      sinon.stub(element, 'getChange').returns(promise);
+
+      const projectLookup = element.getFromProjectLookup(changeNum);
+      promise.resolve({...createChange(), project: repo});
+
+      assert.equal(await projectLookup, repo);
       assert.deepEqual(element._projectLookup, {'555': projectLookup});
     });
+
+    test('getChange fails, but a setInProjectLookup() call is used as fallback', async () => {
+      const promise = mockPromise<null>();
+      sinon.stub(element, 'getChange').returns(promise);
+
+      const projectLookup = element.getFromProjectLookup(changeNum);
+      element.setInProjectLookup(changeNum, repo);
+      promise.resolve(null);
+
+      assert.equal(await projectLookup, repo);
+    });
   });
 
   suite('getChanges populates _projectLookup', () => {
@@ -1594,10 +1609,11 @@ suite('gr-rest-api-service-impl tests', () => {
     test('null config', async () => {
       const probePathMock = sinon.stub(element, 'probePath').resolves(true);
       const docsBaseUrl = await element.getDocsBaseUrl(undefined);
-      assert.isTrue(
-        probePathMock.calledWith('test91/Documentation/index.html')
+      assert.equal(
+        probePathMock.lastCall.args[0],
+        `${getBaseUrl()}/Documentation/index.html`
       );
-      assert.equal(docsBaseUrl, 'test91/Documentation');
+      assert.equal(docsBaseUrl, `${getBaseUrl()}/Documentation`);
     });
 
     test('no doc config', async () => {
@@ -1607,10 +1623,11 @@ suite('gr-rest-api-service-impl tests', () => {
         gerrit: createGerritInfo(),
       };
       const docsBaseUrl = await element.getDocsBaseUrl(config);
-      assert.isTrue(
-        probePathMock.calledWith('test92/Documentation/index.html')
+      assert.equal(
+        probePathMock.lastCall.args[0],
+        `${getBaseUrl()}/Documentation/index.html`
       );
-      assert.equal(docsBaseUrl, 'test92/Documentation');
+      assert.equal(docsBaseUrl, `${getBaseUrl()}/Documentation`);
     });
 
     test('has doc config', async () => {
@@ -1627,8 +1644,9 @@ suite('gr-rest-api-service-impl tests', () => {
     test('no probe', async () => {
       const probePathMock = sinon.stub(element, 'probePath').resolves(false);
       const docsBaseUrl = await element.getDocsBaseUrl(undefined);
-      assert.isTrue(
-        probePathMock.calledWith('test94/Documentation/index.html')
+      assert.equal(
+        probePathMock.lastCall.args[0],
+        `${getBaseUrl()}/Documentation/index.html`
       );
       assert.isNotOk(docsBaseUrl);
     });
diff --git a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts
index d70304e67c..d8bf276927 100644
--- a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts
+++ b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts
@@ -375,7 +375,6 @@ export interface RestApiService extends Finalizable {
   ): Promise<string>;
 
   getDocsBaseUrl(config?: ServerInfo): Promise<string | null>;
-  testOnly_clearDocsBaseUrlCache(): void;
 
   createChange(
     repo: RepoName,
diff --git a/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts b/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
index f8e416066f..bfa881ecdd 100644
--- a/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
+++ b/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
@@ -321,9 +321,6 @@ export const grRestApiMock: RestApiService = {
     }
     return Promise.resolve('');
   },
-  testOnly_clearDocsBaseUrlCache() {
-    return;
-  },
   getDocumentationSearches(): Promise<DocResult[] | undefined> {
     return Promise.resolve([]);
   },