diff --git a/src/app/app.module.ts b/src/app/app.module.ts index a02997abdd..8469911406 100755 --- a/src/app/app.module.ts +++ b/src/app/app.module.ts @@ -46,7 +46,6 @@ export function getMetaReducers(config: GlobalConfig): Array { objectCacheReducer(testState, action); }); + it('should perform the ADD_PATCH action without affecting the previous state', () => { + const action = new AddPatchObjectCacheAction(selfLink1, [{ op: 'replace', path: '/name', value: 'random string' }]); + // testState has already been frozen above + objectCacheReducer(testState, action); + }); + + it('should perform the APPLY_PATCH action without affecting the previous state', () => { + const action = new ApplyPatchObjectCacheAction(selfLink1); + // testState has already been frozen above + objectCacheReducer(testState, action); + }); + }); diff --git a/src/app/core/cache/object-cache.service.spec.ts b/src/app/core/cache/object-cache.service.spec.ts index 38d51f09b3..5a1a3c5ac6 100644 --- a/src/app/core/cache/object-cache.service.spec.ts +++ b/src/app/core/cache/object-cache.service.spec.ts @@ -2,12 +2,20 @@ import { Store } from '@ngrx/store'; import { of as observableOf } from 'rxjs'; import { ObjectCacheService } from './object-cache.service'; -import { AddToObjectCacheAction, RemoveFromObjectCacheAction } from './object-cache.actions'; +import { + AddPatchObjectCacheAction, + AddToObjectCacheAction, ApplyPatchObjectCacheAction, + RemoveFromObjectCacheAction +} from './object-cache.actions'; import { CoreState } from '../core.reducers'; import { ResourceType } from '../shared/resource-type'; import { NormalizedItem } from './models/normalized-item.model'; import { first } from 'rxjs/operators'; import * as ngrx from '@ngrx/store'; +import { Operation } from '../../../../node_modules/fast-json-patch'; +import { RestRequestMethod } from '../data/rest-request-method'; +import { AddToSSBAction } from './server-sync-buffer.actions'; +import { Patch } from './object-cache.reducer'; describe('ObjectCacheService', () => { let service: ObjectCacheService; @@ -16,18 +24,29 @@ describe('ObjectCacheService', () => { const selfLink = 'https://rest.api/endpoint/1698f1d3-be98-4c51-9fd8-6bfedcbd59b7'; const timestamp = new Date().getTime(); const msToLive = 900000; - const objectToCache = { + let objectToCache = { self: selfLink, type: ResourceType.Item }; - const cacheEntry = { - data: objectToCache, - timeAdded: timestamp, - msToLive: msToLive - }; - const invalidCacheEntry = Object.assign({}, cacheEntry, { msToLive: -1 }); + let cacheEntry; + let invalidCacheEntry; + const operations = [{ op: 'replace', path: '/name', value: 'random string' } as Operation]; + + function init() { + objectToCache = { + self: selfLink, + type: ResourceType.Item + }; + cacheEntry = { + data: objectToCache, + timeAdded: timestamp, + msToLive: msToLive + }; + invalidCacheEntry = Object.assign({}, cacheEntry, { msToLive: -1 }) + } beforeEach(() => { + init(); store = new Store(undefined, undefined, undefined); spyOn(store, 'dispatch'); service = new ObjectCacheService(store); @@ -127,4 +146,30 @@ describe('ObjectCacheService', () => { }); }); + describe('patch methods', () => { + it('should dispatch the correct actions when addPatch is called', () => { + service.addPatch(selfLink, operations); + expect(store.dispatch).toHaveBeenCalledWith(new AddPatchObjectCacheAction(selfLink, operations)); + expect(store.dispatch).toHaveBeenCalledWith(new AddToSSBAction(selfLink, RestRequestMethod.PATCH)); + }); + + it('isDirty should return true when the patches list in the cache entry is not empty', () => { + cacheEntry.patches = [ + { + operations: operations + } as Patch]; + const result = (service as any).isDirty(cacheEntry); + expect(result).toBe(true); + }); + + it('isDirty should return false when the patches list in the cache entry is empty', () => { + cacheEntry.patches = []; + const result = (service as any).isDirty(cacheEntry); + expect(result).toBe(false); + }); + it('should dispatch the correct actions when applyPatchesToCachedObject is called', () => { + (service as any).applyPatchesToCachedObject(selfLink); + expect(store.dispatch).toHaveBeenCalledWith(new ApplyPatchObjectCacheAction(selfLink)); + }); + }); }); diff --git a/src/app/core/cache/object-cache.service.ts b/src/app/core/cache/object-cache.service.ts index 909a160068..3ac644a045 100644 --- a/src/app/core/cache/object-cache.service.ts +++ b/src/app/core/cache/object-cache.service.ts @@ -18,10 +18,9 @@ import { coreSelector, CoreState } from '../core.reducers'; import { pathSelector } from '../shared/selectors'; import { NormalizedObjectFactory } from './models/normalized-object-factory'; import { NormalizedObject } from './models/normalized-object.model'; -import { applyPatch, applyReducer, Operation } from 'fast-json-patch'; +import { applyPatch, Operation } from 'fast-json-patch'; import { AddToSSBAction } from './server-sync-buffer.actions'; import { RestRequestMethod } from '../data/rest-request-method'; -import { ReplaceOperation } from 'fast-json-patch/lib/core'; function selfLinkFromUuidSelector(uuid: string): MemoizedSelector { return pathSelector(coreSelector, 'index', IndexName.OBJECT, uuid); @@ -92,9 +91,13 @@ export class ObjectCacheService { getBySelfLink(selfLink: string): Observable { return this.getEntry(selfLink).pipe( map((entry: ObjectCacheEntry) => { - const flatPatch: Operation[] = [].concat(...entry.patches.map((patch) => patch.operations)); - const patchedData = applyPatch(entry.data, flatPatch, undefined, false).newDocument; - return Object.assign({}, entry, { data: patchedData }); + if (isNotEmpty(entry.patches)) { + const flatPatch: Operation[] = [].concat(...entry.patches.map((patch) => patch.operations)); + const patchedData = applyPatch(entry.data, flatPatch, undefined, false).newDocument; + return Object.assign({}, entry, { data: patchedData }); + } else { + return entry; + } } ), map((entry: ObjectCacheEntry) => { @@ -211,7 +214,6 @@ export class ObjectCacheService { } } - /** * Add operations to the existing list of operations for an ObjectCacheEntry * Makes sure the ServerSyncBuffer for this ObjectCacheEntry is updated @@ -220,9 +222,9 @@ export class ObjectCacheService { * @param {Operation[]} patch * list of operations to perform */ - public addPatch(uuid: string, patch: Operation[]) { - this.store.dispatch(new AddPatchObjectCacheAction(uuid, patch)); - this.store.dispatch(new AddToSSBAction(uuid, RestRequestMethod.PATCH)); + public addPatch(selfLink: string, patch: Operation[]) { + this.store.dispatch(new AddPatchObjectCacheAction(selfLink, patch)); + this.store.dispatch(new AddToSSBAction(selfLink, RestRequestMethod.PATCH)); } /** @@ -243,8 +245,8 @@ export class ObjectCacheService { * @param {string} uuid * the uuid of the ObjectCacheEntry */ - private applyPatchesToCachedObject(uuid: string) { - this.store.dispatch(new ApplyPatchObjectCacheAction(uuid)); + private applyPatchesToCachedObject(selfLink: string) { + this.store.dispatch(new ApplyPatchObjectCacheAction(selfLink)); } } diff --git a/src/app/core/cache/server-sync-buffer.actions.ts b/src/app/core/cache/server-sync-buffer.actions.ts index 69eb81b02f..638d837bea 100644 --- a/src/app/core/cache/server-sync-buffer.actions.ts +++ b/src/app/core/cache/server-sync-buffer.actions.ts @@ -64,6 +64,7 @@ export class EmptySSBAction implements Action { * * @param method * an optional method for which the ServerSyncBuffer should remove its entries + * if this parameter is omitted, the buffer will be emptied as a whole */ constructor(method?: RestRequestMethod) { this.payload = method; diff --git a/src/app/core/cache/server-sync-buffer.effects.spec.ts b/src/app/core/cache/server-sync-buffer.effects.spec.ts new file mode 100644 index 0000000000..0a8d50107e --- /dev/null +++ b/src/app/core/cache/server-sync-buffer.effects.spec.ts @@ -0,0 +1,139 @@ +import { TestBed } from '@angular/core/testing'; +import { Observable, of as observableOf } from 'rxjs'; +import { provideMockActions } from '@ngrx/effects/testing'; +import { cold, hot } from 'jasmine-marbles'; +import { ServerSyncBufferEffects } from './server-sync-buffer.effects'; +import { GLOBAL_CONFIG } from '../../../config'; +import { + CommitSSBAction, + EmptySSBAction, + ServerSyncBufferActionTypes +} from './server-sync-buffer.actions'; +import { RestRequestMethod } from '../data/rest-request-method'; +import { Store } from '@ngrx/store'; +import { RequestService } from '../data/request.service'; +import { ObjectCacheService } from './object-cache.service'; +import { MockStore } from '../../shared/testing/mock-store'; +import { ObjectCacheState } from './object-cache.reducer'; +import * as operators from 'rxjs/operators'; +import { spyOnOperator } from '../../shared/testing/utils'; +import { DSpaceObject } from '../shared/dspace-object.model'; +import { getMockRequestService } from '../../shared/mocks/mock-request.service'; +import { ApplyPatchObjectCacheAction } from './object-cache.actions'; + +describe('ServerSyncBufferEffects', () => { + let ssbEffects: ServerSyncBufferEffects; + let actions: Observable; + const testConfig = { + cache: + { + autoSync: + { + timePerMethod: {}, + defaultTime: 0 + } + } + }; + const selfLink = 'https://rest.api/endpoint/1698f1d3-be98-4c51-9fd8-6bfedcbd59b7'; + let store; + + beforeEach(() => { + store = new MockStore({}); + TestBed.configureTestingModule({ + providers: [ + ServerSyncBufferEffects, + provideMockActions(() => actions), + { provide: GLOBAL_CONFIG, useValue: testConfig }, + { provide: RequestService, useValue: getMockRequestService() }, + { + provide: ObjectCacheService, useValue: { + getBySelfLink: (link) => { + const object = new DSpaceObject(); + object.self = link; + return observableOf(object); + } + } + }, + { provide: Store, useValue: store } + // other providers + ], + }); + + ssbEffects = TestBed.get(ServerSyncBufferEffects); + }); + + describe('setTimeoutForServerSync', () => { + beforeEach(() => { + spyOnOperator(operators, 'delay').and.returnValue((v) => v); + }); + + it('should return a COMMIT action in response to an ADD action', () => { + actions = hot('a', { + a: { + type: ServerSyncBufferActionTypes.ADD, + payload: { href: selfLink, method: RestRequestMethod.PUT } + } + }); + + const expected = cold('b', { b: new CommitSSBAction(RestRequestMethod.PUT) }); + + expect(ssbEffects.setTimeoutForServerSync).toBeObservable(expected); + }); + }); + + describe('commitServerSyncBuffer', () => { + describe('when the buffer is not empty', () => { + beforeEach(() => { + store + .subscribe((state) => { + (state as any).core = Object({}); + (state as any).core['cache/syncbuffer'] = { + buffer: [{ + href: selfLink, + method: RestRequestMethod.PATCH + }] + }; + }); + }); + it('should return a list of actions in response to a COMMIT action', () => { + actions = hot('a', { + a: { + type: ServerSyncBufferActionTypes.COMMIT, + payload: RestRequestMethod.PATCH + } + }); + + const expected = cold('(bc)', { + b: new ApplyPatchObjectCacheAction(selfLink), + c: new EmptySSBAction(RestRequestMethod.PATCH) + }); + + expect(ssbEffects.commitServerSyncBuffer).toBeObservable(expected); + }); + }); + + describe('when the buffer is empty', () => { + beforeEach(() => { + store + .subscribe((state) => { + (state as any).core = Object({}); + (state as any).core['cache/syncbuffer'] = { + buffer: [] + }; + }); + }); + it('should return a placeholder action in response to a COMMIT action', () => { + store.subscribe(); + actions = hot('a', { + a: { + type: ServerSyncBufferActionTypes.COMMIT, + payload: { method: RestRequestMethod.PATCH } + } + }); + const expected = cold('b', { b: { type: 'NO_ACTION' } }); + + expect(ssbEffects.commitServerSyncBuffer).toBeObservable(expected); + }); + }); + }); +}); diff --git a/src/app/core/cache/server-sync-buffer.effects.ts b/src/app/core/cache/server-sync-buffer.effects.ts index d5988cb839..db2263c52a 100644 --- a/src/app/core/cache/server-sync-buffer.effects.ts +++ b/src/app/core/cache/server-sync-buffer.effects.ts @@ -25,6 +25,13 @@ import { RestRequestMethod } from '../data/rest-request-method'; @Injectable() export class ServerSyncBufferEffects { + + /** + * When an ADDToSSBAction is dispatched + * Set a time out (configurable per method type) + * Then dispatch a CommitSSBAction + * When the delay is running, no new AddToSSBActions are processed in this effect + */ @Effect() setTimeoutForServerSync = this.actions$ .pipe( ofType(ServerSyncBufferActionTypes.ADD), @@ -35,6 +42,12 @@ export class ServerSyncBufferEffects { }) ); + /** + * When a CommitSSBAction is dispatched + * Create a list of actions for each entry in the current buffer state to be dispatched + * When the list of actions is not empty, also dispatch an EmptySSBAction + * When the list is empty dispatch a NO_ACTION placeholder action + */ @Effect() commitServerSyncBuffer = this.actions$ .pipe( ofType(ServerSyncBufferActionTypes.COMMIT), @@ -55,7 +68,7 @@ export class ServerSyncBufferEffects { if (entry.method === RestRequestMethod.PATCH) { return this.applyPatch(entry.href); } else { - /* TODO other request stuff */ + /* TODO implement for other request method types */ } }); @@ -72,6 +85,12 @@ export class ServerSyncBufferEffects { }) ); + /** + * private method to create an ApplyPatchObjectCacheAction based on a cache entry + * and to do the actual patch request to the server + * @param {string} href The self link of the cache entry + * @returns {Observable} ApplyPatchObjectCacheAction to be dispatched + */ private applyPatch(href: string): Observable { const patchObject = this.objectCache.getBySelfLink(href).pipe(first()); diff --git a/src/app/core/cache/server-sync-buffer.reducer.spec.ts b/src/app/core/cache/server-sync-buffer.reducer.spec.ts new file mode 100644 index 0000000000..666144104b --- /dev/null +++ b/src/app/core/cache/server-sync-buffer.reducer.spec.ts @@ -0,0 +1,93 @@ +import * as deepFreeze from 'deep-freeze'; + +import { objectCacheReducer } from './object-cache.reducer'; +import { + AddPatchObjectCacheAction, + AddToObjectCacheAction, ApplyPatchObjectCacheAction, + RemoveFromObjectCacheAction, + ResetObjectCacheTimestampsAction +} from './object-cache.actions'; +import { Operation } from '../../../../node_modules/fast-json-patch'; +import { serverSyncBufferReducer } from './server-sync-buffer.reducer'; +import { RestRequestMethod } from '../data/rest-request-method'; +import { AddToSSBAction, EmptySSBAction } from './server-sync-buffer.actions'; + +class NullAction extends RemoveFromObjectCacheAction { + type = null; + payload = null; + + constructor() { + super(null); + } +} + +describe('serverSyncBufferReducer', () => { + const selfLink1 = 'https://localhost:8080/api/core/items/1698f1d3-be98-4c51-9fd8-6bfedcbd59b7'; + const selfLink2 = 'https://localhost:8080/api/core/items/28b04544-1766-4e82-9728-c4e93544ecd3'; + const testState = { + buffer: + [ + { + href: selfLink1, + method: RestRequestMethod.PATCH, + }, + { + href: selfLink2, + method: RestRequestMethod.GET, + } + ] + }; + const newSelfLink = 'https://localhost:8080/api/core/items/1ce6b5ae-97e1-4e5a-b4b0-f9029bad10c0'; + + deepFreeze(testState); + + it('should return the current state when no valid actions have been made', () => { + const action = new NullAction(); + const newState = serverSyncBufferReducer(testState, action); + + expect(newState).toEqual(testState); + }); + + it('should start with an empty buffer array', () => { + const action = new NullAction(); + const initialState = serverSyncBufferReducer(undefined, action); + + expect(initialState).toEqual({ buffer: [] }); + }); + + it('should perform the ADD action without affecting the previous state', () => { + const action = new AddToSSBAction(selfLink1, RestRequestMethod.POST); + // testState has already been frozen above + serverSyncBufferReducer(testState, action); + }); + + it('should perform the EMPTY action without affecting the previous state', () => { + const action = new EmptySSBAction(); + // testState has already been frozen above + serverSyncBufferReducer(testState, action); + }); + + it('should empty the buffer if the EmptySSBAction is dispatched without a payload', () => { + const action = new EmptySSBAction(); + // testState has already been frozen above + const emptyState = serverSyncBufferReducer(testState, action); + expect(emptyState).toEqual({ buffer: [] }); + }); + + it('should empty the buffer partially if the EmptySSBAction is dispatched with a payload', () => { + const action = new EmptySSBAction(RestRequestMethod.PATCH); + // testState has already been frozen above + const emptyState = serverSyncBufferReducer(testState, action); + expect(emptyState).toEqual({ buffer: testState.buffer.filter((entry) => entry.method !== RestRequestMethod.PATCH) }); + }); + + it('should add an entry to the buffer if the AddSSBAction is dispatched', () => { + const action = new AddToSSBAction(newSelfLink, RestRequestMethod.PUT); + // testState has already been frozen above + const newState = serverSyncBufferReducer(testState, action); + expect(newState.buffer).toContain({ + href: newSelfLink, method: RestRequestMethod.PUT + }) + ; + }) +}); diff --git a/src/app/core/data/data.service.spec.ts b/src/app/core/data/data.service.spec.ts index e0df5c871e..e60725c4c2 100644 --- a/src/app/core/data/data.service.spec.ts +++ b/src/app/core/data/data.service.spec.ts @@ -10,6 +10,9 @@ import { Observable } from 'rxjs'; import { FindAllOptions } from './request.models'; import { SortOptions, SortDirection } from '../cache/models/sort-options.model'; import { of as observableOf } from 'rxjs'; +import { ObjectCacheService } from '../cache/object-cache.service'; +import { Operation } from '../../../../node_modules/fast-json-patch'; +import { DSpaceObject } from '../shared/dspace-object.model'; const endpoint = 'https://rest.api/core'; @@ -42,7 +45,14 @@ describe('DataService', () => { const requestService = {} as RequestService; const halService = {} as HALEndpointService; const rdbService = {} as RemoteDataBuildService; - const objectCache = {} as ObjectCacheService; + const objectCache = { + addPatch: () => { + /* empty */ + }, + getBySelfLink: () => { + /* empty */ + } + } as any; const store = {} as Store; function initTestService(): TestService { @@ -52,7 +62,8 @@ describe('DataService', () => { rdbService, store, endpoint, - halService + halService, + objectCache ); } @@ -122,5 +133,53 @@ describe('DataService', () => { }); }) }); + describe('patch', () => { + let operations; + let selfLink; + beforeEach(() => { + operations = [{ op: 'replace', path: '/name', value: 'random string' } as Operation]; + selfLink = 'https://rest.api/endpoint/1698f1d3-be98-4c51-9fd8-6bfedcbd59b7'; + spyOn(objectCache, 'addPatch'); + }); + + it('should call addPatch on the object cache with the right parameters', () => { + service.patch(selfLink, operations); + expect(objectCache.addPatch).toHaveBeenCalledWith(selfLink, operations); + }); + }); + + describe('update', () => { + let operations; + let selfLink; + let dso; + let dso2; + const name1 = 'random string'; + const name2 = 'another random string'; + beforeEach(() => { + operations = [{ op: 'replace', path: '/name', value: name2 } as Operation]; + selfLink = 'https://rest.api/endpoint/1698f1d3-be98-4c51-9fd8-6bfedcbd59b7'; + + dso = new DSpaceObject(); + dso.self = selfLink; + dso.name = name1; + + dso2 = new DSpaceObject(); + dso2.self = selfLink; + dso2.name = name2; + + spyOn(objectCache, 'getBySelfLink').and.returnValue(dso); + spyOn(objectCache, 'addPatch'); + }); + + it('should call addPatch on the object cache with the right parameters when there are differences', () => { + service.update(dso2); + expect(objectCache.addPatch).toHaveBeenCalledWith(selfLink, operations); + }); + + it('should not call addPatch on the object cache with the right parameters when there are no differences', () => { + service.update(dso); + expect(objectCache.addPatch).not.toHaveBeenCalled(); + }); + }); }); diff --git a/src/app/core/data/data.service.ts b/src/app/core/data/data.service.ts index 127a8f8afd..d23d7e8064 100644 --- a/src/app/core/data/data.service.ts +++ b/src/app/core/data/data.service.ts @@ -1,6 +1,5 @@ -import { distinctUntilChanged, filter, take, first, map } from 'rxjs/operators'; -import { of as observableOf, Observable } from 'rxjs'; -import {mergeMap, first, take, distinctUntilChanged, map, filter} from 'rxjs/operators'; +import { distinctUntilChanged, filter, first, map, take } from 'rxjs/operators'; +import { Observable } from 'rxjs'; import { Store } from '@ngrx/store'; import { hasValue, isNotEmpty } from '../../shared/empty.util'; import { RemoteDataBuildService } from '../cache/builders/remote-data-build.service'; @@ -95,14 +94,26 @@ export abstract class DataService return this.rdbService.buildSingle(href); } + /** + * Add a new patch to the object cache to a specified object + * @param {string} href The selflink of the object that will be patched + * @param {Operation[]} operations The patch operations to be performed + */ patch(href: string, operations: Operation[]) { this.objectCache.addPatch(href, operations); } + /** + * Add a new patch to the object cache + * The patch is derived from the differences between the given object and its version in the object cache + * @param {DSpaceObject} object The given object + */ update(object: DSpaceObject) { const oldVersion = this.objectCache.getBySelfLink(object.self); const operations = compare(oldVersion, object); - this.objectCache.addPatch(object.self, operations); + if (isNotEmpty(operations)) { + this.objectCache.addPatch(object.self, operations); + } } // TODO implement, after the structure of the REST server's POST response is finalized // create(dso: DSpaceObject): Observable> { diff --git a/src/app/core/shared/dspace-object.model.ts b/src/app/core/shared/dspace-object.model.ts index 3a40d142aa..68338143ba 100644 --- a/src/app/core/shared/dspace-object.model.ts +++ b/src/app/core/shared/dspace-object.model.ts @@ -10,7 +10,7 @@ import { autoserialize } from 'cerialize'; /** * An abstract model class for a DSpaceObject. */ -export class DSpaceObject implements CacheableObject, ListableObject { +export class DSpaceObject implements CacheableObject, ListableObject { self: string; diff --git a/src/app/shared/auth-nav-menu/auth-nav-menu.component.ts b/src/app/shared/auth-nav-menu/auth-nav-menu.component.ts index dd2e6202ce..fc85616de9 100644 --- a/src/app/shared/auth-nav-menu/auth-nav-menu.component.ts +++ b/src/app/shared/auth-nav-menu/auth-nav-menu.component.ts @@ -1,6 +1,6 @@ -import { of as observableOf, Observable , Subscription } from 'rxjs'; +import { Observable, of as observableOf, Subscription } from 'rxjs'; -import { map, filter } from 'rxjs/operators'; +import { filter, map } from 'rxjs/operators'; import { Component, OnInit } from '@angular/core'; import { RouterReducerState } from '@ngrx/router-store'; import { select, Store } from '@ngrx/store'; @@ -16,7 +16,6 @@ import { } from '../../core/auth/selectors'; import { EPerson } from '../../core/eperson/models/eperson.model'; import { AuthService, LOGIN_ROUTE, LOGOUT_ROUTE } from '../../core/auth/auth.service'; -import { Subscription } from 'rxjs'; @Component({ selector: 'ds-auth-nav-menu', diff --git a/src/app/shared/testing/utils.ts b/src/app/shared/testing/utils.ts index 9343ae50fd..8714358100 100644 --- a/src/app/shared/testing/utils.ts +++ b/src/app/shared/testing/utils.ts @@ -30,3 +30,15 @@ export const createTestComponent = (html: string, type: { new(...args: any[]) fixture.detectChanges(); return fixture as ComponentFixture; }; + +export function spyOnOperator(obj: any, prop: string): any { + const oldProp = obj[prop]; + Object.defineProperty(obj, prop, { + configurable: true, + enumerable: true, + value: oldProp, + writable: true + }); + + return spyOn(obj, prop); +} \ No newline at end of file