Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notebooks: Only have One Connection in Attach To Dropdown #8582

Merged
merged 5 commits into from
Dec 8, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions extensions/integration-tests/src/notebook.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ import * as azdata from 'azdata';
import * as vscode from 'vscode';
import { context } from './testContext';
import { sqlNotebookContent, writeNotebookToFile, sqlKernelMetadata, getFileName, pySparkNotebookContent, pySparkKernelMetadata, pythonKernelMetadata, sqlNotebookMultipleCellsContent, notebookContentForCellLanguageTest, sqlKernelSpec, pythonKernelSpec, pySparkKernelSpec, CellTypes } from './notebook.util';
import { getBdcServer, getConfigValue, EnvironmentVariable_PYTHON_PATH } from './testConfig';
import { connectToServer, sleep } from './utils';
import { getBdcServer, getConfigValue, EnvironmentVariable_PYTHON_PATH, TestServerProfile } from './testConfig';
import { connectToServer, sleep, testServerProfileToIConnectionProfile } from './utils';
import * as fs from 'fs';
import { stressify } from 'adstest';
import { isNullOrUndefined } from 'util';
Expand Down Expand Up @@ -392,15 +392,20 @@ class NotebookTester {
async openNotebook(content: azdata.nb.INotebookContents, kernelMetadata: any, testName: string, connectToDifferentServer?: boolean): Promise<azdata.nb.NotebookEditor> {
let notebookConfig = vscode.workspace.getConfiguration('notebook');
notebookConfig.update('pythonPath', getConfigValue(EnvironmentVariable_PYTHON_PATH), 1);
let server: TestServerProfile;
if (!connectToDifferentServer) {
let server = await getBdcServer();
server = await getBdcServer();
assert(server && server.serverName, 'No server could be found in openNotebook');
await connectToServer(server, 6000);
}
let notebookJson = Object.assign({}, content, { metadata: kernelMetadata });
let uri = writeNotebookToFile(notebookJson, testName);
console.log('Notebook uri ' + uri);
let notebook = await azdata.nb.showNotebookDocument(uri);
let nbShowOptions: azdata.nb.NotebookShowOptions;
if (server) {
nbShowOptions = { connectionProfile: testServerProfileToIConnectionProfile(server) };
}
let notebook = await azdata.nb.showNotebookDocument(uri, nbShowOptions);
return notebook;
}

Expand Down
18 changes: 18 additions & 0 deletions extensions/integration-tests/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -271,3 +271,21 @@ export async function assertTableCreationResult(databaseName: string, schema: st
assert(result.columnInfo[0].columnName !== 'ErrorMessage', `Checking for table creation threw error ${result.rows[0][0].displayValue}`);
}
}

