From 081f95c812ab194dadad8e549c93cbbe9cc945f8 Mon Sep 17 00:00:00 2001 From: Jason Aden Date: Tue, 16 Oct 2018 19:50:48 -0700 Subject: [PATCH] feat(router): allow guards to return UrlTree as well as boolean (#26521) * Removed `andObservable` helper function in favor of inline implementation * Flow `boolean | UrlTree` through guards check * Add tests to verify behavior of `checkGuards` function flowing `UrlTree` properly PR Close #26521 --- packages/router/src/apply_redirects.ts | 6 +- packages/router/src/operators/check_guards.ts | 119 +++++++------- packages/router/src/router.ts | 2 +- packages/router/src/utils/collection.ts | 10 +- packages/router/src/utils/type_guards.ts | 5 +- packages/router/test/helpers.ts | 2 +- packages/router/test/router.spec.ts | 146 ++++++++++++++++-- 7 files changed, 213 insertions(+), 77 deletions(-) diff --git a/packages/router/src/apply_redirects.ts b/packages/router/src/apply_redirects.ts index 905a846c7d..8450f6463a 100644 --- a/packages/router/src/apply_redirects.ts +++ b/packages/router/src/apply_redirects.ts @@ -8,14 +8,14 @@ import {Injector, NgModuleRef} from '@angular/core'; import {EmptyError, Observable, Observer, from, of } from 'rxjs'; -import {catchError, concatAll, first, map, mergeMap} from 'rxjs/operators'; +import {catchError, concatAll, every, first, map, mergeMap} from 'rxjs/operators'; import {LoadedRouterConfig, Route, Routes} from './config'; import {CanLoadFn} from './interfaces'; import {RouterConfigLoader} from './router_config_loader'; import {PRIMARY_OUTLET, Params, defaultUrlMatcher, navigationCancelingError} from './shared'; import {UrlSegment, UrlSegmentGroup, UrlSerializer, UrlTree} from './url_tree'; -import {andObservables, forEach, waitForMap, wrapIntoObservable} from './utils/collection'; +import {forEach, waitForMap, wrapIntoObservable} from './utils/collection'; import {isCanLoad, isFunction} from './utils/type_guards'; class NoMatch { @@ -421,7 +421,7 @@ function runCanLoadGuard( return wrapIntoObservable(guardVal); })); - return andObservables(obs); + return obs.pipe(concatAll(), every(result => result === true)); } function match(segmentGroup: UrlSegmentGroup, route: Route, segments: UrlSegment[]): { diff --git a/packages/router/src/operators/check_guards.ts b/packages/router/src/operators/check_guards.ts index 937cccf5f7..a8e80212c0 100644 --- a/packages/router/src/operators/check_guards.ts +++ b/packages/router/src/operators/check_guards.ts @@ -7,17 +7,19 @@ */ import {Injector} from '@angular/core'; -import {MonoTypeOperatorFunction, Observable, from, of } from 'rxjs'; -import {concatMap, every, first, map, mergeMap} from 'rxjs/operators'; +import {MonoTypeOperatorFunction, Observable, defer, from, of } from 'rxjs'; +import {concatAll, concatMap, first, map, mergeMap} from 'rxjs/operators'; import {ActivationStart, ChildActivationStart, Event} from '../events'; import {CanActivateChildFn, CanActivateFn, CanDeactivateFn} from '../interfaces'; import {NavigationTransition} from '../router'; import {ActivatedRouteSnapshot, RouterStateSnapshot} from '../router_state'; import {UrlTree} from '../url_tree'; -import {andObservables, wrapIntoObservable} from '../utils/collection'; +import {wrapIntoObservable} from '../utils/collection'; import {CanActivate, CanDeactivate, getCanActivateChild, getToken} from '../utils/preactivation'; -import {isCanActivate, isCanActivateChild, isCanDeactivate, isFunction} from '../utils/type_guards'; +import {isCanActivate, isCanActivateChild, isCanDeactivate, isFunction, isBoolean} from '../utils/type_guards'; + +import {prioritizedGuardValue} from './prioritized_guard_value'; export function checkGuards(moduleInjector: Injector, forwardEvent?: (evt: Event) => void): MonoTypeOperatorFunction { @@ -33,10 +35,10 @@ export function checkGuards(moduleInjector: Injector, forwardEvent?: (evt: Event canDeactivateChecks, targetSnapshot !, currentSnapshot, moduleInjector) .pipe( mergeMap(canDeactivate => { - return canDeactivate ? + return canDeactivate && isBoolean(canDeactivate) ? runCanActivateChecks( targetSnapshot !, canActivateChecks, moduleInjector, forwardEvent) : - of (false); + of (canDeactivate); }), map(guardsResult => ({...t, guardsResult}))); })); @@ -45,25 +47,30 @@ export function checkGuards(moduleInjector: Injector, forwardEvent?: (evt: Event function runCanDeactivateChecks( checks: CanDeactivate[], futureRSS: RouterStateSnapshot, currRSS: RouterStateSnapshot, - moduleInjector: Injector): Observable { + moduleInjector: Injector) { return from(checks).pipe( - mergeMap(check => { - return runCanDeactivate(check.component, check.route, currRSS, futureRSS, moduleInjector); - }), - every(result => result === true)); + mergeMap( + check => + runCanDeactivate(check.component, check.route, currRSS, futureRSS, moduleInjector)), + first(result => { return result !== true; }, true as boolean | UrlTree)); } function runCanActivateChecks( futureSnapshot: RouterStateSnapshot, checks: CanActivate[], moduleInjector: Injector, - forwardEvent?: (evt: Event) => void): Observable { + forwardEvent?: (evt: Event) => void) { return from(checks).pipe( - concatMap((check: CanActivate) => andObservables(from([ - fireChildActivationStart(check.route.parent, forwardEvent), - fireActivationStart(check.route, forwardEvent), - runCanActivateChild(futureSnapshot, check.path, moduleInjector), - runCanActivate(futureSnapshot, check.route, moduleInjector) - ]))), - every((result: boolean) => result === true)); + concatMap((check: CanActivate) => { + return from([ + fireChildActivationStart(check.route.parent, forwardEvent), + fireActivationStart(check.route, forwardEvent), + runCanActivateChild(futureSnapshot, check.path, moduleInjector), + runCanActivate(futureSnapshot, check.route, moduleInjector) + ]) + .pipe(concatAll(), first(result => { + return result !== true; + }, true as boolean | UrlTree)); + }), + first(result => { return result !== true; }, true as boolean | UrlTree)); } /** @@ -102,27 +109,30 @@ function fireChildActivationStart( function runCanActivate( futureRSS: RouterStateSnapshot, futureARS: ActivatedRouteSnapshot, - moduleInjector: Injector): Observable { + moduleInjector: Injector): Observable { const canActivate = futureARS.routeConfig ? futureARS.routeConfig.canActivate : null; if (!canActivate || canActivate.length === 0) return of (true); - const obs = from(canActivate).pipe(map((c: any) => { - const guard = getToken(c, futureARS, moduleInjector); - let observable; - if (isCanActivate(guard)) { - observable = wrapIntoObservable(guard.canActivate(futureARS, futureRSS)); - } else if (isFunction(guard)) { - observable = wrapIntoObservable(guard(futureARS, futureRSS)); - } else { - throw new Error('Invalid canActivate guard'); - } - return observable.pipe(first()); - })); - return andObservables(obs); + + const canActivateObservables = canActivate.map((c: any) => { + return defer(() => { + const guard = getToken(c, futureARS, moduleInjector); + let observable; + if (isCanActivate(guard)) { + observable = wrapIntoObservable(guard.canActivate(futureARS, futureRSS)); + } else if (isFunction(guard)) { + observable = wrapIntoObservable(guard(futureARS, futureRSS)); + } else { + throw new Error('Invalid CanActivate guard'); + } + return observable.pipe(first()); + }); + }); + return of (canActivateObservables).pipe(prioritizedGuardValue()); } function runCanActivateChild( futureRSS: RouterStateSnapshot, path: ActivatedRouteSnapshot[], - moduleInjector: Injector): Observable { + moduleInjector: Injector): Observable { const futureARS = path[path.length - 1]; const canActivateChildGuards = path.slice(0, path.length - 1) @@ -130,29 +140,32 @@ function runCanActivateChild( .map(p => getCanActivateChild(p)) .filter(_ => _ !== null); - return andObservables(from(canActivateChildGuards).pipe(map((d: any) => { - const obs = from(d.guards).pipe(map((c: any) => { - const guard = getToken(c, d.node, moduleInjector); - let observable; - if (isCanActivateChild(guard)) { - observable = wrapIntoObservable(guard.canActivateChild(futureARS, futureRSS)); - } else if (isFunction(guard)) { - observable = wrapIntoObservable(guard(futureARS, futureRSS)); - } else { - throw new Error('Invalid CanActivateChild guard'); - } - return observable.pipe(first()); - })); - return andObservables(obs); - }))); + const canActivateChildGuardsMapped = canActivateChildGuards.map((d: any) => { + return defer(() => { + const guardsMapped = d.guards.map((c: any) => { + const guard = getToken(c, d.node, moduleInjector); + let observable; + if (isCanActivateChild(guard)) { + observable = wrapIntoObservable(guard.canActivateChild(futureARS, futureRSS)); + } else if (isFunction(guard)) { + observable = wrapIntoObservable(guard(futureARS, futureRSS)); + } else { + throw new Error('Invalid CanActivateChild guard'); + } + return observable.pipe(first()); + }); + return of (guardsMapped).pipe(prioritizedGuardValue()); + }); + }); + return of (canActivateChildGuardsMapped).pipe(prioritizedGuardValue()); } function runCanDeactivate( - component: Object| null, currARS: ActivatedRouteSnapshot, currRSS: RouterStateSnapshot, + component: Object | null, currARS: ActivatedRouteSnapshot, currRSS: RouterStateSnapshot, futureRSS: RouterStateSnapshot, moduleInjector: Injector): Observable { const canDeactivate = currARS && currARS.routeConfig ? currARS.routeConfig.canDeactivate : null; if (!canDeactivate || canDeactivate.length === 0) return of (true); - const canDeactivate$ = from(canDeactivate).pipe(mergeMap((c: any) => { + const canDeactivateObservables = canDeactivate.map((c: any) => { const guard = getToken(c, currARS, moduleInjector); let observable; if (isCanDeactivate(guard)) { @@ -164,6 +177,6 @@ function runCanDeactivate( throw new Error('Invalid CanDeactivate guard'); } return observable.pipe(first()); - })); - return canDeactivate$.pipe(every((result: any) => result === true)); + }); + return of (canDeactivateObservables).pipe(prioritizedGuardValue()); } diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index cfcb5309b3..8e96990f34 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -186,7 +186,7 @@ export type NavigationTransition = { currentRouterState: RouterState, targetRouterState: RouterState | null, guards: Checks, - guardsResult: boolean | null, + guardsResult: boolean | UrlTree | null, }; /** diff --git a/packages/router/src/utils/collection.ts b/packages/router/src/utils/collection.ts index 901d2ed851..6834cec4cf 100644 --- a/packages/router/src/utils/collection.ts +++ b/packages/router/src/utils/collection.ts @@ -8,7 +8,7 @@ import {NgModuleFactory, ɵisObservable as isObservable, ɵisPromise as isPromise} from '@angular/core'; import {Observable, from, of } from 'rxjs'; -import {concatAll, every, last as lastValue, map, mergeAll} from 'rxjs/operators'; +import {concatAll, last as lastValue, map} from 'rxjs/operators'; import {PRIMARY_OUTLET} from '../shared'; @@ -88,14 +88,6 @@ export function waitForMap( return of .apply(null, waitHead.concat(waitTail)).pipe(concatAll(), lastValue(), map(() => res)); } -/** - * ANDs Observables by merging all input observables, reducing to an Observable verifying all - * input Observables return `true`. - */ -export function andObservables(observables: Observable>): Observable { - return observables.pipe(mergeAll(), every((result: any) => result === true)); -} - export function wrapIntoObservable(value: T | NgModuleFactory| Promise| Observable) { if (isObservable(value)) { return value; diff --git a/packages/router/src/utils/type_guards.ts b/packages/router/src/utils/type_guards.ts index befda967be..b6a29bcc80 100644 --- a/packages/router/src/utils/type_guards.ts +++ b/packages/router/src/utils/type_guards.ts @@ -6,7 +6,6 @@ * found in the LICENSE file at https://angular.io/license */ -import {Type} from '@angular/core'; import {CanActivate, CanActivateChild, CanDeactivate, CanLoad} from '../interfaces'; /** @@ -26,6 +25,10 @@ export function isFunction(v: any): v is T { return typeof v === 'function'; } +export function isBoolean(v: any): v is boolean { + return typeof v === 'boolean'; +} + export function isCanLoad(guard: any): guard is CanLoad { return guard && isFunction(guard.canLoad); } diff --git a/packages/router/test/helpers.ts b/packages/router/test/helpers.ts index 674a1db8d3..f7a57ac25b 100644 --- a/packages/router/test/helpers.ts +++ b/packages/router/test/helpers.ts @@ -19,7 +19,7 @@ export class Logger { empty() { this.logs.length = 0; } } -export function provideTokenLogger(token: string, returnValue = true) { +export function provideTokenLogger(token: string, returnValue = true as boolean | UrlTree) { return { provide: token, useFactory: (logger: Logger) => () => (logger.add(token), returnValue), diff --git a/packages/router/test/router.spec.ts b/packages/router/test/router.spec.ts index c39c39e0a8..926c10aa0a 100644 --- a/packages/router/test/router.spec.ts +++ b/packages/router/test/router.spec.ts @@ -17,7 +17,7 @@ import {resolveData as resolveDataOperator} from '../src/operators/resolve_data' import {NavigationTransition, Router} from '../src/router'; import {ChildrenOutletContexts} from '../src/router_outlet_context'; import {RouterStateSnapshot, createEmptyStateSnapshot} from '../src/router_state'; -import {DefaultUrlSerializer} from '../src/url_tree'; +import {DefaultUrlSerializer, UrlTree} from '../src/url_tree'; import {getAllRouteGuards} from '../src/utils/preactivation'; import {TreeNode} from '../src/utils/tree'; import {RouterTestingModule} from '../testing/src/router_testing_module'; @@ -103,23 +103,38 @@ describe('Router', () => { const CA_CHILD = 'canActivate_child'; const CA_CHILD_FALSE = 'canActivate_child_false'; + const CA_CHILD_REDIRECT = 'canActivate_child_redirect'; const CAC_CHILD = 'canActivateChild_child'; const CAC_CHILD_FALSE = 'canActivateChild_child_false'; + const CAC_CHILD_REDIRECT = 'canActivateChild_child_redirect'; const CA_GRANDCHILD = 'canActivate_grandchild'; const CA_GRANDCHILD_FALSE = 'canActivate_grandchild_false'; + const CA_GRANDCHILD_REDIRECT = 'canActivate_grandchild_redirect'; const CDA_CHILD = 'canDeactivate_child'; const CDA_CHILD_FALSE = 'canDeactivate_child_false'; + const CDA_CHILD_REDIRECT = 'canDeactivate_child_redirect'; const CDA_GRANDCHILD = 'canDeactivate_grandchild'; const CDA_GRANDCHILD_FALSE = 'canDeactivate_grandchild_false'; + const CDA_GRANDCHILD_REDIRECT = 'canDeactivate_grandchild_redirect'; beforeEach(() => { + TestBed.configureTestingModule({ + imports: [RouterTestingModule], providers: [ Logger, provideTokenLogger(CA_CHILD), provideTokenLogger(CA_CHILD_FALSE, false), + provideTokenLogger(CA_CHILD_REDIRECT, serializer.parse('/canActivate_child_redirect')), provideTokenLogger(CAC_CHILD), provideTokenLogger(CAC_CHILD_FALSE, false), + provideTokenLogger( + CAC_CHILD_REDIRECT, serializer.parse('/canActivateChild_child_redirect')), provideTokenLogger(CA_GRANDCHILD), provideTokenLogger(CA_GRANDCHILD_FALSE, false), + provideTokenLogger( + CA_GRANDCHILD_REDIRECT, serializer.parse('/canActivate_grandchild_redirect')), provideTokenLogger(CDA_CHILD), provideTokenLogger(CDA_CHILD_FALSE, false), - provideTokenLogger(CDA_GRANDCHILD), provideTokenLogger(CDA_GRANDCHILD_FALSE, false) + provideTokenLogger(CDA_CHILD_REDIRECT, serializer.parse('/canDeactivate_child_redirect')), + provideTokenLogger(CDA_GRANDCHILD), provideTokenLogger(CDA_GRANDCHILD_FALSE, false), + provideTokenLogger( + CDA_GRANDCHILD_REDIRECT, serializer.parse('/canDeactivate_grandchild_redirect')) ] }); @@ -389,11 +404,11 @@ describe('Router', () => { it('should not run activate if deactivate fails guards', () => { /** - * R --> R - * / \ - * prev (CDA) child (CA) - * \ - * grandchild (CA) + * R --> R + * / \ + * prev (CDA: x) child (CA) + * \ + * grandchild (CA) */ const prevSnapshot = createActivatedRouteSnapshot( @@ -460,6 +475,114 @@ describe('Router', () => { expect(logger.logs).toEqual([]); }); }); + + describe('UrlTree', () => { + it('should allow return of UrlTree from CanActivate', () => { + /** + * R --> R + * \ + * child (CA: redirect) + */ + + const childSnapshot = createActivatedRouteSnapshot({ + component: 'child', + routeConfig: { + + canActivate: [CA_CHILD_REDIRECT] + } + }); + + const futureState = new (RouterStateSnapshot as any)( + 'url', new TreeNode(empty.root, [new TreeNode(childSnapshot, [])])); + + checkGuards(futureState, empty, TestBed, (result) => { + expect(serializer.serialize(result as UrlTree)).toBe('/' + CA_CHILD_REDIRECT); + expect(logger.logs).toEqual([CA_CHILD_REDIRECT]); + }); + }); + + it('should allow return of UrlTree from CanActivateChild', () => { + /** + * R --> R + * \ + * child (CAC: redirect) + * \ + * grandchild (CA) + */ + + const childSnapshot = createActivatedRouteSnapshot( + {component: 'child', routeConfig: {canActivateChild: [CAC_CHILD_REDIRECT]}}); + const grandchildSnapshot = createActivatedRouteSnapshot( + {component: 'grandchild', routeConfig: {canActivate: [CA_GRANDCHILD]}}); + + const futureState = new (RouterStateSnapshot as any)( + 'url', new TreeNode( + empty.root, + [new TreeNode(childSnapshot, [new TreeNode(grandchildSnapshot, [])])])); + + checkGuards(futureState, empty, TestBed, (result) => { + expect(serializer.serialize(result as UrlTree)).toBe('/' + CAC_CHILD_REDIRECT); + expect(logger.logs).toEqual([CAC_CHILD_REDIRECT]); + }); + }); + + it('should allow return of UrlTree from a child CanActivate', () => { + /** + * R --> R + * \ + * child (CAC) + * \ + * grandchild (CA: redirect) + */ + + const childSnapshot = createActivatedRouteSnapshot( + {component: 'child', routeConfig: {canActivateChild: [CAC_CHILD]}}); + const grandchildSnapshot = createActivatedRouteSnapshot( + {component: 'grandchild', routeConfig: {canActivate: [CA_GRANDCHILD_REDIRECT]}}); + + const futureState = new (RouterStateSnapshot as any)( + 'url', new TreeNode( + empty.root, + [new TreeNode(childSnapshot, [new TreeNode(grandchildSnapshot, [])])])); + + checkGuards(futureState, empty, TestBed, (result) => { + expect(serializer.serialize(result as UrlTree)).toBe('/' + CA_GRANDCHILD_REDIRECT); + expect(logger.logs).toEqual([CAC_CHILD, CA_GRANDCHILD_REDIRECT]); + }); + }); + + it('should allow return of UrlTree from a child CanDeactivate', () => { + /** + * R --> R + * / \ + * prev (CDA: redirect) child (CA) + * \ + * grandchild (CA) + */ + + const prevSnapshot = createActivatedRouteSnapshot( + {component: 'prev', routeConfig: {canDeactivate: [CDA_CHILD_REDIRECT]}}); + const childSnapshot = createActivatedRouteSnapshot({ + component: 'child', + routeConfig: {canActivate: [CA_CHILD], canActivateChild: [CAC_CHILD]} + }); + const grandchildSnapshot = createActivatedRouteSnapshot( + {component: 'grandchild', routeConfig: {canActivate: [CA_GRANDCHILD]}}); + + const currentState = new (RouterStateSnapshot as any)( + 'prev', new TreeNode(empty.root, [new TreeNode(prevSnapshot, [])])); + const futureState = new (RouterStateSnapshot as any)( + 'url', new TreeNode( + empty.root, + [new TreeNode(childSnapshot, [new TreeNode(grandchildSnapshot, [])])])); + + checkGuards(futureState, currentState, TestBed, (result) => { + expect(serializer.serialize(result as UrlTree)).toBe('/' + CDA_CHILD_REDIRECT); + expect(logger.logs).toEqual([CDA_CHILD_REDIRECT]); + }); + }); + + }); }); describe('resolve', () => { @@ -544,10 +667,15 @@ function checkResolveData( function checkGuards( future: RouterStateSnapshot, curr: RouterStateSnapshot, injector: any, - check: (result: boolean) => void): void { + check: (result: boolean | UrlTree) => void): void { of ({ guards: getAllRouteGuards(future, curr, new ChildrenOutletContexts()) } as Partial) .pipe(checkGuardsOperator(injector)) - .subscribe(t => check(!!t.guardsResult), (e) => { throw e; }); + .subscribe( + t => { + if (t.guardsResult === null) throw new Error('Guard result expected'); + return check(t.guardsResult); + }, + (e) => { throw e; }); }