Update fetchCacheUrl
- Rename to fetchCacheJSON: the method expects the response to be a JSON
- Add documentation and clearly state which requests are considered
"same"
- Include url params in the cache key: None of the existing call sites
set params, but it's error-prone and surprising not to include them.
Params commonly influence the value of the returned response. Since
none of the callers set params at the moment, this is guaranteed safe.
- Add corresponding test for the params
Google-Bug-Id: b/297849592
Release-Notes: skip
Change-Id: Id008a6cfa1462775f18c79ea490e8d7ea918a97e
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper.ts b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper.ts
index 556d976..d5bba65 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper.ts
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper.ts
@@ -440,30 +440,40 @@
return req;
}
- fetchCacheURL(req: FetchRequest): Promise<ParsedJSON | undefined> {
- if (this._fetchPromisesCache.has(req.url)) {
- return this._fetchPromisesCache.get(req.url)!;
+ /**
+ * Fetch JSON using cached value if available.
+ *
+ * If there is an in-flight request with the same url returns the promise for
+ * the in-flight request. If previous call for the same url resulted in the
+ * successful response it is returned. Otherwise a new request is sent.
+ *
+ * Only req.url with req.params is considered for the caching key;
+ * headers or request body are not included in cache key.
+ */
+ fetchCacheJSON(req: FetchRequest): Promise<ParsedJSON | undefined> {
+ const urlWithParams = this.urlWithParams(req.url, req.params);
+ if (this._fetchPromisesCache.has(urlWithParams)) {
+ return this._fetchPromisesCache.get(urlWithParams)!;
}
- // TODO(andybons): Periodic cache invalidation.
- if (this._cache.has(req.url)) {
- return Promise.resolve(this._cache.get(req.url)!);
+ if (this._cache.has(urlWithParams)) {
+ return Promise.resolve(this._cache.get(urlWithParams)!);
}
this._fetchPromisesCache.set(
- req.url,
+ urlWithParams,
this.fetchJSON(req)
.then(response => {
if (response !== undefined) {
- this._cache.set(req.url, response);
+ this._cache.set(urlWithParams, response);
}
- this._fetchPromisesCache.set(req.url, undefined);
+ this._fetchPromisesCache.set(urlWithParams, undefined);
return response;
})
.catch(err => {
- this._fetchPromisesCache.set(req.url, undefined);
+ this._fetchPromisesCache.set(urlWithParams, undefined);
throw err;
})
);
- return this._fetchPromisesCache.get(req.url)!;
+ return this._fetchPromisesCache.get(urlWithParams)!;
}
// if errFn is not set, then only Response possible
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper_test.ts b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper_test.ts
index a212e9d..be1686d 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper_test.ts
@@ -263,6 +263,69 @@
});
});
+ test('cached results', () => {
+ let n = 0;
+ sinon
+ .stub(helper, 'fetchJSON')
+ .callsFake(() => Promise.resolve(makeParsedJSON(++n)));
+ const promises = [];
+ promises.push(helper.fetchCacheJSON({url: '/foo'}));
+ promises.push(helper.fetchCacheJSON({url: '/foo'}));
+ promises.push(helper.fetchCacheJSON({url: '/foo'}));
+
+ return Promise.all(promises).then(results => {
+ assert.deepEqual(results, [
+ makeParsedJSON(1),
+ makeParsedJSON(1),
+ makeParsedJSON(1),
+ ]);
+ return helper.fetchCacheJSON({url: '/foo'}).then(foo => {
+ assert.equal(foo, makeParsedJSON(1));
+ });
+ });
+ });
+
+ test('cached results with param', () => {
+ let n = 0;
+ sinon
+ .stub(helper, 'fetchJSON')
+ .callsFake(() => Promise.resolve(makeParsedJSON(++n)));
+ const promises = [];
+ promises.push(
+ helper.fetchCacheJSON({url: '/foo', params: {hello: 'world'}})
+ );
+ promises.push(helper.fetchCacheJSON({url: '/foo'}));
+ promises.push(
+ helper.fetchCacheJSON({url: '/foo', params: {hello: 'world'}})
+ );
+
+ return Promise.all(promises).then(results => {
+ assert.deepEqual(results, [
+ makeParsedJSON(1),
+ // The url without params is queried again, since it has different url.
+ makeParsedJSON(2),
+ makeParsedJSON(1),
+ ]);
+ return helper
+ .fetchCacheJSON({url: '/foo', params: {hello: 'world'}})
+ .then(foo => {
+ assert.equal(foo, makeParsedJSON(1));
+ });
+ });
+ });
+
+ test('cache invalidation', async () => {
+ cache.set('/foo/bar', makeParsedJSON(1));
+ cache.set('/bar', makeParsedJSON(2));
+ fetchPromisesCache.set('/foo/bar', Promise.resolve(makeParsedJSON(3)));
+ fetchPromisesCache.set('/bar', Promise.resolve(makeParsedJSON(4)));
+ helper.invalidateFetchPromisesPrefix('/foo/');
+ assert.isFalse(cache.has('/foo/bar'));
+ assert.isTrue(cache.has('/bar'));
+ assert.isUndefined(fetchPromisesCache.get('/foo/bar'));
+ assert.strictEqual(makeParsedJSON(4), await fetchPromisesCache.get('/bar'));
+ });
+
test('params are properly encoded', () => {
let url = helper.urlWithParams('/path/', {
sp: 'hola',
