From c4014e8697c555a0e23b16cf77a8ebd5dc00db10 Mon Sep 17 00:00:00 2001 From: Ashwin P Chandran Date: Tue, 10 Jan 2023 15:53:16 -0800 Subject: [PATCH] [VisBuilder] Fixes pipeline aggs (#3137) * fixes pipeline aggs in visbuilder Signed-off-by: Ashwin P Chandran * adds changelog Signed-off-by: Ashwin P Chandran * Adds unit tests Signed-off-by: Ashwin P Chandran * fixes pipeline aggs in visbuilder Signed-off-by: Ashwin P Chandran * adds changelog Signed-off-by: Ashwin P Chandran * Adds unit tests Signed-off-by: Ashwin P Chandran * fixes unit tests Signed-off-by: Ashwin P Chandran Signed-off-by: Ashwin P Chandran Signed-off-by: Arpit Bandejiya --- CHANGELOG.md | 1 + .../components/data_tab/secondary_panel.tsx | 12 +-- .../application/components/workspace.tsx | 21 ++-- .../state_management/handlers/editor_state.ts | 24 +++++ .../state_management/handlers/parent_aggs.ts | 67 +++++++++++++ .../utils/state_management/metadata_slice.ts | 14 +-- .../redux_persistence.test.tsx | 12 +-- .../state_management/redux_persistence.ts | 2 +- .../utils/state_management/store.ts | 53 +++------- .../state_management/visualization_slice.ts | 18 ++++ .../utils/use/use_saved_vis_builder_vis.ts | 17 ++-- .../application/utils/validations/index.ts | 8 ++ .../application/utils/validations/types.ts | 9 ++ .../validations/validate_aggregations.test.ts | 99 +++++++++++++++++++ .../validations/validate_aggregations.ts | 54 ++++++++++ .../validations/validate_schema_state.test.ts | 59 +++++++++++ .../validate_schema_state.ts | 24 ++--- .../vis_builder_state_validation.test.ts | 10 +- .../vis_builder_state_validation.ts | 23 +++++ .../utils/vis_builder_state_validation.ts | 19 ---- .../embeddable/vis_builder_embeddable.tsx | 4 +- 21 files changed, 437 insertions(+), 113 deletions(-) create mode 100644 src/plugins/vis_builder/public/application/utils/state_management/handlers/editor_state.ts create mode 100644 src/plugins/vis_builder/public/application/utils/state_management/handlers/parent_aggs.ts create mode 100644 src/plugins/vis_builder/public/application/utils/validations/index.ts create mode 100644 src/plugins/vis_builder/public/application/utils/validations/types.ts create mode 100644 src/plugins/vis_builder/public/application/utils/validations/validate_aggregations.test.ts create mode 100644 src/plugins/vis_builder/public/application/utils/validations/validate_aggregations.ts create mode 100644 src/plugins/vis_builder/public/application/utils/validations/validate_schema_state.test.ts rename src/plugins/vis_builder/public/application/utils/{ => validations}/validate_schema_state.ts (65%) rename src/plugins/vis_builder/public/application/utils/{ => validations}/vis_builder_state_validation.test.ts (83%) create mode 100644 src/plugins/vis_builder/public/application/utils/validations/vis_builder_state_validation.ts delete mode 100644 src/plugins/vis_builder/public/application/utils/vis_builder_state_validation.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 594c92cc4f9f..fe9806ba766c 100644 --- 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 index 9576d1edc419..18a1991f6d80 100644 --- 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 index 3742cafb976a..6e3371404355 100644 --- 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(); const { toExpression, ui } = useVisualizationType(); + const { aggConfigs } = useAggs(); const [expression, setExpression] = useState(); const [searchContext, setSearchContext] = useState({ 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 new file mode 100644 index 000000000000..279a6cf43687 --- /dev/null +++ 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 new file mode 100644 index 000000000000..255699852c8e --- /dev/null +++ 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 index c1e23b52823d..05ceb324aaa1 100644 --- 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 index ea3db25fbea0..91f760bbf231 100644 --- 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; @@ -22,7 +22,7 @@ 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: '', @@ -30,7 +30,7 @@ describe('test redux state persistence', () => { 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 index 295d3e2b4478..a531986a9ac9 100644 --- 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 index 19f60858b19d..f1b1c0eeae2a 100644 --- 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; export type RenderState = Omit; // Remaining state after auxillary states are removed -type Store = ReturnType; +export type Store = ReturnType; 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 index 2039c93e8ade..ece2618cbbe1 100644 --- 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) => { 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) => { return action.payload; }, @@ -119,6 +136,7 @@ export const { editDraftAgg, saveDraftAgg, updateAggConfigParams, + setAggParamValue, reorderAgg, setState, } = slice.actions; diff --git a/src/plugins/vis_builder/public/application/utils/use/use_saved_vis_builder_vis.ts b/src/plugins/vis_builder/public/application/utils/use/use_saved_vis_builder_vis.ts index 6e5d861c5318..29c14dc07b08 100644 --- a/src/plugins/vis_builder/public/application/utils/use/use_saved_vis_builder_vis.ts +++ b/src/plugins/vis_builder/public/application/utils/use/use_saved_vis_builder_vis.ts @@ -23,7 +23,7 @@ import { } from '../state_management'; import { useOpenSearchDashboards } from '../../../../../opensearch_dashboards_react/public'; import { setEditorState } from '../state_management/metadata_slice'; -import { validateVisBuilderState } from '../vis_builder_state_validation'; +import { validateVisBuilderState } from '../validations/vis_builder_state_validation'; // This function can be used when instantiating a saved vis or creating a new one // using url parameters, embedding and destroying it in DOM @@ -40,7 +40,7 @@ export const useSavedVisBuilderVis = (visualizationIdFromUrl: string | undefined http: { basePath }, toastNotifications, } = services; - const toastNotification = (message) => { + const toastNotification = (message: string) => { toastNotifications.addDanger({ title: i18n.translate('visualize.createVisualization.failedToLoadErrorMessage', { defaultMessage: 'Failed to load the visualization', @@ -48,6 +48,7 @@ export const useSavedVisBuilderVis = (visualizationIdFromUrl: string | undefined text: message, }); }; + const loadSavedVisBuilderVis = async () => { try { const savedVisBuilderVis = await getSavedVisBuilderVis(services, visualizationIdFromUrl); @@ -73,11 +74,13 @@ export const useSavedVisBuilderVis = (visualizationIdFromUrl: string | undefined const validateResult = validateVisBuilderState({ styleState, visualizationState }); if (!validateResult.valid) { - const err = validateResult.errors; - if (err) { - const errMsg = err[0].instancePath + ' ' + err[0].message; - throw new InvalidJSONProperty(errMsg); - } + throw new InvalidJSONProperty( + validateResult.errorMsg || + i18n.translate('visBuilder.useSavedVisBuilderVis.genericJSONError', { + defaultMessage: + 'Something went wrong while loading your saved object. The object may be corrupted or does not match the latest schema', + }) + ); } dispatch(setStyleState(styleState)); diff --git a/src/plugins/vis_builder/public/application/utils/validations/index.ts b/src/plugins/vis_builder/public/application/utils/validations/index.ts new file mode 100644 index 000000000000..2986b354f669 --- /dev/null +++ b/src/plugins/vis_builder/public/application/utils/validations/index.ts @@ -0,0 +1,8 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +export * from './validate_aggregations'; +export * from './validate_schema_state'; +export * from './vis_builder_state_validation'; diff --git a/src/plugins/vis_builder/public/application/utils/validations/types.ts b/src/plugins/vis_builder/public/application/utils/validations/types.ts new file mode 100644 index 000000000000..2763c476f2d3 --- /dev/null +++ b/src/plugins/vis_builder/public/application/utils/validations/types.ts @@ -0,0 +1,9 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +export interface ValidationResult { + errorMsg?: string; + valid: T; +} diff --git a/src/plugins/vis_builder/public/application/utils/validations/validate_aggregations.test.ts b/src/plugins/vis_builder/public/application/utils/validations/validate_aggregations.test.ts new file mode 100644 index 000000000000..bec1ae506928 --- /dev/null +++ b/src/plugins/vis_builder/public/application/utils/validations/validate_aggregations.test.ts @@ -0,0 +1,99 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +import { BUCKET_TYPES, IndexPattern, METRIC_TYPES } from '../../../../../data/public'; +import { dataPluginMock } from '../../../../../data/public/mocks'; +import { validateAggregations } from './validate_aggregations'; + +describe('validateAggregations', () => { + const fields = [ + { + name: '@timestamp', + }, + { + name: 'bytes', + }, + ]; + + const indexPattern = { + id: '1234', + title: 'logstash-*', + fields: { + getByName: (name: string) => fields.find((f) => f.name === name), + filter: () => fields, + }, + } as any; + + const dataStart = dataPluginMock.createStartContract(); + + test('Pipeline aggs should have a bucket agg as the last agg', () => { + const aggConfigs = dataStart.search.aggs.createAggConfigs(indexPattern as IndexPattern, [ + { + id: '1', + enabled: true, + type: METRIC_TYPES.CUMULATIVE_SUM, + schema: 'metric', + params: {}, + }, + ]); + + const { valid, errorMsg } = validateAggregations(aggConfigs.aggs); + + expect(valid).toBe(false); + expect(errorMsg).toMatchInlineSnapshot( + `"Add a bucket with \\"Date Histogram\\" or \\"Histogram\\" aggregation."` + ); + }); + + test('Pipeline aggs should have a valid bucket agg', () => { + const aggConfigs = dataStart.search.aggs.createAggConfigs(indexPattern as IndexPattern, [ + { + id: '0', + enabled: true, + type: BUCKET_TYPES.SIGNIFICANT_TERMS, + schema: 'segment', + params: {}, + }, + { + id: '1', + enabled: true, + type: METRIC_TYPES.CUMULATIVE_SUM, + schema: 'metric', + params: {}, + }, + ]); + + const { valid, errorMsg } = validateAggregations(aggConfigs.aggs); + + expect(valid).toBe(false); + expect(errorMsg).toMatchInlineSnapshot( + `"Last bucket aggregation must be \\"Date Histogram\\" or \\"Histogram\\" when using \\"Cumulative Sum\\" metric aggregation."` + ); + }); + + test('Valid pipeline aggconfigs', () => { + const aggConfigs = dataStart.search.aggs.createAggConfigs(indexPattern as IndexPattern, [ + { + id: '0', + enabled: true, + type: BUCKET_TYPES.DATE_HISTOGRAM, + schema: 'segment', + params: {}, + }, + { + id: '1', + enabled: true, + type: METRIC_TYPES.CUMULATIVE_SUM, + schema: 'metric', + params: {}, + }, + ]); + + const { valid, errorMsg } = validateAggregations(aggConfigs.aggs); + + expect(valid).toBe(true); + expect(errorMsg).not.toBeDefined(); + }); +}); diff --git a/src/plugins/vis_builder/public/application/utils/validations/validate_aggregations.ts b/src/plugins/vis_builder/public/application/utils/validations/validate_aggregations.ts new file mode 100644 index 000000000000..470c83e96895 --- /dev/null +++ b/src/plugins/vis_builder/public/application/utils/validations/validate_aggregations.ts @@ -0,0 +1,54 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +import { i18n } from '@osd/i18n'; +import { findLast } from 'lodash'; +import { AggConfig, BUCKET_TYPES, IMetricAggType } from '../../../../../data/common'; +import { search } from '../../../../../data/public'; +import { ValidationResult } from './types'; + +/** + * Validate if the aggregations to perform are possible + * @param aggs Aggregations to be performed + * @returns ValidationResult + */ +export const validateAggregations = (aggs: AggConfig[]): ValidationResult => { + // Pipeline aggs should have a valid bucket agg + const metricAggs = aggs.filter((agg) => agg.schema === 'metric'); + const lastParentPipelineAgg = findLast( + metricAggs, + ({ type }: { type: IMetricAggType }) => type.subtype === search.aggs.parentPipelineType + ); + const lastBucket = findLast(aggs, (agg) => agg.type.type === 'buckets'); + + if (!lastBucket && lastParentPipelineAgg) { + return { + valid: false, + errorMsg: i18n.translate('visBuilder.aggregation.mustHaveBucketErrorMessage', { + defaultMessage: 'Add a bucket with "Date Histogram" or "Histogram" aggregation.', + description: 'Date Histogram and Histogram should not be translated', + }), + }; + } + + // Last bucket in a Pipeline aggs should be either a date histogram or histogram + if ( + lastBucket && + lastParentPipelineAgg && + !([BUCKET_TYPES.DATE_HISTOGRAM, BUCKET_TYPES.HISTOGRAM] as any).includes(lastBucket.type.name) + ) { + return { + valid: false, + errorMsg: i18n.translate('visBuilder.aggregation.wrongLastBucketTypeErrorMessage', { + defaultMessage: + 'Last bucket aggregation must be "Date Histogram" or "Histogram" when using "{type}" metric aggregation.', + values: { type: (lastParentPipelineAgg as AggConfig).type.title }, + description: 'Date Histogram and Histogram should not be translated', + }), + }; + } + + return { valid: true }; +}; diff --git a/src/plugins/vis_builder/public/application/utils/validations/validate_schema_state.test.ts b/src/plugins/vis_builder/public/application/utils/validations/validate_schema_state.test.ts new file mode 100644 index 000000000000..a0c017cec3c4 --- /dev/null +++ b/src/plugins/vis_builder/public/application/utils/validations/validate_schema_state.test.ts @@ -0,0 +1,59 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +import { Schemas } from '../../../../../vis_default_editor/public'; +import { VisualizationState } from '../state_management'; +import { validateSchemaState } from './validate_schema_state'; + +describe('validateSchemaState', () => { + const schemas = new Schemas([ + { + name: 'metrics', + group: 'metrics', + min: 1, + }, + { + name: 'buckets', + group: 'buckets', + }, + ]); + + test('should error if schema min agg requirement not met', () => { + const visState: VisualizationState = { + searchField: '', + activeVisualization: { + name: 'Test vis', + aggConfigParams: [], + }, + }; + + const { valid, errorMsg } = validateSchemaState(schemas, visState); + + expect(valid).toBe(false); + expect(errorMsg).toMatchInlineSnapshot( + `"The Test vis visualization needs at least 1 field(s) in the agg type \\"metrics\\""` + ); + }); + + test('should be valid if schema requirements are met', () => { + const visState: VisualizationState = { + searchField: '', + activeVisualization: { + name: 'Test vis', + aggConfigParams: [ + { + type: 'count', + schema: 'metrics', + }, + ], + }, + }; + + const { valid, errorMsg } = validateSchemaState(schemas, visState); + + expect(valid).toBe(true); + expect(errorMsg).not.toBeDefined(); + }); +}); diff --git a/src/plugins/vis_builder/public/application/utils/validate_schema_state.ts b/src/plugins/vis_builder/public/application/utils/validations/validate_schema_state.ts similarity index 65% rename from src/plugins/vis_builder/public/application/utils/validate_schema_state.ts rename to src/plugins/vis_builder/public/application/utils/validations/validate_schema_state.ts index 87dc19a3024e..38139768a8f0 100644 --- a/src/plugins/vis_builder/public/application/utils/validate_schema_state.ts +++ b/src/plugins/vis_builder/public/application/utils/validations/validate_schema_state.ts @@ -4,28 +4,24 @@ */ import { countBy } from 'lodash'; -import { Schemas } from '../../../../vis_default_editor/public'; -import { VisualizationState } from './state_management'; +import { Schemas } from '../../../../../vis_default_editor/public'; +import { VisualizationState } from '../state_management'; +import { ValidationResult } from './types'; /** * Validate if the visualization state fits the vis type schema criteria * @param schemas Visualization type config Schema objects * @param state visualization state - * @returns [Validity, 'Message'] + * @returns ValidationResult */ export const validateSchemaState = ( schemas: Schemas, state: VisualizationState -): [boolean, string?] => { +): ValidationResult => { const activeViz = state.activeVisualization; const vizName = activeViz?.name; const aggs = activeViz?.aggConfigParams; - // Check if any aggreagations exist - if (aggs?.length === 0) { - return [false]; - } - // Check if each schema's min agg requirement is met const aggSchemaCount = countBy(aggs, (agg) => agg.schema); const invalidsSchemas = schemas.all.filter((schema) => { @@ -36,11 +32,11 @@ export const validateSchemaState = ( }); if (invalidsSchemas.length > 0) { - return [ - false, - `The ${vizName} visualization needs at least ${invalidsSchemas[0].min} field(s) in the agg type "${invalidsSchemas[0].name}"`, - ]; + return { + valid: false, + errorMsg: `The ${vizName} visualization needs at least ${invalidsSchemas[0].min} field(s) in the agg type "${invalidsSchemas[0].name}"`, + }; } - return [true, '']; + return { valid: true }; }; diff --git a/src/plugins/vis_builder/public/application/utils/vis_builder_state_validation.test.ts b/src/plugins/vis_builder/public/application/utils/validations/vis_builder_state_validation.test.ts similarity index 83% rename from src/plugins/vis_builder/public/application/utils/vis_builder_state_validation.test.ts rename to src/plugins/vis_builder/public/application/utils/validations/vis_builder_state_validation.test.ts index c2d6d41a834f..550e59c65f20 100644 --- a/src/plugins/vis_builder/public/application/utils/vis_builder_state_validation.test.ts +++ b/src/plugins/vis_builder/public/application/utils/validations/vis_builder_state_validation.test.ts @@ -3,6 +3,7 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { RootState } from '../state_management'; import { validateVisBuilderState } from './vis_builder_state_validation'; describe('visBuilder state validation', () => { @@ -12,7 +13,8 @@ describe('visBuilder state validation', () => { legendPosition: '', type: 'metric', }; - const validVisualizationState = { + + const validVisualizationState: RootState['visualization'] = { activeVisualization: { name: 'metric', aggConfigParams: [], @@ -20,6 +22,7 @@ describe('visBuilder state validation', () => { indexPattern: '', searchField: '', }; + describe('correct return when validation suceeds', () => { test('with correct visBuilder state', () => { const validationResult = validateVisBuilderState({ @@ -27,9 +30,10 @@ describe('visBuilder state validation', () => { visualizationState: validVisualizationState, }); expect(validationResult.valid).toBeTruthy(); - expect(validationResult.errors).toBeNull(); + expect(validationResult.errorMsg).toBeUndefined(); }); }); + describe('correct return with errors when validation fails', () => { test('with non object type styleStyle', () => { const validationResult = validateVisBuilderState({ @@ -37,7 +41,7 @@ describe('visBuilder state validation', () => { visualizationState: validVisualizationState, }); expect(validationResult.valid).toBeFalsy(); - expect(validationResult.errors).toBeDefined(); + expect(validationResult.errorMsg).toBeDefined(); }); }); }); diff --git a/src/plugins/vis_builder/public/application/utils/validations/vis_builder_state_validation.ts b/src/plugins/vis_builder/public/application/utils/validations/vis_builder_state_validation.ts new file mode 100644 index 000000000000..e1d85f9ff061 --- /dev/null +++ b/src/plugins/vis_builder/public/application/utils/validations/vis_builder_state_validation.ts @@ -0,0 +1,23 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +import Ajv from 'ajv'; +import visBuilderStateSchema from '../schema.json'; +import { ValidationResult } from './types'; + +const ajv = new Ajv(); +const validateState = ajv.compile(visBuilderStateSchema); + +export const validateVisBuilderState = (visBuilderState: any): ValidationResult => { + const isVisBuilderStateValid = validateState(visBuilderState); + const errorMsg = validateState.errors + ? validateState.errors[0].instancePath + ' ' + validateState.errors[0].message + : undefined; + + return { + valid: isVisBuilderStateValid, + errorMsg, + }; +}; diff --git a/src/plugins/vis_builder/public/application/utils/vis_builder_state_validation.ts b/src/plugins/vis_builder/public/application/utils/vis_builder_state_validation.ts deleted file mode 100644 index 9a601e82594d..000000000000 --- a/src/plugins/vis_builder/public/application/utils/vis_builder_state_validation.ts +++ /dev/null @@ -1,19 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -import Ajv from 'ajv'; -import visBuilderStateSchema from './schema.json'; - -const ajv = new Ajv(); -const validateState = ajv.compile(visBuilderStateSchema); - -export const validateVisBuilderState = (visBuilderState) => { - const isVisBuilderStateValid = validateState(visBuilderState); - - return { - valid: isVisBuilderStateValid, - errors: validateState.errors, - }; -}; diff --git a/src/plugins/vis_builder/public/embeddable/vis_builder_embeddable.tsx b/src/plugins/vis_builder/public/embeddable/vis_builder_embeddable.tsx index a8c41df6cc43..6282845372ac 100644 --- a/src/plugins/vis_builder/public/embeddable/vis_builder_embeddable.tsx +++ b/src/plugins/vis_builder/public/embeddable/vis_builder_embeddable.tsx @@ -27,7 +27,7 @@ import { TimefilterContract, TimeRange, } from '../../../data/public'; -import { validateSchemaState } from '../application/utils/validate_schema_state'; +import { validateSchemaState } from '../application/utils/validations/validate_schema_state'; import { getExpressionLoader, getTypeService } from '../plugin_services'; import { PersistedState } from '../../../visualizations/public'; import { RenderState, VisualizationState } from '../application/utils/state_management'; @@ -139,7 +139,7 @@ export class VisBuilderEmbeddable extends Embeddable