90252: Invalidate requests containing DSO on DataService.delete

Keep track of a list of request UUIDs in the object cache (most recent in front)
When deleting a DSO, mark all of these as stale

src/app/core/cache/object-cache.reducer.spec.ts

@@ -41,7 +41,7 @@ describe('objectCacheReducer', () => {
       alternativeLinks: [altLink1, altLink2],
       timeCompleted: new Date().getTime(),
       msToLive: 900000,
-      requestUUID: requestUUID1,
+      requestUUIDs: [requestUUID1],
       patches: [],
       isDirty: false,
     },
@@ -55,7 +55,7 @@ describe('objectCacheReducer', () => {
       alternativeLinks: [altLink3, altLink4],
       timeCompleted: new Date().getTime(),
       msToLive: 900000,
-      requestUUID: selfLink2,
+      requestUUIDs: [selfLink2],
       patches: [],
       isDirty: false
     }

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

@@ -63,9 +63,11 @@ export class ObjectCacheEntry implements CacheEntry {
   msToLive: number;
 
   /**
-   * The UUID of the request that caused this entry to be added
+   * The UUIDs of the requests that caused this entry to be added
+   * New UUIDs should be added to the front of the array
+   * to make retrieving the latest UUID easier.
    */
-  requestUUID: string;
+  requestUUIDs: string[];
 
   /**
    * An array of patches that were made on the client side to this entry, but haven't been sent to the server yet
@@ -156,11 +158,11 @@ function addToObjectCache(state: ObjectCacheState, action: AddToObjectCacheActio
       data: action.payload.objectToCache,
       timeCompleted: action.payload.timeCompleted,
       msToLive: action.payload.msToLive,
-      requestUUID: action.payload.requestUUID,
+      requestUUIDs: [action.payload.requestUUID, ...(existing.requestUUIDs || [])],
       isDirty: isNotEmpty(existing.patches),
       patches: existing.patches || [],
       alternativeLinks: [...(existing.alternativeLinks || []), ...newAltLinks]
-    }
+    } as ObjectCacheEntry
   });
 }
 

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

@@ -197,7 +197,7 @@ export class ObjectCacheService {
    */
   getRequestUUIDBySelfLink(selfLink: string): Observable<string> {
     return this.getByHref(selfLink).pipe(
-      map((entry: ObjectCacheEntry) => entry.requestUUID),
+      map((entry: ObjectCacheEntry) => entry.requestUUIDs[0]),
       distinctUntilChanged());
   }
 
@@ -282,7 +282,7 @@ export class ObjectCacheService {
     let result = false;
     this.getByHref(href).subscribe((entry: ObjectCacheEntry) => {
       if (isNotEmpty(requestUUID)) {
-        result = entry.requestUUID === requestUUID;
+        result = entry.requestUUIDs[0] === requestUUID;  // todo: may make more sense to do entry.requestUUIDs.includes(requestUUID) instead
       } else {
         result = true;
       }

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

@@ -37,7 +37,12 @@ describe('BitstreamFormatDataService', () => {
     }
   } as Store<CoreState>;
 
-  const objectCache = {} as ObjectCacheService;
+  const requestUUIDs = ['some', 'uuid'];
+
+  const objectCache = jasmine.createSpyObj('objectCache', {
+    getByHref: observableOf({ requestUUIDs })
+  }) as ObjectCacheService;
+
   const halEndpointService = {
     getEndpoint(linkPath: string): Observable<string> {
       return cold('a', { a: bitstreamFormatsEndpoint });
@@ -76,6 +81,7 @@ describe('BitstreamFormatDataService', () => {
         send: {},
         getByHref: observableOf(responseCacheEntry),
         getByUUID: cold('a', { a: responseCacheEntry }),
+        setStaleByUUID: observableOf(true),
         generateRequestId: 'request-id',
         removeByHrefSubstring: {}
       });
@@ -96,6 +102,7 @@ describe('BitstreamFormatDataService', () => {
         send: {},
         getByHref: observableOf(responseCacheEntry),
         getByUUID: cold('a', { a: responseCacheEntry }),
+        setStaleByUUID: observableOf(true),
         generateRequestId: 'request-id',
         removeByHrefSubstring: {}
       });
@@ -118,6 +125,7 @@ describe('BitstreamFormatDataService', () => {
         send: {},
         getByHref: observableOf(responseCacheEntry),
         getByUUID: cold('a', { a: responseCacheEntry }),
+        setStaleByUUID: observableOf(true),
         generateRequestId: 'request-id',
         removeByHrefSubstring: {}
       });
@@ -139,6 +147,7 @@ describe('BitstreamFormatDataService', () => {
         send: {},
         getByHref: observableOf(responseCacheEntry),
         getByUUID: cold('a', { a: responseCacheEntry }),
+        setStaleByUUID: observableOf(true),
         generateRequestId: 'request-id',
         removeByHrefSubstring: {}
       });
@@ -163,6 +172,7 @@ describe('BitstreamFormatDataService', () => {
         send: {},
         getByHref: observableOf(responseCacheEntry),
         getByUUID: cold('a', { a: responseCacheEntry }),
+        setStaleByUUID: observableOf(true),
         generateRequestId: 'request-id',
         removeByHrefSubstring: {}
       });
@@ -186,6 +196,7 @@ describe('BitstreamFormatDataService', () => {
         send: {},
         getByHref: observableOf(responseCacheEntry),
         getByUUID: cold('a', { a: responseCacheEntry }),
+        setStaleByUUID: observableOf(true),
         generateRequestId: 'request-id',
         removeByHrefSubstring: {}
       });
@@ -209,6 +220,7 @@ describe('BitstreamFormatDataService', () => {
         send: {},
         getByHref: observableOf(responseCacheEntry),
         getByUUID: cold('a', { a: responseCacheEntry }),
+        setStaleByUUID: observableOf(true),
         generateRequestId: 'request-id',
         removeByHrefSubstring: {}
       });
@@ -231,6 +243,7 @@ describe('BitstreamFormatDataService', () => {
         send: {},
         getByHref: observableOf(responseCacheEntry),
         getByUUID: cold('a', { a: responseCacheEntry }),
+        setStaleByUUID: observableOf(true),
         generateRequestId: 'request-id',
         removeByHrefSubstring: {}
       });
@@ -253,6 +266,7 @@ describe('BitstreamFormatDataService', () => {
         send: {},
         getByHref: observableOf(responseCacheEntry),
         getByUUID: cold('a', { a: responseCacheEntry }),
+        setStaleByUUID: observableOf(true),
         generateRequestId: 'request-id',
         removeByHrefSubstring: {}
       });
@@ -273,6 +287,7 @@ describe('BitstreamFormatDataService', () => {
         send: {},
         getByHref: observableOf(responseCacheEntry),
         getByUUID: hot('a', { a: responseCacheEntry }),
+        setStaleByUUID: observableOf(true),
         generateRequestId: 'request-id',
         removeByHrefSubstring: {}
       });

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

@@ -11,7 +11,11 @@ import { ObjectCacheService } from '../cache/object-cache.service';
 import { DSpaceObject } from '../shared/dspace-object.model';
 import { HALEndpointService } from '../shared/hal-endpoint.service';
 import { Item } from '../shared/item.model';
-import { createSuccessfulRemoteDataObject$ } from '../../shared/remote-data.utils';
+import {
+  createFailedRemoteDataObject,
+  createSuccessfulRemoteDataObject,
+  createSuccessfulRemoteDataObject$,
+} from '../../shared/remote-data.utils';
 import { ChangeAnalyzer } from './change-analyzer';
 import { DataService } from './data.service';
 import { PatchRequest } from './request.models';
@@ -28,6 +32,8 @@ import { FindListOptions } from './find-list-options.model';
 
 const endpoint = 'https://rest.api/core';
 
+const BOOLEAN = { f: false, t: true };
+
 class TestService extends DataService<any> {
 
   constructor(
@@ -86,6 +92,9 @@ describe('DataService', () => {
       },
       getObjectBySelfLink: () => {
         /* empty */
+      },
+      getByHref: () => {
+        /* empty */
       }
     } as any;
     store = {} as Store<CoreState>;
@@ -833,4 +842,130 @@ describe('DataService', () => {
 
     });
   });
+
+  describe('invalidateByHref', () => {
+    let getByHrefSpy: jasmine.Spy;
+
+    beforeEach(() => {
+      getByHrefSpy = spyOn(objectCache, 'getByHref').and.returnValue(observableOf({
+        requestUUIDs: ['request1', 'request2', 'request3']
+      }));
+
+    });
+
+    it('should call setStaleByUUID for every request associated with this DSO', (done) => {
+      service.invalidateByHref('some-href').subscribe((ok) => {
+        expect(ok).toBeTrue();
+        expect(getByHrefSpy).toHaveBeenCalledWith('some-href');
+        expect(requestService.setStaleByUUID).toHaveBeenCalledWith('request1');
+        expect(requestService.setStaleByUUID).toHaveBeenCalledWith('request2');
+        expect(requestService.setStaleByUUID).toHaveBeenCalledWith('request3');
+        done();
+      });
+    });
+
+    it('should return an Observable that only emits true once all requests are stale', () => {
+      testScheduler.run(({ cold, expectObservable }) => {
+        requestService.setStaleByUUID.and.callFake((uuid) => {
+          switch (uuid) {   // fake requests becoming stale at different times
+            case 'request1':
+              return cold('--(t|)', BOOLEAN);
+            case 'request2':
+              return cold('----(t|)', BOOLEAN);
+            case 'request3':
+              return cold('------(t|)', BOOLEAN);
+          }
+        });
+
+        const done$ = service.invalidateByHref('some-href');
+
+        // emit true as soon as the final request is stale
+        expectObservable(done$).toBe('------(t|)', BOOLEAN);
+      });
+    });
+  });
+
+  describe('delete', () => {
+    let MOCK_SUCCEEDED_RD;
+    let MOCK_FAILED_RD;
+
+    let invalidateByHrefSpy: jasmine.Spy;
+    let buildFromRequestUUIDSpy: 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();
+      getIDHrefObsSpy = spyOn(service, 'getIDHrefObs').and.callThrough();
+      deleteByHrefSpy = spyOn(service, 'deleteByHref').and.callThrough();
+
+      MOCK_SUCCEEDED_RD = createSuccessfulRemoteDataObject({});
+      MOCK_FAILED_RD = createFailedRemoteDataObject('something went wrong');
+    });
+
+    it('should retrieve href by ID and call deleteByHref', () => {
+      getIDHrefObsSpy.and.returnValue(observableOf('some-href'));
+      buildFromRequestUUIDSpy.and.returnValue(null);
+
+      service.delete('some-id').subscribe(rd => {
+        expect(getIDHrefObsSpy).toHaveBeenCalledWith('some-id');
+        expect(deleteByHrefSpy).toHaveBeenCalledWith('some-href');
+      });
+    });
+
+    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();
+          done();
+        });
+      });
+
+      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();
+          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
+          );
+
+          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
+          );
+        });
+      });
+
+      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?
+
+          const done$ = service.deleteByHref('some-href');
+          expectObservable(done$).toBe(
+            '----(r|)', { r: MOCK_SUCCEEDED_RD}         // ...and expect the returned Observable to wait for the request
+          );
+        });
+      });
+    });
+  });
 });

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

