Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Dashboard][Embeddable] Create Explicit Diffing System #121241

Merged
merged 18 commits into from
Jan 11, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
504eea6
initial implementation of explicitInputIsEqual method
ThomThomson Dec 14, 2021
d2b1457
implemented getExplicitInputIsEqual for map embeddable, fixed issues …
ThomThomson Dec 14, 2021
520ad37
add missing await
ThomThomson Dec 14, 2021
91650a4
Merge branch 'main' of github.com:elastic/kibana into embeddable/move…
ThomThomson Dec 15, 2021
ebb8f62
Update src/plugins/embeddable/public/lib/containers/container.ts
ThomThomson Dec 16, 2021
8c66f42
fix map embeddable equality check
nreese Dec 17, 2021
088a182
Merge branch 'main' into embeddable/moveEqualityChecks
kibanamachine Dec 20, 2021
e7c572e
Merge pull request #14 from nreese/unsaved_changes
ThomThomson Dec 20, 2021
a556667
Merge branch 'main' into embeddable/moveEqualityChecks
kibanamachine Dec 21, 2021
cd99627
change variable names to last and current explicit input. Implemented…
ThomThomson Dec 21, 2021
2908043
Use switchmap for previous task cancellation
ThomThomson Dec 22, 2021
aec43a0
Merge branch 'main' into embeddable/moveEqualityChecks
kibanamachine Dec 28, 2021
31af726
Merge branch 'main' into embeddable/moveEqualityChecks
kibanamachine Dec 30, 2021
7524d79
Merge branch 'main' into embeddable/moveEqualityChecks
kibanamachine Jan 4, 2022
aa8eedf
Merge branch 'main' into embeddable/moveEqualityChecks
kibanamachine Jan 6, 2022
81aab35
Merge branch 'main' of github.com:elastic/kibana into embeddable/move…
ThomThomson Jan 6, 2022
7103225
Remove unused title from LensUnwrapMetaInfo
ThomThomson Jan 6, 2022
825e366
Merge branch 'main' into embeddable/moveEqualityChecks
kibanamachine Jan 10, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -92,13 +92,11 @@ export class BookEmbeddable
};

readonly getInputAsValueType = async (): Promise<BookByValueInput> => {
const input = this.attributeService.getExplicitInputFromEmbeddable(this);
return this.attributeService.getInputAsValueType(input);
return this.attributeService.getInputAsValueType(this.getExplicitInput());
};

