fixed caching

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<T>(href)),
+          switchMap((href: string) => this.objectCache.getObjectBySelfLink<T>(href)),
           startWith(undefined)),
         requestEntry$.pipe(
           getResourceLinksFromResponse(),
           switchMap((resourceSelfLinks: string[]) => {
             if (isNotEmpty(resourceSelfLinks)) {
-              return this.objectCache.getBySelfLink<T>(resourceSelfLinks[0]);
+              return this.objectCache.getObjectBySelfLink<T>(resourceSelfLinks[0]);
             } else {
               return observableOf(undefined);
             }

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

src/app/core/cache/object-cache.service.ts

@@ -78,16 +78,16 @@ export class ObjectCacheService {
    * @return Observable<T>
    *    An observable of the requested object
    */
-  getByUUID<T extends CacheableObject>(uuid: string): Observable<NormalizedObject<T>> {
+  getObjectByUUID<T extends CacheableObject>(uuid: string): Observable<NormalizedObject<T>> {
     return this.store.pipe(
       select(selfLinkFromUuidSelector(uuid)),
-      mergeMap((selfLink: string) => this.getBySelfLink(selfLink)
+      mergeMap((selfLink: string) => this.getObjectBySelfLink(selfLink)
       )
     )
   }
 
-  getBySelfLink<T extends CacheableObject>(selfLink: string): Observable<NormalizedObject<T>> {
-    return this.getEntry(selfLink).pipe(
+  getObjectBySelfLink<T extends CacheableObject>(selfLink: string): Observable<NormalizedObject<T>> {
+    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<ObjectCacheEntry> {
+  getBySelfLink(selfLink: string): Observable<ObjectCacheEntry> {
     return this.store.pipe(
       select(entryFromSelfLinkSelector(selfLink)),
       filter((entry) => this.isValid(entry)),
@@ -114,7 +114,7 @@ export class ObjectCacheService {
   }
 
   getRequestUUIDBySelfLink(selfLink: string): Observable<string> {
-    return this.getEntry(selfLink).pipe(
+    return this.getBySelfLink(selfLink).pipe(
       map((entry: ObjectCacheEntry) => entry.requestUUID),
       distinctUntilChanged());
   }
@@ -147,7 +147,7 @@ export class ObjectCacheService {
    */
   getList<T extends CacheableObject>(selfLinks: string[]): Observable<Array<NormalizedObject<T>>> {
     return observableCombineLatest(
-      selfLinks.map((selfLink: string) => this.getBySelfLink<T>(selfLink))
+      selfLinks.map((selfLink: string) => this.getObjectBySelfLink<T>(selfLink))
     );
   }
 

src/app/core/cache/server-sync-buffer.effects.ts

@@ -95,7 +95,7 @@ export class ServerSyncBufferEffects {
    * @returns {Observable<Action>} ApplyPatchObjectCacheAction to be dispatched
    */
   private applyPatch(href: string): Observable<Action> {
-    const patchObject = this.objectCache.getBySelfLink(href).pipe(take(1));
+    const patchObject = this.objectCache.getObjectBySelfLink(href).pipe(take(1));
 
     return patchObject.pipe(
       map((object) => {

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', () => {

src/app/core/data/comcol-data.service.ts

@@ -49,7 +49,7 @@ export abstract class ComColDataService<T extends CacheableObject> 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))
       );

src/app/core/data/data.service.ts

@@ -139,7 +139,7 @@ export abstract class DataService<T extends CacheableObject> {
    * @param {DSpaceObject} object The given object
    */
   update(object: T): Observable<RemoteData<T>> {
-    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)) {

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,57 +333,11 @@ describe('RequestService', () => {
           expect(result).toEqual(expected);
         });
       });
-      describe('in the responseCache', () => {
-        beforeEach(() => {
-          spyOn(serviceAsAny, 'isReusable').and.returnValue(observableOf(true));
-          spyOn(serviceAsAny, 'getByHref').and.returnValue(observableOf(undefined));
-        });
-
-        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', () => {
+      describe('in the request cache', () => {
         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
-                }
-              }
-            ));
+          spyOn(serviceAsAny, 'hasByHref').and.returnValue(true);
         });
-
         it('should return true', () => {
           const result = serviceAsAny.isCachedOrPending(testGetRequest);
           const expected = true;
@@ -391,7 +346,6 @@ describe('RequestService', () => {
         });
       });
     });
-    });
 
     describe('when the request is pending', () => {
       beforeEach(() => {
@@ -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));
       })
     })
   })

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<RestResponse> = 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<boolean> {
-    if (hasNoValue(uuid)) {
-      return observableOf(false);
-    } 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) {
+  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 {
       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;
   }
+
 }