diff --git a/tensorboard/plugins/core/core_plugin.py b/tensorboard/plugins/core/core_plugin.py index b1685f6c3a..085fbf86aa 100644 --- a/tensorboard/plugins/core/core_plugin.py +++ b/tensorboard/plugins/core/core_plugin.py @@ -21,6 +21,7 @@ import functools import gzip import mimetypes +import posixpath import zipfile import six @@ -58,6 +59,7 @@ def __init__(self, context): logdir_spec = context.flags.logdir_spec if context.flags else "" self._logdir = context.logdir or logdir_spec self._window_title = context.window_title + self._path_prefix = context.flags.path_prefix if context.flags else None self._assets_zip_provider = context.assets_zip_provider self._data_provider = context.data_provider @@ -91,7 +93,15 @@ def get_resource_apps(self): with self._assets_zip_provider() as fp: with zipfile.ZipFile(fp) as zip_: for path in zip_.namelist(): - gzipped_asset_bytes = _gzip(zip_.read(path)) + content = zip_.read(path) + # Opt out of gzipping index.html + if path == "index.html": + apps["/" + path] = functools.partial( + self._serve_index, content + ) + continue + + gzipped_asset_bytes = _gzip(content) wsgi_app = functools.partial( self._serve_asset, path, gzipped_asset_bytes ) @@ -115,6 +125,31 @@ def _serve_asset(self, path, gzipped_asset_bytes, request): request, gzipped_asset_bytes, mimetype, content_encoding="gzip" ) + @wrappers.Request.application + def _serve_index(self, index_asset_bytes, request): + """Serves index.html content. + + Note that we opt out of gzipping index.html to write preamble before the + resource content. This inflates the resource size from 2x kiB to 1xx + kiB, but we require an ability to flush preamble with the HTML content. + """ + relpath = ( + posixpath.relpath(self._path_prefix, request.script_root) + if self._path_prefix + else "." + ) + meta_header = ( + '' + % relpath + ) + content = meta_header.encode("utf-8") + index_asset_bytes + # By passing content_encoding, disallow gzipping. Bloats the content + # from ~25 kiB to ~120 kiB but reduces CPU usage and avoid 3ms worth of + # gzipping. + return http_util.Respond( + request, content, "text/html", content_encoding="identity" + ) + @wrappers.Request.application def _serve_environment(self, request): """Serve a JSON object containing some base properties used by the diff --git a/tensorboard/plugins/core/core_plugin_test.py b/tensorboard/plugins/core/core_plugin_test.py index 510a57b655..63e997b5a0 100644 --- a/tensorboard/plugins/core/core_plugin_test.py +++ b/tensorboard/plugins/core/core_plugin_test.py @@ -170,7 +170,11 @@ def testIndex_returnsActualHtml(self): self.assertEqual(200, response.status_code) self.assertStartsWith(response.headers.get("Content-Type"), "text/html") html = response.get_data() - self.assertEqual(html, FAKE_INDEX_HTML) + self.assertEqual( + html, + b'' + + FAKE_INDEX_HTML, + ) def testDataPaths_disableAllCaching(self): """Test the format of the /data/runs endpoint.""" @@ -378,6 +382,76 @@ def FirstEventTimestamp_stub(run_name): ) +class CorePluginPathPrefixTest(tf.test.TestCase): + def _send_request(self, path_prefix, pathname): + multiplexer = event_multiplexer.EventMultiplexer() + logdir = self.get_temp_dir() + provider = data_provider.MultiplexerDataProvider(multiplexer, logdir) + context = base_plugin.TBContext( + assets_zip_provider=get_test_assets_zip_provider(), + logdir=logdir, + data_provider=provider, + window_title="", + flags=FakeFlags(path_prefix=path_prefix), + ) + plugin = core_plugin.CorePlugin(context) + app = application.TensorBoardWSGI([plugin], path_prefix=path_prefix) + server = werkzeug_test.Client(app, wrappers.BaseResponse) + return server.get(pathname) + + def _assert_index(self, response, expected_tb_relative_root): + self.assertEqual(200, response.status_code) + self.assertStartsWith(response.headers.get("Content-Type"), "text/html") + html = response.get_data() + + expected_meta = ( + '' + % expected_tb_relative_root + ).encode() + self.assertEqual( + html, expected_meta + FAKE_INDEX_HTML, + ) + + def testIndex_no_path_prefix(self): + self._assert_index(self._send_request("", "/"), "./") + self._assert_index(self._send_request("", "/index.html"), "./") + + def testIndex_path_prefix_foo(self): + self._assert_index(self._send_request("/foo", "/foo/"), "./") + self._assert_index(self._send_request("/foo", "/foo/index.html"), "./") + + def testIndex_path_prefix_foo_exp_route(self): + self._assert_index( + self._send_request("/foo", "/foo/experiment/123/"), "../../" + ) + + def testIndex_path_prefix_foo_incorrect_route(self): + self.assertEqual( + 404, (self._send_request("/foo", "/foo/meow/").status_code) + ) + self.assertEqual(404, (self._send_request("/foo", "/").status_code)) + self.assertEqual( + 404, (self._send_request("/foo", "/index.html").status_code) + ) + + # Missing trailing "/" causes redirection. + self.assertEqual(301, (self._send_request("/foo", "/foo").status_code)) + self.assertEqual( + 301, (self._send_request("/foo", "/foo/experiment/123").status_code) + ) + + def testIndex_path_prefix_foo_bar(self): + self._assert_index(self._send_request("/foo/bar", "/foo/bar/"), "./") + self._assert_index( + self._send_request("/foo/bar", "/foo/bar/index.html"), "./" + ) + + def testIndex_path_prefix_foo_bar_exp_route(self): + self._assert_index( + self._send_request("/foo/bar", "/foo/bar/experiment/123/"), "../../" + ) + + def get_test_assets_zip_provider(): memfile = six.BytesIO() with zipfile.ZipFile( diff --git a/tensorboard/webapp/app_routing/BUILD b/tensorboard/webapp/app_routing/BUILD index 47e35698b8..00336184b8 100644 --- a/tensorboard/webapp/app_routing/BUILD +++ b/tensorboard/webapp/app_routing/BUILD @@ -10,6 +10,7 @@ tf_ng_module( "app_routing_module.ts", ], deps = [ + ":app_root", ":location", ":programmatical_navigation_module", ":route_registry", @@ -36,6 +37,17 @@ tf_ng_module( ], ) +tf_ng_module( + name = "app_root", + srcs = [ + "app_root.ts", + ], + deps = [ + ":location", + "@npm//@angular/core", + ], +) + tf_ng_module( name = "route_registry", srcs = [ @@ -152,6 +164,7 @@ tf_ng_module( name = "app_routing_test", testonly = True, srcs = [ + "app_root_test.ts", "internal_utils_test.ts", "location_test.ts", "programmatical_navigation_module_test.ts", @@ -159,6 +172,7 @@ tf_ng_module( "route_registry_module_test.ts", ], deps = [ + ":app_root", ":internal_utils", ":location", ":programmatical_navigation_module", diff --git a/tensorboard/webapp/app_routing/app_root.ts b/tensorboard/webapp/app_routing/app_root.ts new file mode 100644 index 0000000000..be97930466 --- /dev/null +++ b/tensorboard/webapp/app_routing/app_root.ts @@ -0,0 +1,68 @@ +/* Copyright 2020 The TensorFlow Authors. All Rights Reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +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. +==============================================================================*/ +import {Injectable} from '@angular/core'; + +import {Location} from './location'; + +@Injectable() +export class AppRootProvider { + protected appRoot: string; + + constructor(location: Location) { + this.appRoot = this.getAppRootFromMetaElement(location); + } + + /** + * appRoot path starts with `/` and always end with `/`. + */ + private getAppRootFromMetaElement(location: Location): string { + const metaEl = document.querySelector('head meta[name="tb-relative-root"]'); + + if (!metaEl) return '/'; + const {pathname} = new URL( + (metaEl as HTMLMetaElement).content, + location.getHref() + ); + return pathname.replace(/\/*$/, '/'); + } + + /** + * Returns actual full pathname that includes path prefix of the application. + */ + getAbsPathnameWithAppRoot(absPathname: string): string { + // appRoot has trailing '/'. Remove one so we don't have "//". + return this.appRoot.slice(0, -1) + absPathname; + } + + getAppRootlessPathname(absPathname: string) { + if (absPathname.startsWith(this.appRoot)) { + // appRoot ends with "/" and we need the trimmed pathname to start with "/" since + // routes are defined with starting "/". + return '/' + absPathname.slice(this.appRoot.length); + } + return absPathname; + } +} + +@Injectable() +export class TestableAppRootProvider extends AppRootProvider { + getAppRoot(): string { + return this.appRoot; + } + + setAppRoot(appRoot: string) { + this.appRoot = appRoot; + } +} diff --git a/tensorboard/webapp/app_routing/app_root_test.ts b/tensorboard/webapp/app_routing/app_root_test.ts new file mode 100644 index 0000000000..831c29b355 --- /dev/null +++ b/tensorboard/webapp/app_routing/app_root_test.ts @@ -0,0 +1,99 @@ +/* Copyright 2020 The TensorFlow Authors. All Rights Reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +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. +==============================================================================*/ +import {TestBed} from '@angular/core/testing'; + +import {AppRootProvider, TestableAppRootProvider} from './app_root'; +import {Location} from './location'; + +describe('app root', () => { + let getHrefSpy: jasmine.Spy; + + beforeEach(async () => { + getHrefSpy = jasmine.createSpy(); + await TestBed.configureTestingModule({ + providers: [ + Location, + {provide: AppRootProvider, useClass: TestableAppRootProvider}, + ], + }).compileComponents(); + + const location = TestBed.inject(Location); + getHrefSpy = spyOn(location, 'getHref').and.returnValue('https://tb.dev/'); + }); + + function setUp(href: string, content: string): TestableAppRootProvider { + getHrefSpy.and.returnValue(href); + const meta = document.createElement('meta'); + meta.name = 'tb-relative-root'; + meta.content = content; + document.head.appendChild(meta); + const appRoot = TestBed.inject(AppRootProvider) as TestableAppRootProvider; + document.head.removeChild(meta); + return appRoot; + } + + [ + {href: 'https://tb.dev/', content: './', expectedAppRoot: '/'}, + {href: 'https://tb.dev/index.html', content: './', expectedAppRoot: '/'}, + { + href: 'https://tb.dev/foo/bar/experiment/1/', + content: '../../', + expectedAppRoot: '/foo/bar/', + }, + // wrong relative content but we handle it correctly. + {href: 'https://tb.dev/', content: '../../', expectedAppRoot: '/'}, + {href: 'https://tb.dev/', content: './/', expectedAppRoot: '/'}, + { + href: 'https://tb.dev/experiment/1/', + content: '../..///', + expectedAppRoot: '/', + }, + ].forEach(({content, href, expectedAppRoot}) => { + describe('appRoot parsing', () => { + it(`returns an absolute path from : ${href} and ${content}`, () => { + expect(setUp(href, content).getAppRoot()).toBe(expectedAppRoot); + }); + }); + }); + + describe('#getAbsPathnameWithAppRoot', () => { + it('returns pathname with appRoot', () => { + expect( + setUp( + 'https://tb.dev/foo/bar/experiment/1/', + '../../' + ).getAbsPathnameWithAppRoot('/cool/test') + ).toBe(`/foo/bar/cool/test`); + }); + }); + + describe('#getAppRootlessPathname', () => { + it('returns a path without app root', () => { + const provider = setUp('https://tb.dev/foo/bar/experiment/1/', '../../'); + expect(provider.getAppRootlessPathname('/foo/bar/')).toBe('/'); + expect(provider.getAppRootlessPathname('/foo/bar/baz')).toBe('/baz'); + }); + + it('does not strip if pathname does not start with appRoot', () => { + const provider = setUp('https://tb.dev/foo/bar/experiment/1/', '../../'); + // misses trailing "/" to exactly match the appRoot. + expect(provider.getAppRootlessPathname('/foo/bar')).toBe('/foo/bar'); + expect(provider.getAppRootlessPathname('/bar')).toBe('/bar'); + expect(provider.getAppRootlessPathname('/fan/foo/bar')).toBe( + '/fan/foo/bar' + ); + }); + }); +}); diff --git a/tensorboard/webapp/app_routing/app_routing_module.ts b/tensorboard/webapp/app_routing/app_routing_module.ts index 670204a6e5..969890a5fb 100644 --- a/tensorboard/webapp/app_routing/app_routing_module.ts +++ b/tensorboard/webapp/app_routing/app_routing_module.ts @@ -16,6 +16,7 @@ import {NgModule} from '@angular/core'; import {EffectsModule} from '@ngrx/effects'; import {StoreModule} from '@ngrx/store'; +import {AppRootProvider} from './app_root'; import {AppRoutingEffects} from './effects'; import {LocationModule} from './location_module'; import {ProgrammaticalNavigationModule} from './programmatical_navigation_module'; @@ -28,6 +29,6 @@ import {APP_ROUTING_FEATURE_KEY} from './store/app_routing_types'; EffectsModule.forFeature([AppRoutingEffects]), LocationModule, ], - providers: [ProgrammaticalNavigationModule], + providers: [ProgrammaticalNavigationModule, AppRootProvider], }) export class AppRoutingModule {} diff --git a/tensorboard/webapp/app_routing/effects/BUILD b/tensorboard/webapp/app_routing/effects/BUILD index 0e5588d1db..99b5bf5982 100644 --- a/tensorboard/webapp/app_routing/effects/BUILD +++ b/tensorboard/webapp/app_routing/effects/BUILD @@ -12,6 +12,7 @@ tf_ng_module( ], deps = [ "//tensorboard/webapp:app_state", + "//tensorboard/webapp/app_routing:app_root", "//tensorboard/webapp/app_routing:internal_utils", "//tensorboard/webapp/app_routing:location", "//tensorboard/webapp/app_routing:programmatical_navigation_module", @@ -36,6 +37,7 @@ tf_ng_module( "//tensorboard/webapp:app_state", "//tensorboard/webapp/angular:expect_angular_core_testing", "//tensorboard/webapp/angular:expect_ngrx_store_testing", + "//tensorboard/webapp/app_routing:app_root", "//tensorboard/webapp/app_routing:location", "//tensorboard/webapp/app_routing:programmatical_navigation_module", "//tensorboard/webapp/app_routing:route_registry", diff --git a/tensorboard/webapp/app_routing/effects/app_routing_effects.ts b/tensorboard/webapp/app_routing/effects/app_routing_effects.ts index 831f8bc6ec..5e04408632 100644 --- a/tensorboard/webapp/app_routing/effects/app_routing_effects.ts +++ b/tensorboard/webapp/app_routing/effects/app_routing_effects.ts @@ -12,7 +12,7 @@ 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. ==============================================================================*/ -import {Injectable} from '@angular/core'; +import {Inject, Injectable} from '@angular/core'; import {Actions, createEffect, ofType} from '@ngrx/effects'; import {Action, createAction, Store} from '@ngrx/store'; import {merge, Observable, of} from 'rxjs'; @@ -33,6 +33,7 @@ import { navigationRequested, stateRehydratedFromUrl, } from '../actions'; +import {AppRootProvider} from '../app_root'; import {areRoutesEqual, getRouteId} from '../internal_utils'; import {Location} from '../location'; import {ProgrammaticalNavigationModule} from '../programmatical_navigation_module'; @@ -60,13 +61,20 @@ export class AppRoutingEffects { private readonly store: Store, private readonly location: Location, registry: RouteRegistryModule, - private readonly programmaticalNavModule: ProgrammaticalNavigationModule + private readonly programmaticalNavModule: ProgrammaticalNavigationModule, + private readonly appRootProvider: AppRootProvider ) { this.routeConfigs = registry.getRouteConfigs(); } private readonly onNavigationRequested$ = this.actions$.pipe( - ofType(navigationRequested) + ofType(navigationRequested), + map((navigation) => { + const resolvedPathname = navigation.pathname.startsWith('/') + ? this.appRootProvider.getAbsPathnameWithAppRoot(navigation.pathname) + : this.location.getResolvedPath(navigation.pathname); + return {...navigation, pathname: resolvedPathname}; + }) ); private readonly onInit$: Observable = this.actions$ @@ -83,19 +91,35 @@ export class AppRoutingEffects { }) ); + /** + * Input observable must have absolute pathname with, when appRoot is present, + * appRoot prefixed (e.g., window.location.pathname). + */ private readonly userInitNavRoute$ = merge( this.onNavigationRequested$, this.onInit$, this.location.onPopState().pipe( map((navigation) => { - return {...navigation, browserInitiated: true}; + return { + pathname: navigation.pathname, + replaceState: navigation.replaceState, + browserInitiated: true, + }; }) ) ).pipe( map((navigation) => { + // Expect to have absolute navigation here. + if (!navigation.pathname.startsWith('/')) { + throw new Error( + `[App routing] pathname must start with '/'. Got: ${navigation.pathname}` + ); + } return { ...navigation, - pathname: this.location.getResolvedPath(navigation.pathname), + pathname: this.appRootProvider.getAppRootlessPathname( + navigation.pathname + ), }; }), map((navigationWithAbsolutePath) => { @@ -254,18 +278,24 @@ export class AppRoutingEffects { }), filter(({route}) => { return !areRoutesEqual(route, { - pathname: this.location.getPath(), + pathname: this.appRootProvider.getAppRootlessPathname( + this.location.getPath() + ), queryParams: this.location.getSearch(), }); }), tap(({preserveHash, route}) => { if (route.navigationOptions.replaceState) { this.location.replaceState( - this.location.getFullPathFromRouteOrNav(route, preserveHash) + this.appRootProvider.getAbsPathnameWithAppRoot( + this.location.getFullPathFromRouteOrNav(route, preserveHash) + ) ); } else { this.location.pushState( - this.location.getFullPathFromRouteOrNav(route, preserveHash) + this.appRootProvider.getAbsPathnameWithAppRoot( + this.location.getFullPathFromRouteOrNav(route, preserveHash) + ) ); } }) diff --git a/tensorboard/webapp/app_routing/effects/app_routing_effects_test.ts b/tensorboard/webapp/app_routing/effects/app_routing_effects_test.ts index fffde9a441..7a2941b49d 100644 --- a/tensorboard/webapp/app_routing/effects/app_routing_effects_test.ts +++ b/tensorboard/webapp/app_routing/effects/app_routing_effects_test.ts @@ -22,6 +22,8 @@ import {of, ReplaySubject} from 'rxjs'; import {State} from '../../app_state'; import * as actions from '../actions'; +import {navigationRequested} from '../actions'; +import {AppRootProvider, TestableAppRootProvider} from '../app_root'; import {Location} from '../location'; import { NavigateToExperiments, @@ -46,6 +48,10 @@ describe('app_routing_effects', () => { let location: Location; let actualActions: Action[]; let onPopStateSubject: ReplaySubject; + let pushStateSpy: jasmine.Spy; + let getHashSpy: jasmine.Spy; + let getPathSpy: jasmine.Spy; + let getSearchSpy: jasmine.Spy; let serializeStateToQueryParamsSpy: jasmine.Spy; let deserializeQueryParamsSpy: jasmine.Spy; @@ -67,6 +73,7 @@ describe('app_routing_effects', () => { routeKind: RouteKind.EXPERIMENTS, path: '/experiments', ngComponent: TestableComponent, + defaultRoute: true, }, { routeKind: RouteKind.COMPARE_EXPERIMENT, @@ -104,6 +111,10 @@ describe('app_routing_effects', () => { AppRoutingEffects, provideMockStore(), provideLocationTesting(), + { + provide: AppRootProvider, + useClass: TestableAppRootProvider, + }, ], }).compileComponents(); @@ -114,17 +125,19 @@ describe('app_routing_effects', () => { location = TestBed.inject(TestableLocation) as Location; onPopStateSubject = new ReplaySubject(1); spyOn(location, 'onPopState').and.returnValue(onPopStateSubject); - store.overrideSelector(getActiveRoute, null); + pushStateSpy = spyOn(location, 'pushState'); + getHashSpy = spyOn(location, 'getHash').and.returnValue(''); + getPathSpy = spyOn(location, 'getPath').and.returnValue(''); + getSearchSpy = spyOn(location, 'getSearch').and.returnValue([]); - effects = TestBed.inject(AppRoutingEffects); + store.overrideSelector(getActiveRoute, null); }); describe('fireNavigatedIfValidRoute$', () => { - let getPathSpy: jasmine.Spy; - let getSearchSpy: jasmine.Spy; let actualActions: Action[]; beforeEach(() => { + effects = TestBed.inject(AppRoutingEffects); actualActions = []; spyOn(store, 'dispatch').and.callFake((action: Action) => { @@ -133,9 +146,6 @@ describe('app_routing_effects', () => { effects.fireNavigatedIfValidRoute$.subscribe((action) => { actualActions.push(action); }); - - getPathSpy = spyOn(location, 'getPath'); - getSearchSpy = spyOn(location, 'getSearch'); }); afterEach(fakeAsync(() => { @@ -534,19 +544,11 @@ describe('app_routing_effects', () => { describe('changeBrowserUrl$', () => { let replaceStateSpy: jasmine.Spy; - let pushStateSpy: jasmine.Spy; - let getHashSpy: jasmine.Spy; - let getPathSpy: jasmine.Spy; - let getSearchSpy: jasmine.Spy; beforeEach(() => { + effects = TestBed.inject(AppRoutingEffects); effects.changeBrowserUrl$.subscribe(() => {}); - replaceStateSpy = spyOn(location, 'replaceState'); - pushStateSpy = spyOn(location, 'pushState'); - getHashSpy = spyOn(location, 'getHash'); - getPathSpy = spyOn(location, 'getPath'); - getSearchSpy = spyOn(location, 'getSearch'); }); it('noops if the new route matches current URL', () => { @@ -724,4 +726,152 @@ describe('app_routing_effects', () => { expect(replaceStateSpy).toHaveBeenCalledWith('/experiment'); }); }); + + describe('path_prefix support', () => { + function setAppRootAndSubscribe(appRoot: string) { + const provider = TestBed.inject( + AppRootProvider + ) as TestableAppRootProvider; + provider.setAppRoot(appRoot); + + effects = TestBed.inject(AppRoutingEffects); + const dispatchSpy = spyOn(store, 'dispatch'); + effects.fireNavigatedIfValidRoute$.subscribe((action) => { + actualActions.push(action); + }); + + actualActions = []; + dispatchSpy.and.callFake((action: Action) => { + actualActions.push(action); + }); + + effects.changeBrowserUrl$.subscribe(() => {}); + } + + it('navigates to default route if popstated to path without prefix', fakeAsync(() => { + setAppRootAndSubscribe('/foo/bar/'); + + onPopStateSubject.next({ + pathname: '/meow', + }); + + expect(actualActions).toEqual([ + actions.navigating({ + after: buildRoute({ + routeKind: RouteKind.EXPERIMENTS, + params: {}, + pathname: '/experiments', + queryParams: [], + navigationOptions: { + replaceState: false, + }, + }), + }), + ]); + + tick(); + })); + + it('navigates to a matching route if popstated to path with prefix', fakeAsync(() => { + setAppRootAndSubscribe('/foo/bar/'); + + onPopStateSubject.next({ + pathname: '/foo/bar/experiment/123', + }); + + expect(actualActions).toEqual([ + actions.navigating({ + after: buildRoute({ + routeKind: RouteKind.EXPERIMENT, + params: {experimentId: '123'}, + pathname: '/experiment/123', + queryParams: [], + navigationOptions: { + replaceState: false, + }, + }), + }), + ]); + + tick(); + })); + + it('navigates with appRoot aware path when navRequest with absPath', fakeAsync(() => { + setAppRootAndSubscribe('/foo/bar/'); + + // Do note that this path name does not contain the appRoot. + action.next(navigationRequested({pathname: '/experiment/123'})); + + expect(actualActions).toEqual([ + actions.navigating({ + after: buildRoute({ + routeKind: RouteKind.EXPERIMENT, + params: {experimentId: '123'}, + pathname: '/experiment/123', + queryParams: [], + navigationOptions: { + replaceState: false, + }, + }), + }), + ]); + + tick(); + })); + + it('navigates with appRoot aware path when navRequest with relPath', fakeAsync(() => { + setAppRootAndSubscribe('/foo/bar/'); + + spyOn(location, 'getResolvedPath') + .withArgs('../experiment/123') + .and.returnValue('/foo/bar/experiment/123'); + + // Do note that this path name does not contain the appRoot. + action.next(navigationRequested({pathname: '../experiment/123'})); + + expect(actualActions).toEqual([ + actions.navigating({ + after: buildRoute({ + routeKind: RouteKind.EXPERIMENT, + params: {experimentId: '123'}, + pathname: '/experiment/123', + queryParams: [], + navigationOptions: { + replaceState: false, + }, + }), + }), + ]); + + tick(); + })); + + describe('change url', () => { + it('navigates to URL with path prefix prefixed', fakeAsync(() => { + setAppRootAndSubscribe('/foo/bar/baz/'); + const activeRoute = buildRoute({ + routeKind: RouteKind.EXPERIMENTS, + pathname: '/experiments', + queryParams: [], + navigationOptions: { + replaceState: false, + }, + }); + store.overrideSelector(getActiveRoute, activeRoute); + store.refreshState(); + getHashSpy.and.returnValue(''); + getPathSpy.and.returnValue(''); + getSearchSpy.and.returnValue([]); + + action.next( + actions.navigated({ + before: null, + after: activeRoute, + }) + ); + + expect(pushStateSpy).toHaveBeenCalledWith('/foo/bar/baz/experiments'); + })); + }); + }); }); diff --git a/tensorboard/webapp/app_routing/location.ts b/tensorboard/webapp/app_routing/location.ts index c4e8e70e30..db0cfd84b8 100644 --- a/tensorboard/webapp/app_routing/location.ts +++ b/tensorboard/webapp/app_routing/location.ts @@ -20,6 +20,8 @@ import {createURLSearchParamsFromSerializableQueryParams} from './internal_utils import {Navigation, Route, SerializableQueryParams} from './types'; export interface LocationInterface { + getHref(): string; + getSearch(): SerializableQueryParams; getHash(): string; @@ -54,6 +56,10 @@ function isNavigation( @Injectable() export class Location implements LocationInterface { + getHref(): string { + return utils.getHref(); + } + getSearch(): SerializableQueryParams { const searchParams = new URLSearchParams(window.location.search); const serializableSearchParams: SerializableQueryParams = []; diff --git a/tensorboard/webapp/app_routing/location_test.ts b/tensorboard/webapp/app_routing/location_test.ts index f5e71de026..f463028261 100644 --- a/tensorboard/webapp/app_routing/location_test.ts +++ b/tensorboard/webapp/app_routing/location_test.ts @@ -22,6 +22,15 @@ describe('location', () => { location = new Location(); }); + describe('#getHref', () => { + it('returns href', () => { + spyOn(TEST_ONLY.utils, 'getHref').and.returnValue( + 'https://t.b/is/cool/product' + ); + expect(location.getHref()).toBe('https://t.b/is/cool/product'); + }); + }); + describe('#getResolvedPath', () => { it('forms absolute path from current href', () => { spyOn(TEST_ONLY.utils, 'getHref').and.returnValue( diff --git a/tensorboard/webapp/app_routing/views/BUILD b/tensorboard/webapp/app_routing/views/BUILD index 3c27bce166..22e2339313 100644 --- a/tensorboard/webapp/app_routing/views/BUILD +++ b/tensorboard/webapp/app_routing/views/BUILD @@ -14,6 +14,7 @@ tf_ng_module( ], deps = [ "//tensorboard/webapp:app_state", + "//tensorboard/webapp/app_routing:app_root", "//tensorboard/webapp/app_routing:internal_utils", "//tensorboard/webapp/app_routing:location", "//tensorboard/webapp/app_routing:route_registry", @@ -41,6 +42,7 @@ tf_ts_library( "//tensorboard/webapp/angular:expect_angular_platform_browser_animations", "//tensorboard/webapp/angular:expect_angular_platform_browser_dynamic_testing", "//tensorboard/webapp/angular:expect_ngrx_store_testing", + "//tensorboard/webapp/app_routing:app_root", "//tensorboard/webapp/app_routing:location", "//tensorboard/webapp/app_routing:route_registry", "//tensorboard/webapp/app_routing:testing", diff --git a/tensorboard/webapp/app_routing/views/app_routing_view_module.ts b/tensorboard/webapp/app_routing/views/app_routing_view_module.ts index 2ce4bd2cc4..d12f25e7af 100644 --- a/tensorboard/webapp/app_routing/views/app_routing_view_module.ts +++ b/tensorboard/webapp/app_routing/views/app_routing_view_module.ts @@ -21,6 +21,7 @@ import {RouterLinkDirectiveContainer} from './router_link_directive_container'; import {RouterOutletComponent} from './router_outlet_component'; import {RouterOutletContainer} from './router_outlet_container'; import {LocationModule} from '../location_module'; +import {AppRootProvider} from '../app_root'; @NgModule({ imports: [CommonModule, LocationModule, RouteRegistryModule], @@ -30,5 +31,6 @@ import {LocationModule} from '../location_module'; RouterOutletComponent, RouterLinkDirectiveContainer, ], + providers: [AppRootProvider], }) export class AppRoutingViewModule {} diff --git a/tensorboard/webapp/app_routing/views/router_link_directive_container.ts b/tensorboard/webapp/app_routing/views/router_link_directive_container.ts index 2dab0d9fe9..7f13f95a45 100644 --- a/tensorboard/webapp/app_routing/views/router_link_directive_container.ts +++ b/tensorboard/webapp/app_routing/views/router_link_directive_container.ts @@ -12,11 +12,18 @@ 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. ==============================================================================*/ -import {Directive, HostBinding, HostListener, Input} from '@angular/core'; +import { + Directive, + HostBinding, + HostListener, + Inject, + Input, +} from '@angular/core'; import {Store} from '@ngrx/store'; import {State} from '../../app_state'; import {navigationRequested} from '../actions'; +import {AppRootProvider} from '../app_root'; import {Location} from '../location'; @Directive({selector: 'a[routerLink]'}) @@ -25,7 +32,8 @@ export class RouterLinkDirectiveContainer { constructor( private readonly store: Store, - private readonly location: Location + private readonly location: Location, + private readonly appRootProvider: AppRootProvider ) {} @HostListener('click', ['$event']) @@ -46,10 +54,11 @@ export class RouterLinkDirectiveContainer { @HostBinding('attr.href') get href() { if (!this.pathname) return null; - - return this.location.getFullPathFromRouteOrNav({ - pathname: this.pathname, - }); + return this.appRootProvider.getAbsPathnameWithAppRoot( + this.location.getFullPathFromRouteOrNav({ + pathname: this.pathname, + }) + ); } /** diff --git a/tensorboard/webapp/app_routing/views/router_link_test.ts b/tensorboard/webapp/app_routing/views/router_link_test.ts index 5f26b0561d..2547aa133f 100644 --- a/tensorboard/webapp/app_routing/views/router_link_test.ts +++ b/tensorboard/webapp/app_routing/views/router_link_test.ts @@ -22,6 +22,7 @@ import {MockStore, provideMockStore} from '@ngrx/store/testing'; import {State} from '../../app_state'; import {navigationRequested} from '../actions'; +import {AppRootProvider, TestableAppRootProvider} from '../app_root'; import {LocationModule} from '../location_module'; import {RouterLinkDirectiveContainer} from './router_link_directive_container'; @@ -36,17 +37,26 @@ class TestableComponent { describe('router_link', () => { let actualDispatches: Action[]; + let appRootProvider: TestableAppRootProvider; beforeEach(async () => { actualDispatches = []; + await TestBed.configureTestingModule({ imports: [LocationModule, NoopAnimationsModule], - providers: [provideMockStore()], + providers: [ + provideMockStore(), + {provide: AppRootProvider, useClass: TestableAppRootProvider}, + ], declarations: [RouterLinkDirectiveContainer, TestableComponent], schemas: [NO_ERRORS_SCHEMA], }).compileComponents(); const store = TestBed.inject>(Store) as MockStore; + appRootProvider = TestBed.inject( + AppRootProvider + ) as TestableAppRootProvider; + spyOn(store, 'dispatch').and.callFake((action: Action) => { actualDispatches.push(action); }); @@ -72,6 +82,18 @@ describe('router_link', () => { expect(anchorArr.attributes['href']).toBe('/foobar/baz/'); }); + it('renders the path as href with appRoot to support path_prefix', () => { + appRootProvider.setAppRoot('/qaz/quz/'); + const anchorStr = createComponentAndGetAnchorDebugElement('/foobar'); + expect(anchorStr.attributes['href']).toBe('/qaz/quz/foobar/'); + + const anchorArr = createComponentAndGetAnchorDebugElement([ + '/foobar', + 'baz', + ]); + expect(anchorArr.attributes['href']).toBe('/qaz/quz/foobar/baz/'); + }); + it('dispatches navigate when clicked on the anchor', () => { const link = createComponentAndGetAnchorDebugElement('/foobar'); const event = new MouseEvent('click'); @@ -84,6 +106,19 @@ describe('router_link', () => { ]); }); + it('dispatches programmatical navigation without appRoot', () => { + appRootProvider.setAppRoot('/qaz/quz/'); + const link = createComponentAndGetAnchorDebugElement('../foobar'); + const event = new MouseEvent('click'); + link.triggerEventHandler('click', event); + + expect(actualDispatches).toEqual([ + navigationRequested({ + pathname: '../foobar/', + }), + ]); + }); + it('supports relative path (..)', () => { const link = createComponentAndGetAnchorDebugElement('../foobar'); const event = new MouseEvent('click');