[tilemap/config] Fix url manipulation and promise (#9780)

Backports PR #9754

**Commit 1:**
[ui/vis_maps/tests] use $injector to avoid awkward naming

* Original sha: 52d35fb108
* Authored by spalger <spalger@users.noreply.github.com> on 2017-01-04T19:39:37Z

**Commit 2:**
[ui/vis_maps/tests] avoid boolean assertions for better error messages

* Original sha: b118a074c2
* Authored by spalger <spalger@users.noreply.github.com> on 2017-01-04T20:39:53Z

**Commit 3:**
[ui/vis_maps/tests] verify addQueryParams behavior over time

* Original sha: dff8315098
* Authored by spalger <spalger@users.noreply.github.com> on 2017-01-05T19:31:45Z

**Commit 4:**
[ui/vis_maps/tests] stub tilemapsConfig rather than monkey-patching it

* Original sha: 3099bb0bfb
* Authored by spalger <spalger@users.noreply.github.com> on 2017-01-04T20:52:30Z

**Commit 5:**
[ui/vis_maps] remove async/await usage because angular

* Original sha: 084b914a1e
* Authored by spalger <spalger@users.noreply.github.com> on 2017-01-04T21:00:21Z

**Commit 6:**
[ui/vis_maps/tests] add failing test

* Original sha: f38e7dc428
* Authored by spalger <spalger@users.noreply.github.com> on 2017-01-05T20:21:20Z

**Commit 7:**
[ui/url] add modifyUrl() helper

* Original sha: d4b9849fe5
* Authored by spalger <spalger@users.noreply.github.com> on 2017-01-03T19:40:20Z

**Commit 8:**
[ui/vis_maps] use modifyUrl() helper to extend query string

* Original sha: bf4083fc74
* Authored by spalger <spalger@users.noreply.github.com> on 2017-01-03T19:49:56Z

**Commit 9:**
Merge branch 'master' of github.com:elastic/kibana into fix/tilemap-manifest-url-manipulation

* Original sha: 22669aaa63
* Authored by spalger <spalger@users.noreply.github.com> on 2017-01-06T19:21:41Z

src/ui/public/url/__tests__/modify_url.js

@@ -0,0 +1,44 @@
+import { parse as parseUrl } from 'url';
+
+import expect from 'expect.js';
+
+import { modifyUrl } from '../modify_url';
+
+describe('modifyUrl()', () => {
+  it('throws an error with invalid input', () => {
+    expect(() => modifyUrl(1, () => {})).to.throwError();
+    expect(() => modifyUrl(undefined, () => {})).to.throwError();
+    expect(() => modifyUrl('http://localhost')).to.throwError(); // no block
+  });
+
+  it('supports returning a new url spec', () => {
+    expect(modifyUrl('http://localhost', () => ({}))).to.eql('');
+  });
+
+  it('supports modifying the passed object', () => {
+    expect(modifyUrl('http://localhost', parsed => {
+      parsed.port = 9999;
+      parsed.auth = 'foo:bar';
+    })).to.eql('http://foo:bar@localhost:9999/');
+  });
+
+  it('supports changing pathname', () => {
+    expect(modifyUrl('http://localhost/some/path', parsed => {
+      parsed.pathname += '/subpath';
+    })).to.eql('http://localhost/some/path/subpath');
+  });
+
+  it('supports changing port', () => {
+    expect(modifyUrl('http://localhost:5601', parsed => {
+      parsed.port = (parsed.port * 1) + 1;
+    })).to.eql('http://localhost:5602/');
+  });
+
+  it('supports changing protocol', () => {
+    expect(modifyUrl('http://localhost', parsed => {
+      parsed.protocol = 'mail';
+      parsed.slashes = false;
+      parsed.pathname = null;
+    })).to.eql('mail:localhost');
+  });
+});

src/ui/public/url/index.js

@@ -0,0 +1,2 @@
+export { KbnUrlProvider as default } from './url';
+export { modifyUrl } from './modify_url';

src/ui/public/url/modify_url.js

@@ -0,0 +1,61 @@
+import { parse as parseUrl, format as formatUrl } from 'url';
+
+/**
+ *  Takes a URL and a function that takes the meaningful parts
+ *  of the URL as a key-value object, modifies some or all of
+ *  the parts, and returns the modified parts formatted again
+ *  as a url.
+ *
+ *  Url Parts sent:
+ *    - protocol
+ *    - slashes (does the url have the //)
+ *    - auth
+ *    - hostname (just the name of the host, no port or auth information)
+ *    - port
+ *    - pathmame (the path after the hostname, no query or hash, starts
+ *        with a slash if there was a path)
+ *    - query (always an object, even when no query on original url)
+ *    - hash
+ *
+ *  Why?
+ *    - The default url library in node produces several conflicting
+ *      properties on the "parsed" output. Modifying any of these might
+ *      lead to the modifications being ignored (depending on which
+ *      property was modified)
+ *    - It's not always clear wither to use path/pathname, host/hostname,
+ *      so this trys to add helpful constraints
+ *
+ *  @param  {String} url - the url to parse
+ *  @param  {Function<Object|undefined>} block - a function that will modify the parsed url, or return a new one
+ *  @return {String} the modified and reformatted url
+ */
+export function modifyUrl(url, block) {
+  if (typeof block !== 'function') {
+    throw new TypeError('You must pass a block to define the modifications desired');
+  }
+
+  const parsed = parseUrl(url, true);
+
+  // copy over the most specific version of each
+  // property. By default, the parsed url includes
+  // several conflicting properties (like path and
+  // pathname + search, or search and query) and keeping
+  // track of which property is actually used when they
+  // are formatted is harder than necessary
+  const meaningfulParts = {
+    protocol: parsed.protocol,
+    slashes: parsed.slashes,
+    auth: parsed.auth,
+    hostname: parsed.hostname,
+    port: parsed.port,
+    pathname: parsed.pathname,
+    query: parsed.query || {},
+    hash: parsed.hash,
+  };
+
+  // the block modifies the meaningfulParts object, or returns a new one
+  const modifications = block(meaningfulParts) || meaningfulParts;
+
+  // format the modified/replaced meaningfulParts back into a url
+  return formatUrl(modifications);
+}

src/ui/public/url/url.js

@@ -8,8 +8,8 @@ import AppStateProvider from 'ui/state_management/app_state';
 uiModules.get('kibana/url')
 .service('kbnUrl', function (Private) { return Private(KbnUrlProvider); });
 
-function KbnUrlProvider($injector, $location, $rootScope, $parse, Private) {
-  let self = this;
+export function KbnUrlProvider($injector, $location, $rootScope, $parse, Private) {
+  const self = this;
 
   /**
    * Navigate to a url
@@ -198,5 +198,3 @@ function KbnUrlProvider($injector, $location, $rootScope, $parse, Private) {
     return (reloadOnSearch && searchSame) || !reloadOnSearch;
   };
 }
-
-export default KbnUrlProvider;

src/ui/public/vis_maps/__tests__/tile_maps/tilemap_settings.js

@@ -3,40 +3,57 @@ import ngMock from 'ng_mock';
 import url from 'url';
 
 describe('tilemaptest - TileMapSettingsTests-deprecated', function () {
-  let theTileMapSettings;
-  let theTilemapsConfig;
+  let tilemapSettings;
+  let tilemapsConfig;
+  let loadSettings;
 
-  beforeEach(ngMock.module('kibana'));
-  beforeEach(ngMock.inject(function (Private, tilemapSettings, tilemapsConfig) {
-    theTileMapSettings = tilemapSettings;
-    theTilemapsConfig = tilemapsConfig;
-    theTilemapsConfig.deprecated.isOverridden = true;
+  beforeEach(ngMock.module('kibana', ($provide) => {
+    $provide.decorator('tilemapsConfig', () => ({
+      manifestServiceUrl: 'https://proxy-tiles.elastic.co/v1/manifest',
+      deprecated: {
+        isOverridden: true,
+        config: {
+          url: 'https://tiles.elastic.co/v1/default/{z}/{x}/{y}.png?my_app_name=kibana_tests',
+          options: {
+            minZoom: 1,
+            maxZoom: 10,
+            attribution: '© [Elastic Tile Service](https://www.elastic.co/elastic_tile_service)'
+          }
+        },
+      }
+    }));
   }));
 
+  beforeEach(ngMock.inject(function ($injector, $rootScope) {
+    tilemapSettings = $injector.get('tilemapSettings');
+    tilemapsConfig = $injector.get('tilemapsConfig');
+
+    loadSettings = () => {
+      tilemapSettings.loadSettings();
+      $rootScope.$digest();
+    };
+  }));
 
   describe('getting settings', function () {
-
-    beforeEach(async function () {
-      await theTileMapSettings.loadSettings();
+    beforeEach(function () {
+      loadSettings();
     });
 
-    it('should get url', async function () {
+    it('should get url', function () {
 
-      const mapUrl = theTileMapSettings.getUrl();
-      expect(mapUrl.indexOf('{x}') > -1).to.be.ok();
-      expect(mapUrl.indexOf('{y}') > -1).to.be.ok();
-      expect(mapUrl.indexOf('{z}') > -1).to.be.ok();
+      const mapUrl = tilemapSettings.getUrl();
+      expect(mapUrl).to.contain('{x}');
+      expect(mapUrl).to.contain('{y}');
+      expect(mapUrl).to.contain('{z}');
 
       const urlObject = url.parse(mapUrl, true);
-      expect(urlObject.host.endsWith('elastic.co')).to.be.ok();
-      expect(urlObject.query).to.have.property('my_app_name');
-      expect(urlObject.query).to.have.property('my_app_version');
-      expect(urlObject.query).to.have.property('elastic_tile_service_tos');
+      expect(urlObject.hostname).to.be('tiles.elastic.co');
+      expect(urlObject.query).to.have.property('my_app_name', 'kibana_tests');
 
     });
 
-    it('should get options', async function () {
-      const options = theTileMapSettings.getOptions();
+    it('should get options', function () {
+      const options = tilemapSettings.getOptions();
       expect(options).to.have.property('minZoom');
       expect(options).to.have.property('maxZoom');
       expect(options).to.have.property('attribution');

src/ui/public/vis_maps/__tests__/tile_maps/tilemap_settings_mocked.js

@@ -3,110 +3,142 @@ import ngMock from 'ng_mock';
 import url from 'url';
 
 describe('tilemaptest - TileMapSettingsTests-mocked', function () {
-  let theTileMapSettings;
-  let theTilemapsConfig;
-  let oldGetManifest;
+  let tilemapSettings;
+  let tilemapsConfig;
+  let loadSettings;
 
-  const mockGetManifest = async function () {
-    const data = JSON.parse(`
-             {
-                "version":"0.0.0",
-                  "expiry":"14d",
-                  "services":[
-                  {
-                    "id":"road_map",
-                    "url":"https://proxy-tiles.elastic.co/v1/default/{z}/{x}/{y}.png",
-                    "minZoom":0,
-                    "maxZoom":12,
-                    "attribution":"© [Elastic Tile Service](https://www.elastic.co/elastic-tile-service)",
-                    "query_parameters":{
-                      "elastic_tile_service_tos":"agree",
-                      "my_app_name":"kibana"
-                    }
-                  }
-                ]
-              }
-            `);
-
-    return {
-      data: data
-    };
-  };
-
-  beforeEach(ngMock.module('kibana'));
-  beforeEach(ngMock.inject(function (Private, tilemapSettings, tilemapsConfig) {
-    theTileMapSettings = tilemapSettings;
-    theTilemapsConfig = tilemapsConfig;
-
-    //mock the use of a manifest
-    theTilemapsConfig.deprecated.isOverridden = false;
-    oldGetManifest = theTileMapSettings._getTileServiceManifest;
-    theTileMapSettings._getTileServiceManifest = mockGetManifest;
+  beforeEach(ngMock.module('kibana', ($provide) => {
+    $provide.decorator('tilemapsConfig', () => ({
+      manifestServiceUrl: 'http://foo.bar/manifest',
+      deprecated: {
+        isOverridden: false,
+        config: {
+          url: '',
+          options: {
+            minZoom: 1,
+            maxZoom: 10,
+            attribution: '© [Elastic Tile Service](https://www.elastic.co/elastic_tile_service)'
+          }
+        },
+      }
+    }));
   }));
 
-  afterEach(function () {
-    //restore overrides.
-    theTilemapsConfig.isOverridden = true;
-    theTileMapSettings._getTileServiceManifest = oldGetManifest;
-  });
+  beforeEach(ngMock.inject(($injector, $httpBackend) => {
+    tilemapSettings = $injector.get('tilemapSettings');
+    tilemapsConfig = $injector.get('tilemapsConfig');
 
+    loadSettings = (expectedUrl) => {
+      // body and headers copied from https://proxy-tiles.elastic.co/v1/manifest
+      const MANIFEST_BODY = `{
+        "version":"0.0.0",
+          "expiry":"14d",
+          "services":[
+          {
+            "id":"road_map",
+            "url":"https://proxy-tiles.elastic.co/v1/default/{z}/{x}/{y}.png",
+            "minZoom":0,
+            "maxZoom":12,
+            "attribution":"© [Elastic Tile Service](https://www.elastic.co/elastic-tile-service)",
+            "query_parameters":{
+              "elastic_tile_service_tos":"agree",
+              "my_app_name":"kibana"
+            }
+          }
+        ]
+      }`;
+
+      const MANIFEST_HEADERS = {
+        'access-control-allow-methods': 'GET, OPTIONS',
+        'access-control-allow-origin': '*',
+        'content-length': `${MANIFEST_BODY.length}`,
+        'content-type': 'application/json; charset=utf-8',
+        date: (new Date()).toUTCString(),
+        server: 'tileprox/20170102101655-a02e54d',
+        status: '200',
+      };
+
+      $httpBackend
+        .expect('GET', expectedUrl ? expectedUrl : () => true)
+        .respond(MANIFEST_BODY, MANIFEST_HEADERS);
+
+      tilemapSettings.loadSettings();
+
+      $httpBackend.flush();
+    };
+  }));
+
+  afterEach(ngMock.inject($httpBackend => {
+    $httpBackend.verifyNoOutstandingRequest();
+    $httpBackend.verifyNoOutstandingExpectation();
+  }));
 
   describe('getting settings', function () {
-
-    beforeEach(function (done) {
-      theTileMapSettings.loadSettings().then(function () {
-        done();
-      });
+    beforeEach(() => {
+      loadSettings();
     });
 
 
     it('should get url', async function () {
 
-      const mapUrl = theTileMapSettings.getUrl();
-      expect(mapUrl.indexOf('{x}') > -1).to.be.ok();
-      expect(mapUrl.indexOf('{y}') > -1).to.be.ok();
-      expect(mapUrl.indexOf('{z}') > -1).to.be.ok();
+      const mapUrl = tilemapSettings.getUrl();
+      expect(mapUrl).to.contain('{x}');
+      expect(mapUrl).to.contain('{y}');
+      expect(mapUrl).to.contain('{z}');
 
       const urlObject = url.parse(mapUrl, true);
-      expect(urlObject.host.endsWith('elastic.co')).to.be.ok();
-      expect(urlObject.query).to.have.property('my_app_name');
-      expect(urlObject.query).to.have.property('elastic_tile_service_tos');
+      expect(urlObject).to.have.property('hostname', 'proxy-tiles.elastic.co');
+      expect(urlObject.query).to.have.property('my_app_name', 'kibana');
+      expect(urlObject.query).to.have.property('elastic_tile_service_tos', 'agree');
 
     });
 
     it('should get options', async function () {
-      const options = theTileMapSettings.getOptions();
-      expect(options).to.have.property('minZoom');
-      expect(options).to.have.property('maxZoom');
-      expect(options).to.have.property('attribution');
+      const options = tilemapSettings.getOptions();
+      expect(options).to.have.property('minZoom', 0);
+      expect(options).to.have.property('maxZoom', 12);
+      expect(options).to.have.property('attribution').contain('©'); // html entity for ©, ensures that attribution is escaped
     });
 
   });
 
   describe('modify', function () {
-
-    beforeEach(function (done) {
-      theTileMapSettings.addQueryParams({ foo: 'bar' });
-      theTileMapSettings.addQueryParams({ bar: 'stool' });
-      theTileMapSettings.addQueryParams({ foo: 'tstool' });
-      theTileMapSettings.loadSettings().then(function () {
-        done();
-      });
-
-    });
-
-
-    it('addQueryParameters', async function () {
-
-      const mapUrl = theTileMapSettings.getUrl();
+    function assertQuery(expected) {
+      const mapUrl = tilemapSettings.getUrl();
       const urlObject = url.parse(mapUrl, true);
-      expect(urlObject.query).to.have.property('foo');
-      expect(urlObject.query).to.have.property('bar');
-      expect(urlObject.query.foo).to.equal('tstool');
-      expect(urlObject.query.bar).to.equal('stool');
+      Object.keys(expected).forEach(key => {
+        expect(urlObject.query).to.have.property(key, expected[key]);
+      });
+    }
 
+    it('accepts an object', () => {
+      tilemapSettings.addQueryParams({ foo: 'bar' });
+      loadSettings();
+      assertQuery({ foo: 'bar' });
     });
 
+    it('merged additions with previous values', () => {
+      // ensure that changes are always additive
+      tilemapSettings.addQueryParams({ foo: 'bar' });
+      tilemapSettings.addQueryParams({ bar: 'stool' });
+      loadSettings();
+      assertQuery({ foo: 'bar', bar: 'stool' });
+    });
+
+    it('overwrites conflicting previous values', () => {
+      // ensure that conflicts are overwritten
+      tilemapSettings.addQueryParams({ foo: 'bar' });
+      tilemapSettings.addQueryParams({ bar: 'stool' });
+      tilemapSettings.addQueryParams({ foo: 'tstool' });
+      loadSettings();
+      assertQuery({ foo: 'tstool', bar: 'stool' });
+    });
+
+    it('merges query params into manifest request', () => {
+      tilemapSettings.addQueryParams({ foo: 'bar' });
+      tilemapsConfig.manifestServiceUrl = 'http://test.com/manifest?v=1';
+      loadSettings('http://test.com/manifest?v=1&foo=bar');
+    });
 
   });
 

src/ui/public/vis_maps/lib/tilemap_settings.js

@@ -3,6 +3,7 @@ import _ from 'lodash';
 import marked from 'marked';
 import url from 'url';
 import uiRoutes from 'ui/routes';
+import { modifyUrl } from 'ui/url';
 
 marked.setOptions({
   gfm: true, // Github-flavored markdown
@@ -25,6 +26,21 @@ uiModules.get('kibana')
     const attributionFromConfig = $sanitize(marked(tilemapsConfig.deprecated.config.options.attribution || ''));
     const optionsFromConfig = _.assign({}, tilemapsConfig.deprecated.config.options, { attribution: attributionFromConfig });
 
+    const extendUrl = (url, props) => (
+      modifyUrl(url, parsed => _.merge(parsed, props))
+    );
+
+    /**
+     *  Unescape a url template that was escaped by encodeURI() so leaflet
+     *  will be able to correctly locate the varables in the template
+     *  @param  {String} url
+     *  @return {String}
+     */
+    const unescapeTemplateVars = url => {
+      const ENCODED_TEMPLATE_VARS_RE = /%7B(\w+?)%7D/g;
+      return url.replace(ENCODED_TEMPLATE_VARS_RE, (total, varName) => `{${varName}}`);
+    };
+
     class TilemapSettings {
 
       constructor() {
@@ -54,40 +70,41 @@ uiModules.get('kibana')
             return true;
           }
 
-          let manifest;
-          try {
-            const response = await this._getTileServiceManifest(tilemapsConfig.manifestServiceUrl, this._queryParams,
-              attributionFromConfig, optionsFromConfig);
-            manifest = response.data;
+          return this._getTileServiceManifest(tilemapsConfig.manifestServiceUrl, this._queryParams)
+          .then(response => {
+            const manifest = response.data;
             this._error = null;
-          } catch (e) {
-            //request failed. Continue to use old settings.
+
+            this._options = {
+              attribution: $sanitize(marked(manifest.services[0].attribution)),
+              minZoom: manifest.services[0].minZoom,
+              maxZoom: manifest.services[0].maxZoom,
+              subdomains: []
+            };
+
+            this._url = unescapeTemplateVars(extendUrl(manifest.services[0].url, {
+              query: {
+                ...(manifest.services[0].query_parameters || {}),
+                ...this._queryParams
+              }
+            }));
+
             this._settingsInitialized = true;
-            this._error = new Error(`Could not retrieve map service configuration from the manifest-service. ${e.message}`);
+          })
+          .catch(e => {
+            this._settingsInitialized = true;
+            this._error = new Error(`Could not retrieve manifest from the tile service: ${e.message}`);
+          })
+          .then(() => {
             return true;
-          }
-
-          this._options = {
-            attribution: $sanitize(marked(manifest.services[0].attribution)),
-            minZoom: manifest.services[0].minZoom,
-            maxZoom: manifest.services[0].maxZoom,
-            subdomains: []
-          };
-
-          //additional query params need to be propagated to the TMS endpoint as well.
-          const queryparams = _.assign({ }, manifest.services[0].query_parameters, this._queryParams);
-          const query = url.format({ query: queryparams });
-          this._url = manifest.services[0].url + query;//must preserve {} patterns from the url, so do not format path.
-
-          this._settingsInitialized = true;
-          return true;
+          });
         });
       }
 
       /**
        * Must be called before getUrl/getOptions can be called.
        */
-      async loadSettings() {
+      loadSettings() {
         return this._loadSettings();
       }
 
@@ -153,21 +170,14 @@ uiModules.get('kibana')
       /**
        * Make this a method to allow for overrides by test code
        */
-      async  _getTileServiceManifest(manifestUrl, additionalQueryParams) {
-        const manifestServiceTokens = url.parse(manifestUrl);
-        manifestServiceTokens.query = _.assign({}, manifestServiceTokens.query, additionalQueryParams);
-        const requestUrl = url.format(manifestServiceTokens);
-        return await $http({
-          url: requestUrl,
+      _getTileServiceManifest(manifestUrl, additionalQueryParams) {
+        return $http({
+          url: extendUrl(manifestUrl, { query: this._queryParams }),
           method: 'GET'
         });
       }
 
     }
 
-
     return new TilemapSettings();
-
-
   });
-