Skip to content

Commit

Permalink
Merge pull request #458 from everett980/ddg-hash-withhold-visEncoding
Browse files Browse the repository at this point in the history 
Add and consume hash to withhold visEncoding
  • Loading branch information
@everett980
everett980 committed Oct 11, 2019
2 parents 88374c6 + 5278f12 commit c5eaca5
Show file tree
Hide file tree
Showing 11 changed files with 158 additions and 50 deletions.
2 changes: 2 additions & 0 deletions packages/jaeger-ui/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
"@types/lodash": "^4.14.123",
"@types/memoize-one": "4.1.1",
"@types/moment": "^2.13.0",
"@types/object-hash": "^1.3.0",
"@types/react-copy-to-clipboard": "^4.2.6",
"@types/react-icons": "2.2.7",
"@types/react-redux": "^5.0.6",
Expand Down Expand Up @@ -72,6 +73,7 @@
"match-sorter": "^3.1.1",
"memoize-one": "^5.0.0",
"moment": "^2.18.1",
"object-hash": "^1.3.1",
"prop-types": "^15.5.10",
"query-string": "^6.3.0",
"raven-js": "^3.22.1",
Expand Down
1 change: 0 additions & 1 deletion packages/jaeger-ui/src/components/DeepDependencies/Header/index.css
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ limitations under the License.
box-shadow: 0 1px 3px 0 rgba(0, 0, 0, 0.1);
display: flex;
flex-direction: column;
flex: 1 1;
position: relative;
z-index: 10;
}
Expand Down
50 changes: 39 additions & 11 deletions packages/jaeger-ui/src/components/DeepDependencies/index.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,26 @@ describe('DeepDependencyGraphPage', () => {
expect(props.history.push).toHaveBeenCalledTimes(1);
});

it('includes props.graphState.model.hash iff it is truthy', () => {
ddgPageImpl.updateUrlState({});
expect(getUrlSpy).toHaveBeenLastCalledWith(expect.not.objectContaining({ hash: expect.anything() }));

const hash = 'testHash';
const propsWithHash = {
...props,
graphState: {
...props.graphState,
model: {
...props.graphState.model,
hash,
},
},
};
const ddgPageWithHash = new DeepDependencyGraphPageImpl(propsWithHash);
ddgPageWithHash.updateUrlState({});
expect(getUrlSpy).toHaveBeenLastCalledWith(expect.objectContaining({ hash }));
});

describe('setDistance', () => {
const mockNewEncoding = '1';
let encodeDistanceSpy;
Expand Down Expand Up @@ -351,20 +371,31 @@ describe('DeepDependencyGraphPage', () => {
},
};
const ownProps = { location: { search } };
const mockGraphModel = { getVisible: () => ({}) };
const mockGraph = { getVisible: () => ({}) };
const hash = 'testHash';
const doneState = _set(
{ ...state },
['ddg', getStateEntryKey({ service, operation, start: 0, end: 0 })],
{
model: { hash },
state: fetchedState.DONE,
}
);
let getUrlStateSpy;
let sanitizeUrlStateSpy;
let makeGraphSpy;

beforeAll(() => {
getUrlStateSpy = jest.spyOn(url, 'getUrlState');
sanitizeUrlStateSpy = jest.spyOn(url, 'sanitizeUrlState');
makeGraphSpy = jest.spyOn(GraphModel, 'makeGraph');
});

beforeEach(() => {
getUrlStateSpy.mockReset();
getUrlStateSpy.mockReturnValue(expected.urlState);
makeGraphSpy.mockReset();
makeGraphSpy.mockReturnValue(mockGraphModel);
makeGraphSpy.mockReturnValue(mockGraph);
});

it('uses gets relevant params from location.search', () => {
Expand Down Expand Up @@ -404,22 +435,19 @@ describe('DeepDependencyGraphPage', () => {
const result = mapStateToProps(reduxState, ownProps);
expect(result.graph).toBe(undefined);

const doneState = _set(
{ ...state },
['ddg', getStateEntryKey({ service, operation, start: 0, end: 0 })],
{
model: {},
state: fetchedState.DONE,
}
);
const doneResult = mapStateToProps(doneState, ownProps);
expect(doneResult.graph).toBe(mockGraphModel);
expect(doneResult.graph).toBe(mockGraph);
});

it('includes services and operationsForService', () => {
expect(mapStateToProps(state, ownProps)).toEqual(
expect.objectContaining({ operationsForService, services })
);
});

it('sanitizes urlState', () => {
mapStateToProps(doneState, ownProps);
expect(sanitizeUrlStateSpy).toHaveBeenLastCalledWith(expected.urlState, hash);
});
});
});
15 changes: 8 additions & 7 deletions packages/jaeger-ui/src/components/DeepDependencies/index.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import { connect } from 'react-redux';

