Skip to content

[Security Solution][Defend Workflows] Fix type errors indicated by TS 4.7.4. #163066

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gergoabraham merged 13 commits into from
Sep 18, 2023
Merged

[Security Solution][Defend Workflows] Fix type errors indicated by TS 4.7.4. #163066

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
8723cbe
fix type errors for `pipeRun`
gergoabraham Aug 2, 2023
76b6a06
fix type errors in detection engine api integration test
gergoabraham Aug 3, 2023
2c16145
remove unnecessary check and add a spinner instead
gergoabraham Aug 3, 2023
d484b5a
remove unnecessary check
gergoabraham Aug 3, 2023
8a4b0d6
Revert "fix type errors for `pipeRun`"
gergoabraham Sep 1, 2023
6f76946
Merge branch 'main' into chore/defend-workflows-fix-ts-4.7.4-type-errors
kibanamachine Sep 1, 2023
13cfd99
Merge branch 'main' into chore/defend-workflows-fix-ts-4.7.4-type-errors
kibanamachine Sep 11, 2023
dac76a5
remove browser filter from osquery cy tests
gergoabraham Sep 13, 2023
cf43db4
Revert "remove browser filter from osquery cy tests"
gergoabraham Sep 13, 2023
fa5836f
Merge branch 'main' into chore/defend-workflows-fix-ts-4.7.4-type-errors
kibanamachine Sep 13, 2023
6ef6e1a
Merge branch 'main' into chore/defend-workflows-fix-ts-4.7.4-type-errors
kibanamachine Sep 13, 2023
44daf46
Merge branch 'main' into chore/defend-workflows-fix-ts-4.7.4-type-errors
kibanamachine Sep 14, 2023
8be2f2c
Merge branch 'main' into chore/defend-workflows-fix-ts-4.7.4-type-errors
kibanamachine Sep 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion x-pack/plugins/osquery/public/shared_components/lazy_osquery_action_params_form.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* 2.0.
*/

import { EuiLoadingSpinner } from '@elastic/eui';
import React, { lazy, Suspense } from 'react';
import type { OsqueryResponseActionsParamsFormProps } from './osquery_response_action_type';

Expand All @@ -16,7 +17,7 @@ export const getLazyOsqueryResponseActionTypeForm =
const { onError, defaultValues, onChange } = props;

