Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

4 changes: 2 additions & 2 deletions src/app/core/breadcrumbs/item-breadcrumb.resolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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,
Expand Down
12 changes: 0 additions & 12 deletions src/app/core/browse/browse.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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';
Expand All @@ -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);
Expand Down Expand Up @@ -129,7 +126,6 @@ describe('BrowseService', () => {
halService,
browseDefinitionDataService,
hrefOnlyDataService,
rdbService,
);
}

Expand All @@ -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', () => {
Expand All @@ -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', () => {
Expand Down Expand Up @@ -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-', {
Expand Down Expand Up @@ -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('----'));
Expand All @@ -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', () => {
Expand Down
19 changes: 12 additions & 7 deletions src/app/core/browse/browse.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
startWith,
} from 'rxjs/operators';

import { environment } from '../../../environments/environment';
import {
hasValue,
hasValueOperator,
Expand All @@ -16,7 +17,6 @@
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';
Expand All @@ -38,9 +38,15 @@
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 = [

Check warning on line 42 in src/app/core/browse/browse.service.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L42 was not covered by tests
followLink('thumbnail'),
];
if (environment.item.showAccessStatuses) {
followLinks.push(followLink('accessStatus'));

Check warning on line 46 in src/app/core/browse/browse.service.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L46 was not covered by tests
}
return followLinks;

Check warning on line 48 in src/app/core/browse/browse.service.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L48 was not covered by tests
}

