Prepare gr-rest-api-impl for using updated helper Both helper instances reuse all internal dependencies Remove cache classes from the old helper and only use the new ones. The semantics are the same, the only difference is TS typing information. The old cache had specific versions of set and get for emails and account details, which were removed, since they were creating confusing typing in the cache itself Also change readResponsePayload from old to readJSONResponsePayload and fix the test for _getChangeDetail that didn't correctly stub the helper call (previously worked because the payload is returned but the `parsed = null` on parsing failure) Google-Bug-Id: b/297849592 Release-Notes: skip Change-Id: Iced83d4cb4d4ab21b81316fd767f2d2120cc1a22
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper-old.ts b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper-old.ts index dd6513f..2a19199 100644 --- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper-old.ts +++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper-old.ts
@@ -6,12 +6,7 @@ import {getBaseUrl} from '../../../../utils/url-util'; import {CancelConditionCallback} from '../../../../services/gr-rest-api/gr-rest-api'; import {AuthService} from '../../../../services/gr-auth/gr-auth'; -import { - AccountDetailInfo, - EmailInfo, - ParsedJSON, - RequestPayload, -} from '../../../../types/common'; +import {ParsedJSON, RequestPayload} from '../../../../types/common'; import {HttpMethod} from '../../../../constants/constants'; import {RpcLogEventDetail} from '../../../../types/events'; import { @@ -23,6 +18,7 @@ import {ErrorCallback} from '../../../../api/rest'; import {Scheduler, Task} from '../../../../services/scheduler/scheduler'; import {RetryError} from '../../../../services/scheduler/retry-scheduler'; +import {FetchPromisesCache, SiteBasedCache} from './gr-rest-api-helper'; export const JSON_PREFIX = ")]}'"; @@ -59,127 +55,6 @@ return JSON.parse(jsonWithPrefix.substring(JSON_PREFIX.length)) as ParsedJSON; } -/** - * Wrapper around Map for caching server responses. Site-based so that - * changes to CANONICAL_PATH will result in a different cache going into - * effect. - */ -export class SiteBasedCache { - // TODO(TS): Type looks unusual. Fix it. - // Container of per-canonical-path caches. - private readonly data = new Map< - string | undefined, - unknown | Map<string, ParsedJSON | null> - >(); - - constructor() { - if (window.INITIAL_DATA) { - // Put all data shipped with index.html into the cache. This makes it - // so that we spare more round trips to the server when the app loads - // initially. - Object.entries(window.INITIAL_DATA).forEach(e => - this._cache().set(e[0], e[1] as unknown as ParsedJSON) - ); - } - } - - // Returns the cache for the current canonical path. - _cache(): Map<string, unknown> { - if (!this.data.has(window.CANONICAL_PATH)) { - this.data.set( - window.CANONICAL_PATH, - new Map<string, ParsedJSON | null>() - ); - } - return this.data.get(window.CANONICAL_PATH) as Map< - string, - ParsedJSON | null - >; - } - - has(key: string) { - return this._cache().has(key); - } - - get(key: '/accounts/self/emails'): EmailInfo[] | null; - - get(key: '/accounts/self/detail'): AccountDetailInfo | null; - - get(key: string): ParsedJSON | null; - - get(key: string): unknown { - return this._cache().get(key); - } - - set(key: '/accounts/self/emails', value: EmailInfo[]): void; - - set(key: '/accounts/self/detail', value: AccountDetailInfo): void; - - set(key: string, value: ParsedJSON | null): void; - - set(key: string, value: unknown) { - this._cache().set(key, value); - } - - delete(key: string) { - this._cache().delete(key); - } - - invalidatePrefix(prefix: string) { - const newMap = new Map<string, unknown>(); - for (const [key, value] of this._cache().entries()) { - if (!key.startsWith(prefix)) { - newMap.set(key, value); - } - } - this.data.set(window.CANONICAL_PATH, newMap); - } -} - -type FetchPromisesCacheData = { - [url: string]: Promise<ParsedJSON | undefined> | undefined; -}; - -export class FetchPromisesCache { - private data: FetchPromisesCacheData; - - constructor() { - this.data = {}; - } - - public testOnlyGetData() { - return this.data; - } - - /** - * @return true only if a value for a key sets and it is not undefined - */ - has(key: string): boolean { - return !!this.data[key]; - } - - get(key: string) { - return this.data[key]; - } - - /** - * @param value a Promise to store in the cache. Pass undefined value to - * mark key as deleted. - */ - set(key: string, value: Promise<ParsedJSON | undefined> | undefined) { - this.data[key] = value; - } - - invalidatePrefix(prefix: string) { - const newData: FetchPromisesCacheData = {}; - Object.entries(this.data).forEach(([key, value]) => { - if (!key.startsWith(prefix)) { - newData[key] = value; - } - }); - this.data = newData; - } -} export type FetchParams = { [name: string]: string[] | string | number | boolean | undefined | null; };
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper-old_test.ts b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper-old_test.ts index 3f3fbcb..deb4ba3 100644 --- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper-old_test.ts +++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper-old_test.ts
@@ -4,11 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ import '../../../../test/common-test-setup'; -import { - SiteBasedCache, - FetchPromisesCache, - GrRestApiHelper, -} from './gr-rest-api-helper-old'; +import {GrRestApiHelper} from './gr-rest-api-helper-old'; import {assertFails, waitEventLoop} from '../../../../test/test-utils'; import {FakeScheduler} from '../../../../services/scheduler/fake-scheduler'; import {RetryScheduler} from '../../../../services/scheduler/retry-scheduler'; @@ -18,6 +14,7 @@ import {assert} from '@open-wc/testing'; import {AuthService} from '../../../../services/gr-auth/gr-auth'; import {GrAuthMock} from '../../../../services/gr-auth/gr-auth_mock'; +import {FetchPromisesCache, SiteBasedCache} from './gr-rest-api-helper'; function makeParsedJSON<T>(val: T): ParsedJSON { return val as unknown as ParsedJSON;
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 280b586..ca4d380 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
@@ -9,13 +9,10 @@ import { FetchJSONRequest, FetchParams, - FetchPromisesCache, GrRestApiHelper, parsePrefixedJSON, - readResponsePayload, SendJSONRequest, SendRequest, - SiteBasedCache, } from '../../elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper-old'; import {GrReviewerUpdatesParser} from '../../elements/shared/gr-rest-api-interface/gr-reviewer-updates-parser'; import {parseDate} from '../../utils/date-util'; @@ -150,6 +147,12 @@ import {FlagsService, KnownExperimentId} from '../flags/flags'; import {RetryScheduler} from '../scheduler/retry-scheduler'; import {FixReplacementInfo} from '../../api/rest-api'; +import { + FetchPromisesCache, + GrRestApiHelper as GrRestApiHelperNew, + readJSONResponsePayload, + SiteBasedCache, +} from '../../elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper'; const MAX_PROJECT_RESULTS = 25; @@ -304,6 +307,8 @@ // Private, but used in tests. readonly _restApiHelper: GrRestApiHelper; + readonly _restApiHelperNew: GrRestApiHelperNew; + // Used to serialize requests for certain RPCs readonly _serialScheduler: Scheduler<Response>; @@ -311,12 +316,21 @@ private readonly authService: AuthService, private readonly flagService: FlagsService ) { + const readScheduler = createReadScheduler(); + const writeScheduler = createWriteScheduler(); this._restApiHelper = new GrRestApiHelper( this._cache, this.authService, this._sharedFetchPromises, - createReadScheduler(), - createWriteScheduler() + readScheduler, + writeScheduler + ); + this._restApiHelperNew = new GrRestApiHelperNew( + this._cache, + this.authService, + this._sharedFetchPromises, + readScheduler, + writeScheduler ); this._serialScheduler = createSerializingScheduler(); } @@ -860,7 +874,9 @@ return this._restApiHelper.send(req).then(() => { // If result of getAccountEmails is in cache, update it in the cache // so we don't have to invalidate it. - const cachedEmails = this._cache.get('/accounts/self/emails'); + const cachedEmails = this._cache.get( + '/accounts/self/emails' + ) as unknown as EmailInfo[]; if (cachedEmails) { const emails = cachedEmails.map(entry => { if (entry.email === email) { @@ -869,7 +885,10 @@ return {email: entry.email, preferred: false}; } }); - this._cache.set('/accounts/self/emails', emails); + this._cache.set( + '/accounts/self/emails', + emails as unknown as ParsedJSON + ); } }); } @@ -1365,16 +1384,16 @@ return Promise.resolve(undefined); } - return readResponsePayload(response).then(payload => { - if (!payload) { - return undefined; - } - this._etags.collect(urlWithParams, response, payload.raw); - // TODO(TS): Why it is always change info? - this._maybeInsertInLookup(payload.parsed as unknown as ChangeInfo); + return readJSONResponsePayload(response) + .then(payload => { + this._etags.collect(urlWithParams, response, payload.raw); + this._maybeInsertInLookup( + payload.parsed as unknown as ChangeInfo + ); - return payload.parsed as unknown as ChangeInfo; - }); + return payload.parsed as unknown as ChangeInfo; + }) + .catch(() => undefined); }); } );
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 312bd31..8b4728e 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
@@ -29,6 +29,7 @@ HttpMethod, } from '../../constants/constants'; import { + AccountDetailInfo, BasePatchSetNum, ChangeInfo, ChangeMessageId, @@ -409,7 +410,7 @@ test('getAccount when resp is undefined clears cache', async () => { const cacheKey = '/accounts/self/detail'; const account = createAccountDetailWithId(); - element._cache.set(cacheKey, account); + element._cache.set(cacheKey, account as unknown as ParsedJSON); const stub = sinon .stub(element._restApiHelper, 'fetchCacheURL') .callsFake(async req => { @@ -426,7 +427,7 @@ test('getAccount when status is 403 clears cache', async () => { const cacheKey = '/accounts/self/detail'; const account = createAccountDetailWithId(); - element._cache.set(cacheKey, account); + element._cache.set(cacheKey, account as unknown as ParsedJSON); const stub = sinon .stub(element._restApiHelper, 'fetchCacheURL') .callsFake(async req => { @@ -446,14 +447,17 @@ const stub = sinon .stub(element._restApiHelper, 'fetchCacheURL') .callsFake(async () => { - element._cache.set(cacheKey, account); + element._cache.set(cacheKey, account as unknown as ParsedJSON); return undefined; }); assert.isFalse(element._cache.has(cacheKey)); await element.getAccount(); assert.isTrue(stub.called); - assert.equal(element._cache.get(cacheKey), account); + assert.equal( + element._cache.get(cacheKey), + account as unknown as ParsedJSON + ); }); const preferenceSetup = function (testJSON: unknown, loggedIn: boolean) { @@ -591,7 +595,7 @@ element._cache.set('/accounts/self/emails', [ {email: email1, preferred: true}, {email: email2, preferred: false}, - ]); + ] as unknown as ParsedJSON); await element.setPreferredAccountEmail(email2); assert.isTrue(sendStub.calledOnce); @@ -603,21 +607,26 @@ assert.deepEqual(element._cache.get('/accounts/self/emails'), [ {email: email1, preferred: false}, {email: email2, preferred: true}, - ]); + ] as unknown as ParsedJSON); }); test('setAccountStatus', async () => { const sendStub = sinon .stub(element._restApiHelper, 'send') .resolves('OOO' as unknown as ParsedJSON); - element._cache.set('/accounts/self/detail', createAccountDetailWithId()); + element._cache.set( + '/accounts/self/detail', + createAccountDetailWithId() as unknown as ParsedJSON + ); await element.setAccountStatus('OOO'); assert.isTrue(sendStub.calledOnce); assert.equal(sendStub.lastCall.args[0].method, HttpMethod.PUT); assert.equal(sendStub.lastCall.args[0].url, '/accounts/self/status'); assert.deepEqual(sendStub.lastCall.args[0].body, {status: 'OOO'}); assert.deepEqual( - element._cache.get('/accounts/self/detail')!.status, + (element._cache.get( + '/accounts/self/detail' + ) as unknown as AccountDetailInfo)!.status, 'OOO' ); }); @@ -1147,6 +1156,15 @@ const expectedUrl = `${window.CANONICAL_PATH}/changes/test~4321/detail?O=516714`; const optionsStub = sinon.stub(element._etags, 'getOptions'); const collectStub = sinon.stub(element._etags, 'collect'); + sinon.stub(element._restApiHelper, 'fetchRawJSON').resolves( + new Response( + JSON_PREFIX + + JSON.stringify({ + ...createChange(), + _number: 123 as NumericChangeId, + }) + ) + ); await element._getChangeDetail(changeNum, '516714'); assert.isTrue(optionsStub.calledWithExactly(expectedUrl)); assert.equal(collectStub.lastCall.args[0], expectedUrl);