From 4f83c116c7468df2b520e09d6f84093fcd54873a Mon Sep 17 00:00:00 2001 From: Anton Dosov Date: Tue, 4 Aug 2020 17:32:35 +0200 Subject: [PATCH 1/4] improve state sync error handling --- .../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 +-- .../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 | 64 ++++++++++------ test/functional/services/toasts.ts | 10 +++ x-pack/plugins/lens/public/app_plugin/app.tsx | 2 + .../maps/public/routing/maps_router.js | 13 +++- .../monitoring/public/angular/app_modules.ts | 11 ++- x-pack/plugins/monitoring/public/url_state.ts | 8 +- 22 files changed, 349 insertions(+), 47 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/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/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 fd9a67599414f..3299319e613a0 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..29eb87816bc6c 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; @@ -122,7 +123,7 @@ export default function ({ getService, getPageObjects }) { }); }); - describe('shared links with state in sessionStorage', async () => { + describe.only('shared links with state in sessionStorage', async () => { let teardown; before(async function () { teardown = await setup({ storeStateInSessionStorage: true }); @@ -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 ab4c4820315ac..9ff8bacc83da1 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, 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; From e415a15d8faa250dc9af8b90215da404d8be6637 Mon Sep 17 00:00:00 2001 From: Anton Dosov Date: Tue, 4 Aug 2020 17:32:57 +0200 Subject: [PATCH 2/4] fix --- test/functional/apps/discover/_shared_links.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/functional/apps/discover/_shared_links.js b/test/functional/apps/discover/_shared_links.js index 29eb87816bc6c..94409a94e9257 100644 --- a/test/functional/apps/discover/_shared_links.js +++ b/test/functional/apps/discover/_shared_links.js @@ -123,7 +123,7 @@ export default function ({ getService, getPageObjects }) { }); }); - describe.only('shared links with state in sessionStorage', async () => { + describe('shared links with state in sessionStorage', async () => { let teardown; before(async function () { teardown = await setup({ storeStateInSessionStorage: true }); From 92cc08733366f4a8d82bc19605b649e22c81dbf6 Mon Sep 17 00:00:00 2001 From: Anton Dosov Date: Wed, 5 Aug 2020 10:39:57 +0200 Subject: [PATCH 3/4] update docs --- ...kibana_utils-public-state_sync.createkbnurlstatestorage.md | 4 +++- src/plugins/kibana_utils/public/state_sync/public.api.md | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) 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/kibana_utils/public/state_sync/public.api.md b/src/plugins/kibana_utils/public/state_sync/public.api.md index c174ba798d01a..64d9b7bb026ea 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" From c1fe5afead90f9cf74959f56379e36f1e503bf0c Mon Sep 17 00:00:00 2001 From: Anton Dosov Date: Thu, 6 Aug 2020 13:42:56 +0200 Subject: [PATCH 4/4] fix lint --- x-pack/plugins/lens/public/app_plugin/app.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/x-pack/plugins/lens/public/app_plugin/app.tsx b/x-pack/plugins/lens/public/app_plugin/app.tsx index 3460cc1ac440b..4a6dbd4a91fbf 100644 --- a/x-pack/plugins/lens/public/app_plugin/app.tsx +++ b/x-pack/plugins/lens/public/app_plugin/app.tsx @@ -168,6 +168,7 @@ export function App({ }, [ data.query.filterManager, data.query.timefilter.timefilter, + core.notifications.toasts, core.uiSettings, data.query, history,