import Header from './Header';
import Graph from './Graph';
import { getUrl, getUrlState } from './url';
import { getUrl, getUrlState, sanitizeUrlState } from './url';
import ErrorMessage from '../common/ErrorMessage';
import LoadingIndicator from '../common/LoadingIndicator';
import { extractUiFindFromState, TExtractUiFindFromStateReturn } from '../common/UiFindInput';
Expand Down Expand Up @@ -79,8 +79,6 @@ export class DeepDependencyGraphPageImpl extends React.PureComponent<TProps> {
}
}

headerWrapper: React.RefObject<HTMLDivElement> = React.createRef();

constructor(props: TProps) {
super(props);
DeepDependencyGraphPageImpl.fetchModelIfStale(props);
Expand Down Expand Up @@ -173,8 +171,11 @@ export class DeepDependencyGraphPageImpl extends React.PureComponent<TProps> {
toggleShowOperations = (enable: boolean) => this.updateUrlState({ showOp: enable });

updateUrlState = (newValues: Partial<TDdgSparseUrlState>) => {
const { uiFind, urlState, history } = this.props;
history.push(getUrl({ uiFind, ...urlState, ...newValues }));
const { graphState, history, uiFind, urlState } = this.props;
const getUrlArg = { uiFind, ...urlState, ...newValues };
const hash = _get(graphState, 'model.hash');
if (hash) getUrlArg.hash = hash;
history.push(getUrl(getUrlArg));
};

render() {
Expand Down Expand Up @@ -225,7 +226,7 @@ export class DeepDependencyGraphPageImpl extends React.PureComponent<TProps> {

return (
<div className="Ddg">
<div ref={this.headerWrapper}>
<div>
<Header
density={density}
distanceToPathElems={distanceToPathElems}
Expand Down Expand Up @@ -272,7 +273,7 @@ export function mapStateToProps(state: ReduxState, ownProps: TOwnProps): TReduxP
graphState,
services,
operationsForService,
urlState,
urlState: sanitizeUrlState(urlState, _get(graphState, 'model.hash')),
...extractUiFindFromState(state),
};
}
Expand Down
66 changes: 48 additions & 18 deletions packages/jaeger-ui/src/components/DeepDependencies/url.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
import queryString from 'query-string';
import * as reactRouterDom from 'react-router-dom';

import { ROUTE_PATH, matches, getUrl, getUrlState } from './url';
import { ROUTE_PATH, matches, getUrl, getUrlState, sanitizeUrlState } from './url';

describe('DeepDependencyGraph/url', () => {
describe('matches', () => {
Expand Down Expand Up @@ -65,9 +65,9 @@ describe('DeepDependencyGraph/url', () => {
});

describe('getUrlState', () => {
const search = 'test search';
const density = 'test density';
const end = '900';
const hash = 'test hash';
const operation = 'operationName';
const service = 'serviceName';
const showOp = '0';
Expand All @@ -76,6 +76,7 @@ describe('DeepDependencyGraph/url', () => {
const acceptableParams = {
density,
end,
hash,
operation,
service,
showOp,
Expand All @@ -85,12 +86,15 @@ describe('DeepDependencyGraph/url', () => {
const expectedParams = {
density,
end: Number.parseInt(end, 10),
hash,
operation,
service,
showOp: Boolean(+showOp),
start: Number.parseInt(start, 10),
visEncoding,
};
let count = 0;
const getSearch = () => `test search ${count++}`;
let warnSpy;
let parseSpy;

Expand All @@ -109,35 +113,33 @@ describe('DeepDependencyGraph/url', () => {
});

it('gets all values from queryString', () => {
const search = getSearch();
parseSpy.mockReturnValue(acceptableParams);
expect(getUrlState(search)).toEqual(expectedParams);
expect(parseSpy).toHaveBeenCalledWith(search);
});

it('handles absent values', () => {
['end', 'operation', 'service', 'start', 'visEncoding'].forEach(param => {
['end', 'hash', 'operation', 'service', 'start', 'visEncoding'].forEach(param => {
const { [param]: unused, ...rest } = expectedParams;
const { [param]: alsoUnused, ...rv } = acceptableParams;
parseSpy.mockReturnValue(rv);
expect(getUrlState(search)).toEqual(rest);
expect(parseSpy).toHaveBeenLastCalledWith(search);
expect(getUrlState(getSearch())).toEqual(rest);
});
});

it('defaults `showOp` to true', () => {
const { showOp: unused, ...rest } = expectedParams;
const { showOp: alsoUnused, ...rv } = acceptableParams;
parseSpy.mockReturnValue(rv);
expect(getUrlState(search)).toEqual({ ...rest, showOp: true });
expect(parseSpy).toHaveBeenLastCalledWith(search);
expect(getUrlState(getSearch())).toEqual({ ...rest, showOp: true });
});

it("defaults `density` to 'ppe'", () => {
const { density: unused, ...rest } = expectedParams;
const { density: alsoUnused, ...rv } = acceptableParams;
parseSpy.mockReturnValue(rv);
expect(getUrlState(search)).toEqual({ ...rest, density: 'ppe' });
expect(parseSpy).toHaveBeenLastCalledWith(search);
expect(getUrlState(getSearch())).toEqual({ ...rest, density: 'ppe' });
});

it('ignores extraneous query parameters', () => {
Expand All @@ -148,31 +150,59 @@ describe('DeepDependencyGraph/url', () => {
...extraneous,
...acceptableParams,
});
expect(getUrlState(search)).toEqual(expect.not.objectContaining(extraneous));
expect(parseSpy).toHaveBeenCalledWith(search);
expect(getUrlState(getSearch())).toEqual(expect.not.objectContaining(extraneous));
});

it('omits falsy values', () => {
['end', 'operation', 'service', 'start', 'visEncoding'].forEach(param => {
['end', 'hash', 'operation', 'service', 'start', 'visEncoding'].forEach(param => {
[null, undefined, ''].forEach(falsyPossibility => {
parseSpy.mockReturnValue({ ...expectedParams, [param]: falsyPossibility });
expect(Reflect.has(getUrlState(search), param)).toBe(false);
expect(parseSpy).toHaveBeenLastCalledWith(search);
expect(Reflect.has(getUrlState(getSearch()), param)).toBe(false);
});
});
});

it('handles and warns on duplicate values', () => {
['end', 'operation', 'service', 'showOp', 'start', 'visEncoding'].forEach(param => {
['end', 'hash', 'operation', 'service', 'showOp', 'start', 'visEncoding'].forEach(param => {
const secondParam = `second ${acceptableParams[param]}`;
parseSpy.mockReturnValue({
...acceptableParams,
[param]: [acceptableParams[param], secondParam],
});
expect(getUrlState(search)[param]).toBe(expectedParams[param]);
expect(warnSpy).toHaveBeenLastCalledWith(expect.stringContaining(secondParam));
expect(parseSpy).toHaveBeenLastCalledWith(search);
expect(getUrlState(getSearch())[param]).toBe(expectedParams[param]);
});
});

it('memoizes correctly', () => {
const search = getSearch();
parseSpy.mockReturnValue(acceptableParams);
expect(getUrlState(search)).toBe(getUrlState(search));
expect(getUrlState(getSearch())).not.toBe(getUrlState(search));
});
});

describe('sanitizeUrlState', () => {
const hash = 'test hash';
const urlStateWithoutVisEncoding = {
hash,
operation: 'test operation',
service: 'test service',
};
const urlState = {
...urlStateWithoutVisEncoding,
visEncoding: 'test visEncoding',
};

it('returns provided state without visEncoding if hash was not provided', () => {
expect(sanitizeUrlState(urlState)).toEqual(urlStateWithoutVisEncoding);
});

it('returns provided state without visEncoding if provided hash does not match urlState hash', () => {
expect(sanitizeUrlState(urlState, `not ${hash}`)).toEqual(urlStateWithoutVisEncoding);
});

it('returns provided state visEncoding if provided hash does matches urlState hash', () => {
expect(sanitizeUrlState(urlState, hash)).toBe(urlState);
});
});
});
20 changes: 18 additions & 2 deletions packages/jaeger-ui/src/components/DeepDependencies/url.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
// limitations under the License.

import _isEmpty from 'lodash/isEmpty';
import memoizeOne from 'memoize-one';
import queryString from 'query-string';
import { matchPath } from 'react-router-dom';

Expand Down Expand Up @@ -49,10 +50,11 @@ function firstParam(arg: string | string[]): string {
return arg;
}

export function getUrlState(search: string): TDdgSparseUrlState {
export const getUrlState = memoizeOne(function getUrlState(search: string): TDdgSparseUrlState {
const {
density = EDdgDensity.PreventPathEntanglement,
end,
hash,
operation,
service,
showOp = '1',
Expand All @@ -66,6 +68,9 @@ export function getUrlState(search: string): TDdgSparseUrlState {
if (end) {
rv.end = Number.parseInt(firstParam(end), 10);
}
if (hash) {
rv.hash = firstParam(hash);
}
if (operation) {
rv.operation = firstParam(operation);
}
Expand All @@ -79,4 +84,15 @@ export function getUrlState(search: string): TDdgSparseUrlState {
rv.visEncoding = firstParam(visEncoding);
}
return rv;
}
});

export const sanitizeUrlState = memoizeOne(function sanitizeUrlStateImpl(
state: TDdgSparseUrlState,
hash?: string
): TDdgSparseUrlState {
if (hash && state.hash === hash) {
return state;
}
const { visEncoding, ...sanitized } = state; // eslint-disable-line @typescript-eslint/no-unused-vars
return sanitized;
});
11 changes: 6 additions & 5 deletions packages/jaeger-ui/src/model/ddg/GraphModel/index.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,13 @@ describe('GraphModel', () => {
}

describe('constructor', () => {
const testGraph = new GraphModel({
ddgModel: simpleModel,
density: EDdgDensity.PreventPathEntanglement,
showOp: true,
});

it('creates five vertices and four edges for one-path ddg', () => {
const testGraph = new GraphModel({
ddgModel: simpleModel,
density: EDdgDensity.PreventPathEntanglement,
showOp: true,
});
validateGraph(testGraph, [
{
visIndices: [0],
Expand Down
Loading

0 comments on commit c5eaca5

Please sign in to comment.