[Security Solution] Add `ComparisonSide` component #189384

nikitaindik · 2024-07-29T13:02:57Z

Partially addresses: #171520

Summary

This PR adds the ComparisonSide component for the ThreeWayDiff UI (see it on the Miro diagram).

ComparisonSide lets the user compare field values from the two selected rule versions. It will be displayed on the left side of the upgrade flyout.

You can view and test it in Storybook by running yarn storybook security_solution in the root Kibana dir. Go to http://localhost:9001 once the Storybook is up and running.

comparison_side_in_storybook.mov

Also updated react-diff-view to the latest version (3.2.0 -> 3.2.1)

nikitaindik · 2024-07-29T13:11:11Z

/ci

nikitaindik · 2024-07-29T14:25:27Z

/ci

elasticmachine · 2024-07-30T08:22:13Z

Pinging @elastic/security-detections-response (Team:Detections and Resp)

elasticmachine · 2024-07-30T08:22:14Z

Pinging @elastic/security-solution (Team: SecuritySolution)

elasticmachine · 2024-07-30T08:22:15Z

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

nikitaindik · 2024-07-30T15:27:45Z

/ci

maximpn

@nikitaindik thanks for you effort in implementing ComparisonSide component. It looks nice and works well. I tested in storybooks.

There are no tests for the added component here. Do you plan to add them in a separate PR?

I left comments regarding subfield changes implementation here. Feel free to reach me for additional clarification.

maximpn · 2024-08-01T11:49:20Z

package.json

@@ -1148,7 +1148,7 @@
    "re2js": "0.4.1",
    "react": "^17.0.2",
    "react-ace": "^7.0.5",
-    "react-diff-view": "^3.2.0",
+    "react-diff-view": "^3.2.1",


Just curious if that update brings some fixes required for the feature?

It fixes a minor bug with displaying newlines at the end of the diff: https://github.com/otakustay/react-diff-view/blob/master/CHANGELOG.md#321-2024-02-18 Not a big deal for us, but I decide to update anyways.

maximpn · 2024-08-01T11:52:23Z

...tion/public/detection_engine/rule_management/components/rule_details/json_diff/diff_view.tsx

