Skip to content
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.

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

[Security][Lists] Add API functions and react hooks for value list APIs #69603

Merged
merged 18 commits into from
Jun 30, 2020

Conversation

rylnd
Copy link
Contributor

@rylnd rylnd commented Jun 19, 2020

Summary

This adds new API functions and their corresponding react hooks to be used for #67068.

This does not represent all value list endpoints, only those needed for the 7.9 management modal:

  • findLists
  • importList
  • deleteList
  • exportList

This also adds a generic hook, useAsyncTask, that wraps an async function to provide basic utilities:

  • loading state
  • error state
  • abort/cancel function
    along with type inference of the provided function.

TODO:

  • validate request/response of API functions

Checklist

Delete any items that are not applicable to this PR.

For maintainers

This also adds a generic hook, useAsyncTask, that wraps an async
function to provide basic utilities:
  * loading state
  * error state
  * abort/cancel function
@rylnd rylnd added Team:SIEM v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels Jun 19, 2020
@rylnd rylnd self-assigned this Jun 19, 2020
@rylnd rylnd changed the title Add pure API functions and react hooks for value list APIs [Security][Lists] Add API functions and react hooks for value list APIs Jun 19, 2020
rylnd added 11 commits June 18, 2020 21:38
These were not caught locally as I was accidentally running typescript
without the full project.
… tuple

This allows callers to further leverage fp-ts functions as needed.
* leverages new validateEither fn which returns an Either
* constructs a pipeline that:
  * validates the payload
  * performs the API call
  * validates the response
and short-circuits if any of those produce a Left value.

It then converts the Either into a promise that either rejects with the
Left or resolves with the Right.
This cleans up our validation pipeline considerably.
* refactors private API functions to accept the encoded request schema
(i.e. snake cased)
* refactors validateEither to use `schema.validate` instead of
`schema.decode` since we don't actually want the decoded value, we just
want to verify that it'll be able to be decoded on the backend.
* Continue to export decoded types without a qualifier
* pull types used by hooks from their new location
* Fix errors with usage of act()
@rylnd rylnd marked this pull request as ready for review June 26, 2020 01:49
@rylnd rylnd requested review from a team as code owners June 26, 2020 01:49
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

@rylnd
Copy link
Contributor Author

rylnd commented Jun 26, 2020

This is ready for review, but I'm a little concerned about the increased bundle size. I'm going to take a look at that but I don't expect it will affect much.

By pulling from the module directly instead of an index, we can
hopefully narrow down our dependencies until tree-shaking does this for
us.
@rylnd
Copy link
Contributor Author

rylnd commented Jun 26, 2020

@elasticmachine merge upstream

import { useCallback, useRef } from 'react';
import useAsyncFn from 'react-use/lib/useAsyncFn';

// Params can be generalized to a ...rest parameter extending unknown[] once https://github.com/microsoft/TypeScript/pull/39094 is available.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The one caveat with useAsyncTask right now. Otherwise, it Just Works™ and is type safe.

args: DeleteListTaskArgs
): ReturnType<typeof deleteList> => deleteList({ signal, ...args });

// eslint-disable-next-line @typescript-eslint/explicit-function-return-type
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Half the value of useAsyncTask is inferring arguments of the passed task, so this rule is ignored.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worth adding this comment in the file itself for future reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yctercero that's a good idea, in the interest of build time I'll add that in the subsequent PR 😉

