From ed8d717aabe1b4c0f3be93dabe32c1d64f4adf8f Mon Sep 17 00:00:00 2001 From: Ashwin P Chandran Date: Sun, 3 Nov 2024 12:37:29 +0000 Subject: [PATCH 1/4] [Discover] Makes caching dataset options optional. --- .../dataset_service/dataset_service.ts | 13 ++++++++----- .../query_string/dataset_service/types.ts | 2 ++ .../public/datasets/s3_type.ts | 1 + .../query_assist/utils/create_extension.tsx | 18 +++++++++++++----- 4 files changed, 24 insertions(+), 10 deletions(-) diff --git a/src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts b/src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts index 55d77c2ccc36..96523ff82894 100644 --- a/src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts +++ b/src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts @@ -161,14 +161,17 @@ export class DatasetService { .join('&'); const cacheKey = `${dataType}.${lastPathItem.id}` + (fetchOptionsKey.length ? `?${fetchOptionsKey}` : ''); - - const cachedDataStructure = this.sessionStorage.get(cacheKey); - if (cachedDataStructure?.children?.length > 0) { - return this.cacheToDataStructure(dataType, cachedDataStructure); + if (type.meta.cacheOptions) { + const cachedDataStructure = this.sessionStorage.get(cacheKey); + if (cachedDataStructure?.children?.length > 0) { + return this.cacheToDataStructure(dataType, cachedDataStructure); + } } const fetchedDataStructure = await type.fetch(services, path, options); - this.cacheDataStructure(dataType, fetchedDataStructure); + if (type.meta.cacheOptions) { + this.cacheDataStructure(dataType, fetchedDataStructure); + } return fetchedDataStructure; } diff --git a/src/plugins/data/public/query/query_string/dataset_service/types.ts b/src/plugins/data/public/query/query_string/dataset_service/types.ts index 020bc369ff5e..f303fa6af56d 100644 --- a/src/plugins/data/public/query/query_string/dataset_service/types.ts +++ b/src/plugins/data/public/query/query_string/dataset_service/types.ts @@ -36,6 +36,8 @@ export interface DatasetTypeConfig { supportsTimeFilter?: boolean; /** Optional isFieldLoadAsync determines if field loads are async */ isFieldLoadAsync?: boolean; + /** Optional cacheOptions determines if the data structure is cacheable. Defaults to false */ + cacheOptions?: boolean; }; /** * Converts a DataStructure to a Dataset. diff --git a/src/plugins/query_enhancements/public/datasets/s3_type.ts b/src/plugins/query_enhancements/public/datasets/s3_type.ts index 90660289c931..09aeef434e24 100644 --- a/src/plugins/query_enhancements/public/datasets/s3_type.ts +++ b/src/plugins/query_enhancements/public/datasets/s3_type.ts @@ -36,6 +36,7 @@ export const s3TypeConfig: DatasetTypeConfig = { searchOnLoad: true, supportsTimeFilter: false, isFieldLoadAsync: true, + cacheOptions: true, }, toDataset: (path: DataStructure[]): Dataset => { diff --git a/src/plugins/query_enhancements/public/query_assist/utils/create_extension.tsx b/src/plugins/query_enhancements/public/query_assist/utils/create_extension.tsx index 3c6532838d52..238ef40ff354 100644 --- a/src/plugins/query_enhancements/public/query_assist/utils/create_extension.tsx +++ b/src/plugins/query_enhancements/public/query_assist/utils/create_extension.tsx @@ -32,20 +32,28 @@ const [getAvailableLanguagesForDataSource, clearCache] = (() => { const pendingRequests: Map> = new Map(); return [ - async (http: HttpSetup, dataSourceId: string | undefined) => { + async (http: HttpSetup, dataSourceId: string | undefined, timeout?: number) => { const cached = availableLanguagesByDataSource.get(dataSourceId); if (cached !== undefined) return cached; const pendingRequest = pendingRequests.get(dataSourceId); if (pendingRequest !== undefined) return pendingRequest; + const controller = timeout ? new AbortController() : undefined; + const timeoutId = timeout ? setTimeout(() => controller?.abort(), timeout) : undefined; + const languagesPromise = http .get<{ configuredLanguages: string[] }>(API.QUERY_ASSIST.LANGUAGES, { query: { dataSourceId }, + signal: controller?.signal, }) .then((response) => response.configuredLanguages) .catch(() => []) - .finally(() => pendingRequests.delete(dataSourceId)); + .finally(() => { + pendingRequests.delete(dataSourceId); + if (timeoutId) clearTimeout(timeoutId); + }); + pendingRequests.set(dataSourceId, languagesPromise); const languages = await languagesPromise; @@ -98,9 +106,9 @@ export const createQueryAssistExtension = ( id: 'query-assist', order: 1000, getDataStructureMeta: async (dataSourceId) => { - const isEnabled = await getAvailableLanguagesForDataSource(http, dataSourceId).then( - (languages) => languages.length > 0 - ); + // [TODO] - The timmeout exists because the loading of the Datasource menu is prevented until this request completes. This if a single cluster is down the request holds the whole menu level in a loading state. We should make this call non blocking and load the datasource meta in the background. + const isEnabled = await getAvailableLanguagesForDataSource(http, dataSourceId, 3000) // 3s timeout for quick check + .then((languages) => languages.length > 0); if (isEnabled) { return { type: DATA_STRUCTURE_META_TYPES.FEATURE, From 633344675e12975b18432f9448d9994c61429f5c Mon Sep 17 00:00:00 2001 From: Ashwin P Chandran Date: Sun, 3 Nov 2024 13:41:22 +0000 Subject: [PATCH 2/4] Adds unit test --- .../dataset_service/dataset_service.test.ts | 62 +++++++++++++++++-- .../dataset_service/dataset_service.ts | 2 +- 2 files changed, 58 insertions(+), 6 deletions(-) diff --git a/src/plugins/data/public/query/query_string/dataset_service/dataset_service.test.ts b/src/plugins/data/public/query/query_string/dataset_service/dataset_service.test.ts index 43dcc78e9bc3..64b99527ea1a 100644 --- a/src/plugins/data/public/query/query_string/dataset_service/dataset_service.test.ts +++ b/src/plugins/data/public/query/query_string/dataset_service/dataset_service.test.ts @@ -88,6 +88,51 @@ describe('DatasetService', () => { expect(mockType.fetch).toHaveBeenCalledTimes(2); }); + test('fetchOptions respects cacheOptions', async () => { + const mockDataStructure = { + id: 'root', + title: 'Test Structure', + type: 'test-type', + children: [ + { + id: 'child1', + title: 'Child 1', + type: 'test-type', + }, + ], + }; + + const fetchMock = jest.fn().mockResolvedValue(mockDataStructure); + + const mockTypeWithCache = { + id: 'test-type', + title: 'Test Type', + meta: { + icon: { type: 'test' }, + cacheOptions: true, + }, + fetch: fetchMock, + toDataset: jest.fn(), + fetchFields: jest.fn(), + supportedLanguages: jest.fn(), + }; + + service.registerType(mockTypeWithCache); + service.clearCache(); // Ensure clean state + + // First call should fetch + const result = await service.fetchOptions(mockDataPluginServices, mockPath, 'test-type'); + expect(result).toMatchObject(mockDataStructure); + + // Clear fetch mock call count + fetchMock.mockClear(); + + // Second call should use cache + const cachedResult = await service.fetchOptions(mockDataPluginServices, mockPath, 'test-type'); + expect(cachedResult).toMatchObject(mockDataStructure); + expect(fetchMock).not.toHaveBeenCalled(); + }); + test('clear cache', async () => { service.registerType(mockType); @@ -99,16 +144,23 @@ describe('DatasetService', () => { }); test('caching object correctly sets last cache time', async () => { - service.registerType(mockType); - const time = Date.now(); - Date.now = jest.fn(() => time); + const mockTypeWithCache = { + ...mockType, + meta: { + ...mockType.meta, + cacheOptions: true, + }, + }; + + service.registerType(mockTypeWithCache); await service.fetchOptions(mockDataPluginServices, mockPath, 'test-type'); expect(service.getLastCacheTime()).toEqual(time); }); + test('calling cacheDataset on dataset caches it', async () => { const mockDataset = { id: 'test-dataset', @@ -117,7 +169,7 @@ describe('DatasetService', () => { } as Dataset; service.registerType(mockType); - await service.cacheDataset(mockDataset); + await service.cacheDataset(mockDataset, mockDataPluginServices); expect(indexPatterns.create).toHaveBeenCalledTimes(1); expect(indexPatterns.saveToCache).toHaveBeenCalledTimes(1); }); @@ -130,7 +182,7 @@ describe('DatasetService', () => { type: DEFAULT_DATA.SET_TYPES.INDEX_PATTERN, } as Dataset; - await service.cacheDataset(mockDataset); + await service.cacheDataset(mockDataset, mockDataPluginServices); expect(indexPatterns.create).toHaveBeenCalledTimes(0); expect(indexPatterns.saveToCache).toHaveBeenCalledTimes(0); }); diff --git a/src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts b/src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts index 96523ff82894..641e0a7a3334 100644 --- a/src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts +++ b/src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts @@ -275,7 +275,7 @@ export class DatasetService { ? { id: dataSource.id, title: dataSource.attributes?.title, - type: dataSource.attributes?.dataSourceEngineType, + type: dataSource.attributes?.dataSourceEngineType || '', } : undefined, }, From 93ddd58a1b8bbba9d63f68578fb4fdf8f5d86b58 Mon Sep 17 00:00:00 2001 From: "opensearch-changeset-bot[bot]" <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Date: Sun, 3 Nov 2024 13:50:35 +0000 Subject: [PATCH 3/4] Changeset file for PR #8799 created/updated --- changelogs/fragments/8799.yml | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 changelogs/fragments/8799.yml diff --git a/changelogs/fragments/8799.yml b/changelogs/fragments/8799.yml new file mode 100644 index 000000000000..7e9798c96321 --- /dev/null +++ b/changelogs/fragments/8799.yml @@ -0,0 +1,2 @@ +fix: +- [Discover] Makes cachign dataset options optional and configurable by the dataset type ([#8799](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8799)) \ No newline at end of file From 707527bb25c73a1b8f1384c23a489cfcd1cdb3d4 Mon Sep 17 00:00:00 2001 From: Kawika Avilla Date: Mon, 4 Nov 2024 02:24:16 +0000 Subject: [PATCH 4/4] Update test Signed-off-by: Kawika Avilla --- .../public/query_assist/utils/create_extension.test.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/src/plugins/query_enhancements/public/query_assist/utils/create_extension.test.tsx b/src/plugins/query_enhancements/public/query_assist/utils/create_extension.test.tsx index cc0c695e8fac..4b0a3b215db3 100644 --- a/src/plugins/query_enhancements/public/query_assist/utils/create_extension.test.tsx +++ b/src/plugins/query_enhancements/public/query_assist/utils/create_extension.test.tsx @@ -100,6 +100,7 @@ describe('CreateExtension', () => { `); expect(httpMock.get).toBeCalledWith('/api/enhancements/assist/languages', { query: { dataSourceId: 'mock-data-source-id2' }, + signal: new AbortController().signal, }); });