Merge pull request #1813 from atmire/w2p-94207_Fix-resource-policy-cache-issues

Fix ResourcePolicy cache issues

Author: tdonohue

src/app/core/cache/builders/remote-data-build.service.spec.ts

@@ -1,4 +1,4 @@
-import { createSuccessfulRemoteDataObject } from '../../../shared/remote-data.utils';
+import { createFailedRemoteDataObject, createPendingRemoteDataObject, createSuccessfulRemoteDataObject } from '../../../shared/remote-data.utils';
 import { buildPaginatedList, PaginatedList } from '../../data/paginated-list.model';
 import { Item } from '../../shared/item.model';
 import { PageInfo } from '../../shared/page-info.model';
@@ -19,6 +19,8 @@ import { HALLink } from '../../shared/hal-link.model';
 import { RequestEntryState } from '../../data/request-entry-state.model';
 import { RequestEntry } from '../../data/request-entry.model';
 import { cold } from 'jasmine-marbles';
+import { TestScheduler } from 'rxjs/testing';
+import { fakeAsync, tick } from '@angular/core/testing';
 
 describe('RemoteDataBuildService', () => {
   let service: RemoteDataBuildService;
@@ -763,4 +765,95 @@ describe('RemoteDataBuildService', () => {
       });
     });
   });
+
+  describe('buildFromRequestUUIDAndAwait', () => {
+    let testScheduler;
+
+    let callback: jasmine.Spy;
+    let buildFromRequestUUIDSpy;
+
+    const BOOLEAN = { t: true, f: false };
+
+    const MOCK_PENDING_RD = createPendingRemoteDataObject();
+    const MOCK_SUCCEEDED_RD = createSuccessfulRemoteDataObject({});
+    const MOCK_FAILED_RD = createFailedRemoteDataObject('failed');
+
+    const RDs = {
+      p: MOCK_PENDING_RD,
+      s: MOCK_SUCCEEDED_RD,
+      f: MOCK_FAILED_RD,
+    };
+
+
+    beforeEach(() => {
+      testScheduler = new TestScheduler((actual, expected) => {
+        expect(actual).toEqual(expected);
+      });
+
+      callback = jasmine.createSpy('callback');
+      callback.and.returnValue(observableOf(undefined));
+      buildFromRequestUUIDSpy = spyOn(service, 'buildFromRequestUUID').and.callThrough();
+    });
+
+    it('should patch through href & followLinks to buildFromRequestUUID', () => {
+      buildFromRequestUUIDSpy.and.returnValue(observableOf(MOCK_SUCCEEDED_RD));
+      service.buildFromRequestUUIDAndAwait('some-href', callback, ...linksToFollow);
+      expect(buildFromRequestUUIDSpy).toHaveBeenCalledWith('some-href', ...linksToFollow);
+    });
+
+    it('should trigger the callback on successful RD', (done) => {
+      buildFromRequestUUIDSpy.and.returnValue(observableOf(MOCK_SUCCEEDED_RD));
+
+      service.buildFromRequestUUIDAndAwait('some-href', callback).subscribe(rd => {
+        expect(rd).toBe(MOCK_SUCCEEDED_RD);
+        expect(callback).toHaveBeenCalled();
+        done();
+      });
+    });
+
+    it('should trigger the callback on successful RD even if nothing subscribes to the returned Observable', fakeAsync(() => {
+      buildFromRequestUUIDSpy.and.returnValue(observableOf(MOCK_SUCCEEDED_RD));
+
+      service.buildFromRequestUUIDAndAwait('some-href', callback);
+      tick();
+
+      expect(callback).toHaveBeenCalled();
+    }));
+
+    it('should not trigger the callback on pending RD', (done) => {
+      buildFromRequestUUIDSpy.and.returnValue(observableOf(MOCK_PENDING_RD));
+
+      service.buildFromRequestUUIDAndAwait('some-href', callback).subscribe(rd => {
+        expect(rd).toBe(MOCK_PENDING_RD);
+        expect(callback).not.toHaveBeenCalled();
+        done();
+      });
+    });
+
+    it('should not trigger the callback on failed RD', (done) => {
+      buildFromRequestUUIDSpy.and.returnValue(observableOf(MOCK_FAILED_RD));
+
+      service.buildFromRequestUUIDAndAwait('some-href', callback).subscribe(rd => {
+        expect(rd).toBe(MOCK_FAILED_RD);
+        expect(callback).not.toHaveBeenCalled();
+        done();
+      });
+    });
+
+    it('should only emit after the callback is done', () => {
+      testScheduler.run(({ cold: tsCold, expectObservable }) => {
+        buildFromRequestUUIDSpy.and.returnValue(
+          tsCold('-p----s', RDs)
+        );
+        callback.and.returnValue(
+          tsCold('      --t', BOOLEAN)
+        );
+
+        const done$ = service.buildFromRequestUUIDAndAwait('some-href', callback);
+        expectObservable(done$).toBe(
+          '       -p------s', RDs       // resulting duration between pending & successful includes the callback
+        );
+      });
+    });
+  });
 });

