From e0efd61e087afb17ec0f3a9d7f1e5869a29bece7 Mon Sep 17 00:00:00 2001 From: Anton Dosov Date: Mon, 10 Aug 2020 10:56:03 +0200 Subject: [PATCH] Improve state sync error handling (#74264) (#74524) Fixes #71461 regression since 7.7 New state syncing utils didn't properly handle errors, Errors happening during URL parsing or writing wasn't handled, so state syncing could stop or in worth case blow out. (see #71461) There are not much scenarios where missing proper error handling could really impact users, except the one described in #71461: Kibana users state:storeInSessionStorage Users often intuitively share hashed dashboard urls directly When someone opens those urls - there is a blank screen with warning In 7.6 - dashboard would still load with default state. Since 7.7 these still could be achieved by removing query params for URL, but it is not obvious for regular users. This PR makes sure that behaviour is similar to one we had before 7.7. Co-authored-by: Elastic Machine Co-authored-by: Elastic Machine --- ...lic-state_sync.createkbnurlstatestorage.md | 4 +- .../public/application/legacy_app.js | 2 + .../public/application/angular/context.js | 1 + .../application/angular/context_state.ts | 11 +++ .../public/application/angular/discover.js | 1 + .../application/angular/discover_state.ts | 11 +++ .../kibana_utils/docs/state_sync/README.md | 1 + .../docs/state_sync/error_handling.md | 6 ++ .../state_sync/storages/kbn_url_storage.md | 57 ++++++++++++++- src/plugins/kibana_utils/public/index.ts | 1 + .../public/state_management/url/errors.ts | 62 ++++++++++++++++ .../public/state_management/url/index.ts | 1 + .../state_management/url/kbn_url_storage.ts | 10 +-- .../public/state_sync/public.api.md | 4 +- .../create_kbn_url_state_storage.test.ts | 73 ++++++++++++++++++- .../create_kbn_url_state_storage.ts | 40 ++++++++-- src/plugins/timelion/public/app.js | 3 +- src/plugins/visualize/public/plugin.ts | 8 +- .../functional/apps/discover/_shared_links.js | 62 ++++++++++------ test/functional/services/toasts.ts | 10 +++ x-pack/plugins/lens/public/app_plugin/app.tsx | 3 + .../maps/public/routing/maps_router.js | 13 +++- .../monitoring/public/angular/app_modules.ts | 11 ++- x-pack/plugins/monitoring/public/url_state.ts | 8 +- 24 files changed, 355 insertions(+), 48 deletions(-) create mode 100644 src/plugins/kibana_utils/docs/state_sync/error_handling.md create mode 100644 src/plugins/kibana_utils/public/state_management/url/errors.ts diff --git a/docs/development/plugins/kibana_utils/public/state_sync/kibana-plugin-plugins-kibana_utils-public-state_sync.createkbnurlstatestorage.md b/docs/development/plugins/kibana_utils/public/state_sync/kibana-plugin-plugins-kibana_utils-public-state_sync.createkbnurlstatestorage.md index 22f70ce22b574..478ba2d409acd 100644 --- a/docs/development/plugins/kibana_utils/public/state_sync/kibana-plugin-plugins-kibana_utils-public-state_sync.createkbnurlstatestorage.md +++ b/docs/development/plugins/kibana_utils/public/state_sync/kibana-plugin-plugins-kibana_utils-public-state_sync.createkbnurlstatestorage.md @@ -9,8 +9,10 @@ Creates [IKbnUrlStateStorage](./kibana-plugin-plugins-kibana_utils-public-state_ Signature: ```typescript -createKbnUrlStateStorage: ({ useHash, history }?: { +createKbnUrlStateStorage: ({ useHash, history, onGetError, onSetError, }?: { useHash: boolean; history?: History | undefined; + onGetError?: ((error: Error) => void) | undefined; + onSetError?: ((error: Error) => void) | undefined; }) => IKbnUrlStateStorage ``` diff --git a/src/plugins/dashboard/public/application/legacy_app.js b/src/plugins/dashboard/public/application/legacy_app.js index 8b8fdcb7a76ac..abe04fb8bd7e3 100644 --- a/src/plugins/dashboard/public/application/legacy_app.js +++ b/src/plugins/dashboard/public/application/legacy_app.js @@ -30,6 +30,7 @@ import { createKbnUrlStateStorage, redirectWhenMissing, SavedObjectNotFound, + withNotifyOnErrors, } from '../../../kibana_utils/public'; import { DashboardListing, EMPTY_FILTER } from './listing/dashboard_listing'; import { addHelpMenuToAppChrome } from './help_menu/help_menu_util'; @@ -65,6 +66,7 @@ export function initDashboardApp(app, deps) { createKbnUrlStateStorage({ history, useHash: deps.uiSettings.get('state:storeInSessionStorage'), + ...withNotifyOnErrors(deps.core.notifications.toasts), }) ); diff --git a/src/plugins/discover/public/application/angular/context.js b/src/plugins/discover/public/application/angular/context.js index a6f591eebb52d..6223090aa9f97 100644 --- a/src/plugins/discover/public/application/angular/context.js +++ b/src/plugins/discover/public/application/angular/context.js @@ -83,6 +83,7 @@ function ContextAppRouteController($routeParams, $scope, $route) { timeFieldName: indexPattern.timeFieldName, storeInSessionStorage: getServices().uiSettings.get('state:storeInSessionStorage'), history: getServices().history(), + toasts: getServices().core.notifications.toasts, }); this.state = { ...appState.getState() }; this.anchorId = $routeParams.id; diff --git a/src/plugins/discover/public/application/angular/context_state.ts b/src/plugins/discover/public/application/angular/context_state.ts index 7a92a6ace125b..5b05d8729c41d 100644 --- a/src/plugins/discover/public/application/angular/context_state.ts +++ b/src/plugins/discover/public/application/angular/context_state.ts @@ -18,11 +18,13 @@ */ import _ from 'lodash'; import { History } from 'history'; +import { NotificationsStart } from 'kibana/public'; import { createStateContainer, createKbnUrlStateStorage, syncStates, BaseStateContainer, + withNotifyOnErrors, } from '../../../../kibana_utils/public'; import { esFilters, FilterManager, Filter, Query } from '../../../../data/public'; @@ -74,6 +76,13 @@ interface GetStateParams { * History instance to use */ history: History; + + /** + * Core's notifications.toasts service + * In case it is passed in, + * kbnUrlStateStorage will use it notifying about inner errors + */ + toasts?: NotificationsStart['toasts']; } interface GetStateReturn { @@ -123,10 +132,12 @@ export function getState({ timeFieldName, storeInSessionStorage = false, history, + toasts, }: GetStateParams): GetStateReturn { const stateStorage = createKbnUrlStateStorage({ useHash: storeInSessionStorage, history, + ...(toasts && withNotifyOnErrors(toasts)), }); const globalStateInitial = stateStorage.get(GLOBAL_STATE_URL_KEY) as GlobalState; diff --git a/src/plugins/discover/public/application/angular/discover.js b/src/plugins/discover/public/application/angular/discover.js index 4a27f261a6220..22da3e877054a 100644 --- a/src/plugins/discover/public/application/angular/discover.js +++ b/src/plugins/discover/public/application/angular/discover.js @@ -220,6 +220,7 @@ function discoverController($element, $route, $scope, $timeout, $window, Promise defaultAppState: getStateDefaults(), storeInSessionStorage: config.get('state:storeInSessionStorage'), history, + toasts: core.notifications.toasts, }); if (appStateContainer.getState().index !== $scope.indexPattern.id) { //used index pattern is different than the given by url/state which is invalid diff --git a/src/plugins/discover/public/application/angular/discover_state.ts b/src/plugins/discover/public/application/angular/discover_state.ts index 46500d9fdf85e..ff8fb9f80a723 100644 --- a/src/plugins/discover/public/application/angular/discover_state.ts +++ b/src/plugins/discover/public/application/angular/discover_state.ts @@ -18,12 +18,14 @@ */ import { isEqual } from 'lodash'; import { History } from 'history'; +import { NotificationsStart } from 'kibana/public'; import { createStateContainer, createKbnUrlStateStorage, syncState, ReduxLikeStateContainer, IKbnUrlStateStorage, + withNotifyOnErrors, } from '../../../../kibana_utils/public'; import { esFilters, Filter, Query } from '../../../../data/public'; import { migrateLegacyQuery } from '../../../../kibana_legacy/public'; @@ -68,6 +70,13 @@ interface GetStateParams { * Browser history */ history: History; + + /** + * Core's notifications.toasts service + * In case it is passed in, + * kbnUrlStateStorage will use it notifying about inner errors + */ + toasts?: NotificationsStart['toasts']; } export interface GetStateReturn { @@ -122,10 +131,12 @@ export function getState({ defaultAppState = {}, storeInSessionStorage = false, history, + toasts, }: GetStateParams): GetStateReturn { const stateStorage = createKbnUrlStateStorage({ useHash: storeInSessionStorage, history, + ...(toasts && withNotifyOnErrors(toasts)), }); const appStateFromUrl = stateStorage.get(APP_STATE_URL_KEY) as AppState; diff --git a/src/plugins/kibana_utils/docs/state_sync/README.md b/src/plugins/kibana_utils/docs/state_sync/README.md index acfe6dcf76fe9..c84bf7f236330 100644 --- a/src/plugins/kibana_utils/docs/state_sync/README.md +++ b/src/plugins/kibana_utils/docs/state_sync/README.md @@ -58,3 +58,4 @@ To run them, start kibana with `--run-examples` flag. - [On-the-fly state migrations](./on_fly_state_migrations.md). - [syncStates helper](./sync_states.md). - [Helpers for Data plugin (syncing TimeRange, RefreshInterval and Filters)](./data_plugin_helpers.md). +- [Error handling](./error_handling.md) diff --git a/src/plugins/kibana_utils/docs/state_sync/error_handling.md b/src/plugins/kibana_utils/docs/state_sync/error_handling.md new file mode 100644 index 0000000000000..b12e1040af260 --- /dev/null +++ b/src/plugins/kibana_utils/docs/state_sync/error_handling.md @@ -0,0 +1,6 @@ +# Error handling + +State syncing util doesn't have specific api for handling errors. +It expects that errors are handled on storage level. + +see [KbnUrlStateStorage](./storages/kbn_url_storage.md#) error handling section for details. diff --git a/src/plugins/kibana_utils/docs/state_sync/storages/kbn_url_storage.md b/src/plugins/kibana_utils/docs/state_sync/storages/kbn_url_storage.md index 3a31f5a326edb..ec27895eed666 100644 --- a/src/plugins/kibana_utils/docs/state_sync/storages/kbn_url_storage.md +++ b/src/plugins/kibana_utils/docs/state_sync/storages/kbn_url_storage.md @@ -65,7 +65,7 @@ To prevent bugs caused by missing history updates, make sure your app uses one i For example, if you use `react-router`: ```tsx -const App = props => { +const App = (props) => { useEffect(() => { const stateStorage = createKbnUrlStateStorage({ useHash: props.uiSettings.get('state:storeInSessionStorage'), @@ -160,3 +160,58 @@ const { start, stop } = syncStates([ ; ``` + +### Error handling + +Errors could occur both during `kbnUrlStateStorage.get()` and `kbnUrlStateStorage.set()` + +#### Handling kbnUrlStateStorage.get() errors + +Possible error scenarios during `kbnUrlStateStorage.get()`: + +1. Rison in URL is malformed. Parsing exception. +2. useHash is enabled and current hash is missing in `sessionStorage` + +In all the cases error is handled internally and `kbnUrlStateStorage.get()` returns `null`, just like if there is no state in the URL anymore + +You can pass callback to get notified about errors. Use it, for example, for notifying users + +```ts +const kbnUrlStateStorage = createKbnUrlStateStorage({ + history, + onGetError: (error) => { + alert(error.message); + }, +}); +``` + +#### Handling kbnUrlStateStorage.set() errors + +Possible errors during `kbnUrlStateStorage.set()`: + +1. `useHash` is enabled and can't store state in `sessionStorage` (overflow or no access) + +In all the cases error is handled internally and URL update is skipped + +You can pass callback to get notified about errors. Use it, for example, for notifying users: + +```ts +const kbnUrlStateStorage = createKbnUrlStateStorage({ + history, + onSetError: (error) => { + alert(error.message); + }, +}); +``` + +#### Helper to integrate with core.notifications.toasts + +The most common scenario is to notify users about issues with state syncing using toast service from core +There is a convenient helper for this: + +```ts +const kbnUrlStateStorage = createKbnUrlStateStorage({ + history, + ...withNotifyOnErrors(core.notifications.toasts), +}); +``` diff --git a/src/plugins/kibana_utils/public/index.ts b/src/plugins/kibana_utils/public/index.ts index e2d6ae647abb1..d1c9eec0e9906 100644 --- a/src/plugins/kibana_utils/public/index.ts +++ b/src/plugins/kibana_utils/public/index.ts @@ -57,6 +57,7 @@ export { getStateFromKbnUrl, getStatesFromKbnUrl, setStateToKbnUrl, + withNotifyOnErrors, } from './state_management/url'; export { syncState, diff --git a/src/plugins/kibana_utils/public/state_management/url/errors.ts b/src/plugins/kibana_utils/public/state_management/url/errors.ts new file mode 100644 index 0000000000000..b8b6523e8070c --- /dev/null +++ b/src/plugins/kibana_utils/public/state_management/url/errors.ts @@ -0,0 +1,62 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { i18n } from '@kbn/i18n'; +import { NotificationsStart } from 'kibana/public'; + +export const restoreUrlErrorTitle = i18n.translate( + 'kibana_utils.stateManagement.url.restoreUrlErrorTitle', + { + defaultMessage: `Error restoring state from URL`, + } +); + +export const saveStateInUrlErrorTitle = i18n.translate( + 'kibana_utils.stateManagement.url.saveStateInUrlErrorTitle', + { + defaultMessage: `Error saving state in URL`, + } +); + +/** + * Helper for configuring {@link IKbnUrlStateStorage} to notify about inner errors + * + * @example + * ```ts + * const kbnUrlStateStorage = createKbnUrlStateStorage({ + * history, + * ...withNotifyOnErrors(core.notifications.toast)) + * } + * ``` + * @param toast - toastApi from core.notifications.toasts + */ +export const withNotifyOnErrors = (toasts: NotificationsStart['toasts']) => { + return { + onGetError: (error: Error) => { + toasts.addError(error, { + title: restoreUrlErrorTitle, + }); + }, + onSetError: (error: Error) => { + toasts.addError(error, { + title: saveStateInUrlErrorTitle, + }); + }, + }; +}; diff --git a/src/plugins/kibana_utils/public/state_management/url/index.ts b/src/plugins/kibana_utils/public/state_management/url/index.ts index e28d183c6560a..66fecd723e3ba 100644 --- a/src/plugins/kibana_utils/public/state_management/url/index.ts +++ b/src/plugins/kibana_utils/public/state_management/url/index.ts @@ -27,3 +27,4 @@ export { } from './kbn_url_storage'; export { createKbnUrlTracker } from './kbn_url_tracker'; export { createUrlTracker } from './url_tracker'; +export { withNotifyOnErrors, saveStateInUrlErrorTitle, restoreUrlErrorTitle } from './errors'; diff --git a/src/plugins/kibana_utils/public/state_management/url/kbn_url_storage.ts b/src/plugins/kibana_utils/public/state_management/url/kbn_url_storage.ts index d9149095a2fa2..fefd5f668c6b3 100644 --- a/src/plugins/kibana_utils/public/state_management/url/kbn_url_storage.ts +++ b/src/plugins/kibana_utils/public/state_management/url/kbn_url_storage.ts @@ -103,7 +103,7 @@ export function setStateToKbnUrl( export interface IKbnUrlControls { /** * Listen for url changes - * @param cb - get's called when url has been changed + * @param cb - called when url has been changed */ listen: (cb: () => void) => () => void; @@ -142,12 +142,12 @@ export interface IKbnUrlControls { */ cancel: () => void; } -export type UrlUpdaterFnType = (currentUrl: string) => string; +export type UrlUpdaterFnType = (currentUrl: string) => string | undefined; export const createKbnUrlControls = ( history: History = createBrowserHistory() ): IKbnUrlControls => { - const updateQueue: Array<(currentUrl: string) => string> = []; + const updateQueue: UrlUpdaterFnType[] = []; // if we should replace or push with next async update, // if any call in a queue asked to push, then we should push @@ -188,7 +188,7 @@ export const createKbnUrlControls = ( function getPendingUrl() { if (updateQueue.length === 0) return undefined; const resultUrl = updateQueue.reduce( - (url, nextUpdate) => nextUpdate(url), + (url, nextUpdate) => nextUpdate(url) ?? url, getCurrentUrl(history) ); @@ -201,7 +201,7 @@ export const createKbnUrlControls = ( cb(); }), update: (newUrl: string, replace = false) => updateUrl(newUrl, replace), - updateAsync: (updater: (currentUrl: string) => string, replace = false) => { + updateAsync: (updater: UrlUpdaterFnType, replace = false) => { updateQueue.push(updater); if (shouldReplace) { shouldReplace = replace; diff --git a/src/plugins/kibana_utils/public/state_sync/public.api.md b/src/plugins/kibana_utils/public/state_sync/public.api.md index ae8c0e8e401b8..a4dfea82cdb59 100644 --- a/src/plugins/kibana_utils/public/state_sync/public.api.md +++ b/src/plugins/kibana_utils/public/state_sync/public.api.md @@ -8,9 +8,11 @@ import { History } from 'history'; import { Observable } from 'rxjs'; // @public -export const createKbnUrlStateStorage: ({ useHash, history }?: { +export const createKbnUrlStateStorage: ({ useHash, history, onGetError, onSetError, }?: { useHash: boolean; history?: History | undefined; + onGetError?: ((error: Error) => void) | undefined; + onSetError?: ((error: Error) => void) | undefined; }) => IKbnUrlStateStorage; // Warning: (ae-unresolved-link) The @link reference could not be resolved: The package "kibana" does not have an export "Storage" diff --git a/src/plugins/kibana_utils/public/state_sync/state_sync_state_storage/create_kbn_url_state_storage.test.ts b/src/plugins/kibana_utils/public/state_sync/state_sync_state_storage/create_kbn_url_state_storage.test.ts index cc708d14ea8b5..e222af91d7729 100644 --- a/src/plugins/kibana_utils/public/state_sync/state_sync_state_storage/create_kbn_url_state_storage.test.ts +++ b/src/plugins/kibana_utils/public/state_sync/state_sync_state_storage/create_kbn_url_state_storage.test.ts @@ -16,12 +16,14 @@ * specific language governing permissions and limitations * under the License. */ -import '../../storage/hashed_item_store/mock'; +import { mockStorage } from '../../storage/hashed_item_store/mock'; import { createKbnUrlStateStorage, IKbnUrlStateStorage } from './create_kbn_url_state_storage'; import { History, createBrowserHistory } from 'history'; import { takeUntil, toArray } from 'rxjs/operators'; import { Subject } from 'rxjs'; import { ScopedHistory } from '../../../../../core/public'; +import { withNotifyOnErrors } from '../../state_management/url'; +import { coreMock } from '../../../../../core/public/mocks'; describe('KbnUrlStateStorage', () => { describe('useHash: false', () => { @@ -93,6 +95,37 @@ describe('KbnUrlStateStorage', () => { expect(await result).toEqual([{ test: 'test', ok: 1 }, { test: 'test', ok: 2 }, null]); }); + + it("shouldn't throw in case of parsing error", async () => { + const key = '_s'; + history.replace(`/#?${key}=(ok:2,test:`); // malformed rison + expect(() => urlStateStorage.get(key)).not.toThrow(); + expect(urlStateStorage.get(key)).toBeNull(); + }); + + it('should notify about errors', () => { + const cb = jest.fn(); + urlStateStorage = createKbnUrlStateStorage({ useHash: false, history, onGetError: cb }); + const key = '_s'; + history.replace(`/#?${key}=(ok:2,test:`); // malformed rison + expect(() => urlStateStorage.get(key)).not.toThrow(); + expect(cb).toBeCalledWith(expect.any(Error)); + }); + + describe('withNotifyOnErrors integration', () => { + test('toast is shown', () => { + const toasts = coreMock.createStart().notifications.toasts; + urlStateStorage = createKbnUrlStateStorage({ + useHash: true, + history, + ...withNotifyOnErrors(toasts), + }); + const key = '_s'; + history.replace(`/#?${key}=(ok:2,test:`); // malformed rison + expect(() => urlStateStorage.get(key)).not.toThrow(); + expect(toasts.addError).toBeCalled(); + }); + }); }); describe('useHash: true', () => { @@ -128,6 +161,44 @@ describe('KbnUrlStateStorage', () => { expect(await result).toEqual([{ test: 'test', ok: 1 }, { test: 'test', ok: 2 }, null]); }); + + describe('hashStorage overflow exception', () => { + let oldLimit: number; + beforeAll(() => { + oldLimit = mockStorage.getStubbedSizeLimit(); + mockStorage.clear(); + mockStorage.setStubbedSizeLimit(0); + }); + afterAll(() => { + mockStorage.setStubbedSizeLimit(oldLimit); + }); + + it("shouldn't throw in case of error", async () => { + expect(() => urlStateStorage.set('_s', { test: 'test' })).not.toThrow(); + await expect(urlStateStorage.set('_s', { test: 'test' })).resolves; // not rejects + expect(getCurrentUrl()).toBe('/'); // url wasn't updated with hash + }); + + it('should notify about errors', async () => { + const cb = jest.fn(); + urlStateStorage = createKbnUrlStateStorage({ useHash: true, history, onSetError: cb }); + await expect(urlStateStorage.set('_s', { test: 'test' })).resolves; // not rejects + expect(cb).toBeCalledWith(expect.any(Error)); + }); + + describe('withNotifyOnErrors integration', () => { + test('toast is shown', async () => { + const toasts = coreMock.createStart().notifications.toasts; + urlStateStorage = createKbnUrlStateStorage({ + useHash: true, + history, + ...withNotifyOnErrors(toasts), + }); + await expect(urlStateStorage.set('_s', { test: 'test' })).resolves; // not rejects + expect(toasts.addError).toBeCalled(); + }); + }); + }); }); describe('ScopedHistory integration', () => { diff --git a/src/plugins/kibana_utils/public/state_sync/state_sync_state_storage/create_kbn_url_state_storage.ts b/src/plugins/kibana_utils/public/state_sync/state_sync_state_storage/create_kbn_url_state_storage.ts index 0c74e1eb9f421..460720b98e30f 100644 --- a/src/plugins/kibana_utils/public/state_sync/state_sync_state_storage/create_kbn_url_state_storage.ts +++ b/src/plugins/kibana_utils/public/state_sync/state_sync_state_storage/create_kbn_url_state_storage.ts @@ -17,8 +17,8 @@ * under the License. */ -import { Observable } from 'rxjs'; -import { map, share } from 'rxjs/operators'; +import { Observable, of } from 'rxjs'; +import { catchError, map, share } from 'rxjs/operators'; import { History } from 'history'; import { IStateStorage } from './types'; import { @@ -68,7 +68,19 @@ export interface IKbnUrlStateStorage extends IStateStorage { * @public */ export const createKbnUrlStateStorage = ( - { useHash = false, history }: { useHash: boolean; history?: History } = { useHash: false } + { + useHash = false, + history, + onGetError, + onSetError, + }: { + useHash: boolean; + history?: History; + onGetError?: (error: Error) => void; + onSetError?: (error: Error) => void; + } = { + useHash: false, + } ): IKbnUrlStateStorage => { const url = createKbnUrlControls(history); return { @@ -78,15 +90,23 @@ export const createKbnUrlStateStorage = ( { replace = false }: { replace: boolean } = { replace: false } ) => { // syncState() utils doesn't wait for this promise - return url.updateAsync( - (currentUrl) => setStateToKbnUrl(key, state, { useHash }, currentUrl), - replace - ); + return url.updateAsync((currentUrl) => { + try { + return setStateToKbnUrl(key, state, { useHash }, currentUrl); + } catch (error) { + if (onSetError) onSetError(error); + } + }, replace); }, get: (key) => { // if there is a pending url update, then state will be extracted from that pending url, // otherwise current url will be used to retrieve state from - return getStateFromKbnUrl(key, url.getPendingUrl()); + try { + return getStateFromKbnUrl(key, url.getPendingUrl()); + } catch (e) { + if (onGetError) onGetError(e); + return null; + } }, change$: (key: string) => new Observable((observer) => { @@ -99,6 +119,10 @@ export const createKbnUrlStateStorage = ( }; }).pipe( map(() => getStateFromKbnUrl(key)), + catchError((error) => { + if (onGetError) onGetError(error); + return of(null); + }), share() ), flush: ({ replace = false }: { replace?: boolean } = {}) => { diff --git a/src/plugins/timelion/public/app.js b/src/plugins/timelion/public/app.js index 0294e71084f98..614a7539de44c 100644 --- a/src/plugins/timelion/public/app.js +++ b/src/plugins/timelion/public/app.js @@ -23,7 +23,7 @@ import { i18n } from '@kbn/i18n'; import { createHashHistory } from 'history'; -import { createKbnUrlStateStorage } from '../../kibana_utils/public'; +import { createKbnUrlStateStorage, withNotifyOnErrors } from '../../kibana_utils/public'; import { syncQueryStateWithUrl } from '../../data/public'; import { getSavedSheetBreadcrumbs, getCreateBreadcrumbs } from './breadcrumbs'; @@ -63,6 +63,7 @@ export function initTimelionApp(app, deps) { createKbnUrlStateStorage({ history, useHash: deps.core.uiSettings.get('state:storeInSessionStorage'), + ...withNotifyOnErrors(deps.core.notifications.toasts), }) ); app.config(watchMultiDecorator); diff --git a/src/plugins/visualize/public/plugin.ts b/src/plugins/visualize/public/plugin.ts index 8591a2e93210d..dbf62767bb361 100644 --- a/src/plugins/visualize/public/plugin.ts +++ b/src/plugins/visualize/public/plugin.ts @@ -31,7 +31,12 @@ import { ScopedHistory, } from 'kibana/public'; -import { Storage, createKbnUrlTracker, createKbnUrlStateStorage } from '../../kibana_utils/public'; +import { + Storage, + createKbnUrlTracker, + createKbnUrlStateStorage, + withNotifyOnErrors, +} from '../../kibana_utils/public'; import { DataPublicPluginStart, DataPublicPluginSetup, esFilters } from '../../data/public'; import { NavigationPublicPluginStart as NavigationStart } from '../../navigation/public'; import { SharePluginStart } from '../../share/public'; @@ -150,6 +155,7 @@ export class VisualizePlugin kbnUrlStateStorage: createKbnUrlStateStorage({ history, useHash: coreStart.uiSettings.get('state:storeInSessionStorage'), + ...withNotifyOnErrors(coreStart.notifications.toasts), }), kibanaLegacy: pluginsStart.kibanaLegacy, pluginInitializerContext: this.initializerContext, diff --git a/test/functional/apps/discover/_shared_links.js b/test/functional/apps/discover/_shared_links.js index 5c6a70450a0aa..94409a94e9257 100644 --- a/test/functional/apps/discover/_shared_links.js +++ b/test/functional/apps/discover/_shared_links.js @@ -26,6 +26,7 @@ export default function ({ getService, getPageObjects }) { const kibanaServer = getService('kibanaServer'); const PageObjects = getPageObjects(['common', 'discover', 'share', 'timePicker']); const browser = getService('browser'); + const toasts = getService('toasts'); describe('shared links', function describeIndexTests() { let baseUrl; @@ -132,28 +133,47 @@ export default function ({ getService, getPageObjects }) { await teardown(); }); - describe('permalink', function () { - it('should allow for copying the snapshot URL as a short URL and should open it', async function () { - const re = new RegExp(baseUrl + '/goto/[0-9a-f]{32}$'); - await PageObjects.share.checkShortenUrl(); - let actualUrl; - await retry.try(async () => { - actualUrl = await PageObjects.share.getSharedUrl(); - expect(actualUrl).to.match(re); - }); + it('should allow for copying the snapshot URL as a short URL and should open it', async function () { + const re = new RegExp(baseUrl + '/goto/[0-9a-f]{32}$'); + await PageObjects.share.checkShortenUrl(); + let actualUrl; + await retry.try(async () => { + actualUrl = await PageObjects.share.getSharedUrl(); + expect(actualUrl).to.match(re); + }); - const actualTime = await PageObjects.timePicker.getTimeConfig(); - - await browser.clearSessionStorage(); - await browser.get(actualUrl, false); - await retry.waitFor('shortUrl resolves and opens', async () => { - const resolvedUrl = await browser.getCurrentUrl(); - expect(resolvedUrl).to.match(/discover/); - const resolvedTime = await PageObjects.timePicker.getTimeConfig(); - expect(resolvedTime.start).to.equal(actualTime.start); - expect(resolvedTime.end).to.equal(actualTime.end); - return true; - }); + const actualTime = await PageObjects.timePicker.getTimeConfig(); + + await browser.clearSessionStorage(); + await browser.get(actualUrl, false); + await retry.waitFor('shortUrl resolves and opens', async () => { + const resolvedUrl = await browser.getCurrentUrl(); + expect(resolvedUrl).to.match(/discover/); + const resolvedTime = await PageObjects.timePicker.getTimeConfig(); + expect(resolvedTime.start).to.equal(actualTime.start); + expect(resolvedTime.end).to.equal(actualTime.end); + return true; + }); + }); + + it("sharing hashed url shouldn't crash the app", async () => { + const currentUrl = await browser.getCurrentUrl(); + const timeBeforeReload = await PageObjects.timePicker.getTimeConfig(); + await browser.clearSessionStorage(); + await browser.get(currentUrl, false); + await retry.waitFor('discover to open', async () => { + const resolvedUrl = await browser.getCurrentUrl(); + expect(resolvedUrl).to.match(/discover/); + const { message } = await toasts.getErrorToast(); + expect(message).to.contain( + 'Unable to completely restore the URL, be sure to use the share functionality.' + ); + await toasts.dismissAllToasts(); + const timeAfterReload = await PageObjects.timePicker.getTimeConfig(); + expect(timeBeforeReload.start).not.to.be(timeAfterReload.start); + expect(timeBeforeReload.end).not.to.be(timeAfterReload.end); + await PageObjects.timePicker.setDefaultAbsoluteRange(); + return true; }); }); }); diff --git a/test/functional/services/toasts.ts b/test/functional/services/toasts.ts index 92f1f726fa039..a70e4ba464ae8 100644 --- a/test/functional/services/toasts.ts +++ b/test/functional/services/toasts.ts @@ -53,6 +53,16 @@ export function ToastsProvider({ getService }: FtrProviderContext) { await dismissButton.click(); } + public async dismissAllToasts() { + const list = await this.getGlobalToastList(); + const toasts = await list.findAllByCssSelector(`.euiToast`); + for (const toast of toasts) { + await toast.moveMouseTo(); + const dismissButton = await testSubjects.findDescendant('toastCloseButton', toast); + await dismissButton.click(); + } + } + private async getToastElement(index: number) { const list = await this.getGlobalToastList(); return await list.findByCssSelector(`.euiToast:nth-child(${index})`); diff --git a/x-pack/plugins/lens/public/app_plugin/app.tsx b/x-pack/plugins/lens/public/app_plugin/app.tsx index a360f1c2a5ba3..ffab84a51a229 100644 --- a/x-pack/plugins/lens/public/app_plugin/app.tsx +++ b/x-pack/plugins/lens/public/app_plugin/app.tsx @@ -19,6 +19,7 @@ import { import { createKbnUrlStateStorage, IStorageWrapper, + withNotifyOnErrors, } from '../../../../../src/plugins/kibana_utils/public'; import { KibanaContextProvider } from '../../../../../src/plugins/kibana_react/public'; import { @@ -152,6 +153,7 @@ export function App({ const kbnUrlStateStorage = createKbnUrlStateStorage({ history, useHash: core.uiSettings.get('state:storeInSessionStorage'), + ...withNotifyOnErrors(core.notifications.toasts), }); const { stop: stopSyncingQueryServiceStateWithUrl } = syncQueryStateWithUrl( data.query, @@ -166,6 +168,7 @@ export function App({ }, [ data.query.filterManager, data.query.timefilter.timefilter, + core.notifications.toasts, core.uiSettings, data.query, history, diff --git a/x-pack/plugins/maps/public/routing/maps_router.js b/x-pack/plugins/maps/public/routing/maps_router.js index 30b2137399c1e..9992bd7a92ab1 100644 --- a/x-pack/plugins/maps/public/routing/maps_router.js +++ b/x-pack/plugins/maps/public/routing/maps_router.js @@ -7,8 +7,11 @@ import React from 'react'; import { render, unmountComponentAtNode } from 'react-dom'; import { Router, Switch, Route, Redirect } from 'react-router-dom'; -import { getCoreI18n } from '../kibana_services'; -import { createKbnUrlStateStorage } from '../../../../../src/plugins/kibana_utils/public'; +import { getCoreI18n, getToasts } from '../kibana_services'; +import { + createKbnUrlStateStorage, + withNotifyOnErrors, +} from '../../../../../src/plugins/kibana_utils/public'; import { getStore } from './store_operations'; import { Provider } from 'react-redux'; import { LoadListAndRender } from './routes/list/load_list_and_render'; @@ -19,7 +22,11 @@ export let kbnUrlStateStorage; export async function renderApp(context, { appBasePath, element, history, onAppLeave }) { goToSpecifiedPath = (path) => history.push(path); - kbnUrlStateStorage = createKbnUrlStateStorage({ useHash: false, history }); + kbnUrlStateStorage = createKbnUrlStateStorage({ + useHash: false, + history, + ...withNotifyOnErrors(getToasts()), + }); render(, element); diff --git a/x-pack/plugins/monitoring/public/angular/app_modules.ts b/x-pack/plugins/monitoring/public/angular/app_modules.ts index f3d77b196b26e..499610045d771 100644 --- a/x-pack/plugins/monitoring/public/angular/app_modules.ts +++ b/x-pack/plugins/monitoring/public/angular/app_modules.ts @@ -23,7 +23,7 @@ import { GlobalState } from '../url_state'; import { getSafeForExternalLink } from '../lib/get_safe_for_external_link'; // @ts-ignore -import { formatNumber, formatMetric } from '../lib/format_number'; +import { formatMetric, formatNumber } from '../lib/format_number'; // @ts-ignore import { extractIp } from '../lib/extract_ip'; // @ts-ignore @@ -65,7 +65,7 @@ export const localAppModule = ({ createLocalPrivateModule(); createLocalStorage(); createLocalConfigModule(core); - createLocalStateModule(query); + createLocalStateModule(query, core.notifications.toasts); createLocalTopNavModule(navigation); createHrefModule(core); createMonitoringAppServices(); @@ -97,7 +97,10 @@ function createMonitoringAppConfigConstants( keys.map(([key, value]) => (constantsModule = constantsModule.constant(key as string, value))); } -function createLocalStateModule(query: any) { +function createLocalStateModule( + query: MonitoringStartPluginDependencies['data']['query'], + toasts: MonitoringStartPluginDependencies['core']['notifications']['toasts'] +) { angular .module('monitoring/State', ['monitoring/Private']) .service('globalState', function ( @@ -106,7 +109,7 @@ function createLocalStateModule(query: any) { $location: ng.ILocationService ) { function GlobalStateProvider(this: any) { - const state = new GlobalState(query, $rootScope, $location, this); + const state = new GlobalState(query, toasts, $rootScope, $location, this); const initialState: any = state.getState(); for (const key in initialState) { if (!initialState.hasOwnProperty(key)) { diff --git a/x-pack/plugins/monitoring/public/url_state.ts b/x-pack/plugins/monitoring/public/url_state.ts index e53497d751f9b..65e48223d7a64 100644 --- a/x-pack/plugins/monitoring/public/url_state.ts +++ b/x-pack/plugins/monitoring/public/url_state.ts @@ -23,6 +23,7 @@ import { IKbnUrlStateStorage, ISyncStateRef, syncState, + withNotifyOnErrors, } from '../../../../src/plugins/kibana_utils/public'; interface Route { @@ -71,6 +72,7 @@ export class GlobalState { constructor( queryService: MonitoringStartPluginDependencies['data']['query'], + toasts: MonitoringStartPluginDependencies['core']['notifications']['toasts'], rootScope: ng.IRootScopeService, ngLocation: ng.ILocationService, externalState: RawObject @@ -78,7 +80,11 @@ export class GlobalState { this.timefilterRef = queryService.timefilter.timefilter; const history: History = createHashHistory(); - this.stateStorage = createKbnUrlStateStorage({ useHash: false, history }); + this.stateStorage = createKbnUrlStateStorage({ + useHash: false, + history, + ...withNotifyOnErrors(toasts), + }); const initialStateFromUrl = this.stateStorage.get(GLOBAL_STATE_KEY) as MonitoringAppState;