Handled missing base version scenarios

elastic · Jul 22, 2024 · e1d3311 · e1d3311
1 parent a96ca38
commit e1d3311
Show file tree

Hide file tree

Showing 10 changed files with 108 additions and 165 deletions.
diff --git a/x-pack/plugins/security_solution/common/api/detection_engine/prebuilt_rules/index.ts b/x-pack/plugins/security_solution/common/api/detection_engine/prebuilt_rules/index.ts
@@ -21,4 +21,3 @@ export * from './model/diff/rule_diff/rule_diff';
 export * from './model/diff/three_way_diff/three_way_diff_outcome';
 export * from './model/diff/three_way_diff/three_way_diff';
 export * from './model/diff/three_way_diff/three_way_diff_conflict';
-export * from './model/diff/three_way_diff/three_way_merge_outcome';
diff --git a/...on/common/api/detection_engine/prebuilt_rules/model/diff/three_way_diff/three_way_diff.ts b/...on/common/api/detection_engine/prebuilt_rules/model/diff/three_way_diff/three_way_diff.ts
@@ -6,7 +6,6 @@
  */
 
 import type { ThreeWayDiffOutcome } from './three_way_diff_outcome';
-import type { ThreeWayMergeOutcome } from './three_way_merge_outcome';
 import type { ThreeWayDiffConflict } from './three_way_diff_conflict';
 
 /**
@@ -111,14 +110,6 @@ export interface ThreeWayDiff<TValue> {
    */
   diff_outcome: ThreeWayDiffOutcome;
 
