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.

Sign up for GitHub

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

Jump to bottom

Implementing ReferenceOrValueEmbeddable for visualize embeddable #76088

Merged
merged 19 commits into from
Sep 15, 2020
Merged

Implementing ReferenceOrValueEmbeddable for visualize embeddable #76088

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
c014e35
Adding dashboard as dependency to visualize
Aug 23, 2020
dbafccb
Making visualize embeddable as ReferenceOrValueEmbeddable
Aug 23, 2020
7f2e4ad
Adding attribute service to visualize embeddable
Aug 24, 2020
3209dc2
Merge branch 'master' into vis-by-value-or-reference
Aug 25, 2020
9788589
Binding createFromSavedObject method
Aug 26, 2020
2b6f075
Add/remove visualize embeddable from library
Aug 27, 2020
e9dff6f
Removing debugger statement
Aug 27, 2020
3c02a0e
Merge master
Sep 7, 2020
f04c20f
Using custom save method on attribute service
Sep 7, 2020
d9e8bfb
Reverting to not using attribute service
Sep 8, 2020
11e878a
Resetting flag value
Sep 8, 2020
020c104
Fix i18n title and update title
Sep 8, 2020
c04d0f7
Using custom save method on attribute service
Sep 9, 2020
db9adc0
Fixing eslint
Sep 9, 2020
1189f44
Using custom save method in visualize embeddable factory
Sep 10, 2020
3e7668b
Making getInputAsValueType return Promise as it should
Sep 10, 2020
e645b98
Merge branch 'master' into vis-by-value-or-reference
Sep 10, 2020
5843645
Better error handling when saving a visualization fails
Sep 14, 2020
cee9b81
Merge branch 'master' into vis-by-value-or-reference
elasticmachine Sep 14, 2020
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
2 changes: 1 addition & 1 deletion src/plugins/dashboard/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import { schema, TypeOf } from '@kbn/config-schema';

export const configSchema = schema.object({
allowByValueEmbeddables: schema.boolean({ defaultValue: false }),
allowByValueEmbeddables: schema.boolean({ defaultValue: true }),
});

export type ConfigSchema = TypeOf<typeof configSchema>;
65 changes: 34 additions & 31 deletions src/plugins/dashboard/public/attribute_service/attribute_service.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,27 +95,26 @@ export class AttributeService<
: undefined;
if (!useRefType) {
return { attributes: newAttributes } as ValType;
} else {
try {
if (savedObjectId) {
await this.savedObjectsClient.update(this.type, savedObjectId, newAttributes);
return { savedObjectId } as RefType;
} else {
const savedItem = await this.savedObjectsClient.create(this.type, newAttributes);
return { savedObjectId: savedItem.id } as RefType;
}
} catch (error) {
this.toasts.addDanger({
title: i18n.translate('dashboard.attributeService.saveToLibraryError', {
defaultMessage: `Panel was not saved to the library. Error: {errorMessage}`,
values: {
errorMessage: error.message,
},
}),
'data-test-subj': 'saveDashboardFailure',
});
return Promise.reject({ error });
}
try {
if (savedObjectId) {
await this.savedObjectsClient.update(this.type, savedObjectId, newAttributes);
return { savedObjectId } as RefType;
} else {
const savedItem = await this.savedObjectsClient.create(this.type, newAttributes);
return { savedObjectId: savedItem.id } as RefType;
}
} catch (error) {
this.toasts.addDanger({
title: i18n.translate('dashboard.attributeService.saveToLibraryError', {
defaultMessage: `Panel was not saved to the library. Error: {errorMessage}`,
values: {
errorMessage: error.message,
},
}),
'data-test-subj': 'saveDashboardFailure',
});
return Promise.reject({ error });
}
}

Expand Down Expand Up @@ -181,18 +180,22 @@ export class AttributeService<
};

if (saveOptions && (saveOptions as { showSaveModal: boolean }).showSaveModal) {
showSaveModal(
<SavedObjectSaveModal
onSave={onSave}
onClose={() => reject()}
title={input.attributes.title}
showCopyOnSave={false}
objectType={this.type}
showDescription={false}
/>,
this.i18nContext
);
this.showSaveModal(onSave, input.attributes.title, this.type);
}
});
};

public showSaveModal(onSave, title, type) {
showSaveModal(
<SavedObjectSaveModal
onSave={onSave}
onClose={() => reject()}
title={title}
showCopyOnSave={false}
objectType={type}
showDescription={false}
/>,
this.i18nContext
);
}
}
10 changes: 5 additions & 5 deletions src/plugins/embeddable/public/lib/embeddables/default_embeddable_factory_provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,11 @@ export const defaultEmbeddableFactoryProvider = <
getExplicitInput: def.getExplicitInput
? def.getExplicitInput.bind(def)
: () => Promise.resolve({}),
createFromSavedObject:
def.createFromSavedObject ??
((savedObjectId: string, input: Partial<I>, parent?: IContainer) => {
throw new Error(`Creation from saved object not supported by type ${def.type}`);
}),
createFromSavedObject: def.createFromSavedObject
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not quite sure why this change is required, why the bind, and why a ternary here instead of nullish coalescing?

