Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
47 changes: 33 additions & 14 deletions x-pack/plugins/lens/public/persistence/saved_object_store.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,22 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { SavedObjectsClientContract, SavedObjectsBulkUpdateObject } from 'kibana/public';
import { SavedObjectIndexStore } from './saved_object_store';

describe('LensStore', () => {
function testStore(testId?: string) {
const client = {
create: jest.fn(() => Promise.resolve({ id: testId || 'testid' })),
update: jest.fn((_type: string, id: string) => Promise.resolve({ id })),
bulkUpdate: jest.fn(([{ id }]: SavedObjectsBulkUpdateObject[]) =>
Promise.resolve({ savedObjects: [{ id }, { id }] })
),
Copy link
Contributor

@wylieconlon wylieconlon Aug 6, 2020

Choose a reason for hiding this comment

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

Instead of using bulkUpdate, is it more correct to use index? I'm basing this off the docs which say that "update" will perform a merge, but "index" rewrites the whole thing. https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-update.html

It also appears that index is a method supported in the repository_es_client.ts but not necessarily exported by the types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes a lot of sense, thanks. I would like to get a fix out without waiting for changed core apis, do you think this approach is fine as a stopgap measure while working with the platform team to get a better solution?

get: jest.fn(),
};

return {
client,
store: new SavedObjectIndexStore(client),
store: new SavedObjectIndexStore((client as unknown) as SavedObjectsClientContract),
};
}

Expand Down Expand Up @@ -108,19 +111,35 @@ describe('LensStore', () => {
},
});

expect(client.update).toHaveBeenCalledTimes(1);
expect(client.update).toHaveBeenCalledWith('lens', 'Gandalf', {
title: 'Even the very wise cannot see all ends.',
visualizationType: 'line',
expression: '',
state: {
datasourceMetaData: { filterableIndexPatterns: [] },
datasourceStates: { indexpattern: { type: 'index_pattern', indexPattern: 'lotr' } },
visualization: { gear: ['staff', 'pointy hat'] },
query: { query: '', language: 'lucene' },
filters: [],
expect(client.bulkUpdate).toHaveBeenCalledTimes(1);
expect(client.bulkUpdate).toHaveBeenCalledWith([
{
type: 'lens',
id: 'Gandalf',
attributes: {
title: null,
visualizationType: null,
expression: null,
state: null,
},
},
});
{
type: 'lens',
id: 'Gandalf',
attributes: {
title: 'Even the very wise cannot see all ends.',
visualizationType: 'line',
expression: '',
state: {
datasourceMetaData: { filterableIndexPatterns: [] },
datasourceStates: { indexpattern: { type: 'index_pattern', indexPattern: 'lotr' } },
visualization: { gear: ['staff', 'pointy hat'] },
query: { query: '', language: 'lucene' },
filters: [],
},
},
},
]);
});
});

Expand Down
44 changes: 25 additions & 19 deletions x-pack/plugins/lens/public/persistence/saved_object_store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { SavedObjectAttributes } from 'kibana/server';
import { SavedObjectAttributes, SavedObjectsClientContract } from 'kibana/public';
import { Query, Filter } from '../../../../../src/plugins/data/public';

export interface Document {
Expand All @@ -27,20 +27,6 @@ export interface Document {

export const DOC_TYPE = 'lens';

interface SavedObjectClient {
create: (type: string, object: SavedObjectAttributes) => Promise<{ id: string }>;
update: (type: string, id: string, object: SavedObjectAttributes) => Promise<{ id: string }>;
get: (
type: string,
id: string
) => Promise<{
id: string;
type: string;
attributes: SavedObjectAttributes;
error?: { statusCode: number; message: string };
}>;
}

export interface DocumentSaver {
save: (vis: Document) => Promise<{ id: string }>;
}
Expand All @@ -52,9 +38,9 @@ export interface DocumentLoader {
export type SavedObjectStore = DocumentLoader & DocumentSaver;

export class SavedObjectIndexStore implements SavedObjectStore {
private client: SavedObjectClient;
private client: SavedObjectsClientContract;

constructor(client: SavedObjectClient) {
constructor(client: SavedObjectsClientContract) {
this.client = client;
}

Expand All @@ -63,13 +49,33 @@ export class SavedObjectIndexStore implements SavedObjectStore {
// TODO: SavedObjectAttributes should support this kind of object,
// remove this workaround when SavedObjectAttributes is updated.
const attributes = (rest as unknown) as SavedObjectAttributes;

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

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

// As Lens is using an object to store its attributes, using the update API
// will merge the new attribute object with the old one, not overwriting deleted
// keys. As Lens is using objects as maps in various places, this is a problem because
// deleted subtrees make it back into the object after a load.
// This function fixes this by doing two updates - one to empty out the document setting
// every key to null, and a second one to load the new content.
private async safeUpdate(id: string, attributes: SavedObjectAttributes) {
const resetAttributes: SavedObjectAttributes = {};
Object.keys(attributes).forEach((key) => {
resetAttributes[key] = null;
});
return (
await this.client.bulkUpdate([
{ type: DOC_TYPE, id, attributes: resetAttributes },
{ type: DOC_TYPE, id, attributes },
])
).savedObjects[1];
}

async load(id: string): Promise<Document> {
const { type, attributes, error } = await this.client.get(DOC_TYPE, id);

Expand All @@ -78,7 +84,7 @@ export class SavedObjectIndexStore implements SavedObjectStore {
}

return {
...attributes,
...(attributes as SavedObjectAttributes),
id,
type,
} as Document;
Expand Down