Skip to content

Commit

Permalink
fix issue where more than one api call was made on every route change
Browse files Browse the repository at this point in the history
  • Loading branch information
artlowel committed Sep 21, 2023
1 parent 2d73373 commit c36fe6c
Show file tree
Hide file tree
Showing 6 changed files with 120 additions and 68 deletions.
24 changes: 24 additions & 0 deletions src/app/core/data/request.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -639,6 +639,30 @@ describe('RequestService', () => {
}));
});

describe('setStaleByHref', () => {
const uuid = 'c574a42c-4818-47ac-bbe1-6c3cd622c81f';
const href = 'https://rest.api/some/object';
const mockRequestEntry: any = {
request: { uuid, href }
};
beforeEach(() => {
spyOn(service, 'getByHref').and.returnValue(cold('a', { a: mockRequestEntry }));
spyOn(service, 'setStaleByUUID').and.returnValue(cold('--(b|)', { b: true }));
});

it(`should call getByHref to retrieve the RequestEntry matching the href`, () => {
service.setStaleByHref(href);
expect(service.getByHref).toHaveBeenCalledWith(href);
});

it(`should use the UUID of the request in the entry retrieved by getByHref to call setStaleByUUID, and return the output of setStaleByUUID`, () => {
const result$ = service.setStaleByHref(href);
expect(result$).toBeObservable(cold('--(c|)', { c: true }));
expect(service.setStaleByUUID).toHaveBeenCalledWith(uuid);
});

});

describe('setStaleByHrefSubstring', () => {
let dispatchSpy: jasmine.Spy;
let getByUUIDSpy: jasmine.Spy;
Expand Down
21 changes: 17 additions & 4 deletions src/app/core/data/request.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -351,7 +351,20 @@ 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<boolean> {
return this.getByHref(href).pipe(
map((re: RequestEntry) => re.request.uuid),
take(1),
switchMap((uuid: string) => this.setStaleByUUID(uuid))
);
}

/**
Expand All @@ -364,10 +377,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 {
Expand Down
38 changes: 20 additions & 18 deletions src/app/core/data/root-data.service.spec.ts
Original file line number Diff line number Diff line change
@@ -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;

Expand All @@ -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$({}));
Expand All @@ -47,12 +49,8 @@ describe('RootDataService', () => {
let result$: Observable<boolean>;

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$<Root>({} as any));

restService.get.and.returnValue(of(mockResponse));
result$ = service.checkServerAvailability();

expect(result$).toBeObservable(cold('(a|)', {
Expand All @@ -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$<Root>('500'));

restService.get.and.returnValue(of(mockResponse));
result$ = service.checkServerAvailability();

expect(result$).toBeObservable(cold('(a|)', {
Expand All @@ -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);
});
});
});
11 changes: 5 additions & 6 deletions src/app/core/data/root-data.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -25,7 +24,6 @@ export class RootDataService extends BaseDataService<Root> {
protected rdbService: RemoteDataBuildService,
protected objectCache: ObjectCacheService,
protected halService: HALEndpointService,
protected restService: DspaceRestService,
) {
super('', requestService, rdbService, objectCache, halService, 6 * 60 * 60 * 1000);
}
Expand All @@ -34,12 +32,13 @@ export class RootDataService extends BaseDataService<Root> {
* Check if root endpoint is available
*/
checkServerAvailability(): Observable<boolean> {
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<Root>) => rootRd.statusCode === 200)
);
}

Expand All @@ -60,6 +59,6 @@ export class RootDataService extends BaseDataService<Root> {
* Set to sale the root endpoint cache hit
*/
invalidateRootCache() {
this.requestService.setStaleByHrefSubstring(this.halService.getRootHref());
this.requestService.setStaleByHref(this.halService.getRootHref());
}
}
61 changes: 28 additions & 33 deletions src/app/core/server-check/server-check.guard.spec.ts
Original file line number Diff line number Diff line change
@@ -1,68 +1,63 @@
import { ServerCheckGuard } from './server-check.guard';
import { Router } from '@angular/router';
import { Router, NavigationStart, UrlTree } from '@angular/router';

import { of } from 'rxjs';
import { take } from 'rxjs/operators';

import { getPageInternalServerErrorRoute } from '../../app-routing-paths';
import { RootDataService } from '../data/root-data.service';
import { TestScheduler } from 'rxjs/testing';
import SpyObj = jasmine.SpyObj;

describe('ServerCheckGuard', () => {
let guard: ServerCheckGuard;
let router: SpyObj<Router>;
let rootDataServiceStub: SpyObj<RootDataService>;

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: of(new NavigationStart(1, '')),
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');
});
});
});
33 changes: 26 additions & 7 deletions src/app/core/server-check/server-check.guard.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -16,24 +23,36 @@ import { getPageInternalServerErrorRoute } from '../../app-routing-paths';
*/
export class ServerCheckGuard implements CanActivateChild {
constructor(private router: Router, private rootDataService: RootDataService) {
/**
* 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.
*/
this.router.events.pipe(
filter(event => event instanceof NavigationStart),
).subscribe(() => {
this.rootDataService.invalidateRootCache();
});
}

/**
* True when root api endpoint is reachable.
*/
canActivateChild(
route: ActivatedRouteSnapshot,
state: RouterStateSnapshot): Observable<boolean> {
state: RouterStateSnapshot
): Observable<boolean | UrlTree> {

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;
}
})
);

}
}

0 comments on commit c36fe6c

Please sign in to comment.