[Security solutions] Adds linter rule to forbid usage of no-non-null-assertion (TypeScript ! bang operator)

[FrankHassanabad]

Summary

Fixes: #114535

What this linter rule does:

• Sets the @typescript-eslint/no-non-null-assertion linter rule to become an error if seen.

If you try to use the ! operator you get an error and nice helper message that tries to encourage better practices such as this one:

Why are we deciding to set this linter rule?

• Recommended from Kibana styleguide for ~2 years now and still recommended.
• A lot of TypeScript has evolved and has operators such as ? which can replace the ! in most cases. Other cases can use a throw explicitly or other ways to manage this.
• Some types can change instead of using this operator and we should just change the types.
• TypeScript flows have improved and when we upgrade the linter will cause errors where we can remove the ! operator which is 👍 better than leaving them when they're not needed anymore.
• Newer programmers and team members sometimes mistake it for the ? when it is not the same thing.
• We have had past bugs and bugs recently because of these.
• It's easier to use the linter to find bugs than to rely on manual tests.

How did Frank fix all the 403 areas in which the linter goes off?

• Anywhere I could remove the ! operator without side effects or type script errors I just removed the ! operator.
• Anywhere in test code where I could replace the code with a ? or a throw I went through that route.
• Within the code areas (non test code) where I saw what looks like a simple bug that I could fix using a ?? [] or ? operator or String(value) or fixing a simple type I would choose that route first. These areas I marked in the code review with the words NOTE: for anyone to look at.
• Within all other areas of the code and tests where anything looked more involved I just disabled the linter for that particular line. When in doubt I chose this route.

Checklist

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

[FrankHassanabad]

NOTE: Zero risk as this is just test code, but using a throw within the tests is preferred over upstream turning off typescript assertions.

[FrankHassanabad]

NOTE: Zero risk as this is just test code, but using a String(value) will return value, using a String(null) will return the string value of null and String(undefined) will return the string undefined. All of which will fail the test.

[FrankHassanabad]

NOTE: Zero risk as this is just test code, but using a String(value) will return value, using a String(null) will return the string value of null and String(undefined) will return the string undefined. All of which will fail the test.

[FrankHassanabad]

NOTE: 🐛 Low risk fix here as we now pass an empty array if the series.value is undefined

[FrankHassanabad]

NOTE: Low risk type fix where I change this to allow an undefined so I could remove anywhere we turn off type assertions. This function should operate the same as before.

[FrankHassanabad]

NOTE: 🐛 Fix low risk, high impact here because if you look below this you see the code at line 246 which says:

skip: !canQueryTimeline || combinedQueries?.filterQuery === undefined, // When the filterQuery comes back as undefined, it means an error has been thrown and the request should be skipped

If this can be undefined then I would assume we want to allow it to be undefined here. Either way, this code written as is will BLOW UP if combinedQueries is undefined. So I figured it's best to use ? operator here.

[FrankHassanabad]

NOTE: 🐛 Low risk fix here because the ! will cause a failure if combinedQueries is undefined. This will ensure we are getting back an undefined for sure instead of blowing up.

[FrankHassanabad]

NOTE: Low risk 🐛 fix here as the has before this should guarantee it's there but the ? allows us to remove the non-assertion operator. In reality we can probably use more ? operator and remove more has in these places but I figured this would be good enough for us.

[FrankHassanabad]

NOTE: Low risk 🐛 fix here as the has before this should guarantee it's there but the ? allows us to remove the non-assertion operator. In reality we can probably use more ? operator and remove more has in these places but I figured this would be good enough for us.

[FrankHassanabad]

NOTE: Small risk 🐛 fix. I don't think we expect null/undefined but using String(null) or String(undefined) will display null or undefined but it won't blow things up. Using String(value) if value is a string will return a value.

[FrankHassanabad]

NOTE: Small risk 🐛 fix. I don't think we expect null/undefined but using String(null) or String(undefined will display null or undefined but it won't blow things up. Using String(value) if value is a string will return a value.

[FrankHassanabad]

NOTE: Small risk 🐛 fix. I don't think we expect null/undefined but using String(null) or String(undefined will display null or undefined but it won't blow things up. Using String(value) if value is a string will return a value.

