Create addParams, replacing queryEncoder

Summary:
The existing `queryEncoder` function had a significant flaw: it always
returned a string starting with `"?"`, but the initial delimiter should
be `"&"` if the URL already contains a query component. This could
happen unexpectedly if, say, an application embeds TensorBoard and
monkey-patches the router to include some parameters in every URL, so
that a call like `pluginRoute` has query parameters where the original
author of the plugin expects none.

A simple solution is to replace this with a function `addParams` that
constructs the query string and attaches it to the URL in one go, adding
to an existing string if one exists. This new function also now supports
repeated parameters for convenience.

Test Plan:
I tested that all existing plugins work as intended. I didn’t test the
JS test because we have no way to run them.

wchargin-branch: add-params

Author: wchargin

tensorboard/components/tf_backend/router.ts

@@ -13,7 +13,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 ==============================================================================*/
 
-import {queryEncoder} from './urlPathHelpers.js';
+import {addParams} from './urlPathHelpers.js';
 
 export type RunTagUrlFn = (tag: string, run: string) => string;
 
@@ -41,7 +41,7 @@ export function createRouter(dataDir = 'data', demoMode = false): Router {
   function standardRoute(route: string, demoExtension = '.json'):
       ((tag: string, run: string) => string) {
     return function(tag: string, run: string): string {
-      return dataDir + '/' + route + queryEncoder({tag: tag, run: run});
+      return dataDir + '/' + addParams(route, {tag, run});
     };
   }
   function pluginRoute(pluginName: string, route: string): string {
@@ -50,7 +50,7 @@ export function createRouter(dataDir = 'data', demoMode = false): Router {
   function pluginRunTagRoute(pluginName: string, route: string):
       ((tag: string, run: string) => string) {
     const base = pluginRoute(pluginName, route);
-    return (tag, run) => base + queryEncoder({tag, run});
+    return (tag, run) => addParams(base, {tag, run});
   }
   return {
     logdir: () => dataDir + '/logdir',

tensorboard/components/tf_backend/test/backendTests.ts

@@ -16,13 +16,26 @@ limitations under the License.
 import {filterTags, getRuns, getTags, RunToTag, TYPES} from '../backend.js';
 import {RequestManager} from '../requestManager.js';
 import {createRouter, setRouter} from '../router.js';
-import {queryEncoder} from '../urlPathHelpers.js';
+import {addParams} from '../urlPathHelpers.js';
 
 describe('urlPathHelpers', () => {
-  it('queryEncoder works on spaces and parens', () => {
-    const params = {foo: 'something with spaces and (parens)'};
-    const actual = queryEncoder(params);
-    const expected = 'something+with+spaces+and+%28parens%29';
+  it('addParams leaves input untouched when there are no parameters', () => {
+    const actual = addParams('http://foo', {a: undefined, b: undefined});
+    const expected = 'http://foo';
+    chai.assert.equal(actual, expected);
+  });
+  it('addParams adds parameters to a URL without parameters', () => {
+    const actual = addParams(
+      'http://foo',
+      {a: "1", b: ["2", "3+4"], c: "5", d: undefined});
+    const expected = 'http://foo?a=1&b=2&b=3%2B4&c=5';
+    chai.assert.equal(actual, expected);
+  });
+  it('addParams adds parameters to a URL with parameters', () => {
+    const actual = addParams(
+      'http://foo?a=1',
+      {b: ["2", "3+4"], c: "5", d: undefined});
+    const expected = 'http://foo?a=1&b=2&b=3%2B4&c=5';
     chai.assert.equal(actual, expected);
   });
 });

tensorboard/components/tf_backend/urlPathHelpers.ts

@@ -12,17 +12,42 @@ 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.
 ==============================================================================*/
-export function queryEncoder(params?: any): string {
-  // It's important that the keys be sorted, so we always grab the right file
-  // if we are talking to the backend generated by serialze_tensorboard.py
-  if (params == null) {
-    return '';
+/**
+ * A query parameter value can either be a string or a list of strings.
+ * A string `"foo"` is encoded as `key=foo`; a list `["foo", "bar"]` is
+ * encoded as `key=foo&key=bar`.
+ */
+export type QueryValue = string | string[];
+
+/**
+ * Add query parameters to a URL. Values will be URL-encoded. The URL
+ * may or may not already have query parameters. For convenience,
+ * parameters whose value is `undefined` will be dropped.
+ *
+ * For example, the following expressions are equivalent:
+ *
+ *     addParams("http://foo", {a: "1", b: ["2", "3+4"], c: "5"})
+ *     addParams("http://foo?a=1", {b: ["2", "3+4"], c: "5", d: undefined})
+ *     "http://foo?a=1&b=2&b=3%2B4&c=5"
+ */
+export function addParams(
+    baseURL: string,
+    params: {[param: string]: QueryValue}): string {
+  const keys = Object.keys(params).sort().filter(k => params[k] !== undefined);
+  if (!keys.length) {
+    return baseURL;  // no need to change '/foo' to '/foo?'
   }
-  const components = _.keys(params)
-                       .sort()
-                       .filter((k) => params[k] !== undefined)
-                       .map((k) => k + '=' + encodeURIComponent(params[k]));
-  const result = components.length ? '?' + components.join('&') : '';
-  // Replace parens for consistency with urllib.urlencode
-  return result.replace(/\(/g, '%28').replace(/\)/g, '%29');
+  const delimiter = baseURL.indexOf('?') !== -1 ? '&' : '?';
+  const parts = [].concat(...keys.map(key => {
+    const rawValue = params[key];
+    const values = Array.isArray(rawValue) ? rawValue : [rawValue];
+    return values.map(value => `${key}=${_encodeURIComponent(value)}`);
+  }));
+  const query = parts.join('&');
+  return baseURL + delimiter + query;
+}
+
+function _encodeURIComponent(x: string): string {
+  // Replace parentheses for consistency with Python's urllib.urlencode.
+  return encodeURIComponent(x).replace(/\(/g, '%28').replace(/\)/g, '%29');
 }

tensorboard/plugins/graph/tf_graph_dashboard/tf-graph-dashboard.html

@@ -129,7 +129,7 @@ <h3>No graph definition files were found.</h3>
 import {Canceller} from "../tf-backend/canceller.js";
 import {compareTagNames} from "../vz-sorting/sorting.js";
 import {getRouter} from "../tf-backend/router.js";
-import {queryEncoder} from "../tf-backend/urlPathHelpers.js";
+import {addParams} from "../tf-backend/urlPathHelpers.js";
 
 Polymer({
   is: 'tf-graph-dashboard',
@@ -271,7 +271,7 @@ <h3>No graph definition files were found.</h3>
       'large_attrs_key': optional(largeAttrsKey),
     };
     const extension = demoMode ? '.pbtxt' : '';
-    return base + queryEncoder(parameters) + extension;
+    return addParams(base, parameters) + extension;
   },
   _shouldRequestHealthPills: function() {
     // Do not load debugger data if the feature is disabled, if the user toggled off the feature,