[Security Solution] Add hook for reading/writing resolver query params by kqualters-elastic · Pull Request #70809 · elastic/kibana

kqualters-elastic · 2020-07-06T14:31:52Z

Summary

This pr adds a hook for use in resolver components that requires a unique key, at this point timeline.Id from siem code, to read and write to url query params without colliding if more than one instance of resolver is on the page at once.

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

For maintainers

This was checked for breaking API changes and was labeled appropriately

bkimmel · 2020-07-07T22:22:15Z

x-pack/plugins/security_solution/public/resolver/view/panel.tsx

ℹ️ [documentLocation + 'crumbId']: '' would let you have any number of them as long as documentLocation is stable and unique across pageloads, right?

oh wait nvm, I see you're already doing that above

bkimmel · 2020-07-13T19:30:26Z

x-pack/plugins/security_solution/public/resolver/view/process_event_dot.tsx

-      },
-      [history, urlSearch]
-    );
+    const { pushToQueryParams } = useResolverQueryParams(documentLocation);


ℹ️ very niiice

oatkiller · 2020-07-13T16:16:44Z

x-pack/plugins/security_solution/public/resolver/view/index.tsx

please add doc comment

oatkiller · 2020-07-13T19:27:24Z

x-pack/plugins/security_solution/public/resolver/view/map.tsx

   * Used as the origin of the Resolver graph.
   */
  databaseDocumentID?: string;
+  documentLocation: string;


please add doc comment

oatkiller · 2020-07-13T19:30:14Z

x-pack/plugins/security_solution/public/resolver/view/use_resolver_query_params.ts

+      [uniqueCrumbEventKey]: '',
+      ...querystring.parse(urlSearch.slice(1)),
+    };
+    newParams.crumbId = newParams[uniqueCrumbIdKey];


If you're setting crumbId and crumbEvent here, why is CrumbInfo typed with an index type?

oatkiller · 2020-07-13T19:33:52Z

x-pack/plugins/security_solution/public/resolver/view/use_resolver_query_params.ts

