From c67111c60bd0bff64fa00268d4ca2664288c9f3f Mon Sep 17 00:00:00 2001 From: Matthew Amato Date: Tue, 20 Mar 2018 20:31:40 -0400 Subject: [PATCH 1/2] Fix ion asset caching ion asset caching was essentially broken because sending the access token in the query string would bust the cache. The initial fix was to allow the token in the Authorization header, which fixed caching but triggered a preflight request for every tile request (due to CORS), this adds unnecessary overhead. To work around this CORS limitation, the ion servers also support including the token in the `Accept` header, which is non-standard but perfectly legal and does not trigger a pre-flight. Unfortunately, including token in the Accept header means we can no longer vary the accept header to handle terrain extensions. Cesium ion supports passing extensions via query parameters, so `CesiumTerrainProvider` now detects an ion resource and uses query parameters for them. Also turned the private and static function `Resource._makeRequest` into a prototype function so that I could override it `IonResource` --- Source/Core/CesiumTerrainProvider.js | 28 +++++-- Source/Core/IonResource.js | 41 ++++++++-- Source/Core/Resource.js | 19 ++--- Source/Core/loadWithXhr.js | 2 +- Specs/Core/CesiumTerrainProviderSpec.js | 19 +++++ Specs/Core/IonResourceSpec.js | 101 ++++++++++++++++++++++-- 6 files changed, 178 insertions(+), 32 deletions(-) diff --git a/Source/Core/CesiumTerrainProvider.js b/Source/Core/CesiumTerrainProvider.js index cdd7644fe905..6c0a1ef0c3cb 100644 --- a/Source/Core/CesiumTerrainProvider.js +++ b/Source/Core/CesiumTerrainProvider.js @@ -623,26 +623,40 @@ define([ extensionList.push('watermask'); } - var resource = layerToUse.resource.getDerivedResource({ - url: urlTemplates[(x + tmsY + level) % urlTemplates.length], + var headers; + var query; + var url = urlTemplates[(x + tmsY + level) % urlTemplates.length]; + var resource = layerToUse.resource; + if (defined(resource._ionEndpoint) && !defined(resource._ionEndpoint.externalType)) { + // ion uses query paremeters to request extensions + if (extensionList.length !== 0) { + query = { extensions: extensionList.join('-') }; + } + headers = getRequestHeader(undefined); + } else { + //All other terrain servers + headers = getRequestHeader(extensionList); + } + + var promise = resource.getDerivedResource({ + url: url, templateValues: { version: layerToUse.version, z: level, x: x, y: tmsY }, - headers: getRequestHeader(extensionList), + queryParameters: query, + headers: headers, request: request - }); - - var promise = resource.fetchArrayBuffer(); + }).fetchArrayBuffer(); if (!defined(promise)) { return undefined; } var that = this; - return when(promise, function(buffer) { + return promise.then(function (buffer) { if (defined(that._heightmapStructure)) { return createHeightmapTerrainData(that, buffer, level, x, y, tmsY); } diff --git a/Source/Core/IonResource.js b/Source/Core/IonResource.js index 329ddba62b3a..bb0b46dd1a78 100644 --- a/Source/Core/IonResource.js +++ b/Source/Core/IonResource.js @@ -42,14 +42,14 @@ define([ Check.defined('endpointResource', endpointResource); //>>includeEnd('debug'); - var externalType = endpoint.externalType; var options; + var externalType = endpoint.externalType; + var isExternal = defined(externalType); - if (!defined(externalType)) { + if (!isExternal) { options = { url: endpoint.url, retryAttempts: 1, - queryParameters: { access_token: endpoint.accessToken }, retryCallback: retryCallback }; } else if (externalType === '3DTILES' || externalType === 'STK_TERRAIN_SERVER') { @@ -74,6 +74,7 @@ define([ // Shared promise for endpooint requests amd credits (only ever set on the root request) this._pendingPromise = undefined; this._credits = undefined; + this._isExternal = isExternal; } if (defined(Object.create)) { @@ -155,13 +156,39 @@ define([ result = Resource.prototype.clone.call(this, result); result._ionRoot = ionRoot; - if (defined(ionRoot.queryParameters.access_token)) { - result.queryParameters.access_token = ionRoot.queryParameters.access_token; - } + result._isExternal = this._isExternal; return result; }; + IonResource.prototype.fetchImage = function (preferBlob, allowCrossOrigin) { + return Resource.prototype.fetchImage.call(this, this._isExternal ? preferBlob : true, allowCrossOrigin); + }; + + IonResource.prototype._makeRequest = function(options) { + if (this._isExternal) { + return Resource.prototype._makeRequest.call(this, options); + } + + var acceptToken = '*/*;access_token=' + this._ionEndpoint.accessToken; + var existingAccept = acceptToken; + + var oldHeaders = this.headers; + if (defined(oldHeaders) && defined(oldHeaders.Accept)) { + existingAccept = oldHeaders.Accept + ',' + acceptToken; + } + + if (!defined(options.headers)) { + options.headers = { Accept: existingAccept }; + } else if (!defined(options.headers.Accept)) { + options.headers.Accept = existingAccept; + } else { + options.headers.Accept = options.headers.Accept + ',' + acceptToken; + } + + return Resource.prototype._makeRequest.call(this, options); + }; + /** * @private */ @@ -204,7 +231,6 @@ define([ .then(function(newEndpoint) { //Set the token for root resource so new derived resources automatically pick it up ionRoot._ionEndpoint = newEndpoint; - ionRoot.queryParameters.access_token = newEndpoint.accessToken; return newEndpoint; }) .always(function(newEndpoint) { @@ -217,7 +243,6 @@ define([ return ionRoot._pendingPromise.then(function(newEndpoint) { // Set the new token and endpoint for this resource that._ionEndpoint = newEndpoint; - that.queryParameters.access_token = newEndpoint.accessToken; return true; }); } diff --git a/Source/Core/Resource.js b/Source/Core/Resource.js index bb51d17e9cde..0817c18f8b66 100644 --- a/Source/Core/Resource.js +++ b/Source/Core/Resource.js @@ -1240,7 +1240,8 @@ define([ /** * @private */ - Resource._makeRequest = function(resource, options) { + Resource.prototype._makeRequest = function(options) { + var resource = this; checkAndResetRequest(resource.request); var request = resource.request; @@ -1248,7 +1249,7 @@ define([ request.requestFunction = function() { var responseType = options.responseType; - var headers = combine(resource.headers, options.headers); + var headers = combine(options.headers, resource.headers); var overrideMimeType = options.overrideMimeType; var method = options.method; var data = options.data; @@ -1369,7 +1370,7 @@ define([ options = defaultClone(options, {}); options.method = 'GET'; - return Resource._makeRequest(this, options); + return this._makeRequest(options); }; /** @@ -1425,7 +1426,7 @@ define([ options = defaultClone(options, {}); options.method = 'DELETE'; - return Resource._makeRequest(this, options); + return this._makeRequest(options); }; /** @@ -1481,7 +1482,7 @@ define([ options = defaultClone(options, {}); options.method = 'HEAD'; - return Resource._makeRequest(this, options); + return this._makeRequest(options); }; /** @@ -1537,7 +1538,7 @@ define([ options = defaultClone(options, {}); options.method = 'OPTIONS'; - return Resource._makeRequest(this, options); + return this._makeRequest(options); }; /** @@ -1597,7 +1598,7 @@ define([ options.method = 'POST'; options.data = data; - return Resource._makeRequest(this, options); + return this._makeRequest(options); }; /** @@ -1658,7 +1659,7 @@ define([ options.method = 'PUT'; options.data = data; - return Resource._makeRequest(this, options); + return this._makeRequest(options); }; /** @@ -1719,7 +1720,7 @@ define([ options.method = 'PATCH'; options.data = data; - return Resource._makeRequest(this, options); + return this._makeRequest(options); }; /** diff --git a/Source/Core/loadWithXhr.js b/Source/Core/loadWithXhr.js index ad3c6a17ffdf..0afb09b7a4d0 100644 --- a/Source/Core/loadWithXhr.js +++ b/Source/Core/loadWithXhr.js @@ -63,7 +63,7 @@ define([ // Take advantage that most parameters are the same var resource = new Resource(options); - return Resource._makeRequest(resource, { + return resource._makeRequest({ responseType: options.responseType, overrideMimeType: options.overrideMimeType, method: defaultValue(options.method, 'GET'), diff --git a/Specs/Core/CesiumTerrainProviderSpec.js b/Specs/Core/CesiumTerrainProviderSpec.js index 73a52c045821..6c57307a14f8 100644 --- a/Specs/Core/CesiumTerrainProviderSpec.js +++ b/Specs/Core/CesiumTerrainProviderSpec.js @@ -5,6 +5,7 @@ defineSuite([ 'Core/GeographicTilingScheme', 'Core/getAbsoluteUri', 'Core/HeightmapTerrainData', + 'Core/IonResource', 'Core/Math', 'Core/QuantizedMeshTerrainData', 'Core/Request', @@ -20,6 +21,7 @@ defineSuite([ GeographicTilingScheme, getAbsoluteUri, HeightmapTerrainData, + IonResource, CesiumMath, QuantizedMeshTerrainData, Request, @@ -746,5 +748,22 @@ defineSuite([ expect(loadedData).toBeInstanceOf(HeightmapTerrainData); }); }); + + it('Uses query parameter extensions for ion resource', function() { + var terrainProvider = new CesiumTerrainProvider({ + url: IonResource.fromAssetId(1), + requestVertexNormals: true, + requestWaterMask: true + }); + + return pollToPromise(function() { + return terrainProvider.ready; + }).then(function() { + var getDerivedResource = spyOn(IonResource.prototype, 'getDerivedResource').and.callThrough(); + terrainProvider.requestTileGeometry(0, 0, 0); + var options = getDerivedResource.calls.argsFor(0)[0]; + expect(options.queryParameters.extensions).toEqual('octvertexnormals-watermask'); + }); + }); }); }); diff --git a/Specs/Core/IonResourceSpec.js b/Specs/Core/IonResourceSpec.js index 2a961007bf98..c3238098d33b 100644 --- a/Specs/Core/IonResourceSpec.js +++ b/Specs/Core/IonResourceSpec.js @@ -32,8 +32,7 @@ defineSuite([ expect(Resource.call).toHaveBeenCalledWith(resource, { url: endpoint.url, retryCallback: resource.retryCallback, - retryAttempts: 1, - queryParameters: { access_token: endpoint.accessToken } + retryAttempts: 1 }); }); @@ -45,7 +44,7 @@ defineSuite([ expect(cloned._ionRoot).toBe(resource); cloned._ionRoot = undefined; expect(cloned.retryCallback).toBe(resource.retryCallback); - expect(cloned.queryParameters.access_token).toBe(resource.queryParameters.access_token); + expect(cloned.headers.Authorization).toBe(resource.headers.Authorization); expect(cloned).toEqual(resource); }); @@ -53,7 +52,6 @@ defineSuite([ var endpointResource = IonResource._createEndpointResource(assetId); var resource = new IonResource(endpoint, endpointResource); expect(resource.getUrlComponent()).toEqual(endpoint.url); - expect(resource.queryParameters).toEqual({ access_token: 'not_really_a_refresh_token' }); expect(resource._ionEndpoint).toBe(endpoint); expect(resource._ionEndpointResource).toEqual(endpointResource); expect(resource.retryCallback).toBeDefined(); @@ -91,7 +89,7 @@ defineSuite([ return IonResource.fromAssetId(123890213) .then(function(resource) { expect(resource.url).toEqual(externalEndpoint.options.url); - expect(resource.queryParameters.access_token).toBeUndefined(); + expect(resource.headers.Authorization).toBeUndefined(); expect(resource.retryCallback).toBeUndefined(); }); } @@ -157,6 +155,95 @@ defineSuite([ Ion.defaultAccessToken = defaultAccessToken; }); + it('Calls base _makeRequest with expected options when resource no Accept header is already defined', function() { + var originalOptions = {}; + var expectedOptions = { + headers: { + Accept: '*/*;access_token=' + endpoint.accessToken + } + }; + + var _makeRequest = spyOn(Resource.prototype, '_makeRequest'); + var endpointResource = IonResource._createEndpointResource(assetId); + var resource = new IonResource(endpoint, endpointResource); + resource._makeRequest(originalOptions); + expect(_makeRequest).toHaveBeenCalledWith(expectedOptions); + }); + + it('Calls base _makeRequest with expected options when resource Accept header is already defined', function() { + var originalOptions = {}; + var expectedOptions = { + headers: { + Accept: 'application/json,*/*;access_token=' + endpoint.accessToken + } + }; + + var _makeRequest = spyOn(Resource.prototype, '_makeRequest'); + var endpointResource = IonResource._createEndpointResource(assetId); + var resource = new IonResource(endpoint, endpointResource); + resource.headers.Accept = 'application/json'; + resource._makeRequest(originalOptions); + expect(_makeRequest).toHaveBeenCalledWith(expectedOptions); + }); + + it('Calls base _makeRequest with expected options when options header.Accept is already defined', function() { + var originalOptions = { + headers: { + Accept: 'application/json' + } + }; + var expectedOptions = { + headers: { + Accept: 'application/json,*/*;access_token=' + endpoint.accessToken + } + }; + + var _makeRequest = spyOn(Resource.prototype, '_makeRequest'); + var endpointResource = IonResource._createEndpointResource(assetId); + var resource = new IonResource(endpoint, endpointResource); + resource._makeRequest(originalOptions); + expect(_makeRequest).toHaveBeenCalledWith(expectedOptions); + }); + + it('Calls base _makeRequest with no changes for external assets', function() { + var externalEndpoint = { + type: '3DTILES', + externalType: '3DTILES', + options: { url: 'https://test.invalid/tileset.json' }, + attributions: [] + }; + var options = {}; + + var _makeRequest = spyOn(Resource.prototype, '_makeRequest'); + var endpointResource = IonResource._createEndpointResource(assetId); + var resource = new IonResource(externalEndpoint, endpointResource); + resource._makeRequest(options); + expect(_makeRequest.calls.argsFor(0)[0]).toBe(options); + }); + + it('Calls base fetchImage with preferBlob for ion assets', function() { + var fetchImage = spyOn(Resource.prototype, 'fetchImage'); + var endpointResource = IonResource._createEndpointResource(assetId); + var resource = new IonResource(endpoint, endpointResource); + resource.fetchImage(false, true); + expect(fetchImage).toHaveBeenCalledWith(true, true); + }); + + it('Calls base fetchImage with no changes for external assets', function() { + var externalEndpoint = { + type: '3DTILES', + externalType: '3DTILES', + options: { url: 'https://test.invalid/tileset.json' }, + attributions: [] + }; + + var fetchImage = spyOn(Resource.prototype, 'fetchImage'); + var endpointResource = IonResource._createEndpointResource(assetId); + var resource = new IonResource(externalEndpoint, endpointResource); + resource.fetchImage(false, true); + expect(fetchImage).toHaveBeenCalledWith(false, true); + }); + describe('retryCallback', function() { var endpointResource; var resource; @@ -200,8 +287,8 @@ defineSuite([ var promise = retryCallback(resource, event); var resultPromise = promise.then(function(result) { - expect(resource.queryParameters.access_token).toEqual(newEndpoint.accessToken); expect(result).toBe(true); + expect(resource._ionEndpoint).toBe(newEndpoint); }); expect(endpointResource.fetchJson).toHaveBeenCalledWith(); @@ -231,7 +318,7 @@ defineSuite([ return testCallback(derived, error) .then(function(){ expect(derived._ionEndpoint).toBe(resource._ionEndpoint); - expect(derived.queryParameters.access_token).toEqual(resource.queryParameters.access_token); + expect(derived.headers.Authorization).toEqual(resource.headers.Authorization); }); }); }); From 8dbcd0d85d6b84eac9c31186b7af80dc480b728c Mon Sep 17 00:00:00 2001 From: Matthew Amato Date: Tue, 20 Mar 2018 20:34:50 -0400 Subject: [PATCH 2/2] Update CHANGES --- CHANGES.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGES.md b/CHANGES.md index 64d757a5d56a..f6dc64f603ba 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -11,6 +11,7 @@ Change Log * `ClippingPlaneCollection` should now be used with `ClippingPlane` objects instead of `Plane`. Use of `Plane` objects has been deprecated and will be removed in Cesium 1.45. ##### Additions :tada: +* Fix Cesium ion browser caching [#6353](https://github.com/AnalyticalGraphicsInc/cesium/pull/6353). * Added support for glTF models with [Draco geometry compression](https://github.com/fanzhanggoogle/glTF/blob/KHR_mesh_compression/extensions/Khronos/KHR_draco_mesh_compression/README.md). * `ClippingPlaneCollection` updates [#6201](https://github.com/AnalyticalGraphicsInc/cesium/pull/6201) * Removed the 6-clipping-plane limit.