export function testServerProfileToIConnectionProfile(serverProfile: TestServerProfile): azdata.IConnectionProfile {
return {
serverName: serverProfile.serverName,
databaseName: serverProfile.database,
authenticationType: serverProfile.authenticationTypeName,
providerName: serverProfile.providerName,
connectionName: '',
userName: serverProfile.userName,
password: serverProfile.password,
savePassword: false,
groupFullName: undefined,
saveProfile: true,
id: undefined,
groupId: undefined,
options: {}
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import { UntitledTextEditorModel } from 'vs/workbench/common/editor/untitledText

export const CODE_SELECTOR: string = 'code-component';
const MARKDOWN_CLASS = 'markdown';
const DEFAULT_OR_LOCAL_CONTEXT_ID = '-1';

@Component({
selector: CODE_SELECTOR,
Expand Down Expand Up @@ -146,8 +147,8 @@ export class CodeComponent extends AngularDisposable implements OnInit, OnChange
let connectionService = this.connectionService;
if (!shouldConnect && connectionService && connectionService.isConnected(cellUri)) {
connectionService.disconnect(cellUri).catch(e => this.logService.error(e));
} else if (shouldConnect && this._model.activeConnection && this._model.activeConnection.id !== '-1') {
connectionService.connect(this._model.activeConnection, cellUri).catch(e => this.logService.error(e));
} else if (shouldConnect && this._model.context && this._model.context.id !== DEFAULT_OR_LOCAL_CONTEXT_ID) {
connectionService.connect(this._model.context, cellUri).catch(e => this.logService.error(e));
}
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/sql/workbench/contrib/notebook/browser/models/cell.ts
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ export class CellModel implements ICellModel {
this.sendNotification(notificationService, Severity.Info, localize('runCellCancelled', "Cell execution cancelled"));
} else {
// TODO update source based on editor component contents
if (kernel.requiresConnection && !this.notebookModel.activeConnection) {
if (kernel.requiresConnection && !this.notebookModel.context) {
let connected = await this.notebookModel.requestConnection();
if (!connected) {
return false;
Expand Down Expand Up @@ -509,11 +509,11 @@ export class CellModel implements ICellModel {
let result = output as nb.IDisplayResult;
if (result && result.data && result.data['text/html']) {
let model = this._options.notebook as NotebookModel;
if (model.activeConnection) {
let gatewayEndpointInfo = this.getGatewayEndpoint(model.activeConnection);
if (model.context) {
let gatewayEndpointInfo = this.getGatewayEndpoint(model.context);
if (gatewayEndpointInfo) {
let hostAndIp = notebookUtils.getHostAndPortFromEndpoint(gatewayEndpointInfo.endpoint);
let host = hostAndIp.host ? hostAndIp.host : model.activeConnection.serverName;
let host = hostAndIp.host ? hostAndIp.host : model.context.serverName;
let port = hostAndIp.port ? ':' + hostAndIp.port : defaultPort;
let html = result.data['text/html'];
// CTP 3.1 and earlier Spark link
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -214,11 +214,6 @@ export interface IClientSession extends IDisposable {
onKernelChanging(changeHandler: ((kernel: nb.IKernelChangedArgs) => Promise<void>)): void;
}

export interface IDefaultConnection {
defaultConnection: ConnectionProfile;
otherConnections: ConnectionProfile[];
}

/**
* A kernel preference.
*/
Expand Down Expand Up @@ -323,10 +318,10 @@ export interface INotebookModel {
readonly specs: nb.IAllKernels | undefined;

/**
* The specs for available contexts, or undefined if these have
* The specs for available context, or undefined if this has
* not been loaded yet
*/
readonly contexts: IDefaultConnection | undefined;
readonly context: ConnectionProfile | undefined;

/**
* Event fired on first initialization of the cells and
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,113 +3,44 @@
* Licensed under the Source EULA. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import { nb } from 'azdata';

import { localize } from 'vs/nls';
import { IDefaultConnection } from 'sql/workbench/contrib/notebook/browser/models/modelInterfaces';
import { IConnectionManagementService } from 'sql/platform/connection/common/connectionManagement';
import { ConnectionProfile } from 'sql/platform/connection/common/connectionProfile';
import { IConnectionProfile } from 'sql/platform/connection/common/interfaces';
import { mssqlProviderName } from 'sql/platform/connection/common/constants';
import { find } from 'vs/base/common/arrays';


export class NotebookContexts {

public static get DefaultContext(): IDefaultConnection {
let defaultConnection: ConnectionProfile = <any>{
public static get DefaultContext(): ConnectionProfile {
return <any>{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any [](start = 10, length = 3)

Why cast to any? Should this be ConnectionProfile?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(same below for LocalContext)


In reply to: 354967886 [](ancestors = 354967886)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sigh. These are fake connection profile objects and I really really really don't like it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to fix it but went down a bit of a rabbit hole, so may have to save that for another PR for more cleanup 😢

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I think the approach i would try if I was fixing this would be to have a context be ConnectionProfile | DefaultContextType, that way instead of doing stuff like checking ID you just check the type of the object at runtime and use that to determine what you do.


In reply to: 354972934 [](ancestors = 354972934)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes a ton of sense, thanks

providerName: mssqlProviderName,
id: '-1',
serverName: localize('selectConnection', "Select Connection")
};

return {
// default context if no other contexts are applicable
defaultConnection: defaultConnection,
otherConnections: [defaultConnection]
};
}

public static get LocalContext(): IDefaultConnection {
let localConnection: ConnectionProfile = <any>{
public static get LocalContext(): ConnectionProfile {
return <any>{
providerName: mssqlProviderName,
id: '-1',
serverName: localize('localhost', "localhost")
};

return {
// default context if no other contexts are applicable
defaultConnection: localConnection,
otherConnections: [localConnection]
};
}

/**
* Get all of the applicable contexts for a given kernel
* @param connectionService connection management service
* Get the applicable context for a given kernel
* @param context current connection profile
* @param connProviderIds array of connection provider ids applicable for a kernel
* @param kernelChangedArgs kernel changed args (both old and new kernel info)
* @param profile current connection profile
*/
public static getContextsForKernel(connectionService: IConnectionManagementService, connProviderIds: string[], kernelChangedArgs?: nb.IKernelChangedArgs, profile?: IConnectionProfile): IDefaultConnection {
let connections: IDefaultConnection = this.DefaultContext;
if (!profile) {
if (!kernelChangedArgs || !kernelChangedArgs.newValue ||
(kernelChangedArgs.oldValue && kernelChangedArgs.newValue.id === kernelChangedArgs.oldValue.id)) {
// nothing to do, kernels are the same or new kernel is undefined
return connections;
}
}
if (kernelChangedArgs && kernelChangedArgs.newValue && kernelChangedArgs.newValue.name && connProviderIds.length < 1) {
return connections;
} else {
connections = this.getActiveContexts(connectionService, connProviderIds, profile);
}
return connections;
}

/**
* Get all active contexts and sort them
* @param connectionService connection service
* @param connProviderIds array of applicable connection providers to filter connections
* @param profile connection profile passed when launching notebook
*/
public static getActiveContexts(connectionService: IConnectionManagementService, connProviderIds: string[], profile?: IConnectionProfile): IDefaultConnection {
let defaultConnection: ConnectionProfile = NotebookContexts.DefaultContext.defaultConnection;
let activeConnections: ConnectionProfile[] = connectionService.getActiveConnections();
if (activeConnections && activeConnections.length > 0) {
activeConnections = activeConnections.filter(conn => conn.id !== '-1');
}
public static getContextForKernel(context: ConnectionProfile, connProviderIds: string[]): ConnectionProfile {
// If no connection provider ids exist for a given kernel, the attach to should show localhost
if (connProviderIds.length === 0) {
return NotebookContexts.LocalContext;
}
// If no active connections exist, show "Select connection" as the default value
if (activeConnections.length === 0) {
return NotebookContexts.DefaultContext;
if (context && context.providerName && connProviderIds.filter(p => p === context.providerName).length > 0) {
return context;
}
// Filter active connections by their provider ids to match kernel's supported connection providers
else if (activeConnections.length > 0) {
let connections = activeConnections.filter(connection => {
return connProviderIds.some(x => x === connection.providerName);
});
if (connections && connections.length > 0) {
defaultConnection = connections[0];
if (profile && profile.options) {
let matchingConn = find(connections, connection => connection.serverName === profile.serverName);
if (matchingConn) {
defaultConnection = matchingConn;
}
}
} else if (connections.length === 0) {
return NotebookContexts.DefaultContext;
}
activeConnections = [];
connections.forEach(connection => activeConnections.push(connection));
}

return {
otherConnections: activeConnections,
defaultConnection: defaultConnection
};
return NotebookContexts.DefaultContext;
}

}
Loading