opendev/gerrit
Code Issues Proposed changes

Clean up gr-rest-api-interface's caching

Browse Source 
Replace a shared object with an instance of a class that wraps Map.

The new cache implementation takes window.CANONICAL_PATH into account.
If PolyGerrit is embedded in an environment where some elements are used
before CANONICAL_PATH is set, they would potentially poison the cache.

Change-Id: I43b0d988f2e371caab48934ee101f7d7e723037d
(cherry picked from commit 24a01b06f0)
This commit is contained in:
Logan Hanks
2018-10-22 13:44:13 -07:00
committed by David Pursehouse
parent 8fc534661e
commit 35efa269fa
2 changed files with 80 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files

68
polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
View File

@@ -135,6 +135,42 @@
const ANONYMIZED_REVISION_BASE_URL = ANONYMIZED_CHANGE_BASE_URL + const ANONYMIZED_REVISION_BASE_URL = ANONYMIZED_CHANGE_BASE_URL +
'/revisions/*'; '/revisions/*';
/**
* Wrapper around Map for caching server responses. Site-based so that
* changes to CANONICAL_PATH will result in a different cache going into
* effect.
*/
class SiteBasedCache {
constructor() {
// Container of per-canonical-path caches.
this._data = new Map();
}
// Returns the cache for the current canonical path.
_cache() {
if (!this._data.has(window.CANONICAL_PATH)) {
this._data.set(window.CANONICAL_PATH, new Map());
}
return this._data.get(window.CANONICAL_PATH);
}
has(key) {
return this._cache().has(key);
}
get(key) {
return this._cache().get(key);
}
set(key, value) {
this._cache().set(key, value);
}
delete(key) {
this._cache().delete(key);
}
}
Polymer({ Polymer({
is: 'gr-rest-api-interface', is: 'gr-rest-api-interface',
@@ -171,7 +207,7 @@
properties: { properties: {
_cache: { _cache: {
type: Object, type: Object,
value: {}, // Intentional to share the object across instances. value: new SiteBasedCache(), // Shared across instances.
}, },
_credentialCheck: { _credentialCheck: {
type: Object, type: Object,
@@ -268,7 +304,7 @@
} }
return res; return res;
}).catch(err => { }).catch(err => {
const isLoggedIn = !!this._cache['/accounts/self/detail']; const isLoggedIn = !!this._cache.get('/accounts/self/detail');
if (isLoggedIn && err && err.message === FAILED_TO_FETCH_ERROR) { if (isLoggedIn && err && err.message === FAILED_TO_FETCH_ERROR) {
this.checkCredentials(); this.checkCredentials();
return; return;
@@ -784,7 +820,7 @@
*/ */
saveDiffPreferences(prefs, opt_errFn) { saveDiffPreferences(prefs, opt_errFn) {
// Invalidate the cache. // Invalidate the cache.
this._cache['/accounts/self/preferences.diff'] = undefined; this._cache.delete('/accounts/self/preferences.diff');
return this._send({ return this._send({
method: 'PUT', method: 'PUT',
url: '/accounts/self/preferences.diff', url: '/accounts/self/preferences.diff',
@@ -800,7 +836,7 @@
*/ */
saveEditPreferences(prefs, opt_errFn) { saveEditPreferences(prefs, opt_errFn) {
// Invalidate the cache. // Invalidate the cache.
this._cache['/accounts/self/preferences.edit'] = undefined; this._cache.delete('/accounts/self/preferences.edit');
return this._send({ return this._send({
method: 'PUT', method: 'PUT',
url: '/accounts/self/preferences.edit', url: '/accounts/self/preferences.edit',
@@ -816,7 +852,7 @@
reportUrlAsIs: true, reportUrlAsIs: true,
errFn: resp => { errFn: resp => {
if (!resp || resp.status === 403) { if (!resp || resp.status === 403) {
this._cache['/accounts/self/detail'] = null; this._cache.delete('/accounts/self/detail');
} }
}, },
}); });
@@ -828,7 +864,7 @@
reportUrlAsIs: true, reportUrlAsIs: true,
errFn: resp => { errFn: resp => {
if (!resp || resp.status === 403) { if (!resp || resp.status === 403) {
this._cache['/accounts/self/avatar.change.url'] = null; this._cache.delete('/accounts/self/avatar.change.url');
} }
}, },
}); });
@@ -910,7 +946,7 @@
return this._send(req).then(() => { return this._send(req).then(() => {
// If result of getAccountEmails is in cache, update it in the cache // If result of getAccountEmails is in cache, update it in the cache
// so we don't have to invalidate it. // so we don't have to invalidate it.
const cachedEmails = this._cache['/accounts/self/emails']; const cachedEmails = this._cache.get('/accounts/self/emails');
if (cachedEmails) { if (cachedEmails) {
const emails = cachedEmails.map(entry => { const emails = cachedEmails.map(entry => {
if (entry.email === email) { if (entry.email === email) {
@@ -919,7 +955,7 @@
return {email}; return {email};
} }
}); });
this._cache['/accounts/self/emails'] = emails; this._cache.set('/accounts/self/emails', emails);
} }
}); });
}, },
@@ -930,11 +966,11 @@
_updateCachedAccount(obj) { _updateCachedAccount(obj) {
// If result of getAccount is in cache, update it in the cache // If result of getAccount is in cache, update it in the cache
// so we don't have to invalidate it. // so we don't have to invalidate it.
const cachedAccount = this._cache['/accounts/self/detail']; const cachedAccount = this._cache.get('/accounts/self/detail');
if (cachedAccount) { if (cachedAccount) {
// Replace object in cache with new object to force UI updates. // Replace object in cache with new object to force UI updates.
this._cache['/accounts/self/detail'] = this._cache.set('/accounts/self/detail',
Object.assign({}, cachedAccount, obj); Object.assign({}, cachedAccount, obj));
} }
}, },
@@ -1064,14 +1100,14 @@
if (!res) { return; } if (!res) { return; }
if (res.status === 403) { if (res.status === 403) {
this.fire('auth-error'); this.fire('auth-error');
this._cache['/accounts/self/detail'] = null; this._cache.delete('/accounts/self/detail');
} else if (res.ok) { } else if (res.ok) {
return this.getResponseObject(res); return this.getResponseObject(res);
} }
}).then(res => { }).then(res => {
this._credentialCheck.checking = false; this._credentialCheck.checking = false;
if (res) { if (res) {
this._cache['/accounts/self/detail'] = res; this._cache.delete('/accounts/self/detail');
} }
return res; return res;
}).catch(err => { }).catch(err => {
@@ -1154,13 +1190,13 @@
return this._sharedFetchPromises[req.url]; return this._sharedFetchPromises[req.url];
} }
// TODO(andybons): Periodic cache invalidation. // TODO(andybons): Periodic cache invalidation.
if (this._cache[req.url] !== undefined) { if (this._cache.has(req.url)) {
return Promise.resolve(this._cache[req.url]); return Promise.resolve(this._cache.get(req.url));
} }
this._sharedFetchPromises[req.url] = this._fetchJSON(req) this._sharedFetchPromises[req.url] = this._fetchJSON(req)
.then(response => { .then(response => {
if (response !== undefined) { if (response !== undefined) {
this._cache[req.url] = response; this._cache.set(req.url, response);
} }
this._sharedFetchPromises[req.url] = undefined; this._sharedFetchPromises[req.url] = undefined;
return response; return response;

50
polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
View File

@@ -38,11 +38,15 @@ limitations under the License.
suite('gr-rest-api-interface tests', () => { suite('gr-rest-api-interface tests', () => {
let element; let element;
let sandbox; let sandbox;
let ctr = 0;
setup(() => { setup(() => {
// Modify CANONICAL_PATH to effectively reset cache.
ctr += 1;
window.CANONICAL_PATH = `test${ctr}`;
sandbox = sinon.sandbox.create(); sandbox = sinon.sandbox.create();
element = fixture('basic'); element = fixture('basic');
element._cache = {};
element._projectLookup = {}; element._projectLookup = {};
const testJSON = ')]}\'\n{"hello": "bonjour"}'; const testJSON = ')]}\'\n{"hello": "bonjour"}';
sandbox.stub(window, 'fetch').returns(Promise.resolve({ sandbox.stub(window, 'fetch').returns(Promise.resolve({
@@ -85,7 +89,7 @@ limitations under the License.
test('cached promise', done => { test('cached promise', done => {
const promise = Promise.reject('foo'); const promise = Promise.reject('foo');
element._cache['/foo'] = promise; element._cache.set('/foo', promise);
element._fetchSharedCacheURL({url: '/foo'}).catch(p => { element._fetchSharedCacheURL({url: '/foo'}).catch(p => {
assert.equal(p, 'foo'); assert.equal(p, 'foo');
done(); done();
@@ -98,19 +102,20 @@ limitations under the License.
gr: 'guten tag', gr: 'guten tag',
noval: null, noval: null,
}); });
assert.equal(url, '/path/?sp=hola&gr=guten%20tag&noval'); assert.equal(url,
window.CANONICAL_PATH + '/path/?sp=hola&gr=guten%20tag&noval');
url = element._urlWithParams('/path/', { url = element._urlWithParams('/path/', {
sp: 'hola', sp: 'hola',
en: ['hey', 'hi'], en: ['hey', 'hi'],
}); });
assert.equal(url, '/path/?sp=hola&en=hey&en=hi'); assert.equal(url, window.CANONICAL_PATH + '/path/?sp=hola&en=hey&en=hi');
// Order must be maintained with array params. // Order must be maintained with array params.
url = element._urlWithParams('/path/', { url = element._urlWithParams('/path/', {
l: ['c', 'b', 'a'], l: ['c', 'b', 'a'],
}); });
assert.equal(url, '/path/?l=c&l=b&l=a'); assert.equal(url, window.CANONICAL_PATH + '/path/?l=c&l=b&l=a');
}); });
test('request callbacks can be canceled', done => { test('request callbacks can be canceled', done => {
@@ -441,7 +446,7 @@ limitations under the License.
Promise.reject({message: 'Failed to fetch'})); Promise.reject({message: 'Failed to fetch'}));
window.fetch.onSecondCall().returns(Promise.resolve(fakeAuthResponse)); window.fetch.onSecondCall().returns(Promise.resolve(fakeAuthResponse));
// Emulate logged in. // Emulate logged in.
element._cache['/accounts/self/detail'] = {}; element._cache.set('/accounts/self/detail', {});
const serverErrorStub = sandbox.stub(); const serverErrorStub = sandbox.stub();
element.addEventListener('server-error', serverErrorStub); element.addEventListener('server-error', serverErrorStub);
const authErrorStub = sandbox.stub(); const authErrorStub = sandbox.stub();
@@ -450,7 +455,7 @@ limitations under the License.
flush(() => { flush(() => {
assert.isTrue(authErrorStub.called); assert.isTrue(authErrorStub.called);
assert.isFalse(serverErrorStub.called); assert.isFalse(serverErrorStub.called);
assert.isNull(element._cache['/accounts/self/detail']); assert.isFalse(element._cache.has('/accounts/self/detail'));
done(); done();
}); });
}); });
@@ -471,7 +476,7 @@ limitations under the License.
]; ];
window.fetch.restore(); window.fetch.restore();
sandbox.stub(window, 'fetch', url => { sandbox.stub(window, 'fetch', url => {
if (url === '/accounts/self/detail') { if (url === window.CANONICAL_PATH + '/accounts/self/detail') {
return Promise.resolve(responses.shift()); return Promise.resolve(responses.shift());
} }
}); });
@@ -487,7 +492,7 @@ limitations under the License.
test('checkCredentials promise rejection', () => { test('checkCredentials promise rejection', () => {
window.fetch.restore(); window.fetch.restore();
element._cache['/accounts/self/detail'] = true; element._cache.set('/accounts/self/detail', true);
sandbox.spy(element, 'checkCredentials'); sandbox.spy(element, 'checkCredentials');
sandbox.stub(window, 'fetch', url => { sandbox.stub(window, 'fetch', url => {
return Promise.reject({message: 'Failed to fetch'}); return Promise.reject({message: 'Failed to fetch'});
@@ -515,10 +520,10 @@ limitations under the License.
test('saveDiffPreferences invalidates cache line', () => { test('saveDiffPreferences invalidates cache line', () => {
const cacheKey = '/accounts/self/preferences.diff'; const cacheKey = '/accounts/self/preferences.diff';
sandbox.stub(element, '_send'); sandbox.stub(element, '_send');
element._cache[cacheKey] = {tab_size: 4}; element._cache.set(cacheKey, {tab_size: 4});
element.saveDiffPreferences({tab_size: 8}); element.saveDiffPreferences({tab_size: 8});
assert.isTrue(element._send.called); assert.isTrue(element._send.called);
assert.notOk(element._cache[cacheKey]); assert.isFalse(element._cache.has(cacheKey));
}); });
test('getAccount when resp is null does not add anything to the cache', test('getAccount when resp is null does not add anything to the cache',
@@ -529,11 +534,11 @@ limitations under the License.
element.getAccount().then(() => { element.getAccount().then(() => {
assert.isTrue(element._fetchSharedCacheURL.called); assert.isTrue(element._fetchSharedCacheURL.called);
assert.isNull(element._cache[cacheKey]); assert.isFalse(element._cache.has(cacheKey));
done(); done();
}); });
element._cache[cacheKey] = 'fake cache'; element._cache.set(cacheKey, 'fake cache');
stub.lastCall.args[0].errFn(); stub.lastCall.args[0].errFn();
}); });
@@ -545,10 +550,10 @@ limitations under the License.
element.getAccount().then(() => { element.getAccount().then(() => {
assert.isTrue(element._fetchSharedCacheURL.called); assert.isTrue(element._fetchSharedCacheURL.called);
assert.isNull(element._cache[cacheKey]); assert.isFalse(element._cache.has(cacheKey));
done(); done();
}); });
element._cache[cacheKey] = 'fake cache'; element._cache.set(cacheKey, 'fake cache');
stub.lastCall.args[0].errFn({status: 403}); stub.lastCall.args[0].errFn({status: 403});
}); });
@@ -559,10 +564,10 @@ limitations under the License.
element.getAccount().then(response => { element.getAccount().then(response => {
assert.isTrue(element._fetchSharedCacheURL.called); assert.isTrue(element._fetchSharedCacheURL.called);
assert.equal(element._cache[cacheKey], 'fake cache'); assert.equal(element._cache.get(cacheKey), 'fake cache');
done(); done();
}); });
element._cache[cacheKey] = 'fake cache'; element._cache.set(cacheKey, 'fake cache');
stub.lastCall.args[0].errFn({}); stub.lastCall.args[0].errFn({});
}); });
@@ -728,7 +733,7 @@ limitations under the License.
test('setAccountStatus', () => { test('setAccountStatus', () => {
sandbox.stub(element, '_send').returns(Promise.resolve('OOO')); sandbox.stub(element, '_send').returns(Promise.resolve('OOO'));
element._cache['/accounts/self/detail'] = {}; element._cache.set('/accounts/self/detail', {});
return element.setAccountStatus('OOO').then(() => { return element.setAccountStatus('OOO').then(() => {
assert.isTrue(element._send.calledOnce); assert.isTrue(element._send.calledOnce);
assert.equal(element._send.lastCall.args[0].method, 'PUT'); assert.equal(element._send.lastCall.args[0].method, 'PUT');
@@ -736,7 +741,7 @@ limitations under the License.
'/accounts/self/status'); '/accounts/self/status');
assert.deepEqual(element._send.lastCall.args[0].body, assert.deepEqual(element._send.lastCall.args[0].body,
{status: 'OOO'}); {status: 'OOO'});
assert.deepEqual(element._cache['/accounts/self/detail'], assert.deepEqual(element._cache.get('/accounts/self/detail'),
{status: 'OOO'}); {status: 'OOO'});
}); });
}); });
@@ -830,7 +835,7 @@ limitations under the License.
Promise.resolve([change_num, file_name, file_contents])); Promise.resolve([change_num, file_name, file_contents]));
sandbox.stub(element, 'getResponseObject') sandbox.stub(element, 'getResponseObject')
.returns(Promise.resolve([change_num, file_name, file_contents])); .returns(Promise.resolve([change_num, file_name, file_contents]));
element._cache['/changes/' + change_num + '/edit/' + file_name] = {}; element._cache.set('/changes/' + change_num + '/edit/' + file_name, {});
return element.saveChangeEdit(change_num, file_name, file_contents) return element.saveChangeEdit(change_num, file_name, file_contents)
.then(() => { .then(() => {
assert.isTrue(element._send.calledOnce); assert.isTrue(element._send.calledOnce);
@@ -849,7 +854,7 @@ limitations under the License.
Promise.resolve([change_num, message])); Promise.resolve([change_num, message]));
sandbox.stub(element, 'getResponseObject') sandbox.stub(element, 'getResponseObject')
.returns(Promise.resolve([change_num, message])); .returns(Promise.resolve([change_num, message]));
element._cache['/changes/' + change_num + '/message'] = {}; element._cache.set('/changes/' + change_num + '/message', {});
return element.putChangeCommitMessage(change_num, message).then(() => { return element.putChangeCommitMessage(change_num, message).then(() => {
assert.isTrue(element._send.calledOnce); assert.isTrue(element._send.calledOnce);
assert.equal(element._send.lastCall.args[0].method, 'PUT'); assert.equal(element._send.lastCall.args[0].method, 'PUT');
@@ -1031,7 +1036,8 @@ limitations under the License.
const changeNum = 4321; const changeNum = 4321;
element._projectLookup[changeNum] = 'test'; element._projectLookup[changeNum] = 'test';
const params = {foo: 'bar'}; const params = {foo: 'bar'};
const expectedUrl = '/changes/test~4321/detail?foo=bar'; const expectedUrl =
window.CANONICAL_PATH + '/changes/test~4321/detail?foo=bar';
sandbox.stub(element._etags, 'getOptions'); sandbox.stub(element._etags, 'getOptions');
sandbox.stub(element._etags, 'collect'); sandbox.stub(element._etags, 'collect');
return element._getChangeDetail(changeNum, params).then(() => { return element._getChangeDetail(changeNum, params).then(() => {