Copy link
Contributor Author

@majagrubic majagrubic Sep 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This wasn't bound properly and was causing an error when creating visualize embeddable from saved object:
https://github.com/elastic/kibana/blob/master/src/plugins/visualizations/public/embeddable/visualize_embeddable_factory.tsx#L108

? def.createFromSavedObject.bind(def)
: (savedObjectId: string, input: Partial<I>, parent?: IContainer) => {
throw new Error(`Creation from saved object not supported by type ${def.type}`);
},
create: def.create.bind(def),
type: def.type,
isEditable: def.isEditable.bind(def),
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/visualizations/kibana.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@
"version": "kibana",
"server": true,
"ui": true,
"requiredPlugins": ["data", "expressions", "uiActions", "embeddable", "usageCollection", "inspector"],
"requiredPlugins": ["data", "expressions", "uiActions", "embeddable", "usageCollection", "inspector", "dashboard"],
Copy link
Member

Choose a reason for hiding this comment

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

Are we certain the only way to address this is to introduce a required dependency between visualizations and dashboard?

This doesn't feel right to me, but I'm also not familiar enough with the AttributeService to understand whether alternatives are possible.

I just wanted to raise the question before we move forward with merging this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey Luke! This is a really good point.

I made the decision to move attributeService out of the embeddable plugin and temporarily into the dashboard plugin in #74302. It was moved because it makes use of the savedObjectsClient, and @streamich and I had a discussion about how best to avoid coupling savedObjects and embeddables.

The attributeService is an additional layer of abstraction on top of savedObjects. It provides a default implementation that embeddables can use to deal more easily with input that can be either 'by value' or 'by reference' . It also provides code for translating between the two types of input.

The dashboard plugin is not its permanent home, however, it needs to stay somewhere until we find a better place for it.

Do you have any thoughts on where it could live?

Copy link
Member

Choose a reason for hiding this comment

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

hmm, right i didn't notice this ...
saved objects is its own plugin, why did we prefer having attribute service in dashboard plugin instead of embeddable ? it seems embeddable specific ? if we don't want to add saved objects dependency to embeddable maybe this should be its own plugin ? shouldn't be a problem now that we can group plugins under domain folders

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Attribute service is indeed embeddable specific, but it's also only used within the dashboard (at least for now). I don't think saved objects plugin is the best place for it, since it's more than just saved objects. If there are major concerns about this living inside the dashboard, I think a separate plugin is the best way to go.
I am going to merge this PR now as much of the logic will remain unaffected regardless of where the attribute service lives. If concerns exists, I will revisit this in a follow-up PR.

"requiredBundles": ["kibanaUtils", "discover", "savedObjects"]
}
24 changes: 23 additions & 1 deletion src/plugins/visualizations/public/embeddable/create_vis_embeddable_from_object.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,13 @@
*/

