Make various enhancements to Notebook Provider registration. (#17609)

* Use built-in SQL ExecuteProvider by default if no other provider exists.

* Gracefully handle case where standardKernels are not defined for a provider.

* Standardize on just using arrays for various provider registration details.

extensions/notebook/package.json

@@ -686,7 +686,7 @@
         ]
       }
     ],
-    "notebook.providers": {
+    "notebook.providers": [{
       "provider": "jupyter",
       "fileExtensions": [
         "IPYNB"
@@ -727,7 +727,7 @@
           "connectionProviderIds": []
         }
       ]
-    }
+    }]
   },
   "dependencies": {
     "@jupyterlab/services": "^3.2.1",

src/sql/workbench/contrib/notebook/test/browser/notebookService.test.ts

@@ -234,12 +234,12 @@ suite.skip('NotebookService:', function (): void {
 		assert.deepStrictEqual(notebookService.getProvidersForFileType('ipynb'), ['sql'], 'sql provider should be registered for ipynb extension');
 
 		const otherProviderRegistration: ProviderDescriptionRegistration = {
-			fileExtensions: 'ipynb',
+			fileExtensions: ['ipynb'],
-			standardKernels: {
+			standardKernels: [{
 				name: 'kernel1',
 				connectionProviderIds: [],
 				displayName: 'Kernel 1'
-			},
+			}],
 			provider: 'otherProvider'
 		};
 
@@ -249,7 +249,7 @@ suite.skip('NotebookService:', function (): void {
 		assert.deepStrictEqual(notebookService.getProvidersForFileType('ipynb'), ['sql', 'otherProvider'], 'otherProvider should also be registered for ipynb extension');
 		assert.deepStrictEqual(notebookService.getSupportedFileExtensions(), ['IPYNB'], 'Only IPYNB should be registered as supported file extension');
 		assert.strictEqual(notebookService.getStandardKernelsForProvider('otherProvider').length, 1, 'otherProvider kernel info could not be found');
-		assert.deepStrictEqual(notebookService.getStandardKernelsForProvider('otherProvider')[0], otherProviderRegistration.standardKernels, 'otherProviderRegistration standard kernels does not match');
+		assert.deepStrictEqual(notebookService.getStandardKernelsForProvider('otherProvider')[0], otherProviderRegistration.standardKernels[0], 'otherProviderRegistration standard kernels does not match');
 	});
 
 	test('tests that dispose() method calls dispose on underlying disposable objects exactly once', async () => {

src/sql/workbench/contrib/notebook/test/electron-browser/notebookUtils.test.ts

@@ -8,10 +8,11 @@ import * as TypeMoq from 'typemoq';
 
 import { nb, ServerInfo } from 'azdata';
 import { getHostAndPortFromEndpoint, isStream, getProvidersForFileName, asyncForEach, clusterEndpointsProperty, getClusterEndpoints, RawEndpoint, IEndpoint, getStandardKernelsForProvider, IStandardKernelWithProvider, rewriteUrlUsingRegex } from 'sql/workbench/services/notebook/browser/models/notebookUtils';
-import { INotebookService, DEFAULT_NOTEBOOK_FILETYPE, DEFAULT_NOTEBOOK_PROVIDER } from 'sql/workbench/services/notebook/browser/notebookService';
+import { INotebookService, DEFAULT_NOTEBOOK_FILETYPE, DEFAULT_NOTEBOOK_PROVIDER, SQL_NOTEBOOK_PROVIDER } from 'sql/workbench/services/notebook/browser/notebookService';
 import { NotebookServiceStub } from 'sql/workbench/contrib/notebook/test/stubs';
 import { tryMatchCellMagic, extractCellMagicCommandPlusArgs } from 'sql/workbench/services/notebook/browser/utils';
 import { RichTextEditStack } from 'sql/workbench/contrib/notebook/browser/cellViews/textCell.component';
+import { notebookConstants } from 'sql/workbench/services/notebook/browser/interfaces';
 
 suite('notebookUtils', function (): void {
 	const mockNotebookService = TypeMoq.Mock.ofType<INotebookService>(NotebookServiceStub);
@@ -22,6 +23,11 @@ suite('notebookUtils', function (): void {
 		displayName: 'testDisplayName',
 		connectionProviderIds: ['testId1', 'testId2']
 	};
+	const sqlStandardKernel: nb.IStandardKernel = {
+		name: notebookConstants.SQL,
+		displayName: notebookConstants.SQL,
+		connectionProviderIds: [notebookConstants.SQL_CONNECTION_PROVIDER]
+	};
 
 	function setupMockNotebookService() {
 		mockNotebookService.setup(n => n.getProvidersForFileType(TypeMoq.It.isAnyString()))
@@ -34,10 +40,17 @@ suite('notebookUtils', function (): void {
 			});
 
 		// getStandardKernelsForProvider
-		mockNotebookService.setup(n => n.getStandardKernelsForProvider(TypeMoq.It.isAnyString()))
+		let returnHandler = (provider) => {
-			.returns((provider) => {
+			if (provider === testProvider) {
 				return [testKernel];
-			});
+			} else if (provider === SQL_NOTEBOOK_PROVIDER) {
+				return [sqlStandardKernel];
+			} else {
+				return undefined;
+			}
+		};
+		mockNotebookService.setup(n => n.getStandardKernelsForProvider(TypeMoq.It.isAnyString())).returns(returnHandler);
+		mockNotebookService.setup(n => n.getStandardKernelsForProvider(TypeMoq.It.isAnyString())).returns(returnHandler);
 	}
 
 	test('isStream Test', async function (): Promise<void> {
@@ -87,6 +100,9 @@ suite('notebookUtils', function (): void {
 		result = getStandardKernelsForProvider('testProvider', undefined);
 		assert.deepStrictEqual(result, []);
 
+		result = getStandardKernelsForProvider('NotARealProvider', mockNotebookService.object);
+		assert.deepStrictEqual(result, [Object.assign({ notebookProvider: 'NotARealProvider' }, sqlStandardKernel)]);
+
 		result = getStandardKernelsForProvider('testProvider', mockNotebookService.object);
 		assert.deepStrictEqual(result, [<IStandardKernelWithProvider>{
 			name: 'testName',

src/sql/workbench/services/notebook/browser/models/notebookModel.ts

@@ -149,7 +149,7 @@ export class NotebookModel extends Disposable implements INotebookModel {
 	public get serializationManager(): ISerializationManager | undefined {
 		let manager = this.serializationManagers.find(manager => manager.providerId === this._providerId);
 		if (!manager) {
-			manager = this.serializationManagers.find(manager => manager.providerId === DEFAULT_NOTEBOOK_PROVIDER);
+			manager = this.serializationManagers.find(manager => manager.providerId === SQL_NOTEBOOK_PROVIDER);
 		}
 		return manager;
 	}
@@ -165,9 +165,7 @@ export class NotebookModel extends Disposable implements INotebookModel {
 	public get executeManager(): IExecuteManager | undefined {
 		let manager = this.executeManagers.find(manager => manager.providerId === this._providerId);
 		if (!manager) {
-			// Note: this seems like a less than ideal scenario. We should ideally pass in the "correct" provider ID and allow there to be a default,
+			manager = this.executeManagers.find(manager => manager.providerId === SQL_NOTEBOOK_PROVIDER);
-			// instead of assuming in the NotebookModel constructor that the option is either SQL or Jupyter
-			manager = this.executeManagers.find(manager => manager.providerId === DEFAULT_NOTEBOOK_PROVIDER);
 		}
 		return manager;
 	}

src/sql/workbench/services/notebook/browser/models/notebookUtils.ts

@@ -5,7 +5,7 @@
 
 import * as path from 'vs/base/common/path';
 import { nb, ServerInfo } from 'azdata';
-import { DEFAULT_NOTEBOOK_PROVIDER, DEFAULT_NOTEBOOK_FILETYPE, INotebookService } from 'sql/workbench/services/notebook/browser/notebookService';
+import { DEFAULT_NOTEBOOK_PROVIDER, DEFAULT_NOTEBOOK_FILETYPE, INotebookService, SQL_NOTEBOOK_PROVIDER } from 'sql/workbench/services/notebook/browser/notebookService';
 import { URI } from 'vs/base/common/uri';
 
 export const clusterEndpointsProperty = 'clusterEndpoints';
@@ -41,6 +41,10 @@ export function getStandardKernelsForProvider(providerId: string, notebookServic
 		return [];
 	}
 	let standardKernels = notebookService.getStandardKernelsForProvider(providerId);
+	if (!standardKernels || standardKernels.length === 0) {
+		// Fall back to using SQL provider instead
+		standardKernels = notebookService.getStandardKernelsForProvider(SQL_NOTEBOOK_PROVIDER) ?? [];
+	}
 	standardKernels.forEach(kernel => {
 		Object.assign(<IStandardKernelWithProvider>kernel, {
 			name: kernel.name,

src/sql/workbench/services/notebook/browser/notebookService.ts

@@ -72,9 +72,9 @@ export interface INotebookService {
 
 	getSupportedFileExtensions(): string[];
 
-	getProvidersForFileType(fileType: string): string[];
+	getProvidersForFileType(fileType: string): string[] | undefined;
 
-	getStandardKernelsForProvider(provider: string): azdata.nb.IStandardKernel[];
+	getStandardKernelsForProvider(provider: string): azdata.nb.IStandardKernel[] | undefined;
 
 	getOrCreateSerializationManager(providerId: string, uri: URI): Promise<ISerializationManager>;
 

src/sql/workbench/services/notebook/browser/notebookServiceImpl.ts

@@ -306,27 +306,21 @@ export class NotebookService extends Disposable implements INotebookService {
 
 	private handleNewProviderDescriptions(p: { id: string; registration: ProviderDescriptionRegistration }) {
 		let registration = p.registration;
-		if (registration.fileExtensions) {
+		if (registration.fileExtensions?.length > 0) {
 			let extensions = registration.fileExtensions;
 			if (!this._serializationProviders.has(p.id)) {
 				// Only add a new provider descriptor if the provider
 				// supports file extensions beyond the default ipynb
-				let isNewFileType = (fileExt: string) => fileExt?.length > 0 && fileExt.toUpperCase() !== DEFAULT_NOTEBOOK_FILETYPE;
+				let addNewProvider = extensions.some(ext => ext?.length > 0 && ext.toUpperCase() !== DEFAULT_NOTEBOOK_FILETYPE);
-				let addNewProvider = Array.isArray(extensions) ? extensions.some(ext => isNewFileType(ext)) : isNewFileType(extensions);
 				if (addNewProvider) {
 					this._serializationProviders.set(p.id, new SerializationProviderDescriptor(p.id));
 				}
 			}
-			if (Array.isArray(extensions)) {
 			for (let fileType of extensions) {
 				this.addFileProvider(fileType, registration);
 			}
 		}
-			else {
+		if (registration.standardKernels?.length > 0) {
-				this.addFileProvider(extensions, registration);
-			}
-		}
-		if (registration.standardKernels) {
 			if (!this._executeProviders.has(p.id)) {
 				this._executeProviders.set(p.id, new ExecuteProviderDescriptor(p.id));
 			}
@@ -402,13 +396,9 @@ export class NotebookService extends Disposable implements INotebookService {
 		if (!standardKernels) {
 			standardKernels = [];
 		}
-		if (Array.isArray(provider.standardKernels)) {
 		provider.standardKernels.forEach(kernel => {
 			standardKernels.push(kernel);
 		});
-		} else {
-			standardKernels.push(provider.standardKernels);
-		}
 		// Filter out unusable kernels when running on a SAW
 		if (this.productService.quality === 'saw') {
 			standardKernels = standardKernels.filter(kernel => !kernel.blockedOnSAW);
@@ -420,14 +410,12 @@ export class NotebookService extends Disposable implements INotebookService {
 		return Array.from(this._fileToProviderDescriptions.keys());
 	}
 
-	getProvidersForFileType(fileType: string): string[] {
+	getProvidersForFileType(fileType: string): string[] | undefined {
-		fileType = fileType.toUpperCase();
+		let providers = this._fileToProviderDescriptions.get(fileType.toUpperCase());
-		let providers = this._fileToProviderDescriptions.get(fileType);
+		return providers?.map(provider => provider.provider);
-
-		return providers ? providers.map(provider => provider.provider) : undefined;
 	}
 
-	getStandardKernelsForProvider(provider: string): nb.IStandardKernel[] {
+	getStandardKernelsForProvider(provider: string): nb.IStandardKernel[] | undefined {
 		return this._providerToStandardKernels.get(provider.toUpperCase());
 	}
 
@@ -700,8 +688,8 @@ export class NotebookService extends Disposable implements INotebookService {
 
 		notebookRegistry.registerProviderDescription({
 			provider: serializationProvider.providerId,
-			fileExtensions: DEFAULT_NOTEBOOK_FILETYPE,
+			fileExtensions: [DEFAULT_NOTEBOOK_FILETYPE],
-			standardKernels: { name: notebookConstants.SQL, displayName: notebookConstants.SQL, connectionProviderIds: [notebookConstants.SQL_CONNECTION_PROVIDER] }
+			standardKernels: [{ name: notebookConstants.SQL, displayName: notebookConstants.SQL, connectionProviderIds: [notebookConstants.SQL_CONNECTION_PROVIDER] }]
 		});
 	}
 

src/sql/workbench/services/notebook/common/notebookRegistry.ts

@@ -19,8 +19,8 @@ export const Extensions = {
 
 export interface ProviderDescriptionRegistration {
 	provider: string;
-	fileExtensions: string | string[];
+	fileExtensions: string[];
-	standardKernels: azdata.nb.IStandardKernel | azdata.nb.IStandardKernel[];
+	standardKernels: azdata.nb.IStandardKernel[];
 }
 
 let providerDescriptionType: IJSONSchema = {
@@ -33,45 +33,22 @@ let providerDescriptionType: IJSONSchema = {
 		},
 		fileExtensions: {
 			description: localize('carbon.extension.contributes.notebook.fileExtensions', "What file extensions should be registered to this notebook provider"),
-			oneOf: [
-				{ type: 'string' },
-				{
 			type: 'array',
 			items: {
 				type: 'string'
 			}
-				}
-			]
 		},
 		standardKernels: {
 			description: localize('carbon.extension.contributes.notebook.standardKernels', "What kernels should be standard with this notebook provider"),
-			oneOf: [
+			type: 'array',
-				{
+			items: {
 				type: 'object',
 				properties: {
 					name: {
-							type: 'string',
+						type: 'string'
 					},
 					displayName: {
-							type: 'string',
-						},
-						connectionProviderIds: {
-							type: 'array',
-							items: {
 						type: 'string'
-							}
-						}
-					}
-				},
-				{
-					type: 'array',
-					items: {
-						type: 'object',
-						items: {
-							type: 'object',
-							properties: {
-								name: {
-									type: 'string',
 					},
 					connectionProviderIds: {
 						type: 'array',
@@ -83,20 +60,12 @@ let providerDescriptionType: IJSONSchema = {
 			}
 		}
 	}
-			]
-		}
-	}
 };
 
 let providerDescriptionContrib: IJSONSchema = {
 	description: localize('vscode.extension.contributes.notebook.providersDescriptions', "Contributes notebook provider descriptions."),
-	oneOf: [
-		providerDescriptionType,
-		{
 	type: 'array',
 	items: providerDescriptionType
-		}
-	]
 };
 let notebookLanguageMagicType: IJSONSchema = {
 	type: 'object',
@@ -116,28 +85,18 @@ let notebookLanguageMagicType: IJSONSchema = {
 		},
 		kernels: {
 			description: localize('carbon.extension.contributes.notebook.kernels', "Optional set of kernels this is valid for, e.g. python3, pyspark, sql"),
-			oneOf: [
-				{ type: 'string' },
-				{
 			type: 'array',
 			items: {
 				type: 'string'
 			}
 		}
-			]
-		}
 	}
 };
 
 let languageMagicContrib: IJSONSchema = {
 	description: localize('vscode.extension.contributes.notebook.languagemagics', "Contributes notebook language."),
-	oneOf: [
-		notebookLanguageMagicType,
-		{
 	type: 'array',
 	items: notebookLanguageMagicType
-		}
-	]
 };
 
 export interface NotebookLanguageMagicRegistration {
@@ -189,28 +148,20 @@ class NotebookProviderRegistry implements INotebookProviderRegistry {
 const notebookProviderRegistry = new NotebookProviderRegistry();
 platform.Registry.add(NotebookProviderRegistryId, notebookProviderRegistry);
 
-ExtensionsRegistry.registerExtensionPoint<ProviderDescriptionRegistration | ProviderDescriptionRegistration[]>({ extensionPoint: Extensions.NotebookProviderDescriptionContribution, jsonSchema: providerDescriptionContrib }).setHandler(extensions => {
+ExtensionsRegistry.registerExtensionPoint<ProviderDescriptionRegistration[]>({ extensionPoint: Extensions.NotebookProviderDescriptionContribution, jsonSchema: providerDescriptionContrib }).setHandler(extensions => {
 	for (let extension of extensions) {
 		const { value } = extension;
-		if (Array.isArray(value)) {
 		for (let command of value) {
 			notebookProviderRegistry.registerProviderDescription(command);
 		}
-		} else {
-			notebookProviderRegistry.registerProviderDescription(value);
-		}
 	}
 });
 
-ExtensionsRegistry.registerExtensionPoint<NotebookLanguageMagicRegistration | NotebookLanguageMagicRegistration[]>({ extensionPoint: Extensions.NotebookLanguageMagicContribution, jsonSchema: languageMagicContrib }).setHandler(extensions => {
+ExtensionsRegistry.registerExtensionPoint<NotebookLanguageMagicRegistration[]>({ extensionPoint: Extensions.NotebookLanguageMagicContribution, jsonSchema: languageMagicContrib }).setHandler(extensions => {
 	for (let extension of extensions) {
 		const { value } = extension;
-		if (Array.isArray(value)) {
 		for (let command of value) {
 			notebookProviderRegistry.registerNotebookLanguageMagic(command);
 		}
-		} else {
-			notebookProviderRegistry.registerNotebookLanguageMagic(value);
-		}
 	}
 });