-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat(query-enhancements): PPL lint backend — feature flag + explain/calcite proxy routes #12255
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
Changes from 1 commit
9191739
4ac79ae
031498a
10285e6
b50e6c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,143 @@ | ||
| /* | ||
| * Copyright OpenSearch Contributors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { coreMock, dynamicConfigServiceMock } from '../../../core/server/mocks'; | ||
| import { dataPluginMock } from '../../data/server/mocks'; | ||
| import { QueryEnhancementsPlugin } from './plugin'; | ||
|
|
||
| describe('QueryEnhancementsPlugin pplLint capability', () => { | ||
| const baseCapabilities = () => | ||
| ({ | ||
| navLinks: {}, | ||
| management: {}, | ||
| catalogue: {}, | ||
| queryEnhancements: { pplLint: false }, | ||
| } as any); | ||
|
|
||
| // Run the real setup() against core mocks and hand back the registered | ||
| // capability switcher plus the plugin's logger so the catch path is | ||
| // observable. No production change is needed — the switcher is captured from | ||
| // the registerSwitcher mock. | ||
| const setupPlugin = () => { | ||
| const initializerContext = coreMock.createPluginInitializerContext(); | ||
| const plugin = new QueryEnhancementsPlugin(initializerContext); | ||
| const core = coreMock.createSetup(); | ||
| const deps = { data: dataPluginMock.createSetupContract() } as any; | ||
|
|
||
| plugin.setup(core, deps); | ||
|
|
||
| const registerSwitcher = core.capabilities.registerSwitcher as jest.Mock; | ||
| const switcher = registerSwitcher.mock.calls[0][0]; | ||
| // this.logger = initializerContext.logger.get(); the loggingSystem mock | ||
| // shares one logger instance, so this is the same logger the plugin holds. | ||
| const logger = initializerContext.logger.get(); | ||
| return { core, switcher, logger }; | ||
| }; | ||
|
|
||
| const stubConfig = ( | ||
| core: any, | ||
| getConfig: Record<string, any>, | ||
| asyncLocalStore?: Map<string, any> | ||
| ) => { | ||
| const startContract = dynamicConfigServiceMock.createStartContract( | ||
| { | ||
| getConfig, | ||
| bulkGetConfigs: new Map(), | ||
| listConfigs: new Map(), | ||
| }, | ||
| asyncLocalStore | ||
| ); | ||
| core.dynamicConfigService.getStartService.mockResolvedValue(startContract); | ||
| return startContract; | ||
| }; | ||
|
|
||
| it('registers a disabled-by-default pplLint capability provider', () => { | ||
| const { core } = setupPlugin(); | ||
| const registerProvider = core.capabilities.registerProvider as jest.Mock; | ||
| const provided = registerProvider.mock.calls[0][0](); | ||
| expect(provided).toEqual({ queryEnhancements: { pplLint: false } }); | ||
| }); | ||
|
|
||
| it('enables pplLint when the dynamic config flag is on', async () => { | ||
| const { core, switcher } = setupPlugin(); | ||
| const startContract = stubConfig(core, { pplLint: { enabled: true } }); | ||
|
|
||
| const result = await switcher({} as any, baseCapabilities()); | ||
|
|
||
| expect(result.queryEnhancements.pplLint).toBe(true); | ||
| // Guard the documented footgun: the lookup MUST use pluginConfigPath, not | ||
| // { name: 'queryEnhancements' } (which snake-cases to the wrong namespace, | ||
| // throws, gets swallowed, and disables pplLint forever). The shared mock | ||
| // returns the stub for any argument, so without this assertion a regression | ||
| // to { name } would pass every test. | ||
| const getConfigMock = startContract.getClient().getConfig as jest.Mock; | ||
| expect(getConfigMock).toHaveBeenCalledTimes(1); | ||
| expect(getConfigMock.mock.calls[0][0]).toEqual({ pluginConfigPath: ['queryEnhancements'] }); | ||
| }); | ||
|
|
||
| it('passes the async local store as context when one is present', async () => { | ||
| const { core, switcher } = setupPlugin(); | ||
| const store = new Map<string, any>([['k', 'v']]); | ||
| const startContract = stubConfig(core, { pplLint: { enabled: true } }, store); | ||
|
|
||
| await switcher({} as any, baseCapabilities()); | ||
|
|
||
| const getConfigMock = startContract.getClient().getConfig as jest.Mock; | ||
| expect(getConfigMock.mock.calls[0][1]).toEqual({ asyncLocalStorageContext: store }); | ||
| }); | ||
|
|
||
| it('omits the options object when no async local store is present', async () => { | ||
| const { core, switcher } = setupPlugin(); | ||
| // No store passed → getAsyncLocalStore() returns undefined. The switcher | ||
| // must pass `undefined` rather than { asyncLocalStorageContext: undefined }. | ||
| const startContract = stubConfig(core, { pplLint: { enabled: true } }); | ||
|
|
||
| await switcher({} as any, baseCapabilities()); | ||
|
|
||
| const getConfigMock = startContract.getClient().getConfig as jest.Mock; | ||
| expect(getConfigMock.mock.calls[0][1]).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('coerces a non-boolean stored flag to false (unvalidated dynamic config)', async () => { | ||
| const { core, switcher } = setupPlugin(); | ||
| // Dynamic config writes are not schema-validated, so the store can hold a | ||
| // string. Only a real boolean `true` may enable the flag. | ||
| stubConfig(core, { pplLint: { enabled: 'true' } } as any); | ||
|
|
||
| const result = await switcher({} as any, baseCapabilities()); | ||
|
|
||
| expect(result.queryEnhancements.pplLint).toBe(false); | ||
| }); | ||
|
|
||
| it('leaves pplLint off when the dynamic config flag is false', async () => { | ||
| const { core, switcher } = setupPlugin(); | ||
| stubConfig(core, { pplLint: { enabled: false } }); | ||
|
|
||
| const result = await switcher({} as any, baseCapabilities()); | ||
|
|
||
| expect(result.queryEnhancements.pplLint).toBe(false); | ||
| }); | ||
|
|
||
| it('leaves pplLint off when the dynamic config is absent', async () => { | ||
| const { core, switcher } = setupPlugin(); | ||
| stubConfig(core, {}); | ||
|
|
||
| const result = await switcher({} as any, baseCapabilities()); | ||
|
|
||
| expect(result.queryEnhancements.pplLint).toBe(false); | ||
| }); | ||
|
|
||
| it('returns capabilities unchanged and logs when loading dynamic config throws', async () => { | ||
| const { core, switcher, logger } = setupPlugin(); | ||
| core.dynamicConfigService.getStartService.mockRejectedValue(new Error('no config store')); | ||
|
|
||
| const capabilities = baseCapabilities(); | ||
| const result = await switcher({} as any, capabilities); | ||
|
|
||
| expect(result).toBe(capabilities); | ||
| expect(result.queryEnhancements.pplLint).toBe(false); | ||
| expect(logger.error).toHaveBeenCalled(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,8 @@ import { | |
| IOpenSearchDashboardsResponse, | ||
| IRouter, | ||
| Logger, | ||
| OpenSearchClient, | ||
| RequestHandlerContext, | ||
| ResponseError, | ||
| } from '../../../../core/server'; | ||
| import { IDataFrameResponse, IOpenSearchDashboardsSearchRequest } from '../../../data/common'; | ||
|
|
@@ -17,16 +19,40 @@ import { registerQueryAssistRoutes } from './query_assist'; | |
| import { registerDataSourceConnectionsRoutes } from './data_source_connection'; | ||
| import { registerResourceRoutes } from './resources'; | ||
| import { registerPPLCancelRoute } from './ppl_cancel'; | ||
| import { definePPLCalciteSettingsRoute } from './ppl_calcite_settings'; | ||
| import { definePPLExplainRoute } from './ppl_explain'; | ||
|
|
||
| /** | ||
| * Coerce status code to 503 for 500 errors from dependency services. Only use | ||
| * this function to handle errors throw by other services, and not from OSD. | ||
| */ | ||
| export const coerceStatusCode = (statusCode: number) => { | ||
| export const coerceStatusCode = (statusCode?: number) => { | ||
| if (statusCode === 500) return 503; | ||
| return statusCode || 503; | ||
| }; | ||
|
|
||
| export const DATASOURCE_UNAVAILABLE_MESSAGE = | ||
| 'dataSourceId is not supported because data source plugin is unavailable'; | ||
|
|
||
| /** | ||
| * Resolves the OpenSearch client for an optional dataSourceId. Returns the | ||
| * data source's client when a dataSourceId is given, the current-user client | ||
| * otherwise, or `null` when a dataSourceId is requested but the data source | ||
| * plugin is unavailable (the caller should respond 400 in that case). | ||
| */ | ||
| export async function resolveOpenSearchClient( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. p1: add a direct multi-data-source test for resolveOpenSearchClientThis helper is the seam for the multi-data-source requirement in the parent RFC (sql#5405 section 2.9 plumbs Suggested coverage in a dedicated unit test for this helper:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a describe('resolveOpenSearchClient') to index.test.ts covering: two distinct dataSourceIds resolve to distinct clients (not just id-forwarding), no id resolves to asCurrentUser, and a dataSourceId with context.dataSource absent resolves to null. |
||
| context: RequestHandlerContext, | ||
| dataSourceId?: string | ||
| ): Promise<OpenSearchClient | null> { | ||
| if (dataSourceId) { | ||
| if (!context.dataSource?.opensearch?.getClient) { | ||
| return null; | ||
| } | ||
| return context.dataSource.opensearch.getClient(dataSourceId); | ||
| } | ||
| return context.core.opensearch.client.asCurrentUser; | ||
| } | ||
|
|
||
| /** | ||
| * @experimental | ||
| * | ||
|
|
@@ -192,4 +218,6 @@ export function defineRoutes( | |
| registerPPLCancelRoute(router, logger); | ||
|
|
||
| definePPLBundleRoute(logger, router); | ||
| definePPLCalciteSettingsRoute(logger, router); | ||
| definePPLExplainRoute(logger, router); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be
ppl.lint.enabled, orlintEnabled: ["ppl"]? i'm thinking if we add more languages in the future, can the config path be more structured?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call — restructured to queryEnhancements.ppl.lint.enabled in 031498a (config schema now nests ppl: { lint: { enabled } }) so that future languages/features extend the same shape.