-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[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
Changes from 6 commits
e0cc7f4
4752552
a675bb0
6e1511e
7f1f453
b7258ed
d1b3c2a
6a26018
71a5793
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,10 +21,6 @@ import { ResolverAction } from '../../store/actions'; | |
* Test a Resolver instance using jest, enzyme, and a mock data layer. | ||
*/ | ||
export class Simulator { | ||
/** | ||
* A string that uniquely identifies this Resolver instance among others mounted in the DOM. | ||
*/ | ||
private readonly resolverComponentInstanceID: string; | ||
/** | ||
* The redux store, creating in the constructor using the `dataAccessLayer`. | ||
* This code subscribes to state transitions. | ||
|
@@ -63,7 +59,6 @@ export class Simulator { | |
databaseDocumentID?: string; | ||
history?: HistoryPackageHistoryInterface<never>; | ||
}) { | ||
this.resolverComponentInstanceID = resolverComponentInstanceID; | ||
// create the spy middleware (for debugging tests) | ||
this.spyMiddleware = spyMiddlewareFactory(); | ||
|
||
|
@@ -90,7 +85,7 @@ export class Simulator { | |
// Render Resolver via the `MockResolver` component, using `enzyme`. | ||
this.wrapper = mount( | ||
<MockResolver | ||
resolverComponentInstanceID={this.resolverComponentInstanceID} | ||
resolverComponentInstanceID={resolverComponentInstanceID} | ||
history={this.history} | ||
store={this.store} | ||
coreStart={coreStart} | ||
|
@@ -99,6 +94,27 @@ export class Simulator { | |
); | ||
} | ||
|
||
/** | ||
* Unmount the Resolver component. Use this to test what happens when code that uses Resolver unmounts it. | ||
*/ | ||
public unmount(): void { | ||
this.wrapper.unmount(); | ||
} | ||
|
||
/** | ||
* Get the component instance ID from the component. | ||
*/ | ||
public get resolverComponentInstanceID(): string { | ||
return this.wrapper.prop('resolverComponentInstanceID'); | ||
} | ||
|
||
/** | ||
* Change the component instance ID (updates the React component props.) | ||
*/ | ||
public set resolverComponentInstanceID(value: string) { | ||
this.wrapper.setProps({ resolverComponentInstanceID: value }); | ||
} | ||
|
||
/** | ||
* Call this to console.log actions (and state). Use this to debug your tests. | ||
* State and actions aren't exposed otherwise because the tests using this simulator should | ||
|
@@ -192,13 +208,11 @@ export class Simulator { | |
} | ||
|
||
/** | ||
* Return the selected node query string values. | ||
* The 'search' part of the URL. | ||
*/ | ||
public queryStringValues(): { selectedNode: string[] } { | ||
const urlSearchParams = new URLSearchParams(this.history.location.search); | ||
return { | ||
selectedNode: urlSearchParams.getAll(`resolver-${this.resolverComponentInstanceID}-id`), | ||
}; | ||
public get historyLocationSearch(): string { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This name is weird. Any suggestions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fine with it, attempting to rename things that are built in and already have names usually just leads to more confusion imo |
||
// Wrap the `search` value from the MemoryHistory using `URLSearchParams` in order to standardize it. | ||
return new URLSearchParams(this.history.location.search).toString(); | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ import { Simulator } from '../test_utilities/simulator'; | |
// Extend jest with a custom matcher | ||
import '../test_utilities/extend_jest'; | ||
import { noAncestorsTwoChildrenWithRelatedEventsOnOrigin } from '../data_access_layer/mocks/no_ancestors_two_children_with_related_events_on_origin'; | ||
import { urlSearch } from '../test_utilities/url_search'; | ||
|
||
let simulator: Simulator; | ||
let databaseDocumentID: string; | ||
|
@@ -89,7 +90,7 @@ describe('Resolver, when analyzing a tree that has no ancestors and 2 children', | |
await expect( | ||
simulator.map(() => ({ | ||
// the query string has a key showing that the second child is selected | ||
queryStringSelectedNode: simulator.queryStringValues().selectedNode, | ||
search: simulator.historyLocationSearch, | ||
// the second child is rendered in the DOM, and shows up as selected | ||
selectedSecondChildNodeCount: simulator.selectedProcessNode(entityIDs.secondChild) | ||
.length, | ||
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This now compares the url query strings. |
||
}), | ||
// The second child is rendered and has `[aria-selected]` | ||
selectedSecondChildNodeCount: 1, | ||
// The origin child is rendered and doesn't have `[aria-selected]` | ||
|
@@ -166,9 +169,6 @@ describe('Resolver, when analyzing a tree that has two related events for the or | |
await expect( | ||
simulator.map(() => simulator.processNodeSubmenuItems().map((node) => node.text())) | ||
).toYieldEqualTo(['2 registry']); | ||
await expect( | ||
simulator.map(() => simulator.processNodeSubmenuItems().length) | ||
).toYieldEqualTo(1); | ||
}); | ||
}); | ||
describe('and when the related events button is clicked again', () => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -152,9 +152,11 @@ describe(`Resolver: when analyzing a tree with no ancestors and two children, an | |
]); | ||
}); | ||
it("should have the first node's ID in the query string", async () => { | ||
await expect(simulator().map(() => simulator().queryStringValues())).toYieldEqualTo({ | ||
selectedNode: [entityIDs.origin], | ||
}); | ||
await expect(simulator().map(() => simulator().historyLocationSearch)).toYieldEqualTo( | ||
urlSearch(resolverComponentInstanceID, { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This now compares the url query strings. |
||
selectedEntityID: entityIDs.origin, | ||
}) | ||
); | ||
}); | ||
describe('and when the node list link has been clicked', () => { | ||
beforeEach(async () => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { noAncestorsTwoChildren } from '../data_access_layer/mocks/no_ancestors_two_children'; | ||
import { Simulator } from '../test_utilities/simulator'; | ||
// Extend jest with a custom matcher | ||
import '../test_utilities/extend_jest'; | ||
import { urlSearch } from '../test_utilities/url_search'; | ||
|
||
let simulator: Simulator; | ||
let databaseDocumentID: string; | ||
let entityIDs: { origin: string; firstChild: string; secondChild: string }; | ||
|
||
// 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'; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this could use a doc level comment. Something like what is in the description of this PR. |
||
describe('Resolver, when analyzing a tree that has no ancestors and 2 children', () => { | ||
beforeEach(async () => { | ||
// create a mock data access layer | ||
const { metadata: dataAccessLayerMetadata, dataAccessLayer } = noAncestorsTwoChildren(); | ||
|
||
// save a reference to the entity IDs exposed by the mock data layer | ||
entityIDs = dataAccessLayerMetadata.entityIDs; | ||
|
||
// save a reference to the `_id` supported by the mock data layer | ||
databaseDocumentID = dataAccessLayerMetadata.databaseDocumentID; | ||
|
||
// create a resolver simulator, using the data access layer and an arbitrary component instance ID | ||
simulator = new Simulator({ databaseDocumentID, dataAccessLayer, resolverComponentInstanceID }); | ||
}); | ||
|
||
describe("when the second child node's first button has been clicked", () => { | ||
beforeEach(async () => { | ||
const node = await simulator.resolveWrapper(() => | ||
simulator.processNodeElements({ entityID: entityIDs.secondChild }).find('button') | ||
); | ||
if (node) { | ||
// Click the first button under the second child element. | ||
node.first().simulate('click'); | ||
} | ||
}); | ||
const expectedSearch = urlSearch(resolverComponentInstanceID, { | ||
selectedEntityID: 'secondChild', | ||
}); | ||
it(`should have a url search of ${expectedSearch}`, async () => { | ||
await expect(simulator.map(() => simulator.historyLocationSearch)).toYieldEqualTo( | ||
urlSearch(resolverComponentInstanceID, { selectedEntityID: 'secondChild' }) | ||
); | ||
}); | ||
describe('when the resolver component gets unmounted', () => { | ||
beforeEach(() => { | ||
simulator.unmount(); | ||
}); | ||
it('should have a history location search of `""`', async () => { | ||
await expect(simulator.map(() => simulator.historyLocationSearch)).toYieldEqualTo(''); | ||
}); | ||
}); | ||
describe('when the resolver component has its component instance ID changed', () => { | ||
const newInstanceID = 'newID'; | ||
beforeEach(() => { | ||
simulator.resolverComponentInstanceID = newInstanceID; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not using your setter? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is the setter you are looking for |
||
}); | ||
it('should have a history location search of `""`', async () => { | ||
await expect(simulator.map(() => simulator.historyLocationSearch)).toYieldEqualTo(''); | ||
}); | ||
describe("when the user clicks the second child node's button again", () => { | ||
beforeEach(async () => { | ||
const node = await simulator.resolveWrapper(() => | ||
simulator.processNodeElements({ entityID: entityIDs.secondChild }).find('button') | ||
); | ||
if (node) { | ||
// Click the first button under the second child element. | ||
node.first().simulate('click'); | ||
} | ||
}); | ||
it(`should have a url search of ${urlSearch(newInstanceID, { | ||
selectedEntityID: 'secondChild', | ||
})}`, async () => { | ||
await expect(simulator.map(() => simulator.historyLocationSearch)).toYieldEqualTo( | ||
urlSearch(newInstanceID, { selectedEntityID: 'secondChild' }) | ||
); | ||
}); | ||
}); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,17 +8,16 @@ | |
|
||
import React, { useContext, useCallback } from 'react'; | ||
import { useSelector } from 'react-redux'; | ||
import { useEffectOnce } from 'react-use'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kqualters-elastic i blame you There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is on your permanent record |
||
import { EuiLoadingSpinner } from '@elastic/eui'; | ||
import { FormattedMessage } from '@kbn/i18n/react'; | ||
import { useResolverQueryParamCleaner } from './use_resolver_query_params_cleaner'; | ||
import * as selectors from '../store/selectors'; | ||
import { EdgeLine } from './edge_line'; | ||
import { GraphControls } from './graph_controls'; | ||
import { ProcessEventDot } from './process_event_dot'; | ||
import { useCamera } from './use_camera'; | ||
import { SymbolDefinitions, useResolverTheme } from './assets'; | ||
import { useStateSyncingActions } from './use_state_syncing_actions'; | ||
import { useResolverQueryParams } from './use_resolver_query_params'; | ||
import { StyledMapContainer, StyledPanel, GraphContainer } from './styles'; | ||
import { entityIDSafeVersion } from '../../../common/endpoint/models/event'; | ||
import { SideEffectContext } from './side_effect_context'; | ||
|
@@ -35,6 +34,8 @@ export const ResolverWithoutProviders = React.memo( | |
{ className, databaseDocumentID, resolverComponentInstanceID }: ResolverProps, | ||
refToForward | ||
) { | ||
// | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. weird comment |
||
useResolverQueryParamCleaner(); | ||
/** | ||
* This is responsible for dispatching actions that include any external data. | ||
* `databaseDocumentID` | ||
|
@@ -70,11 +71,6 @@ export const ResolverWithoutProviders = React.memo( | |
const hasError = useSelector(selectors.hasError); | ||
const activeDescendantId = useSelector(selectors.ariaActiveDescendant); | ||
const { colorMap } = useResolverTheme(); | ||
const { cleanUpQueryParams } = useResolverQueryParams(); | ||
|
||
useEffectOnce(() => { | ||
return () => cleanUpQueryParams(); | ||
}); | ||
|
||
return ( | ||
<StyledMapContainer className={className} backgroundColor={colorMap.resolverBackground}> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { useSelector } from 'react-redux'; | ||
import * as selectors from '../store/selectors'; | ||
|
||
/** | ||
* Get the query string keys used by this Resolver instance. | ||
*/ | ||
export function useQueryStringKeys(): { idKey: string; eventKey: string } { | ||
const resolverComponentInstanceID = useSelector(selectors.resolverComponentInstanceID); | ||
const idKey: string = `resolver-${resolverComponentInstanceID}-id`; | ||
const eventKey: string = `resolver-${resolverComponentInstanceID}-event`; | ||
return { | ||
idKey, | ||
eventKey, | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,11 +5,8 @@ | |
*/ | ||
|
||
import { useCallback, useMemo } from 'react'; | ||
// eslint-disable-next-line import/no-nodejs-modules | ||
import querystring from 'querystring'; | ||
import { useSelector } from 'react-redux'; | ||
import { useHistory, useLocation } from 'react-router-dom'; | ||
import * as selectors from '../store/selectors'; | ||
import { useQueryStringKeys } from './use_query_string_keys'; | ||
import { CrumbInfo } from './panels/panel_content_utilities'; | ||
|
||
export function useResolverQueryParams() { | ||
|
@@ -19,63 +16,40 @@ export function useResolverQueryParams() { | |
*/ | ||
const history = useHistory(); | ||
const urlSearch = useLocation().search; | ||
const resolverComponentInstanceID = useSelector(selectors.resolverComponentInstanceID); | ||
const uniqueCrumbIdKey: string = `resolver-${resolverComponentInstanceID}-id`; | ||
const uniqueCrumbEventKey: string = `resolver-${resolverComponentInstanceID}-event`; | ||
const { idKey, eventKey } = useQueryStringKeys(); | ||
const pushToQueryParams = useCallback( | ||
(newCrumbs: CrumbInfo) => { | ||
// Construct a new set of parameters from the current set (minus empty parameters) | ||
// by assigning the new set of parameters provided in `newCrumbs` | ||
const crumbsToPass = { | ||
...querystring.parse(urlSearch.slice(1)), | ||
[uniqueCrumbIdKey]: newCrumbs.crumbId, | ||
[uniqueCrumbEventKey]: newCrumbs.crumbEvent, | ||
}; | ||
(queryStringState: CrumbInfo) => { | ||
const urlSearchParams = new URLSearchParams(urlSearch); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. using |
||
|
||
// If either was passed in as empty, remove it from the record | ||
if (newCrumbs.crumbId === '') { | ||
delete crumbsToPass[uniqueCrumbIdKey]; | ||
urlSearchParams.set(idKey, queryStringState.crumbId); | ||
urlSearchParams.set(eventKey, queryStringState.crumbEvent); | ||
|
||
// If either was passed in as empty, remove it | ||
if (queryStringState.crumbId === '') { | ||
urlSearchParams.delete(idKey); | ||
} | ||
if (newCrumbs.crumbEvent === '') { | ||
delete crumbsToPass[uniqueCrumbEventKey]; | ||
if (queryStringState.crumbEvent === '') { | ||
urlSearchParams.delete(eventKey); | ||
} | ||
|
||
const relativeURL = { search: querystring.stringify(crumbsToPass) }; | ||
const relativeURL = { search: urlSearchParams.toString() }; | ||
// We probably don't want to nuke the user's history with a huge | ||
// trail of these, thus `.replace` instead of `.push` | ||
return history.replace(relativeURL); | ||
}, | ||
[history, urlSearch, uniqueCrumbIdKey, uniqueCrumbEventKey] | ||
[history, urlSearch, idKey, eventKey] | ||
); | ||
const queryParams: CrumbInfo = useMemo(() => { | ||
const parsed = querystring.parse(urlSearch.slice(1)); | ||
const crumbEvent = parsed[uniqueCrumbEventKey]; | ||
const crumbId = parsed[uniqueCrumbIdKey]; | ||
function valueForParam(param: string | string[]): string { | ||
if (Array.isArray(param)) { | ||
return param[0] || ''; | ||
} | ||
return param || ''; | ||
} | ||
const urlSearchParams = new URLSearchParams(urlSearch); | ||
return { | ||
crumbEvent: valueForParam(crumbEvent), | ||
crumbId: valueForParam(crumbId), | ||
// Use `''` for backwards compatibility with deprecated code. | ||
crumbEvent: urlSearchParams.get(eventKey) ?? '', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a weird behavior. We should not confuse |
||
crumbId: urlSearchParams.get(idKey) ?? '', | ||
}; | ||
}, [urlSearch, uniqueCrumbIdKey, uniqueCrumbEventKey]); | ||
|
||
const cleanUpQueryParams = () => { | ||
const crumbsToPass = { | ||
...querystring.parse(urlSearch.slice(1)), | ||
}; | ||
delete crumbsToPass[uniqueCrumbIdKey]; | ||
delete crumbsToPass[uniqueCrumbEventKey]; | ||
const relativeURL = { search: querystring.stringify(crumbsToPass) }; | ||
history.replace(relativeURL); | ||
}; | ||
}, [urlSearch, idKey, eventKey]); | ||
|
||
return { | ||
pushToQueryParams, | ||
queryParams, | ||
cleanUpQueryParams, | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thoughts? @michaelolo24
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 , but would it be useful to be able to update the
databaseDocumentId
andcomponentInstanceId
? 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 necessaryThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes