From e1b720ad6ac4ffb346ba4f7ccca2ebc1a138c7f4 Mon Sep 17 00:00:00 2001 From: Chris Roberson Date: Mon, 19 Oct 2020 16:53:38 -0400 Subject: [PATCH] [Monitoring] Fix cluster listing page in how it handles global state (#78979) (#81048) * Properly unset global state on the listing page * Fix how we handle global state in getSafeForExternalLink * Fix breadcrumbs for clusters link * Fix tests Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> --- .../components/cluster/listing/listing.js | 2 +- .../plugins/monitoring/public/legacy_shims.ts | 4 +- .../public/lib/get_cluster_from_clusters.js | 6 +- .../lib/get_safe_for_external_link.test.ts | 74 +++++++++++++++++++ .../public/lib/get_safe_for_external_link.ts | 36 ++++++++- .../monitoring/public/lib/route_init.js | 4 +- .../monitoring/public/services/breadcrumbs.js | 7 +- .../public/services/breadcrumbs.test.js | 41 +++++----- .../public/views/cluster/listing/index.js | 6 +- .../monitoring/public/views/loading/index.js | 26 ++++--- 10 files changed, 163 insertions(+), 43 deletions(-) create mode 100644 x-pack/plugins/monitoring/public/lib/get_safe_for_external_link.test.ts diff --git a/x-pack/plugins/monitoring/public/components/cluster/listing/listing.js b/x-pack/plugins/monitoring/public/components/cluster/listing/listing.js index 5173ea89c5dd1..6aa6321ba6081 100644 --- a/x-pack/plugins/monitoring/public/components/cluster/listing/listing.js +++ b/x-pack/plugins/monitoring/public/components/cluster/listing/listing.js @@ -79,7 +79,7 @@ const getColumns = ( if (cluster.isSupported) { return ( {value} diff --git a/x-pack/plugins/monitoring/public/legacy_shims.ts b/x-pack/plugins/monitoring/public/legacy_shims.ts index 488450bafd3a2..bb9f73b5e9ddb 100644 --- a/x-pack/plugins/monitoring/public/legacy_shims.ts +++ b/x-pack/plugins/monitoring/public/legacy_shims.ts @@ -20,6 +20,7 @@ interface BreadcrumbItem { ['data-test-subj']?: string; href?: string; text: string; + ignoreGlobalState?: boolean; } export interface KFetchQuery { @@ -96,9 +97,10 @@ export class Legacy { } breadcrumbs.forEach((breadcrumb: BreadcrumbItem) => { const breadcrumbHref = breadcrumb.href?.split('?')[0]; - if (breadcrumbHref) { + if (breadcrumbHref && !breadcrumb.ignoreGlobalState) { breadcrumb.href = `${breadcrumbHref}?${globalStateStr}`; } + delete breadcrumb.ignoreGlobalState; }); core.chrome.setBreadcrumbs(breadcrumbs.slice(0)); }, diff --git a/x-pack/plugins/monitoring/public/lib/get_cluster_from_clusters.js b/x-pack/plugins/monitoring/public/lib/get_cluster_from_clusters.js index b02c01373c7bb..73422219add95 100644 --- a/x-pack/plugins/monitoring/public/lib/get_cluster_from_clusters.js +++ b/x-pack/plugins/monitoring/public/lib/get_cluster_from_clusters.js @@ -6,7 +6,7 @@ import _ from 'lodash'; -export function getClusterFromClusters(clusters, globalState) { +export function getClusterFromClusters(clusters, globalState, unsetGlobalState = false) { const cluster = (() => { const existingCurrent = _.find(clusters, { cluster_uuid: globalState.cluster_uuid }); if (existingCurrent) { @@ -22,8 +22,8 @@ export function getClusterFromClusters(clusters, globalState) { })(); if (cluster && cluster.license) { - globalState.cluster_uuid = cluster.cluster_uuid; - globalState.ccs = cluster.ccs; + globalState.cluster_uuid = unsetGlobalState ? undefined : cluster.cluster_uuid; + globalState.ccs = unsetGlobalState ? undefined : cluster.ccs; globalState.save(); return cluster; } diff --git a/x-pack/plugins/monitoring/public/lib/get_safe_for_external_link.test.ts b/x-pack/plugins/monitoring/public/lib/get_safe_for_external_link.test.ts new file mode 100644 index 0000000000000..4b40c7f4a88dc --- /dev/null +++ b/x-pack/plugins/monitoring/public/lib/get_safe_for_external_link.test.ts @@ -0,0 +1,74 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { getSafeForExternalLink } from './get_safe_for_external_link'; + +describe('getSafeForExternalLink', () => { + it('should work without a query string', async () => { + const location = { + hash: '', + }; + expect(getSafeForExternalLink('#/overview', {}, location)).toBe('#/overview'); + }); + + it('should work with a query string', async () => { + const location = { + hash: '', + }; + expect(getSafeForExternalLink('#/overview?foo=bar', {}, location)).toBe('#/overview?foo=bar'); + }); + + it('should work without a hash', async () => { + const location = { + hash: undefined, + }; + expect(getSafeForExternalLink('#/overview?foo=bar', {}, location)).toBe('#/overview?foo=bar'); + }); + + it('should work with existing query string', async () => { + const location = { + hash: + '#/overview?_g=(cluster_uuid:ae2womLaSMmZBioEQ9wFjw,refreshInterval:(pause:!t,value:10000),time:(from:now-1h,to:now))', + }; + expect(getSafeForExternalLink('#/overview', {}, location)).toBe( + '#/overview?_g=(cluster_uuid:ae2womLaSMmZBioEQ9wFjw,refreshInterval:(pause:!t,value:10000),time:(from:now-1h,to:now))' + ); + }); + + it('should override global state, for at least cluster_uuid', async () => { + const location = { + hash: `#/home?_g=(cluster_uuid:1,filters:!(),refreshInterval:(pause:!t,value:10000),time:(from:'2017-09-07T20:12:04.011Z',to:'2017-09-07T20:18:55.733Z'))`, + }; + expect( + getSafeForExternalLink( + '#/overview', + { + cluster_uuid: 'NDKg6VXAT6-TaGzEK2Zy7g', + }, + location + ) + ).toBe( + `#/overview?_g=(cluster_uuid:NDKg6VXAT6-TaGzEK2Zy7g,filters:!(),refreshInterval:(pause:!t,value:10000),time:(from:'2017-09-07T20:12:04.011Z',to:'2017-09-07T20:18:55.733Z'))` + ); + }); + + it('should add global state if it does not exist, for at least cluster_uuid', async () => { + const location = { + hash: `#/home?_g=(filters:!(),refreshInterval:(pause:!t,value:10000),time:(from:'2017-09-07T20:12:04.011Z',to:'2017-09-07T20:18:55.733Z'))`, + }; + expect( + getSafeForExternalLink( + '#/overview', + { + cluster_uuid: 'NDKg6VXAT6-TaGzEK2Zy7g', + }, + location + ) + ).toBe( + `#/overview?_g=(filters:!(),refreshInterval:(pause:!t,value:10000),time:(from:'2017-09-07T20:12:04.011Z',to:'2017-09-07T20:18:55.733Z'),cluster_uuid:NDKg6VXAT6-TaGzEK2Zy7g)` + ); + }); +}); diff --git a/x-pack/plugins/monitoring/public/lib/get_safe_for_external_link.ts b/x-pack/plugins/monitoring/public/lib/get_safe_for_external_link.ts index 6f3e398c1414f..3730ed6411227 100644 --- a/x-pack/plugins/monitoring/public/lib/get_safe_for_external_link.ts +++ b/x-pack/plugins/monitoring/public/lib/get_safe_for_external_link.ts @@ -4,6 +4,38 @@ * you may not use this file except in compliance with the Elastic License. */ -export function getSafeForExternalLink(url: string) { - return `${url.split('?')[0]}?${location.hash.split('?')[1]}`; +interface LocationHash { + hash?: string; +} + +export function getSafeForExternalLink( + url: string, + globalState: Record = {}, + location: LocationHash = window.location +) { + let hash = location.hash ? location.hash.split('?')[1] : ''; + const globalStateExecResult = /_g=\((.+)\)$/.exec(hash); + if (!globalStateExecResult || !globalStateExecResult.length) { + if (!hash) { + return url; + } + return `${url.split('?')[0]}?${hash}`; + } + + let newGlobalState = globalStateExecResult[1]; + Object.keys(globalState).forEach((globalStateKey) => { + const keyRegExp = new RegExp(`${globalStateKey}:([^,]+)`); + const execResult = keyRegExp.exec(newGlobalState); + if (execResult && execResult.length) { + newGlobalState = newGlobalState.replace( + execResult[0], + `${globalStateKey}:${globalState[globalStateKey]}` + ); + } else { + newGlobalState += `,${globalStateKey}:${globalState[globalStateKey]}`; + } + }); + + hash = hash.replace(globalStateExecResult[0], `_g=(${newGlobalState})`); + return `${url.split('?')[0]}?${hash}`; } diff --git a/x-pack/plugins/monitoring/public/lib/route_init.js b/x-pack/plugins/monitoring/public/lib/route_init.js index 163688d772022..eebdfa8692f1a 100644 --- a/x-pack/plugins/monitoring/public/lib/route_init.js +++ b/x-pack/plugins/monitoring/public/lib/route_init.js @@ -22,14 +22,14 @@ export function routeInitProvider(Private, monitoringClusters, globalState, lice * the data just has a single cluster or * all the clusters are basic and this is the primary cluster */ - return function routeInit({ codePaths, fetchAllClusters }) { + return function routeInit({ codePaths, fetchAllClusters, unsetGlobalState = false }) { const clusterUuid = fetchAllClusters ? null : globalState.cluster_uuid; return ( monitoringClusters(clusterUuid, undefined, codePaths) // Set the clusters collection and current cluster in globalState .then((clusters) => { const inSetupMode = isInSetupMode(); - const cluster = getClusterFromClusters(clusters, globalState); + const cluster = getClusterFromClusters(clusters, globalState, unsetGlobalState); if (!cluster && !inSetupMode) { window.history.replaceState(null, null, '#/no-data'); return Promise.resolve(); diff --git a/x-pack/plugins/monitoring/public/services/breadcrumbs.js b/x-pack/plugins/monitoring/public/services/breadcrumbs.js index 73ca0eac7b758..1eee32b281ea3 100644 --- a/x-pack/plugins/monitoring/public/services/breadcrumbs.js +++ b/x-pack/plugins/monitoring/public/services/breadcrumbs.js @@ -8,8 +8,8 @@ import { Legacy } from '../legacy_shims'; import { i18n } from '@kbn/i18n'; // Helper for making objects to use in a link element -const createCrumb = (url, label, testSubj) => { - const crumb = { url, label }; +const createCrumb = (url, label, testSubj, ignoreGlobalState = false) => { + const crumb = { url, label, ignoreGlobalState }; if (testSubj) { crumb.testSubj = testSubj; } @@ -177,7 +177,7 @@ export function breadcrumbsProvider() { defaultMessage: 'Clusters', }); - let breadcrumbs = [createCrumb('#/home', homeCrumb, 'breadcrumbClusters')]; + let breadcrumbs = [createCrumb('#/home', homeCrumb, 'breadcrumbClusters', true)]; if (!mainInstance.inOverview && clusterName) { breadcrumbs.push(createCrumb('#/overview', clusterName)); @@ -204,6 +204,7 @@ export function breadcrumbsProvider() { text: b.label, href: b.url, 'data-test-subj': b.testSubj, + ignoreGlobalState: b.ignoreGlobalState, })) ); diff --git a/x-pack/plugins/monitoring/public/services/breadcrumbs.test.js b/x-pack/plugins/monitoring/public/services/breadcrumbs.test.js index 25f8a9e0cd4b1..2e59baac62cbe 100644 --- a/x-pack/plugins/monitoring/public/services/breadcrumbs.test.js +++ b/x-pack/plugins/monitoring/public/services/breadcrumbs.test.js @@ -50,8 +50,8 @@ describe('Monitoring Breadcrumbs Service', () => { }, }); expect(controller.breadcrumbs).to.eql([ - { url: '#/home', label: 'Clusters', testSubj: 'breadcrumbClusters' }, - { url: '#/overview', label: 'test-cluster-foo' }, + { url: '#/home', label: 'Clusters', testSubj: 'breadcrumbClusters', ignoreGlobalState: true }, + { url: '#/overview', label: 'test-cluster-foo', ignoreGlobalState: false }, ]); }); @@ -66,7 +66,7 @@ describe('Monitoring Breadcrumbs Service', () => { }, }); expect(controller.breadcrumbs).to.eql([ - { url: '#/home', label: 'Clusters', testSubj: 'breadcrumbClusters' }, + { url: '#/home', label: 'Clusters', testSubj: 'breadcrumbClusters', ignoreGlobalState: true }, ]); }); @@ -87,11 +87,16 @@ describe('Monitoring Breadcrumbs Service', () => { }, }); expect(controller.breadcrumbs).to.eql([ - { url: '#/home', label: 'Clusters', testSubj: 'breadcrumbClusters' }, - { url: '#/overview', label: 'test-cluster-foo' }, - { url: '#/elasticsearch', label: 'Elasticsearch' }, - { url: '#/elasticsearch/nodes', label: 'Nodes', testSubj: 'breadcrumbEsNodes' }, - { url: null, label: 'es-node-name-01' }, + { url: '#/home', label: 'Clusters', testSubj: 'breadcrumbClusters', ignoreGlobalState: true }, + { url: '#/overview', label: 'test-cluster-foo', ignoreGlobalState: false }, + { url: '#/elasticsearch', label: 'Elasticsearch', ignoreGlobalState: false }, + { + url: '#/elasticsearch/nodes', + label: 'Nodes', + testSubj: 'breadcrumbEsNodes', + ignoreGlobalState: false, + }, + { url: null, label: 'es-node-name-01', ignoreGlobalState: false }, ]); }); @@ -107,9 +112,9 @@ describe('Monitoring Breadcrumbs Service', () => { }, }); expect(controller.breadcrumbs).to.eql([ - { url: '#/home', label: 'Clusters', testSubj: 'breadcrumbClusters' }, - { url: '#/overview', label: 'test-cluster-foo' }, - { url: null, label: 'Kibana' }, + { url: '#/home', label: 'Clusters', testSubj: 'breadcrumbClusters', ignoreGlobalState: true }, + { url: '#/overview', label: 'test-cluster-foo', ignoreGlobalState: false }, + { url: null, label: 'Kibana', ignoreGlobalState: false }, ]); }); @@ -128,9 +133,9 @@ describe('Monitoring Breadcrumbs Service', () => { }, }); expect(controller.breadcrumbs).to.eql([ - { url: '#/home', label: 'Clusters', testSubj: 'breadcrumbClusters' }, - { url: '#/overview', label: 'test-cluster-foo' }, - { url: null, label: 'Logstash' }, + { url: '#/home', label: 'Clusters', testSubj: 'breadcrumbClusters', ignoreGlobalState: true }, + { url: '#/overview', label: 'test-cluster-foo', ignoreGlobalState: false }, + { url: null, label: 'Logstash', ignoreGlobalState: false }, ]); }); @@ -151,10 +156,10 @@ describe('Monitoring Breadcrumbs Service', () => { }, }); expect(controller.breadcrumbs).to.eql([ - { url: '#/home', label: 'Clusters', testSubj: 'breadcrumbClusters' }, - { url: '#/overview', label: 'test-cluster-foo' }, - { url: '#/logstash', label: 'Logstash' }, - { url: '#/logstash/pipelines', label: 'Pipelines' }, + { url: '#/home', label: 'Clusters', testSubj: 'breadcrumbClusters', ignoreGlobalState: true }, + { url: '#/overview', label: 'test-cluster-foo', ignoreGlobalState: false }, + { url: '#/logstash', label: 'Logstash', ignoreGlobalState: false }, + { url: '#/logstash/pipelines', label: 'Pipelines', ignoreGlobalState: false }, ]); }); }); diff --git a/x-pack/plugins/monitoring/public/views/cluster/listing/index.js b/x-pack/plugins/monitoring/public/views/cluster/listing/index.js index dd984f559d469..f3fd6927ca128 100644 --- a/x-pack/plugins/monitoring/public/views/cluster/listing/index.js +++ b/x-pack/plugins/monitoring/public/views/cluster/listing/index.js @@ -26,7 +26,11 @@ uiRoutes resolve: { clusters: (Private) => { const routeInit = Private(routeInitProvider); - return routeInit({ codePaths: CODE_PATHS, fetchAllClusters: true }).then((clusters) => { + return routeInit({ + codePaths: CODE_PATHS, + fetchAllClusters: true, + unsetGlobalState: true, + }).then((clusters) => { if (!clusters || !clusters.length) { window.location.hash = '#/no-data'; return Promise.reject(); diff --git a/x-pack/plugins/monitoring/public/views/loading/index.js b/x-pack/plugins/monitoring/public/views/loading/index.js index 04e0325aa70e2..5ca523899067d 100644 --- a/x-pack/plugins/monitoring/public/views/loading/index.js +++ b/x-pack/plugins/monitoring/public/views/loading/index.js @@ -49,19 +49,21 @@ uiRoutes.when('/loading', { }; const routeInit = Private(routeInitProvider); - routeInit({ codePaths: CODE_PATHS, fetchAllClusters: true }).then((clusters) => { - if (!clusters || !clusters.length) { - window.location.hash = '#/no-data'; - return; - } - if (clusters.length === 1) { - // Bypass the cluster listing if there is just 1 cluster - window.history.replaceState(null, null, '#/overview'); - return; - } + routeInit({ codePaths: CODE_PATHS, fetchAllClusters: true, unsetGlobalState: true }).then( + (clusters) => { + if (!clusters || !clusters.length) { + window.location.hash = '#/no-data'; + return; + } + if (clusters.length === 1) { + // Bypass the cluster listing if there is just 1 cluster + window.history.replaceState(null, null, '#/overview'); + return; + } - window.history.replaceState(null, null, '#/home'); - }); + window.history.replaceState(null, null, '#/home'); + } + ); } }, });