From c45ce9dc028534b2838de26a88ea0793ccaac157 Mon Sep 17 00:00:00 2001 From: Stephan Lee Date: Tue, 30 Oct 2018 08:41:40 -0700 Subject: [PATCH 1/2] Fix bug when data dir contains leading slash When it contains leading slash, it creates url like "//data/..." which is interpretted as an absolute url. --- tensorboard/components/tf_backend/router.ts | 4 +-- .../tf_backend/test/backendTests.ts | 28 +++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/tensorboard/components/tf_backend/router.ts b/tensorboard/components/tf_backend/router.ts index 2f14631852..24a7a00f8e 100644 --- a/tensorboard/components/tf_backend/router.ts +++ b/tensorboard/components/tf_backend/router.ts @@ -88,8 +88,8 @@ export function setRouter(router: Router): void { function createProdPath(pathPrefix: string, path: string, ext: string, params?: URLSearchParams): string { - - const url = new URL(`${window.location.origin}/${pathPrefix}${path}`); + const url = new URL(`${window.location.origin}`); + url.pathname = pathPrefix + path; if (params) url.search = params.toString(); return url.pathname + url.search; } diff --git a/tensorboard/components/tf_backend/test/backendTests.ts b/tensorboard/components/tf_backend/test/backendTests.ts index 52652f2567..dfc5796032 100644 --- a/tensorboard/components/tf_backend/test/backendTests.ts +++ b/tensorboard/components/tf_backend/test/backendTests.ts @@ -105,6 +105,20 @@ describe('backend tests', () => { '/data/plugin/scalars/scalar'); }); + it('ignores leading slash in dataDir', function() { + this.router = createRouter('/data/', /*demoMode=*/false); + assert.equal( + this.router.pluginRoute('scalars', '/scalar'), + '/data/plugin/scalars/scalar'); + }); + + it('does not strip leading slash if dataDir has more than one', () => { + const router = createRouter('////data/', /*demoMode=*/false); + assert.equal( + router.pluginRoute('scalars', '/scalar'), + '////data/plugin/scalars/scalar'); + }); + it('encodes query param correctly', function() { assert.equal( this.router.pluginRoute( @@ -170,6 +184,20 @@ describe('backend tests', () => { this.router = createRouter(base, /*demoMode=*/true); }); + it('ignores leading slash in dataDir', () => { + const router = createRouter('/data/', /*demoMode=*/false); + assert.equal( + router.pluginRoute('scalars', '/scalar'), + '/data/plugin/scalars/scalar'); + }); + + it('does not strip leading slash if dataDir has more than one', () => { + const router = createRouter('////data/', /*demoMode=*/false); + assert.equal( + router.pluginRoute('scalars', '/scalar'), + '////data/plugin/scalars/scalar'); + }); + it('returns correct value for #environment', function() { assert.equal(this.router.environment(), '/data/environment.json'); }); From 23c99c0855f2e4f92848fe84030db69ec7d9e7e2 Mon Sep 17 00:00:00 2001 From: Stephan Lee Date: Tue, 30 Oct 2018 15:57:45 -0700 Subject: [PATCH 2/2] CR address --- tensorboard/components/tf_backend/router.ts | 27 +++++----- .../tf_backend/test/backendTests.ts | 50 ++++++++++--------- 2 files changed, 40 insertions(+), 37 deletions(-) diff --git a/tensorboard/components/tf_backend/router.ts b/tensorboard/components/tf_backend/router.ts index 24a7a00f8e..1619024565 100644 --- a/tensorboard/components/tf_backend/router.ts +++ b/tensorboard/components/tf_backend/router.ts @@ -28,33 +28,33 @@ export interface Router { * Create a router for communicating with the TensorBoard backend. You * can pass this to `setRouter` to make it the global router. * - * @param dataDir {string} The base prefix for finding data on server. - * @param demoMode {boolean} Whether to modify urls for filesystem demo usage. + * @param pathPrefix {string=} The base prefix for data endpoints. + * @param demoMode {boolean=} Whether to modify urls for filesystem demo usage. */ -export function createRouter(dataDir = 'data', demoMode = false): Router { - if (dataDir[dataDir.length - 1] === '/') { - dataDir = dataDir.slice(0, dataDir.length - 1); +export function createRouter(pathPrefix = 'data', demoMode = false): Router { + if (pathPrefix[pathPrefix.length - 1] === '/') { + pathPrefix = pathPrefix.slice(0, pathPrefix.length - 1); } const createPath = demoMode ? createDemoPath : createProdPath; const ext = demoMode ? '.json' : ''; return { - environment: () => createPath(dataDir, '/environment', ext), - experiments: () => createPath(dataDir, '/experiments', ext), + environment: () => createPath(pathPrefix, '/environment', ext), + experiments: () => createPath(pathPrefix, '/experiments', ext), isDemoMode: () => demoMode, pluginRoute: (pluginName: string, route: string, params?: URLSearchParams, demoCustomExt = ext): string => { return createPath( - demoMode ? dataDir : dataDir + '/plugin', + demoMode ? pathPrefix : pathPrefix + '/plugin', `/${pluginName}${route}`, demoCustomExt, params); }, - pluginsListing: () => createPath(dataDir, '/plugins_listing', ext), - runs: () => createPath(dataDir, '/runs', ext), + pluginsListing: () => createPath(pathPrefix, '/plugins_listing', ext), + runs: () => createPath(pathPrefix, '/runs', ext), runsForExperiment: id => { return createPath( - dataDir, + pathPrefix, '/experiment_runs', ext, createSearchParam({experiment: String(id)})); @@ -88,7 +88,8 @@ export function setRouter(router: Router): void { function createProdPath(pathPrefix: string, path: string, ext: string, params?: URLSearchParams): string { - const url = new URL(`${window.location.origin}`); + const url = new URL(window.location.origin); + // Use URL to normalize pathPrefix with leading slash and without. url.pathname = pathPrefix + path; if (params) url.search = params.toString(); return url.pathname + url.search; @@ -116,7 +117,7 @@ function createDemoPath(pathPrefix: string, path: string, normalizedPath = normalizedPath.replace(/\//g, '_'); // Add query parameter as path if it is present. if (encodedQueryParam) normalizedPath += `_${encodedQueryParam}`; - const url = new URL(`${window.location.origin}`); + const url = new URL(window.location.origin); // All demo data are serialized in JSON format. url.pathname = `${pathPrefix}/${normalizedPath}${ext}`; diff --git a/tensorboard/components/tf_backend/test/backendTests.ts b/tensorboard/components/tf_backend/test/backendTests.ts index dfc5796032..6dea12e15c 100644 --- a/tensorboard/components/tf_backend/test/backendTests.ts +++ b/tensorboard/components/tf_backend/test/backendTests.ts @@ -86,6 +86,21 @@ describe('backend tests', () => { this.router = createRouter(base, /*demoMode=*/false); }); + it('ignores leading slash in pathPrefix', () => { + const router = createRouter('/data/', /*demoMode=*/false); + assert.equal(router.runs(), '/data/runs'); + }); + + it('returns leading slash when pathPrefix omits one', () => { + const router = createRouter('data/', /*demoMode=*/false); + assert.equal(router.runs(), '/data/runs'); + }); + + it('does not strip leading slash if pathPrefix has more than one', () => { + const router = createRouter('///data/', /*demoMode=*/false); + assert.equal(router.runs(), '///data/runs'); + }); + it('returns correct value for #environment', function() { assert.equal(this.router.environment(), '/data/environment'); }); @@ -105,20 +120,6 @@ describe('backend tests', () => { '/data/plugin/scalars/scalar'); }); - it('ignores leading slash in dataDir', function() { - this.router = createRouter('/data/', /*demoMode=*/false); - assert.equal( - this.router.pluginRoute('scalars', '/scalar'), - '/data/plugin/scalars/scalar'); - }); - - it('does not strip leading slash if dataDir has more than one', () => { - const router = createRouter('////data/', /*demoMode=*/false); - assert.equal( - router.pluginRoute('scalars', '/scalar'), - '////data/plugin/scalars/scalar'); - }); - it('encodes query param correctly', function() { assert.equal( this.router.pluginRoute( @@ -184,18 +185,19 @@ describe('backend tests', () => { this.router = createRouter(base, /*demoMode=*/true); }); - it('ignores leading slash in dataDir', () => { - const router = createRouter('/data/', /*demoMode=*/false); - assert.equal( - router.pluginRoute('scalars', '/scalar'), - '/data/plugin/scalars/scalar'); + it('ignores leading slash in pathPrefix', () => { + const router = createRouter('/data/', /*demoMode=*/true); + assert.equal(router.runs(), '/data/runs.json'); }); - it('does not strip leading slash if dataDir has more than one', () => { - const router = createRouter('////data/', /*demoMode=*/false); - assert.equal( - router.pluginRoute('scalars', '/scalar'), - '////data/plugin/scalars/scalar'); + it('returns leading slash when pathPrefix omits one', () => { + const router = createRouter('data/', /*demoMode=*/true); + assert.equal(router.runs(), '/data/runs.json'); + }); + + it('does not strip leading slash if pathPrefix has more than one', () => { + const router = createRouter('///data/', /*demoMode=*/true); + assert.equal(router.runs(), '///data/runs.json'); }); it('returns correct value for #environment', function() {