Merge pull request #40 from nrkno/fix/empty-responses

Various fixes around responses with no children

src/Ember/Client/__tests__/index.spec.ts

@@ -84,7 +84,7 @@ describe('client', () => {
 	function createQualifiedNodeResponse(
 		path: string,
 		content: EmberElement,
-		children: Collection<NumberedTreeNode<EmberElement>>
+		children: Collection<NumberedTreeNode<EmberElement>> | undefined
 	): DecodeResult<Root> {
 		const parent = new QualifiedElementImpl<EmberElement>(path, content, children)
 
@@ -95,8 +95,10 @@ describe('client', () => {
 				fixLevel(child, node)
 			}
 		}
-		for (const child of Object.values<NumberedTreeNode<EmberElement>>(children)) {
-			fixLevel(child, parent as any as NumberedTreeNode<EmberElement>)
+		if (children) {
+			for (const child of Object.values<NumberedTreeNode<EmberElement>>(children)) {
+				fixLevel(child, parent as any as NumberedTreeNode<EmberElement>)
+			}
 		}
 		return {
 			value: {
@@ -169,11 +171,22 @@ describe('client', () => {
 
 			await new Promise(setImmediate)
 
+			// lookup on the parameter
+			expect(onSocketWrite).toHaveBeenCalledTimes(3)
+			socket.mockData({
+				value: {
+					1: new QualifiedElementImpl<EmberElement>(
+						'1.1.1',
+						new ParameterImpl(ParameterType.Boolean, 'On', undefined, false)
+					) as Exclude<RootElement, NumberedTreeNode<EmberElement>>,
+				},
+			})
+
+			await new Promise(setImmediate)
+
 			const res = await getByPathPromise
 			expect(res).toBeTruthy()
-			expect(res).toMatchObject(
-				new NumberedTreeNodeImpl(1, new ParameterImpl(ParameterType.Boolean, 'On', undefined, false))
-			)
+			expect(res?.contents).toMatchObject(new ParameterImpl(ParameterType.Boolean, 'On', undefined, false))
 		})
 	})
 
@@ -229,15 +242,34 @@ describe('client', () => {
 
 			await new Promise(setImmediate)
 
-			// Final lookup
+			// Final tree node
 			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)),
+					2: new NumberedTreeNodeImpl(2, new ParameterImpl(ParameterType.Boolean, 'Second', undefined, false)),
 				})
 			)
 
+			await new Promise(setImmediate)
+
+			// last call to the parameters just in case
+			expect(onSocketWrite).toHaveBeenCalledTimes(8)
+			socket.mockData(
+				createQualifiedNodeResponse(
+					'1.1.1.1',
+					new ParameterImpl(ParameterType.Boolean, 'On', undefined, false),
+					undefined
+				)
+			)
+			socket.mockData(
+				createQualifiedNodeResponse(
+					'1.1.1.2',
+					new ParameterImpl(ParameterType.Boolean, 'Second', undefined, false),
+					undefined
+				)
+			)
+
 			// Both completed successfully
 			const res = await getByPathPromise
 			expect(res).toBeTruthy()
@@ -246,4 +278,108 @@ describe('client', () => {
 			expect(res2).toBeTruthy()
 		})
 	})
+
+	it('getElementByPath empty node in the root', 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
+
+			// Request the empty node
+			const req = await client.getDirectory(client.tree[1])
+
+			// Returns empty node
+			expect(onSocketWrite).toHaveBeenCalledTimes(1)
+			socket.mockData({
+				value: {
+					1: new NumberedTreeNodeImpl(1, new EmberNodeImpl()),
+				},
+			})
+
+			await new Promise(setImmediate)
+
+			const res = await req.response
+			expect(res).toBeTruthy()
+		})
+	})
+
+	it('getElementByPath empty node in the tree', 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.Empty')
+
+			// 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<EmberElement>('1.1', new EmberNodeImpl('Sums', undefined, undefined, false), {
+						1: new NumberedTreeNodeImpl(1, new EmberNodeImpl('Empty', undefined, undefined, true)),
+					}) as Exclude<RootElement, NumberedTreeNode<EmberElement>>,
+				},
+			})
+
+			await new Promise(setImmediate)
+
+			const getByPathRes = await getByPathPromise
+			expect(getByPathRes).toBeTruthy()
+
+			const node = client.tree[1].children?.[1].children?.[1]
+			if (!node) throw new Error('Empty res') // really just a typeguard
+
+			// Request the empty node
+			const req = await client.getDirectory(node)
+
+			// lookup on the empty node
+			expect(onSocketWrite).toHaveBeenCalledTimes(3)
+			socket.mockData({
+				value: {
+					1: new QualifiedElementImpl<EmberElement>('1.1.1', new EmberNodeImpl()) as Exclude<
+						RootElement,
+						NumberedTreeNode<EmberElement>
+					>,
+				},
+			})
+
+			await new Promise(setImmediate)
+
+			const res = await req.response
+			expect(res).toBeTruthy()
+		})
+	})
 })

