Skip to content

Commit

Permalink
[Security solutions] Adds linter rule to forbid usage of no-non-null-…
Browse files Browse the repository at this point in the history
…assertion (TypeScript ! bang operator) (elastic#114375) (elastic#115126)

## Summary

Fixes: elastic#114535

**What this linter rule does:**
* Sets the [@typescript-eslint/no-non-null-assertion](https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-non-null-assertion.md) 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:

<img width="1635" alt="Screen Shot 2021-10-07 at 11 26 14 AM" src="https://user-images.githubusercontent.com/1151048/136474207-f38d3461-0af9-4cdc-885b-632cb49d8a24.png">

**Why are we deciding to set this linter rule?**
* Recommended from Kibana [styleguide](https://github.com/elastic/kibana/blob/master/STYLEGUIDE.mdx#avoid-non-null-assertions) 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

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

Co-authored-by: Frank Hassanabad <frank.hassanabad@elastic.co>
  • Loading branch information
kibanamachine and FrankHassanabad authored Oct 15, 2021
1 parent a1a7550 commit d2075f6
Show file tree
Hide file tree
Showing 149 changed files with 367 additions and 238 deletions.
38 changes: 35 additions & 3 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -902,7 +902,12 @@ module.exports = {
},

/**
* Security Solution overrides
* Security Solution overrides. These rules below are maintained and owned by
* the people within the security-solution-platform team. Please see ping them
* or check with them if you are encountering issues, have suggestions, or would
* like to add, change, or remove any particular rule. Linters, Typescript, and rules
* evolve and change over time just like coding styles, so please do not hesitate to
* reach out.
*/
{
// front end and common typescript and javascript files only
Expand All @@ -925,6 +930,22 @@ module.exports = {
],
},
},
{
// 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',
},
},
{
// typescript only for front and back end
files: [
Expand Down Expand Up @@ -1043,7 +1064,12 @@ module.exports = {
},

/**
* Lists overrides
* Lists overrides. These rules below are maintained and owned by
* the people within the security-solution-platform team. Please see ping them
* or check with them if you are encountering issues, have suggestions, or would
* like to add, change, or remove any particular rule. Linters, Typescript, and rules
* evolve and change over time just like coding styles, so please do not hesitate to
* reach out.
*/
{
// front end and common typescript and javascript files only
Expand Down Expand Up @@ -1218,8 +1244,14 @@ module.exports = {
],
},
},

/**
* Metrics entities overrides
* Metrics entities overrides. These rules below are maintained and owned by
* the people within the security-solution-platform team. Please see ping them
* or check with them if you are encountering issues, have suggestions, or would
* like to add, change, or remove any particular rule. Linters, Typescript, and rules
* evolve and change over time just like coding styles, so please do not hesitate to
* reach out.
*/
{
// front end and common typescript and javascript files only
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ export const normalizeThresholdField = (
? thresholdField
: isEmpty(thresholdField)
? []
: [thresholdField!];
: // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
[thresholdField!];
};

export const normalizeThresholdObject = (threshold: Threshold): ThresholdNormalized => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ export async function indexEndpointHostDocs({
const indexedAgentResponse = await indexFleetAgentForHost(
client,
kbnClient,
hostMetadata!,
hostMetadata,
realPolicies[appliedPolicyId].policy_id,
kibanaVersion
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ export const indexFleetAgentForHost = async (
.index<FleetServerAgent>({
index: agentDoc._index,
id: agentDoc._id,
body: agentDoc._source!,
body: agentDoc._source,
op_type: 'create',
})
.catch(wrapErrorAndRejectPromise);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,11 +89,11 @@ describe('Cases connectors', () => {
addServiceNowConnector(snConnector);

cy.wait('@createConnector').then(({ response }) => {
cy.wrap(response!.statusCode).should('eql', 200);
cy.wrap(response?.statusCode).should('eql', 200);
cy.get(TOASTER).should('have.text', "Created 'New connector'");
cy.get(TOASTER).should('not.exist');

selectLastConnectorCreated(response!.body.id);
selectLastConnectorCreated(response?.body.id);

cy.wait('@saveConnector', { timeout: 10000 }).its('response.statusCode').should('eql', 200);
cy.get(SERVICE_NOW_MAPPING).first().should('have.text', 'short_description');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -351,10 +351,10 @@ describe('Custom detection rules deletion and edition', () => {
goToRuleDetails();

cy.wait('@fetchRuleDetails').then(({ response }) => {
cy.wrap(response!.statusCode).should('eql', 200);
cy.wrap(response?.statusCode).should('eql', 200);

cy.wrap(response!.body.max_signals).should('eql', getExistingRule().maxSignals);
cy.wrap(response!.body.enabled).should('eql', false);
cy.wrap(response?.body.max_signals).should('eql', getExistingRule().maxSignals);
cy.wrap(response?.body.enabled).should('eql', false);
});
});

Expand Down Expand Up @@ -415,9 +415,9 @@ describe('Custom detection rules deletion and edition', () => {
saveEditedRule();

cy.wait('@getRule').then(({ response }) => {
cy.wrap(response!.statusCode).should('eql', 200);
cy.wrap(response?.statusCode).should('eql', 200);
// ensure that editing rule does not modify max_signals
cy.wrap(response!.body.max_signals).should('eql', getExistingRule().maxSignals);
cy.wrap(response?.body.max_signals).should('eql', getExistingRule().maxSignals);
});

cy.get(RULE_NAME_HEADER).should('contain', `${getEditedRule().name}`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ describe('Export rules', () => {
goToManageAlertsDetectionRules();
exportFirstRule();
cy.wait('@export').then(({ response }) => {
cy.wrap(response!.body).should('eql', expectedExportedRule(this.ruleResponse));
cy.wrap(response?.body).should('eql', expectedExportedRule(this.ruleResponse));
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ describe('Timeline Templates', () => {
addNameToTimeline(getTimeline().title);

cy.wait('@timeline').then(({ response }) => {
const timelineId = response!.body.data.persistTimeline.timeline.savedObjectId;
const timelineId = response?.body.data.persistTimeline.timeline.savedObjectId;

addDescriptionToTimeline(getTimeline().description);
addNotesToTimeline(getTimeline().notes);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@ describe('Export timelines', () => {
exportTimeline(this.templateId);

cy.wait('@export').then(({ response }) => {
cy.wrap(response!.statusCode).should('eql', 200);
cy.wrap(response?.statusCode).should('eql', 200);

cy.wrap(response!.body).should(
cy.wrap(response?.body).should(
'eql',
expectedExportedTimelineTemplate(this.templateResponse)
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ describe('Export timelines', () => {
exportTimeline(this.timelineId);

cy.wait('@export').then(({ response }) => {
cy.wrap(response!.statusCode).should('eql', 200);
cy.wrap(response?.statusCode).should('eql', 200);

cy.wrap(response!.body).should('eql', expectedExportedTimeline(this.timelineResponse));
cy.wrap(response?.body).should('eql', expectedExportedTimeline(this.timelineResponse));
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -54,17 +54,17 @@ describe('Update kqlMode for timeline', () => {
it('should be able to update timeline kqlMode with filter', () => {
cy.get(TIMELINE_KQLMODE_FILTER).click();
cy.wait('@update').then(({ response }) => {
cy.wrap(response!.statusCode).should('eql', 200);
cy.wrap(response!.body.data.persistTimeline.timeline.kqlMode).should('eql', 'filter');
cy.wrap(response?.statusCode).should('eql', 200);
cy.wrap(response?.body.data.persistTimeline.timeline.kqlMode).should('eql', 'filter');
cy.get(ADD_FILTER).should('exist');
});
});

it('should be able to update timeline kqlMode with search', () => {
cy.get(TIMELINE_KQLMODE_SEARCH).click();
cy.wait('@update').then(({ response }) => {
cy.wrap(response!.statusCode).should('eql', 200);
cy.wrap(response!.body.data.persistTimeline.timeline.kqlMode).should('eql', 'search');
cy.wrap(response?.statusCode).should('eql', 200);
cy.wrap(response?.body.data.persistTimeline.timeline.kqlMode).should('eql', 'search');
cy.get(ADD_FILTER).should('not.exist');
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -250,8 +250,8 @@ describe('url state', () => {

cy.wait('@timeline').then(({ response }) => {
closeTimeline();
cy.wrap(response!.statusCode).should('eql', 200);
const timelineId = response!.body.data.persistTimeline.timeline.savedObjectId;
cy.wrap(response?.statusCode).should('eql', 200);
const timelineId = response?.body.data.persistTimeline.timeline.savedObjectId;
cy.visit('/app/home');
cy.visit(`/app/security/timelines?timeline=(id:'${timelineId}',isOpen:!t)`);
cy.get(DATE_PICKER_APPLY_BUTTON_TIMELINE).should('exist');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -174,8 +174,8 @@ describe('value lists', () => {
cy.wait('@exportList').then(({ response }) => {
cy.fixture(listName).then((list: string) => {
const [lineOne, lineTwo] = list.split('\n');
expect(response!.body).to.contain(lineOne);
expect(response!.body).to.contain(lineTwo);
expect(response?.body).to.contain(lineOne);
expect(response?.body).to.contain(lineTwo);
});
});
});
Expand All @@ -189,8 +189,8 @@ describe('value lists', () => {
cy.wait('@exportList').then(({ response }) => {
cy.fixture(listName).then((list: string) => {
const [lineOne, lineTwo] = list.split('\n');
expect(response!.body).to.contain(lineOne);
expect(response!.body).to.contain(lineTwo);
expect(response?.body).to.contain(lineOne);
expect(response?.body).to.contain(lineTwo);
});
});
});
Expand All @@ -204,8 +204,8 @@ describe('value lists', () => {
cy.wait('@exportList').then(({ response }) => {
cy.fixture(listName).then((list: string) => {
const [lineOne, lineTwo] = list.split('\n');
expect(response!.body).to.contain(lineOne);
expect(response!.body).to.contain(lineTwo);
expect(response?.body).to.contain(lineOne);
expect(response?.body).to.contain(lineTwo);
});
});
});
Expand All @@ -219,7 +219,7 @@ describe('value lists', () => {
cy.wait('@exportList').then(({ response }) => {
cy.fixture(listName).then((list: string) => {
const [lineOne] = list.split('\n');
expect(response!.body).to.contain(lineOne);
expect(response?.body).to.contain(lineOne);
});
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@ export const BULK_ACTIONS = '[data-test-subj="utility-bar-action-button"]';

export const EXPORT_TIMELINE_ACTION = '[data-test-subj="export-timeline-action"]';

export const TIMELINE = (id: string) => {
export const TIMELINE = (id: string | undefined) => {
if (id == null) {
throw new TypeError('id should never be null or undefined');
}
return `[data-test-subj="title-${id}"]`;
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ export const fillDefineCustomRuleWithImportedQueryAndContinue = (
rule: CustomRule | OverrideRule
) => {
cy.get(IMPORT_QUERY_FROM_SAVED_TIMELINE_LINK).click();
cy.get(TIMELINE(rule.timeline.id!)).click();
cy.get(TIMELINE(rule.timeline.id)).click();
cy.get(CUSTOM_QUERY_INPUT).should('have.value', rule.customQuery);
cy.get(DEFINE_CONTINUE_BUTTON).should('exist').click({ force: true });

Expand All @@ -273,7 +273,7 @@ export const fillDefineThresholdRule = (rule: ThresholdRule) => {
const threshold = 1;

cy.get(IMPORT_QUERY_FROM_SAVED_TIMELINE_LINK).click();
cy.get(TIMELINE(rule.timeline.id!)).click();
cy.get(TIMELINE(rule.timeline.id)).click();
cy.get(COMBO_BOX_CLEAR_BTN).first().click();

rule.index.forEach((index) => {
Expand All @@ -298,7 +298,7 @@ export const fillDefineThresholdRuleAndContinue = (rule: ThresholdRule) => {
cy.wrap($el).type(rule.thresholdField, { delay: 35 });

cy.get(IMPORT_QUERY_FROM_SAVED_TIMELINE_LINK).click();
cy.get(TIMELINE(rule.timeline.id!)).click();
cy.get(TIMELINE(rule.timeline.id)).click();
cy.get(CUSTOM_QUERY_INPUT).should('have.value', rule.customQuery);
cy.get(THRESHOLD_INPUT_AREA)
.find(INPUT)
Expand All @@ -314,9 +314,12 @@ export const fillDefineThresholdRuleAndContinue = (rule: ThresholdRule) => {
};

export const fillDefineEqlRuleAndContinue = (rule: CustomRule) => {
if (rule.customQuery == null) {
throw new TypeError('The rule custom query should never be undefined or null ');
}
cy.get(RULES_CREATION_FORM).find(EQL_QUERY_INPUT).should('exist');
cy.get(RULES_CREATION_FORM).find(EQL_QUERY_INPUT).should('be.visible');
cy.get(RULES_CREATION_FORM).find(EQL_QUERY_INPUT).type(rule.customQuery!);
cy.get(RULES_CREATION_FORM).find(EQL_QUERY_INPUT).type(rule.customQuery);
cy.get(RULES_CREATION_FORM).find(EQL_QUERY_VALIDATION_SPINNER).should('not.exist');
cy.get(RULES_CREATION_PREVIEW)
.find(QUERY_PREVIEW_BUTTON)
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/security_solution/cypress/tasks/login.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ const LOGIN_API_ENDPOINT = '/internal/security/login';
*/
export const getUrlWithRoute = (role: ROLES, route: string) => {
const url = Cypress.config().baseUrl;
const kibana = new URL(url!);
const kibana = new URL(String(url));
const theUrl = `${Url.format({
auth: `${role}:changeme`,
username: role,
Expand All @@ -83,7 +83,7 @@ interface User {
*/
export const constructUrlWithUser = (user: User, route: string) => {
const url = Cypress.config().baseUrl;
const kibana = new URL(url!);
const kibana = new URL(String(url));
const hostname = kibana.hostname;
const username = user.username;
const password = user.password;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export const activatesRule = () => {
cy.get(RULE_SWITCH).should('be.visible');
cy.get(RULE_SWITCH).click();
cy.wait('@bulk_update').then(({ response }) => {
cy.wrap(response!.statusCode).should('eql', 200);
cy.wrap(response?.statusCode).should('eql', 200);
});
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ export const BarChartBaseComponent = ({
yAccessors={yAccessors}
timeZone={timeZone}
splitSeriesAccessors={splitSeriesAccessors}
data={series.value!}
data={series.value ?? []}
stackAccessors={get('configs.series.stackAccessors', chartConfigs)}
color={series.color ? series.color : undefined}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { mockBrowserFields } from '../../containers/source/mock';

const aField = {
...mockDetailItemData[4],
...mockBrowserFields.base.fields!['@timestamp'],
...mockBrowserFields.base.fields?.['@timestamp'],
};

describe('helpers', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ export const getColumnsWithTimestamp = ({
export const getExampleText = (example: string | number | null | undefined): string =>
!isEmpty(example) ? `Example: ${example}` : '';

export const getIconFromType = (type: string | null) => {
export const getIconFromType = (type: string | null | undefined) => {
switch (type) {
case 'string': // fall through
case 'keyword':
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ const EventsViewerComponent: React.FC<Props> = ({
useTimelineEvents({
docValueFields,
fields,
filterQuery: combinedQueries!.filterQuery,
filterQuery: combinedQueries?.filterQuery,
id,
indexNames,
limit: itemsPerPage,
Expand Down Expand Up @@ -300,7 +300,7 @@ const EventsViewerComponent: React.FC<Props> = ({
height={headerFilterGroup ? COMPACT_HEADER_HEIGHT : EVENTS_VIEWER_HEADER_HEIGHT}
subtitle={utilityBar ? undefined : subtitle}
title={globalFullScreen ? titleWithExitFullScreen : justTitle}
isInspectDisabled={combinedQueries!.filterQuery === undefined}
isInspectDisabled={combinedQueries?.filterQuery === undefined}
>
{HeaderSectionContent}
</HeaderSection>
Expand Down
Loading

0 comments on commit d2075f6

Please sign in to comment.