[Discover] Makes caching dataset options optional (#8799)

* [Discover] Makes caching dataset options optional. * Adds unit test * Changeset file for PR #8799 created/updated * Update test Signed-off-by: Kawika Avilla <[email protected]> --------- Signed-off-by: Kawika Avilla <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Co-authored-by: Kawika Avilla <[email protected]>
opensearch-project · Nov 4, 2024 · 005a83a · 005a83a
1 parent 5829303
commit 005a83a
Show file tree

Hide file tree

Showing 7 changed files with 85 additions and 16 deletions.
diff --git a/changelogs/fragments/8799.yml 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))
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
@@ -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
@@ -161,14 +161,17 @@ export class DatasetService {
       .join('&');
     const cacheKey =
       `${dataType}.${lastPathItem.id}` + (fetchOptionsKey.length ? `?${fetchOptionsKey}` : '');
-
-    const cachedDataStructure = this.sessionStorage.get<CachedDataStructure>(cacheKey);
-    if (cachedDataStructure?.children?.length > 0) {
-      return this.cacheToDataStructure(dataType, cachedDataStructure);
+    if (type.meta.cacheOptions) {
+      const cachedDataStructure = this.sessionStorage.get<CachedDataStructure>(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;
   }
 
@@ -272,7 +275,7 @@ export class DatasetService {
             ? {
                 id: dataSource.id,
                 title: dataSource.attributes?.title,
-                type: dataSource.attributes?.dataSourceEngineType,
+                type: dataSource.attributes?.dataSourceEngineType || '',
               }
             : undefined,
         },

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
@@ -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
@@ -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.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,
     });
   });
 

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
@@ -32,20 +32,28 @@ const [getAvailableLanguagesForDataSource, clearCache] = (() => {
   const pendingRequests: Map<string | undefined, Promise<string[]>> = 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,
Original file line number	Diff line number	Diff line change
		@@ -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))
-Original file line number
+Diff line change
@@ Expand Up / @@ -100,6 +100,7 @@ describe('CreateExtension', () => { @@
         `);
         expect(httpMock.get).toBeCalledWith('/api/enhancements/assist/languages', {
           query: { dataSourceId: 'mock-data-source-id2' },
+          signal: new AbortController().signal,
         });
       });
@@ Expand Down @@