From 329ea4103cfeaefd19693431053f993036d792ea Mon Sep 17 00:00:00 2001 From: Cory Rivera Date: Tue, 9 Nov 2021 16:00:34 -0800 Subject: [PATCH] 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 | 4 +- .../test/browser/notebookService.test.ts | 8 +- .../electron-browser/notebookUtils.test.ts | 24 +++- .../notebook/browser/models/notebookModel.ts | 6 +- .../notebook/browser/models/notebookUtils.ts | 6 +- .../notebook/browser/notebookService.ts | 4 +- .../notebook/browser/notebookServiceImpl.ts | 40 +++--- .../notebook/common/notebookRegistry.ts | 117 +++++------------- 8 files changed, 83 insertions(+), 126 deletions(-) diff --git a/extensions/notebook/package.json b/extensions/notebook/package.json index 812c9669ce..500a532db4 100644 --- a/extensions/notebook/package.json +++ b/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", diff --git a/src/sql/workbench/contrib/notebook/test/browser/notebookService.test.ts b/src/sql/workbench/contrib/notebook/test/browser/notebookService.test.ts index 5f14a295e3..1f513cfa98 100644 --- a/src/sql/workbench/contrib/notebook/test/browser/notebookService.test.ts +++ b/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', - standardKernels: { + fileExtensions: ['ipynb'], + 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 () => { diff --git a/src/sql/workbench/contrib/notebook/test/electron-browser/notebookUtils.test.ts b/src/sql/workbench/contrib/notebook/test/electron-browser/notebookUtils.test.ts index 0af7e61712..cd38cb81db 100644 --- a/src/sql/workbench/contrib/notebook/test/electron-browser/notebookUtils.test.ts +++ b/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(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())) - .returns((provider) => { + let returnHandler = (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 { @@ -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, [{ name: 'testName', diff --git a/src/sql/workbench/services/notebook/browser/models/notebookModel.ts b/src/sql/workbench/services/notebook/browser/models/notebookModel.ts index e55c49b398..c099f6f2c6 100644 --- a/src/sql/workbench/services/notebook/browser/models/notebookModel.ts +++ b/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, - // 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); + manager = this.executeManagers.find(manager => manager.providerId === SQL_NOTEBOOK_PROVIDER); } return manager; } diff --git a/src/sql/workbench/services/notebook/browser/models/notebookUtils.ts b/src/sql/workbench/services/notebook/browser/models/notebookUtils.ts index 09f2d274c0..d87aaa0a3f 100644 --- a/src/sql/workbench/services/notebook/browser/models/notebookUtils.ts +++ b/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(kernel, { name: kernel.name, diff --git a/src/sql/workbench/services/notebook/browser/notebookService.ts b/src/sql/workbench/services/notebook/browser/notebookService.ts index 408558566c..417df773a5 100644 --- a/src/sql/workbench/services/notebook/browser/notebookService.ts +++ b/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; diff --git a/src/sql/workbench/services/notebook/browser/notebookServiceImpl.ts b/src/sql/workbench/services/notebook/browser/notebookServiceImpl.ts index ab2f680fe3..6a9567f918 100644 --- a/src/sql/workbench/services/notebook/browser/notebookServiceImpl.ts +++ b/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 = Array.isArray(extensions) ? extensions.some(ext => isNewFileType(ext)) : isNewFileType(extensions); + let addNewProvider = extensions.some(ext => ext?.length > 0 && ext.toUpperCase() !== DEFAULT_NOTEBOOK_FILETYPE); 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 { - this.addFileProvider(extensions, registration); + for (let fileType of extensions) { + this.addFileProvider(fileType, registration); } } - if (registration.standardKernels) { + if (registration.standardKernels?.length > 0) { 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); - } + provider.standardKernels.forEach(kernel => { + standardKernels.push(kernel); + }); // 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[] { - fileType = fileType.toUpperCase(); - let providers = this._fileToProviderDescriptions.get(fileType); - - return providers ? providers.map(provider => provider.provider) : undefined; + getProvidersForFileType(fileType: string): string[] | undefined { + let providers = this._fileToProviderDescriptions.get(fileType.toUpperCase()); + return providers?.map(provider => provider.provider); } - 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, - standardKernels: { name: notebookConstants.SQL, displayName: notebookConstants.SQL, connectionProviderIds: [notebookConstants.SQL_CONNECTION_PROVIDER] } + fileExtensions: [DEFAULT_NOTEBOOK_FILETYPE], + standardKernels: [{ name: notebookConstants.SQL, displayName: notebookConstants.SQL, connectionProviderIds: [notebookConstants.SQL_CONNECTION_PROVIDER] }] }); } diff --git a/src/sql/workbench/services/notebook/common/notebookRegistry.ts b/src/sql/workbench/services/notebook/common/notebookRegistry.ts index 909e9ad1b9..9778fe9118 100644 --- a/src/sql/workbench/services/notebook/common/notebookRegistry.ts +++ b/src/sql/workbench/services/notebook/common/notebookRegistry.ts @@ -19,8 +19,8 @@ export const Extensions = { export interface ProviderDescriptionRegistration { provider: string; - fileExtensions: string | string[]; - standardKernels: azdata.nb.IStandardKernel | azdata.nb.IStandardKernel[]; + fileExtensions: string[]; + standardKernels: azdata.nb.IStandardKernel[]; } let providerDescriptionType: IJSONSchema = { @@ -33,70 +33,39 @@ 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' - } - } - ] + type: 'array', + items: { + type: 'string' + } }, standardKernels: { description: localize('carbon.extension.contributes.notebook.standardKernels', "What kernels should be standard with this notebook provider"), - oneOf: [ - { - type: 'object', - properties: { - name: { - type: 'string', - }, - displayName: { - type: 'string', - }, - connectionProviderIds: { - type: 'array', - items: { - type: 'string' - } - } - } - }, - { - type: 'array', - items: { - type: 'object', + type: 'array', + items: { + type: 'object', + properties: { + name: { + type: 'string' + }, + displayName: { + type: 'string' + }, + connectionProviderIds: { + type: 'array', items: { - type: 'object', - properties: { - name: { - type: 'string', - }, - connectionProviderIds: { - type: 'array', - items: { - type: 'string' - } - } - } + type: 'string' } } } - ] + } } } }; let providerDescriptionContrib: IJSONSchema = { description: localize('vscode.extension.contributes.notebook.providersDescriptions', "Contributes notebook provider descriptions."), - oneOf: [ - providerDescriptionType, - { - type: 'array', - items: 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' - } - } - ] + type: 'array', + items: { + type: 'string' + } } } }; let languageMagicContrib: IJSONSchema = { description: localize('vscode.extension.contributes.notebook.languagemagics', "Contributes notebook language."), - oneOf: [ - notebookLanguageMagicType, - { - type: 'array', - items: 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({ extensionPoint: Extensions.NotebookProviderDescriptionContribution, jsonSchema: providerDescriptionContrib }).setHandler(extensions => { +ExtensionsRegistry.registerExtensionPoint({ 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); + for (let command of value) { + notebookProviderRegistry.registerProviderDescription(command); } } }); -ExtensionsRegistry.registerExtensionPoint({ extensionPoint: Extensions.NotebookLanguageMagicContribution, jsonSchema: languageMagicContrib }).setHandler(extensions => { +ExtensionsRegistry.registerExtensionPoint({ 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); + for (let command of value) { + notebookProviderRegistry.registerNotebookLanguageMagic(command); } } });