From 2b45cf628f1391e7918b23e63df44e23f1c07bcc Mon Sep 17 00:00:00 2001 From: Faisal Kanout Date: Fri, 30 Jun 2023 17:11:58 +0300 Subject: [PATCH 1/7] [AO] Add integration test for the Threshold Rule (#160633) ## Summary It fixes #160510 by: - Adding integration tests for the Threshold Rule. - Update the consumer value. - Update the action group. --- .../common/threshold_rule/types.ts | 5 +- .../threshold/components/alert_flyout.tsx | 2 +- .../lib/rules/threshold/threshold_executor.ts | 8 +- .../alerting_api_integration/common/config.ts | 1 + .../helpers/alerting_api_helper.ts | 17 +- .../observability/helpers/data_view.ts | 59 ++++++ .../observability/helpers/syntrace.ts | 159 ++++++++++++++ .../observability/index.ts | 1 + .../observability/metric_threshold_rule.ts | 6 +- .../observability/threshold_rule.ts | 195 ++++++++++++++++++ .../group4/check_registered_rule_types.ts | 1 + 11 files changed, 439 insertions(+), 15 deletions(-) create mode 100644 x-pack/test/alerting_api_integration/observability/helpers/data_view.ts create mode 100644 x-pack/test/alerting_api_integration/observability/helpers/syntrace.ts create mode 100644 x-pack/test/alerting_api_integration/observability/threshold_rule.ts diff --git a/x-pack/plugins/observability/common/threshold_rule/types.ts b/x-pack/plugins/observability/common/threshold_rule/types.ts index 0762fef113ead..9cf5f48fae1d0 100644 --- a/x-pack/plugins/observability/common/threshold_rule/types.ts +++ b/x-pack/plugins/observability/common/threshold_rule/types.ts @@ -8,9 +8,9 @@ import * as rt from 'io-ts'; import { ML_ANOMALY_THRESHOLD } from '@kbn/ml-anomaly-utils/anomaly_threshold'; import { values } from 'lodash'; +import { SerializedSearchSourceFields } from '@kbn/data-plugin/common'; import { Color } from './color_palette'; import { metricsExplorerMetricRT } from './metrics_explorer'; - import { TimeUnitChar } from '../utils/formatters/duration'; import { SNAPSHOT_CUSTOM_AGGREGATIONS } from './constants'; @@ -201,13 +201,14 @@ export interface MetricAnomalyParams { // Types for the executor -export interface MetricThresholdParams { +export interface ThresholdParams { criteria: MetricExpressionParams[]; filterQuery?: string; filterQueryText?: string; sourceId?: string; alertOnNoData?: boolean; alertOnGroupDisappear?: boolean; + searchConfiguration: SerializedSearchSourceFields; } interface BaseMetricExpressionParams { diff --git a/x-pack/plugins/observability/public/components/threshold/components/alert_flyout.tsx b/x-pack/plugins/observability/public/components/threshold/components/alert_flyout.tsx index cc84ccd5081bd..e0d24d58eb0db 100644 --- a/x-pack/plugins/observability/public/components/threshold/components/alert_flyout.tsx +++ b/x-pack/plugins/observability/public/components/threshold/components/alert_flyout.tsx @@ -28,7 +28,7 @@ export function AlertFlyout(props: Props) { () => triggersActionsUI && triggersActionsUI.getAddRuleFlyout({ - consumer: 'infrastructure', + consumer: 'alerts', onClose: onCloseFlyout, canChangeTrigger: false, ruleTypeId: OBSERVABILITY_THRESHOLD_RULE_TYPE_ID, diff --git a/x-pack/plugins/observability/server/lib/rules/threshold/threshold_executor.ts b/x-pack/plugins/observability/server/lib/rules/threshold/threshold_executor.ts index f7387a5764ce2..6fd08c0a15b8e 100644 --- a/x-pack/plugins/observability/server/lib/rules/threshold/threshold_executor.ts +++ b/x-pack/plugins/observability/server/lib/rules/threshold/threshold_executor.ts @@ -71,8 +71,8 @@ export type MetricThresholdAlertContext = { value?: Record | null; }; -export const FIRED_ACTIONS_ID = 'metrics.threshold.fired'; -export const NO_DATA_ACTIONS_ID = 'metrics.threshold.nodata'; +export const FIRED_ACTIONS_ID = 'threshold.fired'; +export const NO_DATA_ACTIONS_ID = 'threshold.nodata'; type MetricThresholdActionGroup = | typeof FIRED_ACTIONS_ID @@ -408,14 +408,14 @@ export const createMetricThresholdExecutor = ({ }; export const FIRED_ACTIONS = { - id: 'metrics.threshold.fired', + id: 'threshold.fired', name: i18n.translate('xpack.observability.threshold.rule.alerting.threshold.fired', { defaultMessage: 'Alert', }), }; export const NO_DATA_ACTIONS = { - id: 'metrics.threshold.nodata', + id: 'threshold.nodata', name: i18n.translate('xpack.observability.threshold.rule.alerting.threshold.nodata', { defaultMessage: 'No Data', }), diff --git a/x-pack/test/alerting_api_integration/common/config.ts b/x-pack/test/alerting_api_integration/common/config.ts index f77542dc09586..851bc653d6252 100644 --- a/x-pack/test/alerting_api_integration/common/config.ts +++ b/x-pack/test/alerting_api_integration/common/config.ts @@ -185,6 +185,7 @@ export function createTestConfig(name: string, options: CreateTestConfigOptions) '--xpack.alerting.healthCheck.interval="1s"', '--xpack.alerting.rules.minimumScheduleInterval.value="1s"', '--xpack.alerting.rules.run.alerts.max=20', + '--xpack.observability.unsafe.thresholdRule.enabled=true', `--xpack.alerting.rules.run.actions.connectorTypeOverrides=${JSON.stringify([ { id: 'test.capped', max: '1' }, ])}`, diff --git a/x-pack/test/alerting_api_integration/observability/helpers/alerting_api_helper.ts b/x-pack/test/alerting_api_integration/observability/helpers/alerting_api_helper.ts index d89908b9bef72..a50e1b4e85c14 100644 --- a/x-pack/test/alerting_api_integration/observability/helpers/alerting_api_helper.ts +++ b/x-pack/test/alerting_api_integration/observability/helpers/alerting_api_helper.ts @@ -5,7 +5,8 @@ * 2.0. */ -import { InfraRuleType, InfraRuleTypeParams } from '@kbn/infra-plugin/common/alerting/metrics'; +import { MetricThresholdParams } from '@kbn/infra-plugin/common/alerting/metrics'; +import { ThresholdParams } from '@kbn/observability-plugin/common/threshold_rule/types'; import type { SuperTest, Test } from 'supertest'; export async function createIndexConnector({ @@ -31,31 +32,35 @@ export async function createIndexConnector({ return body.id as string; } -export async function createMetricThresholdRule({ +export async function createRule({ supertest, name, ruleTypeId, params, actions = [], + tags = [], schedule, + consumer, }: { supertest: SuperTest; - ruleTypeId: T; + ruleTypeId: string; name: string; - params: InfraRuleTypeParams[T]; + params: MetricThresholdParams | ThresholdParams; actions?: any[]; + tags?: any[]; schedule?: { interval: string }; + consumer: string; }) { const { body } = await supertest .post(`/api/alerting/rule`) .set('kbn-xsrf', 'foo') .send({ params, - consumer: 'infrastructure', + consumer, schedule: schedule || { interval: '5m', }, - tags: ['infrastructure'], + tags, name, rule_type_id: ruleTypeId, actions, diff --git a/x-pack/test/alerting_api_integration/observability/helpers/data_view.ts b/x-pack/test/alerting_api_integration/observability/helpers/data_view.ts new file mode 100644 index 0000000000000..e8a03df2d0713 --- /dev/null +++ b/x-pack/test/alerting_api_integration/observability/helpers/data_view.ts @@ -0,0 +1,59 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import { SuperTest, Test } from 'supertest'; + +export const createDataView = async ({ + supertest, + id, + name, + title, +}: { + supertest: SuperTest; + id: string; + name: string; + title: string; +}) => { + const { body } = await supertest + .post(`/api/content_management/rpc/create`) + .set('kbn-xsrf', 'foo') + .send({ + contentTypeId: 'index-pattern', + data: { + fieldAttrs: '{}', + title, + timeFieldName: '@timestamp', + sourceFilters: '[]', + fields: '[]', + fieldFormatMap: '{}', + typeMeta: '{}', + runtimeFieldMap: '{}', + name, + }, + options: { id }, + version: 1, + }); + return body; +}; +export const deleteDataView = async ({ + supertest, + id, +}: { + supertest: SuperTest; + id: string; +}) => { + const { body } = await supertest + .delete(`/api/content_management/rpc/create`) + .set('kbn-xsrf', 'foo') + .send({ + contentTypeId: 'index-pattern', + id, + options: { force: true }, + version: 1, + }); + return body; +}; diff --git a/x-pack/test/alerting_api_integration/observability/helpers/syntrace.ts b/x-pack/test/alerting_api_integration/observability/helpers/syntrace.ts new file mode 100644 index 0000000000000..259924e80d64d --- /dev/null +++ b/x-pack/test/alerting_api_integration/observability/helpers/syntrace.ts @@ -0,0 +1,159 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import { Client } from '@elastic/elasticsearch'; +import { + ApmSynthtraceEsClient, + ApmSynthtraceKibanaClient, + createLogger, + LogLevel, +} from '@kbn/apm-synthtrace'; +import { apm, timerange } from '@kbn/apm-synthtrace-client'; + +export const getSyntraceClient = async ({ + kibanaUrl, + esClient, +}: { + kibanaUrl: string; + esClient: Client; +}) => { + const kibanaClient = new ApmSynthtraceKibanaClient({ + logger: createLogger(LogLevel.info), + target: kibanaUrl, + }); + const packageVersion = await kibanaClient.fetchLatestApmPackageVersion(); + return new ApmSynthtraceEsClient({ + client: esClient, + logger: createLogger(LogLevel.info), + refreshAfterIndex: true, + version: packageVersion, + }); +}; + +export const dataConfig = { + rate: 10, + transaction: { + name: 'GET /data', + duration: 1000, + }, + service: { + name: 'lambda-python-dev-hello', + version: '$LATEST', + runtime: { + name: 'AWS_Lambda_python3.8', + version: '3.8.11', + }, + framework: 'AWS Lambda', + agent: { + name: 'python', + version: '6.6.0', + }, + }, + containerOs: 'linux', + serverless: { + firstFunctionName: 'my-function-1', + secondFunctionName: 'my-function-2', + faasTriggerType: 'other', + }, + cloud: { + provider: 'aws', + availabilityZone: 'us-central1-c', + region: 'us-east-1', + machineType: 'e2-standard-4', + projectName: 'elastic-observability', + serviceName: 'lambda', + }, +}; + +export async function generateData({ + synthtraceEsClient, + start, + end, +}: { + synthtraceEsClient: ApmSynthtraceEsClient; + start: number; + end: number; +}) { + const { rate, service, containerOs, serverless, cloud, transaction } = dataConfig; + const { + provider, + availabilityZone, + region, + machineType, + projectName, + serviceName: cloudServiceName, + } = cloud; + const { faasTriggerType, firstFunctionName, secondFunctionName } = serverless; + const { version, runtime, framework, agent, name: serviceName } = service; + const { name: serviceRunTimeName, version: serviceRunTimeVersion } = runtime; + const { name: agentName, version: agentVersion } = agent; + + const instance = apm + .service({ name: serviceName, environment: 'production', agentName }) + .instance('instance-a'); + + const traceEvents = [ + timerange(start, end) + .interval('30s') + .rate(rate) + .generator((timestamp) => + instance + .containerId('instance-a') + .transaction({ transactionName: transaction.name }) + .timestamp(timestamp) + .defaults({ + 'cloud.provider': provider, + 'cloud.project.name': projectName, + 'cloud.service.name': cloudServiceName, + 'cloud.availability_zone': availabilityZone, + 'cloud.machine.type': machineType, + 'cloud.region': region, + 'faas.id': `arn:aws:lambda:us-west-2:123456789012:function:${firstFunctionName}`, + 'faas.trigger.type': faasTriggerType, + 'host.os.platform': containerOs, + 'kubernetes.pod.uid': '48f4c5a5-0625-4bea-9d94-77ee94a17e70', + 'service.version': version, + 'service.runtime.name': serviceRunTimeName, + 'service.runtime.version': serviceRunTimeVersion, + 'service.framework.name': framework, + 'agent.version': agentVersion, + }) + .duration(transaction.duration) + .success() + ), + + timerange(start, end) + .interval('30s') + .rate(rate) + .generator((timestamp) => + instance + .transaction({ transactionName: transaction.name }) + .timestamp(timestamp) + .defaults({ + 'cloud.provider': provider, + 'cloud.project.name': projectName, + 'cloud.service.name': cloudServiceName, + 'cloud.availability_zone': availabilityZone, + 'cloud.machine.type': machineType, + 'cloud.region': region, + 'faas.id': `arn:aws:lambda:us-west-2:123456789012:function:${secondFunctionName}`, + 'faas.trigger.type': faasTriggerType, + 'host.os.platform': containerOs, + 'kubernetes.pod.uid': '48f4c5a5-0625-4bea-9d94-77ee94a17e70', + 'service.version': version, + 'service.runtime.name': serviceRunTimeName, + 'service.runtime.version': serviceRunTimeVersion, + 'service.framework.name': framework, + 'agent.version': agentVersion, + }) + .duration(transaction.duration) + .success() + ), + ]; + + await synthtraceEsClient.index(traceEvents); +} diff --git a/x-pack/test/alerting_api_integration/observability/index.ts b/x-pack/test/alerting_api_integration/observability/index.ts index 675119da9192d..ad137560c9036 100644 --- a/x-pack/test/alerting_api_integration/observability/index.ts +++ b/x-pack/test/alerting_api_integration/observability/index.ts @@ -9,5 +9,6 @@ export default function ({ loadTestFile }: any) { describe('MetricsUI Endpoints', () => { loadTestFile(require.resolve('./metric_threshold_rule')); + loadTestFile(require.resolve('./threshold_rule')); }); } diff --git a/x-pack/test/alerting_api_integration/observability/metric_threshold_rule.ts b/x-pack/test/alerting_api_integration/observability/metric_threshold_rule.ts index d99437eae68e1..2b727820ade70 100644 --- a/x-pack/test/alerting_api_integration/observability/metric_threshold_rule.ts +++ b/x-pack/test/alerting_api_integration/observability/metric_threshold_rule.ts @@ -15,7 +15,7 @@ import { waitForRuleStatus, } from './helpers/alerting_wait_for_helpers'; import { FtrProviderContext } from '../common/ftr_provider_context'; -import { createIndexConnector, createMetricThresholdRule } from './helpers/alerting_api_helper'; +import { createIndexConnector, createRule } from './helpers/alerting_api_helper'; // eslint-disable-next-line import/no-default-export export default function ({ getService }: FtrProviderContext) { @@ -42,9 +42,11 @@ export default function ({ getService }: FtrProviderContext) { name: 'Index Connector: Metric threshold API test', indexName: ALERT_ACTION_INDEX, }); - const createdRule = await createMetricThresholdRule({ + const createdRule = await createRule({ supertest, ruleTypeId: InfraRuleType.MetricThreshold, + consumer: 'infrastructure', + tags: ['infrastructure'], name: 'Metric threshold rule', params: { criteria: [ diff --git a/x-pack/test/alerting_api_integration/observability/threshold_rule.ts b/x-pack/test/alerting_api_integration/observability/threshold_rule.ts new file mode 100644 index 0000000000000..ab1907f951745 --- /dev/null +++ b/x-pack/test/alerting_api_integration/observability/threshold_rule.ts @@ -0,0 +1,195 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import moment from 'moment'; +import { ApmSynthtraceEsClient } from '@kbn/apm-synthtrace'; +import { format } from 'url'; +import { Aggregators, Comparator } from '@kbn/observability-plugin/common/threshold_rule/types'; +import { FIRED_ACTIONS_ID } from '@kbn/observability-plugin/server/lib/rules/threshold/threshold_executor'; +import expect from '@kbn/expect'; +import { OBSERVABILITY_THRESHOLD_RULE_TYPE_ID } from '@kbn/observability-plugin/common/constants'; +import { FtrProviderContext } from '../common/ftr_provider_context'; +import { createIndexConnector, createRule } from './helpers/alerting_api_helper'; +import { createDataView, deleteDataView } from './helpers/data_view'; +import { getSyntraceClient, generateData } from './helpers/syntrace'; +import { waitForAlertInIndex, waitForRuleStatus } from './helpers/alerting_wait_for_helpers'; + +// eslint-disable-next-line import/no-default-export +export default function ({ getService }: FtrProviderContext) { + const start = moment(Date.now()).subtract(10, 'minutes').valueOf(); + const end = moment(Date.now()).valueOf(); + const esClient = getService('es'); + const config = getService('config'); + const kibanaServerConfig = config.get('servers.kibana'); + const kibanaUrl = format(kibanaServerConfig); + const supertest = getService('supertest'); + + describe('Threshold rule', () => { + const THRESHOLD_RULE_ALERT_INDEX = '.alerts-observability.threshold.alerts-default'; + const ALERT_ACTION_INDEX = 'alert-action-threshold'; + const DATA_VIEW_ID = 'data-view-id'; + + let synthtraceEsClient: ApmSynthtraceEsClient; + let actionId: string; + let ruleId: string; + + before(async () => { + synthtraceEsClient = await getSyntraceClient({ esClient, kibanaUrl }); + await generateData({ synthtraceEsClient, start, end }); + await createDataView({ + supertest, + name: 'test-data-view', + id: DATA_VIEW_ID, + title: 'traces-apm*,metrics-apm*,logs-apm*', + }); + }); + + after(async () => { + await supertest.delete(`/api/alerting/rule/${ruleId}`).set('kbn-xsrf', 'foo'); + await supertest.delete(`/api/actions/connector/${actionId}`).set('kbn-xsrf', 'foo'); + await esClient.deleteByQuery({ + index: THRESHOLD_RULE_ALERT_INDEX, + query: { term: { 'kibana.alert.rule.uuid': ruleId } }, + }); + await esClient.deleteByQuery({ + index: '.kibana-event-log-*', + query: { term: { 'kibana.alert.rule.consumer': 'alerts' } }, + }); + await synthtraceEsClient.clean(); + await deleteDataView({ + supertest, + id: DATA_VIEW_ID, + }); + }); + + describe('Rule creation', () => { + it('creates rule successfully', async () => { + actionId = await createIndexConnector({ + supertest, + name: 'Index Connector: Threshold API test', + indexName: ALERT_ACTION_INDEX, + }); + + const createdRule = await createRule({ + supertest, + tags: ['observability'], + consumer: 'alerts', + name: 'Threshold rule', + ruleTypeId: OBSERVABILITY_THRESHOLD_RULE_TYPE_ID, + params: { + criteria: [ + { + aggType: Aggregators.CUSTOM, + comparator: Comparator.GT, + threshold: [7500000], + timeSize: 5, + timeUnit: 'm', + customMetrics: [ + { name: 'A', field: 'span.self_time.sum.us', aggType: Aggregators.AVERAGE }, + ], + }, + ], + alertOnNoData: true, + alertOnGroupDisappear: true, + searchConfiguration: { + query: { + query: '', + language: 'kuery', + }, + index: DATA_VIEW_ID, + }, + }, + actions: [ + { + group: FIRED_ACTIONS_ID, + id: actionId, + params: { + documents: [ + { + ruleType: '{{rule.type}}', + }, + ], + }, + frequency: { + notify_when: 'onActionGroupChange', + throttle: null, + summary: false, + }, + }, + ], + }); + ruleId = createdRule.id; + expect(ruleId).not.to.be(undefined); + }); + + it('should be active', async () => { + const executionStatus = await waitForRuleStatus({ + id: ruleId, + expectedStatus: 'active', + supertest, + }); + expect(executionStatus.status).to.be('active'); + }); + + it('should set correct information in the alert document', async () => { + const resp = await waitForAlertInIndex({ + esClient, + indexName: THRESHOLD_RULE_ALERT_INDEX, + ruleId, + }); + + expect(resp.hits.hits[0]._source).property( + 'kibana.alert.rule.category', + 'Threshold (Technical Preview)' + ); + expect(resp.hits.hits[0]._source).property('kibana.alert.rule.consumer', 'alerts'); + expect(resp.hits.hits[0]._source).property('kibana.alert.rule.name', 'Threshold rule'); + expect(resp.hits.hits[0]._source).property('kibana.alert.rule.producer', 'observability'); + expect(resp.hits.hits[0]._source).property('kibana.alert.rule.revision', 0); + expect(resp.hits.hits[0]._source).property( + 'kibana.alert.rule.rule_type_id', + 'observability.rules.threshold' + ); + expect(resp.hits.hits[0]._source).property('kibana.alert.rule.uuid', ruleId); + expect(resp.hits.hits[0]._source).property('kibana.space_ids').contain('default'); + expect(resp.hits.hits[0]._source) + .property('kibana.alert.rule.tags') + .contain('observability'); + expect(resp.hits.hits[0]._source).property('kibana.alert.action_group', 'threshold.fired'); + expect(resp.hits.hits[0]._source).property('tags').contain('observability'); + expect(resp.hits.hits[0]._source).property('kibana.alert.instance.id', '*'); + expect(resp.hits.hits[0]._source).property('kibana.alert.workflow_status', 'open'); + expect(resp.hits.hits[0]._source).property('event.kind', 'signal'); + expect(resp.hits.hits[0]._source).property('event.action', 'open'); + + expect(resp.hits.hits[0]._source) + .property('kibana.alert.rule.parameters') + .eql({ + criteria: [ + { + aggType: 'custom', + comparator: '>', + threshold: [7500000], + timeSize: 5, + timeUnit: 'm', + customMetrics: [{ name: 'A', field: 'span.self_time.sum.us', aggType: 'avg' }], + }, + ], + alertOnNoData: true, + alertOnGroupDisappear: true, + searchConfiguration: { index: 'data-view-id', query: { query: '', language: 'kuery' } }, + }); + }); + }); + }); +} diff --git a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group4/check_registered_rule_types.ts b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group4/check_registered_rule_types.ts index cb74808e3d63b..40197f1e18783 100644 --- a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group4/check_registered_rule_types.ts +++ b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group4/check_registered_rule_types.ts @@ -63,6 +63,7 @@ export default function createRegisteredRuleTypeTests({ getService }: FtrProvide 'monitoring_alert_elasticsearch_version_mismatch', 'monitoring_ccr_read_exceptions', 'monitoring_shard_size', + 'observability.rules.threshold', 'apm.transaction_duration', 'apm.anomaly', 'apm.error_rate', From cca2be4e1cfcea2d5d5cdde79bbb508b831b2db8 Mon Sep 17 00:00:00 2001 From: Tiago Costa Date: Fri, 30 Jun 2023 15:20:59 +0100 Subject: [PATCH 2/7] skip flaky suite (#160709) --- .../tests/service_overview/instance_details.spec.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x-pack/test/apm_api_integration/tests/service_overview/instance_details.spec.ts b/x-pack/test/apm_api_integration/tests/service_overview/instance_details.spec.ts index f6e257931ba54..2782e0c4f70df 100644 --- a/x-pack/test/apm_api_integration/tests/service_overview/instance_details.spec.ts +++ b/x-pack/test/apm_api_integration/tests/service_overview/instance_details.spec.ts @@ -46,7 +46,8 @@ export default function ApiTest({ getService }: FtrProviderContext) { } ); - registry.when( + // FLAKY: https://github.com/elastic/kibana/issues/160709 + registry.when.skip( 'Instance details when data is loaded', { config: 'basic', archives: [archiveName] }, () => { From 6f87e1d6960ee4ac48908c7bef75b00abca265cb Mon Sep 17 00:00:00 2001 From: Gerard Soldevila Date: Fri, 30 Jun 2023 16:40:40 +0200 Subject: [PATCH 3/7] [Migrations] Only pickup updated SO types when performing a compatible migration (#159962) ## Summary Tackles the first improvement described in https://github.com/elastic/kibana/issues/160038. When "picking up" the updated mappings, we add a "query" in order to select and update only the SO types that have been updated, compared to the previous version. We achieve this by comparing `migrationMappingPropertyHashes`; we compare the hashes stored in the `.mapping._meta.migrationMappingPropertyHashes` against the ones calculated from the definitions from the `typeRegistry`. --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> --- .../src/lib/utils/included_fields.ts | 2 +- .../src/actions/check_target_mappings.test.ts | 148 +++++++++++++----- .../src/actions/check_target_mappings.ts | 51 ++++-- .../src/actions/index.ts | 3 + .../src/actions/pickup_updated_mappings.ts | 8 +- .../update_and_pickup_mappings.test.ts | 148 ++++++++++++------ .../src/actions/update_and_pickup_mappings.ts | 5 +- .../src/core/build_active_mappings.test.ts | 64 +++++++- .../src/core/build_active_mappings.ts | 24 +++ .../src/model/helpers.test.ts | 16 ++ .../src/model/helpers.ts | 6 +- .../src/model/model.test.ts | 89 +++++++++-- .../src/model/model.ts | 59 +++++-- .../src/next.ts | 3 + .../src/state.ts | 1 + .../group3/dot_kibana_split.test.ts | 5 + .../group3/pickup_updated_types_only.test.ts | 94 +++++++++++ .../migrations/kibana_migrator_test_kit.ts | 10 ++ 18 files changed, 607 insertions(+), 129 deletions(-) create mode 100644 src/core/server/integration_tests/saved_objects/migrations/group3/pickup_updated_types_only.test.ts diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/utils/included_fields.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/utils/included_fields.ts index e0a1f2ca9b02b..50686def81c88 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/utils/included_fields.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/utils/included_fields.ts @@ -11,7 +11,7 @@ const ROOT_FIELDS = [ 'namespaces', 'type', 'references', - 'migrationVersion', + 'migrationVersion', // deprecated, see https://github.com/elastic/kibana/pull/150075 'coreMigrationVersion', 'typeMigrationVersion', 'managed', diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.test.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.test.ts index c98dec8ee48a8..17c55102a3cc7 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.test.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.test.ts @@ -8,22 +8,35 @@ import * as Either from 'fp-ts/lib/Either'; import type { IndexMapping } from '@kbn/core-saved-objects-base-server-internal'; -import { checkTargetMappings } from './check_target_mappings'; -import { diffMappings } from '../core/build_active_mappings'; +import type { SavedObjectsMappingProperties } from '@kbn/core-saved-objects-server'; +import { + checkTargetMappings, + type ComparedMappingsChanged, + type ComparedMappingsMatch, +} from './check_target_mappings'; +import { getUpdatedHashes } from '../core/build_active_mappings'; jest.mock('../core/build_active_mappings'); -const diffMappingsMock = diffMappings as jest.MockedFn; +const getUpdatedHashesMock = getUpdatedHashes as jest.MockedFn; -const actualMappings: IndexMapping = { - properties: { - field: { type: 'integer' }, - }, +const indexTypes = ['type1', 'type2']; + +const properties: SavedObjectsMappingProperties = { + type1: { type: 'long' }, + type2: { type: 'long' }, +}; + +const migrationMappingPropertyHashes = { + type1: 'type1Hash', + type2: 'type2Hash', }; const expectedMappings: IndexMapping = { - properties: { - field: { type: 'long' }, + properties, + dynamic: 'strict', + _meta: { + migrationMappingPropertyHashes, }, }; @@ -32,48 +45,99 @@ describe('checkTargetMappings', () => { jest.clearAllMocks(); }); - it('returns match=false if source mappings are not defined', async () => { - const task = checkTargetMappings({ - expectedMappings, - }); - - const result = await task(); - expect(diffMappings).not.toHaveBeenCalled(); - expect(result).toEqual(Either.right({ match: false })); - }); + describe('when actual mappings are incomplete', () => { + it("returns 'actual_mappings_incomplete' if actual mappings are not defined", async () => { + const task = checkTargetMappings({ + indexTypes, + expectedMappings, + }); - it('calls diffMappings() with the source and target mappings', async () => { - const task = checkTargetMappings({ - actualMappings, - expectedMappings, + const result = await task(); + expect(result).toEqual(Either.left({ type: 'actual_mappings_incomplete' as const })); }); - await task(); - expect(diffMappings).toHaveBeenCalledTimes(1); - expect(diffMappings).toHaveBeenCalledWith(actualMappings, expectedMappings); - }); - - it('returns match=true if diffMappings() match', async () => { - diffMappingsMock.mockReturnValueOnce(undefined); + it("returns 'actual_mappings_incomplete' if actual mappings do not define _meta", async () => { + const task = checkTargetMappings({ + indexTypes, + expectedMappings, + actualMappings: { + properties, + dynamic: 'strict', + }, + }); + + const result = await task(); + expect(result).toEqual(Either.left({ type: 'actual_mappings_incomplete' as const })); + }); - const task = checkTargetMappings({ - actualMappings, - expectedMappings, + it("returns 'actual_mappings_incomplete' if actual mappings do not define migrationMappingPropertyHashes", async () => { + const task = checkTargetMappings({ + indexTypes, + expectedMappings, + actualMappings: { + properties, + dynamic: 'strict', + _meta: {}, + }, + }); + + const result = await task(); + expect(result).toEqual(Either.left({ type: 'actual_mappings_incomplete' as const })); }); - const result = await task(); - expect(result).toEqual(Either.right({ match: true })); + it("returns 'actual_mappings_incomplete' if actual mappings define a different value for 'dynamic' property", async () => { + const task = checkTargetMappings({ + indexTypes, + expectedMappings, + actualMappings: { + properties, + dynamic: false, + _meta: { migrationMappingPropertyHashes }, + }, + }); + + const result = await task(); + expect(result).toEqual(Either.left({ type: 'actual_mappings_incomplete' as const })); + }); }); - it('returns match=false if diffMappings() finds differences', async () => { - diffMappingsMock.mockReturnValueOnce({ changedProp: 'field' }); - - const task = checkTargetMappings({ - actualMappings, - expectedMappings, + describe('when actual mappings are complete', () => { + describe('and mappings do not match', () => { + it('returns the lists of changed root fields and types', async () => { + const task = checkTargetMappings({ + indexTypes, + expectedMappings, + actualMappings: expectedMappings, + }); + + getUpdatedHashesMock.mockReturnValueOnce(['type1', 'type2', 'someRootField']); + + const result = await task(); + const expected: ComparedMappingsChanged = { + type: 'compared_mappings_changed' as const, + updatedRootFields: ['someRootField'], + updatedTypes: ['type1', 'type2'], + }; + expect(result).toEqual(Either.left(expected)); + }); }); - const result = await task(); - expect(result).toEqual(Either.right({ match: false })); + describe('and mappings match', () => { + it('returns a compared_mappings_match response', async () => { + const task = checkTargetMappings({ + indexTypes, + expectedMappings, + actualMappings: expectedMappings, + }); + + getUpdatedHashesMock.mockReturnValueOnce([]); + + const result = await task(); + const expected: ComparedMappingsMatch = { + type: 'compared_mappings_match' as const, + }; + expect(result).toEqual(Either.right(expected)); + }); + }); }); }); diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.ts index 876ce4873d447..26e0f074f43cb 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.ts @@ -8,29 +8,62 @@ import * as Either from 'fp-ts/lib/Either'; import * as TaskEither from 'fp-ts/lib/TaskEither'; -import { IndexMapping } from '@kbn/core-saved-objects-base-server-internal'; -import { diffMappings } from '../core/build_active_mappings'; +import type { IndexMapping } from '@kbn/core-saved-objects-base-server-internal'; +import { getUpdatedHashes } from '../core/build_active_mappings'; /** @internal */ export interface CheckTargetMappingsParams { + indexTypes: string[]; actualMappings?: IndexMapping; expectedMappings: IndexMapping; } /** @internal */ -export interface TargetMappingsCompareResult { - match: boolean; +export interface ComparedMappingsMatch { + type: 'compared_mappings_match'; +} + +export interface ActualMappingsIncomplete { + type: 'actual_mappings_incomplete'; +} + +export interface ComparedMappingsChanged { + type: 'compared_mappings_changed'; + updatedRootFields: string[]; + updatedTypes: string[]; } export const checkTargetMappings = ({ + indexTypes, actualMappings, expectedMappings, - }: CheckTargetMappingsParams): TaskEither.TaskEither => + }: CheckTargetMappingsParams): TaskEither.TaskEither< + ActualMappingsIncomplete | ComparedMappingsChanged, + ComparedMappingsMatch + > => async () => { - if (!actualMappings) { - return Either.right({ match: false }); + if ( + !actualMappings?._meta?.migrationMappingPropertyHashes || + actualMappings.dynamic !== expectedMappings.dynamic + ) { + return Either.left({ type: 'actual_mappings_incomplete' as const }); + } + + const updatedHashes = getUpdatedHashes({ + actual: actualMappings, + expected: expectedMappings, + }); + + if (updatedHashes.length) { + const updatedTypes = updatedHashes.filter((field) => indexTypes.includes(field)); + const updatedRootFields = updatedHashes.filter((field) => !indexTypes.includes(field)); + return Either.left({ + type: 'compared_mappings_changed' as const, + updatedRootFields, + updatedTypes, + }); + } else { + return Either.right({ type: 'compared_mappings_match' as const }); } - const diff = diffMappings(actualMappings, expectedMappings); - return Either.right({ match: !diff }); }; diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/index.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/index.ts index 5af2471a7f72e..270926a10cbab 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/index.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/index.ts @@ -108,6 +108,7 @@ import type { UnknownDocsFound } from './check_for_unknown_docs'; import type { IncompatibleClusterRoutingAllocation } from './initialize_action'; import type { ClusterShardLimitExceeded } from './create_index'; import type { SynchronizationFailed } from './synchronize_migrators'; +import type { ActualMappingsIncomplete, ComparedMappingsChanged } from './check_target_mappings'; export type { CheckForUnknownDocsParams, @@ -176,6 +177,8 @@ export interface ActionErrorTypeMap { cluster_shard_limit_exceeded: ClusterShardLimitExceeded; es_response_too_large: EsResponseTooLargeError; synchronization_failed: SynchronizationFailed; + actual_mappings_incomplete: ActualMappingsIncomplete; + compared_mappings_changed: ComparedMappingsChanged; } /** diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/pickup_updated_mappings.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/pickup_updated_mappings.ts index 8b6205cb7cc6f..632127dd2e084 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/pickup_updated_mappings.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/pickup_updated_mappings.ts @@ -9,6 +9,7 @@ import * as Either from 'fp-ts/lib/Either'; import * as TaskEither from 'fp-ts/lib/TaskEither'; import type { ElasticsearchClient } from '@kbn/core-elasticsearch-server'; +import type { QueryDslQueryContainer } from '@elastic/elasticsearch/lib/api/types'; import { catchRetryableEsClientErrors, type RetryableEsClientError, @@ -20,7 +21,7 @@ export interface UpdateByQueryResponse { /** * Pickup updated mappings by performing an update by query operation on all - * documents in the index. Returns a task ID which can be + * documents matching the passed in query. Returns a task ID which can be * tracked for progress. * * @remarks When mappings are updated to add a field which previously wasn't @@ -35,7 +36,8 @@ export const pickupUpdatedMappings = ( client: ElasticsearchClient, index: string, - batchSize: number + batchSize: number, + query?: QueryDslQueryContainer ): TaskEither.TaskEither => () => { return client @@ -52,6 +54,8 @@ export const pickupUpdatedMappings = refresh: true, // Create a task and return task id instead of blocking until complete wait_for_completion: false, + // Only update the documents that match the provided query + query, }) .then(({ task: taskId }) => { return Either.right({ taskId: String(taskId!) }); diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/update_and_pickup_mappings.test.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/update_and_pickup_mappings.test.ts index 6b227ea2ef66a..2aa4cdbd2830d 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/update_and_pickup_mappings.test.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/update_and_pickup_mappings.test.ts @@ -11,47 +11,76 @@ import { errors as EsErrors } from '@elastic/elasticsearch'; import { elasticsearchClientMock } from '@kbn/core-elasticsearch-client-server-mocks'; import { updateAndPickupMappings } from './update_and_pickup_mappings'; import { DEFAULT_TIMEOUT } from './constants'; +import { pickupUpdatedMappings } from './pickup_updated_mappings'; jest.mock('./catch_retryable_es_client_errors'); +jest.mock('./pickup_updated_mappings'); describe('updateAndPickupMappings', () => { beforeEach(() => { jest.clearAllMocks(); }); - // Create a mock client that rejects all methods with a 503 status code - // response. - const retryableError = new EsErrors.ResponseError( - elasticsearchClientMock.createApiResponse({ - statusCode: 503, - body: { error: { type: 'es_type', reason: 'es_reason' } }, - }) - ); - const client = elasticsearchClientMock.createInternalClient( - elasticsearchClientMock.createErrorTransportRequestPromise(retryableError) - ); + describe('putMappingTask', () => { + // Create a mock client that rejects all methods with a 503 status code + // response. + const retryableError = new EsErrors.ResponseError( + elasticsearchClientMock.createApiResponse({ + statusCode: 503, + body: { error: { type: 'es_type', reason: 'es_reason' } }, + }) + ); + const client = elasticsearchClientMock.createInternalClient( + elasticsearchClientMock.createErrorTransportRequestPromise(retryableError) + ); - it('calls catchRetryableEsClientErrors when the promise rejects', async () => { - const task = updateAndPickupMappings({ - client, - index: 'new_index', - mappings: { properties: {} }, - batchSize: 1000, + it('calls catchRetryableEsClientErrors when the promise rejects', async () => { + const task = updateAndPickupMappings({ + client, + index: 'new_index', + mappings: { properties: {} }, + batchSize: 1000, + }); + try { + await task(); + } catch (e) { + /** ignore */ + } + + expect(catchRetryableEsClientErrors).toHaveBeenCalledWith(retryableError); }); - try { - await task(); - } catch (e) { - /** ignore */ - } - expect(catchRetryableEsClientErrors).toHaveBeenCalledWith(retryableError); - }); + it('calls the indices.putMapping with the mapping properties as well as the _meta information', async () => { + const task = updateAndPickupMappings({ + client, + index: 'new_index', + mappings: { + properties: { + 'apm-indices': { + type: 'object', + dynamic: false, + }, + }, + _meta: { + migrationMappingPropertyHashes: { + references: '7997cf5a56cc02bdc9c93361bde732b0', + 'epm-packages': '860e23f4404fa1c33f430e6dad5d8fa2', + 'cases-connector-mappings': '17d2e9e0e170a21a471285a5d845353c', + }, + }, + }, + batchSize: 1000, + }); + try { + await task(); + } catch (e) { + /** ignore */ + } - it('calls the indices.putMapping with the mapping properties as well as the _meta information', async () => { - const task = updateAndPickupMappings({ - client, - index: 'new_index', - mappings: { + expect(client.indices.putMapping).toHaveBeenCalledTimes(1); + expect(client.indices.putMapping).toHaveBeenCalledWith({ + index: 'new_index', + timeout: DEFAULT_TIMEOUT, properties: { 'apm-indices': { type: 'object', @@ -65,32 +94,47 @@ describe('updateAndPickupMappings', () => { 'cases-connector-mappings': '17d2e9e0e170a21a471285a5d845353c', }, }, - }, - batchSize: 1000, + }); }); - try { - await task(); - } catch (e) { - /** ignore */ - } + }); - expect(client.indices.putMapping).toHaveBeenCalledTimes(1); - expect(client.indices.putMapping).toHaveBeenCalledWith({ - index: 'new_index', - timeout: DEFAULT_TIMEOUT, - properties: { - 'apm-indices': { - type: 'object', - dynamic: false, - }, - }, - _meta: { - migrationMappingPropertyHashes: { - references: '7997cf5a56cc02bdc9c93361bde732b0', - 'epm-packages': '860e23f4404fa1c33f430e6dad5d8fa2', - 'cases-connector-mappings': '17d2e9e0e170a21a471285a5d845353c', + describe('pickupUpdatedMappings', () => { + const client = elasticsearchClientMock.createInternalClient( + elasticsearchClientMock.createSuccessTransportRequestPromise({}) + ); + + it('calls pickupUpdatedMappings with the right parameters', async () => { + const query = { + bool: { + should: [ + { + term: { + type: 'type1', + }, + }, + { + term: { + type: 'type2', + }, + }, + ], }, - }, + }; + const task = updateAndPickupMappings({ + client, + index: 'new_index', + mappings: { properties: {} }, + batchSize: 1000, + query, + }); + try { + await task(); + } catch (e) { + /** ignore */ + } + + expect(pickupUpdatedMappings).toHaveBeenCalledTimes(1); + expect(pickupUpdatedMappings).toHaveBeenCalledWith(client, 'new_index', 1000, query); }); }); }); diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/update_and_pickup_mappings.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/update_and_pickup_mappings.ts index 58fd65c9718d0..8478ad08247a5 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/update_and_pickup_mappings.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/update_and_pickup_mappings.ts @@ -11,6 +11,7 @@ import * as TaskEither from 'fp-ts/lib/TaskEither'; import { pipe } from 'fp-ts/lib/pipeable'; import type { ElasticsearchClient } from '@kbn/core-elasticsearch-server'; import type { IndexMapping } from '@kbn/core-saved-objects-base-server-internal'; +import type { QueryDslQueryContainer } from '@elastic/elasticsearch/lib/api/types'; import { catchRetryableEsClientErrors, type RetryableEsClientError, @@ -29,6 +30,7 @@ export interface UpdateAndPickupMappingsParams { index: string; mappings: IndexMapping; batchSize: number; + query?: QueryDslQueryContainer; } /** * Updates an index's mappings and runs an pickupUpdatedMappings task so that the mapping @@ -39,6 +41,7 @@ export const updateAndPickupMappings = ({ index, mappings, batchSize, + query, }: UpdateAndPickupMappingsParams): TaskEither.TaskEither< RetryableEsClientError, UpdateAndPickupMappingsResponse @@ -76,7 +79,7 @@ export const updateAndPickupMappings = ({ return pipe( putMappingTask, TaskEither.chain((res) => { - return pickupUpdatedMappings(client, index, batchSize); + return pickupUpdatedMappings(client, index, batchSize, query); }) ); }; diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_active_mappings.test.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_active_mappings.test.ts index 7f1542ffc6008..7139e1e60b584 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_active_mappings.test.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_active_mappings.test.ts @@ -10,7 +10,7 @@ import type { IndexMapping, SavedObjectsTypeMappingDefinitions, } from '@kbn/core-saved-objects-base-server-internal'; -import { buildActiveMappings, diffMappings } from './build_active_mappings'; +import { buildActiveMappings, diffMappings, getUpdatedHashes } from './build_active_mappings'; describe('buildActiveMappings', () => { test('creates a strict mapping', () => { @@ -208,3 +208,65 @@ describe('diffMappings', () => { expect(diffMappings(actual, expected)!.changedProp).toEqual('_meta'); }); }); + +describe('getUpdatedHashes', () => { + test('gives all hashes if _meta is missing from actual', () => { + const actual: IndexMapping = { + dynamic: 'strict', + properties: {}, + }; + const expected: IndexMapping = { + _meta: { + migrationMappingPropertyHashes: { foo: 'bar', bar: 'baz' }, + }, + dynamic: 'strict', + properties: {}, + }; + + expect(getUpdatedHashes({ actual, expected })).toEqual(['foo', 'bar']); + }); + + test('gives all hashes if migrationMappingPropertyHashes is missing from actual', () => { + const actual: IndexMapping = { + dynamic: 'strict', + properties: {}, + _meta: {}, + }; + const expected: IndexMapping = { + _meta: { + migrationMappingPropertyHashes: { foo: 'bar', bar: 'baz' }, + }, + dynamic: 'strict', + properties: {}, + }; + + expect(getUpdatedHashes({ actual, expected })).toEqual(['foo', 'bar']); + }); + + test('gives a list of the types with updated hashes', () => { + const actual: IndexMapping = { + dynamic: 'strict', + properties: {}, + _meta: { + migrationMappingPropertyHashes: { + type1: 'type1hash1', + type2: 'type2hash1', + type3: 'type3hash1', // will be removed + }, + }, + }; + const expected: IndexMapping = { + dynamic: 'strict', + properties: {}, + _meta: { + migrationMappingPropertyHashes: { + type1: 'type1hash1', // remains the same + type2: 'type2hash2', // updated + type4: 'type4hash1', // new type + }, + }, + }; + + expect(getUpdatedHashes({ actual, expected })).toEqual(['type2', 'type4']); + }); +}); diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_active_mappings.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_active_mappings.ts index 7dd13acbe8c7f..feec8b5c2aa08 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_active_mappings.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_active_mappings.ts @@ -63,6 +63,30 @@ export function diffMappings(actual: IndexMapping, expected: IndexMapping) { return changedProp ? { changedProp: `properties.${changedProp}` } : undefined; } +/** + * Compares the actual vs expected mappings' hashes. + * Returns a list with all the hashes that have been updated. + */ +export const getUpdatedHashes = ({ + actual, + expected, +}: { + actual: IndexMapping; + expected: IndexMapping; +}): string[] => { + if (!actual._meta?.migrationMappingPropertyHashes) { + return Object.keys(expected._meta!.migrationMappingPropertyHashes!); + } + + const updatedHashes = Object.keys(expected._meta!.migrationMappingPropertyHashes!).filter( + (key) => + actual._meta!.migrationMappingPropertyHashes![key] !== + expected._meta!.migrationMappingPropertyHashes![key] + ); + + return updatedHashes; +}; + // Convert an object to an md5 hash string, using a stable serialization (canonicalStringify) function md5Object(obj: any) { return crypto.createHash('md5').update(canonicalStringify(obj)).digest('hex'); diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/helpers.test.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/helpers.test.ts index edf426d7be973..382eedfe0c2d8 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/helpers.test.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/helpers.test.ts @@ -7,6 +7,7 @@ */ import { FetchIndexResponse } from '../actions/fetch_indices'; +import { BaseState } from '../state'; import { addExcludedTypesToBoolQuery, addMustClausesToBoolQuery, @@ -20,6 +21,7 @@ import { createBulkIndexOperationTuple, hasLaterVersionAlias, aliasVersion, + getIndexTypes, } from './helpers'; describe('addExcludedTypesToBoolQuery', () => { @@ -444,3 +446,17 @@ describe('getTempIndexName', () => { expect(getTempIndexName('.kibana_cases', '8.8.0')).toEqual('.kibana_cases_8.8.0_reindex_temp'); }); }); + +describe('getIndexTypes', () => { + it("returns the list of types that belong to a migrator's index, based on its state", () => { + const baseState = { + indexPrefix: '.kibana_task_manager', + indexTypesMap: { + '.kibana': ['foo', 'bar'], + '.kibana_task_manager': ['task'], + }, + }; + + expect(getIndexTypes(baseState as unknown as BaseState)).toEqual(['task']); + }); +}); diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/helpers.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/helpers.ts index d3e8fcabfc432..4588c6639d883 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/helpers.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/helpers.ts @@ -17,7 +17,7 @@ import type { SavedObjectsRawDoc } from '@kbn/core-saved-objects-server'; import type { IndexMapping } from '@kbn/core-saved-objects-base-server-internal'; import type { AliasAction, FetchIndexResponse } from '../actions'; import type { BulkIndexOperationTuple } from './create_batches'; -import { OutdatedDocumentsSearchRead, ReindexSourceToTempRead } from '../state'; +import type { BaseState, OutdatedDocumentsSearchRead, ReindexSourceToTempRead } from '../state'; /** @internal */ export const REINDEX_TEMP_SUFFIX = '_reindex_temp'; @@ -323,3 +323,7 @@ export const increaseBatchSize = ( const increasedBatchSize = Math.floor(stateP.batchSize * 1.2); return increasedBatchSize > stateP.maxBatchSize ? stateP.maxBatchSize : increasedBatchSize; }; + +export const getIndexTypes = (state: BaseState): string[] => { + return state.indexTypesMap[state.indexPrefix]; +}; diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.test.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.test.ts index 6cb045f6e4d3b..39a03e1e28d47 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.test.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.test.ts @@ -2597,19 +2597,75 @@ describe('migrations v2 model', () => { targetIndex: '.kibana_7.11.0_001', }; - it('CHECK_TARGET_MAPPINGS -> UPDATE_TARGET_MAPPINGS_PROPERTIES if mappings do not match', () => { - const res: ResponseType<'CHECK_TARGET_MAPPINGS'> = Either.right({ match: false }); - const newState = model( - checkTargetMappingsState, - res - ) as UpdateTargetMappingsPropertiesState; - expect(newState.controlState).toBe('UPDATE_TARGET_MAPPINGS_PROPERTIES'); + describe('reindex migration', () => { + it('CHECK_TARGET_MAPPINGS -> UPDATE_TARGET_MAPPINGS_PROPERTIES if origin mappings did not exist', () => { + const res: ResponseType<'CHECK_TARGET_MAPPINGS'> = Either.left({ + type: 'actual_mappings_incomplete' as const, + }); + const newState = model( + checkTargetMappingsState, + res + ) as UpdateTargetMappingsPropertiesState; + expect(newState.controlState).toBe('UPDATE_TARGET_MAPPINGS_PROPERTIES'); + expect(Option.isNone(newState.updatedTypesQuery)).toEqual(true); + }); }); - it('CHECK_TARGET_MAPPINGS -> CHECK_VERSION_INDEX_READY_ACTIONS if mappings match', () => { - const res: ResponseType<'CHECK_TARGET_MAPPINGS'> = Either.right({ match: true }); - const newState = model(checkTargetMappingsState, res) as CheckVersionIndexReadyActions; - expect(newState.controlState).toBe('CHECK_VERSION_INDEX_READY_ACTIONS'); + describe('compatible migration', () => { + it('CHECK_TARGET_MAPPINGS -> UPDATE_TARGET_MAPPINGS_PROPERTIES if core fields have been updated', () => { + const res: ResponseType<'CHECK_TARGET_MAPPINGS'> = Either.left({ + type: 'compared_mappings_changed' as const, + updatedRootFields: ['namespaces'], + updatedTypes: ['dashboard', 'lens'], + }); + const newState = model( + checkTargetMappingsState, + res + ) as UpdateTargetMappingsPropertiesState; + expect(newState.controlState).toBe('UPDATE_TARGET_MAPPINGS_PROPERTIES'); + // since a core field has been updated, we must pickup ALL SOs. + // Thus, we must NOT define a filter query. + expect(Option.isNone(newState.updatedTypesQuery)).toEqual(true); + }); + + it('CHECK_TARGET_MAPPINGS -> UPDATE_TARGET_MAPPINGS_PROPERTIES if only SO types have changed', () => { + const res: ResponseType<'CHECK_TARGET_MAPPINGS'> = Either.left({ + type: 'compared_mappings_changed' as const, + updatedRootFields: [], + updatedTypes: ['dashboard', 'lens'], + }); + const newState = model( + checkTargetMappingsState, + res + ) as UpdateTargetMappingsPropertiesState; + expect(newState.controlState).toBe('UPDATE_TARGET_MAPPINGS_PROPERTIES'); + expect( + Option.isSome(newState.updatedTypesQuery) && newState.updatedTypesQuery.value + ).toEqual({ + bool: { + should: [ + { + term: { + type: 'dashboard', + }, + }, + { + term: { + type: 'lens', + }, + }, + ], + }, + }); + }); + + it('CHECK_TARGET_MAPPINGS -> CHECK_VERSION_INDEX_READY_ACTIONS if mappings match', () => { + const res: ResponseType<'CHECK_TARGET_MAPPINGS'> = Either.right({ + type: 'compared_mappings_match' as const, + }); + const newState = model(checkTargetMappingsState, res) as CheckVersionIndexReadyActions; + expect(newState.controlState).toBe('CHECK_VERSION_INDEX_READY_ACTIONS'); + }); }); }); @@ -2842,6 +2898,17 @@ describe('migrations v2 model', () => { versionIndexReadyActions: Option.none, sourceIndex: Option.some('.kibana') as Option.Some, targetIndex: '.kibana_7.11.0_001', + updatedTypesQuery: Option.fromNullable({ + bool: { + should: [ + { + term: { + type: 'type1', + }, + }, + ], + }, + }), }; test('UPDATE_TARGET_MAPPINGS_PROPERTIES -> UPDATE_TARGET_MAPPINGS_PROPERTIES_WAIT_FOR_TASK', () => { const res: ResponseType<'UPDATE_TARGET_MAPPINGS_PROPERTIES'> = Either.right({ diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.ts index ae1098f2209f5..2505581c6bc3d 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.ts @@ -1422,20 +1422,61 @@ export const model = (currentState: State, resW: ResponseType): } else if (stateP.controlState === 'CHECK_TARGET_MAPPINGS') { const res = resW as ResponseType; if (Either.isRight(res)) { - if (!res.right.match) { - return { - ...stateP, - controlState: 'UPDATE_TARGET_MAPPINGS_PROPERTIES', - }; - } - - // The md5 of the mappings match, so there's no need to update target mappings + // The md5 of ALL mappings match, so there's no need to update target mappings return { ...stateP, controlState: 'CHECK_VERSION_INDEX_READY_ACTIONS', }; } else { - throwBadResponse(stateP, res as never); + const left = res.left; + if (isTypeof(left, 'actual_mappings_incomplete')) { + // reindex migration + // some top-level properties have changed, e.g. 'dynamic' or '_meta' (see checkTargetMappings()) + // we must "pick-up" all documents on the index (by not providing a query) + return { + ...stateP, + controlState: 'UPDATE_TARGET_MAPPINGS_PROPERTIES', + updatedTypesQuery: Option.none, + }; + } else if (isTypeof(left, 'compared_mappings_changed')) { + if (left.updatedRootFields.length) { + // compatible migration: some core fields have been updated + return { + ...stateP, + controlState: 'UPDATE_TARGET_MAPPINGS_PROPERTIES', + // we must "pick-up" all documents on the index (by not providing a query) + updatedTypesQuery: Option.none, + logs: [ + ...stateP.logs, + { + level: 'info', + message: `Kibana is performing a compatible upgrade and the mappings of some root fields have been changed. For Elasticsearch to pickup these mappings, all saved objects need to be updated. Updated root fields: ${left.updatedRootFields}.`, + }, + ], + }; + } else { + // compatible migration: some fields have been updated, and they all correspond to SO types + return { + ...stateP, + controlState: 'UPDATE_TARGET_MAPPINGS_PROPERTIES', + // we can "pick-up" only the SO types that have changed + updatedTypesQuery: Option.some({ + bool: { + should: left.updatedTypes.map((type) => ({ term: { type } })), + }, + }), + logs: [ + ...stateP.logs, + { + level: 'info', + message: `Kibana is performing a compatible upgrade and NO root fields have been udpated. Kibana will update the following SO types so that ES can pickup the updated mappings: ${left.updatedTypes}.`, + }, + ], + }; + } + } else { + throwBadResponse(stateP, res as never); + } } } else if (stateP.controlState === 'UPDATE_TARGET_MAPPINGS_PROPERTIES') { const res = resW as ExcludeRetryableEsError>; diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/next.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/next.ts index 5509fb70c9231..df7e0c23fbc20 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/next.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/next.ts @@ -58,6 +58,7 @@ import { createDelayFn } from './common/utils'; import type { TransformRawDocs } from './types'; import * as Actions from './actions'; import { REMOVED_TYPES } from './core'; +import { getIndexTypes } from './model/helpers'; type ActionMap = ReturnType; @@ -201,6 +202,7 @@ export const nextActionMap = ( Actions.checkTargetMappings({ actualMappings: Option.toUndefined(state.sourceIndexMappings), expectedMappings: state.targetIndexMappings, + indexTypes: getIndexTypes(state), }), UPDATE_TARGET_MAPPINGS_PROPERTIES: (state: UpdateTargetMappingsPropertiesState) => Actions.updateAndPickupMappings({ @@ -208,6 +210,7 @@ export const nextActionMap = ( index: state.targetIndex, mappings: omit(state.targetIndexMappings, ['_meta']), // ._meta property will be updated on a later step batchSize: state.batchSize, + query: Option.toUndefined(state.updatedTypesQuery), }), UPDATE_TARGET_MAPPINGS_PROPERTIES_WAIT_FOR_TASK: ( state: UpdateTargetMappingsPropertiesWaitForTaskState diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/state.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/state.ts index 5dfc5a793046e..d057d464a94d3 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/state.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/state.ts @@ -374,6 +374,7 @@ export interface CheckTargetMappingsState extends PostInitState { export interface UpdateTargetMappingsPropertiesState extends PostInitState { /** Update the mappings of the target index */ readonly controlState: 'UPDATE_TARGET_MAPPINGS_PROPERTIES'; + readonly updatedTypesQuery: Option.Option; } export interface UpdateTargetMappingsPropertiesWaitForTaskState extends PostInitState { diff --git a/src/core/server/integration_tests/saved_objects/migrations/group3/dot_kibana_split.test.ts b/src/core/server/integration_tests/saved_objects/migrations/group3/dot_kibana_split.test.ts index 06abb539c3ef7..258a33fe591d6 100644 --- a/src/core/server/integration_tests/saved_objects/migrations/group3/dot_kibana_split.test.ts +++ b/src/core/server/integration_tests/saved_objects/migrations/group3/dot_kibana_split.test.ts @@ -326,6 +326,8 @@ describe('split .kibana index into multiple system indices', () => { // .kibana_task_manager migrator is NOT involved in relocation, must not sync with other migrators '[.kibana_task_manager] READY_TO_REINDEX_SYNC', '[.kibana_task_manager] DONE_REINDEXING_SYNC', + // .kibana_task_manager migrator performed a REINDEX migration, it must update ALL types + '[.kibana_task_manager] Kibana is performing a compatible update and it will update the following SO types so that ES can pickup the updated mappings', ]); // new indices migrators did not exist, so they all have to reindex (create temp index + sync) @@ -390,6 +392,9 @@ describe('split .kibana index into multiple system indices', () => { // should NOT retransform anything (we reindexed, thus we transformed already) ['.kibana', '.kibana_task_manager', '.kibana_so_ui', '.kibana_so_search'].forEach((index) => { expect(logs).not.toContainLogEntry(`[${index}] OUTDATED_DOCUMENTS_TRANSFORM`); + expect(logs).not.toContainLogEntry( + `[${index}] Kibana is performing a compatible update and it will update the following SO types so that ES can pickup the updated mappings` + ); }); }); diff --git a/src/core/server/integration_tests/saved_objects/migrations/group3/pickup_updated_types_only.test.ts b/src/core/server/integration_tests/saved_objects/migrations/group3/pickup_updated_types_only.test.ts new file mode 100644 index 0000000000000..5b2ab045ca2f0 --- /dev/null +++ b/src/core/server/integration_tests/saved_objects/migrations/group3/pickup_updated_types_only.test.ts @@ -0,0 +1,94 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +import Path from 'path'; +import type { TestElasticsearchUtils } from '@kbn/core-test-helpers-kbn-server'; +import { + clearLog, + createBaseline, + currentVersion, + defaultKibanaIndex, + defaultLogFilePath, + getCompatibleMappingsMigrator, + getIncompatibleMappingsMigrator, + startElasticsearch, +} from '../kibana_migrator_test_kit'; +import '../jest_matchers'; +import { delay, parseLogFile } from '../test_utils'; +import { IndexMappingMeta } from '@kbn/core-saved-objects-base-server-internal'; + +export const logFilePath = Path.join(__dirname, 'pickup_updated_types_only.test.log'); + +describe('pickupUpdatedMappings', () => { + let esServer: TestElasticsearchUtils['es']; + + beforeAll(async () => { + esServer = await startElasticsearch(); + }); + + beforeEach(async () => { + await createBaseline(); + await clearLog(); + }); + + describe('when performing a reindexing migration', () => { + it('should pickup all documents from the index', async () => { + const { runMigrations } = await getIncompatibleMappingsMigrator(); + + await runMigrations(); + + const logs = await parseLogFile(defaultLogFilePath); + + expect(logs).not.toContainLogEntry( + 'Kibana is performing a compatible upgrade and NO root fields have been udpated. Kibana will update the following SO types so that ES can pickup the updated mappings' + ); + }); + }); + + describe('when performing a compatible migration', () => { + it('should pickup only the types that have been updated', async () => { + const { runMigrations } = await getCompatibleMappingsMigrator(); + + await runMigrations(); + + const logs = await parseLogFile(defaultLogFilePath); + + expect(logs).toContainLogEntry( + 'Kibana is performing a compatible upgrade and NO root fields have been udpated. Kibana will update the following SO types so that ES can pickup the updated mappings: complex.' + ); + }); + + it('should pickup ALL documents if any root fields have been updated', async () => { + const { runMigrations, client } = await getCompatibleMappingsMigrator(); + + // we tamper the baseline mappings to simulate some root fields changes + const baselineMappings = await client.indices.getMapping({ index: defaultKibanaIndex }); + const _meta = baselineMappings[`${defaultKibanaIndex}_${currentVersion}_001`].mappings + ._meta as IndexMappingMeta; + _meta.migrationMappingPropertyHashes!.namespace = + _meta.migrationMappingPropertyHashes!.namespace + '_tampered'; + await client.indices.putMapping({ index: defaultKibanaIndex, _meta }); + + await runMigrations(); + + const logs = await parseLogFile(defaultLogFilePath); + + expect(logs).toContainLogEntry( + 'Kibana is performing a compatible upgrade and the mappings of some root fields have been changed. For Elasticsearch to pickup these mappings, all saved objects need to be updated. Updated root fields: namespace.' + ); + expect(logs).not.toContainLogEntry( + 'Kibana is performing a compatible upgrade and NO root fields have been udpated. Kibana will update the following SO types so that ES can pickup the updated mappings' + ); + }); + }); + + afterAll(async () => { + await esServer?.stop(); + await delay(2); + }); +}); diff --git a/src/core/server/integration_tests/saved_objects/migrations/kibana_migrator_test_kit.ts b/src/core/server/integration_tests/saved_objects/migrations/kibana_migrator_test_kit.ts index 57258aef0916e..26d1b2372a920 100644 --- a/src/core/server/integration_tests/saved_objects/migrations/kibana_migrator_test_kit.ts +++ b/src/core/server/integration_tests/saved_objects/migrations/kibana_migrator_test_kit.ts @@ -387,6 +387,16 @@ export const createBaseline = async () => { types: baselineTypes, }); + // remove the testing index (current and next minor) + await client.indices.delete({ + index: [ + defaultKibanaIndex, + `${defaultKibanaIndex}_${currentVersion}_001`, + `${defaultKibanaIndex}_${nextMinor}_001`, + ], + ignore_unavailable: true, + }); + await runMigrations(); await savedObjectsRepository.bulkCreate(baselineDocuments, { From 395ff8e7f4b4d316a1afad333419349de826b1cc Mon Sep 17 00:00:00 2001 From: Bena Kansara <69037875+benakansara@users.noreply.github.com> Date: Fri, 30 Jun 2023 17:32:15 +0200 Subject: [PATCH 4/7] Include group-by information in query for Logs spike analysis in Logs alert detail page (#160647) Resolves https://github.com/elastic/kibana/issues/160257 The group by field/value are added to the filter of the query that is passed to the Log Spikes analysis component. Example of the query when group by on `host.name` in the below Log threshold rule. Screenshot 2023-06-27 at 17 15 18 ### Before ``` { "bool": { "filter": [ { "term": { "log.level": { "value": "info" } } } ], "must_not": [ { "match": { "error.log.message": "test" } } ] } } ``` ### After ``` { "bool": { "filter": [ { "term": { "log.level": { "value": "info" } } }, { "term": { "host.name": { "value": "gke-edge-oblt-pool-2-ce49b4ca-l0nr" } } } ], "must_not": [ { "match": { "error.log.message": "test" } } ] } } ``` --- .../logs/log_threshold/query_helpers.ts | 4 ++- .../components/explain_log_rate_spike.tsx | 6 ++-- .../log_rate_spike_query.ts | 28 +++++++++++++++++-- 3 files changed, 31 insertions(+), 7 deletions(-) diff --git a/x-pack/plugins/infra/common/alerting/logs/log_threshold/query_helpers.ts b/x-pack/plugins/infra/common/alerting/logs/log_threshold/query_helpers.ts index a88d8fa767654..e2e656fe1831c 100644 --- a/x-pack/plugins/infra/common/alerting/logs/log_threshold/query_helpers.ts +++ b/x-pack/plugins/infra/common/alerting/logs/log_threshold/query_helpers.ts @@ -60,7 +60,9 @@ export const buildFiltersFromCriteria = ( }, }; - return { rangeFilter, groupedRangeFilter, mustFilters, mustNotFilters }; + const mustFiltersFields = positiveCriteria.map((criterion) => criterion.field); + + return { rangeFilter, groupedRangeFilter, mustFilters, mustNotFilters, mustFiltersFields }; }; const buildFiltersForCriteria = (criteria: CountCriteria) => { diff --git a/x-pack/plugins/infra/public/alerting/log_threshold/components/alert_details_app_section/components/explain_log_rate_spike.tsx b/x-pack/plugins/infra/public/alerting/log_threshold/components/alert_details_app_section/components/explain_log_rate_spike.tsx index e0e3205ac29af..b51c9beae3836 100644 --- a/x-pack/plugins/infra/public/alerting/log_threshold/components/alert_details_app_section/components/explain_log_rate_spike.tsx +++ b/x-pack/plugins/infra/public/alerting/log_threshold/components/alert_details_app_section/components/explain_log_rate_spike.tsx @@ -28,7 +28,6 @@ import { useKibanaContextForPlugin } from '../../../../../hooks/use_kibana'; import { Comparator, CountRuleParams, - hasGroupBy, isRatioRuleParams, PartialRuleParams, ruleParamsRT, @@ -76,7 +75,9 @@ export const ExplainLogRateSpikes: FC { const esSearchRequest = getESQueryForLogSpike( validatedParams as CountRuleParams, - timestampField + timestampField, + alert, + rule.params.groupBy ) as QueryDslQueryContainer; if (esSearchRequest) { @@ -88,7 +89,6 @@ export const ExplainLogRateSpikes: FC & { criteria: CountCriteria }, - timestampField: string + timestampField: string, + alert: TopAlert>, + groupBy?: string[] | undefined ): object => { - const { mustFilters, mustNotFilters } = buildFiltersFromCriteria(params, timestampField); + const { mustFilters, mustNotFilters, mustFiltersFields } = buildFiltersFromCriteria( + params, + timestampField + ); + + const groupByFilters = groupBy + ? groupBy + .filter((groupByField) => !mustFiltersFields.includes(groupByField)) + .map((groupByField) => { + const groupByValue = get( + alert.fields[ALERT_CONTEXT], + ['groupByKeys', ...groupByField.split('.')], + null + ); + return groupByValue ? { term: { [groupByField]: { value: groupByValue } } } : null; + }) + .filter((groupByFilter) => groupByFilter) + : []; const query = { bool: { - filter: mustFilters, + filter: [...mustFilters, ...groupByFilters], ...(mustNotFilters.length > 0 && { must_not: mustNotFilters }), }, }; From 9ef13cd2943cd48d93cb7cee976dc6d279f774b1 Mon Sep 17 00:00:00 2001 From: Janki Salvi <117571355+js-jankisalvi@users.noreply.github.com> Date: Fri, 30 Jun 2023 17:52:09 +0200 Subject: [PATCH 5/7] [Cases] Create and update case API guardrails for title, description, category, tags (#160844) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Connected to https://github.com/elastic/kibana/issues/146945 ## Summary Description | Limit | Done? | Documented? -- | -- | -- | -- Total number of description characters | 30.000 | ✅ | Yes Total number of tags per case | 200 | ✅ | Yes Total number of characters per tag | 256 | ✅ | Yes - Used schema validation. - Updated documentation. - Added jest and integration tests. **Note:** In this PR, `maximum length of title (160 characters)` and` maximum length of category field (50 characters)` validations are also moved to schema validation. ### Checklist - [x] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### Release notes The Create Case and Update Case APIs put the following limits: - Total number of characters per description: 30K - Total number of tags per case: 200 - Total number of characters per tag: 256 --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> --- x-pack/plugins/cases/common/api/cases/case.ts | 70 +++- .../plugins/cases/common/constants/index.ts | 3 + .../plugins/cases/common/schema/index.test.ts | 149 +++++-- x-pack/plugins/cases/common/schema/index.ts | 44 ++- .../cases/common/utils/validators.test.ts | 51 +-- .../plugins/cases/common/utils/validators.ts | 8 +- .../plugins/cases/docs/openapi/bundled.json | 60 ++- .../plugins/cases/docs/openapi/bundled.yaml | 28 +- .../schemas/create_case_request.yaml | 10 +- .../schemas/update_case_request.yaml | 8 + .../cases/server/client/cases/create.test.ts | 149 ++++++- .../cases/server/client/cases/create.ts | 37 +- .../cases/server/client/cases/update.test.ts | 368 +++++++++++++++++- .../cases/server/client/cases/update.ts | 63 +-- .../tests/common/cases/patch_cases.ts | 161 ++++++++ .../tests/common/cases/post_case.ts | 18 + 16 files changed, 986 insertions(+), 241 deletions(-) diff --git a/x-pack/plugins/cases/common/api/cases/case.ts b/x-pack/plugins/cases/common/api/cases/case.ts index 7fcbc86e3d340..fd734c943de0e 100644 --- a/x-pack/plugins/cases/common/api/cases/case.ts +++ b/x-pack/plugins/cases/common/api/cases/case.ts @@ -13,9 +13,14 @@ import { CommentRt } from './comment'; import { CasesStatusResponseRt, CaseStatusRt } from './status'; import { CaseConnectorRt } from '../connectors/connector'; import { CaseAssigneesRt } from './assignee'; -import { limitedArraySchema, NonEmptyString } from '../../schema'; +import { limitedArraySchema, limitedStringSchema, NonEmptyString } from '../../schema'; import { MAX_DELETE_IDS_LENGTH, + MAX_DESCRIPTION_LENGTH, + MAX_TITLE_LENGTH, + MAX_LENGTH_PER_TAG, + MAX_CATEGORY_LENGTH, + MAX_TAGS_PER_CASE, MAX_ASSIGNEES_FILTER_LENGTH, MAX_REPORTERS_FILTER_LENGTH, MAX_TAGS_FILTER_LENGTH, @@ -139,15 +144,20 @@ export const CasePostRequestRt = rt.intersection([ /** * Description of the case */ - description: rt.string, + description: limitedStringSchema('description', 1, MAX_DESCRIPTION_LENGTH), /** * Identifiers for the case. */ - tags: rt.array(rt.string), + tags: limitedArraySchema( + limitedStringSchema('tag', 1, MAX_LENGTH_PER_TAG), + 0, + MAX_TAGS_PER_CASE, + 'tags' + ), /** * Title of the case */ - title: rt.string, + title: limitedStringSchema('title', 1, MAX_TITLE_LENGTH), /** * The external configuration for the case */ @@ -176,7 +186,7 @@ export const CasePostRequestRt = rt.intersection([ /** * The category of the case. */ - category: rt.union([rt.string, rt.null]), + category: rt.union([limitedStringSchema('category', 1, MAX_CATEGORY_LENGTH), rt.null]), }) ), ]); @@ -355,7 +365,55 @@ export const CasesFindResponseRt = rt.intersection([ ]); export const CasePatchRequestRt = rt.intersection([ - rt.exact(rt.partial(CaseBasicRt.type.props)), + rt.exact( + rt.partial({ + /** + * The description of the case + */ + description: limitedStringSchema('description', 1, MAX_DESCRIPTION_LENGTH), + /** + * The current status of the case (open, closed, in-progress) + */ + status: CaseStatusRt, + /** + * The identifying strings for filter a case + */ + tags: limitedArraySchema( + limitedStringSchema('tag', 1, MAX_LENGTH_PER_TAG), + 0, + MAX_TAGS_PER_CASE, + 'tags' + ), + /** + * The title of a case + */ + title: limitedStringSchema('title', 1, MAX_TITLE_LENGTH), + /** + * The external system that the case can be synced with + */ + connector: CaseConnectorRt, + /** + * The alert sync settings + */ + settings: SettingsRt, + /** + * The plugin owner of the case + */ + owner: rt.string, + /** + * The severity of the case + */ + severity: CaseSeverityRt, + /** + * The users assigned to this case + */ + assignees: CaseAssigneesRt, + /** + * The category of the case. + */ + category: rt.union([limitedStringSchema('category', 1, MAX_CATEGORY_LENGTH), rt.null]), + }) + ), /** * The saved object ID and version */ diff --git a/x-pack/plugins/cases/common/constants/index.ts b/x-pack/plugins/cases/common/constants/index.ts index d41efe9e8d85e..bee8ff9f3b17e 100644 --- a/x-pack/plugins/cases/common/constants/index.ts +++ b/x-pack/plugins/cases/common/constants/index.ts @@ -116,6 +116,9 @@ export const MAX_REPORTERS_FILTER_LENGTH = 100 as const; export const MAX_TITLE_LENGTH = 160 as const; export const MAX_CATEGORY_LENGTH = 50 as const; +export const MAX_DESCRIPTION_LENGTH = 30000 as const; +export const MAX_LENGTH_PER_TAG = 256 as const; +export const MAX_TAGS_PER_CASE = 200 as const; export const MAX_DELETE_IDS_LENGTH = 100 as const; /** diff --git a/x-pack/plugins/cases/common/schema/index.test.ts b/x-pack/plugins/cases/common/schema/index.test.ts index 3f1cfa549955f..826ee6ffbcd31 100644 --- a/x-pack/plugins/cases/common/schema/index.test.ts +++ b/x-pack/plugins/cases/common/schema/index.test.ts @@ -7,70 +7,141 @@ import { PathReporter } from 'io-ts/lib/PathReporter'; -import { limitedArraySchema, NonEmptyString } from '.'; +import { limitedArraySchema, limitedStringSchema, NonEmptyString } from '.'; describe('schema', () => { - it('fails when given an empty string', () => { - expect(PathReporter.report(limitedArraySchema(NonEmptyString, 1, 1).decode(['']))) - .toMatchInlineSnapshot(` - Array [ - "string must have length >= 1", - ] - `); - }); + describe('limitedArraySchema', () => { + const fieldName = 'foobar'; - it('fails when given an empty array', () => { - expect(PathReporter.report(limitedArraySchema(NonEmptyString, 1, 1).decode([]))) - .toMatchInlineSnapshot(` + it('fails when given an empty string', () => { + expect(PathReporter.report(limitedArraySchema(NonEmptyString, 1, 1, fieldName).decode(['']))) + .toMatchInlineSnapshot(` Array [ - "Array must be of length >= 1.", - ] - `); - }); - - it('fails when given an array larger than the limit of one item', () => { - expect(PathReporter.report(limitedArraySchema(NonEmptyString, 1, 1).decode(['a', 'b']))) - .toMatchInlineSnapshot(` - Array [ - "Array must be of length <= 1.", + "string must have length >= 1", ] `); - }); + }); - it('displays field name error message when lower boundary fails', () => { - expect(PathReporter.report(limitedArraySchema(NonEmptyString, 1, 1, 'foobar').decode([]))) - .toMatchInlineSnapshot(` + it('fails when given an empty array', () => { + expect(PathReporter.report(limitedArraySchema(NonEmptyString, 1, 1, fieldName).decode([]))) + .toMatchInlineSnapshot(` Array [ "The length of the field foobar is too short. Array must be of length >= 1.", ] `); - }); + }); - it('displays field name error message when upper boundary fails', () => { - expect( - PathReporter.report(limitedArraySchema(NonEmptyString, 1, 1, 'foobar').decode(['a', 'b'])) - ).toMatchInlineSnapshot(` + it('fails when given an array larger than the limit of one item', () => { + expect( + PathReporter.report(limitedArraySchema(NonEmptyString, 1, 1, fieldName).decode(['a', 'b'])) + ).toMatchInlineSnapshot(` Array [ "The length of the field foobar is too long. Array must be of length <= 1.", ] `); - }); + }); - it('succeeds when given an array of 1 item with a non-empty string', () => { - expect(PathReporter.report(limitedArraySchema(NonEmptyString, 1, 1).decode(['a']))) - .toMatchInlineSnapshot(` + it('succeeds when given an array of 1 item with a non-empty string', () => { + expect(PathReporter.report(limitedArraySchema(NonEmptyString, 1, 1, fieldName).decode(['a']))) + .toMatchInlineSnapshot(` Array [ "No errors!", ] `); - }); + }); - it('succeeds when given an array of 0 item with a non-empty string when the min is 0', () => { - expect(PathReporter.report(limitedArraySchema(NonEmptyString, 0, 2).decode([]))) - .toMatchInlineSnapshot(` + it('succeeds when given an array of 0 item with a non-empty string when the min is 0', () => { + expect(PathReporter.report(limitedArraySchema(NonEmptyString, 0, 2, fieldName).decode([]))) + .toMatchInlineSnapshot(` Array [ "No errors!", ] `); + }); + }); + + describe('limitedStringSchema', () => { + const fieldName = 'foo'; + + it('fails when given string is shorter than minimum', () => { + expect(PathReporter.report(limitedStringSchema(fieldName, 2, 1).decode('a'))) + .toMatchInlineSnapshot(` + Array [ + "The length of the ${fieldName} is too short. The minimum length is 2.", + ] + `); + }); + + it('fails when given string is empty and minimum is not 0', () => { + expect(PathReporter.report(limitedStringSchema(fieldName, 1, 1).decode(''))) + .toMatchInlineSnapshot(` + Array [ + "The ${fieldName} field cannot be an empty string.", + ] + `); + }); + + it('fails when given string consists only empty characters and minimum is not 0', () => { + expect(PathReporter.report(limitedStringSchema(fieldName, 1, 1).decode(' '))) + .toMatchInlineSnapshot(` + Array [ + "The ${fieldName} field cannot be an empty string.", + ] + `); + }); + + it('fails when given string is larger than maximum', () => { + expect(PathReporter.report(limitedStringSchema(fieldName, 1, 5).decode('Hello there!!'))) + .toMatchInlineSnapshot(` + Array [ + "The length of the ${fieldName} is too long. The maximum length is 5.", + ] + `); + }); + + it('succeeds when given string within limit', () => { + expect(PathReporter.report(limitedStringSchema(fieldName, 1, 50).decode('Hello!!'))) + .toMatchInlineSnapshot(` + Array [ + "No errors!", + ] + `); + }); + + it('succeeds when given string is empty and minimum is 0', () => { + expect(PathReporter.report(limitedStringSchema(fieldName, 0, 5).decode(''))) + .toMatchInlineSnapshot(` + Array [ + "No errors!", + ] + `); + }); + + it('succeeds when given string consists only empty characters and minimum is 0', () => { + expect(PathReporter.report(limitedStringSchema(fieldName, 0, 5).decode(' '))) + .toMatchInlineSnapshot(` + Array [ + "No errors!", + ] + `); + }); + + it('succeeds when given string is same as maximum', () => { + expect(PathReporter.report(limitedStringSchema(fieldName, 0, 5).decode('Hello'))) + .toMatchInlineSnapshot(` + Array [ + "No errors!", + ] + `); + }); + + it('succeeds when given string is larger than maximum but same as maximum after trim', () => { + expect(PathReporter.report(limitedStringSchema(fieldName, 0, 5).decode('Hello '))) + .toMatchInlineSnapshot(` + Array [ + "No errors!", + ] + `); + }); }); }); diff --git a/x-pack/plugins/cases/common/schema/index.ts b/x-pack/plugins/cases/common/schema/index.ts index 5e430665b3750..1f8e4e05c4933 100644 --- a/x-pack/plugins/cases/common/schema/index.ts +++ b/x-pack/plugins/cases/common/schema/index.ts @@ -22,11 +22,44 @@ export const NonEmptyString = new rt.Type( rt.identity ); +export const limitedStringSchema = (fieldName: string, min: number, max: number) => + new rt.Type( + 'LimitedString', + rt.string.is, + (input, context) => + either.chain(rt.string.validate(input, context), (s) => { + const trimmedString = s.trim(); + + if (trimmedString.length === 0 && trimmedString.length < min) { + return rt.failure(input, context, `The ${fieldName} field cannot be an empty string.`); + } + + if (trimmedString.length < min) { + return rt.failure( + input, + context, + `The length of the ${fieldName} is too short. The minimum length is ${min}.` + ); + } + + if (trimmedString.length > max) { + return rt.failure( + input, + context, + `The length of the ${fieldName} is too long. The maximum length is ${max}.` + ); + } + + return rt.success(s); + }), + rt.identity + ); + export const limitedArraySchema = ( codec: T, min: number, max: number, - fieldName?: string + fieldName: string ) => new rt.Type>, Array>, unknown>( 'LimitedArray', @@ -34,23 +67,18 @@ export const limitedArraySchema = ( (input, context) => either.chain(rt.array(codec).validate(input, context), (s) => { if (s.length < min) { - const fieldNameErrorMessage = - fieldName != null ? `The length of the field ${fieldName} is too short. ` : ''; - return rt.failure( input, context, - `${fieldNameErrorMessage}Array must be of length >= ${min}.` + `The length of the field ${fieldName} is too short. Array must be of length >= ${min}.` ); } if (s.length > max) { - const fieldNameErrorMessage = - fieldName != null ? `The length of the field ${fieldName} is too long. ` : ''; return rt.failure( input, context, - `${fieldNameErrorMessage}Array must be of length <= ${max}.` + `The length of the field ${fieldName} is too long. Array must be of length <= ${max}.` ); } diff --git a/x-pack/plugins/cases/common/utils/validators.test.ts b/x-pack/plugins/cases/common/utils/validators.test.ts index 78675a18422ff..e05e9223387b6 100644 --- a/x-pack/plugins/cases/common/utils/validators.test.ts +++ b/x-pack/plugins/cases/common/utils/validators.test.ts @@ -5,13 +5,8 @@ * 2.0. */ -import { MAX_ASSIGNEES_PER_CASE, MAX_CATEGORY_LENGTH } from '../constants'; -import { - isInvalidTag, - areTotalAssigneesInvalid, - isCategoryFieldInvalidString, - isCategoryFieldTooLong, -} from './validators'; +import { MAX_ASSIGNEES_PER_CASE } from '../constants'; +import { isInvalidTag, areTotalAssigneesInvalid } from './validators'; describe('validators', () => { describe('isInvalidTag', () => { @@ -55,46 +50,4 @@ describe('validators', () => { expect(areTotalAssigneesInvalid(generateAssignees(MAX_ASSIGNEES_PER_CASE + 1))).toBe(true); }); }); - - describe('isCategoryFieldInvalidString', () => { - it('validates undefined categories correctly', () => { - expect(isCategoryFieldInvalidString()).toBe(false); - }); - - it('validates null categories correctly', () => { - expect(isCategoryFieldInvalidString(null)).toBe(false); - }); - - it('returns false if the category is a non-empty string', () => { - expect(isCategoryFieldInvalidString('foobar')).toBe(false); - }); - - it('returns true if the category is an empty string', () => { - expect(isCategoryFieldInvalidString('')).toBe(true); - }); - - it('returns true if the string contains only spaces', () => { - expect(isCategoryFieldInvalidString(' ')).toBe(true); - }); - }); - - describe('isCategoryFieldTooLong', () => { - it('validates undefined categories correctly', () => { - expect(isCategoryFieldTooLong()).toBe(false); - }); - - it('validates null categories correctly', () => { - expect(isCategoryFieldTooLong(null)).toBe(false); - }); - - it(`returns false if the category is smaller than ${MAX_CATEGORY_LENGTH}`, () => { - expect(isCategoryFieldTooLong('foobar')).toBe(false); - }); - - it(`returns true if the category is longer than ${MAX_CATEGORY_LENGTH}`, () => { - expect(isCategoryFieldTooLong('A very long category with more than fifty characters!')).toBe( - true - ); - }); - }); }); diff --git a/x-pack/plugins/cases/common/utils/validators.ts b/x-pack/plugins/cases/common/utils/validators.ts index 98880e3b445a9..2358ed0a9c2c3 100644 --- a/x-pack/plugins/cases/common/utils/validators.ts +++ b/x-pack/plugins/cases/common/utils/validators.ts @@ -6,7 +6,7 @@ */ import type { CaseAssignees } from '../api'; -import { MAX_ASSIGNEES_PER_CASE, MAX_CATEGORY_LENGTH } from '../constants'; +import { MAX_ASSIGNEES_PER_CASE } from '../constants'; export const isInvalidTag = (value: string) => value.trim() === ''; @@ -17,9 +17,3 @@ export const areTotalAssigneesInvalid = (assignees?: CaseAssignees): boolean => return assignees.length > MAX_ASSIGNEES_PER_CASE; }; - -export const isCategoryFieldInvalidString = (category?: string | null): boolean => - category?.trim().length === 0; - -export const isCategoryFieldTooLong = (category?: string | null): boolean => - category != null && category.length > MAX_CATEGORY_LENGTH; diff --git a/x-pack/plugins/cases/docs/openapi/bundled.json b/x-pack/plugins/cases/docs/openapi/bundled.json index 641db779ff346..4d6905006c08d 100644 --- a/x-pack/plugins/cases/docs/openapi/bundled.json +++ b/x-pack/plugins/cases/docs/openapi/bundled.json @@ -12,18 +12,26 @@ "url": "https://www.elastic.co/licensing/elastic-license" } }, - "tags": [ - { - "name": "cases", - "description": "Case APIs enable you to open and track issues." - } - ], "servers": [ { "url": "http://localhost:5601", "description": "local" } ], + "security": [ + { + "basicAuth": [] + }, + { + "apiKeyAuth": [] + } + ], + "tags": [ + { + "name": "cases", + "description": "Case APIs enable you to open and track issues." + } + ], "paths": { "/api/cases": { "post": { @@ -4609,7 +4617,8 @@ }, "description": { "description": "The description for the case.", - "type": "string" + "type": "string", + "maxLength": 30000 }, "owner": { "$ref": "#/components/schemas/owners" @@ -4623,13 +4632,21 @@ "tags": { "description": "The words and phrases that help categorize cases. It can be an empty array.", "type": "array", + "maxItems": 200, "items": { - "type": "string" + "type": "string", + "maxLength": 256 } }, + "category": { + "description": "Category for the case. It could be a word or a phrase to categorize the case.", + "type": "string", + "maxLength": 50 + }, "title": { "description": "A title for the case.", - "type": "string" + "type": "string", + "maxLength": 160 } } }, @@ -5237,7 +5254,8 @@ }, "id": { "description": "The identifier for the case.", - "type": "string" + "type": "string", + "maxLength": 30000 }, "settings": { "$ref": "#/components/schemas/settings" @@ -5251,13 +5269,21 @@ "tags": { "description": "The words and phrases that help categorize cases.", "type": "array", + "maxItems": 200, "items": { - "type": "string" + "type": "string", + "maxLength": 256 } }, + "category": { + "description": "Category for the case. It could be a word or a phrase to categorize the case.", + "type": "string", + "maxLength": 50 + }, "title": { "description": "A title for the case.", - "type": "string" + "type": "string", + "maxLength": 160 }, "version": { "description": "The current version of the case. To determine this value, use the get case or find cases APIs.", @@ -7135,13 +7161,5 @@ ] } } - }, - "security": [ - { - "basicAuth": [] - }, - { - "apiKeyAuth": [] - } - ] + } } \ No newline at end of file diff --git a/x-pack/plugins/cases/docs/openapi/bundled.yaml b/x-pack/plugins/cases/docs/openapi/bundled.yaml index bf961003f706f..7608a44f3e45e 100644 --- a/x-pack/plugins/cases/docs/openapi/bundled.yaml +++ b/x-pack/plugins/cases/docs/openapi/bundled.yaml @@ -8,12 +8,15 @@ info: license: name: Elastic License 2.0 url: https://www.elastic.co/licensing/elastic-license -tags: - - name: cases - description: Case APIs enable you to open and track issues. servers: - url: http://localhost:5601 description: local +security: + - basicAuth: [] + - apiKeyAuth: [] +tags: + - name: cases + description: Case APIs enable you to open and track issues. paths: /api/cases: post: @@ -2907,6 +2910,7 @@ components: description: description: The description for the case. type: string + maxLength: 30000 owner: $ref: '#/components/schemas/owners' settings: @@ -2916,11 +2920,18 @@ components: tags: description: The words and phrases that help categorize cases. It can be an empty array. type: array + maxItems: 200 items: type: string + maxLength: 256 + category: + description: Category for the case. It could be a word or a phrase to categorize the case. + type: string + maxLength: 50 title: description: A title for the case. type: string + maxLength: 160 case_response_closed_by_properties: title: Case response properties for closed_by type: object @@ -3358,6 +3369,7 @@ components: id: description: The identifier for the case. type: string + maxLength: 30000 settings: $ref: '#/components/schemas/settings' severity: @@ -3367,11 +3379,18 @@ components: tags: description: The words and phrases that help categorize cases. type: array + maxItems: 200 items: type: string + maxLength: 256 + category: + description: Category for the case. It could be a word or a phrase to categorize the case. + type: string + maxLength: 50 title: description: A title for the case. type: string + maxLength: 160 version: description: The current version of the case. To determine this value, use the get case or find cases APIs. type: string @@ -4753,6 +4772,3 @@ components: isPreconfigured: false isDeprecated: false referencedByCount: 0 -security: - - basicAuth: [] - - apiKeyAuth: [] diff --git a/x-pack/plugins/cases/docs/openapi/components/schemas/create_case_request.yaml b/x-pack/plugins/cases/docs/openapi/components/schemas/create_case_request.yaml index 9f6b039fb2563..1092975985dd0 100644 --- a/x-pack/plugins/cases/docs/openapi/components/schemas/create_case_request.yaml +++ b/x-pack/plugins/cases/docs/openapi/components/schemas/create_case_request.yaml @@ -24,6 +24,7 @@ properties: description: description: The description for the case. type: string + maxLength: 30000 owner: $ref: 'owners.yaml' settings: @@ -33,8 +34,15 @@ properties: tags: description: The words and phrases that help categorize cases. It can be an empty array. type: array + maxItems: 200 items: type: string + maxLength: 256 + category: + description: Category for the case. It could be a word or a phrase to categorize the case. + type: string + maxLength: 50 title: description: A title for the case. - type: string \ No newline at end of file + type: string + maxLength: 160 \ No newline at end of file diff --git a/x-pack/plugins/cases/docs/openapi/components/schemas/update_case_request.yaml b/x-pack/plugins/cases/docs/openapi/components/schemas/update_case_request.yaml index 35170dc4ef3eb..bd630aec9ca5a 100644 --- a/x-pack/plugins/cases/docs/openapi/components/schemas/update_case_request.yaml +++ b/x-pack/plugins/cases/docs/openapi/components/schemas/update_case_request.yaml @@ -31,6 +31,7 @@ properties: id: description: The identifier for the case. type: string + maxLength: 30000 settings: $ref: 'settings.yaml' severity: @@ -40,11 +41,18 @@ properties: tags: description: The words and phrases that help categorize cases. type: array + maxItems: 200 items: type: string + maxLength: 256 + category: + description: Category for the case. It could be a word or a phrase to categorize the case. + type: string + maxLength: 50 title: description: A title for the case. type: string + maxLength: 160 version: description: The current version of the case. To determine this value, use the get case or find cases APIs. type: string \ No newline at end of file diff --git a/x-pack/plugins/cases/server/client/cases/create.test.ts b/x-pack/plugins/cases/server/client/cases/create.test.ts index 1459eea6f773d..7c57f85336ea9 100644 --- a/x-pack/plugins/cases/server/client/cases/create.test.ts +++ b/x-pack/plugins/cases/server/client/cases/create.test.ts @@ -5,6 +5,12 @@ * 2.0. */ +import { + MAX_DESCRIPTION_LENGTH, + MAX_TAGS_PER_CASE, + MAX_LENGTH_PER_TAG, + MAX_TITLE_LENGTH, +} from '../../../common/constants'; import { SECURITY_SOLUTION_OWNER } from '../../../common'; import { CaseSeverity, ConnectorTypes } from '../../../common/api'; import { mockCases } from '../../mocks'; @@ -94,6 +100,145 @@ describe('create', () => { `"Failed to create case: Error: invalid keys \\"foo\\""` ); }); + }); + + describe('title', () => { + const clientArgs = createCasesClientMockArgs(); + clientArgs.services.caseService.postNewCase.mockResolvedValue(caseSO); + + beforeEach(() => { + jest.clearAllMocks(); + }); + + it(`should not throw an error if the title is non empty and less than ${MAX_TITLE_LENGTH} characters`, async () => { + await expect( + create({ ...theCase, title: 'This is a test case!!' }, clientArgs) + ).resolves.not.toThrow(); + }); + + it('should throw an error if the title length is too long', async () => { + await expect( + create( + { + ...theCase, + title: + 'This is a very long title with more than one hundred and sixty characters!! To confirm the maximum limit error thrown for more than one hundred and sixty characters!!', + }, + clientArgs + ) + ).rejects.toThrow( + `Failed to create case: Error: The length of the title is too long. The maximum length is ${MAX_TITLE_LENGTH}.` + ); + }); + + it('should throw an error if the title is an empty string', async () => { + await expect(create({ ...theCase, title: '' }, clientArgs)).rejects.toThrow( + 'Failed to create case: Error: The title field cannot be an empty string.' + ); + }); + + it('should throw an error if the title is a string with empty characters', async () => { + await expect(create({ ...theCase, title: ' ' }, clientArgs)).rejects.toThrow( + 'Failed to create case: Error: The title field cannot be an empty string.' + ); + }); + }); + + describe('description', () => { + const clientArgs = createCasesClientMockArgs(); + clientArgs.services.caseService.postNewCase.mockResolvedValue(caseSO); + + beforeEach(() => { + jest.clearAllMocks(); + }); + + it(`should not throw an error if the description is non empty and less than ${MAX_DESCRIPTION_LENGTH} characters`, async () => { + await expect( + create({ ...theCase, description: 'This is a test description!!' }, clientArgs) + ).resolves.not.toThrow(); + }); + + it('should throw an error if the description length is too long', async () => { + const description = Array(MAX_DESCRIPTION_LENGTH + 1) + .fill('x') + .toString(); + + await expect(create({ ...theCase, description }, clientArgs)).rejects.toThrow( + `Failed to create case: Error: The length of the description is too long. The maximum length is ${MAX_DESCRIPTION_LENGTH}.` + ); + }); + + it('should throw an error if the description is an empty string', async () => { + await expect(create({ ...theCase, description: '' }, clientArgs)).rejects.toThrow( + 'Failed to create case: Error: The description field cannot be an empty string.' + ); + }); + + it('should throw an error if the description is a string with empty characters', async () => { + await expect(create({ ...theCase, description: ' ' }, clientArgs)).rejects.toThrow( + 'Failed to create case: Error: The description field cannot be an empty string.' + ); + }); + }); + + describe('tags', () => { + const clientArgs = createCasesClientMockArgs(); + clientArgs.services.caseService.postNewCase.mockResolvedValue(caseSO); + + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('should not throw an error if the tags array is empty', async () => { + await expect(create({ ...theCase, tags: [] }, clientArgs)).resolves.not.toThrow(); + }); + + it('should not throw an error if the tags array has non empty string within limit', async () => { + await expect(create({ ...theCase, tags: ['abc'] }, clientArgs)).resolves.not.toThrow(); + }); + + it('should throw an error if the tags array length is too long', async () => { + const tags = Array(MAX_TAGS_PER_CASE + 1).fill('foo'); + + await expect(create({ ...theCase, tags }, clientArgs)).rejects.toThrow( + `Failed to create case: Error: The length of the field tags is too long. Array must be of length <= ${MAX_TAGS_PER_CASE}.` + ); + }); + + it('should throw an error if the tags array has empty string', async () => { + await expect(create({ ...theCase, tags: [''] }, clientArgs)).rejects.toThrow( + 'Failed to create case: Error: The tag field cannot be an empty string.' + ); + }); + + it('should throw an error if the tags array has string with empty characters', async () => { + await expect(create({ ...theCase, tags: [' '] }, clientArgs)).rejects.toThrow( + 'Failed to create case: Error: The tag field cannot be an empty string.' + ); + }); + + it('should throw an error if the tag length is too long', async () => { + const tag = Array(MAX_LENGTH_PER_TAG + 1) + .fill('f') + .toString(); + + await expect(create({ ...theCase, tags: [tag] }, clientArgs)).rejects.toThrow( + `Failed to create case: Error: The length of the tag is too long. The maximum length is ${MAX_LENGTH_PER_TAG}.` + ); + }); + }); + + describe('Category', () => { + const clientArgs = createCasesClientMockArgs(); + clientArgs.services.caseService.postNewCase.mockResolvedValue(caseSO); + + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('should not throw an error if the category is null', async () => { + await expect(create({ ...theCase, category: null }, clientArgs)).resolves.not.toThrow(); + }); it('should throw an error if the category length is too long', async () => { await expect( @@ -106,13 +251,13 @@ describe('create', () => { it('should throw an error if the category is an empty string', async () => { await expect(create({ ...theCase, category: '' }, clientArgs)).rejects.toThrow( - 'Failed to create case: Error: The category cannot be an empty string.' + 'Failed to create case: Error: The category field cannot be an empty string.,Invalid value "" supplied to "category"' ); }); it('should throw an error if the category is a string with empty characters', async () => { await expect(create({ ...theCase, category: ' ' }, clientArgs)).rejects.toThrow( - 'Failed to create case: Error: The category cannot be an empty string.' + 'Failed to create case: Error: The category field cannot be an empty string.,Invalid value " " supplied to "category"' ); }); }); diff --git a/x-pack/plugins/cases/server/client/cases/create.ts b/x-pack/plugins/cases/server/client/cases/create.ts index d60b1510b56d8..1b787b415c4d0 100644 --- a/x-pack/plugins/cases/server/client/cases/create.ts +++ b/x-pack/plugins/cases/server/client/cases/create.ts @@ -17,17 +17,8 @@ import { CaseSeverity, decodeWithExcessOrThrow, } from '../../../common/api'; -import { - MAX_ASSIGNEES_PER_CASE, - MAX_CATEGORY_LENGTH, - MAX_TITLE_LENGTH, -} from '../../../common/constants'; -import { - isInvalidTag, - areTotalAssigneesInvalid, - isCategoryFieldTooLong, - isCategoryFieldInvalidString, -} from '../../../common/utils/validators'; +import { MAX_ASSIGNEES_PER_CASE } from '../../../common/constants'; +import { areTotalAssigneesInvalid } from '../../../common/utils/validators'; import { Operations } from '../../authorization'; import { createCaseError } from '../../common/error'; @@ -36,18 +27,6 @@ import type { CasesClientArgs } from '..'; import { LICENSING_CASE_ASSIGNMENT_FEATURE } from '../../common/constants'; import { decodeOrThrow } from '../../../common/api/runtime_types'; -function validateCategory(category?: string | null) { - if (isCategoryFieldTooLong(category)) { - throw Boom.badRequest( - `The length of the category is too long. The maximum length is ${MAX_CATEGORY_LENGTH}` - ); - } - - if (isCategoryFieldInvalidString(category)) { - throw Boom.badRequest('The category cannot be an empty string.'); - } -} - /** * Creates a new case. * @@ -64,18 +43,6 @@ export const create = async (data: CasePostRequest, clientArgs: CasesClientArgs) try { const query = decodeWithExcessOrThrow(CasePostRequestRt)(data); - if (query.title.length > MAX_TITLE_LENGTH) { - throw Boom.badRequest( - `The length of the title is too long. The maximum length is ${MAX_TITLE_LENGTH}.` - ); - } - - if (query.tags.some(isInvalidTag)) { - throw Boom.badRequest('A tag must contain at least one non-space character'); - } - - validateCategory(query.category); - const savedObjectID = SavedObjectsUtils.generateId(); await auth.ensureAuthorized({ diff --git a/x-pack/plugins/cases/server/client/cases/update.test.ts b/x-pack/plugins/cases/server/client/cases/update.test.ts index 4c90dc6b43304..e789c6d53ca07 100644 --- a/x-pack/plugins/cases/server/client/cases/update.test.ts +++ b/x-pack/plugins/cases/server/client/cases/update.test.ts @@ -5,6 +5,13 @@ * 2.0. */ +import { + MAX_CATEGORY_LENGTH, + MAX_DESCRIPTION_LENGTH, + MAX_TAGS_PER_CASE, + MAX_LENGTH_PER_TAG, + MAX_TITLE_LENGTH, +} from '../../../common/constants'; import { mockCases } from '../../mocks'; import { createCasesClientMockArgs } from '../mocks'; import { update } from './update'; @@ -286,6 +293,27 @@ describe('update', () => { }); }); + it(`does not throw error when category is non empty string less than ${MAX_CATEGORY_LENGTH} characters`, async () => { + clientArgs.services.caseService.patchCases.mockResolvedValue({ + saved_objects: [{ ...mockCases[0] }], + }); + + await expect( + update( + { + cases: [ + { + id: mockCases[0].id, + version: mockCases[0].version ?? '', + category: 'foobar', + }, + ], + }, + clientArgs + ) + ).resolves.not.toThrow(); + }); + it('does not update the category if the length is too long', async () => { await expect( update( @@ -301,11 +329,11 @@ describe('update', () => { clientArgs ) ).rejects.toThrow( - 'Failed to update case, ids: [{"id":"mock-id-1","version":"WzAsMV0="}]: Error: The length of the category is too long. The maximum length is 50, ids: [mock-id-1]' + `Failed to update case, ids: [{"id":"mock-id-1","version":"WzAsMV0="}]: Error: The length of the category is too long. The maximum length is ${MAX_CATEGORY_LENGTH}.,Invalid value \"A very long category with more than fifty characters!\" supplied to \"cases,category\"` ); }); - it('does not update the category if it is just an empty string', async () => { + it('throws error if category is just an empty string', async () => { await expect( update( { @@ -320,11 +348,11 @@ describe('update', () => { clientArgs ) ).rejects.toThrow( - 'Failed to update case, ids: [{"id":"mock-id-1","version":"WzAsMV0="}]: Error: The category cannot be an empty string. Ids: [mock-id-1]' + 'Failed to update case, ids: [{"id":"mock-id-1","version":"WzAsMV0="}]: Error: The category field cannot be an empty string.,Invalid value "" supplied to "cases,category"' ); }); - it('does not update the category if it is a string with empty characters', async () => { + it('throws error if category is a string with empty characters', async () => { await expect( update( { @@ -339,7 +367,337 @@ describe('update', () => { clientArgs ) ).rejects.toThrow( - 'Failed to update case, ids: [{"id":"mock-id-1","version":"WzAsMV0="}]: Error: The category cannot be an empty string. Ids: [mock-id-1]' + 'Failed to update case, ids: [{"id":"mock-id-1","version":"WzAsMV0="}]: Error: The category field cannot be an empty string.,Invalid value " " supplied to "cases,category"' + ); + }); + }); + + describe('Title', () => { + const clientArgs = createCasesClientMockArgs(); + + beforeEach(() => { + jest.clearAllMocks(); + clientArgs.services.caseService.getCases.mockResolvedValue({ saved_objects: mockCases }); + clientArgs.services.caseService.getAllCaseComments.mockResolvedValue({ + saved_objects: [], + total: 0, + per_page: 10, + page: 1, + }); + }); + + it(`does not throw error when title is non empty string less than ${MAX_TITLE_LENGTH} characters`, async () => { + clientArgs.services.caseService.patchCases.mockResolvedValue({ + saved_objects: [{ ...mockCases[0] }], + }); + + await expect( + update( + { + cases: [ + { + id: mockCases[0].id, + version: mockCases[0].version ?? '', + title: 'This is a test case!!', + }, + ], + }, + clientArgs + ) + ).resolves.not.toThrow(); + }); + + it('throws error if the title is too long', async () => { + await expect( + update( + { + cases: [ + { + id: mockCases[0].id, + version: mockCases[0].version ?? '', + title: + 'This is a very long title with more than one hundred and sixty characters!! To confirm the maximum limit error thrown for more than one hundred and sixty characters!!', + }, + ], + }, + clientArgs + ) + ).rejects.toThrow( + `Failed to update case, ids: [{"id":"mock-id-1","version":"WzAsMV0="}]: Error: The length of the title is too long. The maximum length is ${MAX_TITLE_LENGTH}.` + ); + }); + + it('throws error if title is just an empty string', async () => { + await expect( + update( + { + cases: [ + { + id: mockCases[0].id, + version: mockCases[0].version ?? '', + title: '', + }, + ], + }, + clientArgs + ) + ).rejects.toThrow( + 'Failed to update case, ids: [{"id":"mock-id-1","version":"WzAsMV0="}]: Error: The title field cannot be an empty string.' + ); + }); + + it('throws error if title is a string with empty characters', async () => { + await expect( + update( + { + cases: [ + { + id: mockCases[0].id, + version: mockCases[0].version ?? '', + title: ' ', + }, + ], + }, + clientArgs + ) + ).rejects.toThrow( + 'Failed to update case, ids: [{"id":"mock-id-1","version":"WzAsMV0="}]: Error: The title field cannot be an empty string.' + ); + }); + }); + + describe('Description', () => { + const clientArgs = createCasesClientMockArgs(); + + beforeEach(() => { + jest.clearAllMocks(); + clientArgs.services.caseService.getCases.mockResolvedValue({ saved_objects: mockCases }); + clientArgs.services.caseService.getAllCaseComments.mockResolvedValue({ + saved_objects: [], + total: 0, + per_page: 10, + page: 1, + }); + }); + + it(`does not throw error when description is non empty string less than ${MAX_DESCRIPTION_LENGTH} characters`, async () => { + clientArgs.services.caseService.patchCases.mockResolvedValue({ + saved_objects: [{ ...mockCases[0] }], + }); + + await expect( + update( + { + cases: [ + { + id: mockCases[0].id, + version: mockCases[0].version ?? '', + description: 'New updated description!!', + }, + ], + }, + clientArgs + ) + ).resolves.not.toThrow(); + }); + + it('throws error when the description is too long', async () => { + const description = Array(MAX_DESCRIPTION_LENGTH + 1) + .fill('a') + .toString(); + + await expect( + update( + { + cases: [ + { + id: mockCases[0].id, + version: mockCases[0].version ?? '', + description, + }, + ], + }, + clientArgs + ) + ).rejects.toThrow( + `Failed to update case, ids: [{"id":"mock-id-1","version":"WzAsMV0="}]: Error: The length of the description is too long. The maximum length is ${MAX_DESCRIPTION_LENGTH}.` + ); + }); + + it('throws error if description is just an empty string', async () => { + await expect( + update( + { + cases: [ + { + id: mockCases[0].id, + version: mockCases[0].version ?? '', + description: '', + }, + ], + }, + clientArgs + ) + ).rejects.toThrow( + 'Failed to update case, ids: [{"id":"mock-id-1","version":"WzAsMV0="}]: Error: The description field cannot be an empty string.' + ); + }); + + it('throws error if description is a string with empty characters', async () => { + await expect( + update( + { + cases: [ + { + id: mockCases[0].id, + version: mockCases[0].version ?? '', + description: ' ', + }, + ], + }, + clientArgs + ) + ).rejects.toThrow( + 'Failed to update case, ids: [{"id":"mock-id-1","version":"WzAsMV0="}]: Error: The description field cannot be an empty string.' + ); + }); + }); + + describe('Tags', () => { + const clientArgs = createCasesClientMockArgs(); + + beforeEach(() => { + jest.clearAllMocks(); + clientArgs.services.caseService.getCases.mockResolvedValue({ saved_objects: mockCases }); + clientArgs.services.caseService.getAllCaseComments.mockResolvedValue({ + saved_objects: [], + total: 0, + per_page: 10, + page: 1, + }); + }); + + it('does not throw error when tags array is empty', async () => { + clientArgs.services.caseService.patchCases.mockResolvedValue({ + saved_objects: [{ ...mockCases[0] }], + }); + + await expect( + update( + { + cases: [ + { + id: mockCases[0].id, + version: mockCases[0].version ?? '', + tags: [], + }, + ], + }, + clientArgs + ) + ).resolves.not.toThrow(); + }); + + it(`does not throw error when tags array length is less than ${MAX_TAGS_PER_CASE} and tag has ${MAX_LENGTH_PER_TAG} characters`, async () => { + clientArgs.services.caseService.patchCases.mockResolvedValue({ + saved_objects: [{ ...mockCases[0] }], + }); + + await expect( + update( + { + cases: [ + { + id: mockCases[0].id, + version: mockCases[0].version ?? '', + tags: ['foo', 'bar'], + }, + ], + }, + clientArgs + ) + ).resolves.not.toThrow(); + }); + + it('throws error if the tags array length is too long', async () => { + const tags = Array(MAX_TAGS_PER_CASE + 1).fill('foo'); + + await expect( + update( + { + cases: [ + { + id: mockCases[0].id, + version: mockCases[0].version ?? '', + tags, + }, + ], + }, + clientArgs + ) + ).rejects.toThrow( + `Failed to update case, ids: [{"id":"mock-id-1","version":"WzAsMV0="}]: Error: The length of the field tags is too long. Array must be of length <= ${MAX_TAGS_PER_CASE}.` + ); + }); + + it('throws error if the tag length is too long', async () => { + const tag = Array(MAX_LENGTH_PER_TAG + 1) + .fill('f') + .toString(); + + await expect( + update( + { + cases: [ + { + id: mockCases[0].id, + version: mockCases[0].version ?? '', + tags: [tag], + }, + ], + }, + clientArgs + ) + ).rejects.toThrow( + `Failed to update case, ids: [{"id":"mock-id-1","version":"WzAsMV0="}]: Error: The length of the tag is too long. The maximum length is ${MAX_LENGTH_PER_TAG}.` + ); + }); + + it('throws error if tag is empty string', async () => { + await expect( + update( + { + cases: [ + { + id: mockCases[0].id, + version: mockCases[0].version ?? '', + tags: [''], + }, + ], + }, + clientArgs + ) + ).rejects.toThrow( + 'Failed to update case, ids: [{"id":"mock-id-1","version":"WzAsMV0="}]: Error: The tag field cannot be an empty string.' + ); + }); + + it('throws error if tag is a string with empty characters', async () => { + await expect( + update( + { + cases: [ + { + id: mockCases[0].id, + version: mockCases[0].version ?? '', + tags: [' '], + }, + ], + }, + clientArgs + ) + ).rejects.toThrow( + 'Failed to update case, ids: [{"id":"mock-id-1","version":"WzAsMV0="}]: Error: The tag field cannot be an empty string.' ); }); }); diff --git a/x-pack/plugins/cases/server/client/cases/update.ts b/x-pack/plugins/cases/server/client/cases/update.ts index e415b77e2eb4b..a685f3c2a2fc7 100644 --- a/x-pack/plugins/cases/server/client/cases/update.ts +++ b/x-pack/plugins/cases/server/client/cases/update.ts @@ -17,11 +17,7 @@ import type { import { nodeBuilder } from '@kbn/es-query'; -import { - areTotalAssigneesInvalid, - isCategoryFieldInvalidString, - isCategoryFieldTooLong, -} from '../../../common/utils/validators'; +import { areTotalAssigneesInvalid } from '../../../common/utils/validators'; import type { CaseAssignees, CaseAttributes, @@ -43,8 +39,6 @@ import { CASE_COMMENT_SAVED_OBJECT, CASE_SAVED_OBJECT, MAX_ASSIGNEES_PER_CASE, - MAX_CATEGORY_LENGTH, - MAX_TITLE_LENGTH, } from '../../../common/constants'; import { arraysDifference, getCaseToUpdate } from '../utils'; @@ -78,24 +72,6 @@ function throwIfUpdateOwner(requests: UpdateRequestWithOriginalCase[]) { } } -/** - * Throws an error if any of the requests updates a title and the length is over MAX_TITLE_LENGTH. - */ -function throwIfTitleIsInvalid(requests: UpdateRequestWithOriginalCase[]) { - const requestsInvalidTitle = requests.filter( - ({ updateReq }) => updateReq.title !== undefined && updateReq.title.length > MAX_TITLE_LENGTH - ); - - if (requestsInvalidTitle.length > 0) { - const ids = requestsInvalidTitle.map(({ updateReq }) => updateReq.id); - throw Boom.badRequest( - `The length of the title is too long. The maximum length is ${MAX_TITLE_LENGTH}, ids: [${ids.join( - ', ' - )}]` - ); - } -} - /** * Throws an error if any of the requests attempt to update the assignees of the case * without the appropriate license @@ -122,40 +98,6 @@ function throwIfUpdateAssigneesWithoutValidLicense( } } -/** - * Throws an error if any of the requests tries to update the category and - * the length is > MAX_CATEGORY_LENGTH. - */ -function throwIfCategoryLengthIsInvalid(requests: UpdateRequestWithOriginalCase[]) { - const requestsInvalidCategory = requests.filter(({ updateReq }) => - isCategoryFieldTooLong(updateReq.category) - ); - - if (requestsInvalidCategory.length > 0) { - const ids = requestsInvalidCategory.map(({ updateReq }) => updateReq.id); - throw Boom.badRequest( - `The length of the category is too long. The maximum length is ${MAX_CATEGORY_LENGTH}, ids: [${ids.join( - ', ' - )}]` - ); - } -} - -/** - * Throws an error if any of the requests tries to update the category and - * the new value is an empty string. - */ -function throwIfCategoryIsInvalidString(requests: UpdateRequestWithOriginalCase[]) { - const requestsInvalidCategory = requests.filter(({ updateReq }) => - isCategoryFieldInvalidString(updateReq.category) - ); - - if (requestsInvalidCategory.length > 0) { - const ids = requestsInvalidCategory.map(({ updateReq }) => updateReq.id); - throw Boom.badRequest(`The category cannot be an empty string. Ids: [${ids.join(', ')}]`); - } -} - function notifyPlatinumUsage( licensingService: LicensingService, requests: UpdateRequestWithOriginalCase[] @@ -424,9 +366,6 @@ export const update = async ( const hasPlatinumLicense = await licensingService.isAtLeastPlatinum(); throwIfUpdateOwner(casesToUpdate); - throwIfTitleIsInvalid(casesToUpdate); - throwIfCategoryLengthIsInvalid(casesToUpdate); - throwIfCategoryIsInvalidString(casesToUpdate); throwIfUpdateAssigneesWithoutValidLicense(casesToUpdate, hasPlatinumLicense); throwIfTotalAssigneesAreInvalid(casesToUpdate); diff --git a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/cases/patch_cases.ts b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/cases/patch_cases.ts index 72b75825ce826..f284817db1f9a 100644 --- a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/cases/patch_cases.ts +++ b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/cases/patch_cases.ts @@ -580,6 +580,93 @@ export default ({ getService }: FtrProviderContext): void => { }); }); + it('400s if the title an empty string', async () => { + const postedCase = await createCase(supertest, postCaseReq); + await updateCase({ + supertest, + params: { + cases: [ + { + id: postedCase.id, + version: postedCase.version, + title: '', + }, + ], + }, + expectedHttpCode: 400, + }); + }); + + it('400s if the title is a string with empty characters', async () => { + const postedCase = await createCase(supertest, postCaseReq); + await updateCase({ + supertest, + params: { + cases: [ + { + id: postedCase.id, + version: postedCase.version, + title: ' ', + }, + ], + }, + expectedHttpCode: 400, + }); + }); + + it('400s if the description is too long', async () => { + const longDescription = 'a'.repeat(30001); + + const postedCase = await createCase(supertest, postCaseReq); + await updateCase({ + supertest, + params: { + cases: [ + { + id: postedCase.id, + version: postedCase.version, + description: longDescription, + }, + ], + }, + expectedHttpCode: 400, + }); + }); + + it('400s if the description an empty string', async () => { + const postedCase = await createCase(supertest, postCaseReq); + await updateCase({ + supertest, + params: { + cases: [ + { + id: postedCase.id, + version: postedCase.version, + description: '', + }, + ], + }, + expectedHttpCode: 400, + }); + }); + + it('400s if the description is a string with empty characters', async () => { + const postedCase = await createCase(supertest, postCaseReq); + await updateCase({ + supertest, + params: { + cases: [ + { + id: postedCase.id, + version: postedCase.version, + description: ' ', + }, + ], + }, + expectedHttpCode: 400, + }); + }); + describe('categories', async () => { it('400s when a too long category value is passed', async () => { const postedCase = await createCase(supertest, postCaseReq); @@ -632,6 +719,80 @@ export default ({ getService }: FtrProviderContext): void => { }); }); }); + + describe('tags', async () => { + it('400s when tags array is too long', async () => { + const tags = Array(201).fill('foo'); + + const postedCase = await createCase(supertest, postCaseReq); + await updateCase({ + supertest, + params: { + cases: [ + { + id: postedCase.id, + version: postedCase.version, + tags, + }, + ], + }, + expectedHttpCode: 400, + }); + }); + + it('400s when tag string is too long', async () => { + const tag = 'a'.repeat(257); + + const postedCase = await createCase(supertest, postCaseReq); + await updateCase({ + supertest, + params: { + cases: [ + { + id: postedCase.id, + version: postedCase.version, + tags: [tag], + }, + ], + }, + expectedHttpCode: 400, + }); + }); + + it('400s when an empty string is passed in tags', async () => { + const postedCase = await createCase(supertest, postCaseReq); + await updateCase({ + supertest, + params: { + cases: [ + { + id: postedCase.id, + version: postedCase.version, + tags: ['', 'one'], + }, + ], + }, + expectedHttpCode: 400, + }); + }); + + it('400s when a string with spaces tag value is passed', async () => { + const postedCase = await createCase(supertest, postCaseReq); + await updateCase({ + supertest, + params: { + cases: [ + { + id: postedCase.id, + version: postedCase.version, + tags: [' '], + }, + ], + }, + expectedHttpCode: 400, + }); + }); + }); }); describe('alerts', () => { diff --git a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/cases/post_case.ts b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/cases/post_case.ts index c6aea823f3bbc..f36ebc6e8961f 100644 --- a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/cases/post_case.ts +++ b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/cases/post_case.ts @@ -265,6 +265,12 @@ export default ({ getService }: FtrProviderContext): void => { await createCase(supertest, getPostCaseRequest({ title: longTitle }), 400); }); + it('400s if the description is too long', async () => { + const longDescription = 'a'.repeat(30001); + + await createCase(supertest, getPostCaseRequest({ description: longDescription }), 400); + }); + describe('tags', async () => { it('400s if the a tag is a whitespace', async () => { const tags = ['test', ' ']; @@ -277,6 +283,18 @@ export default ({ getService }: FtrProviderContext): void => { await createCase(supertest, getPostCaseRequest({ tags }), 400); }); + + it('400s if the a tag is too long', async () => { + const tag = 'a'.repeat(257); + + await createCase(supertest, getPostCaseRequest({ tags: [tag] }), 400); + }); + + it('400s if the a tags array is too long', async () => { + const tags = Array(201).fill('foo'); + + await createCase(supertest, getPostCaseRequest({ tags }), 400); + }); }); describe('categories', async () => { From c6dd2d7135acb251cc3bee7df1320b8ca1325716 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Efe=20G=C3=BCrkan=20YALAMAN?= Date: Fri, 30 Jun 2023 18:26:37 +0200 Subject: [PATCH 6/7] [Enterprise Search] Add minute cron editor (#161001) ## Summary https://github.com/elastic/kibana/assets/1410658/b66d0278-e80b-4f2f-a1b3-d1d0f96c226d Add minute cron editor. Enable Minute for Access Control. Fix Week not being remembered on page load. Add helpers to convert cron expression in between `Every X Minutes` and back. ### Checklist Delete any items that are not applicable to this PR. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) --- .../connector_cron_editor.tsx | 8 +- .../connector_scheduling/full_content.tsx | 1 + .../__snapshots__/cron_editor.test.tsx.snap | 989 ++++++++++++++++++ .../shared/cron_editor/constants.ts | 9 +- .../shared/cron_editor/cron_editor.test.tsx | 14 + .../shared/cron_editor/cron_editor.tsx | 30 +- .../shared/cron_editor/cron_minutely.tsx | 60 ++ .../shared/cron_editor/services/cron.ts | 15 + 8 files changed, 1114 insertions(+), 12 deletions(-) create mode 100644 x-pack/plugins/enterprise_search/public/applications/shared/cron_editor/cron_minutely.tsx diff --git a/x-pack/plugins/enterprise_search/public/applications/enterprise_search_content/components/search_index/connector/connector_scheduling/connector_cron_editor.tsx b/x-pack/plugins/enterprise_search/public/applications/enterprise_search_content/components/search_index/connector/connector_scheduling/connector_cron_editor.tsx index 6ddc393c45fad..46eccba6a4f70 100644 --- a/x-pack/plugins/enterprise_search/public/applications/enterprise_search_content/components/search_index/connector/connector_scheduling/connector_cron_editor.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/enterprise_search_content/components/search_index/connector/connector_scheduling/connector_cron_editor.tsx @@ -22,6 +22,7 @@ import { ConnectorSchedulingLogic } from '../connector_scheduling_logic'; interface ConnectorCronEditorProps { disabled?: boolean; + frequencyBlockList?: string[]; onReset?(): void; onSave?(interval: ConnectorScheduling['interval']): void; scheduling: ConnectorScheduling; @@ -30,6 +31,7 @@ interface ConnectorCronEditorProps { export const ConnectorCronEditor: React.FC = ({ disabled = false, + frequencyBlockList = ['MINUTE'], scheduling, onSave, onReset, @@ -77,7 +79,7 @@ export const ConnectorCronEditor: React.FC = ({ setNewInterval(expression); setHasChanges(type); }} - frequencyBlockList={['MINUTE']} + frequencyBlockList={frequencyBlockList} /> @@ -133,7 +135,7 @@ function cronToFrequency(cron: string): Frequency { if (fields.length < 4) { return 'YEAR'; } - if (fields[1] === '*') { + if (fields[1] === '*' || fields[1].startsWith('*/')) { return 'MINUTE'; } if (fields[2] === '*') { @@ -142,7 +144,7 @@ function cronToFrequency(cron: string): Frequency { if (fields[3] === '*') { return 'DAY'; } - if (fields[4] === '?') { + if (fields[3] === '?') { return 'WEEK'; } if (fields[4] === '*') { diff --git a/x-pack/plugins/enterprise_search/public/applications/enterprise_search_content/components/search_index/connector/connector_scheduling/full_content.tsx b/x-pack/plugins/enterprise_search/public/applications/enterprise_search_content/components/search_index/connector/connector_scheduling/full_content.tsx index da8aaa3bb1a75..3fbe4ef8859f2 100644 --- a/x-pack/plugins/enterprise_search/public/applications/enterprise_search_content/components/search_index/connector/connector_scheduling/full_content.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/enterprise_search_content/components/search_index/connector/connector_scheduling/full_content.tsx @@ -215,6 +215,7 @@ export const ConnectorContentScheduling: React.FC { diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/cron_editor/__snapshots__/cron_editor.test.tsx.snap b/x-pack/plugins/enterprise_search/public/applications/shared/cron_editor/__snapshots__/cron_editor.test.tsx.snap index e00fb47fc377d..2e22e3bfe30b0 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/cron_editor/__snapshots__/cron_editor.test.tsx.snap +++ b/x-pack/plugins/enterprise_search/public/applications/shared/cron_editor/__snapshots__/cron_editor.test.tsx.snap @@ -3387,6 +3387,995 @@ exports[`CronEditor is rendered with a MINUTE frequency 1`] = ` + + + } + labelType="label" + > +
+
+ + + +
+
+ + +
+ + + +
+ + + + +
+ + + + + +
+ +
+ + + +
+ + +
+
+ + `; diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/cron_editor/constants.ts b/x-pack/plugins/enterprise_search/public/applications/shared/cron_editor/constants.ts index ad0c2bb45cc39..e2ef66324309f 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/cron_editor/constants.ts +++ b/x-pack/plugins/enterprise_search/public/applications/shared/cron_editor/constants.ts @@ -24,6 +24,11 @@ function makeSequence(min: number, max: number): number[] { return values; } +export const EVERY_MINUTE_OPTIONS = makeSequence(1, 59).map((value) => ({ + value: '*/' + value.toString(), + text: value.toString(), +})); + export const MINUTE_OPTIONS = makeSequence(0, 59).map((value) => ({ value: value.toString(), text: padStart(value.toString(), 2, '0'), @@ -77,7 +82,7 @@ export const UNITS: EuiSelectOption[] = [ ]; export const frequencyToFieldsMap: Record = { - MINUTE: {}, + MINUTE: { minute: true }, HOUR: { minute: true, }, @@ -106,7 +111,7 @@ export const frequencyToFieldsMap: Record = { export const frequencyToBaselineFieldsMap: Record = { MINUTE: { second: '0', - minute: '*', + minute: '*/1', hour: '*', date: '*', month: '*', diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/cron_editor/cron_editor.test.tsx b/x-pack/plugins/enterprise_search/public/applications/shared/cron_editor/cron_editor.test.tsx index 44f69c9fa6e33..668105a93253e 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/cron_editor/cron_editor.test.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/shared/cron_editor/cron_editor.test.tsx @@ -72,6 +72,20 @@ describe('CronEditor', () => { const minuteSelect = findTestSubject(component, 'cronFrequencyYearlyMinuteSelect'); expect(minuteSelect.props().value).toBe('20'); }); + + it('sets the values of the fields for minute', () => { + const component = mountWithI18nProvider( + {}} + /> + ); + + const monthSelect = findTestSubject(component, 'cronFrequencyMinutelyMinuteSelect'); + expect(monthSelect.prop('value')).toBe('*/18'); + }); }); describe('onChange', () => { diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/cron_editor/cron_editor.tsx b/x-pack/plugins/enterprise_search/public/applications/shared/cron_editor/cron_editor.tsx index 06e37aa2366f8..bcda5c37f33c7 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/cron_editor/cron_editor.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/shared/cron_editor/cron_editor.tsx @@ -20,13 +20,16 @@ import { UNITS, frequencyToFieldsMap, frequencyToBaselineFieldsMap, + EVERY_MINUTE_OPTIONS, } from './constants'; import { CronDaily } from './cron_daily'; import { CronHourly } from './cron_hourly'; +import { CronMinutely } from './cron_minutely'; import { CronMonthly } from './cron_monthly'; import { CronWeekly } from './cron_weekly'; import { CronYearly } from './cron_yearly'; import { cronExpressionToParts, cronPartsToExpression } from './services'; +import { convertFromEveryXMinute, convertToEveryXMinute } from './services/cron'; import { Frequency, Field, FieldToValueMap } from './types'; const excludeBlockListedFrequencies = ( @@ -41,10 +44,12 @@ const excludeBlockListedFrequencies = ( }; interface Props { - frequencyBlockList?: string[]; + autoFocus?: boolean; + cronExpression: string; + disabled?: boolean; fieldToPreferredValueMap: FieldToValueMap; frequency: Frequency; - cronExpression: string; + frequencyBlockList?: string[]; onChange: ({ cronExpression, fieldToPreferredValueMap, @@ -54,8 +59,6 @@ interface Props { fieldToPreferredValueMap: FieldToValueMap; frequency: Frequency; }) => void; - autoFocus?: boolean; - disabled?: boolean; } type State = FieldToValueMap; @@ -77,14 +80,20 @@ export class CronEditor extends Component { } onChangeFrequency = (frequency: Frequency) => { - const { onChange, fieldToPreferredValueMap } = this.props; + const { onChange, fieldToPreferredValueMap, frequency: oldFrequency } = this.props; // Update fields which aren't editable with acceptable baseline values. const editableFields = Object.keys(frequencyToFieldsMap[frequency]) as Field[]; const inheritedFields = editableFields.reduce( (fieldBaselines, field) => { if (fieldToPreferredValueMap[field] != null) { - fieldBaselines[field] = fieldToPreferredValueMap[field]; + if (oldFrequency === 'MINUTE') { + fieldBaselines[field] = convertFromEveryXMinute(fieldToPreferredValueMap[field]); + } else if (frequency === 'MINUTE') { + fieldBaselines[field] = convertToEveryXMinute(fieldToPreferredValueMap[field]); + } else { + fieldBaselines[field] = fieldToPreferredValueMap[field]; + } } return fieldBaselines; }, @@ -143,7 +152,14 @@ export class CronEditor extends Component { switch (frequency) { case 'MINUTE': - return; + return ( + + ); case 'HOUR': return ( diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/cron_editor/cron_minutely.tsx b/x-pack/plugins/enterprise_search/public/applications/shared/cron_editor/cron_minutely.tsx new file mode 100644 index 0000000000000..1aedb9737e5ce --- /dev/null +++ b/x-pack/plugins/enterprise_search/public/applications/shared/cron_editor/cron_minutely.tsx @@ -0,0 +1,60 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import React, { Fragment } from 'react'; + +import { EuiFormRow, EuiSelect, EuiSelectOption } from '@elastic/eui'; +import { i18n } from '@kbn/i18n'; +import { FormattedMessage } from '@kbn/i18n-react'; + +interface Props { + disabled?: boolean; + minute?: string; + minuteOptions: EuiSelectOption[]; + onChange: ({ minute }: { minute?: string }) => void; +} + +export const CronMinutely: React.FunctionComponent = ({ + disabled, + minute, + minuteOptions, + onChange, +}) => ( + + + } + fullWidth + data-test-subj="cronFrequencyConfiguration" + > + onChange({ minute: e.target.value })} + fullWidth + prepend={i18n.translate( + 'xpack.enterpriseSearch.cronEditor.cronMinutely.fieldMinute.textAtLabel', + { + defaultMessage: 'Every', + } + )} + append={i18n.translate( + 'xpack.enterpriseSearch.cronEditor.cronMinutely.fieldMinute.textAppendLabel', + { + defaultMessage: 'minutes', + } + )} + data-test-subj="cronFrequencyMinutelyMinuteSelect" + /> + + +); diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/cron_editor/services/cron.ts b/x-pack/plugins/enterprise_search/public/applications/shared/cron_editor/services/cron.ts index 542502fbcbe76..fc2019d63c17d 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/cron_editor/services/cron.ts +++ b/x-pack/plugins/enterprise_search/public/applications/shared/cron_editor/services/cron.ts @@ -56,3 +56,18 @@ export function cronPartsToExpression({ }: FieldToValueMap): string { return `${second} ${minute} ${hour} ${date} ${month} ${day}`; } + +export function convertToEveryXMinute( + minute: FieldToValueMap['minute'] +): FieldToValueMap['minute'] { + if (!minute) return minute; + if (minute.startsWith('*/')) return minute; + return '*/' + minute; +} + +export function convertFromEveryXMinute( + minute: FieldToValueMap['minute'] +): FieldToValueMap['minute'] { + if (!minute) return minute; + return minute.startsWith('*/') ? minute.slice(2) : minute; +} From 4aab4798b4ca78880fdf1ecd472e33880496f0a8 Mon Sep 17 00:00:00 2001 From: Nathan Reese Date: Fri, 30 Jun 2023 11:26:04 -0600 Subject: [PATCH 7/7] [Dashboard] fix Time Slider Overrides Custom Time Ranges (#160938) Close https://github.com/elastic/kibana/issues/159813 --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> --- .../embeddable/dashboard_container.test.tsx | 58 +++++++++++++++++++ .../embeddable/dashboard_container.tsx | 10 +++- 2 files changed, 66 insertions(+), 2 deletions(-) diff --git a/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.test.tsx b/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.test.tsx index 5a360446f03a8..b22c791926574 100644 --- a/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.test.tsx +++ b/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.test.tsx @@ -9,6 +9,7 @@ import React from 'react'; import { mount } from 'enzyme'; +import type { TimeRange } from '@kbn/es-query'; import { mockedReduxEmbeddablePackage } from '@kbn/presentation-util-plugin/public/mocks'; import { findTestSubject, nextTick } from '@kbn/test-jest-helpers'; import { I18nProvider } from '@kbn/i18n-react'; @@ -255,3 +256,60 @@ test('DashboardContainer in edit mode shows edit mode actions', async () => { // const action = findTestSubject(component, `embeddablePanelAction-${editModeAction.id}`); // expect(action.length).toBe(1); }); + +describe('getInheritedInput', () => { + const dashboardTimeRange = { + to: 'now', + from: 'now-15m', + }; + const dashboardTimeslice = [1688061910000, 1688062209000] as [number, number]; + + test('Should pass dashboard timeRange and timeslice to panel when panel does not have custom time range', async () => { + const container = buildMockDashboard({ + timeRange: dashboardTimeRange, + timeslice: dashboardTimeslice, + }); + const embeddable = await container.addNewEmbeddable( + CONTACT_CARD_EMBEDDABLE, + { + firstName: 'Kibana', + } + ); + expect(embeddable).toBeDefined(); + + const embeddableInput = container + .getChild(embeddable.id) + .getInput() as ContactCardEmbeddableInput & { + timeRange: TimeRange; + timeslice: [number, number]; + }; + expect(embeddableInput.timeRange).toEqual(dashboardTimeRange); + expect(embeddableInput.timeslice).toEqual(dashboardTimeslice); + }); + + test('Should not pass dashboard timeRange and timeslice to panel when panel has custom time range', async () => { + const container = buildMockDashboard({ + timeRange: dashboardTimeRange, + timeslice: dashboardTimeslice, + }); + const embeddableTimeRange = { + to: 'now', + from: 'now-24h', + }; + const embeddable = await container.addNewEmbeddable< + ContactCardEmbeddableInput & { timeRange: TimeRange } + >(CONTACT_CARD_EMBEDDABLE, { + firstName: 'Kibana', + timeRange: embeddableTimeRange, + }); + + const embeddableInput = container + .getChild(embeddable.id) + .getInput() as ContactCardEmbeddableInput & { + timeRange: TimeRange; + timeslice: [number, number]; + }; + expect(embeddableInput.timeRange).toEqual(embeddableTimeRange); + expect(embeddableInput.timeslice).toBeUndefined(); + }); +}); diff --git a/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.tsx b/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.tsx index df49e77171053..40b67faf67ed6 100644 --- a/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.tsx +++ b/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.tsx @@ -259,12 +259,16 @@ export class DashboardContainer extends Container)?.timeRange + ); return { searchSessionId: this.searchSessionId, refreshConfig: refreshInterval, @@ -273,11 +277,13 @@ export class DashboardContainer extends Container