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

Optimize augment-vis saved obj searching by adding arg to saved obj client #4595

Merged
merged 4 commits into from
Jul 20, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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 @@ -471,10 +471,7 @@ describe('SavedObjectsClient', () => {
"fields": Array [
"title",
],
"has_reference": Object {
"id": "1",
"type": "reference",
},
"has_reference": "{\\"id\\":\\"1\\",\\"type\\":\\"reference\\"}",
"page": 10,
"per_page": 100,
"search": "what is the meaning of life?|life",
Expand Down
4 changes: 4 additions & 0 deletions src/core/public/saved_objects/saved_objects_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,10 @@ export class SavedObjectsClient {
const renamedQuery = renameKeys<SavedObjectsFindOptions, any>(renameMap, options);
const query = pick.apply(null, [renamedQuery, ...Object.values<string>(renameMap)]);

// has_reference needs post-processing since it is an object that needs to be read as
// a query param
if (query.has_reference) query.has_reference = JSON.stringify(query.has_reference);

const request: ReturnType<SavedObjectsApi['find']> = this.savedObjectsFetch(path, {
method: 'GET',
query,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,12 @@ export class SavedObjectLoader {
* @param fields
* @returns {Promise}
*/
findAll(search: string = '', size: number = 100, fields?: string[]) {
findAll(
search: string = '',
size: number = 100,
fields?: string[],
hasReference?: SavedObjectsFindOptions['hasReference']
) {
return this.savedObjectsClient
.find<Record<string, unknown>>({
type: this.lowercaseType,
Expand All @@ -138,6 +143,7 @@ export class SavedObjectLoader {
searchFields: ['title^3', 'description'],
defaultSearchOperator: 'AND',
fields,
hasReference,
} as SavedObjectsFindOptions)
.then((resp) => {
return {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,20 @@ jest.mock('src/plugins/vis_augmenter/public/services.ts', () => {
},
};
},
getUISettings: () => {
return {
get: (config: string) => {
switch (config) {
case 'visualization:enablePluginAugmentation':
return true;
case 'visualization:enablePluginAugmentation.maxPluginObjects':
return 10;
default:
throw new Error(`Accessing ${config} is not supported in the mock.`);
}
},
};
},
};
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import { i18n } from '@osd/i18n';
import { EuiIconType } from '@elastic/eui/src/components/icon/icon';
import { Action, IncompatibleActionError } from '../../../ui_actions/public';
import { getAllAugmentVisSavedObjs } from '../utils';
import { getAugmentVisSavedObjs } from '../utils';
import { getSavedAugmentVisLoader } from '../services';
import { SavedObjectDeleteContext } from '../ui_actions_bootstrap';

Expand Down Expand Up @@ -45,12 +45,16 @@
throw new IncompatibleActionError();
}

const loader = getSavedAugmentVisLoader();
const allAugmentVisObjs = await getAllAugmentVisSavedObjs(loader);
const augmentVisIdsToDelete = allAugmentVisObjs
.filter((augmentVisObj) => augmentVisObj.visId === savedObjectId)
.map((augmentVisObj) => augmentVisObj.id as string);

if (!isEmpty(augmentVisIdsToDelete)) loader.delete(augmentVisIdsToDelete);
try {
const loader = getSavedAugmentVisLoader();
const augmentVisObjs = await getAugmentVisSavedObjs(savedObjectId, loader);
const augmentVisIdsToDelete = augmentVisObjs.map(
(augmentVisObj) => augmentVisObj.id as string

Check warning on line 52 in src/plugins/vis_augmenter/public/actions/saved_object_delete_action.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/vis_augmenter/public/actions/saved_object_delete_action.ts#L52

Added line #L52 was not covered by tests
);

if (!isEmpty(augmentVisIdsToDelete)) loader.delete(augmentVisIdsToDelete);
// silently fail. this is because this is doing extra cleanup on objects unrelated
// to the user flow so we dont want to add confusing log lines or error toasts
} catch (e) {} // eslint-disable-line no-empty
ohltyler marked this conversation as resolved.
Show resolved Hide resolved
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*/

import { get, isEmpty } from 'lodash';
import { IUiSettingsClient } from 'opensearch-dashboards/public';
import { IUiSettingsClient, SavedObjectsFindOptions } from 'opensearch-dashboards/public';
import {
SavedObjectLoader,
SavedObjectOpenSearchDashboardsServices,
Expand Down Expand Up @@ -91,9 +91,14 @@ export class SavedObjectLoaderAugmentVis extends SavedObjectLoader {
* @param fields
* @returns {Promise}
*/
findAll(search: string = '', size: number = 100, fields?: string[]) {
findAll(
search: string = '',
size: number = 100,
fields?: string[],
hasReference?: SavedObjectsFindOptions['hasReference']
) {
this.isAugmentationEnabled();
return super.findAll(search, size, fields);
return super.findAll(search, size, fields, hasReference);
}

find(search: string = '', size: number = 100) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,23 +33,24 @@
}
const maxAssociatedCount = config.get(PLUGIN_AUGMENTATION_MAX_OBJECTS_SETTING);

await loader.findAll().then(async (resp) => {
if (resp !== undefined) {
const savedAugmentObjects = get(resp, 'hits', []);
// gets all the saved object for this visualization
const savedObjectsForThisVisualization = savedAugmentObjects.filter(
(savedObj) => get(savedObj, 'visId', '') === AugmentVis.visId
);
await loader

Check warning on line 36 in src/plugins/vis_augmenter/public/saved_augment_vis/utils/helpers.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/vis_augmenter/public/saved_augment_vis/utils/helpers.ts#L36

Added line #L36 was not covered by tests
.findAll('', 100, [], {
type: 'visualization',
id: AugmentVis.visId as string,
})
.then(async (resp) => {
if (resp !== undefined) {
const savedObjectsForThisVisualization = get(resp, 'hits', []);

Check warning on line 43 in src/plugins/vis_augmenter/public/saved_augment_vis/utils/helpers.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/vis_augmenter/public/saved_augment_vis/utils/helpers.ts#L43

Added line #L43 was not covered by tests

if (maxAssociatedCount <= savedObjectsForThisVisualization.length) {
throw new Error(
`Cannot associate the plugin resource to the visualization due to the limit of the max
if (maxAssociatedCount <= savedObjectsForThisVisualization.length) {
throw new Error(

Check warning on line 46 in src/plugins/vis_augmenter/public/saved_augment_vis/utils/helpers.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/vis_augmenter/public/saved_augment_vis/utils/helpers.ts#L46

Added line #L46 was not covered by tests
`Cannot associate the plugin resource to the visualization due to the limit of the max
amount of associated plugin resources (${maxAssociatedCount}) with
${savedObjectsForThisVisualization.length} associated to the visualization`
);
);
}
}
}
});
});

return await loader.get((AugmentVis as any) as Record<string, unknown>);
};
14 changes: 1 addition & 13 deletions src/plugins/vis_augmenter/public/utils/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -304,12 +304,6 @@ describe('utils', () => {
pluginResource
);

it('returns no matching saved objs with filtering', async () => {
const loader = createSavedAugmentVisLoader({
savedObjectsClient: getMockAugmentVisSavedObjectClient([obj1, obj2, obj3]),
} as SavedObjectOpenSearchDashboardsServicesWithAugmentVis);
expect((await getAugmentVisSavedObjs(visId3, loader)).length).toEqual(0);
});
it('returns no matching saved objs when client returns empty list', async () => {
const loader = createSavedAugmentVisLoader({
savedObjectsClient: getMockAugmentVisSavedObjectClient([]),
Expand Down Expand Up @@ -349,18 +343,12 @@ describe('utils', () => {
} as SavedObjectOpenSearchDashboardsServicesWithAugmentVis);
expect((await getAugmentVisSavedObjs(visId1, loader)).length).toEqual(1);
});
it('returns multiple matching saved objs without filtering', async () => {
it('returns multiple matching saved objs', async () => {
const loader = createSavedAugmentVisLoader({
savedObjectsClient: getMockAugmentVisSavedObjectClient([obj1, obj2]),
} as SavedObjectOpenSearchDashboardsServicesWithAugmentVis);
expect((await getAugmentVisSavedObjs(visId1, loader)).length).toEqual(2);
});
it('returns multiple matching saved objs with filtering', async () => {
const loader = createSavedAugmentVisLoader({
savedObjectsClient: getMockAugmentVisSavedObjectClient([obj1, obj2, obj3]),
} as SavedObjectOpenSearchDashboardsServicesWithAugmentVis);
expect((await getAugmentVisSavedObjs(visId1, loader)).length).toEqual(2);
});
});

describe('buildPipelineFromAugmentVisSavedObjs', () => {
Expand Down
17 changes: 5 additions & 12 deletions src/plugins/vis_augmenter/public/utils/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ export const isEligibleForVisLayers = (vis: Vis, uiSettingsClient?: IUiSettingsC

/**
* Using a SavedAugmentVisLoader, fetch all saved objects that are of 'augment-vis' type.
* Filter by vis ID.
* Filter by vis ID by passing in a 'hasReferences' obj with the vis ID to the findAll() fn call.
*/
export const getAugmentVisSavedObjs = async (
visId: string | undefined,
Expand All @@ -69,18 +69,11 @@ export const getAugmentVisSavedObjs = async (
'Visualization augmentation is disabled, please enable visualization:enablePluginAugmentation.'
);
}
const allSavedObjects = await getAllAugmentVisSavedObjs(loader);
return allSavedObjects.filter((hit: ISavedAugmentVis) => hit.visId === visId);
};

/**
* Using a SavedAugmentVisLoader, fetch all saved objects that are of 'augment-vis' type.
*/
export const getAllAugmentVisSavedObjs = async (
loader: SavedAugmentVisLoader | undefined
): Promise<ISavedAugmentVis[]> => {
try {
const resp = await loader?.findAll();
const resp = await loader?.findAll('', 100, [], {
type: 'visualization',
id: visId as string,
});
return (get(resp, 'hits', []) as any[]) as ISavedAugmentVis[];
} catch (e) {
return [] as ISavedAugmentVis[];
Expand Down
Loading