-  /**
-   * The type of result of an automatic three-way merge of three values:
-   *   - base version
-   *   - current version
-   *   - target version
-   */
-  merge_outcome: ThreeWayMergeOutcome;
-
   /**
    * Boolean which determines if a base version was found and returned for the three-way-diff of the field
    * - true: the base version of the field was found and is either defined or undefined

diff --git a/.../api/detection_engine/prebuilt_rules/model/diff/three_way_diff/three_way_merge_outcome.ts b/.../api/detection_engine/prebuilt_rules/model/diff/three_way_diff/three_way_merge_outcome.ts
diff --git a/...prebuilt_rules/logic/diff/calculation/algorithms/multi_line_string_diff_algorithm.test.ts b/...prebuilt_rules/logic/diff/calculation/algorithms/multi_line_string_diff_algorithm.test.ts
@@ -8,7 +8,6 @@
 import type { ThreeVersionsOf } from '../../../../../../../../common/api/detection_engine';
 import {
   ThreeWayDiffOutcome,
-  ThreeWayMergeOutcome,
   MissingVersion,
   ThreeWayDiffConflict,
 } from '../../../../../../../../common/api/detection_engine';
@@ -28,7 +27,6 @@ describe('multiLineStringDiffAlgorithm', () => {
       expect.objectContaining({
         merged_version: mockVersions.current_version,
         diff_outcome: ThreeWayDiffOutcome.StockValueNoUpdate,
-        merge_outcome: ThreeWayMergeOutcome.Current,
         conflict: ThreeWayDiffConflict.NONE,
       })
     );
@@ -47,7 +45,6 @@ describe('multiLineStringDiffAlgorithm', () => {
       expect.objectContaining({
         merged_version: mockVersions.current_version,
         diff_outcome: ThreeWayDiffOutcome.CustomizedValueNoUpdate,
-        merge_outcome: ThreeWayMergeOutcome.Current,
         conflict: ThreeWayDiffConflict.NONE,
       })
     );
@@ -66,7 +63,6 @@ describe('multiLineStringDiffAlgorithm', () => {
       expect.objectContaining({
         merged_version: mockVersions.target_version,
         diff_outcome: ThreeWayDiffOutcome.StockValueCanUpdate,
-        merge_outcome: ThreeWayMergeOutcome.Target,
         conflict: ThreeWayDiffConflict.NONE,
       })
     );
@@ -85,7 +81,6 @@ describe('multiLineStringDiffAlgorithm', () => {
       expect.objectContaining({
         merged_version: mockVersions.current_version,
         diff_outcome: ThreeWayDiffOutcome.CustomizedValueSameUpdate,
-        merge_outcome: ThreeWayMergeOutcome.Current,
         conflict: ThreeWayDiffConflict.NONE,
       })
     );
@@ -107,7 +102,6 @@ describe('multiLineStringDiffAlgorithm', () => {
         expect.objectContaining({
           merged_version: expectedMergedVersion,
           diff_outcome: ThreeWayDiffOutcome.CustomizedValueCanUpdate,
-          merge_outcome: ThreeWayMergeOutcome.Merged,
           conflict: ThreeWayDiffConflict.SOLVABLE,
         })
       );
@@ -126,7 +120,6 @@ describe('multiLineStringDiffAlgorithm', () => {
         expect.objectContaining({
           merged_version: mockVersions.current_version,
           diff_outcome: ThreeWayDiffOutcome.CustomizedValueCanUpdate,
-          merge_outcome: ThreeWayMergeOutcome.Conflict,
           conflict: ThreeWayDiffConflict.NON_SOLVABLE,
         })
       );
@@ -146,10 +139,9 @@ describe('multiLineStringDiffAlgorithm', () => {
       expect(result).toEqual(
         expect.objectContaining({
           has_base_version: false,
-          base_version: MissingVersion,
+          base_version: undefined,
           merged_version: mockVersions.current_version,
           diff_outcome: ThreeWayDiffOutcome.StockValueNoUpdate,
-          merge_outcome: ThreeWayMergeOutcome.Current,
           conflict: ThreeWayDiffConflict.NONE,
         })
       );
@@ -167,11 +159,10 @@ describe('multiLineStringDiffAlgorithm', () => {
       expect(result).toEqual(
         expect.objectContaining({
           has_base_version: false,
-          base_version: MissingVersion,
+          base_version: undefined,
           merged_version: mockVersions.target_version,
           diff_outcome: ThreeWayDiffOutcome.StockValueCanUpdate,
-          merge_outcome: ThreeWayMergeOutcome.Target,
-          conflict: ThreeWayDiffConflict.NONE,
+          conflict: ThreeWayDiffConflict.SOLVABLE,
         })
       );
     });

diff --git a/...gine/prebuilt_rules/logic/diff/calculation/algorithms/multi_line_string_diff_algorithm.ts b/...gine/prebuilt_rules/logic/diff/calculation/algorithms/multi_line_string_diff_algorithm.ts
@@ -15,7 +15,6 @@ import {
   determineDiffOutcome,
   determineIfValueCanUpdate,
   ThreeWayDiffOutcome,
-  ThreeWayMergeOutcome,
   MissingVersion,
   ThreeWayDiffConflict,
 } from '../../../../../../../../common/api/detection_engine/prebuilt_rules';
@@ -35,13 +34,15 @@ export const multiLineStringDiffAlgorithm = (
   const diffOutcome = determineDiffOutcome(baseVersion, currentVersion, targetVersion);
   const valueCanUpdate = determineIfValueCanUpdate(diffOutcome);
 
-  const { mergeOutcome, mergedVersion } = mergeVersions({
+  const hasBaseVersion = baseVersion !== MissingVersion;
+
+  const { conflict, mergedVersion } = mergeVersions({
+    hasBaseVersion,
     baseVersion,
     currentVersion,
     targetVersion,
     diffOutcome,
   });
-  const hasBaseVersion = baseVersion !== MissingVersion;
 
   return {
     has_base_version: hasBaseVersion,
@@ -51,77 +52,77 @@ export const multiLineStringDiffAlgorithm = (
     merged_version: mergedVersion,
 
     diff_outcome: diffOutcome,
-    merge_outcome: mergeOutcome,
+    conflict,
     has_update: valueCanUpdate,
-    conflict: determineConflictType(mergeOutcome),
   };
 };
 
 interface MergeResult {
-  mergeOutcome: ThreeWayMergeOutcome;
   mergedVersion: string;
+  conflict: ThreeWayDiffConflict;
 }
 
 interface MergeArgs {
+  hasBaseVersion: boolean;
   baseVersion: string | MissingVersion;
   currentVersion: string;
   targetVersion: string;
   diffOutcome: ThreeWayDiffOutcome;
 }
 
 const mergeVersions = ({
+  hasBaseVersion,
   baseVersion,
   currentVersion,
   targetVersion,
   diffOutcome,
 }: MergeArgs): MergeResult => {
   switch (diffOutcome) {
-    case ThreeWayDiffOutcome.StockValueNoUpdate:
-    case ThreeWayDiffOutcome.CustomizedValueNoUpdate:
-    case ThreeWayDiffOutcome.CustomizedValueSameUpdate: {
+    case ThreeWayDiffOutcome.StockValueNoUpdate: // Scenarios AAA and -AA
+    case ThreeWayDiffOutcome.CustomizedValueNoUpdate: // Scenario ABA
+    case ThreeWayDiffOutcome.CustomizedValueSameUpdate: // Scenario ABB
       return {
-        mergeOutcome: ThreeWayMergeOutcome.Current,
+        conflict: ThreeWayDiffConflict.NONE,
         mergedVersion: currentVersion,
       };
-    }
+
     case ThreeWayDiffOutcome.StockValueCanUpdate: {
+      if (!hasBaseVersion) {
+        // Scenario -AB. Treated as scenario ABC, returns target
+        // version and marked as "SOLVABLE" conflict.
+        // https://github.com/elastic/kibana/pull/184889#discussion_r1636421293
+        return {
+          mergedVersion: targetVersion,
+          conflict: ThreeWayDiffConflict.SOLVABLE,
+        };
+      }
+
+      // Scenario AAB
       return {
-        mergeOutcome: ThreeWayMergeOutcome.Target,
+        conflict: ThreeWayDiffConflict.NONE,
         mergedVersion: targetVersion,
       };
     }
+
+    // Scenario ABC
     case ThreeWayDiffOutcome.CustomizedValueCanUpdate: {
-      if (baseVersion === MissingVersion) {
-        return {
-          mergeOutcome: ThreeWayMergeOutcome.Conflict,
-          mergedVersion: currentVersion,
-        };
-      }
-      const mergedVersion = merge(currentVersion, baseVersion, targetVersion, {
+      // TS does not realize that in ABC scenario, baseVersion cannot be missing
+      // Missing baseVersion scenarios were handled as -AA and -AB.
+      const mergedVersion = merge(currentVersion, baseVersion as string, targetVersion, {
         stringSeparator: /(\S+|\s+)/g, // Retains all whitespace, which we keep to preserve formatting
       });
 
       return mergedVersion.conflict
         ? {
-            mergeOutcome: ThreeWayMergeOutcome.Conflict,
+            conflict: ThreeWayDiffConflict.NON_SOLVABLE,
             mergedVersion: currentVersion,
           }
         : {
-            mergeOutcome: ThreeWayMergeOutcome.Merged,
+            conflict: ThreeWayDiffConflict.SOLVABLE,
             mergedVersion: mergedVersion.result.join(''),
           };
     }
     default:
       return assertUnreachable(diffOutcome);
   }
 };
-
-const determineConflictType = (mergeOutcome: ThreeWayMergeOutcome) => {
-  if (mergeOutcome === ThreeWayMergeOutcome.Conflict) {
-    return ThreeWayDiffConflict.NON_SOLVABLE;
-  }
-  if (mergeOutcome === ThreeWayMergeOutcome.Merged) {
-    return ThreeWayDiffConflict.SOLVABLE;
-  }
-  return ThreeWayDiffConflict.NONE;
-};
diff --git a/...ion_engine/prebuilt_rules/logic/diff/calculation/algorithms/number_diff_algorithm.test.ts b/...ion_engine/prebuilt_rules/logic/diff/calculation/algorithms/number_diff_algorithm.test.ts
@@ -8,7 +8,6 @@
 import type { ThreeVersionsOf } from '../../../../../../../../common/api/detection_engine';
 import {
   ThreeWayDiffOutcome,
-  ThreeWayMergeOutcome,
   MissingVersion,
   ThreeWayDiffConflict,
 } from '../../../../../../../../common/api/detection_engine';
@@ -28,7 +27,6 @@ describe('numberDiffAlgorithm', () => {
       expect.objectContaining({
         merged_version: mockVersions.current_version,
         diff_outcome: ThreeWayDiffOutcome.StockValueNoUpdate,
-        merge_outcome: ThreeWayMergeOutcome.Current,
         conflict: ThreeWayDiffConflict.NONE,
       })
     );
@@ -47,7 +45,6 @@ describe('numberDiffAlgorithm', () => {
       expect.objectContaining({
         merged_version: mockVersions.current_version,
         diff_outcome: ThreeWayDiffOutcome.CustomizedValueNoUpdate,
-        merge_outcome: ThreeWayMergeOutcome.Current,
         conflict: ThreeWayDiffConflict.NONE,
       })
     );
@@ -66,7 +63,6 @@ describe('numberDiffAlgorithm', () => {
       expect.objectContaining({
         merged_version: mockVersions.target_version,
         diff_outcome: ThreeWayDiffOutcome.StockValueCanUpdate,
-        merge_outcome: ThreeWayMergeOutcome.Target,
         conflict: ThreeWayDiffConflict.NONE,
       })
     );
@@ -85,7 +81,6 @@ describe('numberDiffAlgorithm', () => {
       expect.objectContaining({
         merged_version: mockVersions.current_version,
         diff_outcome: ThreeWayDiffOutcome.CustomizedValueSameUpdate,
-        merge_outcome: ThreeWayMergeOutcome.Current,
         conflict: ThreeWayDiffConflict.NONE,
       })
     );
@@ -104,7 +99,6 @@ describe('numberDiffAlgorithm', () => {
       expect.objectContaining({
         merged_version: mockVersions.current_version,
         diff_outcome: ThreeWayDiffOutcome.CustomizedValueCanUpdate,
-        merge_outcome: ThreeWayMergeOutcome.Conflict,
         conflict: ThreeWayDiffConflict.NON_SOLVABLE,
       })
     );
@@ -123,10 +117,9 @@ describe('numberDiffAlgorithm', () => {
       expect(result).toEqual(
         expect.objectContaining({
           has_base_version: false,
-          base_version: MissingVersion,
+          base_version: undefined,
           merged_version: mockVersions.current_version,
           diff_outcome: ThreeWayDiffOutcome.StockValueNoUpdate,
-          merge_outcome: ThreeWayMergeOutcome.Current,
           conflict: ThreeWayDiffConflict.NONE,
         })
       );
@@ -144,11 +137,10 @@ describe('numberDiffAlgorithm', () => {
       expect(result).toEqual(
         expect.objectContaining({
           has_base_version: false,
-          base_version: MissingVersion,
+          base_version: undefined,
           merged_version: mockVersions.target_version,
           diff_outcome: ThreeWayDiffOutcome.StockValueCanUpdate,
-          merge_outcome: ThreeWayMergeOutcome.Target,
-          conflict: ThreeWayDiffConflict.NONE,
+          conflict: ThreeWayDiffConflict.SOLVABLE,
         })
       );
     });