Skip to content

Commit 1cc7f53

Browse files
DosantDosant
committed
[Search] Fix dashboard embeddables don't refetch on searchSessionId change (#84261)
1 parent 2ebc14e commit 1cc7f53

12 files changed

Lines changed: 154 additions & 35 deletions

File tree

‎docs/development/plugins/embeddable/public/kibana-plugin-plugins-embeddable-public.embeddable.getupdated_.md‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
<!-- Do not edit this file. It is automatically generated by API Documenter. -->
2+
3+
[Home](./index.md) &gt; [kibana-plugin-plugins-embeddable-public](./kibana-plugin-plugins-embeddable-public.md) &gt; [Embeddable](./kibana-plugin-plugins-embeddable-public.embeddable.md) &gt; [getUpdated$](./kibana-plugin-plugins-embeddable-public.embeddable.getupdated_.md)
4+
5+
## Embeddable.getUpdated$() method
6+
7+
Merges input$ and output$ streams and denounces emit till next macro-task Could be useful to batch reactions to input$ and output$ updates that happen separately but synchronously In case corresponding state change triggered `reload` this stream is guarantied to emit later which allows to skip any state handling in case `reload` already handled it
8+
9+
<b>Signature:</b>
10+
11+
```typescript
12+
getUpdated$(): Readonly<Rx.Observable<void>>;
13+
```
14+
<b>Returns:</b>
15+
16+
`Readonly<Rx.Observable<void>>`
17+

‎docs/development/plugins/embeddable/public/kibana-plugin-plugins-embeddable-public.embeddable.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,9 @@ export declare abstract class Embeddable<TEmbeddableInput extends EmbeddableInpu
4444
| [getOutput$()](./kibana-plugin-plugins-embeddable-public.embeddable.getoutput_.md) | | |
4545
| [getRoot()](./kibana-plugin-plugins-embeddable-public.embeddable.getroot.md) | | Returns the top most parent embeddable, or itself if this embeddable is not within a parent. |
4646
| [getTitle()](./kibana-plugin-plugins-embeddable-public.embeddable.gettitle.md) | | |
47+
| [getUpdated$()](./kibana-plugin-plugins-embeddable-public.embeddable.getupdated_.md) | | Merges input$ and output$ streams and denounces emit till next macro-task Could be useful to batch reactions to input$ and output$ updates that happen separately but synchronously In case corresponding state change triggered <code>reload</code> this stream is guarantied to emit later which allows to skip any state handling in case <code>reload</code> already handled it |
4748
| [onFatalError(e)](./kibana-plugin-plugins-embeddable-public.embeddable.onfatalerror.md) | | |
48-
| [reload()](./kibana-plugin-plugins-embeddable-public.embeddable.reload.md) | | Reload will be called when there is a request to refresh the data or view, even if the input data did not change. |
49+
| [reload()](./kibana-plugin-plugins-embeddable-public.embeddable.reload.md) | | Reload will be called when there is a request to refresh the data or view, even if the input data did not change.<!-- -->In case if input data did change and reload is requested input$ and output$ would still emit before <code>reload</code> is called<!-- -->The order would be as follows: input$ output$ reload() \-\-\-- updated$ |
4950
| [render(el)](./kibana-plugin-plugins-embeddable-public.embeddable.render.md) | | |
5051
| [supportedTriggers()](./kibana-plugin-plugins-embeddable-public.embeddable.supportedtriggers.md) | | |
5152
| [updateInput(changes)](./kibana-plugin-plugins-embeddable-public.embeddable.updateinput.md) | | |

‎docs/development/plugins/embeddable/public/kibana-plugin-plugins-embeddable-public.embeddable.reload.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,10 @@
66

77
Reload will be called when there is a request to refresh the data or view, even if the input data did not change.
88

9+
In case if input data did change and reload is requested input$ and output$ would still emit before `reload` is called
10+
11+
The order would be as follows: input$ output$ reload() \-\-\-- updated$
12+
913
<b>Signature:</b>
1014