+ * @returns {string} A unified diff string representing the changes.
+ */
+const convertChangesToUnifiedDiffString = (changes: Change[]) => {
+  const unifiedDiff: string = unidiff.formatLines(changes, {


Just curious why do you use explicit typing for a constant but omit function's return type making it inferred?

Underrefactored a bit. Adding an explicit return type is a good idea! Added.

nit: TS can infer string type for unifiedDiff

Suggested change

const unifiedDiff: string = unidiff.formatLines(changes, {

const unifiedDiff = unidiff.formatLines(changes, {

maximpn · 2024-08-01T12:54:11Z

...ent/components/rule_details/three_way_diff/comparison_side/get_subfield_changes_for_field.ts

+} from '../../../../../../../common/api/detection_engine';
+import type { SubfieldChange } from './types';
+
+export const sortAndStringifyJson = (fieldValue: unknown): string => {


Suggested change

export const sortAndStringifyJson = (fieldValue: unknown): string => {

export const stringifyToStableJson = (fieldValue: unknown): string => {

As a general rule of thumb having And in function name is bad practice meaning it either has mixed responsibility and should be split or the naming isn't good.

Discussed this offline. Renamed to stringifyToSortedJson.

maximpn · 2024-08-01T12:56:27Z

...ent/components/rule_details/three_way_diff/comparison_side/get_subfield_changes_for_field.ts

Have you considered creating a folder with multiple files + an index file instead of creation ~500 lines file?

Btw name get_subfield_changes_for_field looks like overkill it looks like get_subfield_changes is enough. WDYT?

I have thought about it but wasn't sure if splitting it was worth the effort. Now since you brought that up I have refactored it into multiple files. 👍

maximpn · 2024-08-05T13:31:36Z

...anagement/components/rule_details/three_way_diff/comparison_side/comparison_side.stories.tsx

+  diff_outcome: ThreeWayDiffOutcome.StockValueCanUpdate,
+  merge_outcome: ThreeWayMergeOutcome.Target,
+  has_update: true,
+  conflict: ThreeWayDiffConflict.NONE,


nit: Shouldn't states be sorted from simple to more complex states? First states with ThreeWayDiffConflict.NONE are placed having ThreeWayDiffConflict.SOLVABLE and ThreeWayDiffConflict.NON_SOLVABLE the last.

Actually my main idea was to just see / show how different field types (single / multi-line, numbers, arrays) are rendered. I didn't pay much attention to conflict since it doesn't directly affect this component.

Input data should be accurate. You can implement some function to generate input data to mitigate such situations. The problem here is that some can use storybook's data as a reference and might be confused by an unexpected conflict value.

Input data should be accurate.

I agree. And I think it is accurate in this case.

conflict is NONE, because it's an AAB situation, so the target value is taken and there's no conflict. Isn't it?

maximpn · 2024-08-05T15:43:12Z

...ent/components/rule_details/three_way_diff/comparison_side/get_subfield_changes_for_field.ts

+export const getSubfieldChangesForDataSource = (
+  oldFieldValue?: RuleDataSource,
+  newFieldValue?: RuleDataSource
+): SubfieldChange[] => {


nit: Shouldn't Readonly<SubfieldChange[]> be used everywhere in this PR instead of SubfieldChange[]?

Made all SubfieldChange properties readonly. But we can't easily make the array of SubfieldChanges readonly because we push into an array as we build it.

maximpn · 2024-08-05T15:45:18Z

...ent/components/rule_details/three_way_diff/comparison_side/get_subfield_changes_for_field.ts

+  return stringify(fieldValue, { space: 2 });
+};
+
+export const getSubfieldChangesForDataSource = (


Does it make sense to invoke this function with both arguments undefined?

Discussed this offline. Only one arg can be undefined but describing it with TS is difficult in this situation.

maximpn · 2024-08-05T15:50:31Z

...le_management/components/rule_details/three_way_diff/comparison_side/get_subfield_changes.ts

+        oldFieldValue as RuleDataSource | undefined,
+        newFieldValue as RuleDataSource | undefined


Using as should be sparse and limited to only necessary cases. In this case TS is force to think oldFieldValue is RuleDataSource type while it can be a different type in runtime.

To get rid of as in this particular case either getSubfieldChangesForDataSource() input types should be relaxed or runtime check should be added.

Def agree this isn't the best approach. I couldn't get it to work quickly without typecasting. I would appreciate it if we could pair and improve the types here.

Discussed this offline. It turned out it's not possible to avoid typecasting since TS doesn't support narrowing union types. Added an explainer comment in code. We may want to discuss the type situation at the upcoming Tech Time.

maximpn · 2024-08-05T16:03:24Z

...e/rule_management/components/rule_details/three_way_diff/comparison_side/subfield_header.tsx

+}
+
+export function SubfieldHeader({ fieldName, subfieldName }: SubfieldHeaderProps) {
+  if (!shouldDisplaySubfieldLabelsForField(fieldName)) {


SubfieldHeader as a leaf component shouldn't know about implementation details of the other components. This logic should be a few levels up in Subfield component. It's also good to leave a comment why header isn't always shown.

maximpn · 2024-08-05T16:06:35Z

...e/rule_management/components/rule_details/three_way_diff/comparison_side/subfield_header.tsx

+
+function shouldDisplaySubfieldLabelsForField(fieldName: string): boolean {
+  return [
+    'data_source',


Hardcoded field names make it harder to maintain this functionality later on when we decide to remove or rename some fields. The good way is to specify a union of rule field names.

Thanks! Updated.

…parison-side-update

nikitaindik · 2024-08-06T17:06:48Z

/ci

nikitaindik · 2024-08-06T17:09:15Z

Thanks for your feedback @maximpn! I have addressed your comments. Feel free to take another look!

As for the tests, I plan to add them later once the base functionality is completed and the test plan is ready.

dplumlee

Component looks great @nikitaindik! This was my first time diving deep with storybooks, very cool. Left a few nits related to overall code structure but I think the refactored code looks good!

dplumlee · 2024-08-07T21:22:32Z

...ction_engine/rule_management/components/rule_details/three_way_diff/comparison_side/utils.ts

+export const stringifyToSortedJson = (fieldValue: unknown): string => {
+  if (fieldValue === undefined) {
+    return '';
+  }
+
+  if (typeof fieldValue === 'string') {
+    return fieldValue;
+  }
+
+  return stringify(fieldValue, { space: 2 });
+};


nit: we define an identical function for the per-field diff here, might be worth abstracting both of them out

Yes, it's actually the same function. I copy-pasted it from the per-field diff. I figured that copy-pasting is fine since we are going to remove the per-field diff code at some point once this new tab is ready. But since the function is very generic, perhaps it's a good idea to extract it away.

Any recommendations for a good place to put it? I couldn't find a suitable utils file. If you have a recommendation, I can move it there in the next PR.

Not exactly the same thing but we could put it in this helpers file: x-pack/plugins/security_solution/public/detection_engine/rule_management/components/rule_details/helpers.ts? I probably should've put it there initially as we have another similar function in rule_diff_tab.tsx that you originally added.

Thanks! Will add to my todo for the next PR.

dplumlee · 2024-08-07T21:32:48Z

...n_engine/rule_management/components/rule_details/three_way_diff/comparison_side/subfield.tsx

+const FIELDS_WITH_SUBFIELDS: Array<keyof DiffableAllFields> = [
+  'data_source',
+  'kql_query',
+  'eql_query',
+  'esql_query',
+  'threat_query',
+  'rule_schedule',
+  'rule_name_override',
+  'timestamp_override',
+  'timeline_template',
+  'building_block',
+  'threshold',
+];


nit: this could be put in a constants.ts file in this folder or in the above folder, we'll likely have other stuff to add to it or reference these at some point in the future (tests, etc)

maximpn

@nikitaindik thank you for addressing my comments 👍

I left a few more but these don't look critical. @dplumlee mentioned that a function producing sorted JSON already exists. It worths to reduce code duplication.

maximpn · 2024-08-08T06:47:02Z

...tion/public/detection_engine/rule_management/components/rule_details/json_diff/diff_view.tsx

+ * @returns {string} A unified diff string representing the changes.
+ */
+const convertChangesToUnifiedDiffString = (changes: Change[]) => {
+  const unifiedDiff: string = unidiff.formatLines(changes, {


nit: TS can infer string type for unifiedDiff

Suggested change

const unifiedDiff: string = unidiff.formatLines(changes, {

const unifiedDiff = unidiff.formatLines(changes, {

maximpn · 2024-08-08T06:51:00Z

...anagement/components/rule_details/three_way_diff/comparison_side/comparison_side.stories.tsx

+  diff_outcome: ThreeWayDiffOutcome.StockValueCanUpdate,
+  merge_outcome: ThreeWayMergeOutcome.Target,
+  has_update: true,
+  conflict: ThreeWayDiffConflict.NONE,


Input data should be accurate. You can implement some function to generate input data to mitigate such situations. The problem here is that some can use storybook's data as a reference and might be confused by an unexpected conflict value.

maximpn · 2024-08-08T06:54:58Z

...anagement/components/rule_details/three_way_diff/comparison_side/comparison_side.stories.tsx

+  diff_outcome: ThreeWayDiffOutcome.CustomizedValueNoUpdate,
+  merge_outcome: ThreeWayMergeOutcome.Current,
+  has_update: false,
+  conflict: ThreeWayDiffConflict.NONE,


Yes, that's correct. I double checked it with the RFC. A ThreeWayDiffConflict's comment is a bit misleading though.

maximpn · 2024-08-08T06:57:59Z

...anagement/components/rule_details/three_way_diff/comparison_side/comparison_side.stories.tsx

+    diff_outcome: ThreeWayDiffOutcome.CustomizedValueCanUpdate,
+    merge_outcome: ThreeWayMergeOutcome.Merged,
+    has_update: true,
+    conflict: ThreeWayDiffConflict.SOLVABLE,


Yes, you are right. I doubled checked the RFC and multi-line field has a solvable conflict in this case.

Btw the file you linked here doesn't exist anymore.

maximpn · 2024-08-08T07:05:39Z

...anagement/components/rule_details/three_way_diff/comparison_side/comparison_side.stories.tsx

+    diff_outcome: ThreeWayDiffOutcome.CustomizedValueCanUpdate,
+    merge_outcome: ThreeWayMergeOutcome.Merged,
+    has_update: true,
+    conflict: ThreeWayDiffConflict.SOLVABLE,


I agree it's beneficial during development phase in storybook to verify the component works with real multiline data. Though preparing and maintaining such data is tricky since spotting differences manually takes time. Let's use simplified abstract text later for testing and leave current text here unchanged.

maximpn · 2024-08-08T07:09:14Z

...agement/components/rule_details/three_way_diff/comparison_side/get_subfield_changes/index.ts

+  newFieldValue?: DiffableAllFields[FieldName]
+): SubfieldChange[] => {
+  switch (fieldName) {
+    /* Typecasting here because narrowing a union type (`DiffableAllFields[FieldName]`) is not supported by TS */


Suggested change

/* Typecasting here because narrowing a union type (`DiffableAllFields[FieldName]`) is not supported by TS */

/* Typecasting `oldFieldValue` and `newFieldValue` to corresponding field type `DiffableAllFields[*]` is required here since `oldFieldValue` and `newFieldValue` concrete types depend on `fieldName` but TS isn't track that. */

maximpn · 2024-08-08T07:12:11Z

...ction_engine/rule_management/components/rule_details/three_way_diff/comparison_side/types.ts

+ * 2.0.
+ */
+
+export interface SubfieldChange {


We discussed that it's good to define SubfieldChanges type as well looking like type SubfieldChanges = Readonly< SubfieldChange[]>;. Are there issues with that?

Yep. The issue is that if you make the array readonly, you won't be able to push into it when you construct it. But I have just figured that you can assign only the return type and it works then. Updated the code.

…parison-side-update

kibana-ci · 2024-08-08T13:59:09Z

💛 Build succeeded, but was flaky

Failed CI Steps

FTR Configs #35

Test Failures

[job] [logs] FTR Configs #35 / security app - login selector Basic functionality can login anonymously preserving original URL

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	20.7MB	20.7MB	+571.0B

History

💚 Build #226504 succeeded 70850f4
💛 Build #226131 was flaky caa065b
💚 Build #224737 succeeded d574848
💔 Build #224658 failed d574848
💚 Build #224401 succeeded 05ffa28e16a9c110ab013847843eac6a5ad35bdf
💔 Build #224373 failed 23f3a4a08ff3138e3902bb023f05d44644c8a845

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @nikitaindik

nikitaindik self-assigned this Jul 29, 2024

nikitaindik marked this pull request as ready for review July 30, 2024 08:22

nikitaindik requested a review from a team as a code owner July 30, 2024 08:22

nikitaindik requested a review from maximpn July 30, 2024 08:22

nikitaindik mentioned this pull request Jul 30, 2024

[Security Solution] Rework Update flyout to display all field updates and build Three-Way-Diff field component #171520

Open

57 tasks

nikitaindik added 4 commits July 30, 2024 15:33

Make DiffView component handle equal strings

98f7f3a

Work on ComparisonSide component

b30171a

Revert type changes

2caa0b4

Remove unused code

d574848

nikitaindik force-pushed the 3wd-comparison-side-update branch from 05ffa28 to d574848 Compare July 30, 2024 13:33

banderror requested a review from dplumlee July 31, 2024 17:59

maximpn reviewed Aug 5, 2024

View reviewed changes

nikitaindik added 2 commits August 6, 2024 12:46

Merge remote-tracking branch 'upstream/main' into nikitaindik_3wd-com…

35b841b

…parison-side-update

Address Maxim's code review comments

caa065b

nikitaindik marked this pull request as draft August 6, 2024 17:04

nikitaindik marked this pull request as ready for review August 6, 2024 17:06

nikitaindik requested a review from maximpn August 6, 2024 17:09

dplumlee approved these changes Aug 7, 2024

View reviewed changes

maximpn approved these changes Aug 8, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into nikitaindik_3wd-com…

8b2f7e2

…parison-side-update

nikitaindik marked this pull request as draft August 8, 2024 12:06

Address review feedback

70850f4

nikitaindik marked this pull request as ready for review August 8, 2024 12:09

Merge branch 'main' into 3wd-comparison-side-update

2f37996

nikitaindik enabled auto-merge (squash) August 8, 2024 12:11

nikitaindik merged commit 3de37ce into elastic:main Aug 8, 2024
46 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Aug 8, 2024

nikitaindik mentioned this pull request Aug 20, 2024

[Security Solution] Expand Rule Management team process documentation #190681

Open

	const unifiedDiff: string = unidiff.formatLines(changes, {
	const unifiedDiff = unidiff.formatLines(changes, {

	export const sortAndStringifyJson = (fieldValue: unknown): string => {
	export const stringifyToStableJson = (fieldValue: unknown): string => {

		oldFieldValue as RuleDataSource \| undefined,
		newFieldValue as RuleDataSource \| undefined

	/* Typecasting here because narrowing a union type (`DiffableAllFields[FieldName]`) is not supported by TS */
	/* Typecasting `oldFieldValue` and `newFieldValue` to corresponding field type `DiffableAllFields[]` is required here since `oldFieldValue` and `newFieldValue` concrete types depend on `fieldName` but TS isn't track that. /

[Security Solution] Add ComparisonSide component #189384

[Security Solution] Add ComparisonSide component #189384

Conversation

nikitaindik commented Jul 29, 2024 • edited Loading

Summary

nikitaindik commented Jul 29, 2024

nikitaindik commented Jul 29, 2024

elasticmachine commented Jul 30, 2024

elasticmachine commented Jul 30, 2024

elasticmachine commented Jul 30, 2024

nikitaindik commented Jul 30, 2024

maximpn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikitaindik commented Aug 6, 2024

nikitaindik commented Aug 6, 2024

dplumlee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximpn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Aug 8, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Async chunks

History

[Security Solution] Add `ComparisonSide` component #189384

[Security Solution] Add `ComparisonSide` component #189384

nikitaindik commented Jul 29, 2024 •

edited

Loading