[Search Sessions] Fix Discover doesn't clean session when navigating …

…to Context (#91874)
elastic · Feb 23, 2021 · ef5d5cf · ef5d5cf
1 parent 4c82ffc
commit ef5d5cf
Show file tree

Hide file tree

Showing 6 changed files with 88 additions and 34 deletions.
diff --git a/src/plugins/data/public/search/session/search_session_state.test.ts b/src/plugins/data/public/search/session/search_session_state.test.ts
@@ -9,6 +9,7 @@
 import { createSessionStateContainer, SearchSessionState } from './search_session_state';
 
 describe('Session state container', () => {
+  const appName = 'appName';
   const { stateContainer: state } = createSessionStateContainer();
 
   afterEach(() => {
@@ -17,23 +18,24 @@ describe('Session state container', () => {
 
   describe('transitions', () => {
     test('start', () => {
-      state.transitions.start();
+      state.transitions.start({ appName });
       expect(state.selectors.getState()).toBe(SearchSessionState.None);
       expect(state.get().sessionId).not.toBeUndefined();
       expect(state.get().startTime).not.toBeUndefined();
+      expect(state.get().appName).toBe(appName);
     });
 
     test('track', () => {
       expect(() => state.transitions.trackSearch({})).toThrowError();
 
-      state.transitions.start();
+      state.transitions.start({ appName });
       state.transitions.trackSearch({});
 
       expect(state.selectors.getState()).toBe(SearchSessionState.Loading);
     });
 
     test('untrack', () => {
-      state.transitions.start();
+      state.transitions.start({ appName });
       const search = {};
       state.transitions.trackSearch(search);
       expect(state.selectors.getState()).toBe(SearchSessionState.Loading);
@@ -42,17 +44,18 @@ describe('Session state container', () => {
     });
 
     test('clear', () => {
-      state.transitions.start();
+      state.transitions.start({ appName });
       state.transitions.clear();
       expect(state.selectors.getState()).toBe(SearchSessionState.None);
       expect(state.get().sessionId).toBeUndefined();
       expect(state.get().startTime).toBeUndefined();
+      expect(state.get().appName).toBeUndefined();
     });
 
     test('cancel', () => {
       expect(() => state.transitions.cancel()).toThrowError();
 
-      state.transitions.start();
+      state.transitions.start({ appName });
       const search = {};
       state.transitions.trackSearch(search);
       expect(state.selectors.getState()).toBe(SearchSessionState.Loading);
@@ -65,7 +68,7 @@ describe('Session state container', () => {
     test('store -> completed', () => {
       expect(() => state.transitions.store()).toThrowError();
 
-      state.transitions.start();
+      state.transitions.start({ appName });
       const search = {};
       state.transitions.trackSearch(search);
       expect(state.selectors.getState()).toBe(SearchSessionState.Loading);
@@ -77,7 +80,7 @@ describe('Session state container', () => {
       expect(state.selectors.getState()).toBe(SearchSessionState.None);
     });
     test('store -> cancel', () => {
-      state.transitions.start();
+      state.transitions.start({ appName });
       const search = {};
       state.transitions.trackSearch(search);
       expect(state.selectors.getState()).toBe(SearchSessionState.Loading);
@@ -89,7 +92,7 @@ describe('Session state container', () => {
       state.transitions.trackSearch(search);
       expect(state.selectors.getState()).toBe(SearchSessionState.Canceled);
 
-      state.transitions.start();
+      state.transitions.start({ appName });
       expect(state.selectors.getState()).toBe(SearchSessionState.None);
     });
 
@@ -108,7 +111,7 @@ describe('Session state container', () => {
       expect(() => state.transitions.cancel()).toThrowError();
       expect(state.selectors.getState()).toBe(SearchSessionState.Restored);
 
-      state.transitions.start();
+      state.transitions.start({ appName });
       expect(state.selectors.getState()).toBe(SearchSessionState.None);
     });
   });

diff --git a/src/plugins/data/public/search/session/search_session_state.ts b/src/plugins/data/public/search/session/search_session_state.ts
@@ -68,6 +68,11 @@ export interface SessionStateInternal<SearchDescriptor = unknown> {
    */
   sessionId?: string;
 
+  /**
+   * App that created this session
+   */
+  appName?: string;
+
   /**
    * Has the session already been stored (i.e. "sent to background")?
    */
@@ -105,6 +110,7 @@ const createSessionDefaultState: <
   SearchDescriptor = unknown
 >() => SessionStateInternal<SearchDescriptor> = () => ({
   sessionId: undefined,
+  appName: undefined,
   isStored: false,
   isRestore: false,
   isCanceled: false,
@@ -116,7 +122,7 @@ export interface SessionPureTransitions<
   SearchDescriptor = unknown,
   S = SessionStateInternal<SearchDescriptor>
 > {
-  start: (state: S) => () => S;
+  start: (state: S) => ({ appName }: { appName: string }) => S;
   restore: (state: S) => (sessionId: string) => S;
   clear: (state: S) => () => S;
   store: (state: S) => () => S;
@@ -126,10 +132,11 @@ export interface SessionPureTransitions<
 }
 
 export const sessionPureTransitions: SessionPureTransitions = {
-  start: (state) => () => ({
+  start: (state) => ({ appName }) => ({
     ...createSessionDefaultState(),
     sessionId: uuid.v4(),
     startTime: new Date(),
+    appName,
   }),
   restore: (state) => (sessionId: string) => ({
     ...createSessionDefaultState(),

diff --git a/src/plugins/data/public/search/session/session_service.test.ts b/src/plugins/data/public/search/session/session_service.test.ts
@@ -21,11 +21,13 @@ describe('Session service', () => {
   let state$: BehaviorSubject<SearchSessionState>;
   let nowProvider: jest.Mocked<NowProviderInternalContract>;
   let userHasAccessToSearchSessions = true;
+  let currentAppId$: BehaviorSubject<string>;
 
   beforeEach(() => {
     const initializerContext = coreMock.createPluginInitializerContext();
     const startService = coreMock.createSetup().getStartServices;
     nowProvider = createNowProviderMock();
+    currentAppId$ = new BehaviorSubject('app');
     sessionService = new SessionService(
       initializerContext,
       () =>
@@ -34,7 +36,7 @@ describe('Session service', () => {
             ...coreStart,
             application: {
               ...coreStart.application,
-              currentAppId$: new BehaviorSubject('app'),
+              currentAppId$,
               capabilities: {
                 ...coreStart.application.capabilities,
                 management: {
@@ -65,6 +67,23 @@ describe('Session service', () => {
       expect(nowProvider.reset).toHaveBeenCalled();
     });
 
+    it("Can't clear other apps' session", async () => {
+      sessionService.start();
+      expect(sessionService.getSessionId()).not.toBeUndefined();
+      currentAppId$.next('change');
+      sessionService.clear();
+      expect(sessionService.getSessionId()).not.toBeUndefined();
+    });
+
+    it("Can start a new session in case there is other apps' stale session", async () => {
+      const s1 = sessionService.start();
+      expect(sessionService.getSessionId()).not.toBeUndefined();
+      currentAppId$.next('change');
+      sessionService.start();
+      expect(sessionService.getSessionId()).not.toBeUndefined();
+      expect(sessionService.getSessionId()).not.toBe(s1);
+    });
+
     it('Restores a session', async () => {
       const sessionId = 'sessionId';
       sessionService.restore(sessionId);

diff --git a/src/plugins/data/public/search/session/session_service.ts b/src/plugins/data/public/search/session/session_service.ts
@@ -68,7 +68,7 @@ export class SessionService {
   private searchSessionInfoProvider?: SearchSessionInfoProvider;
   private searchSessionIndicatorUiConfig?: Partial<SearchSessionIndicatorUiConfig>;
   private subscription = new Subscription();
-  private curApp?: string;
+  private currentApp?: string;
   private hasAccessToSearchSessions: boolean = false;
 
   constructor(
@@ -100,24 +100,24 @@ export class SessionService {
       this.hasAccessToSearchSessions =
         coreStart.application.capabilities.management?.kibana?.[SEARCH_SESSIONS_MANAGEMENT_ID];
 
-      // Apps required to clean up their sessions before unmounting
-      // Make sure that apps don't leave sessions open.
       this.subscription.add(
-        coreStart.application.currentAppId$.subscribe((appName) => {
-          if (this.state.get().sessionId) {
-            const message = `Application '${this.curApp}' had an open session while navigating`;
-            if (initializerContext.env.mode.dev) {
-              // TODO: This setTimeout is necessary due to a race condition while navigating.
-              setTimeout(() => {
-                coreStart.fatalErrors.add(message);
-              }, 100);
-            } else {
-              // eslint-disable-next-line no-console
-              console.warn(message);
-              this.clear();
-            }
+        coreStart.application.currentAppId$.subscribe((newAppName) => {
+          this.currentApp = newAppName;
+          if (!this.getSessionId()) return;
+
+          // Apps required to clean up their sessions before unmounting
+          // Make sure that apps don't leave sessions open by throwing an error in DEV mode
+          const message = `Application '${
+            this.state.get().appName
+          }' had an open session while navigating`;
+          if (initializerContext.env.mode.dev) {
+            coreStart.fatalErrors.add(message);
+          } else {
+            // this should never happen in prod because should be caught in dev mode
+            // in case this happen we don't want to throw fatal error, as most likely possible bugs are not that critical
+            // eslint-disable-next-line no-console
+            console.warn(message);
           }
-          this.curApp = appName;
         })
       );
     });
@@ -187,7 +187,8 @@ export class SessionService {
    * @returns sessionId
    */
   public start() {
-    this.state.transitions.start();
+    if (!this.currentApp) throw new Error('this.currentApp is missing');
+    this.state.transitions.start({ appName: this.currentApp });
     return this.getSessionId()!;
   }
 
@@ -203,6 +204,18 @@ export class SessionService {
    * Cleans up current state
    */
   public clear() {
+    // make sure apps can't clear other apps' sessions
+    const currentSessionApp = this.state.get().appName;
+    if (currentSessionApp && currentSessionApp !== this.currentApp) {
+      // eslint-disable-next-line no-console
+      console.warn(
+        `Skip clearing session "${this.getSessionId()}" because it belongs to a different app. current: "${
+          this.currentApp
+        }", owner: "${currentSessionApp}"`
+      );
+      return;
+    }
+
     this.state.transitions.clear();
     this.searchSessionInfoProvider = undefined;
     this.searchSessionIndicatorUiConfig = undefined;
@@ -229,7 +242,8 @@ export class SessionService {
   public async save(): Promise<void> {
     const sessionId = this.getSessionId();
     if (!sessionId) throw new Error('No current session');
-    if (!this.curApp) throw new Error('No current app id');
+    const currentSessionApp = this.state.get().appName;
+    if (!currentSessionApp) throw new Error('No current session app');
     if (!this.hasAccess()) throw new Error('No access to search sessions');
     const currentSessionInfoProvider = this.searchSessionInfoProvider;
     if (!currentSessionInfoProvider) throw new Error('No info provider for current session');
@@ -240,7 +254,7 @@ export class SessionService {
 
     await this.sessionsClient.create({
       name,
-      appId: this.curApp,
+      appId: currentSessionApp,
       restoreState: (restoreState as unknown) as Record<string, unknown>,
       initialState: (initialState as unknown) as Record<string, unknown>,
       urlGeneratorId,

diff --git a/src/plugins/discover/public/application/angular/discover.js b/src/plugins/discover/public/application/angular/discover.js
@@ -239,7 +239,8 @@ function discoverController($route, $scope, Promise) {
 
     if (!_.isEqual(newStatePartial, oldStatePartial)) {
       $scope.$evalAsync(async () => {
-        if (oldStatePartial.index !== newStatePartial.index) {
+        // NOTE: this is also called when navigating from discover app to context app
+        if (newStatePartial.index && oldStatePartial.index !== newStatePartial.index) {
           //in case of index pattern switch the route has currently to be reloaded, legacy
           isChangingIndexPattern = true;
           $route.reload();

diff --git a/x-pack/test/send_search_to_background_integration/tests/apps/discover/async_search.ts b/x-pack/test/send_search_to_background_integration/tests/apps/discover/async_search.ts
@@ -14,7 +14,8 @@ export default function ({ getPageObjects, getService }: FtrProviderContext) {
   const testSubjects = getService('testSubjects');
   const browser = getService('browser');
   const inspector = getService('inspector');
-  const PageObjects = getPageObjects(['discover', 'common', 'timePicker', 'header']);
+  const docTable = getService('docTable');
+  const PageObjects = getPageObjects(['discover', 'common', 'timePicker', 'header', 'context']);
   const searchSessions = getService('searchSessions');
 
   describe('discover async search', () => {
@@ -62,6 +63,15 @@ export default function ({ getPageObjects, getService }: FtrProviderContext) {
       await searchSessions.expectState('restored');
       expect(await getSearchSessionId()).to.be(fakeSearchSessionId);
     });
+
+    it('navigation to context cleans the session', async () => {
+      await PageObjects.common.clearAllToasts();
+      await docTable.clickRowToggle({ rowIndex: 0 });
+      const rowActions = await docTable.getRowActions({ rowIndex: 0 });
+      await rowActions[0].click();
+      await PageObjects.context.waitUntilContextLoadingHasFinished();
+      await searchSessions.missingOrFail();
+    });
   });
 
   async function getSearchSessionId(): Promise<string> {