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

[Vis Builder] Add app filter and query persistence without using state container #3100

Merged
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,10 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [I18n] Register ru, ru-RU locale ([#2817](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2817))
- Add yarn opensearch arg to setup plugin dependencies ([#2544](https://github.com/opensearch-project/OpenSearch-Dashboards/issues/2544))
- [Multi DataSource] Test the connection to an external data source when creating or updating ([#2973](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2973))
- [Doc] Add current plugin persistence implementation readme ([#3081](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3081))
- [Table Visualization] Refactor table visualization using React and DataGrid component ([#2863](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2863))
- [Vis Builder] Add redux store persistence ([#3088](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3088))
- [Multi DataSource] Improve test connection ([#3110](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3110))
- [Vis Builder] Add app filter and query persistence without using state container ([#3100](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3100))
- [Optimizer] Increase timeout waiting for the exiting of an optimizer worker ([#3193](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3193))

### 🐛 Bug Fixes
Expand Down Expand Up @@ -105,6 +105,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Add `current-usage.md` and more details to `README.md` of `charts` plugin ([#2695](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2695))
- [Doc] Add readme for global query persistence ([#3001](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3001))
- Updates NOTICE file, adds validation to GitHub CI ([#3051](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3051))
- [Doc] Add current plugin persistence implementation readme ([#3081](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3081))
abbyhu2000 marked this conversation as resolved.
Show resolved Hide resolved

### 🛠 Maintenance

Expand Down
2 changes: 2 additions & 0 deletions docs/plugins/data_persistence.md
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,8 @@ There are two types for data persistence:

4. In `useEditorUpdates()`, we use the saved appState to load the visualize editor

5. We can also choose to do state sync without using state container by directing hooking up the state managers with the URL data storage. For example, we implemented state management in Vis Builder with redux store, while the global state persistence implementation is the same, we implemented app state persistence without using any state containers or state syncing utils. For the actual visual data, we directly hook up redux store with `OsdUrlStateStorage` to sync the values by using `saveReduxState()` and `loadReduxState()`. For app filter and query, since they are part of the app states but not part of the redux store, we directly hook up their state managers from data plugin with `OsdUrlStateStorage` and `connectStorageToQueryState()`.

# Refresh
When we refresh the page, both app state and global state should persist:

Expand Down
1 change: 1 addition & 0 deletions src/plugins/data/public/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,7 @@ export { Filter, Query, RefreshInterval, TimeRange } from '../common';

export {
createSavedQueryService,
connectStorageToQueryState,
Copy link
Member

Choose a reason for hiding this comment

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

Should this new feature be documented in the previous doc you created about persistence management? We also may need updates to API docs of the data plugin, because we're exposing a new public method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added documentation about this in the plugin persistence readme, docs/plugins/data_persistence.md.

connectToQueryState,
syncQueryStateWithUrl,
QueryState,
Expand Down
14 changes: 14 additions & 0 deletions src/plugins/data/public/public.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,20 @@ export enum BUCKET_TYPES {
// @public
export const castOpenSearchToOsdFieldTypeName: (opensearchType: OPENSEARCH_FIELD_TYPES | string) => OSD_FIELD_TYPES;

// @public
Copy link
Member

Choose a reason for hiding this comment

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

Was this manually entered?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, this is manually entered as i did not find a mechanism to auto generate the api doc. Are you aware of any mechanism that does that?

export const connectStorageToQueryState = (
{
filterManager,
queryString,
state$,
}: Pick<QueryStart | QuerySetup, 'timefilter' | 'filterManager' | 'queryString' | 'state$'>,
OsdUrlStateStorage: IOsdUrlStateStorage,
syncConfig: {
filters: FilterStateStore;
query: boolean;
}
) => () => void;

// Warning: (ae-forgotten-export) The symbol "QuerySetup" needs to be exported by the entry point index.d.ts
// Warning: (ae-forgotten-export) The symbol "BaseStateContainer" needs to be exported by the entry point index.d.ts
// Warning: (ae-missing-release-tag) "connectToQueryState" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
Expand Down
23 changes: 23 additions & 0 deletions src/plugins/data/public/query/state_sync/README.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,26 @@
# Query state syncing utilities

Set of helpers to connect data services to state containers and state syncing utilities

# Connect to query state

This set of functions help sync state storage and state container with query managers.

1. `connectStorageToQueryState()`
- This function take three input parameters: query state managers, `OsdUrlStateStorage`, and the two configs that it helps syncing, `app filters` and `query`
- If `OsdUrlStateStorage` is empty, then we initialize the `OsdUrlStateStorage` using the default app filter and query by calling `getDefaultQuery()` and `getAppFilters()`
- If the current query state and filter state differentiate from the url state storage, we update the filter and query values using state managers for filter and query from the data plugin. This step ensures that if we refresh the page, filter and query still persists their previous values.
- Then we set up subscriptions for both filter and query, so whenever we change the values for either of them, the new state get synced up with `OsdUrlStateStorage`.
- In the return function, we unsubscribe each one of them.

2. `connectToQueryState()`
- This function take three input parameters: query state managers, `state container`, and the four configs that it helps syncing, `filter`, `query`, `time` and `refresh intervals`
- For initial syncing, we get the initial values from the state managers in the data plugin, and we store the values in the state container
- Then we set up subscriptions for each one of the config in data plugin, so whenever we change the values for any one of them, the new state get saved and sync with the state container.
- We also set up subscriptions for the states in state container, so whenever the value in state container get changed, the state managers in the data plugin will also be updated.
- In the return function, we unsubscribe each one of them.

There are a couple differences between the above two functions:
1. `connectStorageToQueryState()` uses `OsdUrlStateStorage` for syncing, while `connectToQueryState()` uses `state container` for syncing, and `state container` is then synced up with `OsdUrlStateStorage`.
2. `connectStorageToQueryState()` can be used for persisting the app states, specifically app filter and query values, while `connectToQueryState()` can be used for persisting both app states and global states, specificlly app filter and query which are part of app states, global filter, time range, time refresh interval which are parts of global states.
Copy link
Member

Choose a reason for hiding this comment

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

Why cant we setup connectStorageToQueryState to also persist global state? is there a limitation? The way i was thinking of these two functions is that the only difference between connectStorageToQueryState and connectToQueryState is that one uses state containers to talk to url storage and the other uses url storage directly. Tomorrow, should we decide to move away from state containers, connectStorageToQueryState can replace that.

Copy link
Member Author

Choose a reason for hiding this comment

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

It might be possible and i will need to look more into that. The only complications i can think of right now is that, for connectStorageToQueryState, it actually sets up a two way listener between those state managers in data plugins and the state container, while in connectStorageToQueryState, it's only a one way listener from the url storage listening on the changes in data plugin state managers. And for global data persistence, i think it might be necessary to set up the two way listener and some additional logics because we want to also listen to the container for the purpose of persisting global data even when we navigate to another previously inactive plugin.

3. `connectStorageToQueryState()` sets up a one way syncing from data to `OsdUrlStateStorage`, while `connectToQueryState()` sets up two-way syncing of the data and `state container`. Both of the functions serve to connect data services to achieve state syncing.
Original file line number Diff line number Diff line change
Expand Up @@ -31,19 +31,32 @@
import { Subscription } from 'rxjs';
import { FilterManager } from '../filter_manager';
import { getFilter } from '../filter_manager/test_helpers/get_stub_filter';
import { Filter, FilterStateStore, UI_SETTINGS } from '../../../common';
import { Filter, FilterStateStore, Query, UI_SETTINGS } from '../../../common';
import { coreMock } from '../../../../../core/public/mocks';
import {
BaseStateContainer,
createStateContainer,
IOsdUrlStateStorage,
createOsdUrlStateStorage,
Storage,
} from '../../../../opensearch_dashboards_utils/public';
import { QueryService, QueryStart } from '../query_service';
import { StubBrowserStorage } from '../../../../../test_utils/public/stub_browser_storage';
import { connectToQueryState } from './connect_to_query_state';
import { connectStorageToQueryState, connectToQueryState } from './connect_to_query_state';
import { TimefilterContract } from '../timefilter';
import { QueryState } from './types';

import { createBrowserHistory, History } from 'history';
import { QueryStringContract } from '../query_string';

const connectStorageToQueryStateFn = (
query: QueryStart,
OsdUrlStateStorage: IOsdUrlStateStorage
) => {
connectStorageToQueryState(query, OsdUrlStateStorage, {
filters: FilterStateStore.APP_STATE,
query: true,
});
};
const connectToQueryGlobalState = (query: QueryStart, state: BaseStateContainer<QueryState>) =>
connectToQueryState(query, state, {
refreshInterval: true,
Expand Down Expand Up @@ -74,6 +87,110 @@ setupMock.uiSettings.get.mockImplementation((key: string) => {
}
});

describe('connect_storage_to_query_state', () => {
abbyhu2000 marked this conversation as resolved.
Show resolved Hide resolved
let queryServiceStart: QueryStart;
let queryString: QueryStringContract;
let queryChangeSub: Subscription;
let queryChangeTriggered = jest.fn();
let filterManager: FilterManager;
let filterManagerChangeSub: Subscription;
let filterManagerChangeTriggered = jest.fn();
let osdUrlStateStorage: IOsdUrlStateStorage;
let history: History;
let gF1: Filter;
let gF2: Filter;
let aF1: Filter;
let aF2: Filter;
let q1: Query;

beforeEach(() => {
const queryService = new QueryService();
queryService.setup({
uiSettings: setupMock.uiSettings,
storage: new Storage(new StubBrowserStorage()),
});
queryServiceStart = queryService.start({
uiSettings: setupMock.uiSettings,
storage: new Storage(new StubBrowserStorage()),
savedObjectsClient: startMock.savedObjects.client,
});

queryString = queryServiceStart.queryString;
queryChangeTriggered = jest.fn();
queryChangeSub = queryString.getUpdates$().subscribe(queryChangeTriggered);

filterManager = queryServiceStart.filterManager;
filterManagerChangeTriggered = jest.fn();
filterManagerChangeSub = filterManager.getUpdates$().subscribe(filterManagerChangeTriggered);

window.location.href = '/';
history = createBrowserHistory();
osdUrlStateStorage = createOsdUrlStateStorage({ useHash: false, history });

gF1 = getFilter(FilterStateStore.GLOBAL_STATE, true, true, 'key1', 'value1');
gF2 = getFilter(FilterStateStore.GLOBAL_STATE, false, false, 'key2', 'value2');
aF1 = getFilter(FilterStateStore.APP_STATE, true, true, 'key3', 'value3');
aF2 = getFilter(FilterStateStore.APP_STATE, false, false, 'key4', 'value4');

q1 = {
query: 'count is less than 100',
language: 'kuery',
};
});

afterEach(() => {
filterManagerChangeSub.unsubscribe();
queryChangeSub.unsubscribe();
});

test('state is initialized with default state', () => {
expect(osdUrlStateStorage.get('_q')).toBeNull();
connectStorageToQueryStateFn(queryServiceStart, osdUrlStateStorage);

expect(osdUrlStateStorage.get('_q')).toEqual({
query: queryString.getDefaultQuery(),
filters: filterManager.getAppFilters(),
});
});

test('state is initialized with URL states', () => {
const initialStates = {
filters: [aF1, aF2],
query: q1,
};
osdUrlStateStorage.set('_q', initialStates, {
replace: true,
});
connectStorageToQueryStateFn(queryServiceStart, osdUrlStateStorage);
expect(filterManager.getFilters().length).toBe(2);
expect(queryString.getQuery()).toStrictEqual(q1);
});

test('when global filter changes, filter in storage should not be updated', () => {
connectStorageToQueryStateFn(queryServiceStart, osdUrlStateStorage);
const previousStorage = osdUrlStateStorage.get('_q');
filterManager.setFilters([gF1, gF1]);
const updatedStorage = osdUrlStateStorage.get('_q');
expect(previousStorage).toStrictEqual(updatedStorage);
});

test('when app filter changes, filter storage should be updated', () => {
connectStorageToQueryStateFn(queryServiceStart, osdUrlStateStorage);
const previousStorage = osdUrlStateStorage.get('_q');
filterManager.setFilters([aF1, aF1]);
const updatedStorage = osdUrlStateStorage.get('_q');
expect(previousStorage).not.toStrictEqual(updatedStorage);
});

test('when query changes, state updates query', () => {
connectStorageToQueryStateFn(queryServiceStart, osdUrlStateStorage);
const previousStorage = osdUrlStateStorage.get('_q');
queryString.setQuery(q1);
const updatedStorage = osdUrlStateStorage.get('_q');
expect(previousStorage).not.toStrictEqual(updatedStorage);
});
});

describe('connect_to_global_state', () => {
let queryServiceStart: QueryStart;
let filterManager: FilterManager;
Expand Down
103 changes: 102 additions & 1 deletion src/plugins/data/public/query/state_sync/connect_to_query_state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,113 @@
import { Subscription } from 'rxjs';
import { filter, map } from 'rxjs/operators';
import _ from 'lodash';
import { BaseStateContainer } from '../../../../opensearch_dashboards_utils/public';
import {
BaseStateContainer,
IOsdUrlStateStorage,
} from '../../../../opensearch_dashboards_utils/public';
import { QuerySetup, QueryStart } from '../query_service';
import { QueryState, QueryStateChange } from './types';
import { FilterStateStore, COMPARE_ALL_OPTIONS, compareFilters } from '../../../common';
import { validateTimeRange } from '../timefilter';

/**
* Helper function to sync up filter and query services in data plugin
* with a URL state storage so plugins can persist the app filter and query
* values across refresh
* @param QueryService: either setup or start
* @param OsdUrlStateStorage to use for syncing and store data
* @param syncConfig app filter and query
*/
export const connectStorageToQueryState = (
abbyhu2000 marked this conversation as resolved.
Show resolved Hide resolved
{
filterManager,
queryString,
state$,
}: Pick<QueryStart | QuerySetup, 'timefilter' | 'filterManager' | 'queryString' | 'state$'>,
OsdUrlStateStorage: IOsdUrlStateStorage,
syncConfig: {
filters: FilterStateStore;
query: boolean;
}
) => {
try {
const syncKeys: Array<keyof QueryStateChange> = [];
if (syncConfig.query) {
syncKeys.push('query');
}
if (syncConfig.filters === FilterStateStore.APP_STATE) {
syncKeys.push('appFilters');
}

const initialStateFromURL: QueryState = OsdUrlStateStorage.get('_q') ?? {
query: queryString.getDefaultQuery(),
filters: filterManager.getAppFilters(),
};

// set up initial '_q' flag in the URL to sync query and filter changes
if (!OsdUrlStateStorage.get('_q')) {
OsdUrlStateStorage.set('_q', initialStateFromURL, {
replace: true,
});
}

if (syncConfig.query && !_.isEqual(initialStateFromURL.query, queryString.getQuery())) {
if (initialStateFromURL.query) {
queryString.setQuery(_.cloneDeep(initialStateFromURL.query));
}
}

if (syncConfig.filters === FilterStateStore.APP_STATE) {
if (
!initialStateFromURL.filters ||
!compareFilters(initialStateFromURL.filters, filterManager.getAppFilters(), {
...COMPARE_ALL_OPTIONS,
state: false,
})
) {
if (initialStateFromURL.filters) {
filterManager.setAppFilters(_.cloneDeep(initialStateFromURL.filters));
}
}

const subs: Subscription[] = [
state$
.pipe(
filter(({ changes }) => {
return syncKeys.some((syncKey) => changes[syncKey]);
}),
map(({ changes, state }) => {
const newState: QueryState = {
query: state.query,
filters: state.filters,
};
if (syncConfig.query && changes.query) {
newState.query = queryString.getQuery();
}

if (syncConfig.filters === FilterStateStore.APP_STATE && changes.appFilters) {
newState.filters = filterManager.getAppFilters();
}

return newState;
})
)
.subscribe((newState) => {
OsdUrlStateStorage.set('_q', newState, {
replace: true,
});
}),
];

return () => {
subs.forEach((s) => s.unsubscribe());
};
}
} catch (err) {
return;
}
};

/**
* Helper to setup two-way syncing of global data and a state container
* @param QueryService: either setup or start
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/data/public/query/state_sync/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,6 @@
* under the License.
*/

export { connectToQueryState } from './connect_to_query_state';
export { connectToQueryState, connectStorageToQueryState } from './connect_to_query_state';
export { syncQueryStateWithUrl } from './sync_state_with_url';
export { QueryState, QueryStateChange } from './types';
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { setEditorState } from '../utils/state_management/metadata_slice';
import { useCanSave } from '../utils/use/use_can_save';
import { saveStateToSavedObject } from '../../saved_visualizations/transforms';
import { TopNavMenuData } from '../../../../navigation/public';
import { opensearchFilters, connectStorageToQueryState } from '../../../../data/public';

export const TopNav = () => {
// id will only be set for the edit route
Expand All @@ -34,6 +35,10 @@ export const TopNav = () => {

const saveDisabledReason = useCanSave();
const savedVisBuilderVis = useSavedVisBuilderVis(visualizationIdFromUrl);
connectStorageToQueryState(services.data.query, services.osdUrlStateStorage, {
Copy link
Member

Choose a reason for hiding this comment

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

What happens in the case where connectStorageToQueryState fails for some reason? It doesn't look like we have any explicit error handling. Does visbuilder still work as expected without the query state connection?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I added a simple try catch wrap to the function. I also tested if connectStorageToQueryState fails for some reason, the persistence won't work, but vis builder still work as expected.

filters: opensearchFilters.FilterStateStore.APP_STATE,
query: true,
});
const { selected: indexPattern } = useIndexPatterns();
const [config, setConfig] = useState<TopNavMenuData[] | undefined>();
const originatingApp = useTypedSelector((state) => {
Expand Down