Skip to content

Commit

Permalink
Merge pull request #2529 from atmire/fix-mutliple-api-calls-on-route-…
Browse files Browse the repository at this point in the history
…change-8.0.0-next

[Port main] Fix multiple api calls on route change
  • Loading branch information
alanorth authored Sep 27, 2023
2 parents 4491e6c + 623fc6e commit 8045e4c
Show file tree
Hide file tree
Showing 7 changed files with 187 additions and 70 deletions.
37 changes: 37 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,43 @@ describe('RequestService', () => {
}));
});

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

describe('setStaleByHrefSubstring', () => {
let dispatchSpy: jasmine.Spy;
let getByUUIDSpy: jasmine.Spy;
Expand Down
30 changes: 26 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,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<boolean> {
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)
);
}

/**
Expand All @@ -364,10 +386,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());
}
}
88 changes: 53 additions & 35 deletions src/app/core/server-check/server-check.guard.spec.ts
Original file line number Diff line number Diff line change
@@ -1,68 +1,86 @@
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<Router>;
let router: Router;
const eventSubject = new ReplaySubject<RouterEvent>(1);
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'),
findRoot: jasmine.createSpy('findRoot')
});
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 retrieve the root endpoint, without using the cache, when the method is first called`, () => {
testScheduler.run(() => {
guard.listenForRouteChanges();
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(3);
});
});
});
Loading

0 comments on commit 8045e4c

Please sign in to comment.