import { Vis } from '../types';
import { VisualizeInput, VisualizeEmbeddable } from './visualize_embeddable';
import {
VisualizeInput,
VisualizeEmbeddable,
VisualizeByValueInput,
VisualizeByReferenceInput,
VisualizeSavedObjectAttributes,
} from './visualize_embeddable';
import { IContainer, ErrorEmbeddable } from '../../../../plugins/embeddable/public';
import { DisabledLabEmbeddable } from './disabled_lab_embeddable';
import {
Expand All @@ -30,10 +36,14 @@ import {
} from '../services';
import { VisualizeEmbeddableFactoryDeps } from './visualize_embeddable_factory';
import { VISUALIZE_ENABLE_LABS_SETTING } from '../../common/constants';
import { AttributeService } from '../../../dashboard/public';
import { SavedVisualizationsLoader } from '../saved_visualizations';

export const createVisEmbeddableFromObject = (deps: VisualizeEmbeddableFactoryDeps) => async (
vis: Vis,
input: Partial<VisualizeInput> & { id: string },
attributeService: AttributeService,
savedVisualizationsLoader: SavedVisualizationsLoader,
parent?: IContainer
): Promise<VisualizeEmbeddable | ErrorEmbeddable | DisabledLabEmbeddable> => {
const savedVisualizations = getSavedVisualizationsLoader();
Expand All @@ -55,6 +65,16 @@ export const createVisEmbeddableFromObject = (deps: VisualizeEmbeddableFactoryDe
const indexPattern = vis.data.indexPattern;
const indexPatterns = indexPattern ? [indexPattern] : [];
const editable = getCapabilities().visualize.save as boolean;

if (!attributeService) {
const dashboard = deps.start().plugins.dashboard;
attributeService = await dashboard!.getAttributeService<
VisualizeSavedObjectAttributes,
VisualizeByValueInput,
VisualizeByReferenceInput
>('visualization');
}

return new VisualizeEmbeddable(
getTimeFilter(),
{
Expand All @@ -66,6 +86,8 @@ export const createVisEmbeddableFromObject = (deps: VisualizeEmbeddableFactoryDe
deps,
},
input,
attributeService,
savedVisualizationsLoader,
parent
);
} catch (e) {
Expand Down
69 changes: 67 additions & 2 deletions src/plugins/visualizations/public/embeddable/visualize_embeddable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ import {
Embeddable,
IContainer,
Adapters,
SavedObjectEmbeddableInput,
ReferenceOrValueEmbeddable,
} from '../../../../plugins/embeddable/public';
import {
IExpressionLoaderParams,
Expand All @@ -47,6 +49,11 @@ import { getExpressions, getUiActions } from '../services';
import { VIS_EVENT_TO_TRIGGER } from './events';
import { VisualizeEmbeddableFactoryDeps } from './visualize_embeddable_factory';
import { TriggerId } from '../../../ui_actions/public';
import { SavedObjectAttributes } from '../../../../core/types';
import { AttributeService } from '../../../dashboard/public';
import { SavedVisualizationsLoader } from '../saved_visualizations';
import { VisSavedObject } from '../types';
import { OnSaveProps, SaveResult } from '../../../saved_objects/public';

const getKeys = <T extends {}>(o: T): Array<keyof T> => Object.keys(o) as Array<keyof T>;

Expand Down Expand Up @@ -75,9 +82,15 @@ export interface VisualizeOutput extends EmbeddableOutput {
visTypeName: string;
}

export type VisualizeSavedObjectAttributes = SavedObjectAttributes & { title: string };

export type VisualizeByValueInput = { attributes: VisualizeSavedObjectAttributes } & VisualizeInput;
export type VisualizeByReferenceInput = SavedObjectEmbeddableInput & VisualizeInput;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice looking input shapes!


type ExpressionLoader = InstanceType<ExpressionsStart['ExpressionLoader']>;

export class VisualizeEmbeddable extends Embeddable<VisualizeInput, VisualizeOutput> {
export class VisualizeEmbeddable extends Embeddable<VisualizeInput, VisualizeOutput>
implements ReferenceOrValueEmbeddable<VisualizeByValueInput, VisualizeByReferenceInput> {
private handler?: ExpressionLoader;
private timefilter: TimefilterContract;
private timeRange?: TimeRange;
Expand All @@ -93,11 +106,22 @@ export class VisualizeEmbeddable extends Embeddable<VisualizeInput, VisualizeOut
private abortController?: AbortController;
private readonly deps: VisualizeEmbeddableFactoryDeps;
private readonly inspectorAdapters?: Adapters;
private attributeService: AttributeService;

constructor(
timefilter: TimefilterContract,
{ vis, editPath, editUrl, indexPatterns, editable, deps }: VisualizeEmbeddableConfiguration,
{
vis,
editPath,
editUrl,
indexPatterns,
editable,
deps,
visualizations,
}: VisualizeEmbeddableConfiguration,
initialInput: VisualizeInput,
attributeService: AttributeService,
savedVisualizationsLoader?: SavedVisualizationsLoader,
parent?: IContainer
) {
super(
Expand All @@ -118,6 +142,8 @@ export class VisualizeEmbeddable extends Embeddable<VisualizeInput, VisualizeOut
this.vis = vis;
this.vis.uiState.on('change', this.uiStateChangeHandler);
this.vis.uiState.on('reload', this.reload);
this.attributeService = attributeService;
this.savedVisualizationsLoader = savedVisualizationsLoader;

this.autoRefreshFetchSubscription = timefilter
.getAutoRefreshFetch$()
Expand Down Expand Up @@ -381,4 +407,43 @@ export class VisualizeEmbeddable extends Embeddable<VisualizeInput, VisualizeOut
public supportedTriggers(): TriggerId[] {
return this.vis.type.getSupportedTriggers?.() ?? [];
}

inputIsRefType = (input: VisualizeInput): input is VisualizeByReferenceInput => {
return this.attributeService.inputIsRefType(input);
};

getInputAsValueType = async (): Promise<VisualizeByValueInput> => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use the attribute service here as well? The implementation is quite simple, but it could be more consistent that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried, but it's honestly too much hassle. I'd need a customUnwrapMethod and the code becomes so much more complicated. I don't think it's worth the effort.

const input = {
savedVis: this.vis.serialize(),
};
delete input.savedVis.id;
return input;
};

getInputAsRefType = async (): Promise<VisualizeByReferenceInput> => {
return new Promise<RefType>((resolve, reject) => {
const onSave = async (props: OnSaveProps): Promise<SaveResult> => {
majagrubic marked this conversation as resolved.
Show resolved Hide resolved
try {
const savedVis: VisSavedObject = await this.savedVisualizationsLoader.get({});
const saveOptions = {
confirmOverwrite: false,
returnToOrigin: true,
};
savedVis.title = props.newTitle;
savedVis.copyOnSave = props.newCopyOnSave || false;
savedVis.description = props.newDescription || '';
savedVis.searchSourceFields = this.vis?.data.searchSource?.getSerializedFields();
savedVis.visState = this.vis.serialize();
savedVis.uiStateJSON = this.vis.uiState.toString();
const id = await savedVis.save(saveOptions);
resolve({ savedObjectId: id });
return { id };
} catch (error) {
reject(error);
return { error };
}
};
this.attributeService.showSaveModal(onSave, '', 'visualization');
});
};
}
50 changes: 46 additions & 4 deletions src/plugins/visualizations/public/embeddable/visualize_embeddable_factory.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,14 @@ import {
IContainer,
} from '../../../embeddable/public';
import { DisabledLabEmbeddable } from './disabled_lab_embeddable';
import { VisualizeEmbeddable, VisualizeInput, VisualizeOutput } from './visualize_embeddable';
import {
VisualizeByReferenceInput,
VisualizeByValueInput,
VisualizeEmbeddable,
VisualizeInput,
VisualizeOutput,
VisualizeSavedObjectAttributes,
} from './visualize_embeddable';
import { VISUALIZE_EMBEDDABLE_TYPE } from './constants';
import { SerializedVis, Vis } from '../vis';
import {
Expand All @@ -43,13 +50,16 @@ import { createVisEmbeddableFromObject } from './create_vis_embeddable_from_obje
import { StartServicesGetter } from '../../../kibana_utils/public';
import { VisualizationsStartDeps } from '../plugin';
import { VISUALIZE_ENABLE_LABS_SETTING } from '../../common/constants';
import { AttributeService } from '../../../dashboard/public';

interface VisualizationAttributes extends SavedObjectAttributes {
visState: string;
}

export interface VisualizeEmbeddableFactoryDeps {
start: StartServicesGetter<Pick<VisualizationsStartDeps, 'inspector' | 'embeddable'>>;
start: StartServicesGetter<
Pick<VisualizationsStartDeps, 'inspector' | 'embeddable' | 'dashboard'>
>;
}

export class VisualizeEmbeddableFactory
Expand All @@ -62,6 +72,12 @@ export class VisualizeEmbeddableFactory
> {
public readonly type = VISUALIZE_EMBEDDABLE_TYPE;

private attributeService?: AttributeService<
VisualizeSavedObjectAttributes,
VisualizeByValueInput,
VisualizeByReferenceInput
>;

public readonly savedObjectMetaData: SavedObjectMetaData<VisualizationAttributes> = {
name: i18n.translate('visualizations.savedObjectName', { defaultMessage: 'Visualization' }),
includeFields: ['visState'],
Expand Down Expand Up @@ -105,6 +121,19 @@ export class VisualizeEmbeddableFactory
return await this.deps.start().core.application.currentAppId$.pipe(first()).toPromise();
}

private async getAttributeService() {
if (!this.attributeService) {
this.attributeService = await this.deps
.start()
.plugins.dashboard.getAttributeService<
VisualizeSavedObjectAttributes,
VisualizeByValueInput,
VisualizeByReferenceInput
>(this.type);
}
return this.attributeService!;
}

public async createFromSavedObject(
savedObjectId: string,
input: Partial<VisualizeInput> & { id: string },
Expand All @@ -117,7 +146,13 @@ export class VisualizeEmbeddableFactory
const visState = convertToSerializedVis(savedObject);
const vis = new Vis(savedObject.visState.type, visState);
await vis.setState(visState);
return createVisEmbeddableFromObject(this.deps)(vis, input, parent);
return createVisEmbeddableFromObject(this.deps)(
vis,
input,
await this.getAttributeService(),
savedVisualizations,
parent
);
} catch (e) {
console.error(e); // eslint-disable-line no-console
return new ErrorEmbeddable(e, input, parent);
Expand All @@ -131,7 +166,14 @@ export class VisualizeEmbeddableFactory
const visState = input.savedVis;
const vis = new Vis(visState.type, visState);
await vis.setState(visState);
return createVisEmbeddableFromObject(this.deps)(vis, input, parent);
const savedVisualizations = getSavedVisualizationsLoader();
return createVisEmbeddableFromObject(this.deps)(
vis,
input,
await this.getAttributeService(),
savedVisualizations,
parent
);
} else {
showNewVisModal({
originatingApp: await this.getCurrentAppId(),
Expand Down
Loading