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

[Lens] Remove unnecessary fields and indexing from mappings #43285

Merged
merged 5 commits into from
Aug 15, 2019
Merged

[Lens] Remove unnecessary fields and indexing from mappings #43285

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cd75f01
Change Lens document to no longer store JSON blob
chrisdavies Aug 9, 2019
f9ac2df
Merge branch 'feature/lens' of github.com:elastic/kibana into feature…
chrisdavies Aug 14, 2019
e07c5ed
Tweak mappings, remove activeDatasourceId,
chrisdavies Aug 14, 2019
92486e2
Fix typings and tests
chrisdavies Aug 14, 2019
2d47bc1
Remove uneccessary use of any
chrisdavies Aug 15, 2019
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
9 changes: 4 additions & 5 deletions x-pack/legacy/plugins/lens/mappings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,13 @@
"visualizationType": {
"type": "keyword"
},
"activeDatasourceId": {
"type": "keyword"
},
"state": {
"type": "text"
"enabled": false,
"type": "object"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an elasticsearch mapping, right? state.datasourceMetadata and all of the other object properties seem important to map individually

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 think this approach is the right one. It's a blob as far as elasticsearch is concerned. We don't need to dive into the details of it now. If we ever do, we can migrate and extract state as needed.

},
"expression": {
"type": "text"
"index": false,
"type": "keyword"
}
}
}
Expand Down
7 changes: 0 additions & 7 deletions x-pack/legacy/plugins/lens/public/editor_frame_plugin/editor_frame/editor_frame.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,6 @@ describe('editor_frame', () => {
initialVisualizationId="testVis"
ExpressionRenderer={expressionRendererMock}
doc={{
activeDatasourceId: 'testDatasource',
visualizationType: 'testVis',
title: '',
expression: '',
Expand Down Expand Up @@ -482,7 +481,6 @@ describe('editor_frame', () => {
initialVisualizationId="testVis"
ExpressionRenderer={expressionRendererMock}
doc={{
activeDatasourceId: 'testDatasource',
visualizationType: 'testVis',
title: '',
expression: '',
Expand Down Expand Up @@ -725,7 +723,6 @@ describe('editor_frame', () => {
initialVisualizationId="testVis"
ExpressionRenderer={expressionRendererMock}
doc={{
activeDatasourceId: 'testDatasource',
visualizationType: 'testVis',
title: '',
expression: '',
Expand Down Expand Up @@ -779,7 +776,6 @@ describe('editor_frame', () => {
initialVisualizationId="testVis"
ExpressionRenderer={expressionRendererMock}
doc={{
activeDatasourceId: 'testDatasource',
visualizationType: 'testVis',
title: '',
expression: '',
Expand Down Expand Up @@ -1379,7 +1375,6 @@ describe('editor_frame', () => {
expect(onChange).toHaveBeenNthCalledWith(1, {
indexPatternTitles: ['resolved'],
doc: {
activeDatasourceId: 'testDatasource',
expression: '',
id: undefined,
state: {
Expand All @@ -1397,7 +1392,6 @@ describe('editor_frame', () => {
expect(onChange).toHaveBeenLastCalledWith({
indexPatternTitles: ['resolved'],
doc: {
activeDatasourceId: 'testDatasource',
expression: '',
id: undefined,
state: {
Expand Down Expand Up @@ -1457,7 +1451,6 @@ describe('editor_frame', () => {
expect(onChange).toHaveBeenNthCalledWith(3, {
indexPatternTitles: [],
doc: {
activeDatasourceId: 'testDatasource',
expression: expect.stringContaining('vis "expression"'),
id: undefined,
state: {
Expand Down
1 change: 0 additions & 1 deletion x-pack/legacy/plugins/lens/public/editor_frame_plugin/editor_frame/editor_frame.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,6 @@ export function EditorFrame(props: EditorFrameProps) {
),
visualization,
state,
activeDatasourceId: state.activeDatasourceId!,
framePublicAPI,
});

Expand Down
2 changes: 0 additions & 2 deletions x-pack/legacy/plugins/lens/public/editor_frame_plugin/editor_frame/save.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ describe('save editor frame state', () => {
activeDatasourceId: 'indexpattern',
visualization: { activeId: '2', state: {} },
},
activeDatasourceId: 'indexpattern',
framePublicAPI: {
addNewLayer: jest.fn(),
removeLayers: jest.fn(),
Expand Down Expand Up @@ -71,7 +70,6 @@ describe('save editor frame state', () => {
});

expect(doc).toEqual({
activeDatasourceId: 'indexpattern',
id: undefined,
expression: '',
state: {
Expand Down
3 changes: 0 additions & 3 deletions x-pack/legacy/plugins/lens/public/editor_frame_plugin/editor_frame/save.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,13 @@ export interface Props {
state: EditorFrameState;
visualization: Visualization;
framePublicAPI: FramePublicAPI;
activeDatasourceId: string;
}

export function getSavedObjectFormat({
activeDatasources,
state,
visualization,
framePublicAPI,
activeDatasourceId,
}: Props): Document {
const expression = buildExpression({
visualization,
Expand Down Expand Up @@ -53,7 +51,6 @@ export function getSavedObjectFormat({
type: 'lens',
visualizationType: state.visualization.activeId,
expression: expression ? toExpression(expression) : '',
activeDatasourceId,
state: {
datasourceStates,
datasourceMetaData: {
Expand Down
19 changes: 17 additions & 2 deletions x-pack/legacy/plugins/lens/public/editor_frame_plugin/editor_frame/state_management.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,21 @@ export type Action =
newDatasourceId: string;
};

function getActiveDatasourceIdFromDoc(doc?: Document) {
if (!doc) {
return null;
}

const [initialDatasourceId] = Object.keys(doc.state.datasourceStates);
Copy link
Contributor

Choose a reason for hiding this comment

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

Well you were asking about what the role of activeDatasourceId is, and this is it- even though we only have one datasource now, in the future we're expecting multiple datasources. This is grabbing the first datasource ID in position order, but that's not always going to be meaningful. So my general preference is to keep activeDatasourceID in the mapping for this kind of case

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 think using the first one in the doc is fine. activeDatasourceId is a UI concern and shouldn't be persisted, in my opinion.

return initialDatasourceId || null;
}

function getInitialDatasourceId(props: EditorFrameProps) {
return props.initialDatasourceId
? props.initialDatasourceId
: getActiveDatasourceIdFromDoc(props.doc);
}

export const getInitialState = (props: EditorFrameProps): EditorFrameState => {
const datasourceStates: EditorFrameState['datasourceStates'] = {};

Expand All @@ -81,7 +96,7 @@ export const getInitialState = (props: EditorFrameProps): EditorFrameState => {
return {
title: i18n.translate('xpack.lens.chartTitle', { defaultMessage: 'New visualization' }),
datasourceStates,
activeDatasourceId: props.initialDatasourceId ? props.initialDatasourceId : null,
activeDatasourceId: getInitialDatasourceId(props),
visualization: {
state: null,
activeId: props.initialVisualizationId,
Expand Down Expand Up @@ -124,7 +139,7 @@ export const reducer = (state: EditorFrameState, action: Action): EditorFrameSta
}),
{}
),
activeDatasourceId: action.doc.activeDatasourceId,
activeDatasourceId: getActiveDatasourceIdFromDoc(action.doc),
visualization: {
...state.visualization,
activeId: action.doc.visualizationType,
Expand Down
35 changes: 4 additions & 31 deletions x-pack/legacy/plugins/lens/public/persistence/saved_object_store.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,15 +67,15 @@ describe('LensStore', () => {

expression: '',
activeDatasourceId: 'indexpattern',
state: JSON.stringify({
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

state: {
datasourceMetaData: { filterableIndexPatterns: [] },
datasourceStates: {
indexpattern: { type: 'index_pattern', indexPattern: '.kibana_test' },
},
visualization: { x: 'foo', y: 'baz' },
query: { query: '', language: 'lucene' },
filters: [],
}),
},
});
});

Expand Down Expand Up @@ -117,45 +117,18 @@ describe('LensStore', () => {
visualizationType: 'line',
expression: '',
activeDatasourceId: 'indexpattern',
state: JSON.stringify({
state: {
datasourceMetaData: { filterableIndexPatterns: [] },
datasourceStates: { indexpattern: { type: 'index_pattern', indexPattern: 'lotr' } },
visualization: { gear: ['staff', 'pointy hat'] },
query: { query: '', language: 'lucene' },
filters: [],
}),
},
});
});
});

describe('load', () => {
test('parses the visState', async () => {
const { client, store } = testStore();
client.get = jest.fn(async () => ({
id: 'Paul',
type: 'lens',
attributes: {
title: 'Hope clouds observation.',
visualizationType: 'dune',
state: '{ "datasource": { "giantWorms": true } }',
},
}));
const doc = await store.load('Paul');

expect(doc).toEqual({
id: 'Paul',
type: 'lens',
title: 'Hope clouds observation.',
visualizationType: 'dune',
state: {
datasource: { giantWorms: true },
},
});

expect(client.get).toHaveBeenCalledTimes(1);
expect(client.get).toHaveBeenCalledWith('lens', 'Paul');
});

test('throws if an error is returned', async () => {
const { client, store } = testStore();
client.get = jest.fn(async () => ({
Expand Down
17 changes: 7 additions & 10 deletions x-pack/legacy/plugins/lens/public/persistence/saved_object_store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ export interface Document {
type?: string;
visualizationType: string | null;
title: string;
activeDatasourceId: string;
expression: string;
state: {
datasourceMetaData: {
Expand Down Expand Up @@ -62,18 +61,17 @@ export class SavedObjectIndexStore implements SavedObjectStore {

async save(vis: Document) {
const { id, type, ...rest } = vis;
const attributes = {
...rest,
state: JSON.stringify(rest.state),
};

// Any is the most straighforward way out here, since the saved
// object client doesn't allow unknowns, and we have them in
// our object.
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const attributes: any = rest;
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, the types of the saved object client can be improved in this regard. However we don't need to fall back to disabling linting rules, the "any-less" way of doing that would be

// TODO SavedObjectAttributes should support this kind of object,
// remove this workaround till the type is updated.
const attributes = rest as unknown as SavedObjectAttributes;

const result = await (id
? this.client.update(DOC_TYPE, id, attributes)
: this.client.create(DOC_TYPE, attributes));

return {
...vis,
id: result.id,
};
return { ...vis, id: result.id };
}

async load(id: string): Promise<Document> {
Expand All @@ -87,7 +85,6 @@ export class SavedObjectIndexStore implements SavedObjectStore {
...attributes,
id,
type,
state: JSON.parse(((attributes as unknown) as { state: string }).state as string),
} as Document;
}
}