opendev/gerrit
Code Issues Proposed changes

Merge "Store unparsed JSON response in ETag cache"

Browse Source
This commit is contained in:
Wyatt Allen
2017-09-11 17:23:36 +00:00
committed by Gerrit Code Review
parent 4f6fab37ee abd49dd32b
commit b21babc2e9
4 changed files with 159 additions and 67 deletions
Download Patch File Download Diff File Expand all files Collapse all files

38
polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator.js
View File

@@ -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) {

20
polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator_test.html
View File

@@ -92,25 +92,5 @@ limitations under the License.
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>

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

@@ -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;
});
});
},

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

@@ -762,25 +762,90 @@ limitations under the License.
});
});
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 @@ limitations under the License.
});
});
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 @@ limitations under the License.
'/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>