From f368da7a277fc5ed0de2bf9698de0de4ea67ce64 Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Mon, 6 Apr 2020 16:40:45 +0200 Subject: [PATCH 1/3] fix issue where requestservice.getByUUID wouldn't work for requests that were never sent because there was already a copy in the cache --- src/app/core/data/request.service.ts | 30 +++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/src/app/core/data/request.service.ts b/src/app/core/data/request.service.ts index 810b0721ae..c68bd751fd 100644 --- a/src/app/core/data/request.service.ts +++ b/src/app/core/data/request.service.ts @@ -3,9 +3,9 @@ import { HttpHeaders } from '@angular/common/http'; import { createSelector, MemoizedSelector, select, Store } from '@ngrx/store'; import { Observable, race as observableRace } from 'rxjs'; -import { filter, map, mergeMap, take } from 'rxjs/operators'; +import { filter, map, mergeMap, take, switchMap } from 'rxjs/operators'; import { cloneDeep, remove } from 'lodash'; -import { hasValue, isEmpty, isNotEmpty } from '../../shared/empty.util'; +import { hasValue, isEmpty, isNotEmpty, hasValueOperator } from '../../shared/empty.util'; import { CacheableObject } from '../cache/object-cache.reducer'; import { ObjectCacheService } from '../cache/object-cache.service'; import { CoreState } from '../core.reducers'; @@ -111,13 +111,22 @@ export class RequestService { */ getByUUID(uuid: string): Observable { return observableRace( - this.store.pipe(select(entryFromUUIDSelector(uuid))), + this.store.pipe( + select(entryFromUUIDSelector(uuid)), + hasValueOperator() + ), this.store.pipe( select(originalRequestUUIDFromRequestUUIDSelector(uuid)), - mergeMap((originalUUID) => { - return this.store.pipe(select(entryFromUUIDSelector(originalUUID))) + switchMap((originalUUID) => { + if (hasValue(originalUUID)) { + return this.store.pipe(select(entryFromUUIDSelector(originalUUID))) + } else { + return [] + } }, - )) + ), + hasValueOperator() + ) ).pipe( map((entry: RequestEntry) => { // Headers break after being retrieved from the store (because of lazy initialization) @@ -137,7 +146,14 @@ export class RequestService { getByHref(href: string): Observable { return this.store.pipe( select(uuidFromHrefSelector(href)), - mergeMap((uuid: string) => this.getByUUID(uuid)) + mergeMap((uuid: string) => { + if (isNotEmpty(uuid)) { + return this.getByUUID(uuid); + } + else { + return [undefined]; + } + }) ); } From 6351a951e71e11ae6d63eb8a785ad1165216a32f Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Tue, 7 Apr 2020 11:08:20 +0200 Subject: [PATCH 2/3] fix test and lint --- src/app/core/data/request.service.spec.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/core/data/request.service.spec.ts b/src/app/core/data/request.service.spec.ts index 017721fdf9..9f8e20c8b7 100644 --- a/src/app/core/data/request.service.spec.ts +++ b/src/app/core/data/request.service.spec.ts @@ -162,7 +162,7 @@ describe('RequestService', () => { }); }); - describe('if the request with the specified UUID doesn\'t exist in the store', () => { + describe(`if the request with the specified UUID doesn't exist in the store `, () => { beforeEach(() => { selectSpy.and.callFake(() => { return () => { @@ -171,10 +171,10 @@ describe('RequestService', () => { }); }); - it('should return an Observable of undefined', () => { + it(`it shouldn't return anything`, () => { const result = service.getByUUID(testUUID); - scheduler.expectObservable(result).toBe('b', { b: undefined }); + scheduler.expectObservable(result).toBe(''); }); }); From cea01d46f9f3ccd919f15c23c5a1f37b1cc1f1ee Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Tue, 7 Apr 2020 13:31:17 +0200 Subject: [PATCH 3/3] add test to prove the bug is fixed --- src/app/core/data/request.service.spec.ts | 37 ++++++++++++++++++++++- src/app/core/data/request.service.ts | 3 +- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/src/app/core/data/request.service.spec.ts b/src/app/core/data/request.service.spec.ts index 9f8e20c8b7..b70f2dc47d 100644 --- a/src/app/core/data/request.service.spec.ts +++ b/src/app/core/data/request.service.spec.ts @@ -22,6 +22,7 @@ import { } from './request.models'; import { RequestEntry } from './request.reducer'; import { RequestService } from './request.service'; +import { parseJsonSchemaToCommandDescription } from '@angular/cli/utilities/json-schema'; describe('RequestService', () => { let scheduler: TestScheduler; @@ -178,7 +179,41 @@ describe('RequestService', () => { }); }); - }); + describe(`if the request with the specified UUID wasn't sent, because it was already cached`, () => { + beforeEach(() => { + let callCounter = 0; + const responses = [ + cold('a', { a: undefined }), // No hit in the request cache with that UUID + cold('b', { b: 'otherRequestUUID' }), // A hit in the index, which returns the uuid of the cached request + cold('c', { c: { // the call to retrieve the cached request using the UUID from the index + c: { + completed: true + } + } + }) + ]; + selectSpy.and.callFake(() => { + return () => { + const response = responses[callCounter]; + callCounter++; + return () => response; + }; + }); + }); + + it(`it should return the cached request`, () => { + const result = service.getByUUID(testUUID); + + scheduler.expectObservable(result).toBe('c', { c: { + c: { + completed: true + } + } + }); + }); + }); + + }); describe('getByHref', () => { describe('when the request with the specified href exists in the store', () => { diff --git a/src/app/core/data/request.service.ts b/src/app/core/data/request.service.ts index c68bd751fd..f69ad646f7 100644 --- a/src/app/core/data/request.service.ts +++ b/src/app/core/data/request.service.ts @@ -149,8 +149,7 @@ export class RequestService { mergeMap((uuid: string) => { if (isNotEmpty(uuid)) { return this.getByUUID(uuid); - } - else { + } else { return [undefined]; } })