[Security Solution] Adds diff algorithm and unit tests for scalar array values

[dplumlee]

Summary

Related ticket: #180162

Adds the diff algorithm for arrays of scalar values (right now we only have fields of strings) and unit tests to cover all the base cases of what the algorithm can return.

Checklist

Delete any items that are not applicable to this PR.

• Unit or functional tests were updated or added to match the most common scenarios

[elasticmachine]

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

[elasticmachine]

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

[elasticmachine]

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

[jpdjere]

This is the first algorithm in which we actually need to change the way in which we calculate how we determine which scenario we are in.

Until now we have been comparing values A, B and C with LoDash's isEqual, but now that we're dealing with array of scalars we need to take into account that the order of the elements of the array should't matter. Consider this unit test, which passes with the code as is now:

  it.only('returns custom merged version as merged output if all three versions are different', () => {
    const mockVersions: ThreeVersionsOf<string[]> = {
      base_version: ['A', 'B', 'C'],
      current_version: ['C', 'A', 'B'],
      target_version: ['B', 'C', 'A'],
    };
    const expectedMergedVersion = ['A', 'B', 'C'];

    const result = scalarArrayDiffAlgorithm(mockVersions);

    expect(result).toEqual(
      expect.objectContaining({
        has_update: true,
        merged_version: expectedMergedVersion,
        diff_outcome: ThreeWayDiffOutcome.CustomizedValueCanUpdate,
        merge_outcome: ThreeWayMergeOutcome.Merged,
        has_conflict: false,
      })
    );

Base, current and target are "different" when considered with LoDash's isEqual but a reordering of the scalar values should not mean that the arrays have changed; semantically, they are the same for Kibana.

In the test above, has_update should not be true but false and the diff_outcome should actually be ThreeWayDiffOutcome.StockValueNoUpdate. And this applies to all situation where a different order leads to an "incorrect" calculation of the ThreeWayDiffOutcome.

So we need to update (or create a new specific) determineDiffOutcome function in x-pack/plugins/security_solution/common/api/detection_engine/prebuilt_rules/model/diff/three_way_diff/three_way_diff_outcome.ts to update the isEqual to something specific to the case of arrays of scalar values, which checks that the two arrays contain the same scalar values, independently from their order.

---

On a side note, for the case in the test above, I think that we should be returning the current_version as the merged version; that is, we give "preference" to the reordering of the scalar values that the user created, not the one Elastic created or the original base version.

[dplumlee]

@jpdjere great catch, I've added another determineDiffOutcome function to handle that case (we can still fall back on the other one if there are other fields that are order specific) and factored that into some of the unit tests.

And to your side note, we already return the current_version in cases like that so this logic should slot right in

[jpdjere]

Thanks for adding these 🙇‍♂️

[jpdjere]

Hi @approksiu, one thing which is UX related here and we'd like your opinion on:

Currently, we have "mixed" behaviour when dealing with duplicated values in arrays. Take for example, the tags field; if a rule has as its tags the array: ['A', 'B', 'C'].

• Via the UI, in the Rule Editing page, you cannot add 'A' again. The UI blocks you from doing it.
• Via the UI, in the Rules Managament table, when using Bulk Editing to add tags:
  • you cannot add to the selected rules a duplicated value in an array. So if you want to add ['A', 'B', 'C', 'A'] to the selected rules, the second 'A' gets blocked from being added by the UI.
  • if a rule already has 'A' as its tag and you want to add it again, the UI doesn't block you from it (since the addition can apply to multiple rules, some of which may not have the 'A' tag yet), but the API handles the issue by skipping the update for any rules that already have that tag. So for example:
    • if a rule has ['A', 'B', 'C'] and another one has ['B'] and you want to add 'A' to both, the first rule will be skipped and the second one will succeed.
    • if a rule has ['A', 'B', 'C'] and another one has ['B'] and you want to add 'A', 'Z' to both, both will succeed, but 'A' won't be duplicated in the end result of the first. So we actually do de-duplication here.
• Via the API, you can replace the whole array with ['A', 'B', 'C', 'A']. The request is successful and the 'A' does not get de-duplicated.

So the question is how do we want to deal with possible duplicated values in the update workflow: if a rule has as its current version ['A', 'B', 'C', 'A'], and the target version is ['A', 'B', 'C'], do we want to consider these to values to be equal, or not?

My own personal opinion is that we should de-duplicate arrays before comparing them to another version, since duplicate values don't serve any real purpose, at least from the Kibana point of view. If they exist, they seem to be more mistakes on the customer side than actual intended values.

And not de-duplicating these values before starting the upgrade workflow would add more overhead on the client's side, and more decisions to make when upgrading a rule, which is something I think we should avoid, and rather simplify the process as much as we can. Consider the scenario for tags:

base: `['A', 'B']`
current: ['A', 'B', 'C', 'A']`
target: ['A', 'B', 'C']

Without de-duplication, this would lead to a conflict because the three versions are different. However, current and target are semantically the same (no difference for Kibana), and we could avoid the conflict here by deduplicating, since current and target would match. This would mean one less problem for the user.

What do you think?

cc @dplumlee @banderror

[approksiu]

@jpdjere I think deduplication before comparing makes sense. Additional question - is comparison case-sensitive or not?

[jpdjere]

@approksiu Good question - it NOT case sensitive, meaning that we consider the tag "Windows" to be the same as "WINDOWS" and all the same behaviours I described above apply with this consideration in mind.

[dplumlee]

@jpdjere I would agree with that too, I looked through the fields and don't believe any of the proposed fields listed here would have any impact with duplicate values. I don't believe case sensitivity would matter for any of those fields either

[jpdjere]

@dplumlee Sorry for the delay in re-reviewing. Just took another look and we need to account for the fact that our arrays should be case-insensitive when dealing with strings, as @approksiu mentioned. Something like:

const lowercaseIfString = (val: unknown): unknown => {
  return typeof val === 'string' ? val.toLowerCase() : val;
};

const arraysHaveSameElements = <T>(arr1: T[], arr2: T[]) => {
  const set1 = new Set(arr1.map((val) => lowercaseIfString(val)));
  const set2 = new Set(arr2.map((val) => lowercaseIfString(val)));

  if (set1.size !== set2.size) {
    return false;
  }

  for (const val of set1) {
    if (!set2.has(val)) {
      return false;
    }
  }

  return true;
};

And we should add some tests covering this behaviour as well.

[jpdjere]

LGTM ✅ - thanks for addressing all the comments!

[banderror]

@dplumlee @jpdjere I have a concern about case sensitivity. Posting it right away and continuing the review.

[banderror]

Nit: For now, we could restrict elements to be either strings or numbers. There's a chance we'll never need to support other primitive types.

Suggested change

	export const scalarArrayDiffAlgorithm = <TValue>(
	export const scalarArrayDiffAlgorithm = <TValue = string | number>(

[banderror]

@elasticmachine merge upstream

[banderror]

LGTM, thanks for optimizing the implementation and making the test coverage comprehensive. It looks clean and feels very robust now.

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: 9c05138

Metrics [docs]

✅ unchanged

History

• 💚 Build #218871 succeeded 5c64bd2
• 💛 Build #218315 was flaky 916225e
• 💛 Build #217572 was flaky 876a952
• 💛 Build #217062 was flaky e63fc0a
• 💔 Build #217002 failed 65faf40
• 💛 Build #216215 was flaky 65b62a5

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

cc @dplumlee