1115
```typescript

‎src/plugins/dashboard/public/application/dashboard_app_controller.tsx‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -633,9 +633,26 @@ export class DashboardAppController {
633633
removeQueryParam(history, DashboardConstants.SEARCH_SESSION_ID, true);
634634
}
635635

636+
// state keys change in which likely won't need a data fetch
637+
const noRefetchKeys: Array<keyof DashboardContainerInput> = [
638+
'viewMode',
639+
'title',
640+
'description',
641+
'expandedPanelId',
642+
'useMargins',
643+
'isEmbeddedExternally',
644+
'isFullScreenMode',
645+
'isEmptyState',
646+
];
647+
648+
const shouldRefetch = Object.keys(changes).some(
649+
(changeKey) => !noRefetchKeys.includes(changeKey as keyof DashboardContainerInput)
650+
);
651+
636652
dashboardContainer.updateInput({
637653
...changes,
638-
searchSessionId: searchService.session.start(),
654+
// do not start a new session if this is irrelevant state change to prevent excessive searches
655+
...(shouldRefetch && { searchSessionId: searchService.session.start() }),
639656
});
640657
}
641658
};

‎src/plugins/discover/public/application/embeddable/search_embeddable.ts‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
import './search_embeddable.scss';
2020
import angular from 'angular';
2121
import _ from 'lodash';
22-
import * as Rx from 'rxjs';
2322
import { Subscription } from 'rxjs';
2423
import { i18n } from '@kbn/i18n';
2524
import { UiActionsStart, APPLY_FILTER_TRIGGER } from '../../../../ui_actions/public';
@@ -98,6 +97,7 @@ export class SearchEmbeddable
9897
private prevTimeRange?: TimeRange;
9998
private prevFilters?: Filter[];
10099
private prevQuery?: Query;
100+
private prevSearchSessionId?: string;
101101

102102
constructor(
103103
{
@@ -140,7 +140,7 @@ export class SearchEmbeddable
140140
.timefilter.getAutoRefreshFetch$()
141141
.subscribe(this.fetch);
142142

143-
this.subscription = Rx.merge(this.getOutput$(), this.getInput$()).subscribe(() => {
143+
this.subscription = this.getUpdated$().subscribe(() => {
144144
this.panelTitle = this.output.title || '';
145145

146146
if (this.searchScope) {
@@ -262,7 +262,8 @@ export class SearchEmbeddable
262262
}
263263

264264
public reload() {
265-
this.fetch();
265+
if (this.searchScope)
266+
this.pushContainerStateParamsToScope(this.searchScope, { forceFetch: true });
266267
}
267268

268269
private fetch = async () => {
@@ -326,26 +327,31 @@ export class SearchEmbeddable
326327
}
327328
};
328329

329-
private pushContainerStateParamsToScope(searchScope: SearchScope) {
330+
private pushContainerStateParamsToScope(
331+
searchScope: SearchScope,
332+
{ forceFetch = false }: { forceFetch: boolean } = { forceFetch: false }
333+
) {
330334
const isFetchRequired =
331335
!esFilters.onlyDisabledFiltersChanged(this.input.filters, this.prevFilters) ||
332336
!_.isEqual(this.prevQuery, this.input.query) ||
333337
!_.isEqual(this.prevTimeRange, this.input.timeRange) ||
334-
!_.isEqual(searchScope.sort, this.input.sort || this.savedSearch.sort);
338+
!_.isEqual(searchScope.sort, this.input.sort || this.savedSearch.sort) ||
339+
this.prevSearchSessionId !== this.input.searchSessionId;
335340

336341
// If there is column or sort data on the panel, that means the original columns or sort settings have
337342
// been overridden in a dashboard.
338343
searchScope.columns = this.input.columns || this.savedSearch.columns;
339344
searchScope.sort = this.input.sort || this.savedSearch.sort;
340345
searchScope.sharedItemTitle = this.panelTitle;
341346

342-
if (isFetchRequired) {
347+
if (forceFetch || isFetchRequired) {
343348
this.filtersSearchSource!.setField('filter', this.input.filters);
344349
this.filtersSearchSource!.setField('query', this.input.query);
350+
345351
this.prevFilters = this.input.filters;
346352
this.prevQuery = this.input.query;
347353
this.prevTimeRange = this.input.timeRange;
348-
354+
this.prevSearchSessionId = this.input.searchSessionId;
349355
this.fetch();
350356
} else if (this.searchScope) {
351357
// trigger a digest cycle to make sure non-fetch relevant changes are propagated

‎src/plugins/embeddable/public/lib/embeddables/embeddable.test.tsx‎

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919

2020
/* eslint-disable max-classes-per-file */
2121

22-
import { skip } from 'rxjs/operators';
22+
import { skip, take } from 'rxjs/operators';
2323
import { Embeddable } from './embeddable';
2424
import { EmbeddableOutput, EmbeddableInput } from './i_embeddable';
2525
import { ViewMode } from '../types';
@@ -112,3 +112,34 @@ test('updating output state retains instance information', async () => {
112112
expect(outputTest.getOutput().inputUpdatedTimes).toBe(2);
113113
expect(outputTest.getOutput().testClass).toBeInstanceOf(TestClass);
114114
});
115+
116+
test('updated$ called after reload and batches input/output changes', async () => {
117+
const hello = new ContactCardEmbeddable(
118+
{ id: '123', firstName: 'Brienne', lastName: 'Tarth' },
119+
{ execAction: (() => null) as any }
120+
);
121+
122+
const reloadSpy = jest.spyOn(hello, 'reload');
123+
124+
const input$ = hello.getInput$().pipe(skip(1));
125+
let inputEmittedTimes = 0;
126+
input$.subscribe(() => {
127+
inputEmittedTimes++;
128+
});
129+
130+
const updated$ = hello.getUpdated$();
131+
let updatedEmittedTimes = 0;
132+
updated$.subscribe(() => {
133+
updatedEmittedTimes++;
134+
});
135+
const updatedPromise = updated$.pipe(take(1)).toPromise();
136+
137+
hello.updateInput({ nameTitle: 'Sir', lastReloadRequestTime: Date.now() });
138+
expect(reloadSpy).toHaveBeenCalledTimes(1);
139+
expect(inputEmittedTimes).toBe(1);
140+
expect(updatedEmittedTimes).toBe(0);
141+
142+
await updatedPromise;
143+
144+
expect(updatedEmittedTimes).toBe(1);
145+
});

‎src/plugins/embeddable/public/lib/embeddables/embeddable.tsx‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,8 @@
1919

2020
import { cloneDeep, isEqual } from 'lodash';
2121
import * as Rx from 'rxjs';
22-
import { distinctUntilChanged, map } from 'rxjs/operators';
22+
import { merge } from 'rxjs';
23+
import { debounceTime, distinctUntilChanged, map, mapTo, skip } from 'rxjs/operators';
2324
import { RenderCompleteDispatcher } from '../../../../kibana_utils/public';
2425
import { Adapters } from '../types';
2526
import { IContainer } from '../containers';
@@ -104,9 +105,31 @@ export abstract class Embeddable<
104105
/**
105106
* Reload will be called when there is a request to refresh the data or view, even if the
106107
* input data did not change.
108+
*
109+
* In case if input data did change and reload is requested input$ and output$ would still emit before `reload` is called
110+
*
111+
* The order would be as follows:
112+
* input$
113+
* output$
114+
* reload()
115+
* ----
116+
* updated$
107117
*/
108118
public abstract reload(): void;
109119

120+
/**
121+
* Merges input$ and output$ streams and debounces emit till next macro-task.
122+
* Could be useful to batch reactions to input$ and output$ updates that happen separately but synchronously.
123+
* In case corresponding state change triggered `reload` this stream is guarantied to emit later,
124+
* which allows to skip any state handling in case `reload` already handled it.
125+
*/
126+
public getUpdated$(): Readonly<Rx.Observable<void>> {
127+
return merge(this.getInput$().pipe(skip(1)), this.getOutput$().pipe(skip(1))).pipe(
128+
debounceTime(0),
129+
mapTo(undefined)
130+
);
131+
}
132+
110133
public getInput$(): Readonly<Rx.Observable<TEmbeddableInput>> {
111134
return this.input$.asObservable();
112135
}

‎src/plugins/embeddable/public/public.api.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -312,6 +312,7 @@ export abstract class Embeddable<TEmbeddableInput extends EmbeddableInput = Embe
312312
getRoot(): IEmbeddable | IContainer;
313313
// (undocumented)
314314
getTitle(): string;
315+
getUpdated$(): Readonly<Rx.Observable<void>>;
315316
// (undocumented)
316317
readonly id: string;
317318
// (undocumented)

‎src/plugins/visualizations/public/embeddable/visualize_embeddable.ts‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919

2020
import _, { get } from 'lodash';
2121
import { Subscription } from 'rxjs';
22-
import * as Rx from 'rxjs';
2322
import { i18n } from '@kbn/i18n';
2423
import { VISUALIZE_EMBEDDABLE_TYPE } from './constants';
2524
import {
@@ -100,6 +99,7 @@ export class VisualizeEmbeddable
10099
private timeRange?: TimeRange;
101100
private query?: Query;
102101
private filters?: Filter[];
102+
private searchSessionId?: string;
103103
private visCustomizations?: Pick<VisualizeInput, 'vis' | 'table'>;
104104
private subscriptions: Subscription[] = [];
105105
private expression: string = '';
@@ -155,8 +155,11 @@ export class VisualizeEmbeddable
155155
.subscribe(this.updateHandler.bind(this));
156156

157157
this.subscriptions.push(
158-
Rx.merge(this.getOutput$(), this.getInput$()).subscribe(() => {
159-
this.handleChanges();
158+
this.getUpdated$().subscribe(() => {
159+
const isDirty = this.handleChanges();
160+
if (isDirty && this.handler) {
161+
this.updateHandler();
162+
}
160163
})
161164
);
162165

@@ -220,7 +223,7 @@ export class VisualizeEmbeddable
220223
}
221224
}
222225

223-
public async handleChanges() {
226+
private handleChanges(): boolean {
224227
this.transferCustomizationsToUiState();
225228

226229
let dirty = false;
@@ -243,13 +246,16 @@ export class VisualizeEmbeddable
243246
dirty = true;
244247
}
245248

249+
if (this.searchSessionId !== this.input.searchSessionId) {
250+
this.searchSessionId = this.input.searchSessionId;
251+
dirty = true;
252+
}
253+
246254
if (this.vis.description && this.domNode) {
247255
this.domNode.setAttribute('data-description', this.vis.description);
248256
}
249257

250-
if (this.handler && dirty) {
251-
this.updateHandler();
252-
}
258+
return dirty;
253259
}
254260

255261
// this is a hack to make editor still work, will be removed once we clean up editor
@@ -395,6 +401,7 @@ export class VisualizeEmbeddable
395401
}
396402

397403
private handleVisUpdate = async () => {
404+
this.handleChanges();
398405
this.updateHandler();
399406
};
400407

‎x-pack/plugins/lens/public/editor_frame_service/embeddable/embeddable.test.tsx‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,13 +205,19 @@ describe('embeddable', () => {
205205
await embeddable.initializeSavedVis({ id: '123' } as LensEmbeddableInput);
206206
embeddable.render(mountpoint);
207207

208+
expect(expressionRenderer).toHaveBeenCalledTimes(1);
209+
208210
embeddable.updateInput({
209211
timeRange,
210212
query,
211213
filters,
212214
searchSessionId: 'searchSessionId',
213215
});
214216

217+
expect(expressionRenderer).toHaveBeenCalledTimes(1);
218+
219+
await new Promise((resolve) => setTimeout(resolve, 0));
220+
215221
expect(expressionRenderer).toHaveBeenCalledTimes(2);
216222
});
217223

0 commit comments

Comments
 (0)