From 5ad621b27ea13177196c35ea31f5abdc6714762b Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Fri, 22 Sep 2023 10:23:07 +0200 Subject: [PATCH 1/4] fix issue where more than one api call was made on every route change --- src/app/core/data/request.service.spec.ts | 37 +++++++++ src/app/core/data/request.service.ts | 30 ++++++- src/app/core/data/root-data.service.spec.ts | 38 ++++----- src/app/core/data/root-data.service.ts | 13 ++- .../server-check/server-check.guard.spec.ts | 80 +++++++++++-------- .../core/server-check/server-check.guard.ts | 34 ++++++-- src/modules/app/browser-init.service.ts | 13 ++- 7 files changed, 174 insertions(+), 71 deletions(-) diff --git a/src/app/core/data/request.service.spec.ts b/src/app/core/data/request.service.spec.ts index 108a5888818..12208d7e40a 100644 --- a/src/app/core/data/request.service.spec.ts +++ b/src/app/core/data/request.service.spec.ts @@ -638,4 +638,41 @@ describe('RequestService', () => { expect(done$).toBeObservable(cold('-----(t|)', { t: true })); })); }); + + describe('setStaleByHref', () => { + const uuid = 'c574a42c-4818-47ac-bbe1-6c3cd622c81f'; + const href = 'https://rest.api/some/object'; + const freshRE: any = { + request: { uuid, href }, + state: RequestEntryState.Success + }; + const staleRE: any = { + request: { uuid, href }, + state: RequestEntryState.SuccessStale + }; + + it(`should call getByHref to retrieve the RequestEntry matching the href`, () => { + spyOn(service, 'getByHref').and.returnValue(observableOf(staleRE)); + service.setStaleByHref(href); + expect(service.getByHref).toHaveBeenCalledWith(href); + }); + + it(`should dispatch a RequestStaleAction for the RequestEntry returned by getByHref`, (done: DoneFn) => { + spyOn(service, 'getByHref').and.returnValue(observableOf(staleRE)); + spyOn(store, 'dispatch'); + service.setStaleByHref(href).subscribe(() => { + expect(store.dispatch).toHaveBeenCalledWith(new RequestStaleAction(uuid)); + done(); + }); + }); + + it(`should emit true when the request in the store is stale`, () => { + spyOn(service, 'getByHref').and.returnValue(cold('a-b', { + a: freshRE, + b: staleRE + })); + const result$ = service.setStaleByHref(href); + expect(result$).toBeObservable(cold('--(c|)', { c: true })); + }); + }); }); diff --git a/src/app/core/data/request.service.ts b/src/app/core/data/request.service.ts index 1f6680203e0..27176390d7e 100644 --- a/src/app/core/data/request.service.ts +++ b/src/app/core/data/request.service.ts @@ -16,7 +16,7 @@ import { RequestExecuteAction, RequestStaleAction } from './request.actions'; -import { GetRequest} from './request.models'; +import { GetRequest } from './request.models'; import { CommitSSBAction } from '../cache/server-sync-buffer.actions'; import { RestRequestMethod } from './rest-request-method'; import { coreSelector } from '../core.selectors'; @@ -331,7 +331,29 @@ export class RequestService { map((request: RequestEntry) => isStale(request.state)), filter((stale: boolean) => stale), take(1), - ); + ); + } + + /** + * Mark a request as stale + * @param href the href of the request + * @return an Observable that will emit true once the Request becomes stale + */ + setStaleByHref(href: string): Observable { + const requestEntry$ = this.getByHref(href); + + requestEntry$.pipe( + map((re: RequestEntry) => re.request.uuid), + take(1), + ).subscribe((uuid: string) => { + this.store.dispatch(new RequestStaleAction(uuid)); + }); + + return requestEntry$.pipe( + map((request: RequestEntry) => isStale(request.state)), + filter((stale: boolean) => stale), + take(1) + ); } /** @@ -344,10 +366,10 @@ export class RequestService { // if it's not a GET request if (request.method !== RestRequestMethod.GET) { return true; - // if it is a GET request, check it isn't pending + // if it is a GET request, check it isn't pending } else if (this.isPending(request)) { return false; - // if it is pending, check if we're allowed to use a cached version + // if it is pending, check if we're allowed to use a cached version } else if (!useCachedVersionIfAvailable) { return true; } else { diff --git a/src/app/core/data/root-data.service.spec.ts b/src/app/core/data/root-data.service.spec.ts index b65449d0075..c34ad375310 100644 --- a/src/app/core/data/root-data.service.spec.ts +++ b/src/app/core/data/root-data.service.spec.ts @@ -1,16 +1,18 @@ import { RootDataService } from './root-data.service'; import { HALEndpointService } from '../shared/hal-endpoint.service'; -import { createSuccessfulRemoteDataObject$ } from '../../shared/remote-data.utils'; -import { Observable, of } from 'rxjs'; +import { + createSuccessfulRemoteDataObject$, + createFailedRemoteDataObject$ +} from '../../shared/remote-data.utils'; +import { Observable } from 'rxjs'; import { RemoteData } from './remote-data'; import { Root } from './root.model'; -import { RawRestResponse } from '../dspace-rest/raw-rest-response.model'; import { cold } from 'jasmine-marbles'; describe('RootDataService', () => { let service: RootDataService; let halService: HALEndpointService; - let restService; + let requestService; let rootEndpoint; let findByHrefSpy; @@ -19,10 +21,10 @@ describe('RootDataService', () => { halService = jasmine.createSpyObj('halService', { getRootHref: rootEndpoint, }); - restService = jasmine.createSpyObj('halService', { - get: jasmine.createSpy('get'), - }); - service = new RootDataService(null, null, null, halService, restService); + requestService = jasmine.createSpyObj('requestService', [ + 'setStaleByHref', + ]); + service = new RootDataService(requestService, null, null, halService); findByHrefSpy = spyOn(service as any, 'findByHref'); findByHrefSpy.and.returnValue(createSuccessfulRemoteDataObject$({})); @@ -47,12 +49,8 @@ describe('RootDataService', () => { let result$: Observable; it('should return observable of true when root endpoint is available', () => { - const mockResponse = { - statusCode: 200, - statusText: 'OK' - } as RawRestResponse; + spyOn(service, 'findRoot').and.returnValue(createSuccessfulRemoteDataObject$({} as any)); - restService.get.and.returnValue(of(mockResponse)); result$ = service.checkServerAvailability(); expect(result$).toBeObservable(cold('(a|)', { @@ -61,12 +59,8 @@ describe('RootDataService', () => { }); it('should return observable of false when root endpoint is not available', () => { - const mockResponse = { - statusCode: 500, - statusText: 'Internal Server Error' - } as RawRestResponse; + spyOn(service, 'findRoot').and.returnValue(createFailedRemoteDataObject$('500')); - restService.get.and.returnValue(of(mockResponse)); result$ = service.checkServerAvailability(); expect(result$).toBeObservable(cold('(a|)', { @@ -75,4 +69,12 @@ describe('RootDataService', () => { }); }); + + describe(`invalidateRootCache`, () => { + it(`should set the cached root request to stale`, () => { + service.invalidateRootCache(); + expect(halService.getRootHref).toHaveBeenCalled(); + expect(requestService.setStaleByHref).toHaveBeenCalledWith(rootEndpoint); + }); + }); }); diff --git a/src/app/core/data/root-data.service.ts b/src/app/core/data/root-data.service.ts index 54fe614d3e8..88cffdf6cf2 100644 --- a/src/app/core/data/root-data.service.ts +++ b/src/app/core/data/root-data.service.ts @@ -7,12 +7,11 @@ import { HALEndpointService } from '../shared/hal-endpoint.service'; import { Observable, of as observableOf } from 'rxjs'; import { RemoteData } from './remote-data'; import { FollowLinkConfig } from '../../shared/utils/follow-link-config.model'; -import { DspaceRestService } from '../dspace-rest/dspace-rest.service'; -import { RawRestResponse } from '../dspace-rest/raw-rest-response.model'; import { catchError, map } from 'rxjs/operators'; import { BaseDataService } from './base/base-data.service'; import { ObjectCacheService } from '../cache/object-cache.service'; import { dataService } from './base/data-service.decorator'; +import { getFirstCompletedRemoteData } from '../shared/operators'; /** * A service to retrieve the {@link Root} object from the REST API. @@ -25,21 +24,21 @@ export class RootDataService extends BaseDataService { protected rdbService: RemoteDataBuildService, protected objectCache: ObjectCacheService, protected halService: HALEndpointService, - protected restService: DspaceRestService, ) { - super('', requestService, rdbService, objectCache, halService, 6 * 60 * 60 * 1000); + super('', requestService, rdbService, objectCache, halService, 60 * 1000); } /** * Check if root endpoint is available */ checkServerAvailability(): Observable { - return this.restService.get(this.halService.getRootHref()).pipe( + return this.findRoot().pipe( catchError((err ) => { console.error(err); return observableOf(false); }), - map((res: RawRestResponse) => res.statusCode === 200) + getFirstCompletedRemoteData(), + map((rootRd: RemoteData) => rootRd.statusCode === 200) ); } @@ -60,6 +59,6 @@ export class RootDataService extends BaseDataService { * Set to sale the root endpoint cache hit */ invalidateRootCache() { - this.requestService.setStaleByHrefSubstring(this.halService.getRootHref()); + this.requestService.setStaleByHref(this.halService.getRootHref()); } } diff --git a/src/app/core/server-check/server-check.guard.spec.ts b/src/app/core/server-check/server-check.guard.spec.ts index 1f126be5e55..13045b0ff6d 100644 --- a/src/app/core/server-check/server-check.guard.spec.ts +++ b/src/app/core/server-check/server-check.guard.spec.ts @@ -1,68 +1,78 @@ import { ServerCheckGuard } from './server-check.guard'; -import { Router } from '@angular/router'; +import { Router, NavigationStart, UrlTree, NavigationEnd, RouterEvent } from '@angular/router'; -import { of } from 'rxjs'; -import { take } from 'rxjs/operators'; - -import { getPageInternalServerErrorRoute } from '../../app-routing-paths'; +import { of, ReplaySubject } from 'rxjs'; import { RootDataService } from '../data/root-data.service'; +import { TestScheduler } from 'rxjs/testing'; import SpyObj = jasmine.SpyObj; describe('ServerCheckGuard', () => { let guard: ServerCheckGuard; - let router: SpyObj; + let router: Router; + const eventSubject = new ReplaySubject(1); let rootDataServiceStub: SpyObj; - - rootDataServiceStub = jasmine.createSpyObj('RootDataService', { - checkServerAvailability: jasmine.createSpy('checkServerAvailability'), - invalidateRootCache: jasmine.createSpy('invalidateRootCache') - }); - router = jasmine.createSpyObj('Router', { - navigateByUrl: jasmine.createSpy('navigateByUrl') - }); + let testScheduler: TestScheduler; + let redirectUrlTree: UrlTree; beforeEach(() => { + testScheduler = new TestScheduler((actual, expected) => { + expect(actual).toEqual(expected); + }); + rootDataServiceStub = jasmine.createSpyObj('RootDataService', { + checkServerAvailability: jasmine.createSpy('checkServerAvailability'), + invalidateRootCache: jasmine.createSpy('invalidateRootCache') + }); + redirectUrlTree = new UrlTree(); + router = { + events: eventSubject.asObservable(), + navigateByUrl: jasmine.createSpy('navigateByUrl'), + parseUrl: jasmine.createSpy('parseUrl').and.returnValue(redirectUrlTree) + } as any; guard = new ServerCheckGuard(router, rootDataServiceStub); }); - afterEach(() => { - router.navigateByUrl.calls.reset(); - rootDataServiceStub.invalidateRootCache.calls.reset(); - }); - it('should be created', () => { expect(guard).toBeTruthy(); }); - describe('when root endpoint has succeeded', () => { + describe('when root endpoint request has succeeded', () => { beforeEach(() => { rootDataServiceStub.checkServerAvailability.and.returnValue(of(true)); }); - it('should not redirect to error page', () => { - guard.canActivateChild({} as any, {} as any).pipe( - take(1) - ).subscribe((canActivate: boolean) => { - expect(canActivate).toEqual(true); - expect(rootDataServiceStub.invalidateRootCache).not.toHaveBeenCalled(); - expect(router.navigateByUrl).not.toHaveBeenCalled(); + it('should return true', () => { + testScheduler.run(({ expectObservable }) => { + const result$ = guard.canActivateChild({} as any, {} as any); + expectObservable(result$).toBe('(a|)', { a: true }); }); }); }); - describe('when root endpoint has not succeeded', () => { + describe('when root endpoint request has not succeeded', () => { beforeEach(() => { rootDataServiceStub.checkServerAvailability.and.returnValue(of(false)); }); - it('should redirect to error page', () => { - guard.canActivateChild({} as any, {} as any).pipe( - take(1) - ).subscribe((canActivate: boolean) => { - expect(canActivate).toEqual(false); - expect(rootDataServiceStub.invalidateRootCache).toHaveBeenCalled(); - expect(router.navigateByUrl).toHaveBeenCalledWith(getPageInternalServerErrorRoute()); + it('should return a UrlTree with the route to the 500 error page', () => { + testScheduler.run(({ expectObservable }) => { + const result$ = guard.canActivateChild({} as any, {} as any); + expectObservable(result$).toBe('(b|)', { b: redirectUrlTree }); }); + expect(router.parseUrl).toHaveBeenCalledWith('/500'); + }); + }); + + describe(`listenForRouteChanges`, () => { + it(`should invalidate the root cache on every NavigationStart event`, () => { + testScheduler.run(() => { + guard.listenForRouteChanges(); + eventSubject.next(new NavigationStart(1,'')); + eventSubject.next(new NavigationEnd(1,'', '')); + eventSubject.next(new NavigationStart(2,'')); + eventSubject.next(new NavigationEnd(2,'', '')); + eventSubject.next(new NavigationStart(3,'')); + }); + expect(rootDataServiceStub.invalidateRootCache).toHaveBeenCalledTimes(3); }); }); }); diff --git a/src/app/core/server-check/server-check.guard.ts b/src/app/core/server-check/server-check.guard.ts index 8a0e26c01da..ab3bc2c6e99 100644 --- a/src/app/core/server-check/server-check.guard.ts +++ b/src/app/core/server-check/server-check.guard.ts @@ -1,8 +1,15 @@ import { Injectable } from '@angular/core'; -import { ActivatedRouteSnapshot, CanActivateChild, Router, RouterStateSnapshot } from '@angular/router'; +import { + ActivatedRouteSnapshot, + CanActivateChild, + Router, + RouterStateSnapshot, + UrlTree, + NavigationStart +} from '@angular/router'; import { Observable } from 'rxjs'; -import { take, tap } from 'rxjs/operators'; +import { take, map, filter } from 'rxjs/operators'; import { RootDataService } from '../data/root-data.service'; import { getPageInternalServerErrorRoute } from '../../app-routing-paths'; @@ -23,17 +30,32 @@ export class ServerCheckGuard implements CanActivateChild { */ canActivateChild( route: ActivatedRouteSnapshot, - state: RouterStateSnapshot): Observable { + state: RouterStateSnapshot + ): Observable { return this.rootDataService.checkServerAvailability().pipe( take(1), - tap((isAvailable: boolean) => { + map((isAvailable: boolean) => { if (!isAvailable) { - this.rootDataService.invalidateRootCache(); - this.router.navigateByUrl(getPageInternalServerErrorRoute()); + return this.router.parseUrl(getPageInternalServerErrorRoute()); + } else { + return true; } }) ); + } + /** + * Listen to all router events. Every time a new navigation starts, invalidate the cache + * for the root endpoint. That way we retrieve it once per routing operation to ensure the + * backend is not down. But if the guard is called multiple times during the same routing + * operation, the cached version is used. + */ + listenForRouteChanges(): void { + this.router.events.pipe( + filter(event => event instanceof NavigationStart), + ).subscribe(() => { + this.rootDataService.invalidateRootCache(); + }); } } diff --git a/src/modules/app/browser-init.service.ts b/src/modules/app/browser-init.service.ts index 61d57f10f98..bf40f0c68b6 100644 --- a/src/modules/app/browser-init.service.ts +++ b/src/modules/app/browser-init.service.ts @@ -32,6 +32,7 @@ import { logStartupMessage } from '../../../startup-message'; import { MenuService } from '../../app/shared/menu/menu.service'; import { RootDataService } from '../../app/core/data/root-data.service'; import { firstValueFrom, Subscription } from 'rxjs'; +import { ServerCheckGuard } from '../../app/core/server-check/server-check.guard'; /** * Performs client-side initialization. @@ -56,7 +57,8 @@ export class BrowserInitService extends InitService { protected authService: AuthService, protected themeService: ThemeService, protected menuService: MenuService, - private rootDataService: RootDataService + private rootDataService: RootDataService, + protected serverCheckGuard: ServerCheckGuard, ) { super( store, @@ -172,4 +174,13 @@ export class BrowserInitService extends InitService { }); } + /** + * Start route-listening subscriptions + * @protected + */ + protected initRouteListeners(): void { + super.initRouteListeners(); + this.serverCheckGuard.listenForRouteChanges(); + } + } From 8fbe8c16dc2fe999cc53c851bb8ae9df9b8487e4 Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Fri, 22 Sep 2023 12:00:59 +0200 Subject: [PATCH 2/4] fix issue where invalidateRootCache didn't happen when the page first loaded --- src/app/core/server-check/server-check.guard.spec.ts | 6 ++++-- src/app/core/server-check/server-check.guard.ts | 4 ++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/app/core/server-check/server-check.guard.spec.ts b/src/app/core/server-check/server-check.guard.spec.ts index 13045b0ff6d..f18b3867538 100644 --- a/src/app/core/server-check/server-check.guard.spec.ts +++ b/src/app/core/server-check/server-check.guard.spec.ts @@ -63,16 +63,18 @@ describe('ServerCheckGuard', () => { }); describe(`listenForRouteChanges`, () => { - it(`should invalidate the root cache on every NavigationStart event`, () => { + it(`should invalidate the root cache when the method is first called, and then on every NavigationStart event`, () => { testScheduler.run(() => { guard.listenForRouteChanges(); + expect(rootDataServiceStub.invalidateRootCache).toHaveBeenCalledTimes(1); + eventSubject.next(new NavigationStart(1,'')); eventSubject.next(new NavigationEnd(1,'', '')); eventSubject.next(new NavigationStart(2,'')); eventSubject.next(new NavigationEnd(2,'', '')); eventSubject.next(new NavigationStart(3,'')); }); - expect(rootDataServiceStub.invalidateRootCache).toHaveBeenCalledTimes(3); + expect(rootDataServiceStub.invalidateRootCache).toHaveBeenCalledTimes(4); }); }); }); diff --git a/src/app/core/server-check/server-check.guard.ts b/src/app/core/server-check/server-check.guard.ts index ab3bc2c6e99..1cea5f36bab 100644 --- a/src/app/core/server-check/server-check.guard.ts +++ b/src/app/core/server-check/server-check.guard.ts @@ -52,6 +52,10 @@ export class ServerCheckGuard implements CanActivateChild { * operation, the cached version is used. */ listenForRouteChanges(): void { + // we'll always be too late for the first NavigationStart event with the router subscribe below, + // so this statement is for the very first route operation + this.rootDataService.invalidateRootCache(); + this.router.events.pipe( filter(event => event instanceof NavigationStart), ).subscribe(() => { From 32fc28ec54af589b5813321490703896b1843fff Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Tue, 26 Sep 2023 14:44:13 +0200 Subject: [PATCH 3/4] fix dev mode issue where retrieving the login options fails --- .../core/server-check/server-check.guard.spec.ts | 14 ++++++++++---- src/app/core/server-check/server-check.guard.ts | 6 ++++-- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/src/app/core/server-check/server-check.guard.spec.ts b/src/app/core/server-check/server-check.guard.spec.ts index f18b3867538..044609ef427 100644 --- a/src/app/core/server-check/server-check.guard.spec.ts +++ b/src/app/core/server-check/server-check.guard.spec.ts @@ -20,7 +20,8 @@ describe('ServerCheckGuard', () => { }); rootDataServiceStub = jasmine.createSpyObj('RootDataService', { checkServerAvailability: jasmine.createSpy('checkServerAvailability'), - invalidateRootCache: jasmine.createSpy('invalidateRootCache') + invalidateRootCache: jasmine.createSpy('invalidateRootCache'), + findRoot: jasmine.createSpy('findRoot') }); redirectUrlTree = new UrlTree(); router = { @@ -63,18 +64,23 @@ describe('ServerCheckGuard', () => { }); describe(`listenForRouteChanges`, () => { - it(`should invalidate the root cache when the method is first called, and then on every NavigationStart event`, () => { + it(`should retrieve the root endpoint, without using the cache, when the method is first called`, () => { testScheduler.run(() => { guard.listenForRouteChanges(); - expect(rootDataServiceStub.invalidateRootCache).toHaveBeenCalledTimes(1); + expect(rootDataServiceStub.findRoot).toHaveBeenCalledWith(false); + }); + }); + it(`should invalidate the root cache on every NavigationStart event`, () => { + testScheduler.run(() => { + guard.listenForRouteChanges(); eventSubject.next(new NavigationStart(1,'')); eventSubject.next(new NavigationEnd(1,'', '')); eventSubject.next(new NavigationStart(2,'')); eventSubject.next(new NavigationEnd(2,'', '')); eventSubject.next(new NavigationStart(3,'')); }); - expect(rootDataServiceStub.invalidateRootCache).toHaveBeenCalledTimes(4); + expect(rootDataServiceStub.invalidateRootCache).toHaveBeenCalledTimes(3); }); }); }); diff --git a/src/app/core/server-check/server-check.guard.ts b/src/app/core/server-check/server-check.guard.ts index 1cea5f36bab..65ca2b0c498 100644 --- a/src/app/core/server-check/server-check.guard.ts +++ b/src/app/core/server-check/server-check.guard.ts @@ -53,8 +53,10 @@ export class ServerCheckGuard implements CanActivateChild { */ listenForRouteChanges(): void { // we'll always be too late for the first NavigationStart event with the router subscribe below, - // so this statement is for the very first route operation - this.rootDataService.invalidateRootCache(); + // so this statement is for the very first route operation. A `find` without using the cache, + // rather than an invalidateRootCache, because invalidating as the app is bootstrapping can + // break other features + this.rootDataService.findRoot(false); this.router.events.pipe( filter(event => event instanceof NavigationStart), From 6a99185214f836271fe42d6ad73a14dbeb95a648 Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Tue, 26 Sep 2023 16:56:07 +0200 Subject: [PATCH 4/4] roll back unintended change to the responseMsToLive for RootDataservice --- src/app/core/data/root-data.service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/core/data/root-data.service.ts b/src/app/core/data/root-data.service.ts index 88cffdf6cf2..5431a2d1fb2 100644 --- a/src/app/core/data/root-data.service.ts +++ b/src/app/core/data/root-data.service.ts @@ -25,7 +25,7 @@ export class RootDataService extends BaseDataService { protected objectCache: ObjectCacheService, protected halService: HALEndpointService, ) { - super('', requestService, rdbService, objectCache, halService, 60 * 1000); + super('', requestService, rdbService, objectCache, halService, 6 * 60 * 60 * 1000); } /**