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

[VisBuilder ] Fix filter and query bugs in the saved objects #6460

Merged
merged 4 commits into from
Apr 30, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [BUG][Multiple Datasource] Fix on data source selectable and readonly component are not consistent ([#6545]https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6545)
- [BUG][Multiple Datasource] Add validation for title length to be no longer than 32 characters [#6452](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6452))
- [Dev Tool] Add additional themed styles to ace overrides ([#5327](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5327))
- [BUG][VisBuilder] Allow saving and loading filter and query in saved VisBuilder ([#6460](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6460))
ananzh marked this conversation as resolved.
Show resolved Hide resolved

### 🚞 Infrastructure

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,10 @@ describe('getOnSave', () => {
},
],
},
"searchSourceFields": Object {},
"searchSourceFields": Object {
"filter": null,
"query": null,
},
"styleState": "",
"title": "new title",
"version": 0,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ export const getOnSave = (
returnToOrigin: boolean;
newDescription?: string;
}) => {
const { embeddable, toastNotifications, application, history } = services;
const { data, embeddable, toastNotifications, application, history } = services;
const stateTransfer = embeddable.getStateTransfer();

if (!savedVisBuilderVis) {
Expand All @@ -183,6 +183,9 @@ export const getOnSave = (
savedVisBuilderVis.title = newTitle;
savedVisBuilderVis.description = newDescription;
savedVisBuilderVis.copyOnSave = newCopyOnSave;
const searchSourceInstance = savedVisBuilderVis.searchSourceFields;
AMoo-Miki marked this conversation as resolved.
Show resolved Hide resolved
searchSourceInstance.query = data.query.queryString.getQuery() || null;
searchSourceInstance.filter = data.query.filterManager.getFilters() || null;
const newlyCreated = !savedVisBuilderVis.id || savedVisBuilderVis.copyOnSave;

try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
const {
application: { navigateToApp },
chrome,
data,
history,
http: { basePath },
toastNotifications,
Expand All @@ -61,6 +62,19 @@
const { title, state } = getStateFromSavedObject(savedVisBuilderVis);
chrome.setBreadcrumbs(getEditBreadcrumbs(title, navigateToApp));
chrome.docTitle.change(title);
// sync initial app filters from savedObject to filterManager
const filters = savedVisBuilderVis.searchSourceFields.filter;

Check warning on line 66 in src/plugins/vis_builder/public/application/utils/use/use_saved_vis_builder_vis.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/vis_builder/public/application/utils/use/use_saved_vis_builder_vis.ts#L66

Added line #L66 was not covered by tests
const query =
savedVisBuilderVis.searchSourceFields.query || data.query.queryString.getDefaultQuery();
const actualFilters = [];

Check warning on line 69 in src/plugins/vis_builder/public/application/utils/use/use_saved_vis_builder_vis.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/vis_builder/public/application/utils/use/use_saved_vis_builder_vis.ts#L69

Added line #L69 was not covered by tests
if (filters !== undefined) {
const result = typeof filters === 'function' ? filters() : filters;
if (result !== undefined) {
actualFilters.push(...(Array.isArray(result) ? result : [result]));
}
}
ananzh marked this conversation as resolved.
Show resolved Hide resolved
data.query.filterManager.setAppFilters(actualFilters);
data.query.queryString.setQuery(query);

Check warning on line 77 in src/plugins/vis_builder/public/application/utils/use/use_saved_vis_builder_vis.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/vis_builder/public/application/utils/use/use_saved_vis_builder_vis.ts#L76-L77

Added lines #L76 - L77 were not covered by tests

dispatch(setUIStateState(state.ui));
dispatch(setStyleState(state.style));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
getTypeService,
getUIActions,
} from '../plugin_services';
import { PersistedState } from '../../../visualizations/public';
import { PersistedState, prepareJson } from '../../../visualizations/public';
import { VisBuilderSavedVis } from '../saved_visualizations/transforms';
import { handleVisEvent } from '../application/utils/handle_vis_event';
import { VisBuilderEmbeddableFactoryDeps } from './vis_builder_embeddable_factory';
Expand Down Expand Up @@ -246,6 +246,28 @@
this.autoRefreshFetchSubscription.unsubscribe();
}

private async updateExpression() {
// Construct the initial part of the pipeline with context management.
let pipeline = `opensearchDashboards | opensearch_dashboards_context `;

Check warning on line 251 in src/plugins/vis_builder/public/embeddable/vis_builder_embeddable.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/vis_builder/public/embeddable/vis_builder_embeddable.tsx#L251

Added line #L251 was not covered by tests

// Access the query and filters from savedObject if available.
const query = this.savedVis?.searchSourceFields?.query;
const filters = this.savedVis?.searchSourceFields?.filter;

Check warning on line 255 in src/plugins/vis_builder/public/embeddable/vis_builder_embeddable.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/vis_builder/public/embeddable/vis_builder_embeddable.tsx#L254-L255

Added lines #L254 - L255 were not covered by tests

// Append query and filters to the pipeline string if they exist.
if (query) {
pipeline += prepareJson('query', query);

Check warning on line 259 in src/plugins/vis_builder/public/embeddable/vis_builder_embeddable.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/vis_builder/public/embeddable/vis_builder_embeddable.tsx#L259

Added line #L259 was not covered by tests
}
if (filters) {
pipeline += prepareJson('filters', filters);

Check warning on line 262 in src/plugins/vis_builder/public/embeddable/vis_builder_embeddable.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/vis_builder/public/embeddable/vis_builder_embeddable.tsx#L262

Added line #L262 was not covered by tests
}

const currentExpression = (await this.getExpression()) ?? '';

// Replace 'opensearchDashboards' with the constructed pipeline in the existing expression.
return currentExpression.replace('opensearchDashboards', pipeline);

Check warning on line 268 in src/plugins/vis_builder/public/embeddable/vis_builder_embeddable.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/vis_builder/public/embeddable/vis_builder_embeddable.tsx#L268

Added line #L268 was not covered by tests
}

private async updateHandler() {
const expressionParams: IExpressionLoaderParams = {
searchContext: {
Expand All @@ -261,6 +283,8 @@
this.abortController = new AbortController();
const abortController = this.abortController;

this.expression = await this.updateExpression();

Check warning on line 286 in src/plugins/vis_builder/public/embeddable/vis_builder_embeddable.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/vis_builder/public/embeddable/vis_builder_embeddable.tsx#L286

Added line #L286 was not covered by tests

if (this.handler && !abortController.signal.aborted) {
this.handler.update(this.expression, expressionParams);
}
Expand Down Expand Up @@ -296,12 +320,8 @@
dirty = true;
}

if (dirty) {
this.expression = (await this.getExpression()) ?? '';

if (this.handler) {
this.updateHandler();
}
if (this.handler && dirty) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: Why do we need this change? Feels like the original logic was intentional.

Copy link
Member Author

Choose a reason for hiding this comment

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

don't remember why but it is a request change. otherwise will see
Screenshot 2024-04-29 at 1 15 05 PM

this.updateHandler();

Check warning on line 324 in src/plugins/vis_builder/public/embeddable/vis_builder_embeddable.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/vis_builder/public/embeddable/vis_builder_embeddable.tsx#L324

Added line #L324 was not covered by tests
}
}

Expand Down
4 changes: 4 additions & 0 deletions src/plugins/vis_builder/public/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,10 @@

// make sure the index pattern list is up to date
pluginsStart.data.indexPatterns.clearCache();
// make sure the filterManager is refreshed
const filters = pluginsStart.data.query.filterManager.getFilters();
const pinFilters = filters.filter(opensearchFilters.isFilterPinned);

Check warning on line 130 in src/plugins/vis_builder/public/plugin.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/vis_builder/public/plugin.ts#L129-L130

Added lines #L129 - L130 were not covered by tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: pinnedFilters

pluginsStart.data.query.filterManager.setFilters(pinFilters ? pinFilters : []);
// make sure a default index pattern exists
// if not, the page will be redirected to management and visualize won't be rendered
// TODO: Add the redirect
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,17 @@ export const saveStateToSavedObject = (
};

export interface VisBuilderSavedVis
extends Pick<VisBuilderSavedObjectAttributes, 'id' | 'title' | 'description'> {
extends Pick<
VisBuilderSavedObjectAttributes,
'id' | 'title' | 'description' | 'searchSourceFields'
> {
state: RenderState;
}

export const getStateFromSavedObject = (
obj: VisBuilderSavedObjectAttributes
): VisBuilderSavedVis => {
const { id, title, description } = obj;
const { id, title, description, searchSourceFields } = obj;
const styleState = JSON.parse(obj.styleState || '{}');
const uiState = JSON.parse(obj.uiState || '{}');
const vizStateWithoutIndex = JSON.parse(obj.visualizationState || '');
Expand Down Expand Up @@ -74,6 +77,7 @@ export const getStateFromSavedObject = (
id,
title,
description,
searchSourceFields,
state: {
visualization: visualizationState,
style: styleState,
Expand Down
1 change: 1 addition & 0 deletions src/plugins/visualizations/public/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,3 +77,4 @@ export { ExprVisAPIEvents } from './expressions/vis';
export { VisualizationListItem } from './vis_types/vis_type_alias_registry';
export { VISUALIZE_ENABLE_LABS_SETTING } from '../common/constants';
export { createSavedVisLoader } from './saved_visualizations';
export { prepareJson } from './legacy/build_pipeline';
Loading