[FrankHassanabad]

NOTE: Medium 🐛 fix. I don't think we expect user.updatedBy to be null but it can be either null | undefined according to its type. To be safe here I decided to check explicitly against null and then set it to undefined if it is null. Otherwise I fall back on using the regular value which is user.updatedBy.

The other way I could have fixed this was String(user.updatedBy) but since undefined can be valid key I decided to implement the fix this way.

[FrankHassanabad]

NOTE: Low risk type fix here as it seems to want it to be required and no where is it optionally being set to undefined or null so the typing and code is better to just use the expected type here.

[FrankHassanabad]

NOTE: zero risk as the @ts-ignore wasn't needed here and now we only turn off the no-non-null-assertion

[FrankHassanabad]

NOTE: Low risk 🐛 fix here since combinedQueries can be undefined and this is similar to the other areas of the code base we fixed the combinedQueries since it can be undefined and we don't want to blow up here.

[FrankHassanabad]

NOTE: Low risk 🐛 fix here since combinedQueries can be undefined and this is similar to the other areas of the code base we fixed the combinedQueries since it can be undefined and we don't want to blow up here.

[FrankHassanabad]

@elasticmachine merge upstream

[jonathan-buttner]

Is it possible to disable the no-non-null-assertion linter by default for test files? I think there'd be less risk of having the ! in a test files since the test will fail anyway if the value is undefined.

[banderror]

That's a great linting improvement 🙌 Thank you @FrankHassanabad!

I'm not familiar with most of the code that has been changed in this PR, so I'm not sure if it's legitimate for me to approve the PR, but it all looks good at first glance. I see that most of the ! in production code is left with the corresponding eslint-disable comments, so the logic should stay the same.

Specific areas of code for timelines, endpoint integration, etc should probably be reviewed closely by the corresponding owners.

Left 2 minor comments.

[FrankHassanabad]

Is it possible to disable the no-non-null-assertion linter by default for test files? I think there'd be less risk of having the ! in a test files since the test will fail anyway if the value is undefined.

Yep, totally possible. I did it by adding this chunk you should see in the PR now:

    {
      // typescript only for front and back end, but excludes the test files.
      // We use this section to add rules in which we do not want to apply to test files.
      // This should be a very small set as most linter rules are useful for tests as well.
      files: [
        'x-pack/plugins/security_solution/**/*.{ts,tsx}',
        'x-pack/plugins/timelines/**/*.{ts,tsx}',
      ],
      excludedFiles: [
        'x-pack/plugins/security_solution/**/*.{test,mock,test_helper}.{ts,tsx}',
        'x-pack/plugins/timelines/**/*.{test,mock,test_helper}.{ts,tsx}',
      ],
      rules: {
        '@typescript-eslint/no-non-null-assertion': 'error',
      },
    },

And moving the rule into there. Also I removed all the additional workaround code in the tests to make them mostly the same as they were before except for where I could remove ! for ? or remove ! completely. So 👍

[jonathan-buttner]

Is it possible to disable the no-non-null-assertion linter by default for test files? I think there'd be less risk of having the ! in a test files since the test will fail anyway if the value is undefined.

Yep, totally possible. I did it by adding this chunk you should see in the PR now:

Thanks Frank!

[FrankHassanabad]

@elasticmachine merge upstream

[jonathan-buttner]

@cnasikas @semd @jamster10 maybe we should do a follow up PR to add cases here as well?

[cnasikas]

Thanks, @jonathan-buttner! Let's do it!

[cnasikas]

I opened an issue to track it here: #115152

[jonathan-buttner]

@FrankHassanabad @MadameSheema should we exclude cypress .spec files? I'm fine with leaving them as linted.

[FrankHassanabad]

I'm fine leaving them as is.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 519f1e4
• Storybooks Preview

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	4.6MB	4.6MB	+318.0B
timelines	240.3KB	240.3KB	+32.0B
total			+350.0B

History

• 💔 Build #159958 failed 6bb0458
• 💔 Build #159808 failed 1215058
• 💔 Build #159617 failed 81c9444
• 💛 Build #159237 was flaky ad59096
• 💔 Build #159189 failed cf65c58

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

cc @FrankHassanabad

[kibanamachine]

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.