@@ -1,7 +1,7 @@
 import { HttpClient } from '@angular/common/http';
 import { Store } from '@ngrx/store';
 import { Operation } from 'fast-json-patch';
-import { Observable, of as observableOf } from 'rxjs';
+import { combineLatest, from, Observable, of as observableOf } from 'rxjs';
 import {
   distinctUntilChanged,
   filter,
@@ -12,7 +12,7 @@ import {
   takeWhile,
   switchMap,
   tap,
-  skipWhile,
+  skipWhile, toArray
 } from 'rxjs/operators';
 import { hasValue, isNotEmpty, isNotEmptyOperator } from '../../shared/empty.util';
 import { NotificationOptions } from '../../shared/notifications/models/notification-options.model';
@@ -25,7 +25,7 @@ import { ObjectCacheService } from '../cache/object-cache.service';
 import { DSpaceSerializer } from '../dspace-rest/dspace.serializer';
 import { DSpaceObject } from '../shared/dspace-object.model';
 import { HALEndpointService } from '../shared/hal-endpoint.service';
-import { getRemoteDataPayload, getFirstSucceededRemoteData, } from '../shared/operators';
+import { getRemoteDataPayload, getFirstSucceededRemoteData, getFirstCompletedRemoteData } from '../shared/operators';
 import { URLCombiner } from '../url-combiner/url-combiner';
 import { ChangeAnalyzer } from './change-analyzer';
 import { PaginatedList } from './paginated-list.model';
@@ -579,6 +579,37 @@ export abstract class DataService<T extends CacheableObject> implements UpdateDa
     return result$;
   }
 
