Skip to content

Commit

Permalink
[8.12] [Dashboard] Only cache non-alias dashboards (#175635) (#175706)
Browse files Browse the repository at this point in the history
# Backport

This will backport the following commits from `main` to `8.12`:
- [[Dashboard] Only cache non-alias dashboards
(#175635)](#175635)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Hannah
Mudge","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-01-26T16:09:28Z","message":"[Dashboard]
Only cache non-alias dashboards (#175635)\n\n## Summary\r\n\r\nThis PR
fixes the alias redirect problem that was noted in the
attached\r\nSDH.\r\n\r\nFor context, as part of the [Links
panel\r\nwork](#166896), I added
[dashboard\r\ncaching](#162285) to
make\r\nnavigation more efficient - however, when implementing this, I
did not\r\nconsider the redirect behaviour.\r\n\r\nSpecifically, we were
**always** adding dashboards to the cache, even if\r\nthe load outcome
meant a redirect was necessary - so, because the meta\r\ninfo for the
dashboard fetch result was cached, this meant that the\r\n`'aliasMatch'`
outcome was **also** cached. Because of this, after a\r\nredirect
happened, the second attempt to load the dashboard would return\r\nthe
result from the **cache** rather than fetching the dashboard from\r\nthe
CM client - but, as described previously, the cached dashboard
would\r\nstill appear as though it required a redirect because the
cached outcome\r\nwas `'aliasMatch'`.\r\n\r\nTherefore, because of
hitting this early return on **both** fetch\r\nattempts (before the
redirect, after the
redirect)...\r\n\r\n\r\nhttps://github.com/elastic/kibana/blob/4565b1bfba91cd24f73f8ce37fa7be73283a4453/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts#L165-L171\r\n\r\n...
the dashboard information would not get updated. _(Oof!)_
\r\n\r\nDespite the lengthy description of the problem, the solution is
quite\r\nsimple - just don't add dashboards to the cache if they require
a\r\nredirect. And then everything works! 🎉\r\n\r\n###
Videos\r\n**Before**\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/8698078/983b81a6-acf3-4e71-804a-6ed75c36896f\r\n\r\n\r\n**After**\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/8698078/028369ff-f73f-43e6-9abb-b35f0d2f33e1\r\n\r\n\r\n###
Testing\r\nTo test, I followed the directions from [this
PR\r\ndescription](#163658) to get
a\r\ndashboard that requires a redirect - then, I created a markdown
panel\r\nwith the **old** (from the Default space) dashboard ID in the
new space\r\nto see the same redirect behaviour that the customer in the
SDH was\r\nseeing.\r\n\r\n### Checklist\r\n- [x] This was checked for
[cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n-
[x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n\r\n### For
maintainers\r\n\r\n- [ ] This was checked for breaking API changes and
was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"27b34d591111a878001eaac7b0f3ef0173bc8f02","branchLabelMapping":{"^v8.13.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","Feature:Dashboard","release_note:fix","Team:Presentation","loe:small","impact:high","backport:prev-minor","v8.12.1","v8.13.0"],"title":"[Dashboard]
Only cache non-alias
dashboards","number":175635,"url":"https://github.com/elastic/kibana/pull/175635","mergeCommit":{"message":"[Dashboard]
Only cache non-alias dashboards (#175635)\n\n## Summary\r\n\r\nThis PR
fixes the alias redirect problem that was noted in the
attached\r\nSDH.\r\n\r\nFor context, as part of the [Links
panel\r\nwork](#166896), I added
[dashboard\r\ncaching](#162285) to
make\r\nnavigation more efficient - however, when implementing this, I
did not\r\nconsider the redirect behaviour.\r\n\r\nSpecifically, we were
**always** adding dashboards to the cache, even if\r\nthe load outcome
meant a redirect was necessary - so, because the meta\r\ninfo for the
dashboard fetch result was cached, this meant that the\r\n`'aliasMatch'`
outcome was **also** cached. Because of this, after a\r\nredirect
happened, the second attempt to load the dashboard would return\r\nthe
result from the **cache** rather than fetching the dashboard from\r\nthe
CM client - but, as described previously, the cached dashboard
would\r\nstill appear as though it required a redirect because the
cached outcome\r\nwas `'aliasMatch'`.\r\n\r\nTherefore, because of
hitting this early return on **both** fetch\r\nattempts (before the
redirect, after the
redirect)...\r\n\r\n\r\nhttps://github.com/elastic/kibana/blob/4565b1bfba91cd24f73f8ce37fa7be73283a4453/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts#L165-L171\r\n\r\n...
the dashboard information would not get updated. _(Oof!)_
\r\n\r\nDespite the lengthy description of the problem, the solution is
quite\r\nsimple - just don't add dashboards to the cache if they require
a\r\nredirect. And then everything works! 🎉\r\n\r\n###
Videos\r\n**Before**\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/8698078/983b81a6-acf3-4e71-804a-6ed75c36896f\r\n\r\n\r\n**After**\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/8698078/028369ff-f73f-43e6-9abb-b35f0d2f33e1\r\n\r\n\r\n###
Testing\r\nTo test, I followed the directions from [this
PR\r\ndescription](#163658) to get
a\r\ndashboard that requires a redirect - then, I created a markdown
panel\r\nwith the **old** (from the Default space) dashboard ID in the
new space\r\nto see the same redirect behaviour that the customer in the
SDH was\r\nseeing.\r\n\r\n### Checklist\r\n- [x] This was checked for
[cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n-
[x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n\r\n### For
maintainers\r\n\r\n- [ ] This was checked for breaking API changes and
was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"27b34d591111a878001eaac7b0f3ef0173bc8f02"}},"sourceBranch":"main","suggestedTargetBranches":["8.12"],"targetPullRequestStates":[{"branch":"8.12","label":"v8.12.1","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.13.0","branchLabelMappingKey":"^v8.13.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/175635","number":175635,"mergeCommit":{"message":"[Dashboard]
Only cache non-alias dashboards (#175635)\n\n## Summary\r\n\r\nThis PR
fixes the alias redirect problem that was noted in the
attached\r\nSDH.\r\n\r\nFor context, as part of the [Links
panel\r\nwork](#166896), I added
[dashboard\r\ncaching](#162285) to
make\r\nnavigation more efficient - however, when implementing this, I
did not\r\nconsider the redirect behaviour.\r\n\r\nSpecifically, we were
**always** adding dashboards to the cache, even if\r\nthe load outcome
meant a redirect was necessary - so, because the meta\r\ninfo for the
dashboard fetch result was cached, this meant that the\r\n`'aliasMatch'`
outcome was **also** cached. Because of this, after a\r\nredirect
happened, the second attempt to load the dashboard would return\r\nthe
result from the **cache** rather than fetching the dashboard from\r\nthe
CM client - but, as described previously, the cached dashboard
would\r\nstill appear as though it required a redirect because the
cached outcome\r\nwas `'aliasMatch'`.\r\n\r\nTherefore, because of
hitting this early return on **both** fetch\r\nattempts (before the
redirect, after the
redirect)...\r\n\r\n\r\nhttps://github.com/elastic/kibana/blob/4565b1bfba91cd24f73f8ce37fa7be73283a4453/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts#L165-L171\r\n\r\n...
the dashboard information would not get updated. _(Oof!)_
\r\n\r\nDespite the lengthy description of the problem, the solution is
quite\r\nsimple - just don't add dashboards to the cache if they require
a\r\nredirect. And then everything works! 🎉\r\n\r\n###
Videos\r\n**Before**\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/8698078/983b81a6-acf3-4e71-804a-6ed75c36896f\r\n\r\n\r\n**After**\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/8698078/028369ff-f73f-43e6-9abb-b35f0d2f33e1\r\n\r\n\r\n###
Testing\r\nTo test, I followed the directions from [this
PR\r\ndescription](#163658) to get
a\r\ndashboard that requires a redirect - then, I created a markdown
panel\r\nwith the **old** (from the Default space) dashboard ID in the
new space\r\nto see the same redirect behaviour that the customer in the
SDH was\r\nseeing.\r\n\r\n### Checklist\r\n- [x] This was checked for
[cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n-
[x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n\r\n### For
maintainers\r\n\r\n- [ ] This was checked for breaking API changes and
was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"27b34d591111a878001eaac7b0f3ef0173bc8f02"}}]}]
BACKPORT-->

Co-authored-by: Hannah Mudge <[email protected]>
  • Loading branch information
kibanamachine and Heenawter authored Jan 26, 2024
1 parent 5535353 commit 5ebb15e
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 3 deletions.
Original file line number Diff line number Diff line change
@@ -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();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down

0 comments on commit 5ebb15e

Please sign in to comment.