diff --git a/src/plugins/vis_builder/public/application/components/workspace.tsx b/src/plugins/vis_builder/public/application/components/workspace.tsx index 82f64ddc996a..6e3371404355 100644 --- a/src/plugins/vis_builder/public/application/components/workspace.tsx +++ b/src/plugins/vis_builder/public/application/components/workspace.tsx @@ -43,15 +43,17 @@ export const Workspace: FC = ({ children }) => { useEffect(() => { async function loadExpression() { const schemas = ui.containerConfig.data.schemas; - const [validSchema, schemaError] = validateSchemaState(schemas, rootState.visualization); - const [validAggs, aggregationError] = await validateAggregations(aggConfigs?.aggs || []); - if (!validSchema || !validAggs) { - const err = schemaError || aggregationError; - if (typeof err === 'string' && aggConfigs?.aggs.length) { - toasts.addWarning(err); - } + 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; } 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 index 2e1fe32b2b39..279a6cf43687 100644 --- 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 @@ -21,6 +21,4 @@ export const handlerEditorState = (store: Store, state: RootState, previousState ) { store.dispatch(setEditorState({ state: 'dirty' })); } - - return state; }; 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 index 3794acdc2d23..255699852c8e 100644 --- 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 @@ -17,7 +17,7 @@ export const handlerParentAggs = async ( store: Store, state: RootState, services: VisBuilderServices -): Promise => { +) => { const { visualization: { activeVisualization, indexPattern = '' }, } = state; @@ -64,6 +64,4 @@ export const handlerParentAggs = async ( ); } }); - - return state; }; 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..ff8fc84f8d00 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,7 @@ import { VisBuilderServices } from '../../../types'; import { createVisBuilderServicesMock } from '../mocks'; -import { getPreloadedState } from './preload'; -import { loadReduxState, saveReduxState } from './redux_persistence'; +import { loadReduxState, persistReduxState } from './redux_persistence'; describe('test redux state persistence', () => { let mockServices: jest.Mocked; @@ -45,8 +44,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 66c4bfca19ce..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 @@ -9,7 +9,7 @@ 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 { loadReduxState, saveReduxState } from './redux_persistence'; +import { loadReduxState, persistReduxState } from './redux_persistence'; import { handlerEditorState } from './handlers/editor_state'; import { handlerParentAggs } from './handlers/parent_aggs'; @@ -35,20 +35,15 @@ export const getPreloadedStore = async (services: VisBuilderServices) => { // Listen to changes const handleChange = () => { const state = store.getState(); + persistReduxState(state, services); if (isEqual(state, previousState)) return; - previousState = handlerEditorState(store, state, previousState); + // 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 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 4899dcec47ea..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 @@ -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/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 index 8d8154ca7c3d..470c83e96895 100644 --- a/src/plugins/vis_builder/public/application/utils/validations/validate_aggregations.ts +++ b/src/plugins/vis_builder/public/application/utils/validations/validate_aggregations.ts @@ -7,8 +7,14 @@ 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'; -export const validateAggregations = async (aggs: AggConfig[]): Promise<[boolean, string?]> => { +/** + * 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( @@ -18,13 +24,13 @@ export const validateAggregations = async (aggs: AggConfig[]): Promise<[boolean, const lastBucket = findLast(aggs, (agg) => agg.type.type === 'buckets'); if (!lastBucket && lastParentPipelineAgg) { - return [ - false, - i18n.translate('visBuilder.aggregation.mustHaveBucketErrorMessage', { + 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 @@ -33,16 +39,16 @@ export const validateAggregations = async (aggs: AggConfig[]): Promise<[boolean, lastParentPipelineAgg && !([BUCKET_TYPES.DATE_HISTOGRAM, BUCKET_TYPES.HISTOGRAM] as any).includes(lastBucket.type.name) ) { - return [ - false, - i18n.translate('visBuilder.aggregation.wrongLastBucketTypeErrorMessage', { + 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 [true]; + 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/validations/validate_schema_state.ts b/src/plugins/vis_builder/public/application/utils/validations/validate_schema_state.ts index cd9f72554746..38139768a8f0 100644 --- a/src/plugins/vis_builder/public/application/utils/validations/validate_schema_state.ts +++ b/src/plugins/vis_builder/public/application/utils/validations/validate_schema_state.ts @@ -6,26 +6,22 @@ import { countBy } from 'lodash'; 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/validations/vis_builder_state_validation.test.ts b/src/plugins/vis_builder/public/application/utils/validations/vis_builder_state_validation.test.ts index c2d6d41a834f..cd39c6d70f9c 100644 --- a/src/plugins/vis_builder/public/application/utils/validations/vis_builder_state_validation.test.ts +++ b/src/plugins/vis_builder/public/application/utils/validations/vis_builder_state_validation.test.ts @@ -27,7 +27,7 @@ describe('visBuilder state validation', () => { visualizationState: validVisualizationState, }); expect(validationResult.valid).toBeTruthy(); - expect(validationResult.errors).toBeNull(); + expect(validationResult.errorMsg).toBeNull(); }); }); describe('correct return with errors when validation fails', () => { @@ -37,7 +37,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 index 406860c3ed54..e1d85f9ff061 100644 --- 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 @@ -5,15 +5,19 @@ 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) => { +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, - errors: validateState.errors, + errorMsg, }; }; 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 5253740874f8..6282845372ac 100644 --- a/src/plugins/vis_builder/public/embeddable/vis_builder_embeddable.tsx +++ b/src/plugins/vis_builder/public/embeddable/vis_builder_embeddable.tsx @@ -139,7 +139,7 @@ export class VisBuilderEmbeddable extends Embeddable