return (
<Suspense fallback={null}>
<Suspense fallback={<EuiLoadingSpinner />}>
<OsqueryResponseActionParamsForm
onChange={onChange}
defaultValues={defaultValues}
Expand Down
9 changes: 2 additions & 7 deletions ...olution/public/detection_engine/rule_response_actions/osquery/osquery_response_action.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* 2.0.
*/

import React, { useMemo } from 'react';
import React from 'react';
import { EuiCode, EuiEmptyPrompt } from '@elastic/eui';
import { FormattedMessage } from '@kbn/i18n-react';
import { useIsMounted } from '@kbn/securitysolution-hook-utils';
Expand All @@ -25,10 +25,6 @@ const GhostFormField = () => <></>;

export const OsqueryResponseAction = React.memo((props: OsqueryResponseActionProps) => {
const { osquery, application } = useKibana().services;
const OsqueryForm = useMemo(
() => osquery?.OsqueryResponseActionTypeForm,
[osquery?.OsqueryResponseActionTypeForm]
);
const isMounted = useIsMounted();

// serverless component that is returned when users do not have Endpoint.Complete tier
Expand Down Expand Up @@ -85,8 +81,7 @@ export const OsqueryResponseAction = React.memo((props: OsqueryResponseActionPro
);
}

// @ts-expect-error ts upgrade v4.7.4
if (isMounted() && OsqueryForm) {
Copy link
Copy Markdown
Contributor

@tomsonpl tomsonpl Sep 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gergoabraham I remember we had some issues here (not typescript but how it works). Are you sure this change preserves the behavior?

Copy link
Copy Markdown
Contributor Author

@gergoabraham gergoabraham Sep 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am mostly sure, yes...

OsqueryForm comes from useKibana().services.osquery.OsqueryResponseActionTypeForm, which got its value during plugin start, and its value is the return value of getLazyOsqueryResponseActionTypeForm(), which - independently of the lazy load - returns a function component which itself is never falsy, so based on this, I believe there should be no problem.

...but...

I'm happy to test it if you can help what should be tested here. I did some basic tests (i.e. tried to use this component, by throttling network connection so it loads slowly) and there were no issues.

Copy link
Copy Markdown
Contributor

@tomsonpl tomsonpl Sep 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember there was some strange behaviour that is why I made this OqueryForm check. I'll test it manually to see if this is not broken now. I am saying this just because there is a strange behavior in the test. It's not succesful - it's pending...
Zrzut ekranu 2023-09-12 o 16 14 11

Copy link
Copy Markdown
Contributor

@tomsonpl tomsonpl Sep 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this manually and I think it still works as expected, thanks!
However - can you please do me a favor and remove the browser filter from here? Lets see if this make the test run then. https://github.com/elastic/kibana/blob/4180a1a105053e5579e6852082bdeaa06f019cd3/x-pack/plugins/osquery/cypress/e2e/all/alerts_response_actions_form.cy.ts#L33C16-L33C16

Copy link
Copy Markdown
Contributor Author

@gergoabraham gergoabraham Sep 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for testing it!

removed the browser filters, let's see what happens: dac76a5

Copy link
Copy Markdown
Contributor

@tomsonpl tomsonpl Sep 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think something is broken, moreover the artifacts are not build. Feel free to revert this commit and just ship your changes. I'll take care of osquery testing in a separate PR. How does this sound?

Copy link
Copy Markdown
Contributor Author

@gergoabraham gergoabraham Sep 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert is done, thanks for the help, @tomsonpl!
cf43db4

if (isMounted()) {
return (
<UseField
path={`${props.item.path}.params`}
Expand Down
2 changes: 0 additions & 2 deletions x-pack/plugins/security_solution/public/management/components/page_overlay/page_overlay.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,8 +253,6 @@ export const PageOverlay = memo<PageOverlayProps>(
useEffect(() => {
if (
isMounted() &&
// @ts-expect-error ts upgrade v4.7.4
onHide &&
hideOnUrlPathnameChange &&
!isHidden &&
openedOnPathName &&
Expand Down
11 changes: 5 additions & 6 deletions ...pi_integration/security_and_spaces/group8/exception_operators_data_types/keyword_array.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,7 @@ export default ({ getService }: FtrProviderContext) => {
await waitForSignalsToBePresent(supertest, log, 1, [id]);
const signalsOpen = await getSignalsById(supertest, log, id);
const hits = signalsOpen.hits.hits.map((hit) => hit._source?.keyword).sort();
// @ts-expect-error ts upgrade v4.7.4
expect(hits.flat(Number.MAX_SAFE_INTEGER)).to.eql([]);
expect(hits.flat(10)).to.eql([]);
});
});

Expand Down Expand Up @@ -283,7 +282,7 @@ export default ({ getService }: FtrProviderContext) => {
await waitForSignalsToBePresent(supertest, log, 1, [id]);
const signalsOpen = await getSignalsById(supertest, log, id);
const hits = signalsOpen.hits.hits.map((hit) => hit._source?.keyword).sort();
expect(hits.flat(Number.MAX_SAFE_INTEGER)).to.eql([]);
expect(hits.flat(10)).to.eql([]);
});
});

Expand Down Expand Up @@ -345,7 +344,7 @@ export default ({ getService }: FtrProviderContext) => {
await waitForSignalsToBePresent(supertest, log, 1, [id]);
const signalsOpen = await getSignalsById(supertest, log, id);
const hits = signalsOpen.hits.hits.map((hit) => hit._source?.keyword).sort();
expect(hits.flat(Number.MAX_SAFE_INTEGER)).to.eql([]);
expect(hits.flat(10)).to.eql([]);
});
});

Expand Down Expand Up @@ -525,7 +524,7 @@ export default ({ getService }: FtrProviderContext) => {
await waitForSignalsToBePresent(supertest, log, 1, [id]);
const signalsOpen = await getSignalsById(supertest, log, id);
const hits = signalsOpen.hits.hits.map((hit) => hit._source?.keyword).sort();
expect(hits.flat(Number.MAX_SAFE_INTEGER)).to.eql([]);
expect(hits.flat(10)).to.eql([]);
});
});

Expand Down Expand Up @@ -695,7 +694,7 @@ export default ({ getService }: FtrProviderContext) => {
await waitForSignalsToBePresent(supertest, log, 1, [id]);
const signalsOpen = await getSignalsById(supertest, log, id);
const hits = signalsOpen.hits.hits.map((hit) => hit._source?.keyword).sort();
expect(hits.flat(Number.MAX_SAFE_INTEGER)).to.eql([]);
expect(hits.flat(10)).to.eql([]);
});
});

Expand Down