Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solutions][Detection Engine] Creates an autocomplete package and moves duplicate code between lists and security_solution there #105382

Merged
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
b2567e7
Creates an auto-complete packaage
FrankHassanabad Jul 13, 2021
efa6d3b
Fixed i18n keys
FrankHassanabad Jul 13, 2021
31e9a2a
Merge branch 'master' into create-autocomplete-package
FrankHassanabad Jul 13, 2021
0d3ea89
Merge branch 'master' into create-autocomplete-package
FrankHassanabad Jul 13, 2021
57b65e0
Copied a few other patterns to try and get ?? to work
FrankHassanabad Jul 14, 2021
377061b
Merge branch 'master' into create-autocomplete-package
FrankHassanabad Jul 14, 2021
c08a370
Changed out the lists implementation for the autocomplete from packages
FrankHassanabad Jul 14, 2021
fb0a549
Integrated more with packages, some issues with mocks and jest tests
FrankHassanabad Jul 14, 2021
1c6c6aa
Merge branch 'master' into create-autocomplete-package
FrankHassanabad Jul 19, 2021
cd203f0
Moves the rest of the files and tests into the kbn-package and adds K…
FrankHassanabad Jul 20, 2021
70fd0cb
Merge branch 'master' into create-autocomplete-package
FrankHassanabad Jul 20, 2021
1d88059
Merge branch 'master' into create-autocomplete-package
FrankHassanabad Jul 20, 2021
80aa9bf
Merge branch 'master' into create-autocomplete-package
FrankHassanabad Jul 20, 2021
e9a32f4
Fixed i81n keys
FrankHassanabad Jul 20, 2021
08f7308
Merge branch 'master' into create-autocomplete-package
kibanamachine Jul 21, 2021
68a107d
Merge branch 'master' into create-autocomplete-package
FrankHassanabad Jul 22, 2021
cbc133f
Fixing those i18n keys (looks like I am going to conflict for a while)
FrankHassanabad Jul 22, 2021
3b86db2
Merge branch 'create-autocomplete-package' of github.com:FrankHassana…
FrankHassanabad Jul 22, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .i18nrc.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{
"paths": {
"autocomplete": "packages/kbn-securitysolution-autocomplete/src",
"console": "src/plugins/console",
"core": "src/core",
"discover": "src/plugins/discover",
Expand Down
1 change: 1 addition & 0 deletions docs/developer/getting-started/monorepo-packages.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ yarn kbn watch-bazel
- @kbn/optimizer
- @kbn/plugin-helpers
- @kbn/rule-data-utils
- @kbn/securitysolution-autocomplete
- @kbn/securitysolution-es-utils
- @kbn/securitysolution-hook-utils
- @kbn/securitysolution-io-ts-alerting-types
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@
"@kbn/mapbox-gl": "link:bazel-bin/packages/kbn-mapbox-gl",
"@kbn/monaco": "link:bazel-bin/packages/kbn-monaco",
"@kbn/rule-data-utils": "link:bazel-bin/packages/kbn-rule-data-utils",
"@kbn/securitysolution-autocomplete": "link:bazel-bin/packages/kbn-securitysolution-autocomplete",
"@kbn/securitysolution-es-utils": "link:bazel-bin/packages/kbn-securitysolution-es-utils",
"@kbn/securitysolution-hook-utils": "link:bazel-bin/packages/kbn-securitysolution-hook-utils",
"@kbn/securitysolution-io-ts-alerting-types": "link:bazel-bin/packages/kbn-securitysolution-io-ts-alerting-types",
Expand Down
1 change: 1 addition & 0 deletions packages/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ filegroup(
"//packages/kbn-plugin-generator:build",
"//packages/kbn-plugin-helpers:build",
"//packages/kbn-rule-data-utils:build",
"//packages/kbn-securitysolution-autocomplete:build",
"//packages/kbn-securitysolution-list-constants:build",
"//packages/kbn-securitysolution-io-ts-types:build",
"//packages/kbn-securitysolution-io-ts-alerting-types:build",
Expand Down
125 changes: 125 additions & 0 deletions packages/kbn-securitysolution-autocomplete/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
load("@npm//@bazel/typescript:index.bzl", "ts_config", "ts_project")
load("@build_bazel_rules_nodejs//:index.bzl", "js_library", "pkg_npm")

PKG_BASE_NAME = "kbn-securitysolution-autocomplete"

PKG_REQUIRE_NAME = "@kbn/securitysolution-autocomplete"

SOURCE_FILES = glob(
[
"src/**/*.ts",
"src/**/*.tsx"
],
exclude = [
"**/*.test.*",
"**/*.mock.*",
"**/*.mocks.*",
],
)

SRCS = SOURCE_FILES

filegroup(
name = "srcs",
srcs = SRCS,
)

NPM_MODULE_EXTRA_FILES = [
"react/package.json",
"package.json",
"README.md",
]

SRC_DEPS = [
"//packages/kbn-babel-preset",
"//packages/kbn-dev-utils",
"//packages/kbn-i18n",
"//packages/kbn-securitysolution-io-ts-list-types",
"//packages/kbn-securitysolution-list-hooks",
"@npm//@babel/core",
"@npm//babel-loader",
"@npm//@elastic/eui",
"@npm//react",
"@npm//resize-observer-polyfill",
"@npm//rxjs",
"@npm//tslib",
]

TYPES_DEPS = [
"@npm//typescript",
"@npm//@types/jest",
"@npm//@types/node",
"@npm//@types/react",
]

DEPS = SRC_DEPS + TYPES_DEPS

ts_config(
name = "tsconfig",
src = "tsconfig.json",
deps = [
"//:tsconfig.base.json",
],
)

ts_config(
name = "tsconfig_browser",
src = "tsconfig.browser.json",
deps = [
"//:tsconfig.base.json",
"//:tsconfig.browser.json",
],
)

ts_project(
name = "tsc",
args = ["--pretty"],
srcs = SRCS,
deps = DEPS,
allow_js = True,
declaration = True,
declaration_dir = "target_types",
declaration_map = True,
incremental = True,
out_dir = "target_node",
root_dir = "src",
source_map = True,
tsconfig = ":tsconfig",
)

ts_project(
name = "tsc_browser",
args = ['--pretty'],
srcs = SRCS,
deps = DEPS,
allow_js = True,
declaration = False,
incremental = True,
out_dir = "target_web",
source_map = True,
root_dir = "src",
tsconfig = ":tsconfig_browser",
)

js_library(
name = PKG_BASE_NAME,
package_name = PKG_REQUIRE_NAME,
srcs = NPM_MODULE_EXTRA_FILES,
visibility = ["//visibility:public"],
deps = [":tsc", ":tsc_browser"] + DEPS,
)

pkg_npm(
name = "npm_module",
deps = [
":%s" % PKG_BASE_NAME,
]
)

filegroup(
name = "build",
srcs = [
":npm_module",
],
visibility = ["//visibility:public"],
)
19 changes: 19 additions & 0 deletions packages/kbn-securitysolution-autocomplete/babel.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/*
* 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.
*/

module.exports = {
env: {
web: {
presets: ['@kbn/babel-preset/webpack_preset'],
},
node: {
presets: ['@kbn/babel-preset/node_preset'],
},
},
ignore: ['**/*.test.ts', '**/*.test.tsx'],
};
13 changes: 13 additions & 0 deletions packages/kbn-securitysolution-autocomplete/jest.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/*
* 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.
*/

module.exports = {
preset: '@kbn/test',
rootDir: '../..',
roots: ['<rootDir>/packages/kbn-securitysolution-autocomplete'],
};
10 changes: 10 additions & 0 deletions packages/kbn-securitysolution-autocomplete/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"name": "@kbn/securitysolution-autocomplete",
"version": "1.0.0",
"description": "Security Solution auto complete",
"license": "SSPL-1.0 OR Elastic License 2.0",
"browser": "./target_web/index.js",
"main": "./target_node/index.js",
"types": "./target_types/index.d.ts",
"private": true
}
5 changes: 5 additions & 0 deletions packages/kbn-securitysolution-autocomplete/react/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"browser": "../target_web/react",
"main": "../target_node/react",
"types": "../target_types/react/index.d.ts"
}
Original file line number Diff line number Diff line change
@@ -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 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.
*/

// Copied from "src/plugins/data/public/mocks.ts" but without any type information
// TODO: Remove this in favor of the data/public/mocks if/when they become available, https://github.com/elastic/kibana/issues/100715
export const autocompleteStartMock = {
getQuerySuggestions: jest.fn(),
getValueSuggestions: jest.fn(),
hasQuerySuggestions: jest.fn(),
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/*
* 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 { checkEmptyValue } from '.';
import { getField } from '../fields/index.mock';
import * as i18n from '../translations';

describe('check_empty_value', () => {
test('returns no errors if no field has been selected', () => {
const isValid = checkEmptyValue('', undefined, true, false);

expect(isValid).toBeUndefined();
});

test('returns error string if user has touched a required input and left empty', () => {
const isValid = checkEmptyValue(undefined, getField('@timestamp'), true, true);

expect(isValid).toEqual(i18n.FIELD_REQUIRED_ERR);
});

test('returns no errors if required input is empty but user has not yet touched it', () => {
const isValid = checkEmptyValue(undefined, getField('@timestamp'), true, false);

expect(isValid).toBeUndefined();
});

test('returns no errors if user has touched an input that is not required and left empty', () => {
const isValid = checkEmptyValue(undefined, getField('@timestamp'), false, true);

expect(isValid).toBeUndefined();
});

test('returns no errors if user has touched an input that is not required and left empty string', () => {
const isValid = checkEmptyValue('', getField('@timestamp'), false, true);

expect(isValid).toBeUndefined();
});

test('returns null if input value is not empty string or undefined', () => {
const isValid = checkEmptyValue('hellooo', getField('@timestamp'), false, true);

expect(isValid).toBeNull();
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd bring more structure to this package by separating components from utils etc. For example:

src/components/autocomplete_field
src/components/autocomplete_field_match
src/utils/fields/check_empty_value
etc

The current flat structure works since there's not too many files, but still a little bit hard to navigate.

Also, file names don't always correspond to component names, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think from this comment and the comment below, maybe we can do a follow up code sweep or discussion around file name changes, conventions?

I have been following mostly patterns seen within these folders to make things portable if we collapse between this package and the others, but I'm open if we want to do a broad cleanup/sweep of just naming changes in 1 faster PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that sounds good, if more people from the team say this kind of restructure/renaming makes sense, doing it in a separate PR either for only this one or across more packages (kbn-securitysolution-*?) would be a good idea.

* 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 * as i18n from '../translations';

// TODO: I have to use any here for now, but once this is available below, we should use the correct types, https://github.com/elastic/kibana/issues/105731
// import { IFieldType } from '../../../../../../../src/plugins/data/common';
type IFieldType = any;
Comment on lines +11 to +13
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: #103530 is merged

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, for letting me know that merged. They created a Kibana ticket for me I assigned it to myself. I have to fix these across all the packages and that will be a follow up all at once for these which I included in the comments of this PR:
#105731


/**
* Determines if empty value is ok
*/
export const checkEmptyValue = (
param: string | undefined,
field: IFieldType | undefined,
isRequired: boolean,
touched: boolean
): string | undefined | null => {
if (isRequired && touched && (param == null || param.trim() === '')) {
return i18n.FIELD_REQUIRED_ERR;
}

if (
field == null ||
(isRequired && !touched) ||
(!isRequired && (param == null || param === ''))
) {
return undefined;
}

return null;
};
Original file line number Diff line number Diff line change
@@ -1,22 +1,18 @@
/*
* 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.
* 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 React from 'react';
import { mount } from 'enzyme';
import { EuiComboBox, EuiComboBoxOptionOption } from '@elastic/eui';
import { FieldComponent } from '.';
import { fields, getField } from '../fields/index.mock';

import {
fields,
getField,
} from '../../../../../../../src/plugins/data/common/index_patterns/fields/fields.mocks';

import { FieldComponent } from './field';

describe('FieldComponent', () => {
describe('field', () => {
test('it renders disabled if "isDisabled" is true', () => {
const wrapper = mount(
<FieldComponent
Expand Down
Original file line number Diff line number Diff line change
@@ -1,17 +1,23 @@
/*
* 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.
* 2.0 and the Server Side Public License, v 1; you may not use this file except
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: personally I'm not a fan of folder-per-thing structure where inside of each folder you have a single index.ts or something like that. Makes it harder to use inside VS Code:

I'd lean towards less folders and more uniquely named files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think from this comment and the comment below, maybe we can do a follow up code sweep or discussion around file name changes, conventions?

I have been following mostly patterns seen within these folders to make things portable if we collapse between this package and the others, but I'm open if we want to do a broad cleanup/sweep of just naming changes in 1 faster PR.

* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import React, { useCallback, useMemo, useState } from 'react';
import { EuiComboBox, EuiComboBoxOptionOption } from '@elastic/eui';

import { IFieldType, IIndexPattern } from '../../../../../../../src/plugins/data/common';
// TODO: I have to use any here for now, but once this is available below, we should use the correct types, https://github.com/elastic/kibana/issues/105731
// import { IFieldType, IIndexPattern } from '../../../../../../../../src/plugins/data/common';
type IFieldType = any;
type IIndexPattern = any;

import { getGenericComboBoxProps } from './helpers';
import { GetGenericComboBoxPropsReturn } from './types';
import {
getGenericComboBoxProps,
GetGenericComboBoxPropsReturn,
} from '../get_generic_combo_box_props';

const AS_PLAIN_TEXT = { asPlainText: true };

Expand All @@ -28,13 +34,6 @@ interface OperatorProps {
selectedField: IFieldType | undefined;
}

/**
* There is a copy within:
* x-pack/plugins/security_solution/public/common/components/autocomplete/field.tsx
*
* TODO: This should be in its own packaged and not copied, https://github.com/elastic/kibana/issues/105378
* NOTE: This has deviated from the copy and will have to be reconciled.
*/
export const FieldComponent: React.FC<OperatorProps> = ({
fieldInputWidth,
fieldTypeFilter = [],
Expand Down
Loading