Store unparsed JSON response in ETag cache
Instead of storing the parsed response and then serializing and
deserializing on every cache hit, the cache can be simplified by storing
the original serial only, and deserializing on cache hit.
ETag cache decorator methods are given JSDocs to encode the fact that
response serials are stored rather than parsed objects. Tests are added
to encode _getChangeDetail's interactions with the decorator cache.
Bug: Issue 7169
Change-Id: I6398b7477957460a3c2fd9bdaf8fa3c8bfa6f22c
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator.js
index 21aa6cb..3570081 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator.js
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator.js
@@ -25,6 +25,14 @@
this._payloadCache = new Map();
}
+ /**
+ * Get or upgrade fetch options to include an ETag in a request.
+ * @param {string} url The URL being fetched.
+ * @param {!Object=} opt_options Optional options object in which to include
+ * the ETag request header. If omitted, the result will be a fresh option
+ * set.
+ * @return {!Object}
+ */
GrEtagDecorator.prototype.getOptions = function(url, opt_options) {
const etag = this._etags.get(url);
if (!etag) {
@@ -36,6 +44,16 @@
return options;
};
+ /**
+ * Handle a response to a request with ETag headers, potentially incorporating
+ * its result in the payload cache.
+ *
+ * @param {string} url The URL of the request.
+ * @param {!Response} response The response object.
+ * @param {string} payload The raw, unparsed JSON contained in the response
+ * body. Note: because response.text() cannot be read twice, this must be
+ * provided separately.
+ */
GrEtagDecorator.prototype.collect = function(url, response, payload) {
if (!response ||
!response.ok ||
@@ -54,19 +72,19 @@
}
};
+ /**
+ * Get the cached payload for a given URL.
+ * @param {string} url
+ * @return {string|undefined} Returns the unparsed JSON payload from the
+ * cache.
+ */
GrEtagDecorator.prototype.getCachedPayload = function(url) {
- let payload = this._payloadCache.get(url);
-
- if (typeof payload === 'object') {
- // Note: For the sake of cache transparency, deep clone the response
- // object so that cache hits are not equal object references. Some code
- // expects every network response to deserialize to a fresh object.
- payload = JSON.parse(JSON.stringify(payload));
- }
-
- return payload;
+ return this._payloadCache.get(url);
};
+ /**
+ * Limit the cache size to MAX_CACHE_SIZE.
+ */
GrEtagDecorator.prototype._truncateCache = function() {
for (const url of this._etags.keys()) {
if (this._etags.size <= MAX_CACHE_SIZE) {
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator_test.html b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator_test.html
index 40e639e..77edae7 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator_test.html
@@ -92,25 +92,5 @@
etag.collect('/foo', fakeRequest('bar', 200), 'new payload');
assert.strictEqual(etag.getCachedPayload('/foo'), 'new payload');
});
-
- test('getCachedPayload does not preserve object equality', () => {
- const payload = {foo: 'bar'};
- etag.collect('/foo', fakeRequest('bar'), payload);
- assert.deepEqual(etag.getCachedPayload('/foo'), payload);
- assert.notStrictEqual(etag.getCachedPayload('/foo'), payload);
- etag.collect('/foo', fakeRequest('bar', 304), {foo: 'baz'});
- assert.deepEqual(etag.getCachedPayload('/foo'), payload);
- assert.notStrictEqual(etag.getCachedPayload('/foo'), payload);
- etag.collect('/foo', fakeRequest('bar', 200), {foo: 'bar baz'});
- assert.deepEqual(etag.getCachedPayload('/foo'), {foo: 'bar baz'});
- assert.notStrictEqual(etag.getCachedPayload('/foo'), {foo: 'bar baz'});
- });
-
- test('getCachedPayload clones the response deeply', () => {
- const payload = {foo: {bar: 'baz'}};
- etag.collect('/foo', fakeRequest('bar'), payload);
- assert.deepEqual(etag.getCachedPayload('/foo'), payload);
- assert.notStrictEqual(etag.getCachedPayload('/foo').foo, payload.foo);
- });
});
</script>
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
index 684a967..210c42f 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
@@ -186,17 +186,34 @@
* @return {?}
*/
getResponseObject(response) {
+ return this._readResponsePayload(response)
+ .then(payload => payload.parsed);
+ },
+
+ /**
+ * @param {!Object} response
+ * @return {!Object}
+ */
+ _readResponsePayload(response) {
return response.text().then(text => {
let result;
try {
- result = JSON.parse(text.substring(JSON_PREFIX.length));
+ result = this._parsePrefixedJSON(text);
} catch (_) {
result = null;
}
- return result;
+ return {parsed: result, raw: text};
});
},
+ /**
+ * @param {string} source
+ * @return {?}
+ */
+ _parsePrefixedJSON(source) {
+ return JSON.parse(source.substring(JSON_PREFIX.length));
+ },
+
getConfig() {
return this._fetchSharedCacheURL('/config/server/info');
},
@@ -820,8 +837,8 @@
this._etags.getOptions(urlWithParams))
.then(response => {
if (response && response.status === 304) {
- return Promise.resolve(
- this._etags.getCachedPayload(urlWithParams));
+ return Promise.resolve(this._parsePrefixedJSON(
+ this._etags.getCachedPayload(urlWithParams)));
}
if (response && !response.ok) {
@@ -834,13 +851,17 @@
}
const payloadPromise = response ?
- this.getResponseObject(response) :
- Promise.resolve();
- payloadPromise.then(payload => {
- this._etags.collect(urlWithParams, response, payload);
+ this._readResponsePayload(response) :
+ Promise.resolve(null);
+
+ return payloadPromise.then(payload => {
+ if (!payload) { return null; }
+
+ this._etags.collect(urlWithParams, response, payload.raw);
this._maybeInsertInLookup(payload);
+
+ return payload.parsed;
});
- return payloadPromise;
});
});
},
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
index b63258f..2058f59 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
@@ -762,25 +762,90 @@
});
});
- test('_getChangeDetail passes params to ETags decorator', () => {
- const changeNum = 4321;
- element._projectLookup[changeNum] = 'test';
- const params = {foo: 'bar'};
- const expectedUrl = '/changes/test~4321/detail?foo=bar';
- sandbox.stub(element._etags, 'getOptions');
- sandbox.stub(element._etags, 'collect');
- return element._getChangeDetail(changeNum, params).then(() => {
- assert.isTrue(element._etags.getOptions.calledWithExactly(expectedUrl));
- assert.equal(element._etags.collect.lastCall.args[0], expectedUrl);
+ suite('_getChangeDetail', () => {
+ test('_getChangeDetail passes params to ETags decorator', () => {
+ const changeNum = 4321;
+ element._projectLookup[changeNum] = 'test';
+ const params = {foo: 'bar'};
+ const expectedUrl = '/changes/test~4321/detail?foo=bar';
+ sandbox.stub(element._etags, 'getOptions');
+ sandbox.stub(element._etags, 'collect');
+ return element._getChangeDetail(changeNum, params).then(() => {
+ assert.isTrue(element._etags.getOptions.calledWithExactly(
+ expectedUrl));
+ assert.equal(element._etags.collect.lastCall.args[0], expectedUrl);
+ });
});
- });
- test('_getChangeDetail calls errFn on 500', () => {
- const errFn = sinon.stub();
- sandbox.stub(element, '_fetchRawJSON')
- .returns(Promise.resolve({ok: false, status: 500}));
- return element._getChangeDetail(123, {}, errFn).then(() => {
- assert.isTrue(errFn.called);
+ test('_getChangeDetail calls errFn on 500', () => {
+ const errFn = sinon.stub();
+ sandbox.stub(element, '_fetchRawJSON')
+ .returns(Promise.resolve({ok: false, status: 500}));
+ return element._getChangeDetail(123, {}, errFn).then(() => {
+ assert.isTrue(errFn.called);
+ });
+ });
+
+ test('_getChangeDetail populates _projectLookup', () => {
+ sandbox.stub(element, '_fetchRawJSON')
+ .returns(Promise.resolve({ok: true}));
+
+ const mockResponse = {_number: 1, project: 'test'};
+ sandbox.stub(element, '_readResponsePayload').returns(Promise.resolve({
+ parsed: mockResponse,
+ raw: JSON.stringify(mockResponse),
+ }));
+ return element._getChangeDetail(1).then(() => {
+ assert.equal(Object.keys(element._projectLookup).length, 1);
+ assert.equal(element._projectLookup[1], 'test');
+ });
+ });
+
+ suite('_getChangeDetail ETag cache', () => {
+ let requestUrl;
+ let mockResponseSerial;
+ let collectSpy;
+ let getPayloadSpy;
+
+ setup(() => {
+ requestUrl = '/foo/bar';
+ const mockResponse = {foo: 'bar', baz: 42};
+ mockResponseSerial = element.JSON_PREFIX +
+ JSON.stringify(mockResponse);
+ sandbox.stub(element, '_urlWithParams').returns(requestUrl);
+ sandbox.stub(element, 'getChangeActionURL')
+ .returns(Promise.resolve(requestUrl));
+ collectSpy = sandbox.spy(element._etags, 'collect');
+ getPayloadSpy = sandbox.spy(element._etags, 'getCachedPayload');
+ });
+
+ test('contributes to cache', () => {
+ sandbox.stub(element, '_fetchRawJSON').returns(Promise.resolve({
+ text: () => Promise.resolve(mockResponseSerial),
+ status: 200,
+ ok: true,
+ }));
+
+ return element._getChangeDetail(123, {}).then(detail => {
+ assert.isFalse(getPayloadSpy.called);
+ assert.isTrue(collectSpy.calledOnce);
+ const cachedResponse = element._etags.getCachedPayload(requestUrl);
+ assert.equal(cachedResponse, mockResponseSerial);
+ });
+ });
+
+ test('uses cache on HTTP 304', () => {
+ sandbox.stub(element, '_fetchRawJSON').returns(Promise.resolve({
+ text: () => Promise.resolve(mockResponseSerial),
+ status: 304,
+ ok: true,
+ }));
+
+ return element._getChangeDetail(123, {}).then(detail => {
+ assert.isFalse(collectSpy.called);
+ assert.isTrue(getPayloadSpy.calledOnce);
+ });
+ });
});
});
@@ -858,17 +923,6 @@
});
});
- test('getChangeDetail populates _projectLookup', () => {
- sandbox.stub(element, '_fetchRawJSON')
- .returns(Promise.resolve({ok: true}));
- sandbox.stub(element, 'getResponseObject')
- .returns(Promise.resolve({_number: 1, project: 'test'}));
- return element._getChangeDetail(1).then(() => {
- assert.equal(Object.keys(element._projectLookup).length, 1);
- assert.equal(element._projectLookup[1], 'test');
- });
- });
-
test('_getChangeURLAndFetch', () => {
element._projectLookup = {1: 'test'};
const fetchStub = sandbox.stub(element, 'fetchJSON')
@@ -886,5 +940,24 @@
'/changes/test~1/revisions/1/test'));
});
});
+
+ suite('reading responses', () => {
+ test('_readResponsePayload', () => {
+ const mockObject = {foo: 'bar', baz: 'foo'};
+ const serial = element.JSON_PREFIX + JSON.stringify(mockObject);
+ const mockResponse = {text: () => Promise.resolve(serial)};
+ return element._readResponsePayload(mockResponse).then(payload => {
+ assert.deepEqual(payload.parsed, mockObject);
+ assert.equal(payload.raw, serial);
+ });
+ });
+
+ test('_parsePrefixedJSON', () => {
+ const obj = {x: 3, y: {z: 4}, w: 23};
+ const serial = element.JSON_PREFIX + JSON.stringify(obj);
+ const result = element._parsePrefixedJSON(serial);
+ assert.deepEqual(result, obj);
+ });
+ });
});
</script>
