From 2fe5587e0257a9f7587a01cefcf11a144aea582f Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Thu, 16 Dec 2021 14:17:28 +0100 Subject: [PATCH 1/6] move the correlation id to the ngrx store --- src/app/app.module.ts | 15 ----- src/app/app.reducer.ts | 3 + src/app/core/log/log.interceptor.ts | 9 ++- .../correlation-id/correlation-id.actions.ts | 15 +++++ .../correlation-id/correlation-id.reducer.ts | 21 ++++++ .../correlation-id/correlation-id.service.ts | 64 +++++++++++++++++++ src/modules/app/browser-app.module.ts | 17 ++--- src/modules/app/server-app.module.ts | 16 +++-- 8 files changed, 125 insertions(+), 35 deletions(-) create mode 100644 src/app/correlation-id/correlation-id.actions.ts create mode 100644 src/app/correlation-id/correlation-id.reducer.ts create mode 100644 src/app/correlation-id/correlation-id.service.ts diff --git a/src/app/app.module.ts b/src/app/app.module.ts index 98b2d9fe92..4a404e3200 100755 --- a/src/app/app.module.ts +++ b/src/app/app.module.ts @@ -156,21 +156,6 @@ const PROVIDERS = [ useClass: LogInterceptor, multi: true }, - // insert the unique id of the user that is using the application utilizing cookies - { - provide: APP_INITIALIZER, - useFactory: (cookieService: CookieService, uuidService: UUIDService) => { - const correlationId = cookieService.get('CORRELATION-ID'); - - // Check if cookie exists, if don't, set it with unique id - if (!correlationId) { - cookieService.set('CORRELATION-ID', uuidService.generate()); - } - return () => true; - }, - multi: true, - deps: [CookieService, UUIDService] - }, { provide: DYNAMIC_ERROR_MESSAGES_MATCHER, useValue: ValidateEmailErrorStateMatcher diff --git a/src/app/app.reducer.ts b/src/app/app.reducer.ts index a02095d834..5bd4f745d9 100644 --- a/src/app/app.reducer.ts +++ b/src/app/app.reducer.ts @@ -49,6 +49,7 @@ import { import { sidebarReducer, SidebarState } from './shared/sidebar/sidebar.reducer'; import { truncatableReducer, TruncatablesState } from './shared/truncatable/truncatable.reducer'; import { ThemeState, themeReducer } from './shared/theme-support/theme.reducer'; +import { correlationIdReducer } from './correlation-id/correlation-id.reducer'; export interface AppState { router: fromRouter.RouterReducerState; @@ -69,6 +70,7 @@ export interface AppState { communityList: CommunityListState; epeopleRegistry: EPeopleRegistryState; groupRegistry: GroupRegistryState; + correlationId: string; } export const appReducers: ActionReducerMap = { @@ -90,6 +92,7 @@ export const appReducers: ActionReducerMap = { communityList: CommunityListReducer, epeopleRegistry: ePeopleRegistryReducer, groupRegistry: groupRegistryReducer, + correlationId: correlationIdReducer }; export const routerStateSelector = (state: AppState) => state.router; diff --git a/src/app/core/log/log.interceptor.ts b/src/app/core/log/log.interceptor.ts index bf843f1da8..aff1a24963 100644 --- a/src/app/core/log/log.interceptor.ts +++ b/src/app/core/log/log.interceptor.ts @@ -3,9 +3,8 @@ import { HttpEvent, HttpHandler, HttpInterceptor, HttpRequest } from '@angular/c import { Router } from '@angular/router'; import { Observable } from 'rxjs'; - -import { CookieService } from '../services/cookie.service'; import { hasValue } from '../../shared/empty.util'; +import { CorrelationIdService } from '../../correlation-id/correlation-id.service'; /** * Log Interceptor intercepting Http Requests & Responses to @@ -15,12 +14,12 @@ import { hasValue } from '../../shared/empty.util'; @Injectable() export class LogInterceptor implements HttpInterceptor { - constructor(private cookieService: CookieService, private router: Router) {} + constructor(private cidService: CorrelationIdService, private router: Router) {} intercept(request: HttpRequest, next: HttpHandler): Observable> { - // Get Unique id of the user from the cookies - const correlationId = this.cookieService.get('CORRELATION-ID'); + // Get the correlation id for the user from the store + const correlationId = this.cidService.getCorrelationId(); // Add headers from the intercepted request let headers = request.headers; diff --git a/src/app/correlation-id/correlation-id.actions.ts b/src/app/correlation-id/correlation-id.actions.ts new file mode 100644 index 0000000000..0901974a4c --- /dev/null +++ b/src/app/correlation-id/correlation-id.actions.ts @@ -0,0 +1,15 @@ +import { type } from '../shared/ngrx/type'; +import { Action } from '@ngrx/store'; + +export const CorrelationIDActionTypes = { + SET: type('dspace/core/correlationId/SET') +}; + +export class SetCorrelationIdAction implements Action { + type = CorrelationIDActionTypes.SET; + + constructor(public payload: string) { + } +} + +export type CorrelationIdAction = SetCorrelationIdAction; diff --git a/src/app/correlation-id/correlation-id.reducer.ts b/src/app/correlation-id/correlation-id.reducer.ts new file mode 100644 index 0000000000..e8071a3108 --- /dev/null +++ b/src/app/correlation-id/correlation-id.reducer.ts @@ -0,0 +1,21 @@ +import { + CorrelationIdAction, + CorrelationIDActionTypes, + SetCorrelationIdAction +} from './correlation-id.actions'; +import { AppState } from '../app.reducer'; + +const initialState = null; + +export const correlationIdSelector = (state: AppState) => state.correlationId; + +export const correlationIdReducer = (state = initialState, action: CorrelationIdAction): string => { + switch (action.type) { + case CorrelationIDActionTypes.SET: { + return (action as SetCorrelationIdAction).payload; + } + default: { + return state; + } + } +}; diff --git a/src/app/correlation-id/correlation-id.service.ts b/src/app/correlation-id/correlation-id.service.ts new file mode 100644 index 0000000000..6f4b2a5341 --- /dev/null +++ b/src/app/correlation-id/correlation-id.service.ts @@ -0,0 +1,64 @@ +import { CookieService } from '../core/services/cookie.service'; +import { UUIDService } from '../core/shared/uuid.service'; +import { Store, select } from '@ngrx/store'; +import { AppState } from '../app.reducer'; +import { isEmpty } from '../shared/empty.util'; +import { correlationIdSelector } from './correlation-id.reducer'; +import { take } from 'rxjs/operators'; +import { SetCorrelationIdAction } from './correlation-id.actions'; +import { Injectable } from '@angular/core'; + +/** + * Service to manage the correlation id, an id used to give context to server side logs + */ +@Injectable({ + providedIn: 'root' +}) +export class CorrelationIdService { + + constructor( + protected cookieService: CookieService, + protected uuidService: UUIDService, + protected store: Store, + ) { + } + + /** + * Initialize the correlation id based on the cookie or the ngrx store + */ + initCorrelationId(): void { + // first see of there's a cookie with a correlation-id + let correlationId = this.cookieService.get('CORRELATION-ID'); + + // if there isn't see if there's an ID in the store + if (isEmpty(correlationId)) { + correlationId = this.getCorrelationId(); + } + + // if no id was found, create a new id + if (isEmpty(correlationId)) { + correlationId = this.uuidService.generate(); + } + + // Store the correct id both in the store and as a cookie to ensure they're in sync + this.store.dispatch(new SetCorrelationIdAction(correlationId)); + this.cookieService.set('CORRELATION-ID', correlationId); + } + + /** + * Get the correlation id from the store + */ + getCorrelationId(): string { + let correlationId; + + this.store.pipe( + select(correlationIdSelector), + take(1) + ).subscribe((storeId: string) => { + // we can do this because ngrx selects are synchronous + correlationId = storeId; + }); + + return correlationId; + } +} diff --git a/src/modules/app/browser-app.module.ts b/src/modules/app/browser-app.module.ts index 86497d4599..88a59eb157 100644 --- a/src/modules/app/browser-app.module.ts +++ b/src/modules/app/browser-app.module.ts @@ -36,6 +36,7 @@ import { BrowserAuthRequestService } from '../../app/core/auth/browser-auth-requ import { AppConfig, APP_CONFIG_STATE } from '../../config/app-config.interface'; import { DefaultAppConfig } from '../../config/default-app-config'; import { extendEnvironmentWithAppConfig } from '../../config/config.util'; +import { CorrelationIdService } from '../../app/correlation-id/correlation-id.service'; import { environment } from '../../environments/environment'; @@ -81,16 +82,21 @@ export function getRequest(transferState: TransferState): any { providers: [ { provide: APP_INITIALIZER, - useFactory: (transferState: TransferState) => { + useFactory: ( + transferState: TransferState, + dspaceTransferState: DSpaceTransferState, + correlationIdService: CorrelationIdService + ) => { if (transferState.hasKey(APP_CONFIG_STATE)) { const appConfig = transferState.get(APP_CONFIG_STATE, new DefaultAppConfig()); // extend environment with app config for browser extendEnvironmentWithAppConfig(environment, appConfig); } - + dspaceTransferState.transfer(); + correlationIdService.initCorrelationId(); return () => true; }, - deps: [TransferState], + deps: [TransferState, DSpaceTransferState, CorrelationIdService], multi: true }, { @@ -137,9 +143,4 @@ export function getRequest(transferState: TransferState): any { ] }) export class BrowserAppModule { - constructor( - private transferState: DSpaceTransferState, - ) { - this.transferState.transfer(); - } } diff --git a/src/modules/app/server-app.module.ts b/src/modules/app/server-app.module.ts index 8c49554de9..01a5548948 100644 --- a/src/modules/app/server-app.module.ts +++ b/src/modules/app/server-app.module.ts @@ -32,6 +32,7 @@ import { ServerHardRedirectService } from '../../app/core/services/server-hard-r import { Angulartics2Mock } from '../../app/shared/mocks/angulartics2.service.mock'; import { AuthRequestService } from '../../app/core/auth/auth-request.service'; import { ServerAuthRequestService } from '../../app/core/auth/server-auth-request.service'; +import { CorrelationIdService } from '../../app/correlation-id/correlation-id.service'; import { AppConfig, APP_CONFIG_STATE } from '../../config/app-config.interface'; import { environment } from '../../environments/environment'; @@ -65,11 +66,17 @@ export function createTranslateLoader() { // Initialize app config and extend environment { provide: APP_INITIALIZER, - useFactory: (transferState: TransferState) => { + useFactory: ( + transferState: TransferState, + dspaceTransferState: DSpaceTransferState, + correlationIdService: CorrelationIdService, + ) => { transferState.set(APP_CONFIG_STATE, environment as AppConfig); + dspaceTransferState.transfer(); + correlationIdService.initCorrelationId(); return () => true; }, - deps: [TransferState], + deps: [TransferState, DSpaceTransferState, CorrelationIdService], multi: true }, { @@ -117,9 +124,4 @@ export function createTranslateLoader() { ] }) export class ServerAppModule { - constructor( - private transferState: DSpaceTransferState, - ) { - this.transferState.transfer(); - } } From 3c8c425843985d2bd461690aed876dfe0d850279 Mon Sep 17 00:00:00 2001 From: Yura Bondarenko Date: Fri, 17 Dec 2021 17:05:53 +0100 Subject: [PATCH 2/6] 86016: Add unit tests for correlationId reducer & service --- .../correlation-id.reducer.spec.ts | 23 +++++ .../correlation-id.service.spec.ts | 83 +++++++++++++++++++ 2 files changed, 106 insertions(+) create mode 100644 src/app/correlation-id/correlation-id.reducer.spec.ts create mode 100644 src/app/correlation-id/correlation-id.service.spec.ts diff --git a/src/app/correlation-id/correlation-id.reducer.spec.ts b/src/app/correlation-id/correlation-id.reducer.spec.ts new file mode 100644 index 0000000000..c784def1d9 --- /dev/null +++ b/src/app/correlation-id/correlation-id.reducer.spec.ts @@ -0,0 +1,23 @@ +import { correlationIdReducer } from './correlation-id.reducer'; +import { SetCorrelationIdAction } from './correlation-id.actions'; + +describe('correlationIdReducer', () => { + it('should set the correlatinId with SET action', () => { + const initialState = null; + const currentState = correlationIdReducer(initialState, new SetCorrelationIdAction('new ID')); + + expect(currentState).toBe('new ID'); + }); + + it('should leave correlatinId unchanged otherwise', () => { + const initialState = null; + + let currentState = correlationIdReducer(initialState, { type: 'unknown' } as any); + expect(currentState).toBe(null); + + currentState = correlationIdReducer(currentState, new SetCorrelationIdAction('new ID')); + currentState = correlationIdReducer(currentState, { type: 'unknown' } as any); + + expect(currentState).toBe('new ID'); + }); +}); diff --git a/src/app/correlation-id/correlation-id.service.spec.ts b/src/app/correlation-id/correlation-id.service.spec.ts new file mode 100644 index 0000000000..003924c123 --- /dev/null +++ b/src/app/correlation-id/correlation-id.service.spec.ts @@ -0,0 +1,83 @@ +import { CorrelationIdService } from './correlation-id.service'; +import { CookieServiceMock } from '../shared/mocks/cookie.service.mock'; +import { UUIDService } from '../core/shared/uuid.service'; +import { MockStore, provideMockStore } from '@ngrx/store/testing'; +import { TestBed } from '@angular/core/testing'; +import { Store, StoreModule } from '@ngrx/store'; +import { appReducers, AppState, storeModuleConfig } from '../app.reducer'; +import { SetCorrelationIdAction } from './correlation-id.actions'; + +fdescribe('CorrelationIdService', () => { + let service: CorrelationIdService; + + let cookieService; + let uuidService; + let store; + + beforeEach(async () => { + await TestBed.configureTestingModule({ + imports: [ + StoreModule.forRoot(appReducers, storeModuleConfig), + ], + }).compileComponents(); + }); + + beforeEach(() => { + cookieService = new CookieServiceMock(); + uuidService = new UUIDService(); + store = TestBed.inject(Store) as MockStore; + service = new CorrelationIdService(cookieService, uuidService, store); + }); + + describe('getCorrelationId', () => { + it('should get from from store', () => { + expect(service.getCorrelationId()).toBe(null); + store.dispatch(new SetCorrelationIdAction('some value')); + expect(service.getCorrelationId()).toBe('some value'); + }); + }); + + + describe('initCorrelationId', () => { + const cookieCID = 'cookie CID'; + const storeCID = 'store CID'; + + it('should set cookie and store values to a newly generated value if neither ex', () => { + service.initCorrelationId(); + + expect(cookieService.get('CORRELATION-ID')).toBeTruthy(); + expect(service.getCorrelationId()).toBeTruthy(); + expect(cookieService.get('CORRELATION-ID')).toEqual(service.getCorrelationId()); + }); + + it('should set store value to cookie value if present', () => { + expect(service.getCorrelationId()).toBe(null); + + cookieService.set('CORRELATION-ID', cookieCID); + + service.initCorrelationId(); + + expect(cookieService.get('CORRELATION-ID')).toBe(cookieCID); + expect(service.getCorrelationId()).toBe(cookieCID); + }); + + it('should set cookie value to store value if present', () => { + store.dispatch(new SetCorrelationIdAction(storeCID)); + + service.initCorrelationId(); + + expect(cookieService.get('CORRELATION-ID')).toBe(storeCID); + expect(service.getCorrelationId()).toBe(storeCID); + }); + + it('should set store value to cookie value if both are present', () => { + cookieService.set('CORRELATION-ID', cookieCID); + store.dispatch(new SetCorrelationIdAction(storeCID)); + + service.initCorrelationId(); + + expect(cookieService.get('CORRELATION-ID')).toBe(cookieCID); + expect(service.getCorrelationId()).toBe(cookieCID); + }); + }); +}); From 50d8719c41d1ecf7d069b84403353aa6c34c170f Mon Sep 17 00:00:00 2001 From: Yura Bondarenko Date: Mon, 20 Dec 2021 16:09:08 +0100 Subject: [PATCH 3/6] Remove stray fdescribe --- src/app/correlation-id/correlation-id.service.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/correlation-id/correlation-id.service.spec.ts b/src/app/correlation-id/correlation-id.service.spec.ts index 003924c123..64a4d1068a 100644 --- a/src/app/correlation-id/correlation-id.service.spec.ts +++ b/src/app/correlation-id/correlation-id.service.spec.ts @@ -7,7 +7,7 @@ import { Store, StoreModule } from '@ngrx/store'; import { appReducers, AppState, storeModuleConfig } from '../app.reducer'; import { SetCorrelationIdAction } from './correlation-id.actions'; -fdescribe('CorrelationIdService', () => { +describe('CorrelationIdService', () => { let service: CorrelationIdService; let cookieService; From a2515c11e1623ab30d99505977f903f231c441ff Mon Sep 17 00:00:00 2001 From: Yura Bondarenko Date: Tue, 21 Dec 2021 09:51:56 +0100 Subject: [PATCH 4/6] 86016: Update log.interceptor.spec.ts --- src/app/core/log/log.interceptor.spec.ts | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/app/core/log/log.interceptor.spec.ts b/src/app/core/log/log.interceptor.spec.ts index 9bda4b7934..cae9c32202 100644 --- a/src/app/core/log/log.interceptor.spec.ts +++ b/src/app/core/log/log.interceptor.spec.ts @@ -9,12 +9,17 @@ import { RestRequestMethod } from '../data/rest-request-method'; import { CookieService } from '../services/cookie.service'; import { CookieServiceMock } from '../../shared/mocks/cookie.service.mock'; import { RouterStub } from '../../shared/testing/router.stub'; +import { CorrelationIdService } from '../../correlation-id/correlation-id.service'; +import { UUIDService } from '../shared/uuid.service'; +import { StoreModule } from '@ngrx/store'; +import { appReducers, storeModuleConfig } from '../../app.reducer'; describe('LogInterceptor', () => { let service: DspaceRestService; let httpMock: HttpTestingController; let cookieService: CookieService; + let correlationIdService: CorrelationIdService; const router = Object.assign(new RouterStub(),{url : '/statistics'}); // Mock payload/statuses are dummy content as we are not testing the results @@ -28,7 +33,10 @@ describe('LogInterceptor', () => { beforeEach(() => { TestBed.configureTestingModule({ - imports: [HttpClientTestingModule], + imports: [ + HttpClientTestingModule, + StoreModule.forRoot(appReducers, storeModuleConfig), + ], providers: [ DspaceRestService, // LogInterceptor, @@ -39,14 +47,18 @@ describe('LogInterceptor', () => { }, { provide: CookieService, useValue: new CookieServiceMock() }, { provide: Router, useValue: router }, + { provide: CorrelationIdService, useClass: CorrelationIdService }, + { provide: UUIDService, useClass: UUIDService }, ], }); - service = TestBed.get(DspaceRestService); - httpMock = TestBed.get(HttpTestingController); - cookieService = TestBed.get(CookieService); + service = TestBed.inject(DspaceRestService); + httpMock = TestBed.inject(HttpTestingController); + cookieService = TestBed.inject(CookieService); + correlationIdService = TestBed.inject(CorrelationIdService); cookieService.set('CORRELATION-ID','123455'); + correlationIdService.initCorrelationId(); }); From c1e8bbbeaeb00e0d160f6f8b8189dbb89a1b0f79 Mon Sep 17 00:00:00 2001 From: Yura Bondarenko Date: Thu, 23 Dec 2021 16:42:14 +0100 Subject: [PATCH 5/6] 86016: Add typedocs --- src/app/correlation-id/correlation-id.actions.ts | 6 ++++++ src/app/correlation-id/correlation-id.reducer.ts | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/src/app/correlation-id/correlation-id.actions.ts b/src/app/correlation-id/correlation-id.actions.ts index 0901974a4c..d57d41a637 100644 --- a/src/app/correlation-id/correlation-id.actions.ts +++ b/src/app/correlation-id/correlation-id.actions.ts @@ -5,6 +5,9 @@ export const CorrelationIDActionTypes = { SET: type('dspace/core/correlationId/SET') }; +/** + * Action for setting a new correlation ID + */ export class SetCorrelationIdAction implements Action { type = CorrelationIDActionTypes.SET; @@ -12,4 +15,7 @@ export class SetCorrelationIdAction implements Action { } } +/** + * Type alias for all correlation ID actions + */ export type CorrelationIdAction = SetCorrelationIdAction; diff --git a/src/app/correlation-id/correlation-id.reducer.ts b/src/app/correlation-id/correlation-id.reducer.ts index e8071a3108..b7525b0b1c 100644 --- a/src/app/correlation-id/correlation-id.reducer.ts +++ b/src/app/correlation-id/correlation-id.reducer.ts @@ -9,6 +9,12 @@ const initialState = null; export const correlationIdSelector = (state: AppState) => state.correlationId; +/** + * Reducer that handles actions to update the correlation ID + * @param {string} state the previous correlation ID (null if unset) + * @param {CorrelationIdAction} action the action to perform + * @return {string} the new correlation ID + */ export const correlationIdReducer = (state = initialState, action: CorrelationIdAction): string => { switch (action.type) { case CorrelationIDActionTypes.SET: { From fc059520a007cc11482b95c34281cfc5a8430a07 Mon Sep 17 00:00:00 2001 From: Yura Bondarenko Date: Thu, 23 Dec 2021 17:09:44 +0100 Subject: [PATCH 6/6] Fix LGTM issues --- src/app/app.module.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/app/app.module.ts b/src/app/app.module.ts index 4a404e3200..32c3c78348 100755 --- a/src/app/app.module.ts +++ b/src/app/app.module.ts @@ -55,9 +55,6 @@ import { ThemedBreadcrumbsComponent } from './breadcrumbs/themed-breadcrumbs.com import { ThemedHeaderNavbarWrapperComponent } from './header-nav-wrapper/themed-header-navbar-wrapper.component'; import { IdleModalComponent } from './shared/idle-modal/idle-modal.component'; -import { UUIDService } from './core/shared/uuid.service'; -import { CookieService } from './core/services/cookie.service'; - import { AppConfig, APP_CONFIG } from '../config/app-config.interface'; export function getConfig() {