Fix for PG parameters table disappearing when trying to reset a value (#15303)

* Working on table bug

* Removed unit test checking for onEngineSettingsUpdated event to fire since removed from model

* Moved instanceofcheckbox function to utils, pr fixes

* Fix comment

extensions/arc/src/common/utils.ts

@@ -4,6 +4,7 @@
  *--------------------------------------------------------------------------------------------*/
 
 import { ResourceType } from 'arc';
+import * as azdata from 'azdata';
 import * as azurecore from 'azurecore';
 import * as vscode from 'vscode';
 import { ConnectionMode, IconPath, IconPathHelper } from '../constants';
@@ -300,6 +301,13 @@ export function convertToGibibyteString(value: string): string {
 	return String(floatValue);
 }
 
+/**
+ * Used to confirm if object is an azdata CheckBoxComponent
+ */
+export function instanceOfCheckBox(object: any): object is azdata.CheckBoxComponent {
+	return 'checked' in object;
+}
+
 /*
  * Throws an Error with given {@link message} unless {@link condition} is true.
  * This also tells the typescript compiler that the condition is 'truthy' in the remainder of the scope

extensions/arc/src/models/postgresModel.ts

@@ -33,13 +33,12 @@ export class PostgresModel extends ResourceModel {
 	private readonly _azdataApi: azdataExt.IExtension;
 
 	private readonly _onConfigUpdated = new vscode.EventEmitter<azdataExt.PostgresServerShowResult>();
-	public readonly _onEngineSettingsUpdated = new vscode.EventEmitter<EngineSettingsModel[]>();
 	public onConfigUpdated = this._onConfigUpdated.event;
-	public onEngineSettingsUpdated = this._onEngineSettingsUpdated.event;
 	public configLastUpdated?: Date;
 	public engineSettingsLastUpdated?: Date;
 
 	private _refreshPromise?: Deferred<void>;
+	private _engineSettingsPromise?: Deferred<void>;
 
 	constructor(_controllerModel: ControllerModel, private _pgInfo: PGResourceInfo, registration: Registration, private _treeDataProvider: AzureArcTreeDataProvider) {
 		super(_controllerModel, _pgInfo, registration);
@@ -132,55 +131,69 @@ export class PostgresModel extends ResourceModel {
 	}
 
 	public async getEngineSettings(): Promise<void> {
-		if (!this._connectionProfile) {
+		// Only allow to get engine setting once at a time
-			await this.getConnectionProfile();
+		if (this._engineSettingsPromise) {
+			return this._engineSettingsPromise.promise;
 		}
+		this._engineSettingsPromise = new Deferred();
 
-		// We haven't connected yet so do so now and then store the ID for the active connection
+		try {
-		if (!this._activeConnectionId) {
+			if (!this._connectionProfile) {
-			const result = await azdata.connection.connect(this._connectionProfile!, false, false);
+				await this.getConnectionProfile();
-			if (!result.connected) {
-				throw new Error(result.errorMessage);
 			}
-			this._activeConnectionId = result.connectionId;
-		}
 
-		// TODO Need to make separate calls for worker nodes and coordinator node
+			// We haven't connected yet so do so now and then store the ID for the active connection
-		const provider = azdata.dataprotocol.getProvider<azdata.QueryProvider>(this._connectionProfile!.providerName, azdata.DataProviderType.QueryProvider);
+			if (!this._activeConnectionId) {
-		const ownerUri = await azdata.connection.getUriForConnection(this._activeConnectionId);
+				const result = await azdata.connection.connect(this._connectionProfile!, false, false);
-
+				if (!result.connected) {
-		const engineSettings = await provider.runQueryAndReturn(ownerUri, 'select name, setting, short_desc,min_val, max_val, enumvals, vartype from pg_settings');
+					throw new Error(result.errorMessage);
-		if (!engineSettings) {
+				}
-			throw new Error('Could not fetch engine settings');
+				this._activeConnectionId = result.connectionId;
-		}
-
-		const skippedEngineSettings: String[] = [
-			'archive_command', 'archive_timeout', 'log_directory', 'log_file_mode', 'log_filename', 'restore_command',
-			'shared_preload_libraries', 'synchronous_commit', 'ssl', 'unix_socket_permissions', 'wal_level'
-		];
-
-		this.workerNodesEngineSettings = [];
-
-		engineSettings.rows.forEach(row => {
-			let rowValues = row.map(c => c.displayValue);
-			let name = rowValues.shift();
-			if (!skippedEngineSettings.includes(name!)) {
-				let result: EngineSettingsModel = {
-					parameterName: name,
-					value: rowValues.shift(),
-					description: rowValues.shift(),
-					min: rowValues.shift(),
-					max: rowValues.shift(),
-					options: rowValues.shift(),
-					type: rowValues.shift()
-				};
-
-				this.workerNodesEngineSettings.push(result);
 			}
-		});
 
-		this.engineSettingsLastUpdated = new Date();
+			// TODO Need to make separate calls for worker nodes and coordinator node
-		this._onEngineSettingsUpdated.fire(this.workerNodesEngineSettings);
+			const provider = azdata.dataprotocol.getProvider<azdata.QueryProvider>(this._connectionProfile!.providerName, azdata.DataProviderType.QueryProvider);
+			const ownerUri = await azdata.connection.getUriForConnection(this._activeConnectionId);
+
+			const engineSettings = await provider.runQueryAndReturn(ownerUri, 'select name, setting, short_desc,min_val, max_val, enumvals, vartype from pg_settings');
+			if (!engineSettings) {
+				throw new Error('Could not fetch engine settings');
+			}
+
+			const skippedEngineSettings: String[] = [
+				'archive_command', 'archive_timeout', 'log_directory', 'log_file_mode', 'log_filename', 'restore_command',
+				'shared_preload_libraries', 'synchronous_commit', 'ssl', 'unix_socket_permissions', 'wal_level'
+			];
+
+			this.workerNodesEngineSettings = [];
+
+			engineSettings.rows.forEach(row => {
+				let rowValues = row.map(c => c.displayValue);
+				let name = rowValues.shift();
+				if (!skippedEngineSettings.includes(name!)) {
+					let result: EngineSettingsModel = {
+						parameterName: name,
+						value: rowValues.shift(),
+						description: rowValues.shift(),
+						min: rowValues.shift(),
+						max: rowValues.shift(),
+						options: rowValues.shift(),
+						type: rowValues.shift()
+					};
+
+					this.workerNodesEngineSettings.push(result);
+				}
+			});
+
+
+			this.engineSettingsLastUpdated = new Date();
+			this._engineSettingsPromise.resolve();
+		} catch (err) {
+			this._engineSettingsPromise.reject(err);
+			throw err;
+		} finally {
+			this._engineSettingsPromise = undefined;
+		}
 	}
 
 	protected createConnectionProfile(): azdata.IConnectionProfile {

extensions/arc/src/test/models/postgresModel.test.ts

@@ -462,29 +462,6 @@ describe('PostgresModel', function (): void {
 			should(postgresModel.engineSettingsLastUpdated).be.Date();
 		});
 
-		it('Calls onEngineSettingsUpdated event after populating engine settings', async function (): Promise<void> {
-			const connectionResultMock = TypeMoq.Mock.ofType<azdata.ConnectionResult>();
-			connectionResultMock.setup(x => x.connected).returns(() => true);
-			connectionResultMock.setup((x: any) => x.then).returns(() => undefined);
-			sinon.stub(azdata.connection, 'connect').returns(Promise.resolve(connectionResultMock.object));
-
-			const array: azdata.DbCellValue[][] = [];
-
-			const executeMock = TypeMoq.Mock.ofType<azdata.SimpleExecuteResult>();
-			executeMock.setup(x => x.rows).returns(() => array);
-			executeMock.setup((x: any) => x.then).returns(() => undefined);
-
-			const providerMock = TypeMoq.Mock.ofType<azdata.QueryProvider>();
-			providerMock.setup(x => x.runQueryAndReturn(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(async () => executeMock.object);
-			providerMock.setup((x: any) => x.then).returns(() => undefined);
-			sinon.stub(azdata.dataprotocol, 'getProvider').returns(providerMock.object);
-
-			const onEngineSettingsUpdated = sinon.spy(vscode.EventEmitter.prototype, 'fire');
-
-			await postgresModel.getEngineSettings();
-			sinon.assert.calledOnceWithExactly(onEngineSettingsUpdated, postgresModel.workerNodesEngineSettings);
-		});
-
 		it('Populating engine settings skips certain parameters', async function (): Promise<void> {
 			const connectionResultMock = TypeMoq.Mock.ofType<azdata.ConnectionResult>();
 			connectionResultMock.setup(x => x.connected).returns(() => true);

extensions/arc/src/ui/dashboards/postgres/postgresParameters.ts

@@ -11,7 +11,7 @@ import { UserCancelledError } from '../../../common/api';
 import { IconPathHelper, cssStyles } from '../../../constants';
 import { DashboardPage } from '../../components/dashboardPage';
 import { EngineSettingsModel, PostgresModel } from '../../../models/postgresModel';
-import { debounce } from '../../../common/utils';
+import { debounce, instanceOfCheckBox } from '../../../common/utils';
 
 export type ParametersModel = {
 	parameterName: string,
@@ -47,8 +47,7 @@ export abstract class PostgresParametersPage extends DashboardPage {
 		this.initializeSearchBox();
 
 		this.disposables.push(
-			this._postgresModel.onConfigUpdated(() => this.eventuallyRunOnInitialized(() => this.handleServiceUpdated())),
+			this._postgresModel.onConfigUpdated(() => this.eventuallyRunOnInitialized(() => this.handleServiceUpdated()))
-			this._postgresModel.onEngineSettingsUpdated(() => this.eventuallyRunOnInitialized(() => this.refreshParametersTable()))
 		);
 	}
 
@@ -160,9 +159,9 @@ export abstract class PostgresParametersPage extends DashboardPage {
 								throw err;
 							}
 							try {
-								await this._postgresModel.refresh();
+								await this.callGetEngineSettings();
 							} catch (error) {
-								vscode.window.showErrorMessage(loc.refreshFailed(error));
+								vscode.window.showErrorMessage(loc.fetchEngineSettingsFailed(this._postgresModel.info.name, error));
 							}
 						}
 					);
@@ -238,9 +237,9 @@ export abstract class PostgresParametersPage extends DashboardPage {
 							}
 							this.changedComponentValues.clear();
 							try {
-								await this._postgresModel.refresh();
+								await this.callGetEngineSettings();
 							} catch (error) {
-								vscode.window.showErrorMessage(loc.refreshFailed(error));
+								vscode.window.showErrorMessage(loc.fetchEngineSettingsFailed(this._postgresModel.info.name, error));
 							}
 						}
 					);
@@ -321,13 +320,19 @@ export abstract class PostgresParametersPage extends DashboardPage {
 			this.searchBox.enabled = true;
 			this.resetAllButton.enabled = true;
 			this.parameterContainer.addItem(this._parametersTable!);
-			this.refreshParametersTable();
+			this.refreshParametersTableValues();
 		}
 	}
 
 	private async callGetEngineSettings(): Promise<void> {
 		try {
-			await this._postgresModel.getEngineSettings();
+			await this._postgresModel.getEngineSettings().then(() => {
+				if (this._parametersTable.data?.length !== 0) {
+					this.refreshParametersTableValues();
+				} else {
+					this.populateParametersTable();
+				}
+			});
 		} catch (error) {
 			if (error instanceof UserCancelledError) {
 				vscode.window.showWarningMessage(loc.pgConnectionRequired);
@@ -415,9 +420,9 @@ export abstract class PostgresParametersPage extends DashboardPage {
 						async (_progress, _token): Promise<void> => {
 							await this.resetParameter(engineSetting.parameterName!);
 							try {
-								await this._postgresModel.refresh();
+								await this.callGetEngineSettings();
 							} catch (error) {
-								vscode.window.showErrorMessage(loc.refreshFailed(error));
+								vscode.window.showErrorMessage(loc.fetchEngineSettingsFailed(this._postgresModel.info.name, error));
 							}
 						}
 					);
@@ -573,10 +578,6 @@ export abstract class PostgresParametersPage extends DashboardPage {
 	}
 
 	private discardParametersTableChanges(): void {
-		let instanceOfCheckBox = function (object: any): object is azdata.CheckBoxComponent {
-			return 'checked' in object;
-		};
-
 		this.changedComponentValues.forEach(v => {
 			let param = this._parameters.find(p => p.parameterName === v);
 			if (instanceOfCheckBox(param!.valueComponent)) {
@@ -591,7 +592,7 @@ export abstract class PostgresParametersPage extends DashboardPage {
 		});
 	}
 
-	private refreshParametersTable(): void {
+	private populateParametersTable(): void {
 		this._parameters = this.engineSettings.map(parameter => this.createParameterComponents(parameter));
 
 		this._parametersTable.data = this._parameters.map(p => {
@@ -607,14 +608,37 @@ export abstract class PostgresParametersPage extends DashboardPage {
 		});
 	}
 
+	/**
+	 * Checks if exisiting parameter values needs to be updated.
+	 * Only updates exisiting parameters, will not add/remove parameters from the table.
+	 */
+	private refreshParametersTableValues(): void {
+		this.engineSettings.map(parameter => {
+			let param = this._parameters.find(p => p.parameterName === parameter.parameterName);
+			if (param) {
+				if (parameter.value !== param.originalValue) {
+					param.originalValue = parameter.value!;
+
+					if (instanceOfCheckBox(param.valueComponent)) {
+						if (param.originalValue === 'on') {
+							param.valueComponent.checked = true;
+						} else {
+							param.valueComponent.checked = false;
+						}
+					} else {
+						param.valueComponent.value = parameter.value;
+					}
+				}
+			}
+		});
+	}
+
 	private async handleServiceUpdated(): Promise<void> {
 		if (this._postgresModel.configLastUpdated && !this._postgresModel.engineSettingsLastUpdated) {
 			this.connectToServerButton!.enabled = true;
 			this.parametersTableLoading!.loading = false;
 		} else if (this._postgresModel.engineSettingsLastUpdated) {
 			await this.callGetEngineSettings();
-			this.discardButton.enabled = false;
-			this.saveButton.enabled = false;
 		}
 	}