Merge pull request #3415 from alexandrevryghem/w2p-107155_Performance-re-request-embeds-main

Added support for caching embedded objects without a self link & null responses

Author: tdonohue

src/app/core/breadcrumbs/item-breadcrumb.resolver.ts

@@ -7,7 +7,7 @@ import {
 import { Observable } from 'rxjs';
 
 import { BreadcrumbConfig } from '../../breadcrumbs/breadcrumb/breadcrumb-config.model';
-import { ITEM_PAGE_LINKS_TO_FOLLOW } from '../../item-page/item.resolver';
+import { getItemPageLinksToFollow } from '../../item-page/item.resolver';
 import { FollowLinkConfig } from '../../shared/utils/follow-link-config.model';
 import { ItemDataService } from '../data/item-data.service';
 import { DSpaceObject } from '../shared/dspace-object.model';
@@ -24,7 +24,7 @@ export const itemBreadcrumbResolver: ResolveFn<BreadcrumbConfig<Item>> = (
   breadcrumbService: DSOBreadcrumbsService = inject(DSOBreadcrumbsService),
   dataService: ItemDataService = inject(ItemDataService),
 ): Observable<BreadcrumbConfig<Item>> => {
-  const linksToFollow: FollowLinkConfig<DSpaceObject>[] = ITEM_PAGE_LINKS_TO_FOLLOW as FollowLinkConfig<DSpaceObject>[];
+  const linksToFollow: FollowLinkConfig<DSpaceObject>[] = getItemPageLinksToFollow() as FollowLinkConfig<DSpaceObject>[];
   return DSOBreadcrumbResolver(
     route,
     state,

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

@@ -7,7 +7,6 @@ import { of as observableOf } from 'rxjs';
 import { TestScheduler } from 'rxjs/testing';
 
 import { getMockHrefOnlyDataService } from '../../shared/mocks/href-only-data.service.mock';
-import { getMockRemoteDataBuildService } from '../../shared/mocks/remote-data-build.service.mock';
 import { getMockRequestService } from '../../shared/mocks/request.service.mock';
 import {
   createSuccessfulRemoteDataObject,
@@ -18,7 +17,6 @@ import {
   createPaginatedList,
   getFirstUsedArgumentOfSpyMethod,
 } from '../../shared/testing/utils.test';
-import { RemoteDataBuildService } from '../cache/builders/remote-data-build.service';
 import { RequestService } from '../data/request.service';
 import { RequestEntry } from '../data/request-entry.model';
 import { FlatBrowseDefinition } from '../shared/flat-browse-definition.model';
@@ -31,7 +29,6 @@ describe('BrowseService', () => {
   let scheduler: TestScheduler;
   let service: BrowseService;
   let requestService: RequestService;
-  let rdbService: RemoteDataBuildService;
 
   const browsesEndpointURL = 'https://rest.api/browses';
   const halService: any = new HALEndpointServiceStub(browsesEndpointURL);
@@ -129,7 +126,6 @@ describe('BrowseService', () => {
       halService,
       browseDefinitionDataService,
       hrefOnlyDataService,
-      rdbService,
     );
   }
 
@@ -141,11 +137,9 @@ describe('BrowseService', () => {
 
     beforeEach(() => {
       requestService = getMockRequestService(getRequestEntry$(true));
-      rdbService = getMockRemoteDataBuildService();
       service = initTestService();
       spyOn(halService, 'getEndpoint').and
         .returnValue(hot('--a-', { a: browsesEndpointURL }));
-      spyOn(rdbService, 'buildList').and.callThrough();
     });
 
     it('should call BrowseDefinitionDataService to create the RemoteData Observable', () => {
@@ -162,9 +156,7 @@ describe('BrowseService', () => {
 
     beforeEach(() => {
       requestService = getMockRequestService(getRequestEntry$(true));
-      rdbService = getMockRemoteDataBuildService();
       service = initTestService();
-      spyOn(rdbService, 'buildList').and.callThrough();
     });
 
     describe('when getBrowseEntriesFor is called with a valid browse definition id', () => {
@@ -215,7 +207,6 @@ describe('BrowseService', () => {
     describe('if getBrowseDefinitions fires', () => {
       beforeEach(() => {
         requestService = getMockRequestService(getRequestEntry$(true));
-        rdbService = getMockRemoteDataBuildService();
         service = initTestService();
         spyOn(service, 'getBrowseDefinitions').and
           .returnValue(hot('--a-', {
@@ -270,7 +261,6 @@ describe('BrowseService', () => {
     describe('if getBrowseDefinitions doesn\'t fire', () => {
       it('should return undefined', () => {
         requestService = getMockRequestService(getRequestEntry$(true));
-        rdbService = getMockRemoteDataBuildService();
         service = initTestService();
         spyOn(service, 'getBrowseDefinitions').and
           .returnValue(hot('----'));
@@ -288,9 +278,7 @@ describe('BrowseService', () => {
   describe('getFirstItemFor', () => {
     beforeEach(() => {
       requestService = getMockRequestService();
-      rdbService = getMockRemoteDataBuildService();
       service = initTestService();
-      spyOn(rdbService, 'buildList').and.callThrough();
     });
 
     describe('when getFirstItemFor is called with a valid browse definition id', () => {

src/app/core/browse/browse.service.ts

@@ -6,6 +6,7 @@ import {
   startWith,
 } from 'rxjs/operators';
 
+import { environment } from '../../../environments/environment';
 import {
   hasValue,
   hasValueOperator,
@@ -16,7 +17,6 @@ import {
   followLink,
   FollowLinkConfig,
 } from '../../shared/utils/follow-link-config.model';
-import { RemoteDataBuildService } from '../cache/builders/remote-data-build.service';
 import { SortDirection } from '../cache/models/sort-options.model';
 import { HrefOnlyDataService } from '../data/href-only-data.service';
 import { PaginatedList } from '../data/paginated-list.model';
@@ -38,9 +38,15 @@ import { URLCombiner } from '../url-combiner/url-combiner';
 import { BrowseDefinitionDataService } from './browse-definition-data.service';
 import { BrowseEntrySearchOptions } from './browse-entry-search-options.model';
 
-export const BROWSE_LINKS_TO_FOLLOW: FollowLinkConfig<BrowseEntry | Item>[] = [
-  followLink('thumbnail'),
-];
+export function getBrowseLinksToFollow(): FollowLinkConfig<BrowseEntry | Item>[] {
+  const followLinks = [
+    followLink('thumbnail'),
+  ];
+  if (environment.item.showAccessStatuses) {
+    followLinks.push(followLink('accessStatus'));
+  }
+  return followLinks;
+}
 
 /**
  * The service handling all browse requests
@@ -67,7 +73,6 @@ export class BrowseService {
     protected halService: HALEndpointService,
     private browseDefinitionDataService: BrowseDefinitionDataService,
     private hrefOnlyDataService: HrefOnlyDataService,
-    private rdb: RemoteDataBuildService,
   ) {
   }
 
@@ -117,7 +122,7 @@ export class BrowseService {
       }),
     );
     if (options.fetchThumbnail ) {
-      return this.hrefOnlyDataService.findListByHref<BrowseEntry>(href$, {}, undefined, undefined, ...BROWSE_LINKS_TO_FOLLOW);
+      return this.hrefOnlyDataService.findListByHref<BrowseEntry>(href$, {}, undefined, undefined, ...getBrowseLinksToFollow());
     }
     return this.hrefOnlyDataService.findListByHref<BrowseEntry>(href$);
   }
@@ -165,7 +170,7 @@ export class BrowseService {
       }),
     );
     if (options.fetchThumbnail) {
-      return this.hrefOnlyDataService.findListByHref<Item>(href$, {}, undefined, undefined, ...BROWSE_LINKS_TO_FOLLOW);
+      return this.hrefOnlyDataService.findListByHref<Item>(href$, {}, undefined, undefined, ...getBrowseLinksToFollow());
     }
     return this.hrefOnlyDataService.findListByHref<Item>(href$);
   }

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

@@ -174,20 +174,25 @@ export function objectCacheReducer(state = initialState, action: ObjectCacheActi
  *    the new state, with the object added, or overwritten.
  */
 function addToObjectCache(state: ObjectCacheState, action: AddToObjectCacheAction): ObjectCacheState {
-  const existing = state[action.payload.objectToCache._links.self.href] || {} as any;
+  const cacheLink = hasValue(action.payload.objectToCache?._links?.self) ? action.payload.objectToCache._links.self.href : action.payload.alternativeLink;
+  const existing = state[cacheLink] || {} as any;
   const newAltLinks = hasValue(action.payload.alternativeLink) ? [action.payload.alternativeLink] : [];
-  return Object.assign({}, state, {
-    [action.payload.objectToCache._links.self.href]: {
-      data: action.payload.objectToCache,
-      timeCompleted: action.payload.timeCompleted,
-      msToLive: action.payload.msToLive,
-      requestUUIDs: [action.payload.requestUUID, ...(existing.requestUUIDs || [])],
-      dependentRequestUUIDs: existing.dependentRequestUUIDs || [],
-      isDirty: isNotEmpty(existing.patches),
-      patches: existing.patches || [],
-      alternativeLinks: [...(existing.alternativeLinks || []), ...newAltLinks],
-    } as ObjectCacheEntry,
-  });
+  if (hasValue(cacheLink)) {
+    return Object.assign({}, state, {
+      [cacheLink]: {
+        data: action.payload.objectToCache,
+        timeCompleted: action.payload.timeCompleted,
+        msToLive: action.payload.msToLive,
+        requestUUIDs: [action.payload.requestUUID, ...(existing.requestUUIDs || [])],
+        dependentRequestUUIDs: existing.dependentRequestUUIDs || [],
+        isDirty: isNotEmpty(existing.patches),
+        patches: existing.patches || [],
+        alternativeLinks: [...(existing.alternativeLinks || []), ...newAltLinks],
+      } as ObjectCacheEntry,
+    });
+  } else {
+    return state;
+  }
 }
 
 /**

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

@@ -99,7 +99,9 @@ export class ObjectCacheService {
    *    An optional alternative link to this object
    */
   add(object: CacheableObject, msToLive: number, requestUUID: string, alternativeLink?: string): void {
-    object = this.linkService.removeResolvedLinks(object); // Ensure the object we're storing has no resolved links
+    if (hasValue(object)) {
+      object = this.linkService.removeResolvedLinks(object); // Ensure the object we're storing has no resolved links
+    }
     this.store.dispatch(new AddToObjectCacheAction(object, new Date().getTime(), msToLive, requestUUID, alternativeLink));
   }
 
@@ -175,11 +177,15 @@ export class ObjectCacheService {
       },
       ),
       map((entry: ObjectCacheEntry) => {
-        const type: GenericConstructor<T> = getClassForType((entry.data as any).type);
-        if (typeof type !== 'function') {
-          throw new Error(`${type} is not a valid constructor for ${JSON.stringify(entry.data)}`);
+        if (hasValue(entry.data)) {
+          const type: GenericConstructor<T> = getClassForType((entry.data as any).type);
+          if (typeof type !== 'function') {
+            throw new Error(`${type} is not a valid constructor for ${JSON.stringify(entry.data)}`);
+          }
+          return Object.assign(new type(), entry.data) as T;
+        } else {
+          return null;
         }
-        return Object.assign(new type(), entry.data) as T;
       }),
     );
   }

src/app/core/data/dspace-rest-response-parsing.service.ts

@@ -120,6 +120,13 @@ export class DspaceRestResponseParsingService implements ResponseParsingService
               if (hasValue(match)) {
                 embedAltUrl = new URLCombiner(embedAltUrl, `?size=${match.size}`).toString();
               }
+              if (data._embedded[property] == null) {
+                // Embedded object is null, meaning it exists (not undefined), but had an empty response (204) -> cache it as null
+                this.addToObjectCache(null, request, data, embedAltUrl);
+              } else if (!isCacheableObject(data._embedded[property])) {
+                // Embedded object exists, but doesn't contain a self link -> cache it using the alternative link instead
+                this.objectCache.add(data._embedded[property], hasValue(request.responseMsToLive) ? request.responseMsToLive : environment.cache.msToLive.default, request.uuid, embedAltUrl);
+              }
               this.process<ObjectDomain>(data._embedded[property], request, embedAltUrl);
             });
         }
@@ -237,7 +244,7 @@ export class DspaceRestResponseParsingService implements ResponseParsingService
    * @param alternativeURL  an alternative url that can be used to retrieve the object
    */
   addToObjectCache(co: CacheableObject, request: RestRequest, data: any, alternativeURL?: string): void {
-    if (!isCacheableObject(co)) {
+    if (hasValue(co) && !isCacheableObject(co)) {
       const type = hasValue(data) && hasValue(data.type) ? data.type : 'object';
       let dataJSON: string;
       if (hasValue(data._embedded)) {
@@ -251,7 +258,7 @@ export class DspaceRestResponseParsingService implements ResponseParsingService
       return;
     }
 
-    if (alternativeURL === co._links.self.href) {
+    if (hasValue(co) && alternativeURL === co._links.self.href) {
       alternativeURL = undefined;
     }
 

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

@@ -3,6 +3,8 @@ import { Store } from '@ngrx/store';
 import { provideMockStore } from '@ngrx/store/testing';
 import { of as observableOf } from 'rxjs';
 
+import { APP_CONFIG } from '../../../config/app-config.interface';
+import { environment } from '../../../environments/environment.test';
 import { PAGINATED_RELATIONS_TO_ITEMS_OPERATOR } from '../../item-page/simple/item-types/shared/item-relationships-utils';
 import { getMockRemoteDataBuildServiceHrefMap } from '../../shared/mocks/remote-data-build.service.mock';
 import { getMockRequestService } from '../../shared/mocks/request.service.mock';
@@ -150,14 +152,15 @@ describe('RelationshipDataService', () => {
         { provide: RequestService, useValue: requestService },
         { provide: PAGINATED_RELATIONS_TO_ITEMS_OPERATOR, useValue: jasmine.createSpy('paginatedRelationsToItems').and.returnValue((v) => v) },
         { provide: Store, useValue: provideMockStore() },
+        { provide: APP_CONFIG, useValue: environment },
         RelationshipDataService,
       ],
     });
     service = TestBed.inject(RelationshipDataService);
   });
 
   describe('composition', () => {
-    const initService = () => new RelationshipDataService(null, null, null, null, null, null, null, null);
+    const initService = () => new RelationshipDataService(null, null, null, null, null, null, null, null, environment);
 
     testSearchDataImplementation(initService);
   });

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

@@ -24,6 +24,10 @@ import {
   tap,
 } from 'rxjs/operators';
 
+import {
+  APP_CONFIG,
+  AppConfig,
+} from '../../../config/app-config.interface';
 import {
   AppState,
   keySelector,
@@ -133,6 +137,7 @@ export class RelationshipDataService extends IdentifiableDataService<Relationshi
     protected itemService: ItemDataService,
     protected appStore: Store<AppState>,
     @Inject(PAGINATED_RELATIONS_TO_ITEMS_OPERATOR) private paginatedRelationsToItems: (thisId: string) => (source: Observable<RemoteData<PaginatedList<Relationship>>>) => Observable<RemoteData<PaginatedList<Item>>>,
+    @Inject(APP_CONFIG) private appConfig: AppConfig,
   ) {
     super('relationships', requestService, rdbService, objectCache, halService, 15 * 60 * 1000);
 
@@ -319,7 +324,7 @@ export class RelationshipDataService extends IdentifiableDataService<Relationshi
    * @param options
    */
   getRelatedItemsByLabel(item: Item, label: string, options?: FindListOptions): Observable<RemoteData<PaginatedList<Item>>> {
-    const linksToFollow: FollowLinkConfig<Relationship>[] = itemLinksToFollow(options.fetchThumbnail);
+    const linksToFollow: FollowLinkConfig<Relationship>[] = itemLinksToFollow(options.fetchThumbnail, this.appConfig.item.showAccessStatuses);
     linksToFollow.push(followLink('relationshipType'));
 
     return this.getItemRelationshipsByLabel(item, label, options, true, true, ...linksToFollow).pipe(this.paginatedRelationsToItems(item.uuid));

src/app/core/data/version-history-data.service.ts

@@ -190,7 +190,7 @@ export class VersionHistoryDataService extends IdentifiableDataService<VersionHi
     return this.versionDataService.findByHref(versionHref, false, true, followLink('versionhistory')).pipe(
       getFirstCompletedRemoteData(),
       switchMap((versionRD: RemoteData<Version>) => {
-        if (versionRD.hasSucceeded && !versionRD.hasNoContent) {
+        if (versionRD.hasSucceeded && !versionRD.hasNoContent && hasValue(versionRD.payload)) {
           return versionRD.payload.versionhistory.pipe(
             getFirstCompletedRemoteData(),
             map((versionHistoryRD: RemoteData<VersionHistory>) => {

src/app/core/index/index.effects.ts

@@ -45,7 +45,7 @@ export class UUIDIndexEffects {
   addObject$ = createEffect(() => this.actions$
     .pipe(
       ofType(ObjectCacheActionTypes.ADD),
-      filter((action: AddToObjectCacheAction) => hasValue(action.payload.objectToCache.uuid)),
+      filter((action: AddToObjectCacheAction) => hasValue(action.payload.objectToCache) && hasValue(action.payload.objectToCache.uuid)),
       map((action: AddToObjectCacheAction) => {
         return new AddToIndexAction(
           IndexName.OBJECT,
@@ -64,7 +64,7 @@ export class UUIDIndexEffects {
       ofType(ObjectCacheActionTypes.ADD),
       map((action: AddToObjectCacheAction) => {
         const alternativeLink = action.payload.alternativeLink;
-        const selfLink = action.payload.objectToCache._links.self.href;
+        const selfLink = hasValue(action.payload.objectToCache?._links?.self) ? action.payload.objectToCache._links.self.href : alternativeLink;
         if (hasValue(alternativeLink) && alternativeLink !== selfLink) {
           return new AddToIndexAction(
             IndexName.ALTERNATIVE_OBJECT_LINK,