From ade2e11a75fab5ebdf882faa1836343b05de1382 Mon Sep 17 00:00:00 2001 From: Viktar Donich Date: Tue, 18 Jul 2017 09:13:25 -0700 Subject: [PATCH] Use ETag and If-None-Match for GET /change/detail This prevents regenerating JSON response if nothing changed. Feature: Issue 6639 Change-Id: I1d542928d7b48a049cd4328f1a2ac1dcd1017cd6 --- .../gr-etag-decorator.html | 21 ++++ .../gr-etag-decorator.js | 72 ++++++++++++++ .../gr-etag-decorator_test.html | 96 +++++++++++++++++++ .../gr-rest-api-interface.html | 6 +- .../gr-rest-api-interface.js | 82 ++++++++++++---- .../gr-rest-api-interface_test.html | 10 +- 6 files changed, 263 insertions(+), 24 deletions(-) create mode 100644 polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator.html create mode 100644 polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator.js create mode 100644 polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator_test.html diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator.html b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator.html new file mode 100644 index 0000000000..d0306b8812 --- /dev/null +++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator.html @@ -0,0 +1,21 @@ + + + + + + + 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 new file mode 100644 index 0000000000..6c2f6b87dc --- /dev/null +++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator.js @@ -0,0 +1,72 @@ +// Copyright (C) 2017 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +(function(window) { + 'use strict'; + + // Prevent redefinition. + if (window.GrEtagDecorator) { return; } + + // Limit cache size because /change/detail responses may be large. + const MAX_CACHE_SIZE = 30; + + function GrEtagDecorator() { + this._etags = new Map(); + this._payloadCache = new Map(); + } + + GrEtagDecorator.prototype.getOptions = function(url, opt_options) { + const etag = this._etags.get(url); + if (!etag) { + return opt_options; + } + const options = Object.assign({}, opt_options); + options.headers = options.headers || new Headers(); + options.headers.set('If-None-Match', this._etags.get(url)); + return options; + }; + + GrEtagDecorator.prototype.collect = function(url, response, payload) { + if (!response || + !response.ok || + response.status !== 200 || + response.status === 304) { + // 304 Not Modified means etag is still valid. + return; + } + this._payloadCache.set(url, payload); + const etag = response.headers && response.headers.get('etag'); + if (!etag) { + this._etags.delete(url); + } else { + this._etags.set(url, etag); + this._trunkateCache(); + } + }; + + GrEtagDecorator.prototype.getCachedPayload = function(url) { + return this._payloadCache.get(url); + }; + + GrEtagDecorator.prototype._trunkateCache = function() { + for (const url of this._etags.keys()) { + if (this._etags.size <= MAX_CACHE_SIZE) { + break; + } + this._etags.delete(url); + this._payloadCache.delete(url); + } + }; + + window.GrEtagDecorator = GrEtagDecorator; +})(window); 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 new file mode 100644 index 0000000000..8be2352e65 --- /dev/null +++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator_test.html @@ -0,0 +1,96 @@ + + + + +gr-etag-decorator + + + + + + + + diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.html b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.html index 753c26eac3..b8ed52acb7 100644 --- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.html +++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.html @@ -14,10 +14,12 @@ See the License for the specific language governing permissions and limitations under the License. --> - - + + + + 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 eb12791138..4dd56818a2 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 @@ -28,6 +28,7 @@ }; let auth = null; + const etags = new GrEtagDecorator(); Polymer({ is: 'gr-rest-api-interface', @@ -74,24 +75,26 @@ auth = window.USE_GAPI_AUTH ? new GrGapiAuth() : new GrGerritAuth(); }, - fetchJSON(url, opt_errFn, opt_cancelCondition, opt_params, opt_options) { + /** + * Fetch JSON from url provided. + * Returns a Promise that resolves to a native Response. + * Doesn't do error checking. Supports cancel condition. Performs auth. + * Validates auth expiry errors. + * @param {string} url + * @param {function(response, error)} opt_errFn + * @param {function()} opt_cancelCondition + * @param {Object=} opt_params URL params, key-value hash. + * @param {Object=} opt_options Fetch options. + */ + _fetchRawJSON(url, opt_errFn, opt_cancelCondition, opt_params, + opt_options) { const urlWithParams = this._urlWithParams(url, opt_params); return auth.fetch(urlWithParams, opt_options).then(response => { if (opt_cancelCondition && opt_cancelCondition()) { response.body.cancel(); return; } - - if (!response.ok) { - if (opt_errFn) { - opt_errFn.call(null, response); - return; - } - this.fire('server-error', {response}); - return; - } - - return this.getResponseObject(response); + return response; }).catch(err => { if (opt_errFn) { opt_errFn.call(null, null, err); @@ -102,6 +105,35 @@ }); }, + /** + * Fetch JSON from url provided. + * Returns a Promise that resolves to a parsed response. + * Same as {@link _fetchRawJSON}, plus error handling. + * @param {string} url + * @param {function(response, error)} opt_errFn + * @param {function()} opt_cancelCondition + * @param {Object=} opt_params URL params, key-value hash. + * @param {Object=} opt_options Fetch options. + */ + fetchJSON(url, opt_errFn, opt_cancelCondition, opt_params, opt_options) { + return this._fetchRawJSON( + url, opt_errFn, opt_cancelCondition, opt_params, opt_options) + .then(response => { + if (!response) { + return; + } + if (!response.ok) { + if (opt_errFn) { + opt_errFn.call(null, response); + return; + } + this.fire('server-error', {response}); + return; + } + return response && this.getResponseObject(response); + }); + }, + _checkAuthRedirect() { const loggedIn = !!this._cache['/accounts/self/detail']; if (!loggedIn) { @@ -462,20 +494,34 @@ }, getDiffChangeDetail(changeNum, opt_errFn, opt_cancelCondition) { - const options = this.listChangesOptionsToHex( + const params = this.listChangesOptionsToHex( this.ListChangesOption.ALL_REVISIONS ); - return this._getChangeDetail(changeNum, options, opt_errFn, + return this._getChangeDetail(changeNum, params, opt_errFn, opt_cancelCondition); }, - _getChangeDetail(changeNum, options, opt_errFn, + _getChangeDetail(changeNum, params, opt_errFn, opt_cancelCondition) { - return this.fetchJSON( - this.getChangeActionURL(changeNum, null, '/detail'), + const url = this.getChangeActionURL(changeNum, null, '/detail'); + return this._fetchRawJSON( + url, opt_errFn, opt_cancelCondition, - {O: options}); + {O: params}, + etags.getOptions(url)) + .then(response => { + if (response && response.status === 304) { + return Promise.resolve(etags.getCachedPayload(url)); + } else { + const payloadPromise = response ? + this.getResponseObject(response) : Promise.resolve(); + payloadPromise.then(payload => { + etags.collect(url, response, payload); + }); + return payloadPromise; + } + }); }, getChangeCommitInfo(changeNum, patchNum) { 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 63ff596af5..6e34a32a42 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 @@ -441,10 +441,12 @@ limitations under the License. const authErrorStub = sandbox.stub(); element.addEventListener('auth-error', authErrorStub); element.fetchJSON('/bar').then(r => { - assert.isTrue(authErrorStub.called); - assert.isFalse(serverErrorStub.called); - assert.isNull(element._cache['/accounts/self/detail']); - done(); + flush(() => { + assert.isTrue(authErrorStub.called); + assert.isFalse(serverErrorStub.called); + assert.isNull(element._cache['/accounts/self/detail']); + done(); + }); }); });