+  /**
+   * Invalidate an existing DSpaceObject by marking all requests it is included in as stale
+   * @param   objectId The id of the object to be invalidated
+   * @return  An Observable that will emit `true` once all requests are stale
+   */
+  invalidate(objectId: string): Observable<boolean> {
+    return this.getIDHrefObs(objectId).pipe(
+      switchMap((href: string) => this.invalidateByHref(href))
+    );
+  }
+
+  /**
+   * Invalidate an existing DSpaceObject by marking all requests it is included in as stale
+   * @param   href The self link of the object to be invalidated
+   * @return  An Observable that will emit `true` once all requests are stale
+   */
+  invalidateByHref(href: string): Observable<boolean> {
+    return this.objectCache.getByHref(href).pipe(
+      map(oce => oce.requestUUIDs),
+      switchMap(requestUUIDs => {
+        return from(requestUUIDs).pipe(
+          mergeMap(requestUUID => this.requestService.setStaleByUUID(requestUUID)),
+          toArray(),
+        );
+      }),
+      map(areRequestsStale => areRequestsStale.every(Boolean)),
+      distinctUntilChanged(),
+      takeWhile(allStale => allStale === false, true),
+    );
+  }
+
   /**
    * Delete an existing DSpace Object on the server
    * @param   objectId The id of the object to be removed
@@ -600,6 +631,7 @@ export abstract class DataService<T extends CacheableObject> implements UpdateDa
    *                            metadata should be saved as real metadata
    * @return  A RemoteData observable with an empty payload, but still representing the state of the request: statusCode,
    *          errorMessage, timeCompleted, etc
+   *          Only emits once all request related to the DSO has been invalidated.
    */
   deleteByHref(href: string, copyVirtualMetadata?: string[]): Observable<RemoteData<NoContent>> {
     const requestId = this.requestService.generateRequestId();
@@ -618,7 +650,26 @@ export abstract class DataService<T extends CacheableObject> implements UpdateDa
     }
     this.requestService.send(request);
 
