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

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

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

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

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

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

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