/**
* The service handling all browse requests
Expand All @@ -67,7 +73,6 @@
protected halService: HALEndpointService,
private browseDefinitionDataService: BrowseDefinitionDataService,
private hrefOnlyDataService: HrefOnlyDataService,
private rdb: RemoteDataBuildService,
) {
}

Expand Down Expand Up @@ -117,7 +122,7 @@
}),
);
if (options.fetchThumbnail ) {
return this.hrefOnlyDataService.findListByHref<BrowseEntry>(href$, {}, undefined, undefined, ...BROWSE_LINKS_TO_FOLLOW);
return this.hrefOnlyDataService.findListByHref<BrowseEntry>(href$, {}, undefined, undefined, ...getBrowseLinksToFollow());

Check warning on line 125 in src/app/core/browse/browse.service.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L125 was not covered by tests
}
return this.hrefOnlyDataService.findListByHref<BrowseEntry>(href$);
}
Expand Down Expand Up @@ -165,7 +170,7 @@
}),
);
if (options.fetchThumbnail) {
return this.hrefOnlyDataService.findListByHref<Item>(href$, {}, undefined, undefined, ...BROWSE_LINKS_TO_FOLLOW);
return this.hrefOnlyDataService.findListByHref<Item>(href$, {}, undefined, undefined, ...getBrowseLinksToFollow());

Check warning on line 173 in src/app/core/browse/browse.service.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L173 was not covered by tests
}
return this.hrefOnlyDataService.findListByHref<Item>(href$);
}
Expand Down
31 changes: 18 additions & 13 deletions src/app/core/cache/object-cache.reducer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -174,20 +174,25 @@
* 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;

Check warning on line 194 in src/app/core/cache/object-cache.reducer.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L194 was not covered by tests
}
}

/**
Expand Down
16 changes: 11 additions & 5 deletions src/app/core/cache/object-cache.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,9 @@
* 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));
}

Expand Down Expand Up @@ -175,11 +177,15 @@
},
),
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);

Check warning on line 181 in src/app/core/cache/object-cache.service.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L181 was not covered by tests
if (typeof type !== 'function') {
throw new Error(`${type} is not a valid constructor for ${JSON.stringify(entry.data)}`);

Check warning on line 183 in src/app/core/cache/object-cache.service.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L183 was not covered by tests
}
return Object.assign(new type(), entry.data) as T;

Check warning on line 185 in src/app/core/cache/object-cache.service.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L185 was not covered by tests
} else {
return null;

Check warning on line 187 in src/app/core/cache/object-cache.service.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L187 was not covered by tests
}
return Object.assign(new type(), entry.data) as T;
}),
);
}
Expand Down
11 changes: 9 additions & 2 deletions src/app/core/data/dspace-rest-response-parsing.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,13 @@
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);

Check warning on line 125 in src/app/core/data/dspace-rest-response-parsing.service.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L125 was not covered by tests
} 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);
});
}
Expand Down Expand Up @@ -237,7 +244,7 @@
* @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)) {
Expand All @@ -251,7 +258,7 @@
return;
}

if (alternativeURL === co._links.self.href) {
if (hasValue(co) && alternativeURL === co._links.self.href) {
alternativeURL = undefined;
}

Expand Down
5 changes: 4 additions & 1 deletion src/app/core/data/relationship-data.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
});
Expand Down
7 changes: 6 additions & 1 deletion src/app/core/data/relationship-data.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ import {
tap,
} from 'rxjs/operators';

import {
APP_CONFIG,
AppConfig,
} from '../../../config/app-config.interface';
import {
AppState,
keySelector,
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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));
Expand Down
2 changes: 1 addition & 1 deletion src/app/core/data/version-history-data.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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>) => {
Expand Down
4 changes: 2 additions & 2 deletions src/app/core/index/index.effects.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,9 @@
if (this.appConfig.browseBy.showThumbnails) {
linksToFollow.push(followLink('thumbnail'));
}
if (this.appConfig.item.showAccessStatuses) {
linksToFollow.push(followLink('accessStatus'));

Check warning on line 102 in src/app/home-page/recent-item-list/recent-item-list.component.ts

View check run for this annotation

Codecov / codecov/patch

src/app/home-page/recent-item-list/recent-item-list.component.ts#L102

Added line #L102 was not covered by tests
}

this.itemRD$ = this.searchService.search(
new PaginatedSearchOptions({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import { getAllSucceededRemoteData } from '../../../core/shared/operators';
import { hasValue } from '../../../shared/empty.util';
import { NotificationsService } from '../../../shared/notifications/notifications.service';
import { AbstractTrackableComponent } from '../../../shared/trackable/abstract-trackable.component';
import { ITEM_PAGE_LINKS_TO_FOLLOW } from '../../item.resolver';
import { getItemPageLinksToFollow } from '../../item.resolver';
import { getItemPageRoute } from '../../item-page-routing-paths';

@Component({
Expand Down Expand Up @@ -92,7 +92,7 @@ export class AbstractItemUpdateComponent extends AbstractTrackableComponent impl
this.item = rd.payload;
}),
switchMap((rd: RemoteData<Item>) => {
return this.itemService.findByHref(rd.payload._links.self.href, true, true, ...ITEM_PAGE_LINKS_TO_FOLLOW);
return this.itemService.findByHref(rd.payload._links.self.href, true, true, ...getItemPageLinksToFollow());
}),
getAllSucceededRemoteData(),
).subscribe((rd: RemoteData<Item>) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -320,8 +320,8 @@ export class ItemDeleteComponent
this.linkService.resolveLinks(
relationship,
followLink('relationshipType'),
followLink('leftItem'),
followLink('rightItem'),
followLink('leftItem', undefined, followLink<Item>('accessStatus')),
followLink('rightItem', undefined, followLink<Item>('accessStatus')),
);
return relationship.relationshipType.pipe(
getFirstSucceededRemoteData(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -478,7 +478,7 @@ export class EditRelationshipListComponent implements OnInit, OnDestroy {
);

// this adds thumbnail images when required by configuration
const linksToFollow: FollowLinkConfig<Relationship>[] = itemLinksToFollow(this.fetchThumbnail);
const linksToFollow: FollowLinkConfig<Relationship>[] = itemLinksToFollow(this.fetchThumbnail, this.appConfig.item.showAccessStatuses);

this.subs.push(
observableCombineLatest([
Expand Down
Loading
Loading