[VisBuilder] Fixes pipeline aggs (#3137)

* fixes pipeline aggs in visbuilder Signed-off-by: Ashwin P Chandran <[email protected]> * adds changelog Signed-off-by: Ashwin P Chandran <[email protected]> * Adds unit tests Signed-off-by: Ashwin P Chandran <[email protected]> * fixes pipeline aggs in visbuilder Signed-off-by: Ashwin P Chandran <[email protected]> * adds changelog Signed-off-by: Ashwin P Chandran <[email protected]> * Adds unit tests Signed-off-by: Ashwin P Chandran <[email protected]> * fixes unit tests Signed-off-by: Ashwin P Chandran <[email protected]> Signed-off-by: Ashwin P Chandran <[email protected]>
opensearch-project · Jan 10, 2023 · c259952 · c259952
1 parent fba380b
commit c259952
Show file tree

Hide file tree

Showing 21 changed files with 437 additions and 113 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -89,6 +89,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 - [MD] Update dummy url in tests to follow lychee url allowlist ([#3099](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3099))
 - Adds config override to fix obsolete theme:version config value of v8 (beta) rendering issue ([#3045](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3045))
 - [CI] Update test workflow to increase network-timeout for yarn for installing dependencies ([#3118](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3118))
+- [VisBuilder] Fixes pipeline aggs ([#3137](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3137))
 - [Region Maps] Fixes bug that prevents selected join field to be used ([#3213](Fix bug that prevents selected join field to be used))
 
 ### 🚞 Infrastructure

diff --git a/src/plugins/vis_builder/public/application/components/data_tab/secondary_panel.tsx b/src/plugins/vis_builder/public/application/components/data_tab/secondary_panel.tsx
@@ -19,16 +19,16 @@ import {
 import { VisBuilderServices } from '../../../types';
 import { AggParam, IAggType, IFieldParamType } from '../../../../../data/public';
 import { saveDraftAgg, editDraftAgg } from '../../utils/state_management/visualization_slice';
-import { setValidity } from '../../utils/state_management/metadata_slice';
+import { setError } from '../../utils/state_management/metadata_slice';
 import { Storage } from '../../../../../opensearch_dashboards_utils/public';
 
-const EDITOR_KEY = 'CONFIG_PANEL';
+const PANEL_KEY = 'SECONDARY_PANEL';
 
 export function SecondaryPanel() {
   const { draftAgg, aggConfigParams } = useTypedSelector(
     (state) => state.visualization.activeVisualization!
   );
-  const isEditorValid = useTypedSelector((state) => state.metadata.editor.validity[EDITOR_KEY]);
+  const isEditorValid = useTypedSelector((state) => !state.metadata.editor.errors[PANEL_KEY]);
   const [touched, setTouched] = useState(false);
   const dispatch = useTypedDispatch();
   const vizType = useVisualizationType();
@@ -76,9 +76,9 @@ export function SecondaryPanel() {
     (isValid: boolean) => {
       // Set validity state globally
       dispatch(
-        setValidity({
-          key: EDITOR_KEY,
-          valid: isValid,
+        setError({
+          key: PANEL_KEY,
+          error: !isValid,
         })
       );
     },

diff --git a/src/plugins/vis_builder/public/application/components/workspace.tsx b/src/plugins/vis_builder/public/application/components/workspace.tsx
@@ -9,9 +9,9 @@ import React, { FC, useState, useMemo, useEffect, useLayoutEffect } from 'react'
 import { useOpenSearchDashboards } from '../../../../opensearch_dashboards_react/public';
 import { IExpressionLoaderParams } from '../../../../expressions/public';
 import { VisBuilderServices } from '../../types';
-import { validateSchemaState } from '../utils/validate_schema_state';
+import { validateSchemaState, validateAggregations } from '../utils/validations';
 import { useTypedSelector } from '../utils/state_management';
-import { useVisualizationType } from '../utils/use';
+import { useAggs, useVisualizationType } from '../utils/use';
 import { PersistedState } from '../../../../visualizations/public';
 
 import hand_field from '../../assets/hand_field.svg';
@@ -29,6 +29,7 @@ export const Workspace: FC = ({ children }) => {
     },
   } = useOpenSearchDashboards<VisBuilderServices>();
   const { toExpression, ui } = useVisualizationType();
+  const { aggConfigs } = useAggs();
   const [expression, setExpression] = useState<string>();
   const [searchContext, setSearchContext] = useState<IExpressionLoaderParams['searchContext']>({
     query: data.query.queryString.getQuery(),
@@ -42,13 +43,17 @@ export const Workspace: FC = ({ children }) => {
   useEffect(() => {
     async function loadExpression() {
       const schemas = ui.containerConfig.data.schemas;
-      const [valid, errorMsg] = validateSchemaState(schemas, rootState.visualization);
 
-      if (!valid) {
-        if (errorMsg) {
-          toasts.addWarning(errorMsg);
-        }
+      const noAggs = aggConfigs?.aggs?.length === 0;
+      const schemaValidation = validateSchemaState(schemas, rootState.visualization);
+      const aggValidation = validateAggregations(aggConfigs?.aggs || []);
+
+      if (noAggs || !aggValidation.valid || !schemaValidation.valid) {
+        const err = schemaValidation.errorMsg || aggValidation.errorMsg;
+
+        if (err) toasts.addWarning(err);
         setExpression(undefined);
+
         return;
       }
 
@@ -57,7 +62,7 @@ export const Workspace: FC = ({ children }) => {
     }
 
     loadExpression();
-  }, [rootState, toExpression, toasts, ui.containerConfig.data.schemas, searchContext]);
+  }, [rootState, toExpression, toasts, ui.containerConfig.data.schemas, searchContext, aggConfigs]);
 
   useLayoutEffect(() => {
     const subscription = data.query.state$.subscribe(({ state }) => {

diff --git a/src/plugins/vis_builder/public/application/utils/state_management/handlers/editor_state.ts b/src/plugins/vis_builder/public/application/utils/state_management/handlers/editor_state.ts
@@ -0,0 +1,24 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import { setEditorState } from '../metadata_slice';
+import { RootState, Store } from '../store';
+
+export const handlerEditorState = (store: Store, state: RootState, previousState: RootState) => {
+  const { metadata, ...renderState } = state;
+  const { metadata: prevMetadata, ...prevRenderState } = previousState;
+
+  // Need to make sure the editorStates are in the clean states(not the initial states) to indicate the viz finished loading
+  // Because when loading a saved viz from saved object, the previousStore will differ from
+  // the currentStore even tho there is no changes applied ( aggParams will
+  // first be empty, and it then will change to not empty once the viz finished loading)
+  if (
+    prevMetadata.editor.state === 'clean' &&
+    metadata.editor.state === 'clean' &&
+    JSON.stringify(renderState) !== JSON.stringify(prevRenderState)
+  ) {
+    store.dispatch(setEditorState({ state: 'dirty' }));
+  }
+};
diff --git a/src/plugins/vis_builder/public/application/utils/state_management/handlers/parent_aggs.ts b/src/plugins/vis_builder/public/application/utils/state_management/handlers/parent_aggs.ts
@@ -0,0 +1,67 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import { findLast } from 'lodash';
+import { BUCKET_TYPES, IMetricAggType, search } from '../../../../../../data/public';
+import { VisBuilderServices } from '../../../../types';
+import { RootState, Store } from '../store';
+import { setAggParamValue } from '../visualization_slice';
+
+/**
+ * Parent pipeline aggs when combined with histogram aggs need `min_doc_count` to be set appropriately to avoid an error
+ * on opensearch engine https://opensearch.org/docs/2.4/opensearch/pipeline-agg/#parent-aggregations
+ */
+export const handlerParentAggs = async (
+  store: Store,
+  state: RootState,
+  services: VisBuilderServices
+) => {
+  const {
+    visualization: { activeVisualization, indexPattern = '' },
+  } = state;
+
+  const {
+    data: {
+      indexPatterns,
+      search: { aggs: aggService },
+    },
+  } = services;
+
+  if (!activeVisualization) return state;
+
+  const aggConfigs = aggService.createAggConfigs(
+    await indexPatterns.get(indexPattern),
+    activeVisualization.aggConfigParams
+  );
+
+  // Pipeline aggs should have a valid bucket agg
+  const metricAggs = aggConfigs.aggs.filter((agg) => agg.schema === 'metric');
+  const lastParentPipelineAgg = findLast(
+    metricAggs,
+    ({ type }: { type: IMetricAggType }) => type.subtype === search.aggs.parentPipelineType
+  );
+  const lastBucket = findLast(aggConfigs.aggs, (agg) => agg.type.type === 'buckets');
+
+  aggConfigs.aggs.forEach((agg) => {
+    const isLastBucket = lastBucket?.id === agg.id;
+    // When a Parent Pipeline agg is selected and this agg is the last bucket.
+    const isLastBucketAgg = isLastBucket && lastParentPipelineAgg && agg.type;
+
+    if (
+      isLastBucketAgg &&
+      ([BUCKET_TYPES.DATE_HISTOGRAM, BUCKET_TYPES.HISTOGRAM] as any).includes(agg.type.name)
+    ) {
+      store.dispatch(
+        setAggParamValue({
+          aggId: agg.id,
+          paramName: 'min_doc_count',
+          // "histogram" agg has an editor for "min_doc_count" param, which accepts boolean
+          // "date_histogram" agg doesn't have an editor for "min_doc_count" param, it should be set as a numeric value
+          value: agg.type.name === 'histogram' ? true : 0,
+        })
+      );
+    }
+  });
+};
diff --git a/src/plugins/vis_builder/public/application/utils/state_management/metadata_slice.ts b/src/plugins/vis_builder/public/application/utils/state_management/metadata_slice.ts
@@ -15,8 +15,8 @@ type EditorState = 'loading' | 'clean' | 'dirty';
 
 export interface MetadataState {
   editor: {
-    validity: {
-      // Validity for each section in the editor
+    errors: {
+      // Errors for each section in the editor
       [key: string]: boolean;
     };
     state: EditorState;
@@ -26,7 +26,7 @@ export interface MetadataState {
 
 const initialState: MetadataState = {
   editor: {
-    validity: {},
+    errors: {},
     state: 'loading',
   },
   originatingApp: undefined,
@@ -51,9 +51,9 @@ export const slice = createSlice({
   name: 'metadata',
   initialState,
   reducers: {
-    setValidity: (state, action: PayloadAction<{ key: string; valid: boolean }>) => {
-      const { key, valid } = action.payload;
-      state.editor.validity[key] = valid;
+    setError: (state, action: PayloadAction<{ key: string; error: boolean }>) => {
+      const { key, error } = action.payload;
+      state.editor.errors[key] = error;
     },
     setEditorState: (state, action: PayloadAction<{ state: EditorState }>) => {
       state.editor.state = action.payload.state;
@@ -68,4 +68,4 @@ export const slice = createSlice({
 });
 
 export const { reducer } = slice;
-export const { setValidity, setEditorState, setOriginatingApp, setState } = slice.actions;
+export const { setError, setEditorState, setOriginatingApp, setState } = slice.actions;
diff --git a/src/plugins/vis_builder/public/application/utils/state_management/redux_persistence.test.tsx b/src/plugins/vis_builder/public/application/utils/state_management/redux_persistence.test.tsx
@@ -5,8 +5,8 @@
 
 import { VisBuilderServices } from '../../../types';
 import { createVisBuilderServicesMock } from '../mocks';
-import { getPreloadedState } from './preload';
-import { loadReduxState, saveReduxState } from './redux_persistence';
+import { loadReduxState, persistReduxState } from './redux_persistence';
+import { RootState } from './store';
 
 describe('test redux state persistence', () => {
   let mockServices: jest.Mocked<VisBuilderServices>;
@@ -22,15 +22,15 @@ describe('test redux state persistence', () => {
   });
 
   test('test load redux state when url is empty', async () => {
-    const defaultStates = {
+    const defaultStates: RootState = {
       style: 'style default states',
       visualization: {
         searchField: '',
         activeVisualization: { name: 'viz', aggConfigParams: [] },
         indexPattern: 'id',
       },
       metadata: {
-        editor: { validity: {}, state: 'loading' },
+        editor: { errors: {}, state: 'loading' },
         originatingApp: undefined,
       },
     };
@@ -45,8 +45,8 @@ describe('test redux state persistence', () => {
     expect(returnStates).toStrictEqual(reduxStateParams);
   });
 
-  test('test save redux state', () => {
-    saveReduxState(reduxStateParams, mockServices);
+  test('test persist redux state', () => {
+    persistReduxState(reduxStateParams, mockServices);
     const urlStates = mockServices.osdUrlStateStorage.get('_a');
     expect(urlStates).toStrictEqual(reduxStateParams);
   });

diff --git a/src/plugins/vis_builder/public/application/utils/state_management/redux_persistence.ts b/src/plugins/vis_builder/public/application/utils/state_management/redux_persistence.ts
@@ -20,7 +20,7 @@ export const loadReduxState = async (services: VisBuilderServices) => {
   return await getPreloadedState(services);
 };
 
-export const saveReduxState = (
+export const persistReduxState = (
   { style, visualization, metadata },
   services: VisBuilderServices
 ) => {

diff --git a/src/plugins/vis_builder/public/application/utils/state_management/store.ts b/src/plugins/vis_builder/public/application/utils/state_management/store.ts
@@ -4,12 +4,14 @@
  */
 
 import { combineReducers, configureStore, PreloadedState } from '@reduxjs/toolkit';
+import { isEqual } from 'lodash';
 import { reducer as styleReducer } from './style_slice';
 import { reducer as visualizationReducer } from './visualization_slice';
 import { reducer as metadataReducer } from './metadata_slice';
 import { VisBuilderServices } from '../../..';
-import { setEditorState } from './metadata_slice';
-import { loadReduxState, saveReduxState } from './redux_persistence';
+import { loadReduxState, persistReduxState } from './redux_persistence';
+import { handlerEditorState } from './handlers/editor_state';
+import { handlerParentAggs } from './handlers/parent_aggs';
 
 const rootReducer = combineReducers({
   style: styleReducer,
@@ -28,49 +30,20 @@ export const getPreloadedStore = async (services: VisBuilderServices) => {
   const preloadedState = await loadReduxState(services);
   const store = configurePreloadedStore(preloadedState);
 
-  const { metadata: metadataState, style: styleState, visualization: vizState } = store.getState();
-  let previousStore = {
-    viz: vizState,
-    style: styleState,
-  };
-  let previousMetadata = metadataState;
+  let previousState = store.getState();
 
   // Listen to changes
   const handleChange = () => {
-    const {
-      metadata: currentMetadataState,
-      style: currentStyleState,
-      visualization: currentVizState,
-    } = store.getState();
-    const currentStore = {
-      viz: currentVizState,
-      style: currentStyleState,
-    };
-    const currentMetadata = currentMetadataState;
+    const state = store.getState();
+    persistReduxState(state, services);
 
-    // Need to make sure the editorStates are in the clean states(not the initial states) to indicate the viz finished loading
-    // Because when loading a saved viz from saved object, the previousStore will differ from
-    // the currentStore even tho there is no changes applied ( aggParams will
-    // first be empty, and it then will change to not empty once the viz finished loading)
-    if (
-      previousMetadata.editor.state === 'clean' &&
-      currentMetadata.editor.state === 'clean' &&
-      JSON.stringify(currentStore) !== JSON.stringify(previousStore)
-    ) {
-      store.dispatch(setEditorState({ state: 'dirty' }));
-    }
+    if (isEqual(state, previousState)) return;
 
-    previousStore = currentStore;
-    previousMetadata = currentMetadata;
+    // Side effects to apply after changes to the store are made
+    handlerEditorState(store, state, previousState);
+    handlerParentAggs(store, state, services);
 
-    saveReduxState(
-      {
-        style: store.getState().style,
-        visualization: store.getState().visualization,
-        metadata: store.getState().metadata,
-      },
-      services
-    );
+    previousState = state;
   };
 
   // the store subscriber will automatically detect changes and call handleChange function
@@ -82,7 +55,7 @@ export const getPreloadedStore = async (services: VisBuilderServices) => {
 // Infer the `RootState` and `AppDispatch` types from the store itself
 export type RootState = ReturnType<typeof rootReducer>;
 export type RenderState = Omit<RootState, 'metadata'>; // Remaining state after auxillary states are removed
-type Store = ReturnType<typeof configurePreloadedStore>;
+export type Store = ReturnType<typeof configurePreloadedStore>;
 export type AppDispatch = Store['dispatch'];
 
 export { setState as setStyleState, StyleState } from './style_slice';

diff --git a/src/plugins/vis_builder/public/application/utils/state_management/visualization_slice.ts b/src/plugins/vis_builder/public/application/utils/state_management/visualization_slice.ts
@@ -98,6 +98,23 @@ export const slice = createSlice({
     updateAggConfigParams: (state, action: PayloadAction<CreateAggConfigParams[]>) => {
       state.activeVisualization!.aggConfigParams = action.payload;
     },
+    setAggParamValue: (
+      state,
+      action: PayloadAction<{
+        aggId: string;
+        paramName: string;
+        value: any;
+      }>
+    ) => {
+      const aggIndex = state.activeVisualization!.aggConfigParams.findIndex(
+        (agg) => agg.id === action.payload.aggId
+      );
+
+      state.activeVisualization!.aggConfigParams[aggIndex].params = {
+        ...state.activeVisualization!.aggConfigParams[aggIndex].params,
+        [action.payload.paramName]: action.payload.value,
+      };
+    },
     setState: (_state, action: PayloadAction<VisualizationState>) => {
       return action.payload;
     },
@@ -119,6 +136,7 @@ export const {
   editDraftAgg,
   saveDraftAgg,
   updateAggConfigParams,
+  setAggParamValue,
   reorderAgg,
   setState,
 } = slice.actions;