rylnd added 2 commits June 26, 2020 14:47
* master:
  skip failing suite (elastic#70104) (elastic#70103)
  [ENDPOINT] Hide the Timeline Flyout while on the Management Pages (elastic#69998)
  [SIEM][CASE] Persist callout when dismissed (elastic#68372)
  [SIEM][Exceptions] - Cleaned up and updated exception list item comment structure (elastic#69532)
  [Maps] remove indexing state from redux (elastic#69765)
  Add API integration test for deleting data streams. (elastic#70020)
  renames SIEM to Security Solution (elastic#70070)
Rather than returning a promise and requiring the caller to handle a
rejection, we instead return nothing and require the user to watch the
hook's state.

* success can be handled with a useEffect on state.result
* errors can be handled with a useEffect on state.error
@rylnd
Copy link
Contributor Author

rylnd commented Jun 27, 2020

Update: @XavierM diligently asked about useAsyncTask's state being updated after a consuming component had been unmounted. While it does not (although it's hard to pin that down with a unit test), it did uncover some bugs I had introduced by chaining onto the start call.

To solve these and to encourage the safer, more idiomatic use of useEffect, I opted to have start not return anything (02101c7):

// BEFORE: potential unhandled rejection, potential call to handleSuccess after unmounted
task.start().then(handleSuccess);

// AFTER: no rejection to handle, no chance of handleSuccess being called after unmount 
// since task.result will not change
useEffect(() => {
 task.result && handleSuccess(task.result);
}, [task.result]);
task.start();

Assertion count wasn't updated following interface changes; we've now
got two inline expectations so this isn't needed.
@rylnd
Copy link
Contributor Author

rylnd commented Jun 29, 2020

@elasticmachine merge upstream

pageSize,
signal,
}: FindListsParams): Promise<FoundListSchema> =>
pipe(
Copy link
Contributor

Choose a reason for hiding this comment

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

Love this. I did validations on the exceptions, but it's way more verbose. I'd opt to update the exceptions list to use these same patterns.

flow(toPromise)
);

export { importListWithValidation as importList };
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that they're separated out. Maybe for our future selves or other sake we can add a comment above the non validation versions asking that they not be exported. I could see the case of someone in the future getting annoyed and wanting to just skirt the validations 🦺 Just a thought...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yctercero I had a similar thought, but I think that making the "default" function have validations makes sense for now. If in the future we need the unsafe version it'll have to be exported with a new name, and we can discuss said naming then.

Copy link
Contributor

@yctercero yctercero left a comment

Choose a reason for hiding this comment

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

LGTM! Pulled into my branch for the exceptions builder and am using the hooks - working great! Left a few super minor comments. Really like the new patterns introduced in this PR.

Copy link
Contributor

@FrankHassanabad FrankHassanabad left a comment

Choose a reason for hiding this comment

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

Brief, look, this is all great to me though. Lots of good techniques going on. Can't wait to begin using some of it.

@rylnd
Copy link
Contributor Author

rylnd commented Jun 29, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
securitySolution 792 +12 780

History

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

@rylnd rylnd merged commit 590fc8d into elastic:master Jun 30, 2020
@rylnd rylnd deleted the value_lists_api branch June 30, 2020 01:02
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jun 30, 2020
…ata-streams

* 'master' of github.com:elastic/kibana: (50 commits)
  [Logs UI] [Alerting] "Group by" functionality (elastic#68250)
  [Discover] Deangularize Skip to bottom button (elastic#69811)
  Implement recursive plugin discovery (elastic#68811)
  Use ts-expect-error in platform code (elastic#69883)
  [SIEM][Detection Engine][Lists] Moves getQueryFilter to common folder for use by both front and backend
  [Ingest Manager][SECURITY SOLUTION] adjust config reassign link and add roundtrip to Reassignment flow (elastic#70208)
  [Security][Lists] Add API functions and react hooks for value list APIs (elastic#69603)
  [ILM] Fix bug when clearing priority field (elastic#70154)
  [Platform][Security] Updates cluster_manager ignorePaths to include security scripts (elastic#70139)
  [IngestManager] Allow to filter agent by packages (elastic#69731)
  [code coverage] exclude folders: test_helpers, tests_bundle (elastic#70199)
  [Metrics UI] UX improvements for saved views (elastic#69910)
  [APM] docs: unique transaction troubleshooting (elastic#69831)
  Cross cluster search functional test with minimun privileges assigned to the test_user (elastic#70007)
  [Maps] choropleth layer wizard (elastic#69699)
  Make custom errors by extending Error (elastic#69966)
  [Ingest Manager] Support updated package output structure (elastic#69864)
  Resolver test coverage (elastic#70246)
  Async Discover search test (elastic#64388)
  [ui-shared-deps] include styled-components (elastic#69322)
  ...

# Conflicts:
#	x-pack/plugins/snapshot_restore/server/types.ts
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 30, 2020
…bana into alerting/consumer-based-rbac

* 'alerting/consumer-based-rbac' of github.com:gmmorris/kibana: (49 commits)
  [Discover] Deangularize Skip to bottom button (elastic#69811)
  Implement recursive plugin discovery (elastic#68811)
  Use ts-expect-error in platform code (elastic#69883)
  [SIEM][Detection Engine][Lists] Moves getQueryFilter to common folder for use by both front and backend
  [Ingest Manager][SECURITY SOLUTION] adjust config reassign link and add roundtrip to Reassignment flow (elastic#70208)
  [Security][Lists] Add API functions and react hooks for value list APIs (elastic#69603)
  [ILM] Fix bug when clearing priority field (elastic#70154)
  [Platform][Security] Updates cluster_manager ignorePaths to include security scripts (elastic#70139)
  [IngestManager] Allow to filter agent by packages (elastic#69731)
  [code coverage] exclude folders: test_helpers, tests_bundle (elastic#70199)
  [Metrics UI] UX improvements for saved views (elastic#69910)
  [APM] docs: unique transaction troubleshooting (elastic#69831)
  Cross cluster search functional test with minimun privileges assigned to the test_user (elastic#70007)
  [Maps] choropleth layer wizard (elastic#69699)
  Make custom errors by extending Error (elastic#69966)
  [Ingest Manager] Support updated package output structure (elastic#69864)
  Resolver test coverage (elastic#70246)
  Async Discover search test (elastic#64388)
  [ui-shared-deps] include styled-components (elastic#69322)
  SECURITY-ENDPOINT: add host properties (elastic#70238)
  ...
rylnd added a commit that referenced this pull request Jun 30, 2020
…Is (#69603) (#70279)

* Add pure API functions and react hooks for value list APIs

This also adds a generic hook, useAsyncTask, that wraps an async
function to provide basic utilities:
  * loading state
  * error state
  * abort/cancel function

* Fix type errors in hook tests

These were not caught locally as I was accidentally running typescript
without the full project.

* Document current limitations of useAsyncTask

* Defines a new validation function that returns an Either instead of a tuple

This allows callers to further leverage fp-ts functions as needed.

* Remove duplicated copyright comment

* WIP: Perform request/response validations in the FP style

* leverages new validateEither fn which returns an Either
* constructs a pipeline that:
  * validates the payload
  * performs the API call
  * validates the response
and short-circuits if any of those produce a Left value.

It then converts the Either into a promise that either rejects with the
Left or resolves with the Right.

* Adds helper function to convert a TaskEither back to a Promise

This cleans up our validation pipeline considerably.

* Adds request/response validations to findLists

* refactors private API functions to accept the encoded request schema
(i.e. snake cased)
* refactors validateEither to use `schema.validate` instead of
`schema.decode` since we don't actually want the decoded value, we just
want to verify that it'll be able to be decoded on the backend.

* Refactor our API types

* Add request/response validation to import/export functions

* Fix type errors

* Continue to export decoded types without a qualifier
* pull types used by hooks from their new location
* Fix errors with usage of act()

* Attempting to reduce plugin bundle size

By pulling from the module directly instead of an index, we can
hopefully narrow down our dependencies until tree-shaking does this for
us.

* useAsyncFn's initiator does not return a promise

Rather than returning a promise and requiring the caller to handle a
rejection, we instead return nothing and require the user to watch the
hook's state.

* success can be handled with a useEffect on state.result
* errors can be handled with a useEffect on state.error

* Fix failing test

Assertion count wasn't updated following interface changes; we've now
got two inline expectations so this isn't needed.

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
Bamieh pushed a commit to Bamieh/kibana that referenced this pull request Jul 1, 2020
…Is (elastic#69603)

* Add pure API functions and react hooks for value list APIs

This also adds a generic hook, useAsyncTask, that wraps an async
function to provide basic utilities:
  * loading state
  * error state
  * abort/cancel function

* Fix type errors in hook tests

These were not caught locally as I was accidentally running typescript
without the full project.

* Document current limitations of useAsyncTask

* Defines a new validation function that returns an Either instead of a tuple

This allows callers to further leverage fp-ts functions as needed.

* Remove duplicated copyright comment

* WIP: Perform request/response validations in the FP style

* leverages new validateEither fn which returns an Either
* constructs a pipeline that:
  * validates the payload
  * performs the API call
  * validates the response
and short-circuits if any of those produce a Left value.

It then converts the Either into a promise that either rejects with the
Left or resolves with the Right.

* Adds helper function to convert a TaskEither back to a Promise

This cleans up our validation pipeline considerably.

* Adds request/response validations to findLists

* refactors private API functions to accept the encoded request schema
(i.e. snake cased)
* refactors validateEither to use `schema.validate` instead of
`schema.decode` since we don't actually want the decoded value, we just
want to verify that it'll be able to be decoded on the backend.

* Refactor our API types

* Add request/response validation to import/export functions

* Fix type errors

* Continue to export decoded types without a qualifier
* pull types used by hooks from their new location
* Fix errors with usage of act()

* Attempting to reduce plugin bundle size

By pulling from the module directly instead of an index, we can
hopefully narrow down our dependencies until tree-shaking does this for
us.

* useAsyncFn's initiator does not return a promise

Rather than returning a promise and requiring the caller to handle a
rejection, we instead return nothing and require the user to watch the
hook's state.

* success can be handled with a useEffect on state.result
* errors can be handled with a useEffect on state.error

* Fix failing test

Assertion count wasn't updated following interface changes; we've now
got two inline expectations so this isn't needed.

Co-authored-by: Elastic Machine <[email protected]>
@MindyRS MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021
@elasticmachine
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants