From f368da7a277fc5ed0de2bf9698de0de4ea67ce64 Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Mon, 6 Apr 2020 16:40:45 +0200 Subject: [PATCH 1/7] fix issue where requestservice.getByUUID wouldn't work for requests that were never sent because there was already a copy in the cache --- src/app/core/data/request.service.ts | 30 +++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/src/app/core/data/request.service.ts b/src/app/core/data/request.service.ts index 810b0721ae..c68bd751fd 100644 --- a/src/app/core/data/request.service.ts +++ b/src/app/core/data/request.service.ts @@ -3,9 +3,9 @@ import { HttpHeaders } from '@angular/common/http'; import { createSelector, MemoizedSelector, select, Store } from '@ngrx/store'; import { Observable, race as observableRace } from 'rxjs'; -import { filter, map, mergeMap, take } from 'rxjs/operators'; +import { filter, map, mergeMap, take, switchMap } from 'rxjs/operators'; import { cloneDeep, remove } from 'lodash'; -import { hasValue, isEmpty, isNotEmpty } from '../../shared/empty.util'; +import { hasValue, isEmpty, isNotEmpty, hasValueOperator } from '../../shared/empty.util'; import { CacheableObject } from '../cache/object-cache.reducer'; import { ObjectCacheService } from '../cache/object-cache.service'; import { CoreState } from '../core.reducers'; @@ -111,13 +111,22 @@ export class RequestService { */ getByUUID(uuid: string): Observable { return observableRace( - this.store.pipe(select(entryFromUUIDSelector(uuid))), + this.store.pipe( + select(entryFromUUIDSelector(uuid)), + hasValueOperator() + ), this.store.pipe( select(originalRequestUUIDFromRequestUUIDSelector(uuid)), - mergeMap((originalUUID) => { - return this.store.pipe(select(entryFromUUIDSelector(originalUUID))) + switchMap((originalUUID) => { + if (hasValue(originalUUID)) { + return this.store.pipe(select(entryFromUUIDSelector(originalUUID))) + } else { + return [] + } }, - )) + ), + hasValueOperator() + ) ).pipe( map((entry: RequestEntry) => { // Headers break after being retrieved from the store (because of lazy initialization) @@ -137,7 +146,14 @@ export class RequestService { getByHref(href: string): Observable { return this.store.pipe( select(uuidFromHrefSelector(href)), - mergeMap((uuid: string) => this.getByUUID(uuid)) + mergeMap((uuid: string) => { + if (isNotEmpty(uuid)) { + return this.getByUUID(uuid); + } + else { + return [undefined]; + } + }) ); } From 6351a951e71e11ae6d63eb8a785ad1165216a32f Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Tue, 7 Apr 2020 11:08:20 +0200 Subject: [PATCH 2/7] fix test and lint --- src/app/core/data/request.service.spec.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/core/data/request.service.spec.ts b/src/app/core/data/request.service.spec.ts index 017721fdf9..9f8e20c8b7 100644 --- a/src/app/core/data/request.service.spec.ts +++ b/src/app/core/data/request.service.spec.ts @@ -162,7 +162,7 @@ describe('RequestService', () => { }); }); - describe('if the request with the specified UUID doesn\'t exist in the store', () => { + describe(`if the request with the specified UUID doesn't exist in the store `, () => { beforeEach(() => { selectSpy.and.callFake(() => { return () => { @@ -171,10 +171,10 @@ describe('RequestService', () => { }); }); - it('should return an Observable of undefined', () => { + it(`it shouldn't return anything`, () => { const result = service.getByUUID(testUUID); - scheduler.expectObservable(result).toBe('b', { b: undefined }); + scheduler.expectObservable(result).toBe(''); }); }); From cea01d46f9f3ccd919f15c23c5a1f37b1cc1f1ee Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Tue, 7 Apr 2020 13:31:17 +0200 Subject: [PATCH 3/7] add test to prove the bug is fixed --- src/app/core/data/request.service.spec.ts | 37 ++++++++++++++++++++++- src/app/core/data/request.service.ts | 3 +- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/src/app/core/data/request.service.spec.ts b/src/app/core/data/request.service.spec.ts index 9f8e20c8b7..b70f2dc47d 100644 --- a/src/app/core/data/request.service.spec.ts +++ b/src/app/core/data/request.service.spec.ts @@ -22,6 +22,7 @@ import { } from './request.models'; import { RequestEntry } from './request.reducer'; import { RequestService } from './request.service'; +import { parseJsonSchemaToCommandDescription } from '@angular/cli/utilities/json-schema'; describe('RequestService', () => { let scheduler: TestScheduler; @@ -178,7 +179,41 @@ describe('RequestService', () => { }); }); - }); + describe(`if the request with the specified UUID wasn't sent, because it was already cached`, () => { + beforeEach(() => { + let callCounter = 0; + const responses = [ + cold('a', { a: undefined }), // No hit in the request cache with that UUID + cold('b', { b: 'otherRequestUUID' }), // A hit in the index, which returns the uuid of the cached request + cold('c', { c: { // the call to retrieve the cached request using the UUID from the index + c: { + completed: true + } + } + }) + ]; + selectSpy.and.callFake(() => { + return () => { + const response = responses[callCounter]; + callCounter++; + return () => response; + }; + }); + }); + + it(`it should return the cached request`, () => { + const result = service.getByUUID(testUUID); + + scheduler.expectObservable(result).toBe('c', { c: { + c: { + completed: true + } + } + }); + }); + }); + + }); describe('getByHref', () => { describe('when the request with the specified href exists in the store', () => { diff --git a/src/app/core/data/request.service.ts b/src/app/core/data/request.service.ts index c68bd751fd..f69ad646f7 100644 --- a/src/app/core/data/request.service.ts +++ b/src/app/core/data/request.service.ts @@ -149,8 +149,7 @@ export class RequestService { mergeMap((uuid: string) => { if (isNotEmpty(uuid)) { return this.getByUUID(uuid); - } - else { + } else { return [undefined]; } }) From 15e44ff46c2814e29805498e71171ef907a9f712 Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Tue, 7 Apr 2020 18:26:13 +0200 Subject: [PATCH 4/7] ensure getByUUID still returns undefined when there's no match --- src/app/core/data/request.service.spec.ts | 48 +++++++++++++++-------- src/app/core/data/request.service.ts | 19 ++++----- 2 files changed, 38 insertions(+), 29 deletions(-) diff --git a/src/app/core/data/request.service.spec.ts b/src/app/core/data/request.service.spec.ts index b70f2dc47d..cfb3611fc6 100644 --- a/src/app/core/data/request.service.spec.ts +++ b/src/app/core/data/request.service.spec.ts @@ -140,13 +140,21 @@ describe('RequestService', () => { describe('getByUUID', () => { describe('if the request with the specified UUID exists in the store', () => { beforeEach(() => { + let callCounter = 0; + const responses = [ + cold('a', { // A direct hit in the request cache + a: { + completed: true + } + }), + cold('b', { b: undefined }), // No hit in the index + cold('c', { c: undefined }) // So no mapped hit in the request cache + ]; selectSpy.and.callFake(() => { return () => { - return () => hot('a', { - a: { - completed: true - } - }); + const response = responses[callCounter]; + callCounter++; + return () => response; }; }); }); @@ -165,17 +173,25 @@ describe('RequestService', () => { describe(`if the request with the specified UUID doesn't exist in the store `, () => { beforeEach(() => { + let callCounter = 0; + const responses = [ + cold('a', { a: undefined }), // No direct hit in the request cache + cold('b', { b: undefined }), // No hit in the index + cold('c', { c: undefined }), // So no mapped hit in the request cache + ]; selectSpy.and.callFake(() => { return () => { - return () => hot('a', { a: undefined }); + const response = responses[callCounter]; + callCounter++; + return () => response; }; }); }); - it(`it shouldn't return anything`, () => { + it('should return an Observable of undefined', () => { const result = service.getByUUID(testUUID); - scheduler.expectObservable(result).toBe(''); + scheduler.expectObservable(result).toBe('a', { a: undefined }); }); }); @@ -183,12 +199,11 @@ describe('RequestService', () => { beforeEach(() => { let callCounter = 0; const responses = [ - cold('a', { a: undefined }), // No hit in the request cache with that UUID + cold('a', { a: undefined }), // No direct hit in the request cache with that UUID cold('b', { b: 'otherRequestUUID' }), // A hit in the index, which returns the uuid of the cached request - cold('c', { c: { // the call to retrieve the cached request using the UUID from the index - c: { - completed: true - } + cold('c', { // the call to retrieve the cached request using the UUID from the index + c: { + completed: true } }) ]; @@ -204,10 +219,9 @@ describe('RequestService', () => { it(`it should return the cached request`, () => { const result = service.getByUUID(testUUID); - scheduler.expectObservable(result).toBe('c', { c: { - c: { - completed: true - } + scheduler.expectObservable(result).toBe('c', { + c: { + completed: true } }); }); diff --git a/src/app/core/data/request.service.ts b/src/app/core/data/request.service.ts index f69ad646f7..105d84cf4a 100644 --- a/src/app/core/data/request.service.ts +++ b/src/app/core/data/request.service.ts @@ -2,8 +2,8 @@ import { Injectable } from '@angular/core'; import { HttpHeaders } from '@angular/common/http'; import { createSelector, MemoizedSelector, select, Store } from '@ngrx/store'; -import { Observable, race as observableRace } from 'rxjs'; -import { filter, map, mergeMap, take, switchMap } from 'rxjs/operators'; +import { Observable, combineLatest as observableCombineLatest } from 'rxjs'; +import { filter, map, mergeMap, take, switchMap, startWith } from 'rxjs/operators'; import { cloneDeep, remove } from 'lodash'; import { hasValue, isEmpty, isNotEmpty, hasValueOperator } from '../../shared/empty.util'; import { CacheableObject } from '../cache/object-cache.reducer'; @@ -110,24 +110,19 @@ export class RequestService { * Retrieve a RequestEntry based on their uuid */ getByUUID(uuid: string): Observable { - return observableRace( + return observableCombineLatest([ this.store.pipe( - select(entryFromUUIDSelector(uuid)), - hasValueOperator() + select(entryFromUUIDSelector(uuid)) ), this.store.pipe( select(originalRequestUUIDFromRequestUUIDSelector(uuid)), switchMap((originalUUID) => { - if (hasValue(originalUUID)) { return this.store.pipe(select(entryFromUUIDSelector(originalUUID))) - } else { - return [] - } }, ), - hasValueOperator() - ) - ).pipe( + ), + ]).pipe( + map((entries: RequestEntry[]) => entries.find((entry: RequestEntry) => hasValue(entry))), map((entry: RequestEntry) => { // Headers break after being retrieved from the store (because of lazy initialization) // Combining them with a new object fixes this issue From d02c41c089a21b4b6ae10cb2eb361229cec07ffe Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Wed, 15 Apr 2020 15:04:31 +0200 Subject: [PATCH 5/7] ensure cache times are used for all types of requests --- src/app/core/cache/object-cache.service.ts | 1 + src/app/core/data/data.service.ts | 31 +++++++++++++++++-- src/app/core/data/relationship.service.ts | 9 +++++- .../dynamic-form-array.component.ts | 4 +-- 4 files changed, 39 insertions(+), 6 deletions(-) diff --git a/src/app/core/cache/object-cache.service.ts b/src/app/core/cache/object-cache.service.ts index d82a1f31fe..43842cf5fa 100644 --- a/src/app/core/cache/object-cache.service.ts +++ b/src/app/core/cache/object-cache.service.ts @@ -261,6 +261,7 @@ export class ObjectCacheService { const timeOutdated = entry.timeAdded + entry.msToLive; const isOutDated = new Date().getTime() > timeOutdated; if (isOutDated) { + console.log('removing', entry.data._links.self.href); this.store.dispatch(new RemoveFromObjectCacheAction(entry.data._links.self.href)); } return !isOutDated; diff --git a/src/app/core/data/data.service.ts b/src/app/core/data/data.service.ts index 5817a6acea..abf6332504 100644 --- a/src/app/core/data/data.service.ts +++ b/src/app/core/data/data.service.ts @@ -316,7 +316,9 @@ export abstract class DataService { ).subscribe((href: string) => { this.requestService.removeByHrefSubstring(href); const request = new FindListRequest(requestId, href, options); - request.responseMsToLive = 10 * 1000; + if (hasValue(this.responseMsToLive)) { + request.responseMsToLive = this.responseMsToLive; + } this.requestService.configure(request); }); @@ -343,6 +345,9 @@ export abstract class DataService { find((href: string) => hasValue(href)), map((href: string) => { const request = new PatchRequest(requestId, href, operations); + if (hasValue(this.responseMsToLive)) { + request.responseMsToLive = this.responseMsToLive; + } this.requestService.configure(request); }) ).subscribe(); @@ -362,6 +367,11 @@ export abstract class DataService { const requestId = this.requestService.generateRequestId(); const serializedObject = new DSpaceSerializer(object.constructor as GenericConstructor<{}>).serialize(object); const request = new PutRequest(requestId, object._links.self.href, serializedObject); + + if (hasValue(this.responseMsToLive)) { + request.responseMsToLive = this.responseMsToLive; + } + this.requestService.configure(request); return this.requestService.getByUUID(requestId).pipe( @@ -411,7 +421,13 @@ export abstract class DataService { const request$ = endpoint$.pipe( take(1), - map((endpoint: string) => new CreateRequest(requestId, endpoint, JSON.stringify(serializedDso))) + map((endpoint: string) => { + const request = new CreateRequest(requestId, endpoint, JSON.stringify(serializedDso)); + if (hasValue(this.responseMsToLive)) { + request.responseMsToLive = this.responseMsToLive; + } + return request + }) ); // Execute the post request @@ -460,7 +476,13 @@ export abstract class DataService { const request$ = endpoint$.pipe( take(1), - map((endpoint: string) => new CreateRequest(requestId, endpoint, JSON.stringify(serializedDso))) + map((endpoint: string) => { + const request = new CreateRequest(requestId, endpoint, JSON.stringify(serializedDso)); + if (hasValue(this.responseMsToLive)) { + request.responseMsToLive = this.responseMsToLive; + } + return request + }) ); // Execute the post request @@ -508,6 +530,9 @@ export abstract class DataService { ); } const request = new DeleteByIDRequest(requestId, href, dsoID); + if (hasValue(this.responseMsToLive)) { + request.responseMsToLive = this.responseMsToLive; + } this.requestService.configure(request); }) ).subscribe(); diff --git a/src/app/core/data/relationship.service.ts b/src/app/core/data/relationship.service.ts index a643ea2487..6d60220841 100644 --- a/src/app/core/data/relationship.service.ts +++ b/src/app/core/data/relationship.service.ts @@ -52,7 +52,13 @@ import { DefaultChangeAnalyzer } from './default-change-analyzer.service'; import { ItemDataService } from './item-data.service'; import { PaginatedList } from './paginated-list'; import { RemoteData, RemoteDataState } from './remote-data'; -import { DeleteRequest, FindListOptions, PostRequest, RestRequest } from './request.models'; +import { + DeleteRequest, + FindListOptions, + PostRequest, + RestRequest, + FindByIDRequest +} from './request.models'; import { RequestService } from './request.service'; import has = Reflect.has; @@ -86,6 +92,7 @@ const compareItemsByUUID = (itemCheck: Item) => @dataService(RELATIONSHIP) export class RelationshipService extends DataService { protected linkPath = 'relationships'; + protected responseMsToLive = 15 * 60 * 1000; constructor(protected itemService: ItemDataService, protected requestService: RequestService, diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.ts index 2243264158..fac70206d4 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.ts @@ -47,6 +47,7 @@ import { Store } from '@ngrx/store'; import { SubmissionService } from '../../../../../../submission/submission.service'; import { AppState } from '../../../../../../app.reducer'; import { followLink } from '../../../../../utils/follow-link-config.model'; +import { ObjectCacheService } from '../../../../../../core/cache/object-cache.service'; @Component({ selector: 'ds-dynamic-form-array', @@ -74,6 +75,7 @@ export class DsDynamicFormArrayComponent extends DynamicFormArrayComponent imple constructor(protected layoutService: DynamicFormLayoutService, protected validationService: DynamicFormValidationService, + protected objectCacheService: ObjectCacheService, protected relationshipService: RelationshipService, protected changeDetectorRef: ChangeDetectorRef, protected submissionObjectService: SubmissionObjectDataService, @@ -183,7 +185,6 @@ export class DsDynamicFormArrayComponent extends DynamicFormArrayComponent imple let hasMetadataField = false; this.reorderables.forEach((reorderable: Reorderable, index: number) => { if (reorderable.hasMoved) { - console.log('reorderable moved', reorderable); const prevIndex = reorderable.oldIndex; const updatedReorderable = reorderable.update().pipe(take(1)); updatedReorderables.push(updatedReorderable); @@ -192,7 +193,6 @@ export class DsDynamicFormArrayComponent extends DynamicFormArrayComponent imple updatedReorderable.subscribe((v) => { const reoMD = reorderable as ReorderableFormFieldMetadataValue; reoMD.model.value = reoMD.metadataValue; - console.log('reoMD', reoMD); this.onChange({ $event: { previousIndex: prevIndex }, context: { index }, From 2c3d8fd0316e9ce9dc601490b930ca061fcc743d Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Wed, 15 Apr 2020 15:31:06 +0200 Subject: [PATCH 6/7] remove console log --- src/app/core/cache/object-cache.service.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/app/core/cache/object-cache.service.ts b/src/app/core/cache/object-cache.service.ts index 43842cf5fa..d82a1f31fe 100644 --- a/src/app/core/cache/object-cache.service.ts +++ b/src/app/core/cache/object-cache.service.ts @@ -261,7 +261,6 @@ export class ObjectCacheService { const timeOutdated = entry.timeAdded + entry.msToLive; const isOutDated = new Date().getTime() > timeOutdated; if (isOutDated) { - console.log('removing', entry.data._links.self.href); this.store.dispatch(new RemoveFromObjectCacheAction(entry.data._links.self.href)); } return !isOutDated; From 9a68969eec6985a0f1f81ee174a7d372fb97af7a Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Wed, 15 Apr 2020 17:13:43 +0200 Subject: [PATCH 7/7] ensure PUTs are always executed after the PATCH for a move operation --- .../dynamic-form-array.component.ts | 28 ++++++++++--------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.ts index fac70206d4..c4ee3a0b79 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.ts @@ -34,7 +34,7 @@ import { } from '../../../../../../core/shared/operators'; import { SubmissionObject } from '../../../../../../core/submission/models/submission-object.model'; import { SubmissionObjectDataService } from '../../../../../../core/submission/submission-object-data.service'; -import { hasValue } from '../../../../../empty.util'; +import { hasValue, isNotEmpty } from '../../../../../empty.util'; import { FormFieldMetadataValueObject } from '../../../models/form-field-metadata-value.model'; import { Reorderable, @@ -181,16 +181,14 @@ export class DsDynamicFormArrayComponent extends DynamicFormArrayComponent imple this.reorderables = reorderables; if (shouldPropagateChanges) { - const updatedReorderables: Array> = []; + const movedReoRels: Array = []; let hasMetadataField = false; this.reorderables.forEach((reorderable: Reorderable, index: number) => { if (reorderable.hasMoved) { - const prevIndex = reorderable.oldIndex; - const updatedReorderable = reorderable.update().pipe(take(1)); - updatedReorderables.push(updatedReorderable); if (reorderable instanceof ReorderableFormFieldMetadataValue) { + const prevIndex = reorderable.oldIndex; hasMetadataField = true; - updatedReorderable.subscribe((v) => { + reorderable.update().pipe(take(1)).subscribe((v) => { const reoMD = reorderable as ReorderableFormFieldMetadataValue; reoMD.model.value = reoMD.metadataValue; this.onChange({ @@ -202,18 +200,22 @@ export class DsDynamicFormArrayComponent extends DynamicFormArrayComponent imple type: DynamicFormControlEventType.Change }); }); + } else if (reorderable instanceof ReorderableRelationship) { + movedReoRels.push(reorderable) } } }); - observableCombineLatest(updatedReorderables).pipe( + if (isNotEmpty(movedReoRels) && hasMetadataField && hasValue(this.model.relationshipConfig)) { + // if it's a mix between entities and regular metadata fields, + // we need to save, since they use different endpoints and + // otherwise they'll get out of sync. + this.submissionService.dispatchSave(this.model.submissionId); + } + + observableCombineLatest( + movedReoRels.map((movedReoRel) => movedReoRel.update().pipe(take(1))) ).subscribe(() => { - if (hasMetadataField && hasValue(this.model.relationshipConfig)) { - // if it's a mix between entities and regular metadata fields, - // we need to save after every operation, since they use different - // endpoints and otherwise they'll get out of sync. - this.submissionService.dispatchSave(this.model.submissionId); - } this.changeDetectorRef.detectChanges(); }); }