Make connectionProfileGroup support undefined values (#9102)

* Make connectionProfileGroup able to support undefined values being passed

* Fix strict null checks

* More strict null check fixes

src/sql/platform/connection/common/connectionProfileGroup.ts

@@ -19,8 +19,8 @@ export interface IConnectionProfileGroup {
 
 export class ConnectionProfileGroup extends Disposable implements IConnectionProfileGroup {
 
-	public children: ConnectionProfileGroup[] = [];
+	private _childGroups: ConnectionProfileGroup[] = [];
-	public connections: ConnectionProfile[] = [];
+	private _childConnections: ConnectionProfile[] = [];
 	public parentId?: string;
 	private _isRenamed = false;
 	public constructor(
@@ -42,9 +42,9 @@ export class ConnectionProfileGroup extends Disposable implements IConnectionPro
 
 	public toObject(): IConnectionProfileGroup {
 		let subgroups = undefined;
-		if (this.children.length > 0) {
+		if (this._childGroups.length > 0) {
 			subgroups = [];
-			this.children.forEach((group) => {
+			this._childGroups.forEach((group) => {
 				subgroups.push(group.toObject());
 			});
 		}
@@ -75,8 +75,24 @@ export class ConnectionProfileGroup extends Disposable implements IConnectionPro
 		this._isRenamed = val;
 	}
 
+	public get children(): ConnectionProfileGroup[] | undefined {
+		return this._childGroups;
+	}
+
+	public set children(children: ConnectionProfileGroup[] | undefined) {
+		this._childGroups = children ?? [];
+	}
+
+	public get connections(): ConnectionProfile[] | undefined {
+		return this._childConnections;
+	}
+
+	public set connections(connections: ConnectionProfile[] | undefined) {
+		this._childConnections = connections ?? [];
+	}
+
 	public hasChildren(): boolean {
-		if (this.children.length > 0 || this.connections.length > 0) {
+		if (this._childGroups.length > 0 || this._childConnections.length > 0) {
 			return true;
 		}
 		return false;
@@ -86,12 +102,12 @@ export class ConnectionProfileGroup extends Disposable implements IConnectionPro
 	 * Returns true if all connections in the tree have valid options using the correct capabilities
 	 */
 	public get hasValidConnections(): boolean {
-		let invalidConnections = find(this.connections, c => !c.isConnectionOptionsValid);
+		let invalidConnections = find(this._childConnections, c => !c.isConnectionOptionsValid);
 		if (invalidConnections !== undefined) {
 			return false;
 		} else {
 			let childrenAreValid: boolean = true;
-			this.children.forEach(element => {
+			this._childGroups.forEach(element => {
 				let isChildValid = element.hasValidConnections;
 				if (!isChildValid) {
 					childrenAreValid = false;
@@ -103,11 +119,11 @@ export class ConnectionProfileGroup extends Disposable implements IConnectionPro
 
 	public getChildren(): (ConnectionProfile | ConnectionProfileGroup)[] {
 		let allChildren: (ConnectionProfile | ConnectionProfileGroup)[] = [];
-		this.connections.forEach((conn) => {
+		this._childConnections.forEach((conn) => {
 			allChildren.push(conn);
 		});
 
-		this.children.forEach((group) => {
+		this._childGroups.forEach((group) => {
 			allChildren.push(group);
 		});
 		return allChildren;
@@ -120,22 +136,22 @@ export class ConnectionProfileGroup extends Disposable implements IConnectionPro
 		return other.id === this.id;
 	}
 
-	public addConnections(connections: ConnectionProfile[]): void {
+	public addConnections(connections: ConnectionProfile[] | undefined): void {
-		connections.forEach((conn) => {
+		connections?.forEach((conn) => {
-			this.connections = this.connections.filter((curConn) => { return curConn.id !== conn.id; });
+			this._childConnections = this._childConnections.filter((curConn) => { return curConn.id !== conn.id; });
 			conn.parent = this;
 			this._register(conn);
-			this.connections.push(conn);
+			this._childConnections.push(conn);
 		});
 
 	}
 
-	public addGroups(groups: ConnectionProfileGroup[]): void {
+	public addGroups(groups: ConnectionProfileGroup[] | undefined): void {
-		groups.forEach((group) => {
+		groups?.forEach((group) => {
-			this.children = this.children.filter((grp) => { return group.id !== grp.id; });
+			this._childGroups = this._childGroups.filter((grp) => { return group.id !== grp.id; });
 			group.parent = this;
 			this._register(group);
-			this.children.push(group);
+			this._childGroups.push(group);
 		});
 	}
 

src/sql/platform/connection/common/utils.ts

@@ -117,14 +117,14 @@ export function generateUriWithPrefix(connection: IConnectionProfile, prefix: st
 
 export function findProfileInGroup(og: IConnectionProfile, groups: ConnectionProfileGroup[]): ConnectionProfile | undefined {
 	for (let group of groups) {
-		for (let conn of group.connections) {
+		for (let conn of group.connections!) {
 			if (conn.id === og.id) {
 				return conn;
 			}
 		}
 
 		if (group.hasChildren()) {
-			let potentialReturn = findProfileInGroup(og, group.children);
+			let potentialReturn = findProfileInGroup(og, group.children!);
 			if (potentialReturn) {
 				return potentialReturn;
 			}

src/sql/platform/connection/test/common/connectionProfileGroup.test.ts

@@ -5,6 +5,8 @@
 
 import { ConnectionProfileGroup } from 'sql/platform/connection/common/connectionProfileGroup';
 import * as assert from 'assert';
+import { ConnectionProfile } from 'sql/platform/connection/common/connectionProfile';
+import { TestCapabilitiesService } from 'sql/platform/capabilities/test/common/testCapabilitiesService';
 
 suite('SQL ConnectionProfileGroup tests', () => {
 	let root: ConnectionProfileGroup;
@@ -14,12 +16,17 @@ suite('SQL ConnectionProfileGroup tests', () => {
 	let group1Node: ConnectionProfileGroup;
 	let group11Node: ConnectionProfileGroup;
 	let group2Node: ConnectionProfileGroup;
+	let emptyGroup: ConnectionProfileGroup;
+	let connectionProfile: ConnectionProfile;
+
 	setup(() => {
 		root = new ConnectionProfileGroup(ConnectionProfileGroup.RootGroupName, undefined, ConnectionProfileGroup.RootGroupName, undefined, undefined);
 
 		group1Node = new ConnectionProfileGroup(Groups1, root, Groups1, undefined, undefined);
 		group2Node = new ConnectionProfileGroup(Groups2, root, Groups2, undefined, undefined);
 		group11Node = new ConnectionProfileGroup(Groups11, root, Groups11, undefined, undefined);
+		emptyGroup = new ConnectionProfileGroup(ConnectionProfileGroup.RootGroupName, undefined, ConnectionProfileGroup.RootGroupName);
+		connectionProfile = new ConnectionProfile(new TestCapabilitiesService(), undefined!);
 		root.addGroups([group1Node]);
 		group1Node.addGroups([group11Node]);
 		root.addGroups([group2Node]);
@@ -142,4 +149,66 @@ suite('SQL ConnectionProfileGroup tests', () => {
 		let actual = ConnectionProfileGroup.sameGroupName(name1, name2);
 		assert.deepEqual(actual, expected);
 	});
+
+	test('test behavior when children is set to undefined', () => {
+		emptyGroup.children = undefined;
+		assert(emptyGroup.hasChildren() === false, 'Group should report no children after setting children to undefined');
+		const obj = emptyGroup.toObject();
+		assert.equal(obj.id, emptyGroup.id, 'toObject result has wrong id');
+		assert.equal(obj.name, emptyGroup.name, 'toObject result has wrong name');
+		assert(emptyGroup.hasValidConnections === true, 'Expected group to have valid connections');
+		const children = emptyGroup.getChildren();
+		assert(children.length === 0, 'Expected group to have 0 children');
+	});
+
+	test('test behavior when connections is set to undefined', () => {
+		emptyGroup.connections = undefined;
+		assert(emptyGroup.hasChildren() === false, 'Group should report no children after setting connections to undefined');
+		const obj = emptyGroup.toObject();
+		assert.equal(obj.id, emptyGroup.id, 'toObject result has wrong id');
+		assert.equal(obj.name, emptyGroup.name, 'toObject result has wrong name');
+		assert(emptyGroup.hasValidConnections === true, 'Expected group to have valid connections');
+		const children = emptyGroup.getChildren();
+		assert.equal(children.length, 0, 'Expected group to have 0 children');
+	});
+
+	test('test behavior with 1 child group', () => {
+		emptyGroup.addGroups([group1Node]);
+		assert(emptyGroup.hasChildren() === true, 'Group should have children if 1 child group is added');
+		assert(emptyGroup.hasValidConnections === true, 'Expected group to have valid connections');
+		const children = emptyGroup.getChildren();
+		assert.equal(children.length, 1, 'Expected group to have 1 child');
+		assert.equal(children[0].id, group1Node.id, 'Expected group child to be group1Node');
+	});
+
+	test('test behavior with 1 child connection', () => {
+		emptyGroup.addConnections([connectionProfile]);
+		assert(emptyGroup.hasChildren(), 'Group should have children if 1 child group is added');
+		assert(emptyGroup.hasValidConnections === false, 'Expected group not to have valid connections');
+		const children = emptyGroup.getChildren();
+		assert.equal(children.length, 1, 'Expected group to have 1 child');
+		assert.equal(children[0].id, connectionProfile.id, 'Expected group child to be connectionProfile');
+	});
+
+	test('adding undefined groups does nothing', () => {
+		emptyGroup.addGroups(undefined);
+		assert(emptyGroup.hasChildren() === false, 'Expected group not to have any children');
+		// Verify adding undefined doesn't modify existing children
+		emptyGroup.addGroups([group1Node]);
+		emptyGroup.addGroups(undefined);
+		const children = emptyGroup.getChildren();
+		assert.equal(children.length, 1, 'Expected group to have 1 child still');
+		assert.equal(children[0].id, group1Node.id, 'Expected group child to be group1Node');
+	});
+
+	test('adding undefined connections does nothing', () => {
+		emptyGroup.addConnections(undefined);
+		assert(emptyGroup.hasChildren() === false, 'Expected group not to have any children');
+		// Verify adding undefined doesn't modify existing children
+		emptyGroup.addConnections([connectionProfile]);
+		emptyGroup.addConnections(undefined);
+		const children = emptyGroup.getChildren();
+		assert.equal(children.length, 1, 'Expected group to have 1 child still');
+		assert.equal(children[0].id, connectionProfile.id, 'Expected group child to be connectionProfile');
+	});
 });