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

Implementing ReferenceOrValueEmbeddable for visualize embeddable #76088

Merged
merged 19 commits into from
Sep 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 @@ -57,7 +57,7 @@ export interface AttributeServiceOptions<A extends { title: string }> {
type: string,
attributes: A,
savedObjectId?: string
) => Promise<{ id: string }>;
) => Promise<{ id?: string } | { error: Error }>;
customUnwrapMethod?: (savedObject: SimpleSavedObject<A>) => A;
}

Expand Down Expand Up @@ -124,7 +124,10 @@ export class AttributeService<
newAttributes,
savedObjectId
);
return { ...originalInput, savedObjectId: savedItem.id } as RefType;
if ('id' in savedItem) {
return { ...originalInput, savedObjectId: savedItem.id } as RefType;
}
return { ...originalInput } as RefType;
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 wondering about this case for when there isn't an id in the returned saved item - is this part of the error case that you've added to the API? If so, shouldn't we return the error?

Copy link
Contributor Author

@majagrubic majagrubic Sep 14, 2020

Choose a reason for hiding this comment

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

The id check here is to make typescript happy. But good point that we need error handling for the scenario where saving a saved object fails

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay yes, makes sense. You need the check because you changed the signature of the custom save method to send back an error case.

}

if (savedObjectId) {
Expand Down Expand Up @@ -208,7 +211,6 @@ export class AttributeService<
return { error };
}
};

if (saveOptions && (saveOptions as { showSaveModal: boolean }).showSaveModal) {
this.showSaveModal(
<SavedObjectSaveModal
Expand Down
35 changes: 35 additions & 0 deletions src/plugins/dashboard/public/mocks.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { DashboardStart } from './plugin';

export type Start = jest.Mocked<DashboardStart>;

const createStartContract = (): DashboardStart => {
// @ts-ignore
const startContract: DashboardStart = {
getAttributeService: jest.fn(),
};

return startContract;
};

export const dashboardPluginMock = {
createStartContract,
};
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

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
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"]
}
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,18 @@ import {
} from '../services';
import { VisualizeEmbeddableFactoryDeps } from './visualize_embeddable_factory';
import { VISUALIZE_ENABLE_LABS_SETTING } from '../../common/constants';
import { SavedVisualizationsLoader } from '../saved_visualizations';
import { AttributeService } from '../../../dashboard/public';

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

return new VisualizeEmbeddable(
getTimeFilter(),
{
Expand All @@ -66,6 +81,8 @@ export const createVisEmbeddableFromObject = (deps: VisualizeEmbeddableFactoryDe
deps,
},
input,
attributeService,
savedVisualizationsLoader,
parent
);
} catch (e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import _, { get } from 'lodash';
import { Subscription } from 'rxjs';
import * as Rx from 'rxjs';
import { i18n } from '@kbn/i18n';
import { VISUALIZE_EMBEDDABLE_TYPE } from './constants';
import {
IIndexPattern,
Expand All @@ -35,6 +36,8 @@ import {
Embeddable,
IContainer,
Adapters,
SavedObjectEmbeddableInput,
ReferenceOrValueEmbeddable,
} from '../../../../plugins/embeddable/public';
import {
IExpressionLoaderParams,
Expand All @@ -47,6 +50,10 @@ 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';

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

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

export type VisualizeSavedObjectAttributes = SavedObjectAttributes & {
title: string;
vis?: Vis;
savedVis?: VisSavedObject;
};
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 +110,23 @@ export class VisualizeEmbeddable extends Embeddable<VisualizeInput, VisualizeOut
private abortController?: AbortController;
private readonly deps: VisualizeEmbeddableFactoryDeps;
private readonly inspectorAdapters?: Adapters;
private attributeService?: AttributeService<
VisualizeSavedObjectAttributes,
VisualizeByValueInput,
VisualizeByReferenceInput
>;
private savedVisualizationsLoader?: SavedVisualizationsLoader;

constructor(
timefilter: TimefilterContract,
{ vis, editPath, editUrl, indexPatterns, editable, deps }: VisualizeEmbeddableConfiguration,
initialInput: VisualizeInput,
attributeService?: AttributeService<
VisualizeSavedObjectAttributes,
VisualizeByValueInput,
VisualizeByReferenceInput
>,
savedVisualizationsLoader?: SavedVisualizationsLoader,
parent?: IContainer
) {
super(
Expand All @@ -118,6 +147,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 +412,52 @@ export class VisualizeEmbeddable extends Embeddable<VisualizeInput, VisualizeOut
public supportedTriggers(): TriggerId[] {
return this.vis.type.getSupportedTriggers?.() ?? [];
}

inputIsRefType = (input: VisualizeInput): input is VisualizeByReferenceInput => {
if (!this.attributeService) {
throw new Error('AttributeService must be defined for getInputAsRefType');
}
return this.attributeService.inputIsRefType(input as VisualizeByReferenceInput);
};

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(),
};
if (this.getTitle()) {
input.savedVis.title = this.getTitle();
}
delete input.savedVis.id;
return new Promise<VisualizeByValueInput>((resolve) => {
resolve({ ...(input as VisualizeByValueInput) });
});
};

getInputAsRefType = async (): Promise<VisualizeByReferenceInput> => {
const savedVis = await this.savedVisualizationsLoader?.get({});
if (!savedVis) {
throw new Error('Error creating a saved vis object');
}
if (!this.attributeService) {
throw new Error('AttributeService must be defined for getInputAsRefType');
}
const saveModalTitle = this.getTitle()
? this.getTitle()
: i18n.translate('visualizations.embeddable.placeholderTitle', {
defaultMessage: 'Placeholder Title',
});
// @ts-ignore
const attributes: VisualizeSavedObjectAttributes = {
savedVis,
vis: this.vis,
title: this.vis.title,
};
return this.attributeService.getInputAsRefType(
{
id: this.id,
attributes,
},
{ showSaveModal: true, saveModalTitle }
);
};
}
Loading