src/app/core/cache/builders/remote-data-build.service.ts

@@ -1,5 +1,6 @@
 import { Injectable } from '@angular/core';
 import {
+  AsyncSubject,
   combineLatest as observableCombineLatest,
   Observable,
   of as observableOf,
@@ -24,6 +25,7 @@ import { hasSucceeded, isStale, RequestEntryState } from '../../data/request-ent
 import { getRequestFromRequestHref, getRequestFromRequestUUID } from '../../shared/request.operators';
 import { RequestEntry } from '../../data/request-entry.model';
 import { ResponseState } from '../../data/response-state.model';
+import { getFirstCompletedRemoteData } from '../../shared/operators';
 
 @Injectable()
 export class RemoteDataBuildService {
@@ -188,6 +190,49 @@ export class RemoteDataBuildService {
     return this.toRemoteDataObservable<T>(requestEntry$, payload$);
   }
 
+  /**
+   * Creates a {@link RemoteData} object for a rest request and its response
+   * and emits it only after the callback function is completed.
+   *
+   * @param requestUUID$    The UUID of the request we want to retrieve
+   * @param callback        A function that returns an Observable. It will only be called once the request has succeeded.
+   *                        Then, the response will only be emitted after this callback function has emitted.
+   * @param linksToFollow   List of {@link FollowLinkConfig} that indicate which {@link HALLink}s should be automatically resolved
+   */
+  buildFromRequestUUIDAndAwait<T>(requestUUID$: string | Observable<string>, callback: (rd?: RemoteData<T>) => Observable<unknown>, ...linksToFollow: FollowLinkConfig<any>[]): Observable<RemoteData<T>> {
+    const response$ = this.buildFromRequestUUID(requestUUID$, ...linksToFollow);
+
+    const callbackDone$ = new AsyncSubject<boolean>();
+    response$.pipe(
+      getFirstCompletedRemoteData(),
+      switchMap((rd: RemoteData<any>) => {
+        if (rd.hasSucceeded) {
+          // if the request succeeded, execute the callback
+          return callback(rd);
+        } else {
+          // otherwise, emit right away so the subscription doesn't stick around
+          return [true];
+        }
+      }),
+    ).subscribe(() => {
+      callbackDone$.next(true);
+      callbackDone$.complete();
+    });
+
+    return response$.pipe(
+      switchMap((rd: RemoteData<any>) => {
+        if (rd.hasSucceeded) {
+          // if the request succeeded, wait for the callback to finish
+          return callbackDone$.pipe(
+            map(() => rd),
+          );
+        } else {
+          return [rd];
+        }
+      })
+    );
+  }
+
   /**
    * Creates a {@link RemoteData} object for a rest request and its response
    *

src/app/core/data/base/delete-data.spec.ts

@@ -22,7 +22,7 @@ import { RequestEntryState } from '../request-entry-state.model';
 import { DeleteData, DeleteDataImpl } from './delete-data';
 import { NotificationsService } from '../../../shared/notifications/notifications.service';
 import { createFailedRemoteDataObject, createSuccessfulRemoteDataObject, createSuccessfulRemoteDataObject$ } from '../../../shared/remote-data.utils';
-import { fakeAsync, tick } from '@angular/core/testing';
+import { RestRequestMethod } from '../rest-request-method';
 
 /**
  * Tests whether calls to `DeleteData` methods are correctly patched through in a concrete data service that implements it
@@ -63,8 +63,6 @@ export function testDeleteDataImplementation(serviceFactory: () => DeleteData<an
 
 const endpoint = 'https://rest.api/core';
 
-const BOOLEAN = { f: false, t: true };
-
 class TestService extends DeleteDataImpl<any> {
   constructor(
     protected requestService: RequestService,
@@ -155,13 +153,13 @@ describe('DeleteDataImpl', () => {
     let MOCK_FAILED_RD;
 
     let invalidateByHrefSpy: jasmine.Spy;
-    let buildFromRequestUUIDSpy: jasmine.Spy;
+    let buildFromRequestUUIDAndAwaitSpy: jasmine.Spy;
     let getIDHrefObsSpy: jasmine.Spy;
     let deleteByHrefSpy: jasmine.Spy;
 
     beforeEach(() => {
       invalidateByHrefSpy = spyOn(service, 'invalidateByHref').and.returnValue(observableOf(true));
-      buildFromRequestUUIDSpy = spyOn(rdbService, 'buildFromRequestUUID').and.callThrough();
+      buildFromRequestUUIDAndAwaitSpy = spyOn(rdbService, 'buildFromRequestUUIDAndAwait').and.callThrough();
       getIDHrefObsSpy = spyOn(service, 'getIDHrefObs').and.callThrough();
       deleteByHrefSpy = spyOn(service, 'deleteByHref').and.callThrough();
 
@@ -171,7 +169,7 @@ describe('DeleteDataImpl', () => {
 
     it('should retrieve href by ID and call deleteByHref', () => {
       getIDHrefObsSpy.and.returnValue(observableOf('some-href'));
-      buildFromRequestUUIDSpy.and.returnValue(createSuccessfulRemoteDataObject$({}));
+      buildFromRequestUUIDAndAwaitSpy.and.returnValue(createSuccessfulRemoteDataObject$({}));
 
       service.delete('some-id', ['a', 'b', 'c']).subscribe(rd => {
         expect(getIDHrefObsSpy).toHaveBeenCalledWith('some-id');
@@ -180,64 +178,51 @@ describe('DeleteDataImpl', () => {
     });
 
     describe('deleteByHref', () => {
-      it('should call invalidateByHref if the DELETE request succeeds', (done) => {
-        buildFromRequestUUIDSpy.and.returnValue(observableOf(MOCK_SUCCEEDED_RD));
-
-        service.deleteByHref('some-href').subscribe(rd => {
-          expect(rd).toBe(MOCK_SUCCEEDED_RD);
-          expect(invalidateByHrefSpy).toHaveBeenCalled();
+      it('should send a DELETE request', (done) => {
+        buildFromRequestUUIDAndAwaitSpy.and.returnValue(observableOf(MOCK_SUCCEEDED_RD));
+
+        service.deleteByHref('some-href').subscribe(() => {
+          expect(requestService.send).toHaveBeenCalledWith(jasmine.objectContaining({
+            method: RestRequestMethod.DELETE,
+            href: 'some-href',
+          }));
           done();
         });
       });
 
-      it('should call invalidateByHref even if not subscribing to returned Observable', fakeAsync(() => {
-        buildFromRequestUUIDSpy.and.returnValue(observableOf(MOCK_SUCCEEDED_RD));
-
-        service.deleteByHref('some-href');
-        tick();
+      it('should include the virtual metadata to be copied in the DELETE request', (done) => {
+        buildFromRequestUUIDAndAwaitSpy.and.returnValue(observableOf(MOCK_SUCCEEDED_RD));
 
-        expect(invalidateByHrefSpy).toHaveBeenCalled();
-      }));
-
-      it('should not call invalidateByHref if the DELETE request fails', (done) => {
-        buildFromRequestUUIDSpy.and.returnValue(observableOf(MOCK_FAILED_RD));
-
-        service.deleteByHref('some-href').subscribe(rd => {
-          expect(rd).toBe(MOCK_FAILED_RD);
-          expect(invalidateByHrefSpy).not.toHaveBeenCalled();
+        service.deleteByHref('some-href', ['a', 'b', 'c']).subscribe(() => {
+          expect(requestService.send).toHaveBeenCalledWith(jasmine.objectContaining({
+            method: RestRequestMethod.DELETE,
+            href: 'some-href?copyVirtualMetadata=a&copyVirtualMetadata=b&copyVirtualMetadata=c',
+          }));
           done();
         });
       });
 
-      it('should wait for invalidateByHref before emitting', () => {
-        testScheduler.run(({ cold, expectObservable }) => {
-          buildFromRequestUUIDSpy.and.returnValue(
-            cold('(r|)', { r: MOCK_SUCCEEDED_RD})      // RD emits right away
-          );
-          invalidateByHrefSpy.and.returnValue(
-            cold('----(t|)', BOOLEAN)                  // but we pretend that setting requests to stale takes longer
+      it('should invalidate the currently cached object', (done) => {
+        service.deleteByHref('some-href').subscribe(() => {
+          expect(buildFromRequestUUIDAndAwaitSpy).toHaveBeenCalledWith(
+            requestService.generateRequestId(),
+            jasmine.anything(),
           );
 
-          const done$ = service.deleteByHref('some-href');
-          expectObservable(done$).toBe(
-            '----(r|)', { r: MOCK_SUCCEEDED_RD}        // ...and expect the returned Observable to wait until that's done
-          );
+          const callback = (rdbService.buildFromRequestUUIDAndAwait as jasmine.Spy).calls.argsFor(0)[1];
+          callback();
+          expect(service.invalidateByHref).toHaveBeenCalledWith('some-href');
+
+          done();
         });
       });
 
-      it('should wait for the DELETE request to resolve before emitting', () => {
-        testScheduler.run(({ cold, expectObservable }) => {
-          buildFromRequestUUIDSpy.and.returnValue(
-            cold('----(r|)', { r: MOCK_SUCCEEDED_RD})   // the request takes a while
-          );
-          invalidateByHrefSpy.and.returnValue(
-            cold('(t|)', BOOLEAN)                       // but we pretend that setting to stale happens sooner
-          );                                            // e.g.: maybe already stale before this call?
+      it('should return the RemoteData of the response', (done) => {
+        buildFromRequestUUIDAndAwaitSpy.and.returnValue(observableOf(MOCK_SUCCEEDED_RD));
 
-          const done$ = service.deleteByHref('some-href');
-          expectObservable(done$).toBe(
-            '----(r|)', { r: MOCK_SUCCEEDED_RD}         // ...and expect the returned Observable to wait for the request
-          );
+        service.deleteByHref('some-href').subscribe(rd => {
+          expect(rd).toBe(MOCK_SUCCEEDED_RD);
+          done();
         });
       });
     });

src/app/core/data/base/delete-data.ts

@@ -6,13 +6,12 @@
  * http://www.dspace.org/license/
  */
 import { CacheableObject } from '../../cache/cacheable-object.model';
-import { AsyncSubject, combineLatest, Observable } from 'rxjs';
+import { Observable } from 'rxjs';
 import { RemoteData } from '../remote-data';
 import { NoContent } from '../../shared/NoContent.model';
-import { filter, map, switchMap } from 'rxjs/operators';
+import { switchMap } from 'rxjs/operators';
 import { DeleteRequest } from '../request.models';
 import { hasNoValue, hasValue } from '../../../shared/empty.util';
-import { getFirstCompletedRemoteData } from '../../shared/operators';
 import { RequestService } from '../request.service';
 import { RemoteDataBuildService } from '../../cache/builders/remote-data-build.service';
 import { HALEndpointService } from '../../shared/hal-endpoint.service';
@@ -83,26 +82,6 @@ export class DeleteDataImpl<T extends CacheableObject> extends IdentifiableDataS
     }
     this.requestService.send(request);
 
-    const response$ = this.rdbService.buildFromRequestUUID(requestId);
-
-    const invalidated$ = new AsyncSubject<boolean>();
-    response$.pipe(
-      getFirstCompletedRemoteData(),
-      switchMap((rd: RemoteData<NoContent>) => {
-        if (rd.hasSucceeded) {
-          return this.invalidateByHref(href);
-        } else {
-          return [true];
-        }
-      })
-    ).subscribe(() => {
-      invalidated$.next(true);
-      invalidated$.complete();
-    });
-
-    return combineLatest([response$, invalidated$]).pipe(
-      filter(([_, invalidated]) => invalidated),
-      map(([response, _]) => response),
-    );
+    return this.rdbService.buildFromRequestUUIDAndAwait(requestId, () => this.invalidateByHref(href));
   }
 }

src/app/core/data/bitstream-format-data.service.spec.ts

@@ -54,7 +54,8 @@ describe('BitstreamFormatDataService', () => {
   function initTestService(halService) {
     rd = createSuccessfulRemoteDataObject({});
     rdbService = jasmine.createSpyObj('rdbService', {
-      buildFromRequestUUID: observableOf(rd)
+      buildFromRequestUUID: observableOf(rd),
+      buildFromRequestUUIDAndAwait: observableOf(rd),
     });
 
     return new BitstreamFormatDataService(