Skip to content

Commit

Permalink
[Resolver] Stale query string values are removed when resolver's comp…
Browse files Browse the repository at this point in the history
…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*
  • Loading branch information
Robert Austin committed Aug 14, 2020
1 parent 603292c commit 2e52612
Show file tree
Hide file tree
Showing 8 changed files with 220 additions and 72 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,6 @@ import { sideEffectSimulatorFactory } from '../../view/side_effect_simulator_fac
* 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.
Expand Down Expand Up @@ -69,7 +65,6 @@ export class Simulator {
databaseDocumentID?: string;
history?: HistoryPackageHistoryInterface<never>;
}) {
this.resolverComponentInstanceID = resolverComponentInstanceID;
// create the spy middleware (for debugging tests)
this.spyMiddleware = spyMiddlewareFactory();

Expand Down Expand Up @@ -98,7 +93,7 @@ export class Simulator {
// Render Resolver via the `MockResolver` component, using `enzyme`.
this.wrapper = mount(
<MockResolver
resolverComponentInstanceID={this.resolverComponentInstanceID}
resolverComponentInstanceID={resolverComponentInstanceID}
history={this.history}
sideEffectSimulator={this.sideEffectSimulator}
store={this.store}
Expand All @@ -108,6 +103,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
Expand Down Expand Up @@ -203,13 +219,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 {
// Wrap the `search` value from the MemoryHistory using `URLSearchParams` in order to standardize it.
return new URLSearchParams(this.history.location.search).toString();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -93,7 +94,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,
Expand All @@ -102,7 +103,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,
}),
// The second child is rendered and has `[aria-selected]`
selectedSecondChildNodeCount: 1,
// The origin child is rendered and doesn't have `[aria-selected]`
Expand Down Expand Up @@ -175,9 +178,6 @@ describe('Resolver, when analyzing a tree that has two related events for the or
simulator.testSubject('resolver:map:node-submenu-item').map((node) => node.text())
)
).toYieldEqualTo(['2 registry']);
await expect(
simulator.map(() => simulator.testSubject('resolver:map:node-submenu-item').length)
).toYieldEqualTo(1);
});
});
describe('and when the related events button is clicked again', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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, {
selectedEntityID: entityIDs.origin,
})
);
});
describe('and when the node list link has been clicked', () => {
beforeEach(async () => {
Expand Down
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';

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;
});
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
Expand Up @@ -8,17 +8,16 @@

import React, { useContext, useCallback } from 'react';
import { useSelector } from 'react-redux';
import { useEffectOnce } from 'react-use';
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';
Expand All @@ -35,6 +34,7 @@ export const ResolverWithoutProviders = React.memo(
{ className, databaseDocumentID, resolverComponentInstanceID }: ResolverProps,
refToForward
) {
useResolverQueryParamCleaner();
/**
* This is responsible for dispatching actions that include any external data.
* `databaseDocumentID`
Expand Down Expand Up @@ -70,11 +70,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}>
Expand Down
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
Expand Up @@ -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() {
Expand All @@ -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);

// 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) ?? '',
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,
};
}
Loading

0 comments on commit 2e52612

Please sign in to comment.