review improvements

elastic · Jan 12, 2021 · 48c26ba · 48c26ba
1 parent dcaacca
commit 48c26ba
Show file tree

Hide file tree

Showing 13 changed files with 80 additions and 51 deletions.
diff --git a/src/plugins/data/public/public.api.md b/src/plugins/data/public/public.api.md
@@ -2620,7 +2620,7 @@ export const UI_SETTINGS: {
 // src/plugins/data/public/index.ts:433:1 - (ae-forgotten-export) The symbol "propFilter" needs to be exported by the entry point index.d.ts
 // src/plugins/data/public/index.ts:436:1 - (ae-forgotten-export) The symbol "toAbsoluteDates" needs to be exported by the entry point index.d.ts
 // src/plugins/data/public/query/state_sync/connect_to_query_state.ts:45:5 - (ae-forgotten-export) The symbol "FilterStateStore" needs to be exported by the entry point index.d.ts
-// src/plugins/data/public/search/session/session_service.ts:50:5 - (ae-forgotten-export) The symbol "UrlGeneratorStateMapping" needs to be exported by the entry point index.d.ts
+// src/plugins/data/public/search/session/session_service.ts:51:5 - (ae-forgotten-export) The symbol "UrlGeneratorStateMapping" needs to be exported by the entry point index.d.ts
 
 // (No @packageDocumentation comment for this package)
 

diff --git a/src/plugins/data/public/search/search_interceptor.ts b/src/plugins/data/public/search/search_interceptor.ts
@@ -130,16 +130,12 @@ export class SearchInterceptor {
   ): Promise<IKibanaSearchResponse> {
     const { abortSignal, ...requestOptions } = options || {};
 
-    const isCurrentSession =
-      options?.sessionId && this.deps.session.getSessionId() === options.sessionId;
-
     return this.batchedFetch(
       {
         request,
         options: {
           ...requestOptions,
-          isStored: isCurrentSession ? this.deps.session.isStored() : false,
-          isRestore: isCurrentSession ? this.deps.session.isRestore() : false,
+          ...this.deps.session.getSearchOptions(options?.sessionId),
         },
       },
       abortSignal

diff --git a/src/plugins/data/public/search/session/mocks.ts b/src/plugins/data/public/search/session/mocks.ts
@@ -49,5 +49,7 @@ export function getSessionServiceMock(): jest.Mocked<ISessionService> {
     isStored: jest.fn(),
     isRestore: jest.fn(),
     save: jest.fn(),
+    isCurrentSession: jest.fn(),
+    getSearchOptions: jest.fn(),
   };
 }
diff --git a/src/plugins/data/public/search/session/session_service.ts b/src/plugins/data/public/search/session/session_service.ts
@@ -29,6 +29,7 @@ import {
   SessionStateContainer,
 } from './search_session_state';
 import { ISessionsClient } from './sessions_client';
+import { ISearchOptions } from '../../../common';
 
 export type ISessionService = PublicContract<SessionService>;
 
@@ -243,4 +244,27 @@ export class SessionService {
       this.state.transitions.store();
     }
   }
+
+  /**
+   * Checks if passed sessionId is a current sessionId
+   * @param sessionId
+   */
+  public isCurrentSession(sessionId?: string): boolean {
+    return !!sessionId && this.getSessionId() === sessionId;
+  }
+
+  /**
+   * Infers search session options for sessionId using current session state
+   * @param sessionId
+   */
+  public getSearchOptions(
+    sessionId?: string
+  ): Pick<ISearchOptions, 'sessionId' | 'isRestore' | 'isStored'> {
+    const currentSessionId = this.getSessionId();
+    return {
+      sessionId,
+      isRestore: currentSessionId ? this.isRestore() : false,
+      isStored: currentSessionId ? this.isStored() : false,
+    };
+  }
 }
