Skip to content

Commit

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

This PR fixes the alias redirect problem that was noted in the attached
SDH.

For context, as part of the [Links panel
work](#166896), I added [dashboard
caching](#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](#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)
  • Loading branch information
Heenawter authored Jan 26, 2024
1 parent dade4f1 commit 27b34d5
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 27b34d5

Please sign in to comment.