[Resolver] Stale query string values are removed when resolver's component instance ID changes. #74979

oatkiller · 2020-08-13T17:57:26Z

Summary

The app can show more than 1 Resolver at a time. Each instance has a unique ID called the resolverComponentInstanceID.
When the user interacts with Resolver it will add values to the query string. The query string values will contain the resolverComponentInstanceID. This allows each Resolver to keep its state separate. When resolver unmounts it will remove any query string values related to it.

If Resolver's resolverComponentInstanceID changes it should remove query string values related to the old instance ID. It does not. This PR fixes that.

Note: I don't know if it was possible for this bug to actually happen. I can't make it happen, but depending on how Resolver is mounted by its consumers it could

Checklist

Unit or functional tests were updated or added to match the most common scenarios
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-08-13T17:57:28Z

Pinging @elastic/endpoint-data-visibility-team (Team:Endpoint Data Visibility)

elasticmachine · 2020-08-13T17:57:28Z

Pinging @elastic/endpoint-app-team (Feature:Resolver)

oatkiller · 2020-08-13T17:57:53Z

x-pack/plugins/security_solution/public/resolver/test_utilities/simulator/index.tsx

-    return {
-      selectedNode: urlSearchParams.getAll(`resolver-${this.resolverComponentInstanceID}-id`),
-    };
+  public get historyLocationSearch(): string {


This name is weird. Any suggestions?

I'm fine with it, attempting to rename things that are built in and already have names usually just leads to more confusion imo

oatkiller · 2020-08-13T17:57:55Z

x-pack/plugins/security_solution/public/resolver/test_utilities/simulator/index.tsx

+  /**
+   * Change the component instance ID (updates the React component props.)
+   */
+  public set resolverComponentInstanceID(value: string) {


thoughts? @michaelolo24

👍 , but would it be useful to be able to update the databaseDocumentId and componentInstanceId? We don't currently do it, but there could be a situation where we load another tree within an existing resolver window when a user requests additional data. That might just be a page change, but yea. Either way, this can be refactored to that later on as well if necessary

oatkiller · 2020-08-13T17:58:13Z

x-pack/plugins/security_solution/public/resolver/view/clickthrough.test.tsx

@@ -98,7 +99,9 @@ describe('Resolver, when analyzing a tree that has no ancestors and 2 children',
          }))
        ).toYieldEqualTo({
          // Just the second child should be marked as selected in the query string
-          queryStringSelectedNode: [entityIDs.secondChild],
+          search: urlSearch(resolverComponentInstanceID, {
+            selectedEntityID: entityIDs.secondChild,


This now compares the url query strings.

oatkiller · 2020-08-13T17:58:23Z

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

-        selectedNode: [entityIDs.origin],
-      });
+      await expect(simulator().map(() => simulator().historyLocationSearch)).toYieldEqualTo(
+        urlSearch(resolverComponentInstanceID, {


This now compares the url query strings.

oatkiller · 2020-08-13T17:58:59Z

x-pack/plugins/security_solution/public/resolver/view/query_params.test.ts

+
+// the resolver component instance ID, used by the react code to distinguish piece of global state from those used by other resolver instances
+const resolverComponentInstanceID = 'oldID';
+


I think this could use a doc level comment. Something like what is in the description of this PR.

oatkiller · 2020-08-13T17:59:42Z

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

 import { useSelector } from 'react-redux';
-import { useEffectOnce } from 'react-use';


@kqualters-elastic i blame you

this is on your permanent record

oatkiller · 2020-08-13T17:59:59Z

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

-        [uniqueCrumbEventKey]: newCrumbs.crumbEvent,
-      };
+    (queryStringState: CrumbInfo) => {
+      const urlSearchParams = new URLSearchParams(urlSearch);


using URLSearchParams instead of external dep

oatkiller · 2020-08-13T18:00:25Z

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

-      crumbEvent: valueForParam(crumbEvent),
-      crumbId: valueForParam(crumbId),
+      // Use `''` for backwards compatibility with deprecated code.
+      crumbEvent: urlSearchParams.get(eventKey) ?? '',


This is a weird behavior. We should not confuse '' and null | undefined.

kqualters-elastic

I hope https://github.com/elastic/kibana/pull/74979/files#diff-7ad9ae78b9b4a582e275042a28912d39R64 is the only way to make the bug appear and it doesn't happen when using the app, but good change still 👍

kqualters-elastic · 2020-08-13T18:33:35Z

x-pack/plugins/security_solution/public/resolver/test_utilities/simulator/index.tsx

-    return {
-      selectedNode: urlSearchParams.getAll(`resolver-${this.resolverComponentInstanceID}-id`),
-    };
+  public get historyLocationSearch(): string {


I'm fine with it, attempting to rename things that are built in and already have names usually just leads to more confusion imo

oatkiller · 2020-08-13T18:51:30Z

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

@@ -35,6 +34,8 @@ export const ResolverWithoutProviders = React.memo(
    { className, databaseDocumentID, resolverComponentInstanceID }: ResolverProps,
    refToForward
  ) {
+    //


weird comment

michaelolo24 · 2020-08-13T19:19:34Z

x-pack/plugins/security_solution/public/resolver/view/query_params.test.ts

+    describe('when the resolver component has its component instance ID changed', () => {
+      const newInstanceID = 'newID';
+      beforeEach(() => {
+        simulator.resolverComponentInstanceID = newInstanceID;


not using your setter?

this is the setter you are looking for

…ring-stuff

kibanamachine · 2020-08-13T21:48:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 71a5793

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
securitySolution	1909	-126	2035

async chunks size

id	value	diff	baseline
securitySolution	7.2MB	-143.4KB	7.3MB

page load bundle size

id	value	diff	baseline
securitySolution	805.9KB	-5.0B	805.9KB

History

💚 Build #68006 succeeded b7258ed
💔 Build #68002 failed 7f1f453
💔 Build #67993 failed a675bb0

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

* master: (23 commits) Adding API test for custom link transaction example (elastic#74238) [Uptime] Singular alert (elastic#74659) Revert "attempt excluding a codeowners directory" (elastic#75023) [Metrics UI] Remove TSVB dependency from Metrics Explorer APIs (elastic#74804) Remove degraded state from ES status service (elastic#75007) [Reporting/Functional] unskip pagination test (elastic#74973) [Resolver] Stale query string values are removed when resolver's component instance ID changes. (elastic#74979) Add public url to Workplace Search plugin (elastic#74991) [Reporting] Update more Server Types for TaskManager (elastic#74915) [I18n] verify select icu-message options are in english (elastic#74963) Make data.search.aggs available on the server. (elastic#74472) [Security Solution][Resolver] Graph Control Tests and Update Simulator Selectors (elastic#74680) attempt excluding a codeowners directory [ML] DF Analytics: allow failed job to be stopped by force via the UI (elastic#74710) Add kibana-core-ui-designers team (elastic#74970) [Metrics UI] Fix inventory footer misalignment (elastic#74707) Remove legacy optimizer (elastic#73154) Update design-specific GH code-owners (elastic#74877) skip test Reporting paginates content elastic#74922 [Metrics UI] Add Jest tests for alert previews (elastic#74890) ...

…onent instance ID changes. (elastic#74979) The app can show more than 1 Resolver at a time. Each instance has a unique ID called the `resolverComponentInstanceID`. When the user interacts with Resolver it will add values to the query string. The query string values will contain the `resolverComponentInstanceID`. This allows each Resolver to keep its state separate. When resolver unmounts it will remove any query string values related to it. If Resolver's `resolverComponentInstanceID` changes it should remove query string values related to the old instance ID. It does not. This PR fixes that. Note: I don't know if it was possible for this bug to actually happen. I can't make it happen, but depending on how Resolver is mounted by its consumers it *could*

…onent instance ID changes. (#74979) (#75038) The app can show more than 1 Resolver at a time. Each instance has a unique ID called the `resolverComponentInstanceID`. When the user interacts with Resolver it will add values to the query string. The query string values will contain the `resolverComponentInstanceID`. This allows each Resolver to keep its state separate. When resolver unmounts it will remove any query string values related to it. If Resolver's `resolverComponentInstanceID` changes it should remove query string values related to the old instance ID. It does not. This PR fixes that. Note: I don't know if it was possible for this bug to actually happen. I can't make it happen, but depending on how Resolver is mounted by its consumers it *could*

oatkiller added 3 commits August 13, 2020 13:48

working on query string tests

e0cc7f4

doit

4752552

cleanup the url handling code

a675bb0

oatkiller added release_note:fix Team:Endpoint Data Visibility Team managing the endpoint resolver Feature:Resolver Security Solution Resolver feature v7.10.0 labels Aug 13, 2020

oatkiller requested a review from kqualters-elastic August 13, 2020 17:57

oatkiller requested review from a team as code owners August 13, 2020 17:57

oatkiller commented Aug 13, 2020

View reviewed changes

oatkiller requested a review from dplumlee August 13, 2020 18:00

oatkiller added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:fix labels Aug 13, 2020

remove unused stuff

6e1511e

kqualters-elastic approved these changes Aug 13, 2020

View reviewed changes

oatkiller added 2 commits August 13, 2020 14:38

refactor

7f1f453

remove old cleanup effect

b7258ed

oatkiller commented Aug 13, 2020

View reviewed changes

michaelolo24 reviewed Aug 13, 2020

View reviewed changes

michaelolo24 approved these changes Aug 13, 2020

View reviewed changes

oatkiller added 3 commits August 13, 2020 15:47

durp

d1b3c2a

hacked

6a26018

Merge branch 'master' of github.com:elastic/kibana into test-query-st…

71a5793

…ring-stuff

oatkiller merged commit 1729091 into elastic:master Aug 13, 2020

oatkiller deleted the test-query-string-stuff branch August 13, 2020 21:50

oatkiller mentioned this pull request Aug 14, 2020

[7.x] [Resolver] Stale query string values are removed when resolver's component instance ID changes. (#74979) #75038

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Resolver] Stale query string values are removed when resolver's component instance ID changes. #74979

[Resolver] Stale query string values are removed when resolver's component instance ID changes. #74979

oatkiller commented Aug 13, 2020 •

edited

Loading

elasticmachine commented Aug 13, 2020

elasticmachine commented Aug 13, 2020

oatkiller Aug 13, 2020

kqualters-elastic Aug 13, 2020

oatkiller Aug 13, 2020

michaelolo24 Aug 13, 2020

oatkiller Aug 13, 2020

oatkiller Aug 13, 2020

oatkiller Aug 13, 2020

oatkiller Aug 13, 2020

oatkiller Aug 13, 2020

oatkiller Aug 13, 2020

oatkiller Aug 13, 2020

oatkiller Aug 13, 2020

kqualters-elastic left a comment

kqualters-elastic Aug 13, 2020

oatkiller Aug 13, 2020

michaelolo24 Aug 13, 2020

oatkiller Aug 13, 2020

kibanamachine commented Aug 13, 2020


		// the resolver component instance ID, used by the react code to distinguish piece of global state from those used by other resolver instances
		const resolverComponentInstanceID = 'oldID';

		import { useSelector } from 'react-redux';
		import { useEffectOnce } from 'react-use';

[Resolver] Stale query string values are removed when resolver's component instance ID changes. #74979

[Resolver] Stale query string values are removed when resolver's component instance ID changes. #74979

Conversation

oatkiller commented Aug 13, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Aug 13, 2020

elasticmachine commented Aug 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kqualters-elastic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Aug 13, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

async chunks size

page load bundle size

History

oatkiller commented Aug 13, 2020 •

edited

Loading