+import { useHistory, useLocation } from 'react-router-dom';
+import { CrumbInfo } from './panels/panel_content_utilities';
+
+export function useResolverQueryParams(documentLocation: string) {


instead of passing this around, put the documentLocation in the action dispatched by useStateSyncingActions, then use useSelector in this hook to get it.

Are there some cool things you could do by having it passed in as an argument like this, though? Just thinking in the future stuff like "pre-focusing" a Resolver by pushing to query params (I guess if you knew its document location) before you open it?

oatkiller

Please keep the document location ID in state instead of passing it around

bkimmel · 2020-07-13T19:39:31Z

x-pack/plugins/security_solution/public/resolver/view/use_resolver_query_params.ts

+import { useHistory, useLocation } from 'react-router-dom';
+import { CrumbInfo } from './panels/panel_content_utilities';
+
+export function useResolverQueryParams(documentLocation: string) {


❔ My only question here would be if this is "passable" in a URL they way we envisioned. I think we put it in this way now regardless, but maybe we should test that someone on the same Kibana instance can always open the URL and restore/rehydrate context based on this.

bkimmel · 2020-07-13T19:44:27Z

x-pack/plugins/security_solution/public/resolver/view/index.tsx

+   * A string literal describing where in the app resolver is located,
+   * used to prevent collisions in things like query params
+   */
+  documentLocation: string;


❔ defer for now, but something like Pick<TimelineModel,'id'> might be a little more descriptive/helpful here.

...but maybe also too narrow, carry on actually.

bkimmel · 2020-07-13T19:48:48Z

x-pack/plugins/security_solution/public/resolver/view/panels/panel_content_utilities.tsx

 export interface CrumbInfo {
-  readonly crumbId: string;
-  readonly crumbEvent: string;
+  readonly [x: string]: string;


❔ (defer: do do not this until after merge if you decide to) At this point, you could probably just change it to a Record<string, string> for clarity.

oatkiller

cool

oatkiller · 2020-07-13T21:50:08Z

x-pack/plugins/security_solution/public/resolver/store/data/reducer.ts

just use undefined.

bkimmel · 2020-07-13T23:26:23Z

x-pack/plugins/security_solution/public/resolver/store/data/selectors.test.ts

      expect(selectors.databaseDocumentIDToFetch(state())).toBe(secondDatabaseDocumentID);
    });
+    it('should use the correct location for the first resolver', () => {
+      expect(selectors.resolverComponentInstanceID(state())).toBe(resolverComponentInstanceID1);


❔ How does this work? It looks like we expect different things back to back from the same selector...

❔ If it's because of the aborted request, that's really hard to understand here.

It fetched the first resolver, and then this is it resolved. Technically the second one is in-progress at this point and resolves in the checks below. It's the way the actions resolve for the state() calls

bkimmel · 2020-07-13T23:28:38Z

x-pack/plugins/security_solution/public/resolver/types.ts

   * The id used for the pending request, if there is one.
   */
  readonly pendingRequestDatabaseDocumentID?: string;
+  readonly resolverComponentInstanceID: string;


❔ A doc comment

bkimmel

My comments here are all ❔ . Merge.

kibanamachine · 2020-07-14T07:38:11Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 9d2eeca

Build metrics

✅ unchanged

History

💔 Build #61166 failed b4c9faf
💔 Build #61168 failed 364ac4c
💔 Build #61144 failed f226888f0a25177f04ea114d8b30a8039074218e
💔 Build #61123 failed 1da0dc22cc43ff6b63332ae45f7cc4ed8167bcc3
💔 Build #60960 failed 1206284

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

* master: (314 commits) [APM] Use status_code field to calculate error rate (elastic#71109) [Observability] Change appLink passing the date range (elastic#71259) [Security] Add Timeline improvements (elastic#71506) adjust vislib bar opacity (elastic#71421) Fix ScopedHistory mock and adapt usages (elastic#71404) [Security Solution] Add hook for reading/writing resolver query params (elastic#70809) [APM] Bug fixes from ML integration testing (elastic#71564) [Discover] Add caused_by.type and caused_by.reason to error toast modal (elastic#70404) [Security Solution] Add 3rd level breadcrumb to admin page (elastic#71275) [Security Solution][Exceptions] Exception modal bulk close alerts that match exception attributes (elastic#71321) Change signal.rule.risk score mapping from keyword to float (elastic#71126) Added help text where needed on connectors and alert actions UI (elastic#69601) [SIEM][Detections] Value Lists Management Modal (elastic#67068) [test] Skips test preventing promotion of ES snapshot elastic#71582 [test] Skips test preventing promotion of ES snapshot elastic#71555 [ILM] Fix alignment of the timing field (elastic#71273) [SIEM][Detection Engine][Lists] Adds the ability for exception lists to be multi-list queried. (elastic#71540) initial telemetry setup (elastic#69330) [Reporting] Formatting fixes for CSV export in Discover, CSV download from Dashboard panel (elastic#67027) Search across spaces (elastic#67644) ...

…t-apps-page-titles * 'master' of github.com:elastic/kibana: (88 commits) [ML] Functional tests - disable DFA creation and cloning tests [APM] Use status_code field to calculate error rate (elastic#71109) [Observability] Change appLink passing the date range (elastic#71259) [Security] Add Timeline improvements (elastic#71506) adjust vislib bar opacity (elastic#71421) Fix ScopedHistory mock and adapt usages (elastic#71404) [Security Solution] Add hook for reading/writing resolver query params (elastic#70809) [APM] Bug fixes from ML integration testing (elastic#71564) [Discover] Add caused_by.type and caused_by.reason to error toast modal (elastic#70404) [Security Solution] Add 3rd level breadcrumb to admin page (elastic#71275) [Security Solution][Exceptions] Exception modal bulk close alerts that match exception attributes (elastic#71321) Change signal.rule.risk score mapping from keyword to float (elastic#71126) Added help text where needed on connectors and alert actions UI (elastic#69601) [SIEM][Detections] Value Lists Management Modal (elastic#67068) [test] Skips test preventing promotion of ES snapshot elastic#71582 [test] Skips test preventing promotion of ES snapshot elastic#71555 [ILM] Fix alignment of the timing field (elastic#71273) [SIEM][Detection Engine][Lists] Adds the ability for exception lists to be multi-list queried. (elastic#71540) initial telemetry setup (elastic#69330) [Reporting] Formatting fixes for CSV export in Discover, CSV download from Dashboard panel (elastic#67027) ... # Conflicts: # x-pack/plugins/index_management/public/application/index.tsx

* master: (72 commits) [test] Skips test preventing promotion of ES snapshot elastic#71612 [Logs UI] Remove UUID from Alert Instances (elastic#71340) [Metrics UI] Remove UUID from Alert Instance IDs (elastic#71335) [ML] Functional tests - disable DFA creation and cloning tests [APM] Use status_code field to calculate error rate (elastic#71109) [Observability] Change appLink passing the date range (elastic#71259) [Security] Add Timeline improvements (elastic#71506) adjust vislib bar opacity (elastic#71421) Fix ScopedHistory mock and adapt usages (elastic#71404) [Security Solution] Add hook for reading/writing resolver query params (elastic#70809) [APM] Bug fixes from ML integration testing (elastic#71564) [Discover] Add caused_by.type and caused_by.reason to error toast modal (elastic#70404) [Security Solution] Add 3rd level breadcrumb to admin page (elastic#71275) [Security Solution][Exceptions] Exception modal bulk close alerts that match exception attributes (elastic#71321) Change signal.rule.risk score mapping from keyword to float (elastic#71126) Added help text where needed on connectors and alert actions UI (elastic#69601) [SIEM][Detections] Value Lists Management Modal (elastic#67068) [test] Skips test preventing promotion of ES snapshot elastic#71582 [test] Skips test preventing promotion of ES snapshot elastic#71555 [ILM] Fix alignment of the timing field (elastic#71273) ...

… params (#70809) (#71659) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

… params (#70809) (#71905) * Move resolver query param logic into shared hook * Store document location in state * Rename documentLocation to resolverComponentInstanceID * Use undefined for initial resolverComponentID value * Update type for initial state of component id

elasticmachine · 2021-09-23T14:31:54Z

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

kqualters-elastic changed the title ~~Add hook, doesn't work~~ [Security Solution] Add hook for reading/writing resolver query params Jul 6, 2020

bkimmel reviewed Jul 7, 2020

View reviewed changes

Move resolver query param logic into shared hook

1206284

kqualters-elastic force-pushed the resolver-query-param-selectors branch from 2ba047e to 1206284 Compare July 13, 2020 16:24

bkimmel reviewed Jul 13, 2020

View reviewed changes

oatkiller reviewed Jul 13, 2020

View reviewed changes

oatkiller suggested changes Jul 13, 2020

View reviewed changes

bkimmel reviewed Jul 13, 2020

View reviewed changes

oatkiller approved these changes Jul 13, 2020

View reviewed changes

x-pack/plugins/security_solution/public/resolver/store/data/reducer.ts Outdated

Copy link

Contributor

oatkiller Jul 13, 2020

Choose a reason for hiding this comment

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

just use undefined.

kqualters-elastic added 2 commits July 13, 2020 19:12

Store document location in state

6532698

Rename documentLocation to resolverComponentInstanceID

b4c9faf

kqualters-elastic force-pushed the resolver-query-param-selectors branch from f226888 to b4c9faf Compare July 13, 2020 23:12

Use undefined for initial resolverComponentID value

364ac4c

kqualters-elastic marked this pull request as ready for review July 13, 2020 23:21

kqualters-elastic requested review from a team as code owners July 13, 2020 23:21

bkimmel reviewed Jul 13, 2020

View reviewed changes

bkimmel approved these changes Jul 13, 2020

View reviewed changes

bkimmel added v8.0.0 v7.9.0 release_note:skip Skip the PR/issue when compiling release notes labels Jul 13, 2020

Update type for initial state of component id

9d2eeca

kqualters-elastic merged commit 0f143a3 into elastic:master Jul 14, 2020

kqualters-elastic deleted the resolver-query-param-selectors branch July 14, 2020 07:39

kqualters-elastic mentioned this pull request Jul 14, 2020

[7.x] [Security Solution] Add hook for reading/writing resolver query params (#70809) #71659

Merged

kqualters-elastic added a commit that referenced this pull request Jul 15, 2020

[7.x] [Security Solution] Add hook for reading/writing resolver query…

c9f56a3

… params (#70809) (#71659) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

kqualters-elastic mentioned this pull request Jul 15, 2020

[7.9] [Security Solution] Add hook for reading/writing resolver query params (#70809) #71905

Merged

LeeDr added the Team:SIEM label Jul 16, 2020

MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021

Conversation

kqualters-elastic commented Jul 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oatkiller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oatkiller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelolo24 Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkimmel left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jul 14, 2020

💚 Build Succeeded

Build metrics

History

Uh oh!

elasticmachine commented Sep 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

kqualters-elastic commented Jul 6, 2020 •

edited

Loading

michaelolo24 Jul 13, 2020 •

edited

Loading