diff --git a/src/app/core/cache/builders/remote-data-build.service.ts b/src/app/core/cache/builders/remote-data-build.service.ts index a59039c226..8d40741dc0 100644 --- a/src/app/core/cache/builders/remote-data-build.service.ts +++ b/src/app/core/cache/builders/remote-data-build.service.ts @@ -56,13 +56,13 @@ export class RemoteDataBuildService { const payload$ = observableCombineLatest( href$.pipe( - switchMap((href: string) => this.objectCache.getBySelfLink(href)), + switchMap((href: string) => this.objectCache.getObjectBySelfLink(href)), startWith(undefined)), requestEntry$.pipe( getResourceLinksFromResponse(), switchMap((resourceSelfLinks: string[]) => { if (isNotEmpty(resourceSelfLinks)) { - return this.objectCache.getBySelfLink(resourceSelfLinks[0]); + return this.objectCache.getObjectBySelfLink(resourceSelfLinks[0]); } else { return observableOf(undefined); } diff --git a/src/app/core/cache/object-cache.service.spec.ts b/src/app/core/cache/object-cache.service.spec.ts index af353a38c1..eae7c06be7 100644 --- a/src/app/core/cache/object-cache.service.spec.ts +++ b/src/app/core/cache/object-cache.service.spec.ts @@ -80,7 +80,7 @@ describe('ObjectCacheService', () => { }); // due to the implementation of spyOn above, this subscribe will be synchronous - service.getBySelfLink(selfLink).pipe(first()).subscribe((o) => { + service.getObjectBySelfLink(selfLink).pipe(first()).subscribe((o) => { expect(o.self).toBe(selfLink); // this only works if testObj is an instance of TestClass expect(o instanceof NormalizedItem).toBeTruthy(); @@ -96,7 +96,7 @@ describe('ObjectCacheService', () => { }); let getObsHasFired = false; - const subscription = service.getBySelfLink(selfLink).subscribe((o) => getObsHasFired = true); + const subscription = service.getObjectBySelfLink(selfLink).subscribe((o) => getObsHasFired = true); expect(getObsHasFired).toBe(false); subscription.unsubscribe(); }); @@ -106,7 +106,7 @@ describe('ObjectCacheService', () => { it('should return an observable of the array of cached objects with the specified self link and type', () => { const item = new NormalizedItem(); item.self = selfLink; - spyOn(service, 'getBySelfLink').and.returnValue(observableOf(item)); + spyOn(service, 'getObjectBySelfLink').and.returnValue(observableOf(item)); service.getList([selfLink, selfLink]).pipe(first()).subscribe((arr) => { expect(arr[0].self).toBe(selfLink); diff --git a/src/app/core/cache/object-cache.service.ts b/src/app/core/cache/object-cache.service.ts index d4d52b404f..384df23c22 100644 --- a/src/app/core/cache/object-cache.service.ts +++ b/src/app/core/cache/object-cache.service.ts @@ -78,16 +78,16 @@ export class ObjectCacheService { * @return Observable * An observable of the requested object */ - getByUUID(uuid: string): Observable> { + getObjectByUUID(uuid: string): Observable> { return this.store.pipe( select(selfLinkFromUuidSelector(uuid)), - mergeMap((selfLink: string) => this.getBySelfLink(selfLink) + mergeMap((selfLink: string) => this.getObjectBySelfLink(selfLink) ) ) } - getBySelfLink(selfLink: string): Observable> { - return this.getEntry(selfLink).pipe( + getObjectBySelfLink(selfLink: string): Observable> { + return this.getBySelfLink(selfLink).pipe( map((entry: ObjectCacheEntry) => { if (isNotEmpty(entry.patches)) { const flatPatch: Operation[] = [].concat(...entry.patches.map((patch) => patch.operations)); @@ -105,7 +105,7 @@ export class ObjectCacheService { ); } - private getEntry(selfLink: string): Observable { + getBySelfLink(selfLink: string): Observable { return this.store.pipe( select(entryFromSelfLinkSelector(selfLink)), filter((entry) => this.isValid(entry)), @@ -114,7 +114,7 @@ export class ObjectCacheService { } getRequestUUIDBySelfLink(selfLink: string): Observable { - return this.getEntry(selfLink).pipe( + return this.getBySelfLink(selfLink).pipe( map((entry: ObjectCacheEntry) => entry.requestUUID), distinctUntilChanged()); } @@ -147,7 +147,7 @@ export class ObjectCacheService { */ getList(selfLinks: string[]): Observable>> { return observableCombineLatest( - selfLinks.map((selfLink: string) => this.getBySelfLink(selfLink)) + selfLinks.map((selfLink: string) => this.getObjectBySelfLink(selfLink)) ); } diff --git a/src/app/core/cache/server-sync-buffer.effects.ts b/src/app/core/cache/server-sync-buffer.effects.ts index 0d7392e555..2e11f15540 100644 --- a/src/app/core/cache/server-sync-buffer.effects.ts +++ b/src/app/core/cache/server-sync-buffer.effects.ts @@ -95,7 +95,7 @@ export class ServerSyncBufferEffects { * @returns {Observable} ApplyPatchObjectCacheAction to be dispatched */ private applyPatch(href: string): Observable { - const patchObject = this.objectCache.getBySelfLink(href).pipe(take(1)); + const patchObject = this.objectCache.getObjectBySelfLink(href).pipe(take(1)); return patchObject.pipe( map((object) => { diff --git a/src/app/core/data/comcol-data.service.spec.ts b/src/app/core/data/comcol-data.service.spec.ts index 784075c855..d93600a06a 100644 --- a/src/app/core/data/comcol-data.service.spec.ts +++ b/src/app/core/data/comcol-data.service.spec.ts @@ -94,7 +94,7 @@ describe('ComColDataService', () => { function initMockObjectCacheService(): ObjectCacheService { return jasmine.createSpyObj('objectCache', { - getByUUID: cold('d-', { + getObjectByUUID: cold('d-', { d: { _links: { [LINK_NAME]: scopedEndpoint @@ -159,7 +159,7 @@ describe('ComColDataService', () => { it('should fetch the scope Community from the cache', () => { scheduler.schedule(() => service.getBrowseEndpoint(options).subscribe()); scheduler.flush(); - expect(objectCache.getByUUID).toHaveBeenCalledWith(scopeID); + expect(objectCache.getObjectByUUID).toHaveBeenCalledWith(scopeID); }); it('should return the endpoint to fetch resources within the given scope', () => { diff --git a/src/app/core/data/comcol-data.service.ts b/src/app/core/data/comcol-data.service.ts index 693b8af58b..9d82cc5047 100644 --- a/src/app/core/data/comcol-data.service.ts +++ b/src/app/core/data/comcol-data.service.ts @@ -49,7 +49,7 @@ export abstract class ComColDataService extends DataS ); const successResponses = responses.pipe( filter((response) => response.isSuccessful), - mergeMap(() => this.objectCache.getByUUID(options.scopeID)), + mergeMap(() => this.objectCache.getObjectByUUID(options.scopeID)), map((nc: NormalizedCommunity) => nc._links[linkPath]), filter((href) => isNotEmpty(href)) ); diff --git a/src/app/core/data/data.service.ts b/src/app/core/data/data.service.ts index 42115b6b6c..72af52c4c8 100644 --- a/src/app/core/data/data.service.ts +++ b/src/app/core/data/data.service.ts @@ -139,7 +139,7 @@ export abstract class DataService { * @param {DSpaceObject} object The given object */ update(object: T): Observable> { - const oldVersion$ = this.objectCache.getBySelfLink(object.self); + const oldVersion$ = this.objectCache.getObjectBySelfLink(object.self); return oldVersion$.pipe(take(1), mergeMap((oldVersion: T) => { const operations = this.comparator.diff(oldVersion, object); if (isNotEmpty(operations)) { diff --git a/src/app/core/data/request.service.spec.ts b/src/app/core/data/request.service.spec.ts index b28436f3a8..69d64cf470 100644 --- a/src/app/core/data/request.service.spec.ts +++ b/src/app/core/data/request.service.spec.ts @@ -24,7 +24,7 @@ import { BehaviorSubject } from 'rxjs/internal/BehaviorSubject'; import { MockStore } from '../../shared/testing/mock-store'; import { IndexState } from '../index/index.reducer'; -describe('RequestService', () => { +fdescribe('RequestService', () => { let scheduler: TestScheduler; let service: RequestService; let serviceAsAny: any; @@ -323,6 +323,7 @@ describe('RequestService', () => { describe('in the ObjectCache', () => { beforeEach(() => { (objectCache.hasBySelfLink as any).and.returnValue(true); + spyOn(serviceAsAny, 'hasByHref').and.returnValue(false); }); it('should return true', () => { @@ -332,63 +333,16 @@ describe('RequestService', () => { expect(result).toEqual(expected); }); }); - describe('in the responseCache', () => { + describe('in the request cache', () => { beforeEach(() => { - spyOn(serviceAsAny, 'isReusable').and.returnValue(observableOf(true)); - spyOn(serviceAsAny, 'getByHref').and.returnValue(observableOf(undefined)); + (objectCache.hasBySelfLink as any).and.returnValue(false); + spyOn(serviceAsAny, 'hasByHref').and.returnValue(true); }); + it('should return true', () => { + const result = serviceAsAny.isCachedOrPending(testGetRequest); + const expected = true; - describe('and it\'s a DSOSuccessResponse', () => { - beforeEach(() => { - (serviceAsAny.getByHref as any).and.returnValue(observableOf({ - response: { - isSuccessful: true, - resourceSelfLinks: [ - 'https://rest.api/endpoint/selfLink1', - 'https://rest.api/endpoint/selfLink2' - ] - } - } - )); - }); - - it('should return true if all top level links in the response are cached in the object cache', () => { - (objectCache.hasBySelfLink as any).and.returnValues(false, true, true); - - const result = serviceAsAny.isCachedOrPending(testGetRequest); - const expected = true; - - expect(result).toEqual(expected); - }); - it('should return false if not all top level links in the response are cached in the object cache', () => { - (objectCache.hasBySelfLink as any).and.returnValues(false, true, false); - spyOn(service, 'isPending').and.returnValue(false); - - const result = serviceAsAny.isCachedOrPending(testGetRequest); - const expected = false; - - expect(result).toEqual(expected); - }); - }); - - describe('and it isn\'t a DSOSuccessResponse', () => { - beforeEach(() => { - (objectCache.hasBySelfLink as any).and.returnValue(false); - (service as any).isReusable.and.returnValue(observableOf(true)); - (serviceAsAny.getByHref as any).and.returnValue(observableOf({ - response: { - isSuccessful: true - } - } - )); - }); - - it('should return true', () => { - const result = serviceAsAny.isCachedOrPending(testGetRequest); - const expected = true; - - expect(result).toEqual(expected); - }); + expect(result).toEqual(expected); }); }); }); @@ -462,56 +416,56 @@ describe('RequestService', () => { }); }); - describe('isReusable', () => { - describe('when the given UUID is has no value', () => { - let reusable; + describe('isValid', () => { + describe('when the given UUID has no value', () => { + let valid; beforeEach(() => { const uuid = undefined; - reusable = serviceAsAny.isReusable(uuid); + valid = serviceAsAny.isValid(uuid); }); it('return an observable emitting false', () => { - reusable.subscribe((isReusable) => expect(isReusable).toBe(false)); + valid.subscribe((isValid) => expect(isValid).toBe(false)); }) }); describe('when the given UUID has a value, but no cached entry is found', () => { - let reusable; + let valid; beforeEach(() => { spyOn(service, 'getByUUID').and.returnValue(observableOf(undefined)); const uuid = 'a45bb291-1adb-40d9-b2fc-7ad9080607be'; - reusable = serviceAsAny.isReusable(uuid); + valid = serviceAsAny.isValid(uuid); }); it('return an observable emitting false', () => { - reusable.subscribe((isReusable) => expect(isReusable).toBe(false)); + valid.subscribe((isValid) => expect(isValid).toBe(false)); }) }); describe('when the given UUID has a value, a cached entry is found, but it has no response', () => { - let reusable; + let valid; beforeEach(() => { spyOn(service, 'getByUUID').and.returnValue(observableOf({ response: undefined })); const uuid = '53c9b814-ad8b-4567-9bc1-d9bb6cfba6c8'; - reusable = serviceAsAny.isReusable(uuid); + valid = serviceAsAny.isValid(uuid); }); it('return an observable emitting false', () => { - reusable.subscribe((isReusable) => expect(isReusable).toBe(false)); + valid.subscribe((isValid) => expect(isValid).toBe(false)); }) }); describe('when the given UUID has a value, a cached entry is found, but its response was not successful', () => { - let reusable; + let valid; beforeEach(() => { spyOn(service, 'getByUUID').and.returnValue(observableOf({ response: { isSuccessful: false } })); const uuid = '694c9b32-7b2e-4788-835b-ef3fc2252e6c'; - reusable = serviceAsAny.isReusable(uuid); + valid = serviceAsAny.isValid(uuid); }); it('return an observable emitting false', () => { - reusable.subscribe((isReusable) => expect(isReusable).toBe(false)); + valid.subscribe((isValid) => expect(isValid).toBe(false)); }) }); - describe('when the given UUID has a value, a cached entry is found, its response was successful, but the response is outdated', () => { - let reusable; + fdescribe('when the given UUID has a value, a cached entry is found, its response was successful, but the response is outdated', () => { + let valid; const now = 100000; const timeAdded = 99899; const msToLive = 100; @@ -528,16 +482,16 @@ describe('RequestService', () => { } })); const uuid = 'f9b85788-881c-4994-86b6-bae8dad024d2'; - reusable = serviceAsAny.isReusable(uuid); + valid = serviceAsAny.isValid(uuid); }); it('return an observable emitting false', () => { - reusable.subscribe((isReusable) => expect(isReusable).toBe(false)); + valid.subscribe((isValid) => expect(isValid).toBe(false)); }) }); describe('when the given UUID has a value, a cached entry is found, its response was successful, and the response is not outdated', () => { - let reusable; + let valid; const now = 100000; const timeAdded = 99999; const msToLive = 100; @@ -554,11 +508,11 @@ describe('RequestService', () => { } })); const uuid = 'f9b85788-881c-4994-86b6-bae8dad024d2'; - reusable = serviceAsAny.isReusable(uuid); + valid = serviceAsAny.isValid(uuid); }); it('return an observable emitting true', () => { - reusable.subscribe((isReusable) => expect(isReusable).toBe(true)); + valid.subscribe((isValid) => expect(isValid).toBe(true)); }) }) }) diff --git a/src/app/core/data/request.service.ts b/src/app/core/data/request.service.ts index 93a7a10506..4fd7b84196 100644 --- a/src/app/core/data/request.service.ts +++ b/src/app/core/data/request.service.ts @@ -1,36 +1,25 @@ -import { merge as observableMerge, Observable, of as observableOf } from 'rxjs'; -import { - distinctUntilChanged, - filter, - find, - first, - map, - mergeMap, - reduce, - startWith, - switchMap, - take, - tap -} from 'rxjs/operators'; -import { race as observableRace } from 'rxjs'; +import { Observable, race as observableRace } from 'rxjs'; +import { filter, mergeMap, take } from 'rxjs/operators'; import { Injectable } from '@angular/core'; import { createSelector, MemoizedSelector, select, Store } from '@ngrx/store'; -import { hasNoValue, hasValue, isNotEmpty, isNotUndefined } from '../../shared/empty.util'; +import { hasValue, isNotEmpty } from '../../shared/empty.util'; import { CacheableObject } from '../cache/object-cache.reducer'; import { ObjectCacheService } from '../cache/object-cache.service'; -import { DSOSuccessResponse, RestResponse } from '../cache/response.models'; import { coreSelector, CoreState } from '../core.reducers'; import { IndexName, IndexState } from '../index/index.reducer'; import { pathSelector } from '../shared/selectors'; import { UUIDService } from '../shared/uuid.service'; -import { RequestConfigureAction, RequestExecuteAction, RequestRemoveAction } from './request.actions'; -import { GetRequest, RestRequest } from './request.models'; +import { + RequestConfigureAction, + RequestExecuteAction, + RequestRemoveAction +} from './request.actions'; +import { EndpointMapRequest, GetRequest, RestRequest } from './request.models'; import { RequestEntry } from './request.reducer'; import { CommitSSBAction } from '../cache/server-sync-buffer.actions'; import { RestRequestMethod } from './rest-request-method'; -import { getResponseFromEntry } from '../shared/operators'; import { AddToIndexAction, RemoveFromIndexBySubstringAction } from '../index/index.actions'; @Injectable() @@ -104,7 +93,6 @@ export class RequestService { .subscribe((re: RequestEntry) => { isPending = (hasValue(re) && !re.completed) }); - return isPending; } @@ -183,31 +171,11 @@ export class RequestService { * @param {GetRequest} request The request to check * @returns {boolean} True if the request is cached or still pending */ - private isCachedOrPending(request: GetRequest) { - let isCached = this.objectCache.hasBySelfLink(request.href); - if (isCached) { - const responses: Observable = this.isReusable(request.uuid).pipe( - filter((reusable: boolean) => reusable), - switchMap(() => { - return this.getByHref(request.href).pipe( - getResponseFromEntry(), - take(1) - ); - } - )); + private isCachedOrPending(request: GetRequest): boolean { + const inReqCache = this.hasByHref(request.href); + const inObjCache = this.objectCache.hasBySelfLink(request.href); + const isCached = inReqCache || inObjCache; - const errorResponses = responses.pipe(filter((response) => !response.isSuccessful), map(() => true)); // TODO add a configurable number of retries in case of an error. - const dsoSuccessResponses = responses.pipe( - filter((response) => response.isSuccessful && hasValue((response as DSOSuccessResponse).resourceSelfLinks)), - map((response: DSOSuccessResponse) => response.resourceSelfLinks), - map((resourceSelfLinks: string[]) => resourceSelfLinks - .every((selfLink) => this.objectCache.hasBySelfLink(selfLink)) - )); - - const otherSuccessResponses = responses.pipe(filter((response) => response.isSuccessful && !hasValue((response as DSOSuccessResponse).resourceSelfLinks)), map(() => true)); - - observableMerge(errorResponses, otherSuccessResponses, dsoSuccessResponses).subscribe((c) => isCached = c); - } const isPending = this.isPending(request); return isCached || isPending; } @@ -230,7 +198,7 @@ export class RequestService { */ private trackRequestsOnTheirWayToTheStore(request: GetRequest) { this.requestsOnTheirWayToTheStore = [...this.requestsOnTheirWayToTheStore, request.href]; - this.store.pipe(select(this.entryFromUUIDSelector(request.href)), + this.getByHref(request.href).pipe( filter((re: RequestEntry) => hasValue(re)), take(1) ).subscribe((re: RequestEntry) => { @@ -247,31 +215,40 @@ export class RequestService { } /** - * Check whether a Response should still be cached + * Check whether a cached response should still be valid * - * @param uuid - * the uuid of the entry to check + * @param entry + * the entry to check * @return boolean - * false if the uuid has no value, no entry could be found, the response was nog successful or its time to - * live has exceeded, true otherwise + * false if the uuid has no value, the response was not successful or its time to + * live was exceeded, true otherwise */ - private isReusable(uuid: string): Observable { - if (hasNoValue(uuid)) { - return observableOf(false); + private isValid(entry: RequestEntry): boolean { + if (hasValue(entry) && entry.completed && entry.response.isSuccessful) { + const timeOutdated = entry.response.timeAdded + entry.request.responseMsToLive; + const isOutDated = new Date().getTime() > timeOutdated; + return !isOutDated; } else { - const requestEntry$ = this.getByUUID(uuid); - return requestEntry$.pipe( - filter((entry: RequestEntry) => hasValue(entry) && hasValue(entry.response)), - map((entry: RequestEntry) => { - if (hasValue(entry) && entry.response.isSuccessful) { - const timeOutdated = entry.response.timeAdded + entry.request.responseMsToLive; - const isOutDated = new Date().getTime() > timeOutdated; - return !isOutDated; - } else { - return false; - } - }) - ); + return false; } } + + /** + * Check whether the request with the specified href is cached + * + * @param href + * The link of the request to check + * @return boolean + * true if the request with the specified href is cached, + * false otherwise + */ + hasByHref(href: string): boolean { + let result = false; + this.getByHref(href).pipe( + take(1) + ).subscribe((requestEntry: RequestEntry) => result = this.isValid(requestEntry)); + + return result; + } + }