diff --git a/src/plugins/vis_type_timelion/public/helpers/timelion_request_handler.ts b/src/plugins/vis_type_timelion/public/helpers/timelion_request_handler.ts
@@ -96,10 +96,8 @@ export function getTimelionRequestHandler({
 
     // parse the time range client side to make sure it behaves like other charts
     const timeRangeBounds = timefilter.calculateBounds(timeRange);
-    const isCurrentSession = () =>
-      !!searchSessionId && searchSessionId === dataSearch.session.getSessionId();
     const untrackSearch =
-      isCurrentSession() &&
+      dataSearch.session.isCurrentSession(searchSessionId) &&
       dataSearch.session.trackSearch({
         abort: () => {
           // TODO: support search cancellations
@@ -121,9 +119,7 @@ export function getTimelionRequestHandler({
             interval: visParams.interval,
             timezone,
           },
-          sessionId: searchSessionId,
-          isRestore: isCurrentSession() ? dataSearch.session.isRestore() : false,
-          isStored: isCurrentSession() ? dataSearch.session.isStored() : false,
+          searchSession: dataSearch.session.getSearchOptions(searchSessionId),
         }),
       });
     } catch (e) {
@@ -139,7 +135,8 @@ export function getTimelionRequestHandler({
         throw e;
       }
     } finally {
-      if (untrackSearch && isCurrentSession()) {
+      if (untrackSearch && dataSearch.session.isCurrentSession(searchSessionId)) {
+        // call `untrack` if this search still belongs to current session
         untrackSearch();
       }
     }

diff --git a/src/plugins/vis_type_timelion/server/routes/run.ts b/src/plugins/vis_type_timelion/server/routes/run.ts
@@ -75,9 +75,13 @@ export function runRoute(
               to: schema.maybe(schema.string()),
             })
           ),
-          sessionId: schema.maybe(schema.string()),
-          isRestore: schema.maybe(schema.boolean()),
-          isStored: schema.maybe(schema.boolean()),
+          searchSession: schema.maybe(
+            schema.object({
+              sessionId: schema.maybe(schema.string()),
+              isRestore: schema.maybe(schema.boolean()),
+              isStored: schema.maybe(schema.boolean()),
+            })
+          ),
         }),
       },
     },

