From 7b6dbf0952385ef279e897f74cd12475c8bebe42 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Wed, 8 Mar 2017 22:24:41 +0000 Subject: [PATCH] fix(aio): `LocationService` search should handle corner cases Adds tests and fixes corners cases for both `search()` and `setSearc()` for things like empty queries and param keys that need encoding. This commit refactors the `LocationService` to rely upon the `PlatformLocation` rather than using `window.history` directly. This makes testing easier but also makes the code simpler since `PlatformLocation` deals with platforms that do not support history for us. --- aio/src/app/shared/location.service.spec.ts | 102 +++++++++++++++++++- aio/src/app/shared/location.service.ts | 15 ++- 2 files changed, 105 insertions(+), 12 deletions(-) diff --git a/aio/src/app/shared/location.service.spec.ts b/aio/src/app/shared/location.service.spec.ts index 65bd27b35a..60cf4d6eac 100644 --- a/aio/src/app/shared/location.service.spec.ts +++ b/aio/src/app/shared/location.service.spec.ts @@ -1,8 +1,13 @@ import { ReflectiveInjector } from '@angular/core'; -import { Location, LocationStrategy } from '@angular/common'; +import { Location, LocationStrategy, PlatformLocation } from '@angular/common'; import { MockLocationStrategy } from '@angular/common/testing'; import { LocationService } from './location.service'; +class MockPlatformLocation { + pathname = 'a/b/c'; + replaceState = jasmine.createSpy('PlatformLocation.replaceState'); +} + describe('LocationService', () => { let injector: ReflectiveInjector; @@ -11,7 +16,8 @@ describe('LocationService', () => { injector = ReflectiveInjector.resolveAndCreate([ LocationService, Location, - { provide: LocationStrategy, useClass: MockLocationStrategy } + { provide: LocationStrategy, useClass: MockLocationStrategy }, + { provide: PlatformLocation, useClass: MockPlatformLocation } ]); }); @@ -121,10 +127,98 @@ describe('LocationService', () => { }); describe('search', () => { - it('should ...', () => { }); + it('should read the query from the current location.path', () => { + const location: MockLocationStrategy = injector.get(LocationStrategy); + const service: LocationService = injector.get(LocationService); + + location.simulatePopState('a/b/c?foo=bar&moo=car'); + expect(service.search()).toEqual({ foo: 'bar', moo: 'car' }); + }); + + it('should cope with an empty query', () => { + const location: MockLocationStrategy = injector.get(LocationStrategy); + const service: LocationService = injector.get(LocationService); + + location.simulatePopState('a/b/c'); + expect(service.search()).toEqual({ }); + + location.simulatePopState('x/y/z?'); + expect(service.search()).toEqual({ }); + + location.simulatePopState('x/y/z?x='); + expect(service.search()).toEqual({ x: '' }); + + location.simulatePopState('x/y/z?x'); + expect(service.search()).toEqual({ x: undefined }); + }); + + it('should URL decode query values', () => { + const location: MockLocationStrategy = injector.get(LocationStrategy); + const service: LocationService = injector.get(LocationService); + + location.simulatePopState('a/b/c?query=a%26b%2Bc%20d'); + expect(service.search()).toEqual({ query: 'a&b+c d' }); + }); + + it('should URL decode query keys', () => { + const location: MockLocationStrategy = injector.get(LocationStrategy); + const service: LocationService = injector.get(LocationService); + + location.simulatePopState('a/b/c?a%26b%2Bc%20d=value'); + expect(service.search()).toEqual({ 'a&b+c d': 'value' }); + }); + + it('should cope with a hash on the URL', () => { + const location: MockLocationStrategy = injector.get(LocationStrategy); + const service: LocationService = injector.get(LocationService); + + spyOn(location, 'path').and.callThrough(); + service.search(); + expect(location.path).toHaveBeenCalledWith(false); + }); }); describe('setSearch', () => { - it('should ...', () => { }); + it('should call replaceState on PlatformLocation', () => { + const location: MockPlatformLocation = injector.get(PlatformLocation); + const service: LocationService = injector.get(LocationService); + + const params = {}; + service.setSearch('Some label', params); + expect(location.replaceState).toHaveBeenCalledWith(jasmine.any(Object), 'Some label', 'a/b/c'); + }); + + it('should convert the params to a query string', () => { + const location: MockPlatformLocation = injector.get(PlatformLocation); + const service: LocationService = injector.get(LocationService); + + const params = { foo: 'bar', moo: 'car' }; + service.setSearch('Some label', params); + expect(location.replaceState).toHaveBeenCalledWith(jasmine.any(Object), 'Some label', jasmine.any(String)); + const [path, query] = location.replaceState.calls.mostRecent().args[2].split('?'); + expect(path).toEqual('a/b/c'); + expect(query).toContain('foo=bar'); + expect(query).toContain('moo=car'); + }); + + it('should URL encode param values', () => { + const location: MockPlatformLocation = injector.get(PlatformLocation); + const service: LocationService = injector.get(LocationService); + + const params = { query: 'a&b+c d' }; + service.setSearch('', params); + const [, query] = location.replaceState.calls.mostRecent().args[2].split('?'); + expect(query).toContain('query=a%26b%2Bc%20d'); + }); + + it('should URL encode param keys', () => { + const location: MockPlatformLocation = injector.get(PlatformLocation); + const service: LocationService = injector.get(LocationService); + + const params = { 'a&b+c d': 'value' }; + service.setSearch('', params); + const [, query] = location.replaceState.calls.mostRecent().args[2].split('?'); + expect(query).toContain('a%26b%2Bc%20d=value'); + }); }); }); diff --git a/aio/src/app/shared/location.service.ts b/aio/src/app/shared/location.service.ts index 80f4a3f088..1accc3138f 100644 --- a/aio/src/app/shared/location.service.ts +++ b/aio/src/app/shared/location.service.ts @@ -1,5 +1,5 @@ import { Injectable } from '@angular/core'; -import { Location } from '@angular/common'; +import { Location, PlatformLocation } from '@angular/common'; import { Observable } from 'rxjs/Observable'; import { BehaviorSubject } from 'rxjs/BehaviorSubject'; @@ -9,7 +9,7 @@ export class LocationService { private urlSubject: BehaviorSubject; get currentUrl() { return this.urlSubject.asObservable(); } - constructor(private location: Location) { + constructor(private location: Location, private platformLocation: PlatformLocation) { const initialUrl = this.stripLeadingSlashes(location.path(true)); this.urlSubject = new BehaviorSubject(initialUrl); @@ -39,7 +39,9 @@ export class LocationService { const params = path.substr(q + 1).split('&'); params.forEach(p => { const pair = p.split('='); - search[pair[0]] = decodeURIComponent(pair[1]); + if (pair[0]) { + search[decodeURIComponent(pair[0])] = pair[1] && decodeURIComponent(pair[1]); + } }); } catch (e) { /* don't care */ } } @@ -47,16 +49,13 @@ export class LocationService { } setSearch(label: string, params: {}) { - if (!window || !window.history) { return; } - const search = Object.keys(params).reduce((acc, key) => { const value = params[key]; // tslint:disable-next-line:triple-equals return value == undefined ? acc : - acc += (acc ? '&' : '?') + `${key}=${encodeURIComponent(value)}`; + acc += (acc ? '&' : '?') + `${encodeURIComponent(key)}=${encodeURIComponent(value)}`; }, ''); - // this.location.replaceState doesn't let you set the history stack label - window.history.replaceState({}, 'API Search', window.location.pathname + search); + this.platformLocation.replaceState({}, label, this.platformLocation.pathname + search); } }