[index patterns] improve index pattern cache (elastic#83368)

* cache index pattern promise, not index pattern
mattkime · Nov 19, 2020 · f234131 · f234131
1 parent b3363fb
commit f234131
Show file tree

Hide file tree

Showing 3 changed files with 62 additions and 23 deletions.
diff --git a/src/plugins/data/common/index_patterns/index_patterns/_pattern_cache.ts b/src/plugins/data/common/index_patterns/index_patterns/_pattern_cache.ts
@@ -20,8 +20,8 @@
 import { IndexPattern } from './index_pattern';
 
 export interface PatternCache {
-  get: (id: string) => IndexPattern;
-  set: (id: string, value: IndexPattern) => IndexPattern;
+  get: (id: string) => Promise<IndexPattern> | undefined;
+  set: (id: string, value: Promise<IndexPattern>) => Promise<IndexPattern>;
   clear: (id: string) => void;
   clearAll: () => void;
 }

diff --git a/src/plugins/data/common/index_patterns/index_patterns/index_patterns.test.ts b/src/plugins/data/common/index_patterns/index_patterns/index_patterns.test.ts
@@ -40,6 +40,7 @@ function setDocsourcePayload(id: string | null, providedPayload: any) {
 describe('IndexPatterns', () => {
   let indexPatterns: IndexPatternsService;
   let savedObjectsClient: SavedObjectsClientCommon;
+  let SOClientGetDelay = 0;
 
   beforeEach(() => {
     const indexPatternObj = { id: 'id', version: 'a', attributes: { title: 'title' } };
@@ -49,11 +50,14 @@ describe('IndexPatterns', () => {
     );
     savedObjectsClient.delete = jest.fn(() => Promise.resolve({}) as Promise<any>);
     savedObjectsClient.create = jest.fn();
-    savedObjectsClient.get = jest.fn().mockImplementation(async (type, id) => ({
-      id: object.id,
-      version: object.version,
-      attributes: object.attributes,
-    }));
+    savedObjectsClient.get = jest.fn().mockImplementation(async (type, id) => {
+      await new Promise((resolve) => setTimeout(resolve, SOClientGetDelay));
+      return {
+        id: object.id,
+        version: object.version,
+        attributes: object.attributes,
+      };
+    });
     savedObjectsClient.update = jest
       .fn()
       .mockImplementation(async (type, id, body, { version }) => {
@@ -88,6 +92,7 @@ describe('IndexPatterns', () => {
   });
 
   test('does cache gets for the same id', async () => {
+    SOClientGetDelay = 1000;
     const id = '1';
     setDocsourcePayload(id, {
       id: 'foo',
@@ -97,10 +102,17 @@ describe('IndexPatterns', () => {
       },
     });
 
-    const indexPattern = await indexPatterns.get(id);
+    // make two requests before first can complete
+    const indexPatternPromise = indexPatterns.get(id);
+    indexPatterns.get(id);
 
-    expect(indexPattern).toBeDefined();
-    expect(indexPattern).toBe(await indexPatterns.get(id));
+    indexPatternPromise.then((indexPattern) => {
+      expect(savedObjectsClient.get).toBeCalledTimes(1);
+      expect(indexPattern).toBeDefined();
+    });
+
+    expect(await indexPatternPromise).toBe(await indexPatterns.get(id));
+    SOClientGetDelay = 0;
   });
 
   test('savedObjectCache pre-fetches only title', async () => {
@@ -212,4 +224,25 @@ describe('IndexPatterns', () => {
 
     expect(indexPatterns.savedObjectToSpec(savedObject)).toMatchSnapshot();
   });
+
+  test('failed requests are not cached', async () => {
+    savedObjectsClient.get = jest
+      .fn()
+      .mockImplementation(async (type, id) => {
+        return {
+          id: object.id,
+          version: object.version,
+          attributes: object.attributes,
+        };
+      })
+      .mockRejectedValueOnce({});
+
+    const id = '1';
+
+    // failed request!
+    expect(indexPatterns.get(id)).rejects.toBeDefined();
+
+    // successful subsequent request
+    expect(async () => await indexPatterns.get(id)).toBeDefined();
+  });
 });
diff --git a/src/plugins/data/common/index_patterns/index_patterns/index_patterns.ts b/src/plugins/data/common/index_patterns/index_patterns/index_patterns.ts
@@ -361,17 +361,7 @@ export class IndexPatternsService {
     };
   };
 
-  /**
-   * Get an index pattern by id. Cache optimized
-   * @param id
-   */
-
-  get = async (id: string): Promise<IndexPattern> => {
-    const cache = indexPatternCache.get(id);
-    if (cache) {
-      return cache;
-    }
-
+  private getSavedObjectAndInit = async (id: string): Promise<IndexPattern> => {
     const savedObject = await this.savedObjectsClient.get<IndexPatternAttributes>(
       savedObjectType,
       id
@@ -427,7 +417,6 @@ export class IndexPatternsService {
       : {};
 
     const indexPattern = await this.create(spec, true);
-    indexPatternCache.set(id, indexPattern);
     if (isSaveRequired) {
       try {
         this.updateSavedObject(indexPattern);
@@ -477,6 +466,23 @@ export class IndexPatternsService {
       .then(() => this);
   }
 
+  /**
+   * Get an index pattern by id. Cache optimized
+   * @param id
+   */
+
+  get = async (id: string): Promise<IndexPattern> => {
+    const indexPatternPromise =
+      indexPatternCache.get(id) || indexPatternCache.set(id, this.getSavedObjectAndInit(id));
+
+    // don't cache failed requests
+    indexPatternPromise.catch(() => {
+      indexPatternCache.clear(id);
+    });
+
+    return indexPatternPromise;
+  };
+
   /**
    * Create a new index pattern instance
    * @param spec
@@ -535,7 +541,7 @@ export class IndexPatternsService {
       id: indexPattern.id,
     });
     indexPattern.id = response.id;
-    indexPatternCache.set(indexPattern.id, indexPattern);
+    indexPatternCache.set(indexPattern.id, Promise.resolve(indexPattern));
     return indexPattern;
   }