diff --git a/src/plugins/vis_type_timelion/server/series_functions/es/es.test.js b/src/plugins/vis_type_timelion/server/series_functions/es/es.test.js
@@ -65,9 +65,11 @@ describe('es', () => {
         ...stubRequestAndServer({ rawResponse: esResponse }),
         request: {
           body: {
-            sessionId: '1',
-            isRestore: true,
-            isStored: false,
+            searchSession: {
+              sessionId: '1',
+              isRestore: true,
+              isStored: false,
+            },
           },
         },
       };

diff --git a/src/plugins/vis_type_timelion/server/series_functions/es/index.js b/src/plugins/vis_type_timelion/server/series_functions/es/index.js
@@ -133,9 +133,7 @@ export default new Datasource('es', {
       .search(
         body,
         {
-          sessionId: tlConfig.request?.body.sessionId,
-          isRestore: tlConfig.request?.body.isRestore,
-          isStored: tlConfig.request?.body.isStored,
+          ...tlConfig.request?.body.searchSession,
         },
         tlConfig.context
       )

diff --git a/src/plugins/vis_type_timeseries/common/vis_schema.ts b/src/plugins/vis_type_timeseries/common/vis_schema.ts
@@ -282,9 +282,11 @@ export const visPayloadSchema = schema.object({
     max: stringRequired,
   }),
 
-  // search sessions integration start
-  sessionId: schema.maybe(schema.string()),
-  isRestore: schema.maybe(schema.boolean()),
-  isStored: schema.maybe(schema.boolean()),
-  // search sessions integration end
+  searchSession: schema.maybe(
+    schema.object({
+      sessionId: schema.maybe(schema.string()),
+      isRestore: schema.maybe(schema.boolean()),
+      isStored: schema.maybe(schema.boolean()),
+    })
+  ),
 });
diff --git a/src/plugins/vis_type_timeseries/public/request_handler.ts b/src/plugins/vis_type_timeseries/public/request_handler.ts
@@ -41,19 +41,18 @@ export const metricsRequestHandler = async ({
   const config = getUISettings();
   const timezone = getTimezone(config);
   const uiStateObj = uiState[visParams.type] ?? {};
-  const dataSearch = getDataStart();
-  const parsedTimeRange = dataSearch.query.timefilter.timefilter.calculateBounds(input?.timeRange!);
+  const data = getDataStart();
+  const dataSearch = getDataStart().search;
+  const parsedTimeRange = data.query.timefilter.timefilter.calculateBounds(input?.timeRange!);
 
   if (visParams && visParams.id && !visParams.isModelInvalid) {
     const maxBuckets = config.get(MAX_BUCKETS_SETTING);
 
     validateInterval(parsedTimeRange, visParams, maxBuckets);
 
-    const isCurrentSession = () =>
-      !!searchSessionId && searchSessionId === dataSearch.search.session.getSessionId();
     const untrackSearch =
-      isCurrentSession() &&
-      dataSearch.search.session.trackSearch({
+      dataSearch.session.isCurrentSession(searchSessionId) &&
+      dataSearch.session.trackSearch({
         abort: () => {
           // TODO: support search cancellations
         },
@@ -70,13 +69,12 @@ export const metricsRequestHandler = async ({
           filters: input?.filters,
           panels: [visParams],
           state: uiStateObj,
-          sessionId: searchSessionId,
-          isRestore: isCurrentSession() ? dataSearch.search.session.isRestore() : false,
-          isStored: isCurrentSession() ? dataSearch.search.session.isStored() : false,
+          searchSession: dataSearch.session.getSearchOptions(searchSessionId),
         }),
       });
     } finally {
-      if (untrackSearch && isCurrentSession()) {
+      if (untrackSearch && dataSearch.session.isCurrentSession(searchSessionId)) {
+        // untrack if this search still belongs to current session
         untrackSearch();
       }
     }

diff --git a/..._type_timeseries/server/lib/search_strategies/strategies/abstract_search_strategy.test.js b/..._type_timeseries/server/lib/search_strategies/strategies/abstract_search_strategy.test.js
@@ -62,7 +62,11 @@ describe('AbstractSearchStrategy', () => {
     const responses = await abstractSearchStrategy.search(
       {
         payload: {
-          sessionId: 1,
+          searchSession: {
+            sessionId: '1',
+            isRestore: false,
+            isStored: true,
+          },
         },
         requestContext: {
           search: { search: searchFn },
@@ -81,7 +85,9 @@ describe('AbstractSearchStrategy', () => {
         indexType: undefined,
       },
       {
-        sessionId: 1,
+        sessionId: '1',
+        isRestore: false,
+        isStored: true,
       }
     );
   });

diff --git a/...s/vis_type_timeseries/server/lib/search_strategies/strategies/abstract_search_strategy.ts b/...s/vis_type_timeseries/server/lib/search_strategies/strategies/abstract_search_strategy.ts
@@ -49,7 +49,6 @@ export interface ReqFacade<T = unknown> extends FakeRequest {
 export abstract class AbstractSearchStrategy {
   async search(req: ReqFacade<VisPayload>, bodies: any[], indexType?: string) {
     const requests: any[] = [];
-    const { sessionId, isRestore, isStored } = req.payload;
 
     bodies.forEach((body) => {
       requests.push(
@@ -61,11 +60,7 @@ export abstract class AbstractSearchStrategy {
                 ...body,
               },
             },
-            {
-              sessionId,
-              isRestore,
-              isStored,
-            }
+            req.payload.searchSession
           )
           .toPromise()
       );

diff --git a/x-pack/plugins/data_enhanced/public/search/search_interceptor.ts b/x-pack/plugins/data_enhanced/public/search/search_interceptor.ts
@@ -64,20 +64,24 @@ export class EnhancedSearchInterceptor extends SearchInterceptor {
     const search = () => this.runSearch({ id, ...request }, searchOptions);
 
     this.pendingCount$.next(this.pendingCount$.getValue() + 1);
-    const isCurrentSession = () =>
-      !!options.sessionId && options.sessionId === this.deps.session.getSessionId();
 
-    const untrackSearch = isCurrentSession() && this.deps.session.trackSearch({ abort });
+    const untrackSearch =
+      this.deps.session.isCurrentSession(options.sessionId) &&
+      this.deps.session.trackSearch({ abort });
 
     // track if this search's session will be send to background
     // if yes, then we don't need to cancel this search when it is aborted
     let isSavedToBackground = false;
     const savedToBackgroundSub =
-      isCurrentSession() &&
+      this.deps.session.isCurrentSession(options.sessionId) &&
       this.deps.session.state$
         .pipe(
           skip(1), // ignore any state, we are only interested in transition x -> BackgroundLoading
-          filter((state) => isCurrentSession() && state === SearchSessionState.BackgroundLoading),
+          filter(
+            (state) =>
+              this.deps.session.isCurrentSession(options.sessionId) &&
+              state === SearchSessionState.BackgroundLoading
+          ),
           take(1)
         )
         .subscribe(() => {
@@ -93,7 +97,8 @@ export class EnhancedSearchInterceptor extends SearchInterceptor {
       finalize(() => {
         this.pendingCount$.next(this.pendingCount$.getValue() - 1);
         cleanup();
-        if (untrackSearch && isCurrentSession()) {
+        if (untrackSearch && this.deps.session.isCurrentSession(options.sessionId)) {
+          // untrack if this search still belongs to current session
           untrackSearch();
         }
         if (savedToBackgroundSub) {