readonly getInputAsRefType = async (): Promise<BookByReferenceInput> => {
const input = this.attributeService.getExplicitInputFromEmbeddable(this);
return this.attributeService.getInputAsRefType(input, {
return this.attributeService.getInputAsRefType(this.getExplicitInput(), {
showSaveModal: true,
saveModalTitle: this.getTitle(),
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ export const createEditBookAction = (getStartServices: () => Promise<StartServic
const newInput = await attributeService.wrapAttributes(
attributes,
useRefType,
attributeService.getExplicitInputFromEmbeddable(embeddable)
embeddable.getExplicitInput()
);
if (!useRefType && (embeddable.getInput() as SavedObjectEmbeddableInput).savedObjectId) {
// Set the saved object ID to null so that update input will remove the existing savedObjectId...
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -262,9 +262,13 @@ export const useDashboardAppState = ({
dashboardBuildContext.$checkForUnsavedChanges,
])
.pipe(debounceTime(DashboardConstants.CHANGE_CHECK_DEBOUNCE))
.subscribe((states) => {
.subscribe(async (states) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of async piece, I think we should put this logic into switchMap to avoid race conditions

Copy link
Contributor Author

@ThomThomson ThomThomson Dec 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call @Dosant !
I didn't think of it, because the function in its current form is almost synchronous due to the only async piece being the untilEmbeddableLoaded method which is synchronous unless the dashboard is loading.

That said, this could have easily caused race conditions if a downstream implementation used a long-running task in their diffing method. I added a switchmap operator to the pipe and return when the inner observable is closed. I tested by adding a one second delay to the diff_dashboard_state function, firing off many changes in quick succession, and ensuring that only the latest changes are diffed.

Copy link
Contributor

@Dosant Dosant Dec 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe it can be simpler with wrapping promise into an observable using from:

stream$.pipe(switchMap((v) => from(op(v)))).subscribe((v) => {
 // logic
});

const [lastSaved, current] = states;
const unsavedChanges = diffDashboardState(lastSaved, current);
const unsavedChanges = await diffDashboardState({
getEmbeddable: (id: string) => dashboardContainer.untilEmbeddableLoaded(id),
originalState: lastSaved,
newState: current,
});

const savedTimeChanged =
lastSaved.timeRestore &&
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
/*
* 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 { Filter } from '@kbn/es-query';

import { DashboardOptions, DashboardState } from '../../types';
import { diffDashboardState } from './diff_dashboard_state';
import { EmbeddableInput, IEmbeddable, ViewMode } from '../../services/embeddable';

const testFilter: Filter = {
meta: {
alias: null,
disabled: false,
negate: false,
},
query: { query: 'hi' },
};

const getEmbeddable = (id: string) =>
Promise.resolve({
getExplicitInputIsEqual: (previousInput: EmbeddableInput) => true,
} as unknown as IEmbeddable);

const getDashboardState = (state?: Partial<DashboardState>): DashboardState => {
const defaultState: DashboardState = {
description: 'This is a dashboard which is very neat',
query: { query: '', language: 'kql' },
title: 'A very neat dashboard',
viewMode: ViewMode.VIEW,
fullScreenMode: false,
filters: [testFilter],
timeRestore: false,
tags: [],
options: {
hidePanelTitles: false,
useMargins: true,
syncColors: false,
},
panels: {
panel_1: {
type: 'panel_type',
gridData: { w: 0, h: 0, x: 0, y: 0, i: 'panel_1' },
panelRefName: 'panel_panel_1',
explicitInput: {
id: 'panel_1',
},
},
panel_2: {
type: 'panel_type',
gridData: { w: 0, h: 0, x: 0, y: 0, i: 'panel_2' },
panelRefName: 'panel_panel_2',
explicitInput: {
id: 'panel_1',
},
},
},
};
return { ...defaultState, ...state };
};

const getKeysFromDiff = async (partialState?: Partial<DashboardState>): Promise<string[]> =>
Object.keys(
await diffDashboardState({
originalState: getDashboardState(),
newState: getDashboardState(partialState),
getEmbeddable,
})
);

describe('Dashboard state diff function', () => {
it('finds no difference in equal states', async () => {
expect(await getKeysFromDiff()).toEqual([]);
});

it('diffs simple state keys correctly', async () => {
expect(
(
await getKeysFromDiff({
timeRestore: true,
title: 'what a cool new title',
description: 'what a cool new description',
query: { query: 'woah a query', language: 'kql' },
})
).sort()
).toEqual(['description', 'query', 'timeRestore', 'title']);
});

it('picks up differences in dashboard options', async () => {
expect(
await getKeysFromDiff({
options: {
hidePanelTitles: false,
useMargins: false,
syncColors: false,
},
})
).toEqual(['options']);
});

it('considers undefined and false to be equivalent in dashboard options', async () => {
expect(
await getKeysFromDiff({
options: {
useMargins: true,
syncColors: undefined,
} as unknown as DashboardOptions,
})
).toEqual([]);
});

it('calls getExplicitInputIsEqual on each panel', async () => {
const mockedGetEmbeddable = jest.fn().mockImplementation((id) => getEmbeddable(id));
await diffDashboardState({
originalState: getDashboardState(),
newState: getDashboardState(),
getEmbeddable: mockedGetEmbeddable,
});
expect(mockedGetEmbeddable).toHaveBeenCalledTimes(2);
});

it('short circuits panels comparison when one panel returns false', async () => {
const mockedGetEmbeddable = jest.fn().mockImplementation((id) => {
if (id === 'panel_1') {
return Promise.resolve({
getExplicitInputIsEqual: (previousInput: EmbeddableInput) => false,
} as unknown as IEmbeddable);
}
getEmbeddable(id);
});

await diffDashboardState({
originalState: getDashboardState(),
newState: getDashboardState(),
getEmbeddable: mockedGetEmbeddable,
});
expect(mockedGetEmbeddable).toHaveBeenCalledTimes(1);
});

it('skips individual panel comparisons if panel ids are different', async () => {
const mockedGetEmbeddable = jest.fn().mockImplementation((id) => getEmbeddable(id));
const stateDiff = await diffDashboardState({
originalState: getDashboardState(),
newState: getDashboardState({
panels: {
panel_1: {
type: 'panel_type',
gridData: { w: 0, h: 0, x: 0, y: 0, i: 'panel_1' },
panelRefName: 'panel_panel_1',
explicitInput: {
id: 'panel_1',
},
},
// panel 2 has been deleted
},
}),
getEmbeddable: mockedGetEmbeddable,
});
expect(mockedGetEmbeddable).not.toHaveBeenCalled();
expect(Object.keys(stateDiff)).toEqual(['panels']);
});
});
Loading