From 12ab5b3694e8ecbade756da84ab2b9d38bcf2d94 Mon Sep 17 00:00:00 2001 From: Wafaa Nasr Date: Mon, 30 Oct 2023 18:04:39 +0100 Subject: [PATCH 01/10] [Security Solution][Exceptions][API testing] Move and restructures action groups in the new api integration test folder (#169234) ## Summary - Following the initial work in this https://github.com/elastic/kibana/pull/166755 - Addresses part of https://github.com/elastic/kibana/issues/151902 for actions https://docs.google.com/document/d/1CRFfDWMzw3ob03euWIvT4-IoiLXjoiPWI8mTBqP4Zks/edit - Enable migrations of legacy actions to run only in ESS - Add the `@skipInQA` tag to the failing tests in QA env --------- Co-authored-by: Vitalii Dmyterko <92328789+vitaliidm@users.noreply.github.com> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> --- .buildkite/ftr_configs.yml | 4 +- .../security_and_spaces/group1/index.ts | 2 - .../security_and_spaces/group10/index.ts | 2 - .../group10/legacy_actions_migrations.ts | 354 -------------- .../security_and_spaces/group10/migrations.ts | 102 ---- .../config/serverless/config.base.ts | 5 - .../package.json | 7 +- .../scripts/index.js | 6 +- .../default_license/actions}/add_actions.ts | 34 +- .../actions/configs/ess.config.ts | 22 + .../actions/configs/serverless.config.ts | 15 + .../default_license/actions/index.ts | 15 + .../default_license/actions/migrations.ts | 451 ++++++++++++++++++ .../actions}/update_actions.ts | 49 +- .../utils/actions/create_new_action.ts | 35 ++ .../utils/actions/index.ts | 1 + .../rules/get_rule_with_web_hook_action.ts | 34 ++ ...simple_rule_output_with_web_hook_action.ts | 29 ++ .../detections_response/utils/rules/index.ts | 4 + .../utils/rules/rule_to_update_schema.ts | 37 ++ .../utils/rules/update_rule.ts | 41 ++ 21 files changed, 748 insertions(+), 501 deletions(-) delete mode 100644 x-pack/test/detection_engine_api_integration/security_and_spaces/group10/legacy_actions_migrations.ts delete mode 100644 x-pack/test/detection_engine_api_integration/security_and_spaces/group10/migrations.ts rename x-pack/test/{detection_engine_api_integration/security_and_spaces/group1 => security_solution_api_integration/test_suites/detections_response/default_license/actions}/add_actions.ts (71%) create mode 100644 x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions/configs/ess.config.ts create mode 100644 x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions/configs/serverless.config.ts create mode 100644 x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions/index.ts create mode 100644 x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions/migrations.ts rename x-pack/test/{detection_engine_api_integration/security_and_spaces/group1 => security_solution_api_integration/test_suites/detections_response/default_license/actions}/update_actions.ts (76%) create mode 100644 x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/actions/create_new_action.ts create mode 100644 x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/rules/get_rule_with_web_hook_action.ts create mode 100644 x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/rules/get_simple_rule_output_with_web_hook_action.ts create mode 100644 x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/rules/rule_to_update_schema.ts create mode 100644 x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/rules/update_rule.ts diff --git a/.buildkite/ftr_configs.yml b/.buildkite/ftr_configs.yml index f026499502e0d..7abd52a1ea153 100644 --- a/.buildkite/ftr_configs.yml +++ b/.buildkite/ftr_configs.yml @@ -457,7 +457,9 @@ enabled: - x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/exceptions/operators_data_types/ips_text_array/configs/ess.config.ts - x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_creation/configs/serverless.config.ts - x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_creation/configs/ess.config.ts - + - x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions/configs/serverless.config.ts + - x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions/configs/ess.config.ts + diff --git a/x-pack/test/detection_engine_api_integration/security_and_spaces/group1/index.ts b/x-pack/test/detection_engine_api_integration/security_and_spaces/group1/index.ts index 2ce7684dedd48..1c9c874127660 100644 --- a/x-pack/test/detection_engine_api_integration/security_and_spaces/group1/index.ts +++ b/x-pack/test/detection_engine_api_integration/security_and_spaces/group1/index.ts @@ -15,8 +15,6 @@ export default ({ loadTestFile }: FtrProviderContext): void => { // existence being near 0. loadTestFile(require.resolve('./aliases')); - loadTestFile(require.resolve('./add_actions')); - loadTestFile(require.resolve('./update_actions')); loadTestFile(require.resolve('./check_privileges')); loadTestFile(require.resolve('./create_index')); loadTestFile(require.resolve('./preview_rules')); diff --git a/x-pack/test/detection_engine_api_integration/security_and_spaces/group10/index.ts b/x-pack/test/detection_engine_api_integration/security_and_spaces/group10/index.ts index 8844107744854..7822d11698c95 100644 --- a/x-pack/test/detection_engine_api_integration/security_and_spaces/group10/index.ts +++ b/x-pack/test/detection_engine_api_integration/security_and_spaces/group10/index.ts @@ -17,7 +17,6 @@ export default ({ loadTestFile }: FtrProviderContext): void => { loadTestFile(require.resolve('./get_rule_execution_results')); loadTestFile(require.resolve('./import_rules')); loadTestFile(require.resolve('./import_export_rules')); - loadTestFile(require.resolve('./legacy_actions_migrations')); loadTestFile(require.resolve('./read_rules')); loadTestFile(require.resolve('./resolve_read_rules')); loadTestFile(require.resolve('./update_rules')); @@ -36,7 +35,6 @@ export default ({ loadTestFile }: FtrProviderContext): void => { loadTestFile(require.resolve('./runtime')); loadTestFile(require.resolve('./throttle')); loadTestFile(require.resolve('./ignore_fields')); - loadTestFile(require.resolve('./migrations')); loadTestFile(require.resolve('./risk_engine/init_and_status_apis')); loadTestFile(require.resolve('./risk_engine/risk_score_preview')); loadTestFile(require.resolve('./risk_engine/risk_score_calculation')); diff --git a/x-pack/test/detection_engine_api_integration/security_and_spaces/group10/legacy_actions_migrations.ts b/x-pack/test/detection_engine_api_integration/security_and_spaces/group10/legacy_actions_migrations.ts deleted file mode 100644 index 236bf6991c9d6..0000000000000 --- a/x-pack/test/detection_engine_api_integration/security_and_spaces/group10/legacy_actions_migrations.ts +++ /dev/null @@ -1,354 +0,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 expect from '@kbn/expect'; - -import { DETECTION_ENGINE_RULES_URL } from '@kbn/security-solution-plugin/common/constants'; -import { FtrProviderContext } from '../../common/ftr_provider_context'; -import { - getLegacyActionSOById, - getLegacyActionNotificationSOById, - getRuleSOById, -} from '../../utils'; - -/** - * @deprecated Once the legacy notification system is removed, remove this test too. - */ -// eslint-disable-next-line import/no-default-export -export default ({ getService }: FtrProviderContext) => { - const supertest = getService('supertest'); - const es = getService('es'); - const esArchiver = getService('esArchiver'); - - // This test suite is not meant to test a specific route, but to test the legacy action migration - // code that lives in multiple routes. This code is also tested in each of the routes it lives in - // but not in as much detail and relying on mocks. This test loads an es_archive containing rules - // created in 7.15 with legacy actions. - // For new routes that do any updates on a rule, please ensure that you are including the legacy - // action migration code. We are monitoring legacy action telemetry to clean up once we see their - // existence being near 0. - describe('migrate_legacy_actions', () => { - before(async () => { - await esArchiver.load('x-pack/test/functional/es_archives/security_solution/legacy_actions'); - }); - - after(async () => { - await esArchiver.unload( - 'x-pack/test/functional/es_archives/security_solution/legacy_actions' - ); - }); - - it('migrates legacy actions for rule with no actions', async () => { - const soId = '9095ee90-b075-11ec-bb3f-1f063f8e06cf'; - const ruleId = '2297be91-894c-4831-830f-b424a0ec84f0'; - const legacySidecarId = '926668d0-b075-11ec-bb3f-1f063f8e06cf'; - - // check for legacy sidecar action - const sidecarActionSO = await getLegacyActionSOById(es, legacySidecarId); - expect(sidecarActionSO.hits.hits.length).to.eql(1); - - // check for legacy notification SO - // should not have been created for a rule with no actions - const legacyNotificationSO = await getLegacyActionNotificationSOById(es, soId); - expect(legacyNotificationSO.hits.hits.length).to.eql(0); - - // patch enable the rule - // any route that edits the rule should trigger the migration - await supertest - .patch(DETECTION_ENGINE_RULES_URL) - .set('kbn-xsrf', 'true') - .set('elastic-api-version', '2023-10-31') - .send({ rule_id: ruleId, enabled: false }) - .expect(200); - - const { - hits: { - hits: [{ _source: ruleSO }], - }, - } = await getRuleSOById(es, soId); - - // Sidecar should be removed - const sidecarActionsSOAfterMigration = await getLegacyActionSOById(es, legacySidecarId); - expect(sidecarActionsSOAfterMigration.hits.hits.length).to.eql(0); - - expect(ruleSO?.alert.actions).to.eql([]); - expect(ruleSO?.alert.throttle).to.eql(null); - expect(ruleSO?.alert.notifyWhen).to.eql(null); - }); - - it('migrates legacy actions for rule with action run on every run', async () => { - const soId = 'dc6595f0-b075-11ec-bb3f-1f063f8e06cf'; - const ruleId = '72a0d429-363b-4f70-905e-c6019a224d40'; - const legacySidecarId = 'dde13970-b075-11ec-bb3f-1f063f8e06cf'; - - // check for legacy sidecar action - const sidecarActionSO = await getLegacyActionSOById(es, legacySidecarId); - expect(sidecarActionSO.hits.hits.length).to.eql(1); - - // check for legacy notification SO - // should not have been created for a rule that runs on every rule run - const legacyNotificationSO = await getLegacyActionNotificationSOById(es, soId); - expect(legacyNotificationSO.hits.hits.length).to.eql(0); - - // patch enable the rule - // any route that edits the rule should trigger the migration - await supertest - .patch(DETECTION_ENGINE_RULES_URL) - .set('kbn-xsrf', 'true') - .set('elastic-api-version', '2023-10-31') - .send({ rule_id: ruleId, enabled: false }) - .expect(200); - - const { - hits: { - hits: [{ _source: ruleSO }], - }, - } = await getRuleSOById(es, soId); - - // Sidecar should be removed - const sidecarActionsSOAfterMigration = await getLegacyActionSOById(es, legacySidecarId); - expect(sidecarActionsSOAfterMigration.hits.hits.length).to.eql(0); - - expect(ruleSO?.alert.actions).to.eql([ - { - actionRef: 'action_0', - actionTypeId: '.email', - group: 'default', - params: { - message: 'Rule {{context.rule.name}} generated {{state.signals_count}} alerts', - subject: 'Test Actions', - to: ['test@test.com'], - }, - uuid: ruleSO?.alert.actions[0].uuid, - frequency: { summary: true, throttle: null, notifyWhen: 'onActiveAlert' }, - }, - { - actionRef: 'action_1', - actionTypeId: '.email', - group: 'default', - params: { - message: 'Rule {{context.rule.name}} generated {{state.signals_count}} alerts', - subject: 'Test Actions', - to: ['test@test.com'], - }, - uuid: ruleSO?.alert.actions[1].uuid, - frequency: { summary: true, throttle: null, notifyWhen: 'onActiveAlert' }, - }, - ]); - expect(ruleSO?.alert.throttle).to.eql(null); - expect(ruleSO?.alert.notifyWhen).to.eql(null); - expect(ruleSO?.references).to.eql([ - { - id: 'c95cb100-b075-11ec-bb3f-1f063f8e06cf', - name: 'action_0', - type: 'action', - }, - { - id: 'c95cb100-b075-11ec-bb3f-1f063f8e06cf', - name: 'action_1', - type: 'action', - }, - ]); - }); - - it('migrates legacy actions for rule with action run hourly', async () => { - const soId = '064e3160-b076-11ec-bb3f-1f063f8e06cf'; - const ruleId = '4c056b05-75ac-4209-be32-82100f771eb4'; - const legacySidecarId = '07aa8d10-b076-11ec-bb3f-1f063f8e06cf'; - - // check for legacy sidecar action - const sidecarActionSO = await getLegacyActionSOById(es, legacySidecarId); - expect(sidecarActionSO.hits.hits.length).to.eql(1); - - // check for legacy notification SO - const legacyNotificationSO = await getLegacyActionNotificationSOById(es, soId); - expect(legacyNotificationSO.hits.hits.length).to.eql(1); - - // patch enable the rule - // any route that edits the rule should trigger the migration - await supertest - .patch(DETECTION_ENGINE_RULES_URL) - .set('kbn-xsrf', 'true') - .set('elastic-api-version', '2023-10-31') - .send({ rule_id: ruleId, enabled: false }) - .expect(200); - - const { - hits: { - hits: [{ _source: ruleSO }], - }, - } = await getRuleSOById(es, soId); - - // Sidecar should be removed - const sidecarActionsSOAfterMigration = await getLegacyActionSOById(es, legacySidecarId); - expect(sidecarActionsSOAfterMigration.hits.hits.length).to.eql(0); - - // Legacy notification should be removed - const legacyNotificationSOAfterMigration = await getLegacyActionNotificationSOById(es, soId); - expect(legacyNotificationSOAfterMigration.hits.hits.length).to.eql(0); - - expect(ruleSO?.alert.actions).to.eql([ - { - actionTypeId: '.email', - params: { - subject: 'Rule email', - to: ['test@test.com'], - message: 'Rule {{context.rule.name}} generated {{state.signals_count}} alerts', - }, - actionRef: 'action_0', - group: 'default', - uuid: ruleSO?.alert.actions[0].uuid, - frequency: { summary: true, throttle: '1h', notifyWhen: 'onThrottleInterval' }, - }, - { - actionTypeId: '.slack', - params: { - message: 'Rule {{context.rule.name}} generated {{state.signals_count}} alerts', - }, - actionRef: 'action_1', - group: 'default', - uuid: ruleSO?.alert.actions[1].uuid, - frequency: { summary: true, throttle: '1h', notifyWhen: 'onThrottleInterval' }, - }, - ]); - expect(ruleSO?.alert.throttle).to.eql(undefined); - expect(ruleSO?.alert.notifyWhen).to.eql(null); - expect(ruleSO?.references).to.eql([ - { - id: 'c95cb100-b075-11ec-bb3f-1f063f8e06cf', - name: 'action_0', - type: 'action', - }, - { - id: '207fa0e0-c04e-11ec-8a52-4fb92379525a', - name: 'action_1', - type: 'action', - }, - ]); - }); - - it('migrates legacy actions for rule with action run daily', async () => { - const soId = '27639570-b076-11ec-bb3f-1f063f8e06cf'; - const ruleId = '8e2c8550-f13f-4e21-be0c-92148d71a5f1'; - const legacySidecarId = '291ae260-b076-11ec-bb3f-1f063f8e06cf'; - - // check for legacy sidecar action - const sidecarActionSO = await getLegacyActionSOById(es, legacySidecarId); - expect(sidecarActionSO.hits.hits.length).to.eql(1); - - // check for legacy notification SO - const legacyNotificationSO = await getLegacyActionNotificationSOById(es, soId); - expect(legacyNotificationSO.hits.hits.length).to.eql(1); - - // patch enable the rule - await supertest - .patch(DETECTION_ENGINE_RULES_URL) - .set('kbn-xsrf', 'true') - .set('elastic-api-version', '2023-10-31') - .send({ rule_id: ruleId, enabled: false }) - .expect(200); - - const { - hits: { - hits: [{ _source: ruleSO }], - }, - } = await getRuleSOById(es, soId); - - // Sidecar should be removed - const sidecarActionsSOAfterMigration = await getLegacyActionSOById(es, legacySidecarId); - expect(sidecarActionsSOAfterMigration.hits.hits.length).to.eql(0); - - // Legacy notification should be removed - const legacyNotificationSOAfterMigration = await getLegacyActionNotificationSOById(es, soId); - expect(legacyNotificationSOAfterMigration.hits.hits.length).to.eql(0); - - expect(ruleSO?.alert.actions).to.eql([ - { - actionRef: 'action_0', - actionTypeId: '.email', - group: 'default', - params: { - message: 'Rule {{context.rule.name}} generated {{state.signals_count}} alerts', - subject: 'Test Actions', - to: ['test@test.com'], - }, - uuid: ruleSO?.alert.actions[0].uuid, - frequency: { summary: true, throttle: '1d', notifyWhen: 'onThrottleInterval' }, - }, - ]); - expect(ruleSO?.alert.throttle).to.eql(undefined); - expect(ruleSO?.alert.notifyWhen).to.eql(null); - expect(ruleSO?.references).to.eql([ - { - id: 'c95cb100-b075-11ec-bb3f-1f063f8e06cf', - name: 'action_0', - type: 'action', - }, - ]); - }); - - it('migrates legacy actions for rule with action run weekly', async () => { - const soId = '61ec7a40-b076-11ec-bb3f-1f063f8e06cf'; - const ruleId = '05fbdd2a-e802-420b-bdc3-95ae0acca454'; - const legacySidecarId = '63aa2fd0-b076-11ec-bb3f-1f063f8e06cf'; - - // check for legacy sidecar action - const sidecarActionSO = await getLegacyActionSOById(es, legacySidecarId); - expect(sidecarActionSO.hits.hits.length).to.eql(1); - - // check for legacy notification SO - const legacyNotificationSO = await getLegacyActionNotificationSOById(es, soId); - expect(legacyNotificationSO.hits.hits.length).to.eql(1); - - // patch enable the rule - await supertest - .patch(DETECTION_ENGINE_RULES_URL) - .set('kbn-xsrf', 'true') - .set('elastic-api-version', '2023-10-31') - .send({ rule_id: ruleId, enabled: false }) - .expect(200); - - const { - hits: { - hits: [{ _source: ruleSO }], - }, - } = await getRuleSOById(es, soId); - - // Sidecar should be removed - const sidecarActionsSOAfterMigration = await getLegacyActionSOById(es, legacySidecarId); - expect(sidecarActionsSOAfterMigration.hits.hits.length).to.eql(0); - - // Legacy notification should be removed - const legacyNotificationSOAfterMigration = await getLegacyActionNotificationSOById(es, soId); - expect(legacyNotificationSOAfterMigration.hits.hits.length).to.eql(0); - - expect(ruleSO?.alert.actions).to.eql([ - { - actionRef: 'action_0', - actionTypeId: '.email', - group: 'default', - params: { - message: 'Rule {{context.rule.name}} generated {{state.signals_count}} alerts', - subject: 'Test Actions', - to: ['test@test.com'], - }, - uuid: ruleSO?.alert.actions[0].uuid, - frequency: { summary: true, throttle: '7d', notifyWhen: 'onThrottleInterval' }, - }, - ]); - expect(ruleSO?.alert.throttle).to.eql(undefined); - expect(ruleSO?.alert.notifyWhen).to.eql(null); - expect(ruleSO?.references).to.eql([ - { - id: 'c95cb100-b075-11ec-bb3f-1f063f8e06cf', - name: 'action_0', - type: 'action', - }, - ]); - }); - }); -}; diff --git a/x-pack/test/detection_engine_api_integration/security_and_spaces/group10/migrations.ts b/x-pack/test/detection_engine_api_integration/security_and_spaces/group10/migrations.ts deleted file mode 100644 index d43ecaff6823e..0000000000000 --- a/x-pack/test/detection_engine_api_integration/security_and_spaces/group10/migrations.ts +++ /dev/null @@ -1,102 +0,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 { SECURITY_SOLUTION_SAVED_OBJECT_INDEX } from '@kbn/core-saved-objects-server'; -import expect from '@kbn/expect'; -import { FtrProviderContext } from '../../common/ftr_provider_context'; - -// eslint-disable-next-line import/no-default-export -export default ({ getService }: FtrProviderContext): void => { - const esArchiver = getService('esArchiver'); - const es = getService('es'); - - describe('migrations', () => { - before(async () => { - await esArchiver.load('x-pack/test/functional/es_archives/security_solution/migrations'); - }); - - after(async () => { - await esArchiver.unload('x-pack/test/functional/es_archives/security_solution/migrations'); - }); - - describe('7.16.0', () => { - it('migrates legacy siem-detection-engine-rule-actions to use saved object references', async () => { - const response = await es.get<{ - 'siem-detection-engine-rule-actions': { - ruleAlertId: string; - actions: [{ id: string; actionRef: string }]; - }; - references: [{}]; - }>( - { - index: SECURITY_SOLUTION_SAVED_OBJECT_INDEX, - id: 'siem-detection-engine-rule-actions:fce024a0-0452-11ec-9b15-d13d79d162f3', - }, - { - meta: true, - } - ); - expect(response.statusCode).to.eql(200); - - // references exist and are expected values - expect(response.body._source?.references).to.eql([ - { - name: 'alert_0', - id: 'fb1046a0-0452-11ec-9b15-d13d79d162f3', - type: 'alert', - }, - { - name: 'action_0', - id: 'f6e64c00-0452-11ec-9b15-d13d79d162f3', - type: 'action', - }, - ]); - - // actionRef exists and is the expected value - expect( - response.body._source?.['siem-detection-engine-rule-actions'].actions[0].actionRef - ).to.eql('action_0'); - - // ruleAlertId no longer exist - expect(response.body._source?.['siem-detection-engine-rule-actions'].ruleAlertId).to.eql( - undefined - ); - - // actions.id no longer exist - expect(response.body._source?.['siem-detection-engine-rule-actions'].actions[0].id).to.eql( - undefined - ); - }); - - it('migrates legacy siem-detection-engine-rule-actions and retains "ruleThrottle" and "alertThrottle" as the same attributes as before', async () => { - const response = await es.get<{ - 'siem-detection-engine-rule-actions': { - ruleThrottle: string; - alertThrottle: string; - }; - }>( - { - index: SECURITY_SOLUTION_SAVED_OBJECT_INDEX, - id: 'siem-detection-engine-rule-actions:fce024a0-0452-11ec-9b15-d13d79d162f3', - }, - { - meta: true, - } - ); - expect(response.statusCode).to.eql(200); - - // "alertThrottle" and "ruleThrottle" should still exist - expect(response.body._source?.['siem-detection-engine-rule-actions'].alertThrottle).to.eql( - '7d' - ); - expect(response.body._source?.['siem-detection-engine-rule-actions'].ruleThrottle).to.eql( - '7d' - ); - }); - }); - }); -}; diff --git a/x-pack/test/security_solution_api_integration/config/serverless/config.base.ts b/x-pack/test/security_solution_api_integration/config/serverless/config.base.ts index 105701ec61e08..6790c39c85115 100644 --- a/x-pack/test/security_solution_api_integration/config/serverless/config.base.ts +++ b/x-pack/test/security_solution_api_integration/config/serverless/config.base.ts @@ -5,8 +5,6 @@ * 2.0. */ import { FtrConfigProviderContext } from '@kbn/test'; -// import { ES_RESOURCES } from '@kbn/security-solution-plugin/scripts/endpoint/common/roles_users/serverless'; - export interface CreateTestConfigOptions { testFiles: string[]; junit: { reportName: string }; @@ -24,9 +22,6 @@ export function createTestConfig(options: CreateTestConfigOptions) { ...svlSharedConfig.get('kbnTestServer'), serverArgs: [...svlSharedConfig.get('kbnTestServer.serverArgs'), '--serverless=security'], }, - // esServerlessOptions: { - // resources: Object.values(ES_RESOURCES), - // }, testFiles: options.testFiles, junit: options.junit, diff --git a/x-pack/test/security_solution_api_integration/package.json b/x-pack/test/security_solution_api_integration/package.json index 4562cfc82cfc9..305db2251e0b1 100644 --- a/x-pack/test/security_solution_api_integration/package.json +++ b/x-pack/test/security_solution_api_integration/package.json @@ -31,6 +31,11 @@ "rule_creation:runner:serverless": "npm run run-tests rule_creation serverless serverlessEnv", "rule_creation:qa:serverless": "npm run run-tests rule_creation serverless qaEnv", "rule_creation:server:ess": "npm run initialize-server rule_creation ess", - "rule_creation:runner:ess": "npm run run-tests rule_creation ess essEnv" + "rule_creation:runner:ess": "npm run run-tests rule_creation ess essEnv", + "actions:server:serverless": "npm run initialize-server actions serverless", + "actions:runner:serverless": "npm run run-tests actions serverless serverlessEnv", + "actions:qa:serverless": "npm run run-tests actions serverless qaEnv", + "actions:server:ess": "npm run initialize-server actions ess", + "actions:runner:ess": "npm run run-tests actions ess essEnv" } } diff --git a/x-pack/test/security_solution_api_integration/scripts/index.js b/x-pack/test/security_solution_api_integration/scripts/index.js index 635c135e2c8b1..af12482f9f293 100644 --- a/x-pack/test/security_solution_api_integration/scripts/index.js +++ b/x-pack/test/security_solution_api_integration/scripts/index.js @@ -31,15 +31,15 @@ let grepArgs = []; if (type !== 'server') { switch (environment) { case 'serverlessEnv': - grepArgs = ['--grep', '@serverless', '--grep', '@brokenInServerless', '--invert']; + grepArgs = ['--grep', '/^(?!.*@brokenInServerless).*@serverless.*/']; break; case 'essEnv': - grepArgs = ['--grep', '@ess']; + grepArgs = ['--grep', '/^(?!.*@brokenInEss).*@ess.*/']; break; case 'qaEnv': - grepArgs = ['--grep', '@serverless', '--grep', '@brokenInServerless|@skipInQA', '--invert']; + grepArgs = ['--grep', '/^(?!.*@brokenInServerless|.*@skipInQA).*@serverless.*/']; break; default: diff --git a/x-pack/test/detection_engine_api_integration/security_and_spaces/group1/add_actions.ts b/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions/add_actions.ts similarity index 71% rename from x-pack/test/detection_engine_api_integration/security_and_spaces/group1/add_actions.ts rename to x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions/add_actions.ts index 2bfd3c6102e61..257378f7442b9 100644 --- a/x-pack/test/detection_engine_api_integration/security_and_spaces/group1/add_actions.ts +++ b/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions/add_actions.ts @@ -8,9 +8,8 @@ import expect from '@kbn/expect'; import { RuleCreateProps } from '@kbn/security-solution-plugin/common/api/detection_engine'; -import { FtrProviderContext } from '../../common/ftr_provider_context'; import { - createSignalsIndex, + createAlertsIndex, deleteAllRules, removeServerGeneratedProperties, getWebHookAction, @@ -19,27 +18,35 @@ import { waitForRuleSuccess, createRule, deleteAllAlerts, + updateUsername, } from '../../utils'; +import { FtrProviderContext } from '../../../../ftr_provider_context'; +import { EsArchivePathBuilder } from '../../../../es_archive_path_builder'; -// eslint-disable-next-line import/no-default-export export default ({ getService }: FtrProviderContext) => { const supertest = getService('supertest'); const esArchiver = getService('esArchiver'); const log = getService('log'); const es = getService('es'); + // TODO: add a new service + const config = getService('config'); + const ELASTICSEARCH_USERNAME = config.get('servers.kibana.username'); + const isServerless = config.get('serverless'); + const dataPathBuilder = new EsArchivePathBuilder(isServerless); + const path = dataPathBuilder.getPath('auditbeat/hosts'); - describe('add_actions', () => { + describe('@serverless @ess add_actions', () => { describe('adding actions', () => { before(async () => { - await esArchiver.load('x-pack/test/functional/es_archives/auditbeat/hosts'); + await esArchiver.load(path); }); after(async () => { - await esArchiver.unload('x-pack/test/functional/es_archives/auditbeat/hosts'); + await esArchiver.unload(path); }); beforeEach(async () => { - await createSignalsIndex(supertest, log); + await createAlertsIndex(supertest, log); }); afterEach(async () => { @@ -52,17 +59,18 @@ export default ({ getService }: FtrProviderContext) => { const { body: hookAction } = await supertest .post('/api/actions/action') .set('kbn-xsrf', 'true') + .set('x-elastic-internal-origin', 'foo') .send(getWebHookAction()) .expect(200); const rule = await createRule(supertest, log, getRuleWithWebHookAction(hookAction.id)); const bodyToCompare = removeServerGeneratedProperties(rule); - expect(bodyToCompare).to.eql( - getSimpleRuleOutputWithWebHookAction( - `${bodyToCompare?.actions?.[0].id}`, - `${bodyToCompare?.actions?.[0].uuid}` - ) + const expected = getSimpleRuleOutputWithWebHookAction( + `${bodyToCompare?.actions?.[0].id}`, + `${bodyToCompare?.actions?.[0].uuid}` ); + const expectedRuleWithUserUpdated = updateUsername(expected, ELASTICSEARCH_USERNAME); + expect(bodyToCompare).to.eql(expectedRuleWithUserUpdated); }); it('should be able to create a new webhook action and attach it to a rule without a meta field and run it correctly', async () => { @@ -70,6 +78,7 @@ export default ({ getService }: FtrProviderContext) => { const { body: hookAction } = await supertest .post('/api/actions/action') .set('kbn-xsrf', 'true') + .set('x-elastic-internal-origin', 'foo') .send(getWebHookAction()) .expect(200); @@ -86,6 +95,7 @@ export default ({ getService }: FtrProviderContext) => { const { body: hookAction } = await supertest .post('/api/actions/action') .set('kbn-xsrf', 'true') + .set('x-elastic-internal-origin', 'foo') .send(getWebHookAction()) .expect(200); diff --git a/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions/configs/ess.config.ts b/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions/configs/ess.config.ts new file mode 100644 index 0000000000000..cec8d1cca41b5 --- /dev/null +++ b/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions/configs/ess.config.ts @@ -0,0 +1,22 @@ +/* + * 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 { FtrConfigProviderContext } from '@kbn/test'; + +export default async function ({ readConfigFile }: FtrConfigProviderContext) { + const functionalConfig = await readConfigFile( + require.resolve('../../../../../config/ess/config.base.trial') + ); + + return { + ...functionalConfig.getAll(), + testFiles: [require.resolve('..')], + junit: { + reportName: 'Detection Engine ESS/Actions API Integration Tests', + }, + }; +} diff --git a/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions/configs/serverless.config.ts b/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions/configs/serverless.config.ts new file mode 100644 index 0000000000000..66edc0eef7f30 --- /dev/null +++ b/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions/configs/serverless.config.ts @@ -0,0 +1,15 @@ +/* + * 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 { createTestConfig } from '../../../../../config/serverless/config.base'; + +export default createTestConfig({ + testFiles: [require.resolve('..')], + junit: { + reportName: 'Detection Engine Serverless/Actions API Integration Tests', + }, +}); diff --git a/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions/index.ts b/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions/index.ts new file mode 100644 index 0000000000000..5c26d445eb158 --- /dev/null +++ b/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions/index.ts @@ -0,0 +1,15 @@ +/* + * 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 { FtrProviderContext } from '../../../../ftr_provider_context'; + +export default function ({ loadTestFile }: FtrProviderContext) { + describe('Actions API', function () { + loadTestFile(require.resolve('./add_actions')); + loadTestFile(require.resolve('./update_actions')); + loadTestFile(require.resolve('./migrations')); + }); +} diff --git a/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions/migrations.ts b/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions/migrations.ts new file mode 100644 index 0000000000000..ce5c87d2c3fb4 --- /dev/null +++ b/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions/migrations.ts @@ -0,0 +1,451 @@ +/* + * 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 expect from '@kbn/expect'; +import { SECURITY_SOLUTION_SAVED_OBJECT_INDEX } from '@kbn/core-saved-objects-server'; + +import { DETECTION_ENGINE_RULES_URL } from '@kbn/security-solution-plugin/common/constants'; +import { + getLegacyActionSOById, + getLegacyActionNotificationSOById, + getRuleSOById, +} from '../../../../../detection_engine_api_integration/utils'; +import { FtrProviderContext } from '../../../../ftr_provider_context'; + +/** + * @deprecated Once the legacy notification system is removed, remove this test too. + */ +export default ({ getService }: FtrProviderContext) => { + const supertest = getService('supertest'); + const es = getService('es'); + const esArchiver = getService('esArchiver'); + + describe('@ess @skipInQA actions migrations', () => { + // This test suite is not meant to test a specific route, but to test the legacy action migration + // code that lives in multiple routes. This code is also tested in each of the routes it lives in + // but not in as much detail and relying on mocks. This test loads an es_archive containing rules + // created in 7.15 with legacy actions. + // For new routes that do any updates on a rule, please ensure that you are including the legacy + // action migration code. We are monitoring legacy action telemetry to clean up once we see their + // existence being near 0. + describe('legacy actions', () => { + before(async () => { + await esArchiver.load( + 'x-pack/test/functional/es_archives/security_solution/legacy_actions' + ); + }); + + after(async () => { + await esArchiver.unload( + 'x-pack/test/functional/es_archives/security_solution/legacy_actions' + ); + }); + + it('migrates legacy actions for rule with no actions', async () => { + const soId = '9095ee90-b075-11ec-bb3f-1f063f8e06cf'; + const ruleId = '2297be91-894c-4831-830f-b424a0ec84f0'; + const legacySidecarId = '926668d0-b075-11ec-bb3f-1f063f8e06cf'; + + // check for legacy sidecar action + const sidecarActionSO = await getLegacyActionSOById(es, legacySidecarId); + expect(sidecarActionSO.hits.hits.length).to.eql(1); + + // check for legacy notification SO + // should not have been created for a rule with no actions + const legacyNotificationSO = await getLegacyActionNotificationSOById(es, soId); + expect(legacyNotificationSO.hits.hits.length).to.eql(0); + + // patch enable the rule + // any route that edits the rule should trigger the migration + await supertest + .patch(DETECTION_ENGINE_RULES_URL) + .set('kbn-xsrf', 'true') + .set('elastic-api-version', '2023-10-31') + .send({ rule_id: ruleId, enabled: false }) + .expect(200); + + const { + hits: { + hits: [{ _source: ruleSO }], + }, + } = await getRuleSOById(es, soId); + + // Sidecar should be removed + const sidecarActionsSOAfterMigration = await getLegacyActionSOById(es, legacySidecarId); + expect(sidecarActionsSOAfterMigration.hits.hits.length).to.eql(0); + + expect(ruleSO?.alert.actions).to.eql([]); + expect(ruleSO?.alert.throttle).to.eql(null); + expect(ruleSO?.alert.notifyWhen).to.eql(null); + }); + + it('migrates legacy actions for rule with action run on every run', async () => { + const soId = 'dc6595f0-b075-11ec-bb3f-1f063f8e06cf'; + const ruleId = '72a0d429-363b-4f70-905e-c6019a224d40'; + const legacySidecarId = 'dde13970-b075-11ec-bb3f-1f063f8e06cf'; + + // check for legacy sidecar action + const sidecarActionSO = await getLegacyActionSOById(es, legacySidecarId); + expect(sidecarActionSO.hits.hits.length).to.eql(1); + + // check for legacy notification SO + // should not have been created for a rule that runs on every rule run + const legacyNotificationSO = await getLegacyActionNotificationSOById(es, soId); + expect(legacyNotificationSO.hits.hits.length).to.eql(0); + + // patch enable the rule + // any route that edits the rule should trigger the migration + await supertest + .patch(DETECTION_ENGINE_RULES_URL) + .set('kbn-xsrf', 'true') + .set('elastic-api-version', '2023-10-31') + .send({ rule_id: ruleId, enabled: false }) + .expect(200); + + const { + hits: { + hits: [{ _source: ruleSO }], + }, + } = await getRuleSOById(es, soId); + + // Sidecar should be removed + const sidecarActionsSOAfterMigration = await getLegacyActionSOById(es, legacySidecarId); + expect(sidecarActionsSOAfterMigration.hits.hits.length).to.eql(0); + + expect(ruleSO?.alert.actions).to.eql([ + { + actionRef: 'action_0', + actionTypeId: '.email', + group: 'default', + params: { + message: 'Rule {{context.rule.name}} generated {{state.signals_count}} alerts', + subject: 'Test Actions', + to: ['test@test.com'], + }, + uuid: ruleSO?.alert.actions[0].uuid, + frequency: { summary: true, throttle: null, notifyWhen: 'onActiveAlert' }, + }, + { + actionRef: 'action_1', + actionTypeId: '.email', + group: 'default', + params: { + message: 'Rule {{context.rule.name}} generated {{state.signals_count}} alerts', + subject: 'Test Actions', + to: ['test@test.com'], + }, + uuid: ruleSO?.alert.actions[1].uuid, + frequency: { summary: true, throttle: null, notifyWhen: 'onActiveAlert' }, + }, + ]); + expect(ruleSO?.alert.throttle).to.eql(null); + expect(ruleSO?.alert.notifyWhen).to.eql(null); + expect(ruleSO?.references).to.eql([ + { + id: 'c95cb100-b075-11ec-bb3f-1f063f8e06cf', + name: 'action_0', + type: 'action', + }, + { + id: 'c95cb100-b075-11ec-bb3f-1f063f8e06cf', + name: 'action_1', + type: 'action', + }, + ]); + }); + + it('migrates legacy actions for rule with action run hourly', async () => { + const soId = '064e3160-b076-11ec-bb3f-1f063f8e06cf'; + const ruleId = '4c056b05-75ac-4209-be32-82100f771eb4'; + const legacySidecarId = '07aa8d10-b076-11ec-bb3f-1f063f8e06cf'; + + // check for legacy sidecar action + const sidecarActionSO = await getLegacyActionSOById(es, legacySidecarId); + expect(sidecarActionSO.hits.hits.length).to.eql(1); + + // check for legacy notification SO + const legacyNotificationSO = await getLegacyActionNotificationSOById(es, soId); + expect(legacyNotificationSO.hits.hits.length).to.eql(1); + + // patch enable the rule + // any route that edits the rule should trigger the migration + await supertest + .patch(DETECTION_ENGINE_RULES_URL) + .set('kbn-xsrf', 'true') + .set('elastic-api-version', '2023-10-31') + .send({ rule_id: ruleId, enabled: false }) + .expect(200); + + const { + hits: { + hits: [{ _source: ruleSO }], + }, + } = await getRuleSOById(es, soId); + + // Sidecar should be removed + const sidecarActionsSOAfterMigration = await getLegacyActionSOById(es, legacySidecarId); + expect(sidecarActionsSOAfterMigration.hits.hits.length).to.eql(0); + + // Legacy notification should be removed + const legacyNotificationSOAfterMigration = await getLegacyActionNotificationSOById( + es, + soId + ); + expect(legacyNotificationSOAfterMigration.hits.hits.length).to.eql(0); + + expect(ruleSO?.alert.actions).to.eql([ + { + actionTypeId: '.email', + params: { + subject: 'Rule email', + to: ['test@test.com'], + message: 'Rule {{context.rule.name}} generated {{state.signals_count}} alerts', + }, + actionRef: 'action_0', + group: 'default', + uuid: ruleSO?.alert.actions[0].uuid, + frequency: { summary: true, throttle: '1h', notifyWhen: 'onThrottleInterval' }, + }, + { + actionTypeId: '.slack', + params: { + message: 'Rule {{context.rule.name}} generated {{state.signals_count}} alerts', + }, + actionRef: 'action_1', + group: 'default', + uuid: ruleSO?.alert.actions[1].uuid, + frequency: { summary: true, throttle: '1h', notifyWhen: 'onThrottleInterval' }, + }, + ]); + expect(ruleSO?.alert.throttle).to.eql(undefined); + expect(ruleSO?.alert.notifyWhen).to.eql(null); + expect(ruleSO?.references).to.eql([ + { + id: 'c95cb100-b075-11ec-bb3f-1f063f8e06cf', + name: 'action_0', + type: 'action', + }, + { + id: '207fa0e0-c04e-11ec-8a52-4fb92379525a', + name: 'action_1', + type: 'action', + }, + ]); + }); + + it('migrates legacy actions for rule with action run daily', async () => { + const soId = '27639570-b076-11ec-bb3f-1f063f8e06cf'; + const ruleId = '8e2c8550-f13f-4e21-be0c-92148d71a5f1'; + const legacySidecarId = '291ae260-b076-11ec-bb3f-1f063f8e06cf'; + + // check for legacy sidecar action + const sidecarActionSO = await getLegacyActionSOById(es, legacySidecarId); + expect(sidecarActionSO.hits.hits.length).to.eql(1); + + // check for legacy notification SO + const legacyNotificationSO = await getLegacyActionNotificationSOById(es, soId); + expect(legacyNotificationSO.hits.hits.length).to.eql(1); + + // patch enable the rule + await supertest + .patch(DETECTION_ENGINE_RULES_URL) + .set('kbn-xsrf', 'true') + .set('elastic-api-version', '2023-10-31') + .send({ rule_id: ruleId, enabled: false }) + .expect(200); + + const { + hits: { + hits: [{ _source: ruleSO }], + }, + } = await getRuleSOById(es, soId); + + // Sidecar should be removed + const sidecarActionsSOAfterMigration = await getLegacyActionSOById(es, legacySidecarId); + expect(sidecarActionsSOAfterMigration.hits.hits.length).to.eql(0); + + // Legacy notification should be removed + const legacyNotificationSOAfterMigration = await getLegacyActionNotificationSOById( + es, + soId + ); + expect(legacyNotificationSOAfterMigration.hits.hits.length).to.eql(0); + + expect(ruleSO?.alert.actions).to.eql([ + { + actionRef: 'action_0', + actionTypeId: '.email', + group: 'default', + params: { + message: 'Rule {{context.rule.name}} generated {{state.signals_count}} alerts', + subject: 'Test Actions', + to: ['test@test.com'], + }, + uuid: ruleSO?.alert.actions[0].uuid, + frequency: { summary: true, throttle: '1d', notifyWhen: 'onThrottleInterval' }, + }, + ]); + expect(ruleSO?.alert.throttle).to.eql(undefined); + expect(ruleSO?.alert.notifyWhen).to.eql(null); + expect(ruleSO?.references).to.eql([ + { + id: 'c95cb100-b075-11ec-bb3f-1f063f8e06cf', + name: 'action_0', + type: 'action', + }, + ]); + }); + + it('migrates legacy actions for rule with action run weekly', async () => { + const soId = '61ec7a40-b076-11ec-bb3f-1f063f8e06cf'; + const ruleId = '05fbdd2a-e802-420b-bdc3-95ae0acca454'; + const legacySidecarId = '63aa2fd0-b076-11ec-bb3f-1f063f8e06cf'; + + // check for legacy sidecar action + const sidecarActionSO = await getLegacyActionSOById(es, legacySidecarId); + expect(sidecarActionSO.hits.hits.length).to.eql(1); + + // check for legacy notification SO + const legacyNotificationSO = await getLegacyActionNotificationSOById(es, soId); + expect(legacyNotificationSO.hits.hits.length).to.eql(1); + + // patch enable the rule + await supertest + .patch(DETECTION_ENGINE_RULES_URL) + .set('kbn-xsrf', 'true') + .set('elastic-api-version', '2023-10-31') + .send({ rule_id: ruleId, enabled: false }) + .expect(200); + + const { + hits: { + hits: [{ _source: ruleSO }], + }, + } = await getRuleSOById(es, soId); + + // Sidecar should be removed + const sidecarActionsSOAfterMigration = await getLegacyActionSOById(es, legacySidecarId); + expect(sidecarActionsSOAfterMigration.hits.hits.length).to.eql(0); + + // Legacy notification should be removed + const legacyNotificationSOAfterMigration = await getLegacyActionNotificationSOById( + es, + soId + ); + expect(legacyNotificationSOAfterMigration.hits.hits.length).to.eql(0); + + expect(ruleSO?.alert.actions).to.eql([ + { + actionRef: 'action_0', + actionTypeId: '.email', + group: 'default', + params: { + message: 'Rule {{context.rule.name}} generated {{state.signals_count}} alerts', + subject: 'Test Actions', + to: ['test@test.com'], + }, + uuid: ruleSO?.alert.actions[0].uuid, + frequency: { summary: true, throttle: '7d', notifyWhen: 'onThrottleInterval' }, + }, + ]); + expect(ruleSO?.alert.throttle).to.eql(undefined); + expect(ruleSO?.alert.notifyWhen).to.eql(null); + expect(ruleSO?.references).to.eql([ + { + id: 'c95cb100-b075-11ec-bb3f-1f063f8e06cf', + name: 'action_0', + type: 'action', + }, + ]); + }); + }); + + describe('7.16.0', () => { + before(async () => { + await esArchiver.load('x-pack/test/functional/es_archives/security_solution/migrations'); + }); + + after(async () => { + await esArchiver.unload('x-pack/test/functional/es_archives/security_solution/migrations'); + }); + + it('migrates legacy siem-detection-engine-rule-actions to use saved object references', async () => { + const response = await es.get<{ + 'siem-detection-engine-rule-actions': { + ruleAlertId: string; + actions: [{ id: string; actionRef: string }]; + }; + references: [{}]; + }>( + { + index: SECURITY_SOLUTION_SAVED_OBJECT_INDEX, + id: 'siem-detection-engine-rule-actions:fce024a0-0452-11ec-9b15-d13d79d162f3', + }, + { + meta: true, + } + ); + expect(response.statusCode).to.eql(200); + + // references exist and are expected values + expect(response.body._source?.references).to.eql([ + { + name: 'alert_0', + id: 'fb1046a0-0452-11ec-9b15-d13d79d162f3', + type: 'alert', + }, + { + name: 'action_0', + id: 'f6e64c00-0452-11ec-9b15-d13d79d162f3', + type: 'action', + }, + ]); + + // actionRef exists and is the expected value + expect( + response.body._source?.['siem-detection-engine-rule-actions'].actions[0].actionRef + ).to.eql('action_0'); + + // ruleAlertId no longer exist + expect(response.body._source?.['siem-detection-engine-rule-actions'].ruleAlertId).to.eql( + undefined + ); + + // actions.id no longer exist + expect(response.body._source?.['siem-detection-engine-rule-actions'].actions[0].id).to.eql( + undefined + ); + }); + + it('migrates legacy siem-detection-engine-rule-actions and retains "ruleThrottle" and "alertThrottle" as the same attributes as before', async () => { + const response = await es.get<{ + 'siem-detection-engine-rule-actions': { + ruleThrottle: string; + alertThrottle: string; + }; + }>( + { + index: SECURITY_SOLUTION_SAVED_OBJECT_INDEX, + id: 'siem-detection-engine-rule-actions:fce024a0-0452-11ec-9b15-d13d79d162f3', + }, + { + meta: true, + } + ); + expect(response.statusCode).to.eql(200); + + // "alertThrottle" and "ruleThrottle" should still exist + expect(response.body._source?.['siem-detection-engine-rule-actions'].alertThrottle).to.eql( + '7d' + ); + expect(response.body._source?.['siem-detection-engine-rule-actions'].ruleThrottle).to.eql( + '7d' + ); + }); + }); + }); +}; diff --git a/x-pack/test/detection_engine_api_integration/security_and_spaces/group1/update_actions.ts b/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions/update_actions.ts similarity index 76% rename from x-pack/test/detection_engine_api_integration/security_and_spaces/group1/update_actions.ts rename to x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions/update_actions.ts index 8390fc10ad7fc..f56d949eadd38 100644 --- a/x-pack/test/detection_engine_api_integration/security_and_spaces/group1/update_actions.ts +++ b/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/actions/update_actions.ts @@ -10,9 +10,8 @@ import { omit } from 'lodash'; import { RuleCreateProps } from '@kbn/security-solution-plugin/common/api/detection_engine'; import { ELASTIC_SECURITY_RULE_ID } from '@kbn/security-solution-plugin/common'; -import { FtrProviderContext } from '../../common/ftr_provider_context'; import { - createSignalsIndex, + createAlertsIndex, deleteAllRules, deleteAllAlerts, removeServerGeneratedProperties, @@ -29,32 +28,40 @@ import { getPrebuiltRulesAndTimelinesStatus, getSimpleRuleOutput, ruleToUpdateSchema, + updateUsername, } from '../../utils'; +import { FtrProviderContext } from '../../../../ftr_provider_context'; +import { EsArchivePathBuilder } from '../../../../es_archive_path_builder'; -// eslint-disable-next-line import/no-default-export export default ({ getService }: FtrProviderContext) => { const es = getService('es'); const supertest = getService('supertest'); const esArchiver = getService('esArchiver'); const log = getService('log'); + // TODO: add a new service + const config = getService('config'); + const ELASTICSEARCH_USERNAME = config.get('servers.kibana.username'); + const isServerless = config.get('serverless'); + const dataPathBuilder = new EsArchivePathBuilder(isServerless); + const path = dataPathBuilder.getPath('auditbeat/hosts'); const getImmutableRule = async () => { await installMockPrebuiltRules(supertest, es); return getRule(supertest, log, ELASTIC_SECURITY_RULE_ID); }; - describe('update_actions', () => { + describe('@serverless @ess update_actions', () => { describe('updating actions', () => { before(async () => { - await esArchiver.load('x-pack/test/functional/es_archives/auditbeat/hosts'); + await esArchiver.load(path); }); after(async () => { - await esArchiver.unload('x-pack/test/functional/es_archives/auditbeat/hosts'); + await esArchiver.unload(path); }); beforeEach(async () => { - await createSignalsIndex(supertest, log); + await createAlertsIndex(supertest, log); }); afterEach(async () => { @@ -70,14 +77,16 @@ export default ({ getService }: FtrProviderContext) => { const updatedRule = await updateRule(supertest, log, ruleToUpdate); const bodyToCompare = removeServerGeneratedProperties(updatedRule); - const expected = { + const expectedRule = { ...getSimpleRuleOutputWithWebHookAction( `${bodyToCompare.actions?.[0].id}`, `${bodyToCompare.actions?.[0].uuid}` ), revision: 1, // revision bump is required since this is an updated rule and this is part of the testing that we do bump the revision number on update }; - expect(bodyToCompare).to.eql(expected); + const expectedRuleWithUserUpdated = updateUsername(expectedRule, ELASTICSEARCH_USERNAME); + + expect(bodyToCompare).to.eql(expectedRuleWithUserUpdated); }); it('should be able to add a new webhook action and then remove the action from the rule again', async () => { @@ -92,7 +101,8 @@ export default ({ getService }: FtrProviderContext) => { ...getSimpleRuleOutput(), revision: 2, // revision bump is required since this is an updated rule and this is part of the testing that we do bump the revision number on update }; - expect(bodyToCompare).to.eql(expected); + const expectedRuleWithUserUpdated = updateUsername(expected, ELASTICSEARCH_USERNAME); + expect(bodyToCompare).to.eql(expectedRuleWithUserUpdated); }); it('should be able to create a new webhook action and attach it to a rule without a meta field and run it correctly', async () => { @@ -104,7 +114,7 @@ export default ({ getService }: FtrProviderContext) => { await waitForRuleSuccess({ supertest, log, id: updatedRule.id }); }); - it('should be able to create a new webhook action and attach it to a rule with a meta field and run it correctly', async () => { + it('@skipInQA should be able to create a new webhook action and attach it to a rule with a meta field and run it correctly', async () => { const hookAction = await createNewAction(supertest, log); const rule = getSimpleRule(); await createRule(supertest, log, rule); @@ -116,7 +126,7 @@ export default ({ getService }: FtrProviderContext) => { await waitForRuleSuccess({ supertest, log, id: updatedRule.id }); }); - it('should not change properties of immutable rule when applying actions to it', async () => { + it('@skipInQA should not change properties of immutable rule when applying actions to it', async () => { // actions and throttle to be removed from assertion (it asserted in a separate test case) const actionsProps = ['actions', 'throttle']; @@ -139,7 +149,7 @@ export default ({ getService }: FtrProviderContext) => { expect(expected.immutable).to.be(true); // It should stay immutable true when returning }); - it('should be able to create a new webhook action and attach it to an immutable rule', async () => { + it('@skipInQA should be able to create a new webhook action and attach it to an immutable rule', async () => { const immutableRule = await getImmutableRule(); const hookAction = await createNewAction(supertest, log); const ruleToUpdate = getRuleWithWebHookAction( @@ -155,11 +165,12 @@ export default ({ getService }: FtrProviderContext) => { `${bodyToCompare.actions?.[0].uuid}` ); - expect(bodyToCompare.actions).to.eql(expected.actions); - expect(bodyToCompare.throttle).to.eql(expected.throttle); + const expectedRuleWithUserUpdated = updateUsername(expected, ELASTICSEARCH_USERNAME); + expect(bodyToCompare.actions).to.eql(expectedRuleWithUserUpdated.actions); + expect(bodyToCompare.throttle).to.eql(expectedRuleWithUserUpdated.throttle); }); - it('should be able to create a new webhook action, attach it to an immutable rule and the count of prepackaged rules should not increase. If this fails, suspect the immutable tags are not staying on the rule correctly.', async () => { + it('@skipInQA should be able to create a new webhook action, attach it to an immutable rule and the count of prepackaged rules should not increase. If this fails, suspect the immutable tags are not staying on the rule correctly.', async () => { const immutableRule = await getImmutableRule(); const hookAction = await createNewAction(supertest, log); const ruleToUpdate = getRuleWithWebHookAction( @@ -173,7 +184,7 @@ export default ({ getService }: FtrProviderContext) => { expect(status.rules_not_installed).to.eql(0); }); - it('should be able to create a new webhook action, attach it to an immutable rule and the rule should stay immutable when searching against immutable tags', async () => { + it('@skipInQA should be able to create a new webhook action, attach it to an immutable rule and the rule should stay immutable when searching against immutable tags', async () => { const immutableRule = await getImmutableRule(); const hookAction = await createNewAction(supertest, log); const ruleToUpdate = getRuleWithWebHookAction( @@ -190,8 +201,8 @@ export default ({ getService }: FtrProviderContext) => { `${bodyToCompare.actions?.[0].id}`, `${bodyToCompare.actions?.[0].uuid}` ); - - expect(bodyToCompare.actions).to.eql(expected.actions); + const expectedRuleWithUserUpdated = updateUsername(expected, ELASTICSEARCH_USERNAME); + expect(bodyToCompare.actions).to.eql(expectedRuleWithUserUpdated.actions); expect(bodyToCompare.immutable).to.be(true); }); }); diff --git a/x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/actions/create_new_action.ts b/x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/actions/create_new_action.ts new file mode 100644 index 0000000000000..7cbe8e858a043 --- /dev/null +++ b/x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/actions/create_new_action.ts @@ -0,0 +1,35 @@ +/* + * 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 type { ToolingLog } from '@kbn/tooling-log'; +import type SuperTest from 'supertest'; + +import { getWebHookAction } from './get_web_hook_action'; + +/** + * Helper to cut down on the noise in some of the tests. This + * creates a new action and expects a 200 and does not do any retries. + * @param supertest The supertest deps + */ +export const createNewAction = async ( + supertest: SuperTest.SuperTest, + log: ToolingLog +) => { + const response = await supertest + .post('/api/actions/action') + .set('kbn-xsrf', 'true') + .set('x-elastic-internal-origin', 'foo') + .send(getWebHookAction()); + if (response.status !== 200) { + log.error( + `Did not get an expected 200 "ok" when creating a new action. CI issues could happen. Suspect this line if you are seeing CI issues. body: ${JSON.stringify( + response.body + )}, status: ${JSON.stringify(response.status)}` + ); + } + return response.body; +}; diff --git a/x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/actions/index.ts b/x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/actions/index.ts index 438d983a69e05..d9b65ba596dd6 100644 --- a/x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/actions/index.ts +++ b/x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/actions/index.ts @@ -7,3 +7,4 @@ export * from './get_slack_action'; export * from './get_web_hook_action'; export * from './remove_uuid_from_actions'; +export * from './create_new_action'; diff --git a/x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/rules/get_rule_with_web_hook_action.ts b/x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/rules/get_rule_with_web_hook_action.ts new file mode 100644 index 0000000000000..6437df274098d --- /dev/null +++ b/x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/rules/get_rule_with_web_hook_action.ts @@ -0,0 +1,34 @@ +/* + * 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 type { + RuleCreateProps, + RuleUpdateProps, +} from '@kbn/security-solution-plugin/common/api/detection_engine'; +import { getSimpleRule } from './get_simple_rule'; + +export const getRuleWithWebHookAction = ( + id: string, + enabled = false, + rule?: RuleCreateProps +): RuleCreateProps | RuleUpdateProps => { + const finalRule = rule != null ? { ...rule, enabled } : getSimpleRule('rule-1', enabled); + return { + ...finalRule, + throttle: 'rule', + actions: [ + { + group: 'default', + id, + params: { + body: '{}', + }, + action_type_id: '.webhook', + }, + ], + }; +}; diff --git a/x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/rules/get_simple_rule_output_with_web_hook_action.ts b/x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/rules/get_simple_rule_output_with_web_hook_action.ts new file mode 100644 index 0000000000000..7ecee679e50b3 --- /dev/null +++ b/x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/rules/get_simple_rule_output_with_web_hook_action.ts @@ -0,0 +1,29 @@ +/* + * 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 { NOTIFICATION_DEFAULT_FREQUENCY } from '@kbn/security-solution-plugin/common/constants'; +import { getSimpleRuleOutput } from './get_simple_rule_output'; +import { RuleWithoutServerGeneratedProperties } from './remove_server_generated_properties'; + +export const getSimpleRuleOutputWithWebHookAction = ( + actionId: string, + uuid: string +): RuleWithoutServerGeneratedProperties => ({ + ...getSimpleRuleOutput(), + actions: [ + { + action_type_id: '.webhook', + group: 'default', + id: actionId, + params: { + body: '{}', + }, + uuid, + frequency: NOTIFICATION_DEFAULT_FREQUENCY, + }, + ], +}); diff --git a/x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/rules/index.ts b/x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/rules/index.ts index ba91dea27743e..0170faa8ceeda 100644 --- a/x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/rules/index.ts +++ b/x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/rules/index.ts @@ -26,5 +26,9 @@ export * from './find_immutable_rule_by_id'; export * from './create_rule_with_exception_entries'; export * from './downgrade_immutable_rule'; export * from './get_eql_rule_for_alert_testing'; +export * from './get_rule_with_web_hook_action'; +export * from './get_simple_rule_output_with_web_hook_action'; +export * from './rule_to_update_schema'; +export * from './update_rule'; export * from './prebuilt_rules'; diff --git a/x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/rules/rule_to_update_schema.ts b/x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/rules/rule_to_update_schema.ts new file mode 100644 index 0000000000000..f6669a1325eb1 --- /dev/null +++ b/x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/rules/rule_to_update_schema.ts @@ -0,0 +1,37 @@ +/* + * 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 type { + RuleResponse, + RuleUpdateProps, +} from '@kbn/security-solution-plugin/common/api/detection_engine'; +import { omit, pickBy } from 'lodash'; + +const propertiesToRemove = [ + 'id', + 'immutable', + 'updated_at', + 'updated_by', + 'created_at', + 'created_by', + 'related_integrations', + 'required_fields', + 'revision', + 'setup', + 'execution_summary', +]; + +/** + * transforms RuleResponse rule to RuleUpdateProps + * returned result can be used in rule update API calls + */ +export const ruleToUpdateSchema = (rule: RuleResponse): RuleUpdateProps => { + const removedProperties = omit(rule, propertiesToRemove); + + // We're only removing undefined values, so this cast correctly narrows the type + return pickBy(removedProperties, (value) => value !== undefined) as RuleUpdateProps; +}; diff --git a/x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/rules/update_rule.ts b/x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/rules/update_rule.ts new file mode 100644 index 0000000000000..53c1beb272764 --- /dev/null +++ b/x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/rules/update_rule.ts @@ -0,0 +1,41 @@ +/* + * 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 type { ToolingLog } from '@kbn/tooling-log'; +import type SuperTest from 'supertest'; + +import { DETECTION_ENGINE_RULES_URL } from '@kbn/security-solution-plugin/common/constants'; +import { + RuleUpdateProps, + RuleResponse, +} from '@kbn/security-solution-plugin/common/api/detection_engine'; + +/** + * Helper to cut down on the noise in some of the tests. This checks for + * an expected 200 still and does not do any retries. + * @param supertest The supertest deps + * @param rule The rule to create + */ +export const updateRule = async ( + supertest: SuperTest.SuperTest, + log: ToolingLog, + updatedRule: RuleUpdateProps +): Promise => { + const response = await supertest + .put(DETECTION_ENGINE_RULES_URL) + .set('kbn-xsrf', 'true') + .set('elastic-api-version', '2023-10-31') + .send(updatedRule); + if (response.status !== 200) { + log.error( + `Did not get an expected 200 "ok" when updating a rule (updateRule). CI issues could happen. Suspect this line if you are seeing CI issues. body: ${JSON.stringify( + response.body + )}, status: ${JSON.stringify(response.status)}` + ); + } + return response.body; +}; From 4493efa9a9d12551dcd75281fcb4a05d9fe2d571 Mon Sep 17 00:00:00 2001 From: Coen Warmer Date: Mon, 30 Oct 2023 18:42:04 +0100 Subject: [PATCH 02/10] i18n Translation ESLint Rule Fixes And Improvements (#169955) --- packages/kbn-eslint-plugin-i18n/README.mdx | 12 + .../helpers/get_i18n_import_fixer.ts | 101 +++++-- .../helpers/get_intent_from_node.ts | 24 +- .../helpers/utils.test.ts | 55 ++++ .../kbn-eslint-plugin-i18n/helpers/utils.ts | 14 + ..._translated_with_formatted_message.test.ts | 256 +++++++++++----- ...ld_be_translated_with_formatted_message.ts | 58 ++-- ...ngs_should_be_translated_with_i18n.test.ts | 284 ++++++++++++------ .../strings_should_be_translated_with_i18n.ts | 60 ++-- 9 files changed, 615 insertions(+), 249 deletions(-) create mode 100644 packages/kbn-eslint-plugin-i18n/helpers/utils.test.ts diff --git a/packages/kbn-eslint-plugin-i18n/README.mdx b/packages/kbn-eslint-plugin-i18n/README.mdx index 100f83d167b6e..174457477e81a 100644 --- a/packages/kbn-eslint-plugin-i18n/README.mdx +++ b/packages/kbn-eslint-plugin-i18n/README.mdx @@ -20,3 +20,15 @@ It kicks in on JSXText elements and specific JSXAttributes (`label` and `aria-la This rule warns engineers to translate their strings by using `` from the '@kbn/i18n-react' package. It provides an autofix that takes into account the context of the translatable string in the JSX tree and to generate a translation ID. It kicks in on JSXText elements and specific JSXAttributes (`label` and `aria-label`) which expect a translated value. + +## Exemptions and exceptions + +A JSXText element or JSXAttribute `label` or `aria-label` of which the value is: + +- wrapped in a `EuiCode` or `EuiBetaBadge` component, +- made up of non alpha characters such as `!@#$%^&*(){}` or numbers, +- wrapped in three backticks, + +are exempt from this rule. + +If this rule kicks in on a string value that you don't like, you can escape it by wrapping the string inside a JSXExpression: `{'my escaped value'}`. diff --git a/packages/kbn-eslint-plugin-i18n/helpers/get_i18n_import_fixer.ts b/packages/kbn-eslint-plugin-i18n/helpers/get_i18n_import_fixer.ts index f26bc62c555f0..cf3a7330f7584 100644 --- a/packages/kbn-eslint-plugin-i18n/helpers/get_i18n_import_fixer.ts +++ b/packages/kbn-eslint-plugin-i18n/helpers/get_i18n_import_fixer.ts @@ -8,9 +8,6 @@ import { SourceCode } from 'eslint'; -const KBN_I18N_I18N_IMPORT = "import { i18n } from '@kbn/i18n';" as const; -const KBN_I18N_REACT_FORMATTED_MESSAGE_IMPORT = - "import { FormattedMessage } from '@kbn/i18n-react';" as const; export function getI18nImportFixer({ sourceCode, mode, @@ -18,33 +15,89 @@ export function getI18nImportFixer({ sourceCode: SourceCode; mode: 'i18n.translate' | 'FormattedMessage'; }) { - const hasI18nImportLine = Boolean( - sourceCode.lines.find((l) => - mode === 'i18n.translate' - ? l === KBN_I18N_I18N_IMPORT - : l === KBN_I18N_REACT_FORMATTED_MESSAGE_IMPORT - ) - ); + let existingI18nImportLineIndex = -1; + let i18nImportLineToBeAdded = ''; - if (hasI18nImportLine) return { hasI18nImportLine }; + /** + * + * Searching through sourcecode to see if there is already an import of i18n package, + * and check if it includes the module we need. + * + * If any of these conditions are not met, we prepare the import line we need to add. + * + * */ - // Translation package has not been imported yet so we need to add it. - // Pretty safe bet to add it underneath the React import. - const reactImportLineIndex = sourceCode.lines.findIndex((l) => l.includes("from 'react'")); + if (mode === 'i18n.translate') { + existingI18nImportLineIndex = sourceCode.lines.findIndex((l) => l.includes("from '@kbn/i18n'")); - const targetLine = sourceCode.lines[reactImportLineIndex]; - const column = targetLine.length; + const i18nImportLineInSource = sourceCode.lines[existingI18nImportLineIndex]; - const start = sourceCode.getIndexFromLoc({ line: reactImportLineIndex + 1, column: 0 }); - const end = sourceCode.getIndexFromLoc({ - line: reactImportLineIndex + 1, - column, - }); + if (i18nImportLineInSource) { + const modules = i18nImportLineInSource.split('{')[1].split('}')[0].trim(); + + if (modules.split(',').includes('i18n')) { + return { hasI18nImportLine: true }; + } + + // Existing import is something like: `import { SomeOtherModule } from '@kbn/i18n';` + i18nImportLineToBeAdded = `import { ${modules}, i18n } from '@kbn/i18n';`; + } else { + i18nImportLineToBeAdded = `import { i18n } from '@kbn/i18n';`; + } + } + + if (mode === 'FormattedMessage') { + existingI18nImportLineIndex = sourceCode.lines.findIndex((l) => + l.includes("from '@kbn/i18n-react'") + ); + const i18nImportLineInSource = sourceCode.lines[existingI18nImportLineIndex]; + + if (i18nImportLineInSource) { + const modules = i18nImportLineInSource.split('{')[1].split('}')[0].trim(); + + if (modules.split(',').includes('FormattedMessage')) { + return { hasI18nImportLine: true }; + } + + // Existing import is something like: `import { SomeOtherModule } from '@kbn/i18n-react';` + i18nImportLineToBeAdded = `import { ${modules}, FormattedMessage } from '@kbn/i18n-react';`; + } else { + i18nImportLineToBeAdded = `import { FormattedMessage } from '@kbn/i18n-react';`; + } + } + + /** + * + * Determining where in the source code to add the import line. + * + * */ + + // If the file already has an import line for the translation package but it doesn't include the module we need, we need to add it. + if (existingI18nImportLineIndex > -1) { + const targetLine = sourceCode.lines[existingI18nImportLineIndex]; + const column = targetLine.length; + + const start = sourceCode.getIndexFromLoc({ line: existingI18nImportLineIndex + 1, column: 0 }); + const end = start + column; + + return { + i18nImportLine: i18nImportLineToBeAdded, + rangeToAddI18nImportLine: [start, end] as [number, number], + mode: 'replace', + }; + } + + // If the file doesn't have an import line for the translation package yet, we need to add it. + // Pretty safe bet to add it underneath the import line for React. + const lineIndex = sourceCode.lines.findIndex((l) => l.includes("from 'react'")); + const targetLine = sourceCode.lines[lineIndex]; + + const start = sourceCode.getIndexFromLoc({ line: lineIndex + 1, column: 0 }); + const end = start + targetLine.length; return { - hasI18nImportLine, - i18nPackageImportLine: - mode === 'i18n.translate' ? KBN_I18N_I18N_IMPORT : KBN_I18N_REACT_FORMATTED_MESSAGE_IMPORT, + i18nImportLine: i18nImportLineToBeAdded, rangeToAddI18nImportLine: [start, end] as [number, number], + mode: 'insert', }; } diff --git a/packages/kbn-eslint-plugin-i18n/helpers/get_intent_from_node.ts b/packages/kbn-eslint-plugin-i18n/helpers/get_intent_from_node.ts index 687bfd31cfba2..d27f2407ce64c 100644 --- a/packages/kbn-eslint-plugin-i18n/helpers/get_intent_from_node.ts +++ b/packages/kbn-eslint-plugin-i18n/helpers/get_intent_from_node.ts @@ -6,15 +6,18 @@ * Side Public License, v 1. */ import { AST_NODE_TYPES, TSESTree } from '@typescript-eslint/typescript-estree'; -import { lowerCaseFirstLetter, upperCaseFirstLetter } from './utils'; +import { cleanString, lowerCaseFirstLetter, upperCaseFirstLetter } from './utils'; -export function getIntentFromNode(value: string, parent: TSESTree.Node | undefined): string { +const EXEMPTED_TAG_NAMES = ['EuiCode', 'EuiBetaBadge', 'FormattedMessage']; + +export function getIntentFromNode( + value: string, + parent: TSESTree.Node | undefined +): string | false { const processedValue = lowerCaseFirstLetter( - value - .replace(/[?!@#$%^&*()_+\][{}|/<>,'"]/g, '') - .trim() + cleanString(value) .split(' ') - .filter((v, i) => i < 4) + .filter((_, i) => i < 4) .map(upperCaseFirstLetter) .join('') ); @@ -27,6 +30,11 @@ export function getIntentFromNode(value: string, parent: TSESTree.Node | undefin ) { const parentTagName = String(parent.openingElement.name.name); + // Exceptions + if (EXEMPTED_TAG_NAMES.includes(parentTagName)) { + return false; + } + if (parentTagName.includes('Eui')) { return `${processedValue}${parentTagName.replace('Eui', '')}Label`; } @@ -45,6 +53,10 @@ export function getIntentFromNode(value: string, parent: TSESTree.Node | undefin ) { const parentTagName = String(parent.parent.name.name); + if (EXEMPTED_TAG_NAMES.includes(parentTagName)) { + return false; + } + return `${lowerCaseFirstLetter(parentTagName)}.${processedValue}Label`; } diff --git a/packages/kbn-eslint-plugin-i18n/helpers/utils.test.ts b/packages/kbn-eslint-plugin-i18n/helpers/utils.test.ts new file mode 100644 index 0000000000000..9d36c7a7e1e9c --- /dev/null +++ b/packages/kbn-eslint-plugin-i18n/helpers/utils.test.ts @@ -0,0 +1,55 @@ +/* + * 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 { cleanString, lowerCaseFirstLetter } from './utils'; + +describe('Utils', () => { + describe('lowerCaseFirstLetter', () => { + it('should lowercase the first letter', () => { + expect(lowerCaseFirstLetter('Hello')).toBe('hello'); + expect(lowerCaseFirstLetter('GreatSuccessYes')).toBe('greatSuccessYes'); + expect(lowerCaseFirstLetter('How is it going?')).toBe('how is it going?'); + }); + + it('should lowercase all letters if the passed string is in ALL CAPS', () => { + expect(lowerCaseFirstLetter('HELLO')).toBe('hello'); + expect(lowerCaseFirstLetter('GREATSUCCESSYES')).toBe('greatsuccessyes'); + }); + }); + + describe('cleanString', () => { + it('should remove all numbers', () => { + expect(cleanString('123')).toBe(''); + }); + + it('should remove all white spaces from beginning and end', () => { + expect(cleanString(' abc ')).toBe('abc'); + expect(cleanString(' This is a test ')).toBe('This is a test'); + expect( + cleanString(` + + + hello + + + + great!`) + ).toBe('hello great'); + }); + + it('should remove all non alphabet characters', () => { + expect(cleanString('abc123!@#')).toBe('abc'); + expect(cleanString('!@#$%^&*()_+{}|')).toBe(''); + expect(cleanString(' Hey, this is great! Success. ')).toBe('Hey this is great Success'); + }); + + it('should leave markdown alone', () => { + expect(cleanString('```hello```')).toBe(''); + }); + }); +}); diff --git a/packages/kbn-eslint-plugin-i18n/helpers/utils.ts b/packages/kbn-eslint-plugin-i18n/helpers/utils.ts index 74ce8aa7c5c55..bfa0ceaf2aecc 100644 --- a/packages/kbn-eslint-plugin-i18n/helpers/utils.ts +++ b/packages/kbn-eslint-plugin-i18n/helpers/utils.ts @@ -7,6 +7,8 @@ */ export function lowerCaseFirstLetter(str: string) { + if (isUpperCase(str)) return str.toLowerCase(); + return str.charAt(0).toLowerCase() + str.slice(1); } @@ -17,3 +19,15 @@ export function upperCaseFirstLetter(str: string) { export function isTruthy(value: T): value is NonNullable { return value != null; } + +function isUpperCase(val: string) { + return /^[A-Z]+$/.test(val); +} + +export function cleanString(str: string) { + return str + .replace(/```\w*```/g, '') + .replace(/\s+/g, ' ') + .replace(/[^a-zA-Z\s]*/g, '') + .trim(); +} diff --git a/packages/kbn-eslint-plugin-i18n/rules/strings_should_be_translated_with_formatted_message.test.ts b/packages/kbn-eslint-plugin-i18n/rules/strings_should_be_translated_with_formatted_message.test.ts index 10bdbda351892..009fac255fc63 100644 --- a/packages/kbn-eslint-plugin-i18n/rules/strings_should_be_translated_with_formatted_message.test.ts +++ b/packages/kbn-eslint-plugin-i18n/rules/strings_should_be_translated_with_formatted_message.test.ts @@ -38,165 +38,275 @@ const babelTester = [ }), ] as const; -const valid = [ +const invalid: RuleTester.InvalidTestCase[] = [ { + name: 'A JSX element with a string literal should be translated with i18n', filename: 'x-pack/plugins/observability/public/test_component.tsx', code: ` import React from 'react'; + +function TestComponent() { + return ( +
This is a test
+ ) +}`, + errors: [ + { + line: 6, + message: `Strings should be translated with . Use the autofix suggestion or add your own.`, + }, + ], + output: ` +import React from 'react'; import { FormattedMessage } from '@kbn/i18n-react'; function TestComponent() { - return ( -
+ return ( +
- ) + ) }`, }, { + name: 'A JSX element with a string literal that are inside an Eui component should take the component name of the parent into account', filename: 'x-pack/plugins/observability/public/another_component.tsx', code: ` import React from 'react'; + +function AnotherComponent() { + return ( + + + + This is a test + + + + ) +}`, + errors: [ + { + line: 9, + message: `Strings should be translated with . Use the autofix suggestion or add your own.`, + }, + ], + output: ` +import React from 'react'; import { FormattedMessage } from '@kbn/i18n-react'; function AnotherComponent() { - return ( - - - - + return ( + + + + - - - - ) + + + + ) }`, }, { + name: 'When no import of the translation module is present, the import line should be added', filename: 'x-pack/plugins/observability/public/yet_another_component.tsx', code: ` import React from 'react'; + +function YetAnotherComponent() { + return ( +
+ Select me +
+ ) +}`, + errors: [ + { + line: 7, + message: `Strings should be translated with . Use the autofix suggestion or add your own.`, + }, + ], + output: ` +import React from 'react'; import { FormattedMessage } from '@kbn/i18n-react'; function YetAnotherComponent() { - return ( -
- + return ( +
+ -
- ) +
+ ) }`, }, { + name: 'Import lines without the necessary translation module should be updated to include i18n', filename: 'x-pack/plugins/observability/public/test_component.tsx', code: ` import React from 'react'; -import { FormattedMessage } from '@kbn/i18n-react'; +import { SomeOtherModule } from '@kbn/i18n-react'; function TestComponent() { - return ( - } /> - ) - }`, - }, -]; - -const invalid = [ - { - filename: valid[0].filename, - code: ` -import React from 'react'; - -function TestComponent() { - return ( -
This is a test
- ) + return ( + + ) }`, errors: [ { - line: 6, + line: 7, message: `Strings should be translated with . Use the autofix suggestion or add your own.`, }, ], - output: valid[0].code, + output: ` +import React from 'react'; +import { SomeOtherModule, FormattedMessage } from '@kbn/i18n-react'; + +function TestComponent() { + return ( + } /> + ) +}`, }, { - filename: valid[1].filename, + name: 'JSX elements that have a label or aria-label prop with a string value should be translated with i18n', + filename: 'x-pack/plugins/observability/public/test_component.tsx', code: ` import React from 'react'; +import { FormattedMessage } from '@kbn/i18n-react'; -function AnotherComponent() { - return ( - - - - This is a test - - - - ) +function TestComponent() { + return ( + + ) }`, errors: [ { - line: 9, + line: 7, message: `Strings should be translated with . Use the autofix suggestion or add your own.`, }, ], - output: valid[1].code, + output: ` +import React from 'react'; +import { FormattedMessage } from '@kbn/i18n-react'; + +function TestComponent() { + return ( + } /> + ) +}`, }, { - filename: valid[2].filename, + name: 'JSX elements that have a label or aria-label prop with a JSXExpression value that is a string should be translated with i18n', + filename: 'x-pack/plugins/observability/public/test_component.tsx', code: ` -import React from 'react'; + import React from 'react'; + import { FormattedMessage } from '@kbn/i18n-react'; -function YetAnotherComponent() { + function TestComponent() { return ( -
- Select me -
+ ) -}`, + }`, errors: [ { line: 7, message: `Strings should be translated with . Use the autofix suggestion or add your own.`, }, ], - output: valid[2].code, + output: ` + import React from 'react'; + import { FormattedMessage } from '@kbn/i18n-react'; + + function TestComponent() { + return ( + } /> + ) + }`, }, +]; + +const valid: RuleTester.ValidTestCase[] = [ { - filename: valid[3].filename, + name: 'A JSXText element inside a EuiCode component should not be translated', + filename: 'x-pack/plugins/observability/public/test_component.tsx', code: ` import React from 'react'; -import { FormattedMessage } from '@kbn/i18n-react'; function TestComponent() { - return ( - - ) - }`, - errors: [ - { - line: 7, - message: `Strings should be translated with . Use the autofix suggestion or add your own.`, - }, - ], - output: valid[3].code, + return ( + This is a test + ) +}`, + }, + { + name: 'A JSXText element that contains anything other than alpha characters should not be translated', + filename: 'x-pack/plugins/observability/public/test_component.tsx', + code: ` +import React from 'react'; + +function TestComponent() { + return ( +
!@#$%^&*()_+{}123 456 789
+ ) +}`, + }, + { + name: 'A JSXText element that is wrapped in three backticks (markdown) should not be translated', + filename: 'x-pack/plugins/observability/public/test_component.tsx', + code: ` +import React from 'react'; + +function TestComponent() { + return ( +
\`\`\`hello\`\`\`
+ ) +}`, + }, + { + name: invalid[0].name, + filename: invalid[0].filename, + code: invalid[0].output as string, + }, + { + name: invalid[1].name, + filename: invalid[1].filename, + code: invalid[1].output as string, + }, + { + name: invalid[2].name, + filename: invalid[2].filename, + code: invalid[2].output as string, + }, + { + name: invalid[3].name, + filename: invalid[3].filename, + code: invalid[3].output as string, + }, + { + name: invalid[4].name, + filename: invalid[4].filename, + code: invalid[4].output as string, + }, + { + name: invalid[5].name, + filename: invalid[5].filename, + code: invalid[5].output as string, }, ]; for (const [name, tester] of [tsTester, babelTester]) { describe(name, () => { tester.run( - '@kbn/event_generating_elements_should_be_instrumented', + '@kbn/strings_should_be_translated_with_formatted_message', StringsShouldBeTranslatedWithFormattedMessage, { valid, diff --git a/packages/kbn-eslint-plugin-i18n/rules/strings_should_be_translated_with_formatted_message.ts b/packages/kbn-eslint-plugin-i18n/rules/strings_should_be_translated_with_formatted_message.ts index 67e2aaec256d7..77b5918951036 100644 --- a/packages/kbn-eslint-plugin-i18n/rules/strings_should_be_translated_with_formatted_message.ts +++ b/packages/kbn-eslint-plugin-i18n/rules/strings_should_be_translated_with_formatted_message.ts @@ -6,13 +6,13 @@ * Side Public License, v 1. */ -import { TSESTree } from '@typescript-eslint/typescript-estree'; +import type { TSESTree } from '@typescript-eslint/typescript-estree'; import type { Rule } from 'eslint'; import { getIntentFromNode } from '../helpers/get_intent_from_node'; import { getI18nIdentifierFromFilePath } from '../helpers/get_i18n_identifier_from_file_path'; import { getFunctionName } from '../helpers/get_function_name'; import { getI18nImportFixer } from '../helpers/get_i18n_import_fixer'; -import { isTruthy } from '../helpers/utils'; +import { cleanString, isTruthy } from '../helpers/utils'; export const StringsShouldBeTranslatedWithFormattedMessage: Rule.RuleModule = { meta: { @@ -24,7 +24,7 @@ export const StringsShouldBeTranslatedWithFormattedMessage: Rule.RuleModule = { return { JSXText: (node: TSESTree.JSXText) => { - const value = node.value.trim(); + const value = cleanString(node.value); // If the JSXText element is empty we don't need to do anything if (!value) return; @@ -34,22 +34,21 @@ export const StringsShouldBeTranslatedWithFormattedMessage: Rule.RuleModule = { const whiteSpaces = node.value.match(regex)?.[1] ?? ''; // Start building the translation ID suggestion + const intent = getIntentFromNode(value, node.parent); + if (intent === false) return; + const i18nAppId = getI18nIdentifierFromFilePath(filename, cwd); const functionDeclaration = getScope().block as TSESTree.FunctionDeclaration; const functionName = getFunctionName(functionDeclaration); - const intent = getIntentFromNode(value, node.parent); const translationIdSuggestion = `${i18nAppId}.${functionName}.${intent}`; // 'xpack.observability.overview.logs.loadMoreLabel' - // Check if i18n has already been imported into the file. - const { - hasI18nImportLine, - i18nPackageImportLine: i18nImportLine, - rangeToAddI18nImportLine, - } = getI18nImportFixer({ - sourceCode, - mode: 'FormattedMessage', - }); + // Check if i18n has already been imported into the file + const { hasI18nImportLine, i18nImportLine, rangeToAddI18nImportLine, mode } = + getI18nImportFixer({ + sourceCode, + mode: 'FormattedMessage', + }); // Show warning to developer and offer autofix suggestion report({ @@ -65,8 +64,10 @@ export const StringsShouldBeTranslatedWithFormattedMessage: Rule.RuleModule = { defaultMessage="${value}" />` ), - !hasI18nImportLine - ? fixer.insertTextAfterRange(rangeToAddI18nImportLine, `\n${i18nImportLine}`) + !hasI18nImportLine && rangeToAddI18nImportLine + ? mode === 'replace' + ? fixer.replaceTextRange(rangeToAddI18nImportLine, i18nImportLine) + : fixer.insertTextAfterRange(rangeToAddI18nImportLine, `\n${i18nImportLine}`) : null, ].filter(isTruthy); }, @@ -84,33 +85,32 @@ export const StringsShouldBeTranslatedWithFormattedMessage: Rule.RuleModule = { 'value' in node.value.expression && typeof node.value.expression.value === 'string' ) { - val = node.value.expression.value; + val = cleanString(node.value.expression.value); } // label="foo" if (node.value && 'value' in node.value && typeof node.value.value === 'string') { - val = node.value.value; + val = cleanString(node.value.value); } if (!val) return; // Start building the translation ID suggestion + const intent = getIntentFromNode(val, node); + if (intent === false) return; + const i18nAppId = getI18nIdentifierFromFilePath(filename, cwd); const functionDeclaration = getScope().block as TSESTree.FunctionDeclaration; const functionName = getFunctionName(functionDeclaration); - const intent = getIntentFromNode(val, node); const translationIdSuggestion = `${i18nAppId}.${functionName}.${intent}`; // 'xpack.observability.overview.logs.loadMoreLabel' // Check if i18n has already been imported into the file. - const { - hasI18nImportLine, - i18nPackageImportLine: i18nImportLine, - rangeToAddI18nImportLine, - } = getI18nImportFixer({ - sourceCode, - mode: 'FormattedMessage', - }); + const { hasI18nImportLine, i18nImportLine, rangeToAddI18nImportLine, mode } = + getI18nImportFixer({ + sourceCode, + mode: 'FormattedMessage', + }); // Show warning to developer and offer autofix suggestion report({ @@ -123,8 +123,10 @@ export const StringsShouldBeTranslatedWithFormattedMessage: Rule.RuleModule = { node.value!.range, `{}` ), - !hasI18nImportLine - ? fixer.insertTextAfterRange(rangeToAddI18nImportLine, `\n${i18nImportLine}`) + !hasI18nImportLine && rangeToAddI18nImportLine + ? mode === 'replace' + ? fixer.replaceTextRange(rangeToAddI18nImportLine, i18nImportLine) + : fixer.insertTextAfterRange(rangeToAddI18nImportLine, `\n${i18nImportLine}`) : null, ].filter(isTruthy); }, diff --git a/packages/kbn-eslint-plugin-i18n/rules/strings_should_be_translated_with_i18n.test.ts b/packages/kbn-eslint-plugin-i18n/rules/strings_should_be_translated_with_i18n.test.ts index dc938cd6effd3..f470ed885682f 100644 --- a/packages/kbn-eslint-plugin-i18n/rules/strings_should_be_translated_with_i18n.test.ts +++ b/packages/kbn-eslint-plugin-i18n/rules/strings_should_be_translated_with_i18n.test.ts @@ -38,158 +38,264 @@ const babelTester = [ }), ] as const; -const valid = [ +const invalid: RuleTester.InvalidTestCase[] = [ { + name: 'A JSX element with a string literal should be translated with i18n', filename: 'x-pack/plugins/observability/public/test_component.tsx', code: ` import React from 'react'; + +function TestComponent() { + return ( +
This is a test
+ ) +}`, + errors: [ + { + line: 6, + message: `Strings should be translated with i18n. Use the autofix suggestion or add your own.`, + }, + ], + output: ` +import React from 'react'; import { i18n } from '@kbn/i18n'; function TestComponent() { - return ( -
{i18n.translate('app_not_found_in_i18nrc.testComponent.div.thisIsATestLabel', { defaultMessage: 'This is a test'})}
- ) - }`, + return ( +
{i18n.translate('app_not_found_in_i18nrc.testComponent.div.thisIsATestLabel', { defaultMessage: 'This is a test' })}
+ ) +}`, }, { + name: 'A JSX element with a string literal that are inside an Eui component should take the component name of the parent into account', filename: 'x-pack/plugins/observability/public/another_component.tsx', code: ` import React from 'react'; -import { i18n } from '@kbn/i18n'; function AnotherComponent() { - return ( - - - - {i18n.translate('app_not_found_in_i18nrc.anotherComponent.thisIsATestButtonLabel', { defaultMessage: 'This is a test'})} - - - - ) - }`, - }, - { - filename: 'x-pack/plugins/observability/public/yet_another_component.tsx', - code: ` - import React from 'react'; -import { i18n } from '@kbn/i18n'; - - function YetAnotherComponent() { - return ( -
- {i18n.translate('app_not_found_in_i18nrc.yetAnotherComponent.selectMeSelectLabel', { defaultMessage: 'Select me'})} -
- ) - }`, - }, - { - filename: 'x-pack/plugins/observability/public/test_component.tsx', - code: ` + return ( + + + + This is a test + + + + ) +}`, + errors: [ + { + line: 9, + message: `Strings should be translated with i18n. Use the autofix suggestion or add your own.`, + }, + ], + output: ` import React from 'react'; import { i18n } from '@kbn/i18n'; -function TestComponent() { - return ( - - ) - }`, +function AnotherComponent() { + return ( + + + + {i18n.translate('app_not_found_in_i18nrc.anotherComponent.thisIsATestButtonLabel', { defaultMessage: 'This is a test' })} + + + + ) +}`, }, -]; - -const invalid = [ { - filename: valid[0].filename, + name: 'When no import of the translation module is present, the import line should be added', + filename: 'x-pack/plugins/observability/public/yet_another_component.tsx', code: ` import React from 'react'; -function TestComponent() { - return ( -
This is a test
- ) - }`, +function YetAnotherComponent() { + return ( +
+ Select me +
+ ) +}`, errors: [ { - line: 6, + line: 7, message: `Strings should be translated with i18n. Use the autofix suggestion or add your own.`, }, ], - output: valid[0].code, + output: ` +import React from 'react'; +import { i18n } from '@kbn/i18n'; + +function YetAnotherComponent() { + return ( +
+ {i18n.translate('app_not_found_in_i18nrc.yetAnotherComponent.selectMeSelectLabel', { defaultMessage: 'Select me' })} +
+ ) +}`, }, { - filename: valid[1].filename, + name: 'Import lines without the necessary translation module should be updated to include i18n', + filename: 'x-pack/plugins/observability/public/test_component.tsx', code: ` import React from 'react'; +import { SomeOtherModule } from '@kbn/i18n'; -function AnotherComponent() { - return ( - - - - This is a test - - - - ) - }`, +function TestComponent() { + return ( + + ) +}`, errors: [ { - line: 9, + line: 7, message: `Strings should be translated with i18n. Use the autofix suggestion or add your own.`, }, ], - output: valid[1].code, + output: ` +import React from 'react'; +import { SomeOtherModule, i18n } from '@kbn/i18n'; + +function TestComponent() { + return ( + + ) +}`, }, { - filename: valid[2].filename, + name: 'JSX elements that have a label or aria-label prop with a string value should be translated with i18n', + filename: 'x-pack/plugins/observability/public/test_component.tsx', code: ` - import React from 'react'; - - function YetAnotherComponent() { - return ( -
- Select me -
- ) - }`, +import React from 'react'; +import { i18n } from '@kbn/i18n'; + +function TestComponent() { + return ( + + ) +}`, errors: [ { line: 7, message: `Strings should be translated with i18n. Use the autofix suggestion or add your own.`, }, ], - output: valid[2].code, + output: ` +import React from 'react'; +import { i18n } from '@kbn/i18n'; + +function TestComponent() { + return ( + + ) +}`, }, { - filename: valid[3].filename, + name: 'JSX elements that have a label or aria-label prop with a JSXExpression value that is a string should be translated with i18n', + filename: 'x-pack/plugins/observability/public/test_component.tsx', code: ` import React from 'react'; import { i18n } from '@kbn/i18n'; function TestComponent() { - return ( - - ) - }`, + return ( + + ) +}`, errors: [ { line: 7, message: `Strings should be translated with i18n. Use the autofix suggestion or add your own.`, }, ], - output: valid[3].code, + output: ` +import React from 'react'; +import { i18n } from '@kbn/i18n'; + +function TestComponent() { + return ( + + ) +}`, + }, +]; + +const valid: RuleTester.ValidTestCase[] = [ + { + name: 'A JSXText element inside a EuiCode component should not be translated', + filename: 'x-pack/plugins/observability/public/test_component.tsx', + code: ` +import React from 'react'; + +function TestComponent() { + return ( + This is a test + ) +}`, + }, + { + name: 'A JSXText element that contains anything other than alpha characters should not be translated', + filename: 'x-pack/plugins/observability/public/test_component.tsx', + code: ` +import React from 'react'; + +function TestComponent() { + return ( +
!@#$%^&*()_+{}123 456 789
+ ) +}`, + }, + { + name: 'A JSXText element that is wrapped in three backticks (markdown) should not be translated', + filename: 'x-pack/plugins/observability/public/test_component.tsx', + code: ` +import React from 'react'; + +function TestComponent() { + return ( +
\`\`\`hello\`\`\`
+ ) +}`, + }, + { + name: invalid[0].name, + filename: invalid[0].filename, + code: invalid[0].output as string, + }, + { + name: invalid[1].name, + filename: invalid[1].filename, + code: invalid[1].output as string, + }, + { + name: invalid[2].name, + filename: invalid[2].filename, + code: invalid[2].output as string, + }, + { + name: invalid[3].name, + filename: invalid[3].filename, + code: invalid[3].output as string, + }, + { + name: invalid[4].name, + filename: invalid[4].filename, + code: invalid[4].output as string, + }, + { + name: invalid[5].name, + filename: invalid[5].filename, + code: invalid[5].output as string, }, ]; for (const [name, tester] of [tsTester, babelTester]) { describe(name, () => { - tester.run( - '@kbn/event_generating_elements_should_be_instrumented', - StringsShouldBeTranslatedWithI18n, - { - valid, - invalid, - } - ); + tester.run('@kbn/strings_should_be_translated_with_i18n', StringsShouldBeTranslatedWithI18n, { + valid, + invalid, + }); }); } diff --git a/packages/kbn-eslint-plugin-i18n/rules/strings_should_be_translated_with_i18n.ts b/packages/kbn-eslint-plugin-i18n/rules/strings_should_be_translated_with_i18n.ts index ba31f6109075a..fea04d33d555f 100644 --- a/packages/kbn-eslint-plugin-i18n/rules/strings_should_be_translated_with_i18n.ts +++ b/packages/kbn-eslint-plugin-i18n/rules/strings_should_be_translated_with_i18n.ts @@ -6,13 +6,13 @@ * Side Public License, v 1. */ -import { TSESTree } from '@typescript-eslint/typescript-estree'; +import type { TSESTree } from '@typescript-eslint/typescript-estree'; import type { Rule } from 'eslint'; import { getIntentFromNode } from '../helpers/get_intent_from_node'; import { getI18nIdentifierFromFilePath } from '../helpers/get_i18n_identifier_from_file_path'; import { getFunctionName } from '../helpers/get_function_name'; import { getI18nImportFixer } from '../helpers/get_i18n_import_fixer'; -import { isTruthy } from '../helpers/utils'; +import { cleanString, isTruthy } from '../helpers/utils'; export const StringsShouldBeTranslatedWithI18n: Rule.RuleModule = { meta: { @@ -24,7 +24,7 @@ export const StringsShouldBeTranslatedWithI18n: Rule.RuleModule = { return { JSXText: (node: TSESTree.JSXText) => { - const value = node.value.trim(); + const value = cleanString(node.value); // If the JSXText element is empty we don't need to do anything if (!value) return; @@ -34,22 +34,21 @@ export const StringsShouldBeTranslatedWithI18n: Rule.RuleModule = { const whiteSpaces = node.value.match(regex)?.[1] ?? ''; // Start building the translation ID suggestion + const intent = getIntentFromNode(value, node.parent); + if (intent === false) return; + const i18nAppId = getI18nIdentifierFromFilePath(filename, cwd); const functionDeclaration = getScope().block as TSESTree.FunctionDeclaration; const functionName = getFunctionName(functionDeclaration); - const intent = getIntentFromNode(value, node.parent); const translationIdSuggestion = `${i18nAppId}.${functionName}.${intent}`; // 'xpack.observability.overview.logs.loadMoreLabel' // Check if i18n has already been imported into the file - const { - hasI18nImportLine, - i18nPackageImportLine: i18nImportLine, - rangeToAddI18nImportLine, - } = getI18nImportFixer({ - sourceCode, - mode: 'i18n.translate', - }); + const { hasI18nImportLine, i18nImportLine, rangeToAddI18nImportLine, mode } = + getI18nImportFixer({ + sourceCode, + mode: 'i18n.translate', + }); // Show warning to developer and offer autofix suggestion report({ @@ -60,10 +59,12 @@ export const StringsShouldBeTranslatedWithI18n: Rule.RuleModule = { return [ fixer.replaceText( node, - `${whiteSpaces}{i18n.translate('${translationIdSuggestion}', { defaultMessage: '${value}'})}` + `${whiteSpaces}{i18n.translate('${translationIdSuggestion}', { defaultMessage: '${value}' })}` ), - !hasI18nImportLine - ? fixer.insertTextAfterRange(rangeToAddI18nImportLine, `\n${i18nImportLine}`) + !hasI18nImportLine && rangeToAddI18nImportLine + ? mode === 'replace' + ? fixer.replaceTextRange(rangeToAddI18nImportLine, i18nImportLine) + : fixer.insertTextAfterRange(rangeToAddI18nImportLine, `\n${i18nImportLine}`) : null, ].filter(isTruthy); }, @@ -81,33 +82,32 @@ export const StringsShouldBeTranslatedWithI18n: Rule.RuleModule = { 'value' in node.value.expression && typeof node.value.expression.value === 'string' ) { - val = node.value.expression.value; + val = cleanString(node.value.expression.value); } // label="foo" if (node.value && 'value' in node.value && typeof node.value.value === 'string') { - val = node.value.value; + val = cleanString(node.value.value); } if (!val) return; // Start building the translation ID suggestion + const intent = getIntentFromNode(val, node); + if (intent === false) return; + const i18nAppId = getI18nIdentifierFromFilePath(filename, cwd); const functionDeclaration = getScope().block as TSESTree.FunctionDeclaration; const functionName = getFunctionName(functionDeclaration); - const intent = getIntentFromNode(val, node); const translationIdSuggestion = `${i18nAppId}.${functionName}.${intent}`; // 'xpack.observability.overview.logs.loadMoreLabel' // Check if i18n has already been imported into the file. - const { - hasI18nImportLine, - i18nPackageImportLine: i18nImportLine, - rangeToAddI18nImportLine, - } = getI18nImportFixer({ - sourceCode, - mode: 'i18n.translate', - }); + const { hasI18nImportLine, i18nImportLine, rangeToAddI18nImportLine, mode } = + getI18nImportFixer({ + sourceCode, + mode: 'i18n.translate', + }); // Show warning to developer and offer autofix suggestion report({ @@ -118,10 +118,12 @@ export const StringsShouldBeTranslatedWithI18n: Rule.RuleModule = { return [ fixer.replaceTextRange( node.value!.range, - `{i18n.translate('${translationIdSuggestion}', { defaultMessage: '${val}'})}` + `{i18n.translate('${translationIdSuggestion}', { defaultMessage: '${val}' })}` ), - !hasI18nImportLine - ? fixer.insertTextAfterRange(rangeToAddI18nImportLine, `\n${i18nImportLine}`) + !hasI18nImportLine && rangeToAddI18nImportLine + ? mode === 'replace' + ? fixer.replaceTextRange(rangeToAddI18nImportLine, i18nImportLine) + : fixer.insertTextAfterRange(rangeToAddI18nImportLine, `\n${i18nImportLine}`) : null, ].filter(isTruthy); }, From dbc64e526e841e8b9e50128d19d30312d73b9cb6 Mon Sep 17 00:00:00 2001 From: Nathan Reese Date: Mon, 30 Oct 2023 12:06:23 -0600 Subject: [PATCH 03/10] [maps] only show vector tile inspector panel when map uses elasticsearch vector tile API (#170043) Closes https://github.com/elastic/kibana/issues/170042 PR disables vector tile adapter when map does not contain vector tile layers ### Test 1) create new map 2) add documents layer 3) set scaling to "limit to 10000" 4) open inspector. Notice inspector only displays request adapter 5) change scaling to "vector tiles" 6) open inspector. Notice inspector displays vector tile adapter and request adapter --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> --- .../inspector/vector_tile_adapter/vector_tile_adapter.ts | 4 ++++ .../vector_tile_adapter/vector_tile_inspector_view.tsx | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/x-pack/plugins/maps/public/inspector/vector_tile_adapter/vector_tile_adapter.ts b/x-pack/plugins/maps/public/inspector/vector_tile_adapter/vector_tile_adapter.ts index d0210367ab3b3..51a12368d6ad4 100644 --- a/x-pack/plugins/maps/public/inspector/vector_tile_adapter/vector_tile_adapter.ts +++ b/x-pack/plugins/maps/public/inspector/vector_tile_adapter/vector_tile_adapter.ts @@ -22,6 +22,10 @@ export class VectorTileAdapter extends EventEmitter { this._onChange(); } + hasLayers() { + return Object.keys(this._layers).length > 0; + } + setTiles(tiles: Array<{ x: number; y: number; z: number }>) { this._tiles = tiles; this._onChange(); diff --git a/x-pack/plugins/maps/public/inspector/vector_tile_adapter/vector_tile_inspector_view.tsx b/x-pack/plugins/maps/public/inspector/vector_tile_adapter/vector_tile_inspector_view.tsx index 42d7423e6e789..560b2789d5a74 100644 --- a/x-pack/plugins/maps/public/inspector/vector_tile_adapter/vector_tile_inspector_view.tsx +++ b/x-pack/plugins/maps/public/inspector/vector_tile_adapter/vector_tile_inspector_view.tsx @@ -23,7 +23,7 @@ export const VectorTileInspectorView = { defaultMessage: 'View the vector tile search requests used to collect the data', }), shouldShow(adapters: Adapters) { - return Boolean(adapters.vectorTiles); + return Boolean(adapters.vectorTiles?.hasLayers()); }, component: (props: { adapters: Adapters }) => { return ; From e17988c3d8a75712862b2ed06598d76dff7412ac Mon Sep 17 00:00:00 2001 From: Jeramy Soucy Date: Mon, 30 Oct 2023 14:40:50 -0400 Subject: [PATCH 04/10] =?UTF-8?q?Upgrades=20axios@1.4.0=E2=86=921.6.0=20(#?= =?UTF-8?q?170070)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Upgrades the `axios` dependency from v1.4.0 to v1.6.0 wherever possible. We have lingering dependencies on older versions 0.21.4 and 0.26.1 via `@slack/webhook`@5.0.4 and `openai`@3.3.0 respectively. --- package.json | 2 +- yarn.lock | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/package.json b/package.json index 5f144902b00e2..114bd462b8d80 100644 --- a/package.json +++ b/package.json @@ -854,7 +854,7 @@ "archiver": "^5.3.1", "async": "^3.2.3", "aws4": "^1.12.0", - "axios": "^1.4.0", + "axios": "^1.6.0", "base64-js": "^1.3.1", "bitmap-sdf": "^1.0.3", "blurhash": "^2.0.1", diff --git a/yarn.lock b/yarn.lock index 7d0bb20bda72c..2d38221a2d11a 100644 --- a/yarn.lock +++ b/yarn.lock @@ -11468,10 +11468,10 @@ axios@^0.26.0: dependencies: follow-redirects "^1.14.8" -axios@^1.3.4, axios@^1.4.0: - version "1.4.0" - resolved "https://registry.yarnpkg.com/axios/-/axios-1.4.0.tgz#38a7bf1224cd308de271146038b551d725f0be1f" - integrity sha512-S4XCWMEmzvo64T9GfvQDOXgYRDJ/wsSZc7Jvdgx5u1sd0JwsuPLqb3SYmusag+edF6ziyMensPVqLTSc1PiSEA== +axios@^1.3.4, axios@^1.4.0, axios@^1.6.0: + version "1.6.0" + resolved "https://registry.yarnpkg.com/axios/-/axios-1.6.0.tgz#f1e5292f26b2fd5c2e66876adc5b06cdbd7d2102" + integrity sha512-EZ1DYihju9pwVB+jg67ogm+Tmqc6JmhamRN6I4Zt8DfZu5lbcQGw3ozH9lFejSJgs/ibaef3A9PMXPLeefFGJg== dependencies: follow-redirects "^1.15.0" form-data "^4.0.0" From 8fe512b8c23479d23b0f3bdcfd92fe0977673f9d Mon Sep 17 00:00:00 2001 From: Alexi Doak <109488926+doakalexi@users.noreply.github.com> Date: Mon, 30 Oct 2023 11:43:53 -0700 Subject: [PATCH 05/10] [ResponseOps] Hyperlinks in Slack messages are broken when there is "_" or "*" in the URL (#170067) Resolves https://github.com/elastic/kibana/issues/165673 ## Summary `escapeSlack` function in the mustasche_renderer breakes the hyperlinks by wrapping the URL with backticks. So the below markdown template does not work. `<{{context.alertDetailsUrl}}|View alert details>` This PR removes the code that adds backticks. With this change action variables with text wrapped in asterisks \*text\* will render as **text** or wrapped in underscores \_text\_ will render as _text_ . ### Checklist - [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 ### To verify - Create a slack connector - Create a rule that uses a slack connector and use the above markdown template - Verify that hyperlink is working properly in slack --- .../actions/server/lib/mustache_renderer.test.ts | 2 ++ x-pack/plugins/actions/server/lib/mustache_renderer.ts | 10 +++++++--- .../common/plugins/alerts/server/alert_types.ts | 1 + .../tests/alerting/group4/mustache_templates.ts | 6 ++++-- 4 files changed, 14 insertions(+), 5 deletions(-) diff --git a/x-pack/plugins/actions/server/lib/mustache_renderer.test.ts b/x-pack/plugins/actions/server/lib/mustache_renderer.test.ts index 3a02ce0d1a983..14bd5f47507e7 100644 --- a/x-pack/plugins/actions/server/lib/mustache_renderer.test.ts +++ b/x-pack/plugins/actions/server/lib/mustache_renderer.test.ts @@ -34,6 +34,7 @@ const variables = { ul: '_', st_lt: '*<', vl: '|', + link: 'https://te_st.com/', }; describe('mustache_renderer', () => { @@ -111,6 +112,7 @@ describe('mustache_renderer', () => { expect(renderMustacheString('{{ul}}', variables, 'slack')).toBe('`_`'); // html escapes not needed when using backtic escaping expect(renderMustacheString('{{st_lt}}', variables, 'slack')).toBe('`*<`'); + expect(renderMustacheString('{{link}}', variables, 'slack')).toBe('https://te_st.com/'); }); it('handles escape:json with commonly escaped strings', () => { diff --git a/x-pack/plugins/actions/server/lib/mustache_renderer.ts b/x-pack/plugins/actions/server/lib/mustache_renderer.ts index 37713167e9a34..c478d7e9ea1c3 100644 --- a/x-pack/plugins/actions/server/lib/mustache_renderer.ts +++ b/x-pack/plugins/actions/server/lib/mustache_renderer.ts @@ -144,10 +144,14 @@ function escapeSlack(value: unknown): string { if (value == null) return ''; const valueString = `${value}`; - // if the value contains * or _, escape the whole thing with back tics + // if the value contains * or _ and is not a url, escape the whole thing with back tics if (valueString.includes('_') || valueString.includes('*')) { - // replace unescapable back tics with single quote - return '`' + valueString.replace(/`/g, `'`) + '`'; + try { + new URL(valueString); + } catch (e) { + // replace unescapable back tics with single quote + return '`' + valueString.replace(/`/g, `'`) + '`'; + } } // otherwise, do "standard" escaping diff --git a/x-pack/test/alerting_api_integration/common/plugins/alerts/server/alert_types.ts b/x-pack/test/alerting_api_integration/common/plugins/alerts/server/alert_types.ts index 70ae9edfffb9c..8bd5ebfad1b21 100644 --- a/x-pack/test/alerting_api_integration/common/plugins/alerts/server/alert_types.ts +++ b/x-pack/test/alerting_api_integration/common/plugins/alerts/server/alert_types.ts @@ -28,6 +28,7 @@ export const EscapableStrings = { escapableHtml: '<&>', escapableDoubleQuote: '"double quote"', escapableLineFeed: 'line\x0afeed', + escapableLink: 'https://te_st.com/', }; export const DeepContextVariables = { diff --git a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group4/mustache_templates.ts b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group4/mustache_templates.ts index 3103ea6fee794..946ecfcbe6ecd 100644 --- a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group4/mustache_templates.ts +++ b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group4/mustache_templates.ts @@ -84,7 +84,7 @@ export default function executionStatusAlertTests({ getService }: FtrProviderCon // from x-pack/test/alerting_api_integration/common/plugins/alerts/server/alert_types.ts, // const EscapableStrings const template = - '{{context.escapableBacktic}} -- {{context.escapableBold}} -- {{context.escapableBackticBold}} -- {{context.escapableHtml}}'; + '{{context.escapableBacktic}} -- {{context.escapableBold}} -- {{context.escapableBackticBold}} -- {{context.escapableHtml}} -- {{context.escapableLink}}'; const rule = await createRule({ id: slackConnector.id, @@ -95,7 +95,9 @@ export default function executionStatusAlertTests({ getService }: FtrProviderCon }); const body = await retry.try(async () => waitForActionBody(slackSimulatorURL, rule.id)); - expect(body).to.be("back'tic -- `*bold*` -- `'*bold*'` -- <&>"); + expect(body).to.be( + "back'tic -- `*bold*` -- `'*bold*'` -- <&> -- https://te_st.com/" + ); }); it('should handle context variable object expansion', async () => { From beae7b1448d00f95c3e89ccd6f62c38d5a973106 Mon Sep 17 00:00:00 2001 From: Dario Gieselaar Date: Mon, 30 Oct 2023 20:03:35 +0100 Subject: [PATCH 06/10] [Obs AI Assistant] Use ELSER v2 (#170092) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> --- .../observability_ai_assistant/server/service/index.ts | 9 ++++++++- .../server/service/kb_service/index.ts | 10 ++++++---- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/x-pack/plugins/observability_ai_assistant/server/service/index.ts b/x-pack/plugins/observability_ai_assistant/server/service/index.ts index 7a268fb7eb3d7..ab37c59563713 100644 --- a/x-pack/plugins/observability_ai_assistant/server/service/index.ts +++ b/x-pack/plugins/observability_ai_assistant/server/service/index.ts @@ -25,6 +25,8 @@ function getResourceName(resource: string) { return `.kibana-observability-ai-assistant-${resource}`; } +export const ELSER_MODEL_ID = '.elser_model_2'; + export const INDEX_QUEUED_DOCUMENTS_TASK_ID = 'observabilityAIAssistant:indexQueuedDocumentsTask'; export const INDEX_QUEUED_DOCUMENTS_TASK_TYPE = INDEX_QUEUED_DOCUMENTS_TASK_ID + 'Type'; @@ -120,6 +122,11 @@ export class ObservabilityAIAssistantService { auto_expand_replicas: '0-1', hidden: true, }, + mappings: { + _meta: { + model: ELSER_MODEL_ID, + }, + }, }, }); @@ -150,7 +157,7 @@ export class ObservabilityAIAssistantService { processors: [ { inference: { - model_id: '.elser_model_1', + model_id: ELSER_MODEL_ID, target_field: 'ml', field_map: { text: 'text_field', diff --git a/x-pack/plugins/observability_ai_assistant/server/service/kb_service/index.ts b/x-pack/plugins/observability_ai_assistant/server/service/kb_service/index.ts index be10c3eaaa5d5..e5a92b3467768 100644 --- a/x-pack/plugins/observability_ai_assistant/server/service/kb_service/index.ts +++ b/x-pack/plugins/observability_ai_assistant/server/service/kb_service/index.ts @@ -13,7 +13,11 @@ import type { TaskManagerStartContract } from '@kbn/task-manager-plugin/server'; import pLimit from 'p-limit'; import pRetry from 'p-retry'; import { map } from 'lodash'; -import { INDEX_QUEUED_DOCUMENTS_TASK_ID, INDEX_QUEUED_DOCUMENTS_TASK_TYPE } from '..'; +import { + ELSER_MODEL_ID, + INDEX_QUEUED_DOCUMENTS_TASK_ID, + INDEX_QUEUED_DOCUMENTS_TASK_TYPE, +} from '..'; import type { KnowledgeBaseEntry } from '../../../common/types'; import type { ObservabilityAIAssistantResourceNames } from '../types'; import { getAccessQuery } from '../util/get_access_query'; @@ -42,8 +46,6 @@ function isAlreadyExistsError(error: Error) { ); } -const ELSER_MODEL_ID = '.elser_model_1'; - function throwKnowledgeBaseNotReady(body: any) { throw serverUnavailable(`Knowledge base is not ready yet`, body); } @@ -199,7 +201,7 @@ export class KnowledgeBaseService { text_expansion: { 'ml.tokens': { model_text: text, - model_id: '.elser_model_1', + model_id: ELSER_MODEL_ID, }, } as unknown as QueryDslTextExpansionQuery, })), From c7e785383a87f7e18509c601a5c089c755ac028e Mon Sep 17 00:00:00 2001 From: Marta Bondyra <4283304+mbondyra@users.noreply.github.com> Date: Mon, 30 Oct 2023 20:52:15 +0100 Subject: [PATCH 07/10] [Lens] Allow non-numeric metrics for metric vis (#169258) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Fixes #137756 Allows to set non numeric metrics for metric visualization for primary and secondary metric. Screenshot 2023-10-19 at 13 45 47 Screenshot 2023-10-19 at 13 46 37 Doesn't include coloring by terms. When primary metric is non-numeric: 1. when maximum value is empty, we hide maximum value group 2. when maximum value has a value we set an error message on dimension 3. we don’t allow to use a palette for coloring 4. we don’t allow to set a trendline Screenshot 2023-10-19 at 13 30 16 Screenshot 2023-10-19 at 13 30 22 ### Checklist Delete any items that are not applicable to this PR. - [ ] 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 - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] 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)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### Risk Matrix Delete this section if it is not applicable to this PR. Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release. When forming the risk matrix, consider some of the following examples and how they may potentially impact the change: | Risk | Probability | Severity | Mitigation/Notes | |---------------------------|-------------|----------|-------------------------| | Multiple Spaces—unexpected behavior in non-default Kibana Space. | Low | High | Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces. | | Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. | High | Low | Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure. | | Code should gracefully handle cases when feature X or plugin Y are disabled. | Medium | High | Unit tests will verify that any feature flag or plugin combination still results in our service operational. | | [See more potential risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) | ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --- .../public/components/metric_vis.tsx | 59 ++- .../config_panel/layer_panel.test.tsx | 27 ++ .../editor_frame/config_panel/layer_panel.tsx | 447 +++++++++--------- x-pack/plugins/lens/public/mocks/index.ts | 24 + .../shared_components/collapse_setting.tsx | 1 + x-pack/plugins/lens/public/types.ts | 1 + .../__snapshots__/visualization.test.ts.snap | 13 + .../metric/dimension_editor.test.tsx | 112 +++-- .../metric/dimension_editor.tsx | 265 ++++++----- .../visualizations/metric/to_expression.ts | 21 +- .../metric/visualization.test.ts | 41 +- .../visualizations/metric/visualization.tsx | 71 ++- .../xy/reference_line_helpers.test.ts | 70 ++- 13 files changed, 671 insertions(+), 481 deletions(-) diff --git a/src/plugins/chart_expressions/expression_metric/public/components/metric_vis.tsx b/src/plugins/chart_expressions/expression_metric/public/components/metric_vis.tsx index b80d8efe68e7c..a0d02562d7623 100644 --- a/src/plugins/chart_expressions/expression_metric/public/components/metric_vis.tsx +++ b/src/plugins/chart_expressions/expression_metric/public/components/metric_vis.tsx @@ -20,12 +20,14 @@ import { MetricWTrend, MetricWNumber, SettingsProps, + MetricWText, } from '@elastic/charts'; import { getColumnByAccessor, getFormatByAccessor } from '@kbn/visualizations-plugin/common/utils'; import { ExpressionValueVisDimension } from '@kbn/visualizations-plugin/common'; import type { Datatable, DatatableColumn, + DatatableRow, IInterpreterRenderHandlers, RenderMode, } from '@kbn/expressions-plugin/common'; @@ -65,6 +67,28 @@ function enhanceFieldFormat(serializedFieldFormat: SerializedFieldFormat | undef return serializedFieldFormat ?? { id: formatId }; } +const renderSecondaryMetric = ( + columns: DatatableColumn[], + row: DatatableRow, + config: Pick +) => { + let secondaryMetricColumn: DatatableColumn | undefined; + let formatSecondaryMetric: ReturnType; + if (config.dimensions.secondaryMetric) { + secondaryMetricColumn = getColumnByAccessor(config.dimensions.secondaryMetric, columns); + formatSecondaryMetric = getMetricFormatter(config.dimensions.secondaryMetric, columns); + } + const secondaryPrefix = config.metric.secondaryPrefix ?? secondaryMetricColumn?.name; + return ( + + {secondaryPrefix} + {secondaryMetricColumn + ? `${secondaryPrefix ? ' ' : ''}${formatSecondaryMetric!(row[secondaryMetricColumn.id])}` + : undefined} + + ); +}; + const getMetricFormatter = ( accessor: ExpressionValueVisDimension | string, columns: Datatable['columns'] @@ -149,13 +173,6 @@ export const MetricVis = ({ const primaryMetricColumn = getColumnByAccessor(config.dimensions.metric, data.columns)!; const formatPrimaryMetric = getMetricFormatter(config.dimensions.metric, data.columns); - let secondaryMetricColumn: DatatableColumn | undefined; - let formatSecondaryMetric: ReturnType; - if (config.dimensions.secondaryMetric) { - secondaryMetricColumn = getColumnByAccessor(config.dimensions.secondaryMetric, data.columns); - formatSecondaryMetric = getMetricFormatter(config.dimensions.secondaryMetric, data.columns); - } - let breakdownByColumn: DatatableColumn | undefined; let formatBreakdownValue: FieldFormatConvertFunction; if (config.dimensions.breakdownBy) { @@ -172,28 +189,32 @@ export const MetricVis = ({ const metricConfigs: MetricSpec['data'][number] = ( breakdownByColumn ? data.rows : data.rows.slice(0, 1) ).map((row, rowIdx) => { - const value: number = row[primaryMetricColumn.id] !== null ? row[primaryMetricColumn.id] : NaN; + const value: number | string = + row[primaryMetricColumn.id] !== null ? row[primaryMetricColumn.id] : NaN; const title = breakdownByColumn ? formatBreakdownValue(row[breakdownByColumn.id]) : primaryMetricColumn.name; const subtitle = breakdownByColumn ? primaryMetricColumn.name : config.metric.subtitle; - const secondaryPrefix = config.metric.secondaryPrefix ?? secondaryMetricColumn?.name; + + if (typeof value !== 'number') { + const nonNumericMetric: MetricWText = { + value: formatPrimaryMetric(value), + title: String(title), + subtitle, + icon: config.metric?.icon ? getIcon(config.metric?.icon) : undefined, + extra: renderSecondaryMetric(data.columns, row, config), + color: config.metric.color ?? defaultColor, + }; + return nonNumericMetric; + } + const baseMetric: MetricWNumber = { value, valueFormatter: formatPrimaryMetric, title: String(title), subtitle, icon: config.metric?.icon ? getIcon(config.metric?.icon) : undefined, - extra: ( - - {secondaryPrefix} - {secondaryMetricColumn - ? `${secondaryPrefix ? ' ' : ''}${formatSecondaryMetric!( - row[secondaryMetricColumn.id] - )}` - : undefined} - - ), + extra: renderSecondaryMetric(data.columns, row, config), color: config.metric.palette && value != null ? getColor( diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.test.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.test.tsx index be3393d3b52e3..cef598de31af0 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.test.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.test.tsx @@ -224,6 +224,33 @@ describe('LayerPanel', () => { const group = instance.find('.lnsLayerPanel__dimensionContainer[data-test-subj="lnsGroup"]'); expect(group).toHaveLength(1); }); + it('does not render a hidden group', async () => { + mockVisualization.getConfiguration.mockReturnValue({ + groups: [ + { + groupLabel: 'A', + groupId: 'a', + accessors: [], + filterOperations: () => true, + supportsMoreColumns: true, + dataTestSubj: 'lnsGroup', + }, + { + groupLabel: 'B', + groupId: 'b', + accessors: [], + filterOperations: () => true, + isHidden: true, + supportsMoreColumns: true, + dataTestSubj: 'lnsGroup', + }, + ], + }); + + const { instance } = await mountWithProvider(); + const group = instance.find('.lnsLayerPanel__dimensionContainer[data-test-subj="lnsGroup"]'); + expect(group).toHaveLength(1); + }); it('should render the required warning when only one group is configured', async () => { mockVisualization.getConfiguration.mockReturnValue({ diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx index 47987926b039f..78a06408902b5 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx @@ -427,239 +427,244 @@ export function LayerPanel( )} - {dimensionGroups.map((group, groupIndex) => { - let errorText: string = ''; - - if (!isEmptyLayer) { - if ( - group.requiredMinDimensionCount && - group.requiredMinDimensionCount > group.accessors.length - ) { - if (group.requiredMinDimensionCount > 1) { + {dimensionGroups + .filter((group) => !group.isHidden) + .map((group, groupIndex) => { + let errorText: string = ''; + + if (!isEmptyLayer) { + if ( + group.requiredMinDimensionCount && + group.requiredMinDimensionCount > group.accessors.length + ) { + if (group.requiredMinDimensionCount > 1) { + errorText = i18n.translate( + 'xpack.lens.editorFrame.requiresTwoOrMoreFieldsWarningLabel', + { + defaultMessage: 'Requires {requiredMinDimensionCount} fields', + values: { + requiredMinDimensionCount: group.requiredMinDimensionCount, + }, + } + ); + } else { + errorText = i18n.translate('xpack.lens.editorFrame.requiresFieldWarningLabel', { + defaultMessage: 'Requires field', + }); + } + } else if (group.dimensionsTooMany && group.dimensionsTooMany > 0) { errorText = i18n.translate( - 'xpack.lens.editorFrame.requiresTwoOrMoreFieldsWarningLabel', + 'xpack.lens.editorFrame.tooManyDimensionsSingularWarningLabel', { - defaultMessage: 'Requires {requiredMinDimensionCount} fields', + defaultMessage: + 'Please remove {dimensionsTooMany, plural, one {a dimension} other {{dimensionsTooMany} dimensions}}', values: { - requiredMinDimensionCount: group.requiredMinDimensionCount, + dimensionsTooMany: group.dimensionsTooMany, }, } ); - } else { - errorText = i18n.translate('xpack.lens.editorFrame.requiresFieldWarningLabel', { - defaultMessage: 'Requires field', - }); } - } else if (group.dimensionsTooMany && group.dimensionsTooMany > 0) { - errorText = i18n.translate( - 'xpack.lens.editorFrame.tooManyDimensionsSingularWarningLabel', - { - defaultMessage: - 'Please remove {dimensionsTooMany, plural, one {a dimension} other {{dimensionsTooMany} dimensions}}', - values: { - dimensionsTooMany: group.dimensionsTooMany, - }, - } - ); } - } - const isOptional = !group.requiredMinDimensionCount && !group.suggestedValue; - return ( - - {group.groupLabel} - {group.groupTooltip && ( - <> - - - )} - - } - labelAppend={ - isOptional ? ( - - {i18n.translate('xpack.lens.editorFrame.optionalDimensionLabel', { - defaultMessage: 'Optional', - })} - - ) : null - } - labelType="legend" - key={group.groupId} - isInvalid={Boolean(errorText)} - error={errorText} - > - <> - {group.accessors.length ? ( - - {group.accessors.map((accessorConfig, accessorIndex) => { - const { columnId } = accessorConfig; - - const messages = - props?.getUserMessages?.('dimensionButton', { - dimensionId: columnId, - }) ?? []; - - return ( - + {group.groupLabel} + {group.groupTooltip && ( + <> + setHideTooltip(true)} - onDragEnd={() => setHideTooltip(false)} - onDrop={onDrop} - indexPatterns={dataViews.indexPatterns} - > - { - setActiveDimension({ - isNew: false, - activeGroup: group, - activeId: id, - }); - }} - onRemoveClick={(id: string) => { - props.onRemoveDimension({ columnId: id, layerId }); - removeButtonRef(id); - }} - message={{ - severity: messages[0]?.severity, - content: messages[0]?.shortMessage || messages[0]?.longMessage, + position="top" + size="s" + type="questionInCircle" + /> + + )} + + } + labelAppend={ + isOptional ? ( + + {i18n.translate('xpack.lens.editorFrame.optionalDimensionLabel', { + defaultMessage: 'Optional', + })} + + ) : null + } + labelType="legend" + key={group.groupId} + isInvalid={Boolean(errorText)} + error={errorText} + > + <> + {group.accessors.length ? ( + + {group.accessors.map((accessorConfig, accessorIndex) => { + const { columnId } = accessorConfig; + + const messages = + props?.getUserMessages?.('dimensionButton', { + dimensionId: columnId, + }) ?? []; + + return ( + setHideTooltip(true)} + onDragEnd={() => setHideTooltip(false)} + onDrop={onDrop} + indexPatterns={dataViews.indexPatterns} > - {layerDatasource ? ( - <> - {layerDatasource.DimensionTriggerComponent({ - ...layerDatasourceConfigProps, - columnId: accessorConfig.columnId, - groupId: group.groupId, - filterOperations: group.filterOperations, - indexPatterns: dataViews.indexPatterns, - })} - - ) : ( - <> - {activeVisualization?.DimensionTriggerComponent?.({ - columnId, - label: columnLabelMap?.[columnId] ?? '', - hideTooltip, - })} - - )} - - - ); - })} - - ) : null} - - {group.fakeFinalAccessor && ( -
- -
- )} + { + setActiveDimension({ + isNew: false, + activeGroup: group, + activeId: id, + }); + }} + onRemoveClick={(id: string) => { + props.onRemoveDimension({ columnId: id, layerId }); + removeButtonRef(id); + }} + message={{ + severity: messages[0]?.severity, + content: messages[0]?.shortMessage || messages[0]?.longMessage, + }} + > + {layerDatasource ? ( + <> + {layerDatasource.DimensionTriggerComponent({ + ...layerDatasourceConfigProps, + columnId: accessorConfig.columnId, + groupId: group.groupId, + filterOperations: group.filterOperations, + indexPatterns: dataViews.indexPatterns, + })} + + ) : ( + <> + {activeVisualization?.DimensionTriggerComponent?.({ + columnId, + label: columnLabelMap?.[columnId] ?? '', + hideTooltip, + })} + + )} + + + ); + })} + + ) : null} + + {group.fakeFinalAccessor && ( +
+ +
+ )} - {group.supportsMoreColumns ? ( - { - props.onEmptyDimensionAdd(id, group); - setActiveDimension({ - activeGroup: group, - activeId: id, - isNew: !group.supportStaticValue && Boolean(layerDatasource), - }); - }} - onDrop={onDrop} - indexPatterns={dataViews.indexPatterns} - /> - ) : null} - -
- ); - })} + {group.supportsMoreColumns ? ( + { + props.onEmptyDimensionAdd(id, group); + setActiveDimension({ + activeGroup: group, + activeId: id, + isNew: !group.supportStaticValue && Boolean(layerDatasource), + }); + }} + onDrop={onDrop} + indexPatterns={dataViews.indexPatterns} + /> + ) : null} + + + ); + })} {(layerDatasource?.LayerSettingsComponent || activeVisualization?.LayerSettingsComponent) && ( diff --git a/x-pack/plugins/lens/public/mocks/index.ts b/x-pack/plugins/lens/public/mocks/index.ts index c71da33fda3c9..f9760c50005f4 100644 --- a/x-pack/plugins/lens/public/mocks/index.ts +++ b/x-pack/plugins/lens/public/mocks/index.ts @@ -6,6 +6,7 @@ */ import { DragContextState, DragContextValue } from '@kbn/dom-drag-drop'; +import { DatatableColumnType } from '@kbn/expressions-plugin/common'; import { createMockDataViewsState } from '../data_views_service/mocks'; import { FramePublicAPI, FrameDatasourceAPI } from '../types'; export { mockDataPlugin } from './data_plugin_mock'; @@ -83,3 +84,26 @@ export function createMockedDragDropContext( setState ? setState : jest.fn(), ]; } + +export function generateActiveData( + json: Array<{ + id: string; + rows: Array>; + }> +) { + return json.reduce((memo, { id, rows }) => { + const columns = Object.keys(rows[0]).map((columnId) => ({ + id: columnId, + name: columnId, + meta: { + type: typeof rows[0][columnId]! as DatatableColumnType, + }, + })); + memo[id] = { + type: 'datatable' as const, + columns, + rows, + }; + return memo; + }, {} as NonNullable); +} diff --git a/x-pack/plugins/lens/public/shared_components/collapse_setting.tsx b/x-pack/plugins/lens/public/shared_components/collapse_setting.tsx index bbaf5296a4e28..9d855fa2bccfe 100644 --- a/x-pack/plugins/lens/public/shared_components/collapse_setting.tsx +++ b/x-pack/plugins/lens/public/shared_components/collapse_setting.tsx @@ -28,6 +28,7 @@ export function CollapseSetting({ return ( <> { diff --git a/x-pack/plugins/lens/public/visualizations/metric/__snapshots__/visualization.test.ts.snap b/x-pack/plugins/lens/public/visualizations/metric/__snapshots__/visualization.test.ts.snap index 8ad5a53ee57b2..2490904751e8c 100644 --- a/x-pack/plugins/lens/public/visualizations/metric/__snapshots__/visualization.test.ts.snap +++ b/x-pack/plugins/lens/public/visualizations/metric/__snapshots__/visualization.test.ts.snap @@ -55,6 +55,7 @@ Object { "groupId": "max", "groupLabel": "Maximum value", "groupTooltip": "If the maximum value is specified, the minimum value is fixed at zero.", + "isHidden": false, "paramEditorCustomProps": Object { "headingLabel": "Value", }, @@ -100,6 +101,10 @@ Array [ "dataType": "number", "isBucketed": false, }, + Object { + "dataType": "string", + "isBucketed": false, + }, ] `; @@ -109,6 +114,10 @@ Array [ "dataType": "number", "isBucketed": false, }, + Object { + "dataType": "string", + "isBucketed": false, + }, ] `; @@ -118,6 +127,10 @@ Array [ "dataType": "number", "isBucketed": false, }, + Object { + "dataType": "string", + "isBucketed": false, + }, ] `; diff --git a/x-pack/plugins/lens/public/visualizations/metric/dimension_editor.test.tsx b/x-pack/plugins/lens/public/visualizations/metric/dimension_editor.test.tsx index de0d59ef1bc4b..5d2c6e2a50ca4 100644 --- a/x-pack/plugins/lens/public/visualizations/metric/dimension_editor.test.tsx +++ b/x-pack/plugins/lens/public/visualizations/metric/dimension_editor.test.tsx @@ -24,7 +24,7 @@ import { mountWithIntl } from '@kbn/test-jest-helpers'; import { LayoutDirection } from '@elastic/charts'; import { act } from 'react-dom/test-utils'; import { EuiColorPickerOutput } from '@elastic/eui/src/components/color_picker/color_picker'; -import { createMockFramePublicAPI } from '../../mocks'; +import { createMockFramePublicAPI, generateActiveData } from '../../mocks'; import { chartPluginMock } from '@kbn/charts-plugin/public/mocks'; import { euiLightVars } from '@kbn/ui-theme'; import { DebouncedInput } from '@kbn/visualization-ui-components'; @@ -47,6 +47,18 @@ const SELECTORS = { BREAKDOWN_EDITOR: '[data-test-subj="lnsMetricDimensionEditor_breakdown"]', }; +const nonNumericMetricFrame = createMockFramePublicAPI({ + activeData: generateActiveData([ + { + id: 'first', + rows: Array(3).fill({ + 'metric-col-id': 'nonNumericData', + 'max-col-id': 1000, + }), + }, + ]), +}); + // see https://github.com/facebook/jest/issues/4402#issuecomment-534516219 const expectCalledBefore = (mock1: jest.Mock, mock2: jest.Mock) => expect(mock1.mock.invocationCallOrder[0]).toBeLessThan(mock2.mock.invocationCallOrder[0]); @@ -102,7 +114,14 @@ describe('dimension editor', () => { } as unknown as DatasourcePublicAPI, removeLayer: jest.fn(), addLayer: jest.fn(), - frame: createMockFramePublicAPI(), + frame: createMockFramePublicAPI({ + activeData: generateActiveData([ + { + id: 'first', + rows: Array(3).fill({ 'metric-col-id': 100, 'secondary-metric-col-id': 1 }), + }, + ]), + }), setState: jest.fn(), panelRef: {} as React.MutableRefObject, paletteService: chartPluginMock.createPaletteRegistry(), @@ -112,27 +131,9 @@ describe('dimension editor', () => { afterEach(() => jest.clearAllMocks()); describe('primary metric dimension', () => { - const accessor = 'primary-metric-col-id'; + const accessor = 'metric-col-id'; const metricAccessorState = { ...fullState, metricAccessor: accessor }; - beforeEach(() => { - props.frame.activeData = { - first: { - type: 'datatable', - columns: [ - { - id: accessor, - name: 'foo', - meta: { - type: 'number', - }, - }, - ], - rows: [], - }, - }; - }); - class Harness { public _wrapper; @@ -146,6 +147,10 @@ describe('dimension editor', () => { return this._wrapper.find(DimensionEditor); } + public get colorModeSwitch() { + return this._wrapper.find('EuiButtonGroup[data-test-subj="lnsMetric_color_mode_buttons"]'); + } + public get colorPicker() { return this._wrapper.find(EuiColorPicker); } @@ -163,11 +168,16 @@ describe('dimension editor', () => { const mockSetState = jest.fn(); - const getHarnessWithState = (state: MetricVisualizationState, datasource = props.datasource) => + const getHarnessWithState = ( + state: MetricVisualizationState, + datasource = props.datasource, + propsOverrides: Partial> = {} + ) => new Harness( mountWithIntl( { expect(component.exists(SELECTORS.BREAKDOWN_EDITOR)).toBeFalsy(); }); + it('Color mode switch is not shown when the primary metric is non-numeric', () => { + expect(getHarnessWithState(fullState, undefined).colorModeSwitch.exists()).toBeTruthy(); + expect( + getHarnessWithState(fullState, undefined, { + frame: nonNumericMetricFrame, + }).colorModeSwitch.exists() + ).toBeFalsy(); + }); + describe('static color controls', () => { it('is hidden when dynamic coloring is enabled', () => { const harnessWithPalette = getHarnessWithState({ ...metricAccessorState, palette }); @@ -202,6 +221,13 @@ describe('dimension editor', () => { }); expect(harnessNoPalette.colorPicker.exists()).toBeTruthy(); }); + it('is visible when metric is non-numeric even if palette is set', () => { + expect( + getHarnessWithState(fullState, undefined, { + frame: nonNumericMetricFrame, + }).colorPicker.exists() + ).toBeTruthy(); + }); it('fills with default value', () => { const localHarness = getHarnessWithState({ @@ -240,24 +266,6 @@ describe('dimension editor', () => { describe('secondary metric dimension', () => { const accessor = 'secondary-metric-col-id'; - beforeEach(() => { - props.frame.activeData = { - first: { - type: 'datatable', - columns: [ - { - id: accessor, - name: 'foo', - meta: { - type: 'number', - }, - }, - ], - rows: [], - }, - }; - }); - it('renders when the accessor matches', () => { const component = shallow( { const setState = jest.fn(); const localState = { ...fullState, - secondaryPrefix: 'foo', + secondaryPrefix: 'secondary-metric-col-id2', secondaryMetricAccessor: accessor, }; const component = mount( @@ -341,7 +349,7 @@ describe('dimension editor', () => { const buttonGroup = component.find(EuiButtonGroup); // make sure that if the user was to select the "custom" option, they would get the default value - expect(buttonGroup.props().options[1].value).toBe('foo'); + expect(buttonGroup.props().options[1].value).toBe('secondary-metric-col-id'); const newVal = 'bar'; @@ -461,7 +469,7 @@ describe('dimension editor', () => { }); describe('additional section', () => { - const accessor = 'primary-metric-col-id'; + const accessor = 'metric-col-id'; const metricAccessorState = { ...fullState, metricAccessor: accessor }; class Harness { @@ -473,6 +481,10 @@ describe('dimension editor', () => { this._wrapper = wrapper; } + public get wrapper() { + return this._wrapper; + } + private get rootComponent() { return this._wrapper.find(DimensionEditorAdditionalSection); } @@ -520,11 +532,16 @@ describe('dimension editor', () => { const mockSetState = jest.fn(); - const getHarnessWithState = (state: MetricVisualizationState, datasource = props.datasource) => + const getHarnessWithState = ( + state: MetricVisualizationState, + datasource = props.datasource, + propsOverrides: Partial> = {} + ) => new Harness( mountWithIntl( { } as DatasourcePublicAPI).isDisabled('trendline') ).toBeTruthy(); }); + it('should not show a trendline button group when primary metric dimension is non-numeric', () => { + expect( + getHarnessWithState(fullState, undefined, { + frame: nonNumericMetricFrame, + }).wrapper.isEmptyRender() + ).toBeTruthy(); + }); describe('responding to buttons', () => { it('enables trendline', () => { diff --git a/x-pack/plugins/lens/public/visualizations/metric/dimension_editor.tsx b/x-pack/plugins/lens/public/visualizations/metric/dimension_editor.tsx index 7d2561369ee6b..c84fa45834fa0 100644 --- a/x-pack/plugins/lens/public/visualizations/metric/dimension_editor.tsx +++ b/x-pack/plugins/lens/public/visualizations/metric/dimension_editor.tsx @@ -143,78 +143,77 @@ function SecondaryMetricEditor({ accessor, idPrefix, frame, layerId, setState, s const defaultPrefix = columnName || ''; return ( -
- - <> - { + + <> + { + setState({ + ...state, + secondaryPrefix, + }); + }} + /> + + {state.secondaryPrefix && ( + { setState({ ...state, - secondaryPrefix, + secondaryPrefix: newPrefix, }); }} /> - - {state.secondaryPrefix && ( - { - setState({ - ...state, - secondaryPrefix: newPrefix, - }); - }} - /> - )} - - -
+ )} + +
); } @@ -225,11 +224,13 @@ function PrimaryMetricEditor(props: SubProps) { const currentData = frame.activeData?.[state.layerId]; - if (accessor == null || !isNumericFieldForDatatable(currentData, accessor)) { + const isMetricNumeric = isNumericFieldForDatatable(currentData, accessor); + + if (accessor == null) { return null; } - const hasDynamicColoring = Boolean(state?.palette); + const hasDynamicColoring = Boolean(isMetricNumeric && state?.palette); const supportsPercentPalette = Boolean( state.maxAccessor || @@ -265,62 +266,65 @@ function PrimaryMetricEditor(props: SubProps) { return ( <> - - { - const colorMode = id.replace(idPrefix, '') as 'static' | 'dynamic'; - - const params = - colorMode === 'dynamic' - ? { - palette: { - ...activePalette, - params: { - ...activePalette.params, - stops: displayStops, + > + { + const colorMode = id.replace(idPrefix, '') as 'static' | 'dynamic'; + + const params = + colorMode === 'dynamic' + ? { + palette: { + ...activePalette, + params: { + ...activePalette.params, + stops: displayStops, + }, }, - }, - } - : { - palette: undefined, - }; - setState({ - ...state, - color: undefined, - ...params, - }); - }} - /> - + color: undefined, + } + : { + palette: undefined, + color: undefined, + }; + setState({ + ...state, + ...params, + }); + }} + /> + + )} {!hasDynamicColoring && } {hasDynamicColoring && ( ) { +function StaticColorControls({ + state, + setState, + frame, +}: Pick) { const colorLabel = i18n.translate('xpack.lens.metric.color', { defaultMessage: 'Color', }); + const currentData = frame.activeData?.[state.layerId]; + const isMetricNumeric = Boolean( + state.metricAccessor && isNumericFieldForDatatable(currentData, state.metricAccessor) + ); const setColor = useCallback( (color: string) => { @@ -420,7 +432,7 @@ function StaticColorControls({ state, setState }: Pick( { onChange: setColor, - value: state.color || getDefaultColor(state), + value: state.color || getDefaultColor(state, isMetricNumeric), }, { allowFalsyValue: true } ); @@ -448,10 +460,12 @@ export function DimensionEditorAdditionalSection({ addLayer, removeLayer, accessor, + frame, }: VisualizationDimensionEditorProps) { const { euiTheme } = useEuiTheme(); - if (accessor !== state.metricAccessor) { + const currentData = frame.activeData?.[state.layerId]; + if (accessor !== state.metricAccessor || !isNumericFieldForDatatable(currentData, accessor)) { return null; } @@ -566,7 +580,6 @@ export function DimensionEditorAdditionalSection({ }`} onChange={(id) => { const supportingVisualizationType = id.split('--')[1] as SupportingVisType; - switch (supportingVisualizationType) { case 'trendline': setState({ diff --git a/x-pack/plugins/lens/public/visualizations/metric/to_expression.ts b/x-pack/plugins/lens/public/visualizations/metric/to_expression.ts index b2367bb7c1fc8..281e758fcbf44 100644 --- a/x-pack/plugins/lens/public/visualizations/metric/to_expression.ts +++ b/x-pack/plugins/lens/public/visualizations/metric/to_expression.ts @@ -90,6 +90,10 @@ export const toExpression = ( const datasource = datasourceLayers[state.layerId]; const datasourceExpression = datasourceExpressionsByLayers[state.layerId]; + const isMetricNumeric = Boolean( + state.metricAccessor && + datasource?.getOperationForColumnId(state.metricAccessor)?.dataType === 'number' + ); const maxPossibleTiles = // if there's a collapse function, no need to calculate since we're dealing with a single tile state.breakdownByAccessor && !state.collapseFn @@ -142,15 +146,16 @@ export const toExpression = ( trendline: trendlineExpression ? [trendlineExpression] : [], subtitle: state.subtitle ?? undefined, progressDirection: state.progressDirection as LayoutDirection, - color: state.color || getDefaultColor(state), + color: state.color || getDefaultColor(state, isMetricNumeric), icon: state.icon, - palette: state.palette?.params - ? [ - paletteService - .get(CUSTOM_PALETTE) - .toExpression(computePaletteParams(state.palette.params as CustomPaletteParams)), - ] - : [], + palette: + isMetricNumeric && state.palette?.params + ? [ + paletteService + .get(CUSTOM_PALETTE) + .toExpression(computePaletteParams(state.palette.params as CustomPaletteParams)), + ] + : [], maxCols: state.maxCols ?? DEFAULT_MAX_COLUMNS, minTiles: maxPossibleTiles ?? undefined, inspectorTableId: state.layerId, diff --git a/x-pack/plugins/lens/public/visualizations/metric/visualization.test.ts b/x-pack/plugins/lens/public/visualizations/metric/visualization.test.ts index 61fac1418196e..78ea5331e34cf 100644 --- a/x-pack/plugins/lens/public/visualizations/metric/visualization.test.ts +++ b/x-pack/plugins/lens/public/visualizations/metric/visualization.test.ts @@ -10,7 +10,7 @@ import { CustomPaletteParams, PaletteOutput } from '@kbn/coloring'; import { ExpressionAstExpression, ExpressionAstFunction } from '@kbn/expressions-plugin/common'; import { euiLightVars, euiThemeVars } from '@kbn/ui-theme'; import { LayerTypes } from '@kbn/expression-xy-plugin/public'; -import { createMockDatasource, createMockFramePublicAPI } from '../../mocks'; +import { createMockDatasource, createMockFramePublicAPI, generateActiveData } from '../../mocks'; import { DatasourceLayers, DatasourcePublicAPI, @@ -82,7 +82,14 @@ describe('metric visualization', () => { ...trendlineProps, }; - const mockFrameApi = createMockFramePublicAPI(); + const mockFrameApi = createMockFramePublicAPI({ + activeData: generateActiveData([ + { + id: 'first', + rows: Array(3).fill({ 'metric-col-id': 20, 'max-metric-col-id': 100 }), + }, + ]), + }); describe('initialization', () => { test('returns a default state', () => { @@ -268,6 +275,7 @@ describe('metric visualization', () => { mockDatasource.publicAPIMock.getMaxPossibleNumValues.mockReturnValue(maxPossibleNumValues); mockDatasource.publicAPIMock.getOperationForColumnId.mockReturnValue({ isStaticValue: false, + dataType: 'number', } as OperationDescriptor); datasourceLayers = { @@ -616,7 +624,7 @@ describe('metric visualization', () => { it('always applies max function to static max dimensions', () => { ( datasourceLayers.first as jest.Mocked - ).getOperationForColumnId.mockReturnValueOnce({ + ).getOperationForColumnId.mockReturnValue({ isStaticValue: true, } as OperationDescriptor); @@ -648,6 +656,9 @@ describe('metric visualization', () => { "type": "function", } `); + ( + datasourceLayers.first as jest.Mocked + ).getOperationForColumnId.mockClear(); }); }); @@ -1109,4 +1120,28 @@ describe('metric visualization', () => { noPadding: true, }); }); + + describe('#getUserMessages', () => { + it('returns error for non numeric primary metric if maxAccessor exists', () => { + const frame = createMockFramePublicAPI({ + activeData: generateActiveData([ + { + id: 'first', + rows: Array(3).fill({ 'metric-col-id': '100', 'max-metric-col-id': 100 }), + }, + ]), + }); + expect(visualization.getUserMessages!(fullState, { frame })).toHaveLength(1); + + const frameNoErrors = createMockFramePublicAPI({ + activeData: generateActiveData([ + { + id: 'first', + rows: Array(3).fill({ 'metric-col-id': 30, 'max-metric-col-id': 100 }), + }, + ]), + }); + expect(visualization.getUserMessages!(fullState, { frame: frameNoErrors })).toHaveLength(0); + }); + }); }); diff --git a/x-pack/plugins/lens/public/visualizations/metric/visualization.tsx b/x-pack/plugins/lens/public/visualizations/metric/visualization.tsx index 4dee7938da049..21a7067f7191a 100644 --- a/x-pack/plugins/lens/public/visualizations/metric/visualization.tsx +++ b/x-pack/plugins/lens/public/visualizations/metric/visualization.tsx @@ -14,6 +14,7 @@ import { LayoutDirection } from '@elastic/charts'; import { euiLightVars, euiThemeVars } from '@kbn/ui-theme'; import { IconChartMetric } from '@kbn/chart-icons'; import { AccessorConfig } from '@kbn/visualization-ui-components'; +import { isNumericFieldForDatatable } from '../../../common/expressions/datatable/utils'; import { CollapseFunction } from '../../../common/expressions'; import type { LayerType } from '../../../common/types'; import { layerTypes } from '../../../common/layer_types'; @@ -25,6 +26,7 @@ import { VisualizationConfigProps, VisualizationDimensionGroupConfig, Suggestion, + UserMessage, } from '../../types'; import { GROUP_ID, LENS_METRIC_ID } from './constants'; import { DimensionEditor, DimensionEditorAdditionalSection } from './dimension_editor'; @@ -40,8 +42,11 @@ export const showingBar = ( ): state is MetricVisualizationState & { showBar: true; maxAccessor: string } => Boolean(state.showBar && state.maxAccessor); -export const getDefaultColor = (state: MetricVisualizationState) => - showingBar(state) ? euiLightVars.euiColorPrimary : euiThemeVars.euiColorLightestShade; +export const getDefaultColor = (state: MetricVisualizationState, isMetricNumeric?: boolean) => { + return showingBar(state) && isMetricNumeric + ? euiLightVars.euiColorPrimary + : euiThemeVars.euiColorLightestShade; +}; export interface MetricVisualizationState { layerId: string; @@ -70,7 +75,13 @@ export interface MetricVisualizationState { trendlineBreakdownByAccessor?: string; } -export const supportedDataTypes = new Set(['number']); +export const supportedDataTypes = new Set(['string', 'boolean', 'number', 'ip', 'date']); + +const isSupportedMetric = (op: OperationMetadata) => + !op.isBucketed && supportedDataTypes.has(op.dataType); + +const isSupportedDynamicMetric = (op: OperationMetadata) => + !op.isBucketed && supportedDataTypes.has(op.dataType) && !op.isStaticValue; export const metricLabel = i18n.translate('xpack.lens.metric.label', { defaultMessage: 'Metric', @@ -84,29 +95,25 @@ const getMetricLayerConfiguration = ( ): { groups: VisualizationDimensionGroupConfig[]; } => { - const isSupportedMetric = (op: OperationMetadata) => - !op.isBucketed && supportedDataTypes.has(op.dataType); + const currentData = props.frame.activeData?.[props.state.layerId]; - const isSupportedDynamicMetric = (op: OperationMetadata) => - !op.isBucketed && supportedDataTypes.has(op.dataType) && !op.isStaticValue; + const isMetricNumeric = Boolean( + props.state.metricAccessor && + isNumericFieldForDatatable(currentData, props.state.metricAccessor) + ); const getPrimaryAccessorDisplayConfig = (): Partial => { + const hasDynamicColoring = Boolean(isMetricNumeric && props.state.palette); const stops = props.state.palette?.params?.stops || []; - const hasStaticColoring = !!props.state.color; - const hasDynamicColoring = !!props.state.palette; + return hasDynamicColoring ? { triggerIconType: 'colorBy', palette: stops.map(({ color }) => color), } - : hasStaticColoring - ? { - triggerIconType: 'color', - color: props.state.color, - } : { triggerIconType: 'color', - color: getDefaultColor(props.state), + color: props.state.color ?? getDefaultColor(props.state, isMetricNumeric), }; }; @@ -180,6 +187,7 @@ const getMetricLayerConfiguration = ( }, ] : [], + isHidden: !props.state.maxAccessor && !isMetricNumeric, supportsMoreColumns: !props.state.maxAccessor, filterOperations: isSupportedMetric, enableDimensionEditor: true, @@ -632,7 +640,7 @@ export const getMetricVisualization = ({ return suggestion; }, - getVisualizationInfo(state) { + getVisualizationInfo(state, frame) { const dimensions = []; if (state.metricAccessor) { dimensions.push({ @@ -676,6 +684,11 @@ export const getMetricVisualization = ({ const hasStaticColoring = !!state.color; const hasDynamicColoring = !!state.palette; + const currentData = frame?.activeData?.[state.layerId]; + const isMetricNumeric = Boolean( + state.metricAccessor && isNumericFieldForDatatable(currentData, state.metricAccessor) + ); + return { layers: [ { @@ -688,10 +701,34 @@ export const getMetricVisualization = ({ ? stops.map(({ color }) => color) : hasStaticColoring ? [state.color] - : [getDefaultColor(state)] + : [getDefaultColor(state, isMetricNumeric)] ).filter(nonNullable), }, ], }; }, + + getUserMessages(state, { frame }) { + const currentData = frame.activeData?.[state.layerId]; + + const errors: UserMessage[] = []; + + if (state.maxAccessor) { + const isMetricNonNumeric = Boolean( + state.metricAccessor && !isNumericFieldForDatatable(currentData, state.metricAccessor) + ); + if (isMetricNonNumeric) { + errors.push({ + severity: 'error', + fixableInEditor: true, + displayLocations: [{ id: 'dimensionButton', dimensionId: state.maxAccessor }], + shortMessage: i18n.translate('xpack.lens.lnsMetric_maxDimensionPanel.nonNumericError', { + defaultMessage: 'Primary metric must be numeric to set a maximum value.', + }), + longMessage: '', + }); + } + } + return errors; + }, }); diff --git a/x-pack/plugins/lens/public/visualizations/xy/reference_line_helpers.test.ts b/x-pack/plugins/lens/public/visualizations/xy/reference_line_helpers.test.ts index 61f32b3adfc66..35aab159d42f6 100644 --- a/x-pack/plugins/lens/public/visualizations/xy/reference_line_helpers.test.ts +++ b/x-pack/plugins/lens/public/visualizations/xy/reference_line_helpers.test.ts @@ -5,25 +5,9 @@ * 2.0. */ -import { FramePublicAPI } from '../../types'; import { computeOverallDataDomain, getStaticValue } from './reference_line_helpers'; import { XYDataLayerConfig } from './types'; - -function getActiveData(json: Array<{ id: string; rows: Array> }>) { - return json.reduce((memo, { id, rows }) => { - const columns = Object.keys(rows[0]).map((columnId) => ({ - id: columnId, - name: columnId, - meta: { type: 'number' as const }, - })); - memo[id] = { - type: 'datatable' as const, - columns, - rows, - }; - return memo; - }, {} as NonNullable); -} +import { generateActiveData } from '../../mocks'; describe('reference_line helpers', () => { describe('getStaticValue', () => { @@ -41,7 +25,7 @@ describe('reference_line helpers', () => { [], 'x', { - activeData: getActiveData([ + activeData: generateActiveData([ { id: 'id-a', rows: Array(3).fill({ a: 100, b: 100, c: 100 }) }, ]), }, @@ -54,7 +38,7 @@ describe('reference_line helpers', () => { [{ layerId: 'id-a', seriesType: 'area' } as XYDataLayerConfig], // missing xAccessor for groupId == x 'x', { - activeData: getActiveData([ + activeData: generateActiveData([ { id: 'id-a', rows: Array(3).fill({ a: 100, b: 100, c: 100 }) }, ]), }, @@ -73,7 +57,7 @@ describe('reference_line helpers', () => { ], // missing hit of accessor "d" in data 'yLeft', { - activeData: getActiveData([ + activeData: generateActiveData([ { id: 'id-a', rows: Array(3).fill({ a: 100, b: 100, c: 100 }) }, ]), }, @@ -92,7 +76,7 @@ describe('reference_line helpers', () => { ], // missing yConfig fallbacks to left axis, but the requested group is yRight 'yRight', { - activeData: getActiveData([ + activeData: generateActiveData([ { id: 'id-a', rows: Array(3).fill({ a: 100, b: 100, c: 100 }) }, ]), }, @@ -111,7 +95,7 @@ describe('reference_line helpers', () => { ], // same as above with x groupId 'x', { - activeData: getActiveData([ + activeData: generateActiveData([ { id: 'id-a', rows: Array(3).fill({ a: 100, b: 100, c: 100 }) }, ]), }, @@ -134,7 +118,7 @@ describe('reference_line helpers', () => { ], 'yRight', { - activeData: getActiveData([ + activeData: generateActiveData([ { id: 'id-a', rows: [{ a: -30 }, { a: 10 }], @@ -159,7 +143,7 @@ describe('reference_line helpers', () => { ], 'yLeft', { - activeData: getActiveData([ + activeData: generateActiveData([ { id: 'id-a', rows: Array(3).fill({ a: 100, b: 100, c: 100 }) }, ]), }, @@ -182,7 +166,7 @@ describe('reference_line helpers', () => { ], 'yRight', { - activeData: getActiveData([ + activeData: generateActiveData([ { id: 'id-a', rows: Array(3).fill({ a: 100, b: 100, c: 100 }) }, ]), }, @@ -192,7 +176,7 @@ describe('reference_line helpers', () => { }); it('should correctly distribute axis on left and right with different formatters when in auto', () => { - const tables = getActiveData([ + const tables = generateActiveData([ { id: 'id-a', rows: Array(3).fill({ a: 100, b: 200, c: 100 }) }, ]); tables['id-a'].columns[0].meta.params = { id: 'number' }; // a: number formatter @@ -230,7 +214,7 @@ describe('reference_line helpers', () => { }); it('should ignore hasHistogram for left or right axis', () => { - const tables = getActiveData([ + const tables = generateActiveData([ { id: 'id-a', rows: Array(3).fill({ a: 100, b: 200, c: 100 }) }, ]); tables['id-a'].columns[0].meta.params = { id: 'number' }; // a: number formatter @@ -285,7 +269,7 @@ describe('reference_line helpers', () => { ], 'x', // this is influenced by the callback { - activeData: getActiveData([ + activeData: generateActiveData([ { id: 'id-a', rows: Array(3).fill({ a: 100, b: 100, c: 100 }) }, ]), }, @@ -312,7 +296,7 @@ describe('reference_line helpers', () => { ], 'x', { - activeData: getActiveData([ + activeData: generateActiveData([ { id: 'id-a', rows: Array(3) @@ -334,7 +318,7 @@ describe('reference_line helpers', () => { computeOverallDataDomain( [{ layerId: 'id-a', seriesType, accessors: ['a', 'b', 'c'] } as XYDataLayerConfig], ['a', 'b', 'c'], - getActiveData([ + generateActiveData([ { id: 'id-a', rows: Array(3) @@ -360,7 +344,7 @@ describe('reference_line helpers', () => { computeOverallDataDomain( [{ layerId: 'id-a', seriesType, accessors: ['a', 'b', 'c'] } as XYDataLayerConfig], ['a', 'b', 'c'], - getActiveData([ + generateActiveData([ { id: 'id-a', rows: Array(3) @@ -385,7 +369,7 @@ describe('reference_line helpers', () => { { layerId: 'id-b', seriesType, accessors: ['d', 'e', 'f'] }, ] as XYDataLayerConfig[], ['a', 'b', 'c', 'd', 'e', 'f'], - getActiveData([ + generateActiveData([ { id: 'id-a', rows: [{ a: 25, b: 100, c: 100 }] }, { id: 'id-b', rows: [{ d: 50, e: 50, f: 50 }] }, ]) @@ -399,7 +383,7 @@ describe('reference_line helpers', () => { { layerId: 'id-b', seriesType, accessors: ['d', 'e', 'f'] }, ] as XYDataLayerConfig[], ['a', 'b', 'c', 'd', 'e', 'f'], - getActiveData([ + generateActiveData([ { id: 'id-a', rows: Array(3) @@ -435,7 +419,7 @@ describe('reference_line helpers', () => { { layerId: 'id-b', seriesType, accessors: ['d', 'e', 'f'] }, ] as XYDataLayerConfig[], ['a', 'b', 'c', 'd', 'e', 'f'], - getActiveData([ + generateActiveData([ { id: 'id-a', rows: Array(3).fill({ a: 100, b: 100, c: 100 }) }, { id: 'id-b', rows: Array(3).fill({ d: 50, e: 50, f: 50 }) }, ]) @@ -453,7 +437,7 @@ describe('reference_line helpers', () => { { layerId: 'id-b', seriesType: stackedSeries, accessors: ['d', 'e', 'f'] }, ] as XYDataLayerConfig[], ['a', 'b', 'c', 'd', 'e', 'f'], - getActiveData([ + generateActiveData([ { id: 'id-a', rows: [{ a: 100, b: 100, c: 100 }] }, { id: 'id-b', rows: [{ d: 50, e: 50, f: 50 }] }, ]) @@ -475,7 +459,7 @@ describe('reference_line helpers', () => { { layerId: 'id-b', seriesType, xAccessor: 'f', accessors: ['d', 'e'] }, ] as XYDataLayerConfig[], ['a', 'b', 'd', 'e'], - getActiveData([ + generateActiveData([ { id: 'id-a', rows: Array(3) @@ -502,7 +486,7 @@ describe('reference_line helpers', () => { { layerId: 'id-b', seriesType, accessors: ['f'] }, ] as XYDataLayerConfig[], ['c', 'f'], - getActiveData([ + generateActiveData([ { id: 'id-a', rows: Array(3) @@ -530,7 +514,7 @@ describe('reference_line helpers', () => { { layerId: 'id-b', seriesType, xAccessor: 'f', accessors: ['d', 'e'] }, ] as XYDataLayerConfig[], ['a', 'b', 'd', 'e'], - getActiveData([ + generateActiveData([ { id: 'id-a', rows: Array(3) @@ -560,7 +544,7 @@ describe('reference_line helpers', () => { } as XYDataLayerConfig, ], ['a', 'b', 'c'], - getActiveData([ + generateActiveData([ { id: 'id-a', rows: Array(3) @@ -584,7 +568,7 @@ describe('reference_line helpers', () => { } as XYDataLayerConfig, ], ['a', 'b', 'c'], - getActiveData([ + generateActiveData([ { id: 'id-a', rows: Array(3) @@ -605,7 +589,7 @@ describe('reference_line helpers', () => { computeOverallDataDomain( [], ['a', 'b', 'c'], - getActiveData([{ id: 'id-a', rows: Array(3).fill({ a: 100, b: 100, c: 100 }) }]) + generateActiveData([{ id: 'id-a', rows: Array(3).fill({ a: 100, b: 100, c: 100 }) }]) ) ).toEqual({ min: undefined, max: undefined }); }); @@ -618,7 +602,7 @@ describe('reference_line helpers', () => { { layerId: 'id-b', seriesType: 'line', accessors: ['d', 'e', 'f'] }, ] as XYDataLayerConfig[], ['a', 'b'], - getActiveData([{ id: 'id-c', rows: [{ a: 100, b: 100 }] }]) // mind the layer id here + generateActiveData([{ id: 'id-c', rows: [{ a: 100, b: 100 }] }]) // mind the layer id here ) ).toEqual({ min: undefined, max: undefined }); @@ -629,7 +613,7 @@ describe('reference_line helpers', () => { { layerId: 'id-b', seriesType: 'bar_stacked' }, ] as XYDataLayerConfig[], ['a', 'b'], - getActiveData([]) + generateActiveData([]) ) ).toEqual({ min: undefined, max: undefined }); }); From be21e7979e86edb84e07b313c263057a9bd2fc80 Mon Sep 17 00:00:00 2001 From: Elastic Machine Date: Tue, 31 Oct 2023 07:21:12 +1100 Subject: [PATCH 08/10] Update kubernetes templates for elastic-agent (#170146) Automated by https://fleet-ci.elastic.co/job/elastic-agent/job/elastic-agent-mbp/job/main/1479/ Co-authored-by: obscloudnativemonitoring --- x-pack/plugins/fleet/server/services/elastic_agent_manifest.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/fleet/server/services/elastic_agent_manifest.ts b/x-pack/plugins/fleet/server/services/elastic_agent_manifest.ts index 385b1095cb9f4..8bd0f823cb5c6 100644 --- a/x-pack/plugins/fleet/server/services/elastic_agent_manifest.ts +++ b/x-pack/plugins/fleet/server/services/elastic_agent_manifest.ts @@ -42,7 +42,7 @@ spec: # - -c # - >- # mkdir -p /etc/elastic-agent/inputs.d && - # wget -O - https://github.com/elastic/elastic-agent/archive/main.tar.gz | tar xz -C /etc/elastic-agent/inputs.d --strip=5 "elastic-agent-main/deploy/kubernetes/elastic-agent/templates.d" + # wget -O - https://github.com/elastic/elastic-agent/archive/8.12.tar.gz | tar xz -C /etc/elastic-agent/inputs.d --strip=5 "elastic-agent-8.12/deploy/kubernetes/elastic-agent/templates.d" # volumeMounts: # - name: external-inputs # mountPath: /etc/elastic-agent/inputs.d From 788d30239640e4f549eaeedebad766072de45a05 Mon Sep 17 00:00:00 2001 From: Davis McPhee Date: Mon, 30 Oct 2023 17:36:32 -0300 Subject: [PATCH 09/10] [Discover] Revert data table row height to 3 by default (#169724) ## Summary This PR reverts the changes from #164218 that updated the default Discover grid row height to "auto", and changes it back to "3". ### Checklist - [ ] 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 - [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 - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] 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)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --- packages/kbn-unified-data-table/src/constants.ts | 2 +- .../src/hooks/use_row_heights_options.test.tsx | 14 +++++++++----- src/plugins/discover/server/ui_settings.ts | 2 +- .../apps/discover/group2/_data_grid_row_height.ts | 4 ++-- 4 files changed, 13 insertions(+), 9 deletions(-) diff --git a/packages/kbn-unified-data-table/src/constants.ts b/packages/kbn-unified-data-table/src/constants.ts index f648f09f558f3..c4c236c828fc1 100644 --- a/packages/kbn-unified-data-table/src/constants.ts +++ b/packages/kbn-unified-data-table/src/constants.ts @@ -20,7 +20,7 @@ export const ROWS_PER_PAGE_OPTIONS = [10, 25, 50, DEFAULT_ROWS_PER_PAGE, 250, 50 export const ROWS_HEIGHT_OPTIONS = { auto: -1, single: 0, - default: -1, + default: 3, }; export const defaultRowLineHeight = '1.6em'; export const defaultMonacoEditorWidth = 370; diff --git a/packages/kbn-unified-data-table/src/hooks/use_row_heights_options.test.tsx b/packages/kbn-unified-data-table/src/hooks/use_row_heights_options.test.tsx index 1ef0d9c70d139..2da08c178720a 100644 --- a/packages/kbn-unified-data-table/src/hooks/use_row_heights_options.test.tsx +++ b/packages/kbn-unified-data-table/src/hooks/use_row_heights_options.test.tsx @@ -11,6 +11,8 @@ import { Storage } from '@kbn/kibana-utils-plugin/public'; import { LocalStorageMock } from '../../__mocks__/local_storage_mock'; import { useRowHeightsOptions } from './use_row_heights_options'; +const CONFIG_ROW_HEIGHT = 3; + describe('useRowHeightsOptions', () => { test('should apply rowHeight from savedSearch', () => { const { result } = renderHook(() => { @@ -30,7 +32,7 @@ describe('useRowHeightsOptions', () => { storage: new LocalStorageMock({ ['discover:dataGridRowHeight']: { previousRowHeight: 5, - previousConfigRowHeight: -1, + previousConfigRowHeight: 3, }, }) as unknown as Storage, consumer: 'discover', @@ -50,7 +52,7 @@ describe('useRowHeightsOptions', () => { }); expect(result.current.defaultHeight).toEqual({ - lineCount: 3, + lineCount: CONFIG_ROW_HEIGHT, }); }); @@ -59,8 +61,8 @@ describe('useRowHeightsOptions', () => { return useRowHeightsOptions({ storage: new LocalStorageMock({ ['discover:dataGridRowHeight']: { - previousRowHeight: 5, - // different from uiSettings (config), now user changed it to -1, but prev was 4 + previousRowHeight: 4, + // different from uiSettings (config), now user changed it to 3, but prev was 4 previousConfigRowHeight: 4, }, }) as unknown as Storage, @@ -68,6 +70,8 @@ describe('useRowHeightsOptions', () => { }); }); - expect(result.current.defaultHeight).toEqual('auto'); + expect(result.current.defaultHeight).toEqual({ + lineCount: CONFIG_ROW_HEIGHT, + }); }); }); diff --git a/src/plugins/discover/server/ui_settings.ts b/src/plugins/discover/server/ui_settings.ts index bc5ad09d260c4..d6bbbd0eed9f0 100644 --- a/src/plugins/discover/server/ui_settings.ts +++ b/src/plugins/discover/server/ui_settings.ts @@ -287,7 +287,7 @@ export const getUiSettings: (docLinks: DocLinksServiceSetup) => Record { await dataGrid.clickGridSettings(); - expect(await dataGrid.getCurrentRowHeightValue()).to.be('Auto fit'); + expect(await dataGrid.getCurrentRowHeightValue()).to.be('Custom'); await dataGrid.changeRowHeightValue('Single'); From d7ab75ffc51c8a31728efae5570261abd7a10c8c Mon Sep 17 00:00:00 2001 From: Jon Date: Mon, 30 Oct 2023 15:52:24 -0500 Subject: [PATCH 10/10] Revert "[licensing] add license fetcher cache (#170006)" (#170185) This reverts commit 21c0b0b4205ba2d442656776b23b18e2dc062940. --- .../licensing/common/license_update.ts | 15 +- .../licensing/server/license_fetcher.test.ts | 172 ------------------ .../licensing/server/license_fetcher.ts | 133 -------------- .../licensing/server/licensing_config.ts | 12 +- .../plugins/licensing/server/plugin.test.ts | 139 +++++++++----- x-pack/plugins/licensing/server/plugin.ts | 120 ++++++++++-- x-pack/plugins/licensing/server/types.ts | 2 - x-pack/plugins/licensing/tsconfig.json | 3 +- 8 files changed, 210 insertions(+), 386 deletions(-) delete mode 100644 x-pack/plugins/licensing/server/license_fetcher.test.ts delete mode 100644 x-pack/plugins/licensing/server/license_fetcher.ts diff --git a/x-pack/plugins/licensing/common/license_update.ts b/x-pack/plugins/licensing/common/license_update.ts index a35b7aa6e6785..b344d8ce2d16a 100644 --- a/x-pack/plugins/licensing/common/license_update.ts +++ b/x-pack/plugins/licensing/common/license_update.ts @@ -17,7 +17,6 @@ import { takeUntil, finalize, startWith, - throttleTime, } from 'rxjs/operators'; import { hasLicenseInfoChanged } from './has_license_info_changed'; import type { ILicense } from './types'; @@ -30,15 +29,11 @@ export function createLicenseUpdate( ) { const manuallyRefresh$ = new Subject(); - const fetched$ = merge( - triggerRefresh$, - manuallyRefresh$.pipe( - throttleTime(1000, undefined, { - leading: true, - trailing: true, - }) - ) - ).pipe(takeUntil(stop$), exhaustMap(fetcher), share()); + const fetched$ = merge(triggerRefresh$, manuallyRefresh$).pipe( + takeUntil(stop$), + exhaustMap(fetcher), + share() + ); // provide a first, empty license, so that we can compare in the filter below const startWithArgs = initialValues ? [undefined, initialValues] : [undefined]; diff --git a/x-pack/plugins/licensing/server/license_fetcher.test.ts b/x-pack/plugins/licensing/server/license_fetcher.test.ts deleted file mode 100644 index efd9b001fa0ff..0000000000000 --- a/x-pack/plugins/licensing/server/license_fetcher.test.ts +++ /dev/null @@ -1,172 +0,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 type * as estypes from '@elastic/elasticsearch/lib/api/typesWithBodyKey'; -import { getLicenseFetcher } from './license_fetcher'; -import { loggerMock, type MockedLogger } from '@kbn/logging-mocks'; -import { elasticsearchServiceMock } from '@kbn/core/server/mocks'; - -type EsLicense = estypes.XpackInfoMinimalLicenseInformation; - -const delay = (ms: number) => new Promise((res) => setTimeout(res, ms)); - -function buildRawLicense(options: Partial = {}): EsLicense { - return { - uid: 'uid-000000001234', - status: 'active', - type: 'basic', - mode: 'basic', - expiry_date_in_millis: 1000, - ...options, - }; -} - -describe('LicenseFetcher', () => { - let logger: MockedLogger; - let clusterClient: ReturnType; - - beforeEach(() => { - logger = loggerMock.create(); - clusterClient = elasticsearchServiceMock.createClusterClient(); - }); - - it('returns the license for successful calls', async () => { - clusterClient.asInternalUser.xpack.info.mockResponse({ - license: buildRawLicense({ - uid: 'license-1', - }), - features: {}, - } as any); - - const fetcher = getLicenseFetcher({ - logger, - clusterClient, - cacheDurationMs: 50_000, - }); - - const license = await fetcher(); - expect(license.uid).toEqual('license-1'); - }); - - it('returns the latest license for successful calls', async () => { - clusterClient.asInternalUser.xpack.info - .mockResponseOnce({ - license: buildRawLicense({ - uid: 'license-1', - }), - features: {}, - } as any) - .mockResponseOnce({ - license: buildRawLicense({ - uid: 'license-2', - }), - features: {}, - } as any); - - const fetcher = getLicenseFetcher({ - logger, - clusterClient, - cacheDurationMs: 50_000, - }); - - let license = await fetcher(); - expect(license.uid).toEqual('license-1'); - - license = await fetcher(); - expect(license.uid).toEqual('license-2'); - }); - - it('returns an error license in case of error', async () => { - clusterClient.asInternalUser.xpack.info.mockResponseImplementation(() => { - throw new Error('woups'); - }); - - const fetcher = getLicenseFetcher({ - logger, - clusterClient, - cacheDurationMs: 50_000, - }); - - const license = await fetcher(); - expect(license.error).toEqual('woups'); - }); - - it('returns a license successfully fetched after an error', async () => { - clusterClient.asInternalUser.xpack.info - .mockResponseImplementationOnce(() => { - throw new Error('woups'); - }) - .mockResponseOnce({ - license: buildRawLicense({ - uid: 'license-1', - }), - features: {}, - } as any); - - const fetcher = getLicenseFetcher({ - logger, - clusterClient, - cacheDurationMs: 50_000, - }); - - let license = await fetcher(); - expect(license.error).toEqual('woups'); - license = await fetcher(); - expect(license.uid).toEqual('license-1'); - }); - - it('returns the latest fetched license after an error within the cache duration period', async () => { - clusterClient.asInternalUser.xpack.info - .mockResponseOnce({ - license: buildRawLicense({ - uid: 'license-1', - }), - features: {}, - } as any) - .mockResponseImplementationOnce(() => { - throw new Error('woups'); - }); - - const fetcher = getLicenseFetcher({ - logger, - clusterClient, - cacheDurationMs: 50_000, - }); - - let license = await fetcher(); - expect(license.uid).toEqual('license-1'); - license = await fetcher(); - expect(license.uid).toEqual('license-1'); - }); - - it('returns an error license after an error exceeding the cache duration period', async () => { - clusterClient.asInternalUser.xpack.info - .mockResponseOnce({ - license: buildRawLicense({ - uid: 'license-1', - }), - features: {}, - } as any) - .mockResponseImplementationOnce(() => { - throw new Error('woups'); - }); - - const fetcher = getLicenseFetcher({ - logger, - clusterClient, - cacheDurationMs: 1, - }); - - let license = await fetcher(); - expect(license.uid).toEqual('license-1'); - - await delay(50); - - license = await fetcher(); - expect(license.error).toEqual('woups'); - }); -}); diff --git a/x-pack/plugins/licensing/server/license_fetcher.ts b/x-pack/plugins/licensing/server/license_fetcher.ts deleted file mode 100644 index 43d9c204bbf66..0000000000000 --- a/x-pack/plugins/licensing/server/license_fetcher.ts +++ /dev/null @@ -1,133 +0,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 type * as estypes from '@elastic/elasticsearch/lib/api/typesWithBodyKey'; -import { createHash } from 'crypto'; -import stringify from 'json-stable-stringify'; -import type { MaybePromise } from '@kbn/utility-types'; -import { isPromise } from '@kbn/std'; -import type { IClusterClient, Logger } from '@kbn/core/server'; -import type { - ILicense, - PublicLicense, - PublicFeatures, - LicenseType, - LicenseStatus, -} from '../common/types'; -import { License } from '../common/license'; -import type { ElasticsearchError, LicenseFetcher } from './types'; - -export const getLicenseFetcher = ({ - clusterClient, - logger, - cacheDurationMs, -}: { - clusterClient: MaybePromise; - logger: Logger; - cacheDurationMs: number; -}): LicenseFetcher => { - let currentLicense: ILicense | undefined; - let lastSuccessfulFetchTime: number | undefined; - - return async () => { - const client = isPromise(clusterClient) ? await clusterClient : clusterClient; - try { - const response = await client.asInternalUser.xpack.info(); - const normalizedLicense = - response.license && response.license.type !== 'missing' - ? normalizeServerLicense(response.license) - : undefined; - const normalizedFeatures = response.features - ? normalizeFeatures(response.features) - : undefined; - - const signature = sign({ - license: normalizedLicense, - features: normalizedFeatures, - error: '', - }); - - currentLicense = new License({ - license: normalizedLicense, - features: normalizedFeatures, - signature, - }); - lastSuccessfulFetchTime = Date.now(); - - return currentLicense; - } catch (error) { - logger.warn( - `License information could not be obtained from Elasticsearch due to ${error} error` - ); - - if (lastSuccessfulFetchTime && lastSuccessfulFetchTime + cacheDurationMs > Date.now()) { - return currentLicense!; - } else { - const errorMessage = getErrorMessage(error); - const signature = sign({ error: errorMessage }); - - return new License({ - error: getErrorMessage(error), - signature, - }); - } - } - }; -}; - -function normalizeServerLicense( - license: estypes.XpackInfoMinimalLicenseInformation -): PublicLicense { - return { - uid: license.uid, - type: license.type as LicenseType, - mode: license.mode as LicenseType, - expiryDateInMillis: - typeof license.expiry_date_in_millis === 'string' - ? parseInt(license.expiry_date_in_millis, 10) - : license.expiry_date_in_millis, - status: license.status as LicenseStatus, - }; -} - -function normalizeFeatures(rawFeatures: estypes.XpackInfoFeatures) { - const features: PublicFeatures = {}; - for (const [name, feature] of Object.entries(rawFeatures)) { - features[name] = { - isAvailable: feature.available, - isEnabled: feature.enabled, - }; - } - return features; -} - -function sign({ - license, - features, - error, -}: { - license?: PublicLicense; - features?: PublicFeatures; - error?: string; -}) { - return createHash('sha256') - .update( - stringify({ - license, - features, - error, - }) - ) - .digest('hex'); -} - -function getErrorMessage(error: ElasticsearchError): string { - if (error.status === 400) { - return 'X-Pack plugin is not installed on the Elasticsearch cluster.'; - } - return error.message; -} diff --git a/x-pack/plugins/licensing/server/licensing_config.ts b/x-pack/plugins/licensing/server/licensing_config.ts index 66899602e04cb..459c69b650dbb 100644 --- a/x-pack/plugins/licensing/server/licensing_config.ts +++ b/x-pack/plugins/licensing/server/licensing_config.ts @@ -10,18 +10,12 @@ import { PluginConfigDescriptor } from '@kbn/core/server'; const configSchema = schema.object({ api_polling_frequency: schema.duration({ defaultValue: '30s' }), - license_cache_duration: schema.duration({ - defaultValue: '300s', - validate: (value) => { - if (value.asMinutes() > 15) { - return 'license cache duration must be shorter than 15 minutes'; - } - }, - }), }); export type LicenseConfigType = TypeOf; export const config: PluginConfigDescriptor = { - schema: configSchema, + schema: schema.object({ + api_polling_frequency: schema.duration({ defaultValue: '30s' }), + }), }; diff --git a/x-pack/plugins/licensing/server/plugin.test.ts b/x-pack/plugins/licensing/server/plugin.test.ts index 129dc6aee66da..b087b6f3f03fa 100644 --- a/x-pack/plugins/licensing/server/plugin.test.ts +++ b/x-pack/plugins/licensing/server/plugin.test.ts @@ -56,23 +56,22 @@ describe('licensing plugin', () => { return client; }; - let plugin: LicensingPlugin; - let pluginInitContextMock: ReturnType; + describe('#start', () => { + describe('#license$', () => { + let plugin: LicensingPlugin; + let pluginInitContextMock: ReturnType; - beforeEach(() => { - pluginInitContextMock = coreMock.createPluginInitializerContext({ - api_polling_frequency: moment.duration(100), - license_cache_duration: moment.duration(1000), - }); - plugin = new LicensingPlugin(pluginInitContextMock); - }); + beforeEach(() => { + pluginInitContextMock = coreMock.createPluginInitializerContext({ + api_polling_frequency: moment.duration(100), + }); + plugin = new LicensingPlugin(pluginInitContextMock); + }); - afterEach(async () => { - await plugin?.stop(); - }); + afterEach(async () => { + await plugin.stop(); + }); - describe('#start', () => { - describe('#license$', () => { it('returns license', async () => { const esClient = createEsClient({ license: buildRawLicense(), @@ -80,8 +79,8 @@ describe('licensing plugin', () => { }); const coreSetup = createCoreSetupWith(esClient); - plugin.setup(coreSetup); - const { license$ } = plugin.start(); + await plugin.setup(coreSetup); + const { license$ } = await plugin.start(); const license = await firstValueFrom(license$); expect(license.isAvailable).toBe(true); }); @@ -93,8 +92,8 @@ describe('licensing plugin', () => { }); const coreSetup = createCoreSetupWith(esClient); - plugin.setup(coreSetup); - const { license$ } = plugin.start(); + await plugin.setup(coreSetup); + const { license$ } = await plugin.start(); await firstValueFrom(license$); expect(esClient.asInternalUser.xpack.info).toHaveBeenCalledTimes(1); @@ -112,8 +111,8 @@ describe('licensing plugin', () => { }); const coreSetup = createCoreSetupWith(esClient); - plugin.setup(coreSetup); - const { license$ } = plugin.start(); + await plugin.setup(coreSetup); + const { license$ } = await plugin.start(); const [first, second, third] = await firstValueFrom(license$.pipe(take(3), toArray())); expect(first.type).toBe('basic'); @@ -126,8 +125,8 @@ describe('licensing plugin', () => { esClient.asInternalUser.xpack.info.mockRejectedValue(new Error('test')); const coreSetup = createCoreSetupWith(esClient); - plugin.setup(coreSetup); - const { license$ } = plugin.start(); + await plugin.setup(coreSetup); + const { license$ } = await plugin.start(); const license = await firstValueFrom(license$); expect(license.isAvailable).toBe(false); @@ -141,8 +140,8 @@ describe('licensing plugin', () => { esClient.asInternalUser.xpack.info.mockRejectedValue(error); const coreSetup = createCoreSetupWith(esClient); - plugin.setup(coreSetup); - const { license$ } = plugin.start(); + await plugin.setup(coreSetup); + const { license$ } = await plugin.start(); const license = await firstValueFrom(license$); expect(license.isAvailable).toBe(false); @@ -170,8 +169,8 @@ describe('licensing plugin', () => { }); const coreSetup = createCoreSetupWith(esClient); - plugin.setup(coreSetup); - const { license$ } = plugin.start(); + await plugin.setup(coreSetup); + const { license$ } = await plugin.start(); const [first, second, third] = await firstValueFrom(license$.pipe(take(3), toArray())); @@ -187,8 +186,8 @@ describe('licensing plugin', () => { }); const coreSetup = createCoreSetupWith(esClient); - plugin.setup(coreSetup); - plugin.start(); + await plugin.setup(coreSetup); + await plugin.start(); await flushPromises(); @@ -202,8 +201,8 @@ describe('licensing plugin', () => { }); const coreSetup = createCoreSetupWith(esClient); - plugin.setup(coreSetup); - plugin.start(); + await plugin.setup(coreSetup); + await plugin.start(); await flushPromises(); @@ -230,8 +229,8 @@ describe('licensing plugin', () => { }); const coreSetup = createCoreSetupWith(esClient); - plugin.setup(coreSetup); - const { license$ } = plugin.start(); + await plugin.setup(coreSetup); + const { license$ } = await plugin.start(); const [first, second, third] = await firstValueFrom(license$.pipe(take(3), toArray())); expect(first.signature === third.signature).toBe(true); @@ -240,12 +239,16 @@ describe('licensing plugin', () => { }); describe('#refresh', () => { + let plugin: LicensingPlugin; + afterEach(async () => { + await plugin.stop(); + }); + it('forces refresh immediately', async () => { plugin = new LicensingPlugin( coreMock.createPluginInitializerContext({ // disable polling mechanism api_polling_frequency: moment.duration(50000), - license_cache_duration: moment.duration(1000), }) ); const esClient = createEsClient({ @@ -254,26 +257,31 @@ describe('licensing plugin', () => { }); const coreSetup = createCoreSetupWith(esClient); - plugin.setup(coreSetup); - const { refresh, license$ } = plugin.start(); + await plugin.setup(coreSetup); + const { refresh, license$ } = await plugin.start(); expect(esClient.asInternalUser.xpack.info).toHaveBeenCalledTimes(0); - await firstValueFrom(license$); + await license$.pipe(take(1)).toPromise(); expect(esClient.asInternalUser.xpack.info).toHaveBeenCalledTimes(1); - await refresh(); + refresh(); await flushPromises(); expect(esClient.asInternalUser.xpack.info).toHaveBeenCalledTimes(2); }); }); describe('#createLicensePoller', () => { + let plugin: LicensingPlugin; + + afterEach(async () => { + await plugin.stop(); + }); + it(`creates a poller fetching license from passed 'clusterClient' every 'api_polling_frequency' ms`, async () => { plugin = new LicensingPlugin( coreMock.createPluginInitializerContext({ api_polling_frequency: moment.duration(50000), - license_cache_duration: moment.duration(1000), }) ); @@ -282,8 +290,8 @@ describe('licensing plugin', () => { features: {}, }); const coreSetup = createCoreSetupWith(esClient); - plugin.setup(coreSetup); - const { createLicensePoller, license$ } = plugin.start(); + await plugin.setup(coreSetup); + const { createLicensePoller, license$ } = await plugin.start(); const customClient = createEsClient({ license: buildRawLicense({ type: 'gold' }), @@ -305,13 +313,19 @@ describe('licensing plugin', () => { expect(customLicense.isAvailable).toBe(true); expect(customLicense.type).toBe('gold'); - expect(await firstValueFrom(license$)).not.toBe(customLicense); + expect(await license$.pipe(take(1)).toPromise()).not.toBe(customLicense); }); it('creates a poller with a manual refresh control', async () => { + plugin = new LicensingPlugin( + coreMock.createPluginInitializerContext({ + api_polling_frequency: moment.duration(100), + }) + ); + const coreSetup = coreMock.createSetup(); - plugin.setup(coreSetup); - const { createLicensePoller } = plugin.start(); + await plugin.setup(coreSetup); + const { createLicensePoller } = await plugin.start(); const customClient = createEsClient({ license: buildRawLicense({ type: 'gold' }), @@ -330,10 +344,24 @@ describe('licensing plugin', () => { }); describe('extends core contexts', () => { + let plugin: LicensingPlugin; + + beforeEach(() => { + plugin = new LicensingPlugin( + coreMock.createPluginInitializerContext({ + api_polling_frequency: moment.duration(100), + }) + ); + }); + + afterEach(async () => { + await plugin.stop(); + }); + it('provides a licensing context to http routes', async () => { const coreSetup = coreMock.createSetup(); - plugin.setup(coreSetup); + await plugin.setup(coreSetup); expect(coreSetup.http.registerRouteHandlerContext.mock.calls).toMatchInlineSnapshot(` Array [ @@ -347,10 +375,22 @@ describe('licensing plugin', () => { }); describe('registers on pre-response interceptor', () => { + let plugin: LicensingPlugin; + + beforeEach(() => { + plugin = new LicensingPlugin( + coreMock.createPluginInitializerContext({ api_polling_frequency: moment.duration(100) }) + ); + }); + + afterEach(async () => { + await plugin.stop(); + }); + it('once', async () => { const coreSetup = coreMock.createSetup(); - plugin.setup(coreSetup); + await plugin.setup(coreSetup); expect(coreSetup.http.registerOnPreResponse).toHaveBeenCalledTimes(1); }); @@ -359,9 +399,14 @@ describe('licensing plugin', () => { describe('#stop', () => { it('stops polling', async () => { + const plugin = new LicensingPlugin( + coreMock.createPluginInitializerContext({ + api_polling_frequency: moment.duration(100), + }) + ); const coreSetup = coreMock.createSetup(); - plugin.setup(coreSetup); - const { license$ } = plugin.start(); + await plugin.setup(coreSetup); + const { license$ } = await plugin.start(); let completed = false; license$.subscribe({ complete: () => (completed = true) }); diff --git a/x-pack/plugins/licensing/server/plugin.ts b/x-pack/plugins/licensing/server/plugin.ts index b3ac583e7c81e..0d21cd689bf46 100644 --- a/x-pack/plugins/licensing/server/plugin.ts +++ b/x-pack/plugins/licensing/server/plugin.ts @@ -8,7 +8,12 @@ import type { Observable, Subject, Subscription } from 'rxjs'; import { ReplaySubject, timer } from 'rxjs'; import moment from 'moment'; +import { createHash } from 'crypto'; +import stringify from 'json-stable-stringify'; + +import type * as estypes from '@elastic/elasticsearch/lib/api/typesWithBodyKey'; import type { MaybePromise } from '@kbn/utility-types'; +import { isPromise } from '@kbn/std'; import type { CoreSetup, Logger, @@ -16,17 +21,73 @@ import type { PluginInitializerContext, IClusterClient, } from '@kbn/core/server'; + import { registerAnalyticsContextProvider } from '../common/register_analytics_context_provider'; -import type { ILicense } from '../common/types'; +import type { + ILicense, + PublicLicense, + PublicFeatures, + LicenseType, + LicenseStatus, +} from '../common/types'; import type { LicensingPluginSetup, LicensingPluginStart } from './types'; +import { License } from '../common/license'; import { createLicenseUpdate } from '../common/license_update'; + +import type { ElasticsearchError } from './types'; import { registerRoutes } from './routes'; import { FeatureUsageService } from './services'; + import type { LicenseConfigType } from './licensing_config'; import { createRouteHandlerContext } from './licensing_route_handler_context'; import { createOnPreResponseHandler } from './on_pre_response_handler'; import { getPluginStatus$ } from './plugin_status'; -import { getLicenseFetcher } from './license_fetcher'; + +function normalizeServerLicense( + license: estypes.XpackInfoMinimalLicenseInformation +): PublicLicense { + return { + uid: license.uid, + type: license.type as LicenseType, + mode: license.mode as LicenseType, + expiryDateInMillis: + typeof license.expiry_date_in_millis === 'string' + ? parseInt(license.expiry_date_in_millis, 10) + : license.expiry_date_in_millis, + status: license.status as LicenseStatus, + }; +} + +function normalizeFeatures(rawFeatures: estypes.XpackInfoFeatures) { + const features: PublicFeatures = {}; + for (const [name, feature] of Object.entries(rawFeatures)) { + features[name] = { + isAvailable: feature.available, + isEnabled: feature.enabled, + }; + } + return features; +} + +function sign({ + license, + features, + error, +}: { + license?: PublicLicense; + features?: PublicFeatures; + error?: string; +}) { + return createHash('sha256') + .update( + stringify({ + license, + features, + error, + }) + ) + .digest('hex'); +} /** * @public @@ -92,16 +153,9 @@ export class LicensingPlugin implements Plugin + this.fetchLicense(clusterClient) ); this.loggingSubscription = license$.subscribe((license) => @@ -124,6 +178,50 @@ export class LicensingPlugin implements Plugin): Promise => { + const client = isPromise(clusterClient) ? await clusterClient : clusterClient; + try { + const response = await client.asInternalUser.xpack.info(); + const normalizedLicense = + response.license && response.license.type !== 'missing' + ? normalizeServerLicense(response.license) + : undefined; + const normalizedFeatures = response.features + ? normalizeFeatures(response.features) + : undefined; + + const signature = sign({ + license: normalizedLicense, + features: normalizedFeatures, + error: '', + }); + + return new License({ + license: normalizedLicense, + features: normalizedFeatures, + signature, + }); + } catch (error) { + this.logger.warn( + `License information could not be obtained from Elasticsearch due to ${error} error` + ); + const errorMessage = this.getErrorMessage(error); + const signature = sign({ error: errorMessage }); + + return new License({ + error: this.getErrorMessage(error), + signature, + }); + } + }; + + private getErrorMessage(error: ElasticsearchError): string { + if (error.status === 400) { + return 'X-Pack plugin is not installed on the Elasticsearch cluster.'; + } + return error.message; + } + public start() { if (!this.refresh || !this.license$) { throw new Error('Setup has not been completed'); diff --git a/x-pack/plugins/licensing/server/types.ts b/x-pack/plugins/licensing/server/types.ts index fcccdecb66c00..83b39cb663715 100644 --- a/x-pack/plugins/licensing/server/types.ts +++ b/x-pack/plugins/licensing/server/types.ts @@ -14,8 +14,6 @@ export interface ElasticsearchError extends Error { status?: number; } -export type LicenseFetcher = () => Promise; - /** * Result from remote request fetching raw feature set. * @internal diff --git a/x-pack/plugins/licensing/tsconfig.json b/x-pack/plugins/licensing/tsconfig.json index 1deb735f99466..323f77b3b0ebc 100644 --- a/x-pack/plugins/licensing/tsconfig.json +++ b/x-pack/plugins/licensing/tsconfig.json @@ -15,8 +15,7 @@ "@kbn/i18n", "@kbn/analytics-client", "@kbn/subscription-tracking", - "@kbn/core-analytics-browser", - "@kbn/logging-mocks" + "@kbn/core-analytics-browser" ], "exclude": ["target/**/*"] }