Address feedback on createPointInTimeFinder.

Author: lukeelmers

src/core/server/saved_objects/export/find_with_point_in_time.test.ts renamed to src/core/server/saved_objects/export/point_in_time_finder.test.ts

@@ -11,8 +11,7 @@ import { loggerMock, MockedLogger } from '../../logging/logger.mock';
 import { SavedObjectsFindOptions } from '../types';
 import { SavedObjectsFindResult } from '../service';
 
-import type { FindWithPointInTime } from './find_with_point_in_time';
-import { findWithPointInTime } from './find_with_point_in_time';
+import { createPointInTimeFinder } from './point_in_time_finder';
 
 const mockHits = [
   {
@@ -39,18 +38,55 @@ const mockHits = [
   },
 ];
 
-describe('findWithPointInTime()', () => {
+describe('createPointInTimeFinder()', () => {
   let logger: MockedLogger;
   let savedObjectsClient: ReturnType<typeof savedObjectsClientMock.create>;
-  let finder: FindWithPointInTime;
 
   beforeEach(() => {
     logger = loggerMock.create();
     savedObjectsClient = savedObjectsClientMock.create();
-    finder = findWithPointInTime({ savedObjectsClient, logger });
   });
 
   describe('#find', () => {
+    test('throws if a PIT is already open', async () => {
+      savedObjectsClient.openPointInTimeForType.mockResolvedValueOnce({
+        id: 'abc123',
+      });
+      savedObjectsClient.find.mockResolvedValueOnce({
+        total: 2,
+        saved_objects: mockHits,
+        pit_id: 'abc123',
+        per_page: 1,
+        page: 0,
+      });
+      savedObjectsClient.find.mockResolvedValueOnce({
+        total: 2,
+        saved_objects: mockHits,
+        pit_id: 'abc123',
+        per_page: 1,
+        page: 1,
+      });
+
+      const findOptions: SavedObjectsFindOptions = {
+        type: ['visualization'],
+        search: 'foo*',
+        perPage: 1,
+      };
+
+      const finder = createPointInTimeFinder({ findOptions, logger, savedObjectsClient });
+      await finder.find().next();
+
+      expect(savedObjectsClient.find).toHaveBeenCalledTimes(1);
+      savedObjectsClient.find.mockClear();
+
+      expect(async () => {
+        await finder.find().next();
+      }).rejects.toThrowErrorMatchingInlineSnapshot(
+        `"Point In Time has already been opened for this finder instance. Please call \`close()\` before calling \`find()\` again."`
+      );
+      expect(savedObjectsClient.find).toHaveBeenCalledTimes(0);
+    });
+
     test('works with a single page of results', async () => {
       savedObjectsClient.openPointInTimeForType.mockResolvedValueOnce({
         id: 'abc123',
@@ -63,13 +99,14 @@ describe('findWithPointInTime()', () => {
         page: 0,
       });
 
-      const options: SavedObjectsFindOptions = {
+      const findOptions: SavedObjectsFindOptions = {
         type: ['visualization'],
         search: 'foo*',
       };
 
+      const finder = createPointInTimeFinder({ findOptions, logger, savedObjectsClient });
       const hits: SavedObjectsFindResult[] = [];
-      for await (const result of finder.find(options)) {
+      for await (const result of finder.find()) {
         hits.push(...result.saved_objects);
       }
 
@@ -113,14 +150,15 @@ describe('findWithPointInTime()', () => {
         page: 0,
       });
 
-      const options: SavedObjectsFindOptions = {
+      const findOptions: SavedObjectsFindOptions = {
         type: ['visualization'],
         search: 'foo*',
         perPage: 1,
       };
 
+      const finder = createPointInTimeFinder({ findOptions, logger, savedObjectsClient });
       const hits: SavedObjectsFindResult[] = [];
-      for await (const result of finder.find(options)) {
+      for await (const result of finder.find()) {
         hits.push(...result.saved_objects);
       }
 
@@ -154,14 +192,15 @@ describe('findWithPointInTime()', () => {
         page: 0,
       });
 
-      const options: SavedObjectsFindOptions = {
+      const findOptions: SavedObjectsFindOptions = {
         type: ['visualization'],
         search: 'foo*',
         perPage: 2,
       };
 
+      const finder = createPointInTimeFinder({ findOptions, logger, savedObjectsClient });
       const hits: SavedObjectsFindResult[] = [];
-      for await (const result of finder.find(options)) {
+      for await (const result of finder.find()) {
         hits.push(...result.saved_objects);
         await finder.close();
       }
@@ -195,14 +234,15 @@ describe('findWithPointInTime()', () => {
         page: 0,
       });
 
-      const options: SavedObjectsFindOptions = {
+      const findOptions: SavedObjectsFindOptions = {
         type: ['visualization'],
         search: 'foo*',
         perPage: 1,
       };
 
+      const finder = createPointInTimeFinder({ findOptions, logger, savedObjectsClient });
       const hits: SavedObjectsFindResult[] = [];
-      for await (const result of finder.find(options)) {
+      for await (const result of finder.find()) {
         hits.push(...result.saved_objects);
         await finder.close();
       }
@@ -217,15 +257,16 @@ describe('findWithPointInTime()', () => {
       });
       savedObjectsClient.find.mockRejectedValueOnce(new Error('oops'));
 
-      const options: SavedObjectsFindOptions = {
+      const findOptions: SavedObjectsFindOptions = {
         type: ['visualization'],
         search: 'foo*',
         perPage: 2,
       };
 
+      const finder = createPointInTimeFinder({ findOptions, logger, savedObjectsClient });
       const hits: SavedObjectsFindResult[] = [];
       try {
-        for await (const result of finder.find(options)) {
+        for await (const result of finder.find()) {
           hits.push(...result.saved_objects);
         }
       } catch (e) {
@@ -234,5 +275,47 @@ describe('findWithPointInTime()', () => {
 
       expect(savedObjectsClient.closePointInTime).toHaveBeenCalledWith('test');
     });
+
+    test('finder can be reused after closing', async () => {
+      savedObjectsClient.openPointInTimeForType.mockResolvedValueOnce({
+        id: 'abc123',
+      });
+      savedObjectsClient.find.mockResolvedValueOnce({
+        total: 2,
+        saved_objects: mockHits,
+        pit_id: 'abc123',
+        per_page: 1,
+        page: 0,
+      });
+      savedObjectsClient.find.mockResolvedValueOnce({
+        total: 2,
+        saved_objects: mockHits,
+        pit_id: 'abc123',
+        per_page: 1,
+        page: 1,
+      });
+
+      const findOptions: SavedObjectsFindOptions = {
+        type: ['visualization'],
+        search: 'foo*',
+        perPage: 1,
+      };
+
+      const finder = createPointInTimeFinder({ findOptions, logger, savedObjectsClient });
+
+      const findA = finder.find();
+      await findA.next();
+      await finder.close();
+
+      const findB = finder.find();
+      await findB.next();
+      await finder.close();
+
+      expect((await findA.next()).done).toBe(true);
+      expect((await findB.next()).done).toBe(true);
+      expect(savedObjectsClient.openPointInTimeForType).toHaveBeenCalledTimes(2);
+      expect(savedObjectsClient.find).toHaveBeenCalledTimes(2);
+      expect(savedObjectsClient.closePointInTime).toHaveBeenCalledTimes(2);
+    });
   });
 });

src/core/server/saved_objects/export/find_with_point_in_time.ts renamed to src/core/server/saved_objects/export/point_in_time_finder.ts

@@ -25,70 +25,75 @@ import { SavedObjectsFindResponse } from '../service';
  *
  * @example
  * ```ts
- * const finder = findWithPointInTime({
- *   logger,
- *   savedObjectsClient,
- * });
- *
- * const options: SavedObjectsFindOptions = {
+ * const findOptions: SavedObjectsFindOptions = {
  *   type: 'visualization',
  *   search: 'foo*',
  *   perPage: 100,
  * };
  *
+ * const finder = createPointInTimeFinder({
+ *   logger,
+ *   savedObjectsClient,
+ *   findOptions,
+ * });
+ *
  * const responses: SavedObjectFindResponse[] = [];
- * for await (const response of finder.find(options)) {
+ * for await (const response of finder.find()) {
  *   responses.push(...response);
  *   if (doneSearching) {
  *     await finder.close();
  *   }
  * }
  * ```
  */
-export function findWithPointInTime({
+export function createPointInTimeFinder({
+  findOptions,
   logger,
   savedObjectsClient,
 }: {
+  findOptions: SavedObjectsFindOptions;
   logger: Logger;
   savedObjectsClient: SavedObjectsClientContract;
 }) {
-  return new FindWithPointInTime({ logger, savedObjectsClient });
+  return new PointInTimeFinder({ findOptions, logger, savedObjectsClient });
 }
 
 /**
  * @internal
  */
-export class FindWithPointInTime {
+export class PointInTimeFinder {
   readonly #log: Logger;
   readonly #savedObjectsClient: SavedObjectsClientContract;
-  #open?: boolean;
-  #perPage?: number;
+  readonly #findOptions: SavedObjectsFindOptions;
+  #open: boolean = false;
   #pitId?: string;
-  #type?: string | string[];
 
   constructor({
-    savedObjectsClient,
+    findOptions,
     logger,
+    savedObjectsClient,
   }: {
-    savedObjectsClient: SavedObjectsClientContract;
+    findOptions: SavedObjectsFindOptions;
     logger: Logger;
+    savedObjectsClient: SavedObjectsClientContract;
   }) {
     this.#log = logger;
     this.#savedObjectsClient = savedObjectsClient;
+    this.#findOptions = {
+      // Default to 1000 items per page as a tradeoff between
+      // speed and memory consumption.
+      perPage: 1000,
+      ...findOptions,
+    };
   }
 
-  async *find(options: SavedObjectsFindOptions) {
-    this.#open = true;
-    this.#type = options.type;
-    // Default to 1000 items per page as a tradeoff between
-    // speed and memory consumption.
-    this.#perPage = options.perPage ?? 1000;
-
-    const findOptions: SavedObjectsFindOptions = {
-      ...options,
-      perPage: this.#perPage,
-      type: this.#type,
-    };
+  async *find() {
+    if (this.#open) {
+      throw new Error(
+        'Point In Time has already been opened for this finder instance. ' +
+          'Please call `close()` before calling `find()` again.'
+      );
+    }
 
     // Open PIT and request our first page of hits
     await this.open();
@@ -97,7 +102,7 @@ export class FindWithPointInTime {
     let lastHitSortValue: unknown[] | undefined;
     do {
       const results = await this.findNext({
-        findOptions,
+        findOptions: this.#findOptions,
         id: this.#pitId,
         ...(lastHitSortValue ? { searchAfter: lastHitSortValue } : {}),
       });
@@ -108,44 +113,44 @@ export class FindWithPointInTime {
       this.#log.debug(`Collected [${lastResultsCount}] saved objects for export.`);
 
       // Close PIT if this was our last page
-      if (this.#pitId && lastResultsCount < this.#perPage) {
+      if (this.#pitId && lastResultsCount < this.#findOptions.perPage!) {
         await this.close();
       }
 
       yield results;
       // We've reached the end when there are fewer hits than our perPage size,
       // or when `close()` has been called.
-    } while (this.#open && lastHitSortValue && lastResultsCount >= this.#perPage);
+    } while (this.#open && lastHitSortValue && lastResultsCount >= this.#findOptions.perPage!);
 
     return;
   }
 
   async close() {
     try {
       if (this.#pitId) {
-        this.#log.debug(`Closing PIT for types [${this.#type}]`);
+        this.#log.debug(`Closing PIT for types [${this.#findOptions.type}]`);
         await this.#savedObjectsClient.closePointInTime(this.#pitId);
         this.#pitId = undefined;
       }
-      this.#type = undefined;
       this.#open = false;
     } catch (e) {
-      this.#log.error(`Failed to close PIT for types [${this.#type}]`);
+      this.#log.error(`Failed to close PIT for types [${this.#findOptions.type}]`);
       throw e;
     }
   }
 
   private async open() {
     try {
-      const { id } = await this.#savedObjectsClient.openPointInTimeForType(this.#type!);
+      const { id } = await this.#savedObjectsClient.openPointInTimeForType(this.#findOptions.type);
       this.#pitId = id;
+      this.#open = true;
     } catch (e) {
       // Since `find` swallows 404s, it is expected that exporter will do the same,
       // so we only rethrow non-404 errors here.
       if (e.output.statusCode !== 404) {
         throw e;
       }
-      this.#log.debug(`Unable to open PIT for types [${this.#type}]: 404 ${e}`);
+      this.#log.debug(`Unable to open PIT for types [${this.#findOptions.type}]: 404 ${e}`);
     }
   }