-    return this.rdbService.buildFromRequestUUID(requestId);
+    const response$ = this.rdbService.buildFromRequestUUID(requestId);
+
+    const invalidated$ = response$.pipe(
+      getFirstCompletedRemoteData(),
+      switchMap(rd => {
+        if (rd.hasSucceeded) {
+          return this.invalidateByHref(href);
+        } else {
+          return [true];
+        }
+      })
+    );
+
+    return combineLatest([response$, invalidated$]).pipe(
+      filter(([_, invalidated]) => invalidated),
+      tap(() => {
+        console.log(`DataService.deleteByHref() href=${href} done.`);
+      }),
+      map(([response, _]) => response),
+    );
   }
 
   /**

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

@@ -8,7 +8,7 @@ import { defaultUUID, getMockUUIDService } from '../../shared/mocks/uuid.service
 import { ObjectCacheService } from '../cache/object-cache.service';
 import { coreReducers} from '../core.reducers';
 import { UUIDService } from '../shared/uuid.service';
-import { RequestConfigureAction, RequestExecuteAction } from './request.actions';
+import { RequestConfigureAction, RequestExecuteAction, RequestStaleAction } from './request.actions';
 import {
   DeleteRequest,
   GetRequest,
@@ -19,7 +19,7 @@ import {
   PutRequest
 } from './request.models';
 import { RequestService } from './request.service';
-import { TestBed, waitForAsync } from '@angular/core/testing';
+import { fakeAsync, TestBed, waitForAsync } from '@angular/core/testing';
 import { storeModuleConfig } from '../../app.reducer';
 import { MockStore, provideMockStore } from '@ngrx/store/testing';
 import { RequestEntryState } from './request-entry-state.model';
@@ -426,7 +426,7 @@ describe('RequestService', () => {
           describe('and it is cached', () => {
             describe('in the ObjectCache', () => {
               beforeEach(() => {
-                (objectCache.getByHref as any).and.returnValue(observableOf({ requestUUID: 'some-uuid' }));
+                (objectCache.getByHref as any).and.returnValue(observableOf({ requestUUIDs: ['some-uuid'] }));
                 spyOn(serviceAsAny, 'hasByHref').and.returnValue(false);
                 spyOn(serviceAsAny, 'hasByUUID').and.returnValue(true);
               });
@@ -596,4 +596,33 @@ describe('RequestService', () => {
     });
   });
 
+  describe('setStaleByUUID', () => {
+    let dispatchSpy: jasmine.Spy;
+    let getByUUIDSpy: jasmine.Spy;
+
+    beforeEach(() => {
+      dispatchSpy = spyOn(store, 'dispatch');
+      getByUUIDSpy = spyOn(service, 'getByUUID').and.callThrough();
+    });
+
+    it('should dispatch a RequestStaleAction', () => {
+      service.setStaleByUUID('something');
+      const firstAction = dispatchSpy.calls.argsFor(0)[0];
+      expect(firstAction).toBeInstanceOf(RequestStaleAction);
+      expect(firstAction.payload).toEqual({ uuid: 'something' });
+    });
+
+    it('should return an Observable that emits true as soon as the request is stale', fakeAsync(() => {
+      dispatchSpy.and.callFake(() => { /* empty */ });   // don't actually set as stale
+      getByUUIDSpy.and.returnValue(cold('a-b--c--d-', {  // but fake the state in the cache
+        a: { state: RequestEntryState.ResponsePending },
+        b: { state: RequestEntryState.Success },
+        c: { state: RequestEntryState.SuccessStale },
+        d: { state: RequestEntryState.Error },
+      }));
+
+      const done$ = service.setStaleByUUID('something');
+      expect(done$).toBeObservable(cold('-----(t|)', { t: true }));
+    }));
+  });
 });

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

