From fbaa2327c614bdc66d40fd12b21670223b41238c Mon Sep 17 00:00:00 2001 From: Yury Shkoda Date: Fri, 23 Jul 2021 12:44:34 +0300 Subject: [PATCH 1/7] fix(session): remove retry limit if could not get state --- src/SessionManager.ts | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/src/SessionManager.ts b/src/SessionManager.ts index eb195a0..d4f002a 100644 --- a/src/SessionManager.ts +++ b/src/SessionManager.ts @@ -5,8 +5,7 @@ import { prefixMessage } from '@sasjs/utils/error' import { RequestClient } from './request/RequestClient' const MAX_SESSION_COUNT = 1 -const RETRY_LIMIT: number = 3 -let RETRY_COUNT: number = 0 +const loggedErrors: NoSessionStateError[] = [] export class SessionManager { constructor( @@ -161,7 +160,7 @@ export class SessionManager { const stateLink = session.links.find((l: any) => l.rel === 'state') - return new Promise(async (resolve, reject) => { + return new Promise(async (resolve) => { if ( sessionState === 'pending' || sessionState === 'running' || @@ -192,23 +191,25 @@ export class SessionManager { this.printedSessionState.printed = false } - // There is an internal error present in SAS Viya 3.5 - // Retry to wait for a session status in such case of SAS internal error if (!sessionState) { - if (RETRY_COUNT < RETRY_LIMIT) { - RETRY_COUNT++ + const stateError = new NoSessionStateError( + responseStatus, + this.serverUrl + stateLink.href, + session.links.find((l: any) => l.rel === 'log')?.href as string + ) - resolve(this.waitForSession(session, etag, accessToken)) - } else { - reject( - new NoSessionStateError( - responseStatus, - this.serverUrl + stateLink.href, - session.links.find((l: any) => l.rel === 'log') - ?.href as string - ) + if ( + !loggedErrors.find( + (err: NoSessionStateError) => + err.serverResponseStatus === stateError.serverResponseStatus ) + ) { + loggedErrors.push(stateError) + + logger.error(stateError.message) } + + resolve(this.waitForSession(session, etag, accessToken)) } resolve(sessionState) From 6c901f1c2193d4bbcdc27e406514669bbd682006 Mon Sep 17 00:00:00 2001 From: Yury Shkoda Date: Mon, 26 Jul 2021 10:40:15 +0300 Subject: [PATCH 2/7] chore(session): change log level from error to info --- src/SessionManager.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/SessionManager.ts b/src/SessionManager.ts index d4f002a..d016356 100644 --- a/src/SessionManager.ts +++ b/src/SessionManager.ts @@ -206,7 +206,7 @@ export class SessionManager { ) { loggedErrors.push(stateError) - logger.error(stateError.message) + logger.info(stateError.message) } resolve(this.waitForSession(session, etag, accessToken)) From fb7a0f43e1ffe15bbcfb7a8b5b5130c6a529ee01 Mon Sep 17 00:00:00 2001 From: Yury Shkoda Date: Mon, 26 Jul 2021 12:17:19 +0300 Subject: [PATCH 3/7] test(session): added test to cover 304 response --- src/test/SessionManager.spec.ts | 36 +++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/src/test/SessionManager.spec.ts b/src/test/SessionManager.spec.ts index c818d4f..d9b7cc0 100644 --- a/src/test/SessionManager.spec.ts +++ b/src/test/SessionManager.spec.ts @@ -3,6 +3,7 @@ import { RequestClient } from '../request/RequestClient' import { NoSessionStateError } from '../types/errors' import * as dotenv from 'dotenv' import axios from 'axios' +import { Logger, LogLevel } from '@sasjs/utils' jest.mock('axios') const mockedAxios = axios as jest.Mocked @@ -47,12 +48,26 @@ describe('SessionManager', () => { }) describe('waitForSession', () => { - it('should reject with NoSessionStateError if SAS server did not provide session state', async () => { - const responseStatus = 304 + beforeEach(() => { + ;(process as any).logger = new Logger(LogLevel.Off) + }) - mockedAxios.get.mockImplementation(() => - Promise.resolve({ data: '', status: responseStatus }) - ) + it('should reject with NoSessionStateError if SAS server did not provide session state', async () => { + let requestAttempt = 0 + + mockedAxios.get.mockImplementation(() => { + requestAttempt += 1 + + if (requestAttempt > 10) { + return Promise.resolve({ data: 'idle', status: 200 }) + } + + return Promise.resolve({ data: '', status: 304 }) + }) + + mockedAxios + + jest.spyOn((process as any).logger, 'info') await expect( sessionManager['waitForSession']( @@ -70,12 +85,11 @@ describe('SessionManager', () => { null, 'access_token' ) - ).rejects.toEqual( - new NoSessionStateError( - responseStatus, - process.env.SERVER_URL as string, - 'logUrl' - ) + ).resolves.toEqual('idle') + + expect((process as any).logger.info).toHaveBeenCalledTimes(1) + expect((process as any).logger.info).toHaveBeenLastCalledWith( + `Could not get session state. Server responded with 304 whilst checking state: ${process.env.SERVER_URL}` ) }) }) From 0b9284e4815c5cf7b98b0cedd5859b7e9ab24227 Mon Sep 17 00:00:00 2001 From: Yury Shkoda Date: Tue, 27 Jul 2021 16:03:41 +0300 Subject: [PATCH 4/7] refactor(session): improve waitForSession method --- src/SessionManager.ts | 110 +++++++++++++++++++++--------------------- 1 file changed, 55 insertions(+), 55 deletions(-) diff --git a/src/SessionManager.ts b/src/SessionManager.ts index d016356..400d317 100644 --- a/src/SessionManager.ts +++ b/src/SessionManager.ts @@ -153,71 +153,71 @@ export class SessionManager { session: Session, etag: string | null, accessToken?: string - ) { + ): Promise { const logger = process.logger || console let sessionState = session.state const stateLink = session.links.find((l: any) => l.rel === 'state') - return new Promise(async (resolve) => { - if ( - sessionState === 'pending' || - sessionState === 'running' || - sessionState === '' - ) { - if (stateLink) { - if (this.debug && !this.printedSessionState.printed) { - logger.info('Polling session status...') + if ( + sessionState === 'pending' || + sessionState === 'running' || + sessionState === '' + ) { + if (stateLink) { + if (this.debug && !this.printedSessionState.printed) { + logger.info('Polling session status...') - this.printedSessionState.printed = true - } - - const { result: state, responseStatus: responseStatus } = - await this.getSessionState( - `${this.serverUrl}${stateLink.href}?wait=30`, - etag!, - accessToken - ).catch((err) => { - throw prefixMessage(err, 'Error while getting session state.') - }) - - sessionState = state.trim() - - if (this.debug && this.printedSessionState.state !== sessionState) { - logger.info(`Current session state is '${sessionState}'`) - - this.printedSessionState.state = sessionState - this.printedSessionState.printed = false - } - - if (!sessionState) { - const stateError = new NoSessionStateError( - responseStatus, - this.serverUrl + stateLink.href, - session.links.find((l: any) => l.rel === 'log')?.href as string - ) - - if ( - !loggedErrors.find( - (err: NoSessionStateError) => - err.serverResponseStatus === stateError.serverResponseStatus - ) - ) { - loggedErrors.push(stateError) - - logger.info(stateError.message) - } - - resolve(this.waitForSession(session, etag, accessToken)) - } - - resolve(sessionState) + this.printedSessionState.printed = true } + + const { result: state, responseStatus: responseStatus } = + await this.getSessionState( + `${this.serverUrl}${stateLink.href}?wait=30`, + etag!, + accessToken + ).catch((err) => { + throw prefixMessage(err, 'Error while getting session state.') + }) + + sessionState = state.trim() + + if (this.debug && this.printedSessionState.state !== sessionState) { + logger.info(`Current session state is '${sessionState}'`) + + this.printedSessionState.state = sessionState + this.printedSessionState.printed = false + } + + if (!sessionState) { + const stateError = new NoSessionStateError( + responseStatus, + this.serverUrl + stateLink.href, + session.links.find((l: any) => l.rel === 'log')?.href as string + ) + + if ( + !loggedErrors.find( + (err: NoSessionStateError) => + err.serverResponseStatus === stateError.serverResponseStatus + ) + ) { + loggedErrors.push(stateError) + + logger.info(stateError.message) + } + + return await this.waitForSession(session, etag, accessToken) + } + + return sessionState } else { - resolve(sessionState) + throw 'Error while getting session state link.' } - }) + } else { + return sessionState + } } private async getSessionState( From ac8821baecad5c85812ba759fb7ca49c7ec60d55 Mon Sep 17 00:00:00 2001 From: Yury Shkoda Date: Tue, 27 Jul 2021 16:06:43 +0300 Subject: [PATCH 5/7] test(session): add assertion of get request quantity --- src/test/SessionManager.spec.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/test/SessionManager.spec.ts b/src/test/SessionManager.spec.ts index d9b7cc0..976f919 100644 --- a/src/test/SessionManager.spec.ts +++ b/src/test/SessionManager.spec.ts @@ -54,19 +54,18 @@ describe('SessionManager', () => { it('should reject with NoSessionStateError if SAS server did not provide session state', async () => { let requestAttempt = 0 + const requestAttemptLimit = 10 mockedAxios.get.mockImplementation(() => { requestAttempt += 1 - if (requestAttempt > 10) { + if (requestAttempt >= requestAttemptLimit) { return Promise.resolve({ data: 'idle', status: 200 }) } return Promise.resolve({ data: '', status: 304 }) }) - mockedAxios - jest.spyOn((process as any).logger, 'info') await expect( @@ -91,6 +90,7 @@ describe('SessionManager', () => { expect((process as any).logger.info).toHaveBeenLastCalledWith( `Could not get session state. Server responded with 304 whilst checking state: ${process.env.SERVER_URL}` ) + expect(mockedAxios.get).toHaveBeenCalledTimes(requestAttemptLimit) }) }) }) From 6f9196c6900cb15921c46d55f25379564849ff4c Mon Sep 17 00:00:00 2001 From: Yury Shkoda Date: Wed, 28 Jul 2021 09:39:52 +0300 Subject: [PATCH 6/7] refactor(session): make loggedErrors a private property --- src/SessionManager.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/SessionManager.ts b/src/SessionManager.ts index 400d317..a0c68d7 100644 --- a/src/SessionManager.ts +++ b/src/SessionManager.ts @@ -5,9 +5,10 @@ import { prefixMessage } from '@sasjs/utils/error' import { RequestClient } from './request/RequestClient' const MAX_SESSION_COUNT = 1 -const loggedErrors: NoSessionStateError[] = [] export class SessionManager { + private loggedErrors: NoSessionStateError[] = [] + constructor( private serverUrl: string, private contextName: string, @@ -198,12 +199,12 @@ export class SessionManager { ) if ( - !loggedErrors.find( + !this.loggedErrors.find( (err: NoSessionStateError) => err.serverResponseStatus === stateError.serverResponseStatus ) ) { - loggedErrors.push(stateError) + this.loggedErrors.push(stateError) logger.info(stateError.message) } @@ -211,11 +212,15 @@ export class SessionManager { return await this.waitForSession(session, etag, accessToken) } + this.loggedErrors = [] + return sessionState } else { throw 'Error while getting session state link.' } } else { + this.loggedErrors = [] + return sessionState } } From 5317c14d54a6333071cbf278d47e55ff074c46d2 Mon Sep 17 00:00:00 2001 From: Yury Shkoda Date: Thu, 29 Jul 2021 10:24:03 +0300 Subject: [PATCH 7/7] test(sessionManager): improve test coverage of 'waitForSession' --- src/test/SessionManager.spec.ts | 85 +++++++++++++++++++++++++-------- 1 file changed, 64 insertions(+), 21 deletions(-) diff --git a/src/test/SessionManager.spec.ts b/src/test/SessionManager.spec.ts index 976f919..ace8526 100644 --- a/src/test/SessionManager.spec.ts +++ b/src/test/SessionManager.spec.ts @@ -4,6 +4,7 @@ import { NoSessionStateError } from '../types/errors' import * as dotenv from 'dotenv' import axios from 'axios' import { Logger, LogLevel } from '@sasjs/utils' +import { Session } from '../types' jest.mock('axios') const mockedAxios = axios as jest.Mocked @@ -48,6 +49,16 @@ describe('SessionManager', () => { }) describe('waitForSession', () => { + const session: Session = { + id: 'id', + state: '', + links: [{ rel: 'state', href: '', uri: '', type: '', method: 'GET' }], + attributes: { + sessionInactiveTimeout: 0 + }, + creationTimeStamp: '' + } + beforeEach(() => { ;(process as any).logger = new Logger(LogLevel.Off) }) @@ -55,12 +66,13 @@ describe('SessionManager', () => { it('should reject with NoSessionStateError if SAS server did not provide session state', async () => { let requestAttempt = 0 const requestAttemptLimit = 10 + const sessionState = 'idle' mockedAxios.get.mockImplementation(() => { requestAttempt += 1 if (requestAttempt >= requestAttemptLimit) { - return Promise.resolve({ data: 'idle', status: 200 }) + return Promise.resolve({ data: sessionState, status: 200 }) } return Promise.resolve({ data: '', status: 304 }) @@ -68,29 +80,60 @@ describe('SessionManager', () => { jest.spyOn((process as any).logger, 'info') - await expect( - sessionManager['waitForSession']( - { - id: 'id', - state: '', - links: [ - { rel: 'state', href: '', uri: '', type: '', method: 'GET' } - ], - attributes: { - sessionInactiveTimeout: 0 - }, - creationTimeStamp: '' - }, - null, - 'access_token' - ) - ).resolves.toEqual('idle') + sessionManager.debug = true - expect((process as any).logger.info).toHaveBeenCalledTimes(1) - expect((process as any).logger.info).toHaveBeenLastCalledWith( + await expect( + sessionManager['waitForSession'](session, null, 'access_token') + ).resolves.toEqual(sessionState) + + expect(mockedAxios.get).toHaveBeenCalledTimes(requestAttemptLimit) + expect((process as any).logger.info).toHaveBeenCalledTimes(3) + expect((process as any).logger.info).toHaveBeenNthCalledWith( + 1, + 'Polling session status...' + ) + expect((process as any).logger.info).toHaveBeenNthCalledWith( + 2, `Could not get session state. Server responded with 304 whilst checking state: ${process.env.SERVER_URL}` ) - expect(mockedAxios.get).toHaveBeenCalledTimes(requestAttemptLimit) + expect((process as any).logger.info).toHaveBeenNthCalledWith( + 3, + `Current session state is '${sessionState}'` + ) + }) + + it('should throw an error if there is no session link', async () => { + const customSession = JSON.parse(JSON.stringify(session)) + customSession.links = [] + + mockedAxios.get.mockImplementation(() => + Promise.resolve({ data: customSession.state, status: 200 }) + ) + + await expect( + sessionManager['waitForSession'](customSession, null, 'access_token') + ).rejects.toContain('Error while getting session state link.') + }) + + it('should throw an error if could not get session state', async () => { + mockedAxios.get.mockImplementation(() => Promise.reject('Mocked error')) + + await expect( + sessionManager['waitForSession'](session, null, 'access_token') + ).rejects.toContain('Error while getting session state.') + }) + + it('should return session state', async () => { + const customSession = JSON.parse(JSON.stringify(session)) + customSession.state = 'completed' + + mockedAxios.get.mockImplementation(() => + Promise.resolve({ data: customSession.state, status: 200 }) + ) + + await expect( + sessionManager['waitForSession'](customSession, null, 'access_token') + ).resolves.toEqual(customSession.state) }) }) })