src/Ember/Client/index.ts

@@ -29,7 +29,7 @@ import { Connection, ConnectionDisposition, ConnectionOperation } from '../../mo
 import { EmberNode } from '../../model/EmberNode'
 import { EventEmitter } from 'eventemitter3'
 import { S101Client } from '../Socket'
-import { getPath, assertQualifiedEmberNode, insertCommand, updateProps } from '../Lib/util'
+import { getPath, assertQualifiedEmberNode, insertCommand, updateProps, isEmptyNode } from '../Lib/util'
 import { berEncode } from '../..'
 import { NumberedTreeNodeImpl } from '../../model/Tree'
 import { EmberFunction } from '../../model/EmberFunction'
@@ -70,6 +70,7 @@ export interface Subscription {
 export interface Change {
 	path: string | undefined
 	node: RootElement
+	emptyNode?: boolean
 }
 
 export enum ConnectionStatus {
@@ -386,19 +387,27 @@ export class EmberClient extends EventEmitter<EmberClientEvents> {
 
 		while (pathArr.length) {
 			const i = pathArr.shift()
-			if (!i) break // TODO - this will break the loop if the path was `1..0`
+			if (i === undefined) break // TODO - this will break the loop if the path was `1..0`
 			if (!tree) break
+
 			let next = getNextChild(tree, i)
 			if (!next) {
 				const req = await this.getDirectory(tree)
 				tree = (await req.response) as NumberedTreeNode<EmberElement>
 				next = getNextChild(tree, i)
 			}
 			tree = next
+
 			if (!tree) throw new Error(`Could not find node ${i} on given path ${numberedPath.join()}`)
 			if (tree?.number !== undefined) numberedPath.push(tree.number)
 		}
 
+		if (tree?.contents.type === ElementType.Parameter) {
+			// do an additional getDirectory because Providers do not _have_ to send updates without that (should vs shall)
+			const req = await this.getDirectory(tree)
+			await req.response
+		}
+
 		if (cb && numberedPath) {
 			this._subscriptions.push({
 				path: numberedPath.join('.'),
@@ -505,7 +514,15 @@ export class EmberClient extends EventEmitter<EmberClientEvents> {
 			)
 			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.nodeResponse === ExpectResponse.HasChildren && !change.node.children) {
+					if (change.node.contents.type === ElementType.Parameter) {
+						// can't have children, therefore don't continue
+					} else if (change.emptyNode) {
+						// update comes from an empty node, so we can't continue anyway
+					} else {
+						continue
+					}
+				}
 
 				if (req.cb) req.cb(change.node)
 				if (req.resolve) {
@@ -588,9 +605,18 @@ export class EmberClient extends EventEmitter<EmberClientEvents> {
 					if (inserted) continue
 					changes.push(...this._updateTree(rootElement, tree))
 				} else {
-					if (this.tree[rootElement.number]) {
-						changes.push(...this._updateTree(rootElement, this.tree[rootElement.number]))
+					if (rootElement.children) {
+						if (this.tree[rootElement.number]) {
+							changes.push(...this._updateTree(rootElement, this.tree[rootElement.number]))
+						} else {
+							this.tree[rootElement.number] = rootElement
+							changes.push({ path: undefined, node: rootElement })
+						}
+					} else if (isEmptyNode(rootElement)) {
+						// empty node on the root of the tree must mean we have done a getDir on that specific node
+						changes.push({ path: rootElement.number + '', node: rootElement, emptyNode: true })
 					} else {
+						// this must have been something on the root of the tree (like GetDirectory)
 						this.tree[rootElement.number] = rootElement
 						changes.push({ path: undefined, node: rootElement })
 					}
@@ -605,7 +631,8 @@ export class EmberClient extends EventEmitter<EmberClientEvents> {
 		const changes: Array<Change> = []
 
 		if (update.contents.type === tree.contents.type) {
-			changes.push({ path: getPath(tree), node: tree })
+			changes.push({ path: getPath(tree), node: tree, emptyNode: isEmptyNode(update) })
+			// changes.push({ path: getPath(tree), node: tree })
 			switch (tree.contents.type) {
 				case ElementType.Node:
 					this._updateEmberNode(update.contents as EmberNode, tree.contents)
@@ -628,6 +655,9 @@ export class EmberClient extends EventEmitter<EmberClientEvents> {
 		} else if (update.children) {
 			changes.push({ path: getPath(tree), node: tree })
 			tree.children = update.children
+			for (const c of Object.values<NumberedTreeNode<EmberElement>>(update.children)) {
+				c.parent = tree
+			}
 		}
 
 		return changes

src/Ember/Lib/util.ts

@@ -2,6 +2,7 @@ import { QualifiedElement, NumberedTreeNode, RootElement } from '../../types/typ
 import { EmberElement, ElementType } from '../../model/EmberElement'
 import { Command } from '../../model/Command'
 import { QualifiedElementImpl, NumberedTreeNodeImpl, TreeElement } from '../../model/Tree'
+import { EmberNode } from '../../model'
 
 export function assertQualifiedEmberNode(node: RootElement): Exclude<RootElement, NumberedTreeNode<EmberElement>> {
 	if ('path' in node) {
@@ -76,3 +77,31 @@ export function normalizeError(e: unknown): Error {
 
 	return new Error(typeof e === 'string' ? e : (e as any)?.toString())
 }
+
+export function isEmptyNode(node: TreeElement<EmberElement>): boolean {
+	const isNode = (node: TreeElement<EmberElement>): node is TreeElement<EmberNode> => {
+		return node.contents.type === ElementType.Node
+	}
+
+	if (!isNode(node)) {
+		return false
+	}
+
+	if (node.children) {
+		return false
+	}
+
+	if (
+		node.contents.description ??
+		node.contents.identifier ??
+		node.contents.isOnline ??
+		node.contents.isRoot ??
+		node.contents.schemaIdentifiers ??
+		node.contents.templateReference
+	) {
+		return false
+	}
+
+	// node is a node, node has no children, node has no properties set => node must be empty
+	return true
+}

src/Ember/Server/index.ts

@@ -11,6 +11,7 @@ import {
 	MatrixImpl,
 	Matrix,
 	Connections,
+	EmberNodeImpl,
 } from '../../model'
 import {
 	Collection,
@@ -367,6 +368,10 @@ export class EmberServer extends EventEmitter<EmberServerEvents> {
 						qualified.children[i as unknown as number] = new NumberedTreeNodeImpl(child.number, child.contents)
 					}
 				}
+			} else if (qualified.contents.type === ElementType.Node && !('children' in tree && tree.children)) {
+				// node without children -> none of the properties should be set
+				qualified.contents = new EmberNodeImpl()
+				qualified.children = undefined
 			}
 			const data = berEncode([qualified as RootElement], RootType.Elements)
 			client.sendBER(data)

src/encodings/ber/encoder/Tree.ts

@@ -128,11 +128,7 @@ function hasChildren(el: TreeElement<EmberElement>): boolean {
 	return (
 		'children' in el &&
 		el.children !== undefined &&
-		!(
-			el.contents.type === ElementType.Command ||
-			el.contents.type === ElementType.Parameter ||
-			el.contents.type === ElementType.Template
-		)
+		!(el.contents.type === ElementType.Command || el.contents.type === ElementType.Template)
 	)
 }