@@ -311,6 +311,21 @@ export class RequestService {
     );
   }
 
+  /**
+   * Mark a request as stale
+   * @param uuid  the UUID of the request
+   * @return      an Observable that will emit true once the Request becomes stale
+   */
+  setStaleByUUID(uuid: string): Observable<boolean> {
+    this.store.dispatch(new RequestStaleAction(uuid));
+
+    return this.getByUUID(uuid).pipe(
+      map(request => isStale(request.state)),
+      filter(stale => stale === true),
+      take(1),
+    );
+  }
+
   /**
    * Check if a GET request is in the cache or if it's still pending
    * @param {GetRequest} request The request to check
@@ -339,7 +354,7 @@ export class RequestService {
           .subscribe((entry: ObjectCacheEntry) => {
             // if the object cache has a match, check if the request that the object came with is
             // still valid
-            inObjCache = this.hasByUUID(entry.requestUUID);
+            inObjCache = this.hasByUUID(entry.requestUUIDs[0]);
           }).unsubscribe();
 
         // we should send the request if it isn't cached

src/app/shared/mocks/request.service.mock.ts

@@ -13,6 +13,7 @@ export function getMockRequestService(requestEntry$: Observable<RequestEntry> =
     isCachedOrPending: false,
     removeByHrefSubstring: observableOf(true),
     setStaleByHrefSubstring: observableOf(true),
+    setStaleByUUID: observableOf(true),
     hasByHref$: observableOf(false)
   });
 }