From 3d756aec59998015d4f0f331720dcb517d677b43 Mon Sep 17 00:00:00 2001 From: Julian Waller Date: Mon, 16 Oct 2023 09:50:23 +0100 Subject: [PATCH] fix: Setting 2 full path properties fails SOFIE-2628 (#34) Co-authored-by: Mint de Wit --- src/Ember/Client/__tests__/index.spec.ts | 249 +++++++++++++++++++++++ src/Ember/Client/index.ts | 66 +++--- src/Ember/Server/index.ts | 2 +- src/Ember/Socket/S101Client.ts | 4 +- src/Ember/Socket/S101Socket.ts | 41 ++-- src/__mocks__/S101Client.ts | 109 ++++++++++ 6 files changed, 418 insertions(+), 53 deletions(-) create mode 100644 src/Ember/Client/__tests__/index.spec.ts create mode 100644 src/__mocks__/S101Client.ts diff --git a/src/Ember/Client/__tests__/index.spec.ts b/src/Ember/Client/__tests__/index.spec.ts new file mode 100644 index 0000000..02ad8b2 --- /dev/null +++ b/src/Ember/Client/__tests__/index.spec.ts @@ -0,0 +1,249 @@ +import { + NumberedTreeNode, + EmberElement, + NumberedTreeNodeImpl, + EmberNodeImpl, + ParameterImpl, + ParameterType, + QualifiedElementImpl, +} from '../../../model' +import { Collection, Root, RootElement } from '../../../types/types' +import { EmberClient } from '../' +import S101ClientMock from '../../../__mocks__/S101Client' +import { DecodeResult } from '../../../encodings/ber/decoder/DecodeResult' +// import { EmberTreeNode, RootElement } from '../../../types/types' +// import { ElementType, EmberElement } from '../../../model/EmberElement' +// import { Parameter, ParameterType } from '../../../model/Parameter' + +jest.mock('../../Socket/S101Client', () => require('../../../__mocks__/S101Client')) + +describe('client', () => { + const onSocketCreate = jest.fn() + const onConnection = jest.fn() + const onSocketClose = jest.fn() + const onSocketWrite = jest.fn() + const onConnectionChanged = jest.fn() + + function setupSocketMock() { + S101ClientMock.mockOnNextSocket((socket: any) => { + onSocketCreate() + + socket.onConnect = onConnection + socket.onWrite = onSocketWrite + socket.onClose = onSocketClose + }) + } + + beforeEach(() => { + setupSocketMock() + }) + afterEach(() => { + const sockets = S101ClientMock.openSockets() + // Destroy any lingering sockets, to prevent a failing test from affecting other tests: + sockets.forEach((s) => s.destroy()) + + S101ClientMock.clearMockOnNextSocket() + onSocketCreate.mockClear() + onConnection.mockClear() + onSocketClose.mockClear() + onSocketWrite.mockClear() + onConnectionChanged.mockClear() + + // Just a check to ensure that the unit tests cleaned up the socket after themselves: + // eslint-disable-next-line jest/no-standalone-expect + expect(sockets).toHaveLength(0) + }) + + async function runWithConnection(fn: (connection: EmberClient, socket: S101ClientMock) => Promise) { + const client = new EmberClient('test') + try { + expect(client).toBeTruthy() + + await client.connect() + + // Wait for connection + await new Promise(setImmediate) + + // Should be connected + expect(client.connected).toBeTruthy() + + const sockets = S101ClientMock.openSockets() + expect(sockets).toHaveLength(1) + expect(onSocketWrite).toHaveBeenCalledTimes(0) + + await fn(client, sockets[0]) + } finally { + // Ensure cleaned up + await client.disconnect() + client.discard() + + await new Promise(setImmediate) + } + } + + function createQualifiedNodeResponse( + path: string, + content: EmberElement, + children: Collection> + ): DecodeResult { + const parent = new QualifiedElementImpl(path, content, children) + + const fixLevel = (node: NumberedTreeNode, parent: NumberedTreeNode) => { + node.parent = parent + + for (const child of Object.values>(node.children ?? {})) { + fixLevel(child, node) + } + } + for (const child of Object.values>(children)) { + fixLevel(child, parent as any as NumberedTreeNode) + } + return { + value: { + 0: parent as Exclude>, + }, + } + } + + it('getDirectory resolves', async () => { + await runWithConnection(async (client, socket) => { + // Do initial load + const getRootDirReq = await client.getDirectory(client.tree) + getRootDirReq.response?.catch(() => null) // Ensure uncaught response is ok + expect(onSocketWrite).toHaveBeenCalledTimes(1) + // TODO: should the value of the call be checked? + + // Mock a valid response + socket.mockData({ + value: { + 1: new NumberedTreeNodeImpl(1, new EmberNodeImpl('Ruby', undefined, undefined, true)), + }, + }) + + // Should have a response + const res = (await getRootDirReq.response) as NumberedTreeNodeImpl + expect(res).toMatchObject(new NumberedTreeNodeImpl(1, new EmberNodeImpl('Ruby', undefined, undefined, true))) + }) + }) + + it('getElementByPath', async () => { + await runWithConnection(async (client, socket) => { + // Do initial load + const getRootDirReq = await client.getDirectory(client.tree) + getRootDirReq.response?.catch(() => null) // Ensure uncaught response is ok + expect(onSocketWrite).toHaveBeenCalledTimes(1) + onSocketWrite.mockClear() + + // Mock a valid response + socket.mockData({ + value: { + 1: new NumberedTreeNodeImpl(1, new EmberNodeImpl('Ruby', undefined, undefined, true)), + }, + }) + await getRootDirReq.response + + // Run the tree + const getByPathPromise = client.getElementByPath('Ruby.Sums.On') + + // First lookup + expect(onSocketWrite).toHaveBeenCalledTimes(1) + socket.mockData({ + value: { + 1: new NumberedTreeNodeImpl(1, new EmberNodeImpl('Ruby', undefined, undefined, true), { + 1: new NumberedTreeNodeImpl(1, new EmberNodeImpl('Sums', undefined, undefined, true)), + }), + }, + }) + + await new Promise(setImmediate) + + // Second lookup + expect(onSocketWrite).toHaveBeenCalledTimes(2) + socket.mockData({ + value: { + 1: new QualifiedElementImpl('1.1', new EmberNodeImpl('Sums', undefined, undefined, false), { + 1: new NumberedTreeNodeImpl(1, new ParameterImpl(ParameterType.Boolean, 'On', undefined, false)), + }) as Exclude>, + }, + }) + + await new Promise(setImmediate) + + const res = await getByPathPromise + expect(res).toBeTruthy() + expect(res).toMatchObject( + new NumberedTreeNodeImpl(1, new ParameterImpl(ParameterType.Boolean, 'On', undefined, false)) + ) + }) + }) + + it('getElementByPath concurrent', async () => { + await runWithConnection(async (client, socket) => { + // Do initial load + const getRootDirReq = await client.getDirectory(client.tree) + getRootDirReq.response?.catch(() => null) // Ensure uncaught response is ok + expect(onSocketWrite).toHaveBeenCalledTimes(1) + onSocketWrite.mockClear() + + // Mock a valid response + socket.mockData({ + value: { + 1: new NumberedTreeNodeImpl(1, new EmberNodeImpl('Ruby', undefined, undefined, true)), + }, + }) + await getRootDirReq.response + + // Run the tree + const getByPathPromise = client.getElementByPath('Ruby.Sums.MAIN.On') + const getByPathPromise2 = client.getElementByPath('Ruby.Sums.MAIN.Second') + + // First lookup from both + expect(onSocketWrite).toHaveBeenCalledTimes(2) + socket.mockData( + createQualifiedNodeResponse('1', new EmberNodeImpl('Ruby', undefined, undefined, true), { + 1: new NumberedTreeNodeImpl(1, new EmberNodeImpl('Sums', undefined, undefined, false)), + }) + ) + + socket.mockData( + createQualifiedNodeResponse('1', new EmberNodeImpl('Ruby', undefined, undefined, true), { + 1: new NumberedTreeNodeImpl(1, new EmberNodeImpl('Sums', undefined, undefined, false)), + }) + ) + + await new Promise(setImmediate) + + // Second lookup + expect(onSocketWrite).toHaveBeenCalledTimes(4) + socket.mockData( + createQualifiedNodeResponse('1.1', new EmberNodeImpl('Sums', undefined, undefined, false), { + 1: new NumberedTreeNodeImpl(1, new EmberNodeImpl('MAIN', undefined, undefined, false)), + }) + ) + await new Promise(setImmediate) + socket.mockData( + createQualifiedNodeResponse('1.1', new EmberNodeImpl('Sums', undefined, undefined, false), { + 1: new NumberedTreeNodeImpl(1, new EmberNodeImpl('MAIN', undefined, undefined, false)), + }) + ) + + await new Promise(setImmediate) + + // Final lookup + expect(onSocketWrite).toHaveBeenCalledTimes(6) + socket.mockData( + createQualifiedNodeResponse('1.1.1', new EmberNodeImpl('MAIN', undefined, undefined, false), { + 1: new NumberedTreeNodeImpl(1, new ParameterImpl(ParameterType.Boolean, 'On', undefined, false)), + 2: new NumberedTreeNodeImpl(1, new ParameterImpl(ParameterType.Boolean, 'Second', undefined, false)), + }) + ) + + // Both completed successfully + const res = await getByPathPromise + expect(res).toBeTruthy() + + const res2 = await getByPathPromise2 + expect(res2).toBeTruthy() + }) + }) +}) diff --git a/src/Ember/Client/index.ts b/src/Ember/Client/index.ts index c843925..67f461b 100644 --- a/src/Ember/Client/index.ts +++ b/src/Ember/Client/index.ts @@ -43,9 +43,17 @@ export interface RequestPromiseArguments { response?: Promise } +export enum ExpectResponse { + None = 'none', + Any = 'any', + HasChildren = 'has-children', +} + export interface Request { reqId: string node: RootElement + // Basic validation of the response change + nodeResponse: ExpectResponse resolve: (res: any) => void reject: (err: Error) => void cb?: (EmberNode: TreeElement) => void @@ -192,7 +200,7 @@ export class EmberClient extends EventEmitter { cb, }) - return this._sendRequest(new NumberedTreeNodeImpl(0, command)) + return this._sendRequest(new NumberedTreeNodeImpl(0, command), ExpectResponse.Any) } if (cb) @@ -201,7 +209,7 @@ export class EmberClient extends EventEmitter { cb, }) - return this._sendCommand(node, command) + return this._sendCommand(node, command, ExpectResponse.HasChildren) } async subscribe( node: RootElement | Array, @@ -220,7 +228,7 @@ export class EmberClient extends EventEmitter { cb, }) - return this._sendRequest(new NumberedTreeNodeImpl(0, command)) + return this._sendRequest(new NumberedTreeNodeImpl(0, command), ExpectResponse.Any) } if (cb) @@ -229,7 +237,7 @@ export class EmberClient extends EventEmitter { cb, }) - return this._sendCommand(node, command, false) + return this._sendCommand(node, command, ExpectResponse.None) } async unsubscribe(node: NumberedTreeNode | Array): RequestPromise { if (!node) { @@ -246,10 +254,10 @@ export class EmberClient extends EventEmitter { } if (Array.isArray(node)) { - return this._sendRequest(new NumberedTreeNodeImpl(0, command)) + return this._sendRequest(new NumberedTreeNodeImpl(0, command), ExpectResponse.Any) } - return this._sendCommand(node, command, false) + return this._sendCommand(node, command, ExpectResponse.None) } async invoke( node: NumberedTreeNode | QualifiedElement, @@ -268,7 +276,7 @@ export class EmberClient extends EventEmitter { args, }, } - return this._sendCommand(node, command) + return this._sendCommand(node, command, ExpectResponse.Any) } /** Sending ember+ values */ @@ -287,7 +295,10 @@ export class EmberClient extends EventEmitter { // TODO - should other properties be scrapped? qualifiedParam.contents.value = value - return this._sendRequest>(qualifiedParam, awaitResponse) + return this._sendRequest>( + qualifiedParam, + awaitResponse ? ExpectResponse.Any : ExpectResponse.None + ) } async matrixConnect( matrix: QualifiedElement | NumberedTreeNode, @@ -355,24 +366,27 @@ export class EmberClient extends EventEmitter { cb?: (EmberNode: TreeElement) => void, delimiter = '.' ): Promise | undefined> { - const getNext = (elements: Collection>, i?: string) => + const getNodeInCollection = (elements: Collection>, identifier: string) => Object.values>(elements || {}).find( (r) => - r.number === Number(i) || - (r.contents as EmberNode).identifier === i || - (r.contents as EmberNode).description === i + r.number === Number(identifier) || + (r.contents as EmberNode).identifier === identifier || + (r.contents as EmberNode).description === identifier ) - const getNextChild = (node: TreeElement, i: string) => node.children && getNext(node.children, i) + const getNextChild = (node: TreeElement, identifier: string) => + node.children && getNodeInCollection(node.children, identifier) const numberedPath: Array = [] const pathArr = path.split(delimiter) - const i = pathArr.shift() - let tree: NumberedTreeNode | undefined = getNext(this.tree, i) - if (tree?.number) numberedPath.push(tree?.number) + const firstIdentifier = pathArr.shift() + if (!firstIdentifier) throw new Error('Expected at least one segment in the path') + + let tree: NumberedTreeNode | undefined = getNodeInCollection(this.tree, firstIdentifier) + if (tree?.number !== undefined) numberedPath.push(tree.number) while (pathArr.length) { const i = pathArr.shift() - if (!i) break + if (!i) break // TODO - this will break the loop if the path was `1..0` if (!tree) break let next = getNextChild(tree, i) if (!next) { @@ -382,7 +396,7 @@ export class EmberClient extends EventEmitter { } tree = next if (!tree) throw new Error(`Could not find node ${i} on given path ${numberedPath.join()}`) - if (tree?.number) numberedPath.push(tree?.number) + if (tree?.number !== undefined) numberedPath.push(tree.number) } if (cb && numberedPath) { @@ -415,19 +429,19 @@ export class EmberClient extends EventEmitter { qualifiedMatrix.contents.connections = [connection] - return this._sendRequest>(qualifiedMatrix) + return this._sendRequest>(qualifiedMatrix, ExpectResponse.Any) } - private async _sendCommand(EmberNode: RootElement, command: Command, hasResponse?: boolean) { + private async _sendCommand(node: RootElement, command: Command, expectResponse: ExpectResponse) { // assert a qualified EmberNode - const qualifiedEmberNode = assertQualifiedEmberNode(EmberNode) + const qualifiedEmberNode = assertQualifiedEmberNode(node) // insert command const commandEmberNode = insertCommand(qualifiedEmberNode, command) // send request - return this._sendRequest(commandEmberNode, hasResponse) + return this._sendRequest(commandEmberNode, expectResponse) } - private async _sendRequest(node: RootElement, hasResponse = true): RequestPromise { + private async _sendRequest(node: RootElement, expectResponse: ExpectResponse): RequestPromise { const reqId = Math.random().toString(24).substr(-4) const requestPromise: RequestPromiseArguments = { reqId, @@ -436,11 +450,12 @@ export class EmberClient extends EventEmitter { const message = berEncode([node], RootType.Elements) - if (hasResponse) { + if (expectResponse !== ExpectResponse.None) { const p = new Promise((resolve, reject) => { const request: Request = { reqId, node, + nodeResponse: expectResponse, resolve, reject, message, @@ -489,6 +504,9 @@ export class EmberClient extends EventEmitter { (s) => (!('path' in s.node) && !change.path) || ('path' in s.node && s.node.path === change.path) ) for (const req of reqs) { + // Don't complete the response, if the call was expecting the children to be loaded + if (req.nodeResponse === ExpectResponse.HasChildren && !change.node.children) continue + if (req.cb) req.cb(change.node) if (req.resolve) { req.resolve(change.node) diff --git a/src/Ember/Server/index.ts b/src/Ember/Server/index.ts index 2d2e361..9bc8109 100644 --- a/src/Ember/Server/index.ts +++ b/src/Ember/Server/index.ts @@ -60,7 +60,7 @@ export class EmberServer extends EventEmitter { this._server.on('connection', (client: S101Socket) => { this._clients.add(client) - client.on('emberTree', (tree: DecodeResult>) => this._handleIncoming(tree, client)) + client.on('emberTree', (tree) => this._handleIncoming(tree as DecodeResult>, client)) client.on('error', (e) => { this.emit('clientError', client, e) diff --git a/src/Ember/Socket/S101Client.ts b/src/Ember/Socket/S101Client.ts index 8986860..b5247bb 100644 --- a/src/Ember/Socket/S101Client.ts +++ b/src/Ember/Socket/S101Client.ts @@ -105,12 +105,12 @@ export default class S101Client extends S101Socket { return super.disconnect(timeout) } - handleClose(): void { + protected handleClose(): void { if (this.keepaliveIntervalTimer) clearInterval(this.keepaliveIntervalTimer) this.socket?.destroy() } - _autoReconnectionAttempt(): void { + private _autoReconnectionAttempt(): void { if (this._autoReconnect) { if (this._reconnectAttempts > 0) { // no reconnection if no valid reconnectionAttemps is set diff --git a/src/Ember/Socket/S101Socket.ts b/src/Ember/Socket/S101Socket.ts index cbd446d..f86efa2 100644 --- a/src/Ember/Socket/S101Socket.ts +++ b/src/Ember/Socket/S101Socket.ts @@ -5,35 +5,34 @@ import { S101Codec } from '../../S101' import { berDecode } from '../..' import { ConnectionStatus } from '../Client' import { normalizeError } from '../Lib/util' +import { Root } from '../../types' +import { DecodeResult } from '../../encodings/ber/decoder/DecodeResult' export type Request = any export type S101SocketEvents = { error: [Error] emberPacket: [packet: Buffer] - emberTree: [root: any] + emberTree: [root: DecodeResult] connecting: [] connected: [] disconnected: [] } export default class S101Socket extends EventEmitter { - socket: Socket | undefined - keepaliveInterval = 10 - keepaliveMaxResponseTime = 500 - keepaliveIntervalTimer: NodeJS.Timeout | undefined - keepaliveResponseWindowTimer: NodeJS.Timer | null - pendingRequests: Array = [] - activeRequest: Request | undefined + protected socket: Socket | undefined + private readonly keepaliveInterval = 10 + private readonly keepaliveMaxResponseTime = 500 + protected keepaliveIntervalTimer: NodeJS.Timeout | undefined + private keepaliveResponseWindowTimer: NodeJS.Timer | null status: ConnectionStatus - codec = new S101Codec() + protected readonly codec = new S101Codec() constructor(socket?: Socket) { super() this.socket = socket this.keepaliveIntervalTimer = undefined this.keepaliveResponseWindowTimer = null - this.activeRequest = undefined this.status = this.isConnected() ? ConnectionStatus.Connected : ConnectionStatus.Disconnected this.codec.on('keepaliveReq', () => { @@ -59,7 +58,7 @@ export default class S101Socket extends EventEmitter { this._initSocket() } - _initSocket(): void { + private _initSocket(): void { if (this.socket != null) { this.socket.on('data', (data) => { try { @@ -82,16 +81,6 @@ export default class S101Socket extends EventEmitter { } } - /** - * @returns {string} - ie: "10.1.1.1:9000" - */ - remoteAddress(): string { - if (this.socket === undefined) { - return 'not connected' - } - return `${this.socket.remoteAddress}:${this.socket.remotePort}` - } - /** * @param {number} timeout=2 */ @@ -130,14 +119,14 @@ export default class S101Socket extends EventEmitter { /** * */ - handleClose(): void { + protected handleClose(): void { this.socket = undefined if (this.keepaliveIntervalTimer) clearInterval(this.keepaliveIntervalTimer) this.status = ConnectionStatus.Disconnected this.emit('disconnected') } - isConnected(): boolean { + private isConnected(): boolean { return this.socket !== undefined && !!this.socket } @@ -161,7 +150,7 @@ export default class S101Socket extends EventEmitter { /** * */ - sendKeepaliveRequest(): void { + private sendKeepaliveRequest(): void { if (this.isConnected() && this.socket) { try { this.socket.write(this.codec.keepAliveRequest()) @@ -177,7 +166,7 @@ export default class S101Socket extends EventEmitter { /** * */ - sendKeepaliveResponse(): void { + private sendKeepaliveResponse(): void { if (this.isConnected() && this.socket) { try { this.socket.write(this.codec.keepAliveResponse()) @@ -193,7 +182,7 @@ export default class S101Socket extends EventEmitter { // this.sendBER(ber) // } - startKeepAlive(): void { + protected startKeepAlive(): void { this.keepaliveIntervalTimer = setInterval(() => { try { this.sendKeepaliveRequest() diff --git a/src/__mocks__/S101Client.ts b/src/__mocks__/S101Client.ts new file mode 100644 index 0000000..6109082 --- /dev/null +++ b/src/__mocks__/S101Client.ts @@ -0,0 +1,109 @@ +import { ConnectionStatus } from '../Ember/Client' +import type OrigS101Client from '../Ember/Socket/S101Client' +import { EventEmitter } from 'eventemitter3' +import { S101SocketEvents } from '../Ember/Socket/S101Socket' +import { DecodeResult } from '../encodings/ber/decoder/DecodeResult' +import { Root } from '../types' +const sockets: Array = [] +const onNextSocket: Array<(socket: S101Client) => void> = [] + +const orgSetImmediate = setImmediate + +export default class S101Client + extends EventEmitter + implements + Omit< + OrigS101Client, + // EventEmitter: + 'on' | 'off' | 'once' | 'addListener' | 'removeListener' | 'removeAllListeners' + > +{ + public onWrite?: (data: Buffer) => boolean + public onConnect?: () => void + public onDisconnect?: () => void + public onClose?: () => void + + address: string + port: number + autoConnect = false + + public destroyed = false + + status: ConnectionStatus = ConnectionStatus.Disconnected + + constructor(address: string, port = 9000, autoConnect?: boolean) { + super() + + this.address = address + this.port = port + + this.autoConnect = !!autoConnect + + const cb = onNextSocket.shift() + if (cb) { + cb(this) + } + + sockets.push(this) + + if (this.autoConnect) this.connect().catch(() => null) // errors are already emitted + } + async connect(_timeout?: number): Promise { + this.status = ConnectionStatus.Connecting + + if (this.onConnect) this.onConnect() + orgSetImmediate(() => { + this.setConnected() + }) + } + async disconnect(_timeout?: number | undefined): Promise { + if (this.onConnect) this.onConnect() + orgSetImmediate(() => { + this.setDisconnected() + }) + } + + sendBER(data: Buffer): boolean { + if (this.onWrite) { + return this.onWrite(data) ?? true + } else { + return true + } + } + + public static mockSockets(): S101Client[] { + return sockets + } + public static openSockets(): S101Client[] { + return sockets.filter((s) => !s.destroyed) + } + public static mockOnNextSocket(cb: (s: S101Client) => void): void { + onNextSocket.push(cb) + } + public static clearMockOnNextSocket(): void { + onNextSocket.splice(0, 99999) + } + + public mockClose(): void { + this.setDisconnected() + } + public mockData(tree: DecodeResult): void { + this.emit('emberTree', tree) + } + + public destroy(): void { + this.destroyed = true + } + + private setConnected() { + this.destroyed = false + this.status = ConnectionStatus.Connected + this.emit('connected') + } + private setDisconnected() { + this.destroyed = true + this.status = ConnectionStatus.Disconnected + this.emit('disconnected') + if (this.onClose) this.onClose() + } +}