From 90735d0a1781c8e523a619c2edcb09d63c7168a1 Mon Sep 17 00:00:00 2001 From: Hannah Mudge Date: Fri, 26 Jan 2024 09:09:28 -0700 Subject: [PATCH] [Dashboard] Only cache non-alias dashboards (#175635) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary This PR fixes the alias redirect problem that was noted in the attached SDH. For context, as part of the [Links panel work](https://github.com/elastic/kibana/pull/166896), I added [dashboard caching](https://github.com/elastic/kibana/pull/162285) to make navigation more efficient - however, when implementing this, I did not consider the redirect behaviour. Specifically, we were **always** adding dashboards to the cache, even if the load outcome meant a redirect was necessary - so, because the meta info for the dashboard fetch result was cached, this meant that the `'aliasMatch'` outcome was **also** cached. Because of this, after a redirect happened, the second attempt to load the dashboard would return the result from the **cache** rather than fetching the dashboard from the CM client - but, as described previously, the cached dashboard would still appear as though it required a redirect because the cached outcome was `'aliasMatch'`. Therefore, because of hitting this early return on **both** fetch attempts (before the redirect, after the redirect)... https://github.com/elastic/kibana/blob/4565b1bfba91cd24f73f8ce37fa7be73283a4453/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts#L165-L171 ... the dashboard information would not get updated. _(Oof!)_ Despite the lengthy description of the problem, the solution is quite simple - just don't add dashboards to the cache if they require a redirect. And then everything works! 🎉 ### Videos **Before** https://github.com/elastic/kibana/assets/8698078/983b81a6-acf3-4e71-804a-6ed75c36896f **After** https://github.com/elastic/kibana/assets/8698078/028369ff-f73f-43e6-9abb-b35f0d2f33e1 ### Testing To test, I followed the directions from [this PR description](https://github.com/elastic/kibana/pull/163658) to get a dashboard that requires a redirect - then, I created a markdown panel with the **old** (from the Default space) dashboard ID in the new space to see the same redirect behaviour that the customer in the SDH was seeing. ### Checklist - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit 27b34d591111a878001eaac7b0f3ef0173bc8f02) --- .../lib/load_dashboard_state.test.ts | 81 +++++++++++++++++++ .../lib/load_dashboard_state.ts | 13 ++- 2 files changed, 91 insertions(+), 3 deletions(-) create mode 100644 src/plugins/dashboard/public/services/dashboard_content_management/lib/load_dashboard_state.test.ts diff --git a/src/plugins/dashboard/public/services/dashboard_content_management/lib/load_dashboard_state.test.ts b/src/plugins/dashboard/public/services/dashboard_content_management/lib/load_dashboard_state.test.ts new file mode 100644 index 000000000000000..b2289ac5ca9753a --- /dev/null +++ b/src/plugins/dashboard/public/services/dashboard_content_management/lib/load_dashboard_state.test.ts @@ -0,0 +1,81 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +import { registry } from '../../plugin_services.stub'; +import { pluginServices } from '../../plugin_services'; +import { getSampleDashboardInput } from '../../../mocks'; +import { loadDashboardState } from './load_dashboard_state'; +import { dashboardContentManagementCache } from '../dashboard_content_management_service'; + +pluginServices.setRegistry(registry.start({})); +const { data, embeddable, contentManagement, savedObjectsTagging } = pluginServices.getServices(); + +const allServices = { + data, + embeddable, + contentManagement, + savedObjectsTagging, +}; + +describe('Load dashboard state', () => { + it('should return cached result if available', async () => { + dashboardContentManagementCache.fetchDashboard = jest.fn().mockImplementation((id: string) => { + return { + item: { + id, + version: 1, + references: [], + type: 'dashboard', + attributes: { + kibanaSavedObjectMeta: { searchSourceJSON: '' }, + title: 'Test dashboard', + }, + }, + meta: {}, + }; + }); + dashboardContentManagementCache.addDashboard = jest.fn(); + contentManagement.client.get = jest.fn(); + + const { id } = getSampleDashboardInput(); + const result = await loadDashboardState({ + id, + ...allServices, + }); + expect(dashboardContentManagementCache.fetchDashboard).toBeCalled(); + expect(dashboardContentManagementCache.addDashboard).not.toBeCalled(); + expect(contentManagement.client.get).not.toBeCalled(); + expect(result).toMatchObject({ + dashboardId: id, + dashboardFound: true, + dashboardInput: { + title: 'Test dashboard', + }, + }); + }); + + it('should not add to cache for alias redirect result', async () => { + dashboardContentManagementCache.fetchDashboard = jest.fn().mockImplementation(() => undefined); + dashboardContentManagementCache.addDashboard = jest.fn(); + contentManagement.client.get = jest.fn().mockImplementation(({ id }) => { + return Promise.resolve({ + item: { id }, + meta: { + outcome: 'aliasMatch', + }, + }); + }); + const { id } = getSampleDashboardInput(); + await loadDashboardState({ + id, + ...allServices, + }); + expect(dashboardContentManagementCache.fetchDashboard).toBeCalled(); + expect(dashboardContentManagementCache.addDashboard).not.toBeCalled(); + }); +}); diff --git a/src/plugins/dashboard/public/services/dashboard_content_management/lib/load_dashboard_state.ts b/src/plugins/dashboard/public/services/dashboard_content_management/lib/load_dashboard_state.ts index c022e05d91d3d9c..5b748c740cea4da 100644 --- a/src/plugins/dashboard/public/services/dashboard_content_management/lib/load_dashboard_state.ts +++ b/src/plugins/dashboard/public/services/dashboard_content_management/lib/load_dashboard_state.ts @@ -63,8 +63,8 @@ export const loadDashboardState = async ({ /** * Load the saved object from Content Management */ - let rawDashboardContent; - let resolveMeta; + let rawDashboardContent: DashboardCrudTypes['GetOut']['item']; + let resolveMeta: DashboardCrudTypes['GetOut']['meta']; const cachedDashboard = dashboardContentManagementCache.fetchDashboard(id); if (cachedDashboard) { @@ -81,8 +81,15 @@ export const loadDashboardState = async ({ throw new SavedObjectNotFound(DASHBOARD_CONTENT_ID, id); }); - dashboardContentManagementCache.addDashboard(result); ({ item: rawDashboardContent, meta: resolveMeta } = result); + const { outcome: loadOutcome } = resolveMeta; + if (loadOutcome !== 'aliasMatch') { + /** + * Only add the dashboard to the cache if it does not require a redirect - otherwise, the meta + * alias info gets cached and prevents the dashboard contents from being updated + */ + dashboardContentManagementCache.addDashboard(result); + } } if (!rawDashboardContent || !rawDashboardContent.version) {