-
Notifications
You must be signed in to change notification settings - Fork 487
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
Don't rely on json-markup for non-json strings #587
Changes from all commits
20fb5ec
6c76e93
de51eb6
70cb812
b1671da
52e64dd
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 | ||
---|---|---|---|---|
|
@@ -46,9 +46,10 @@ describe('<KeyValuesTable>', () => { | |||
let wrapper; | ||||
|
||||
const data = [ | ||||
{ key: 'span.kind', value: 'client' }, | ||||
{ key: 'omg', value: 'mos-def' }, | ||||
{ key: 'numericString', value: '12345678901234567890' }, | ||||
{ key: 'span.kind', value: 'client', expected: 'client' }, | ||||
{ key: 'omg', value: 'mos-def', expected: 'mos-def' }, | ||||
{ key: 'numericString', value: '12345678901234567890', expected: '12345678901234567890' }, | ||||
{ key: 'numeric', value: 123456789, expected: '123456789' }, | ||||
{ key: 'jsonkey', value: JSON.stringify({ hello: 'world' }) }, | ||||
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. sorry, how does this one pass if it doesn't have 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. Line 141 in 52e64dd
|
||||
]; | ||||
|
||||
|
@@ -138,7 +139,7 @@ describe('<KeyValuesTable>', () => { | |||
expect(el.length).toBe(data.length); | ||||
el.forEach((valueDiv, i) => { | ||||
if (data[i].key !== 'jsonkey') { | ||||
expect(valueDiv.html()).toMatch(`"${data[i].value}"`); | ||||
expect(valueDiv.html()).toMatch(data[i].expected); | ||||
} | ||||
}); | ||||
}); | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,16 +25,42 @@ import './KeyValuesTable.css'; | |
|
||
const jsonObjectOrArrayStartRegex = /^(\[|\{)/; | ||
|
||
function parseIfComplexJson(value: any) { | ||
function tryParseJson(value: string) { | ||
// if the value is a string representing actual json object or array, then use json-markup | ||
if (typeof value === 'string' && jsonObjectOrArrayStartRegex.test(value)) { | ||
// otherwise just return as is | ||
try { | ||
return JSON.parse(value); | ||
// eslint-disable-next-line no-empty | ||
} catch (_) {} | ||
// otherwise just return as is | ||
try { | ||
return jsonObjectOrArrayStartRegex.test(value) ? JSON.parse(value) : value; | ||
} catch (_) { | ||
return value; | ||
} | ||
return value; | ||
} | ||
|
||
const stringMarkup = (value: string) => ( | ||
<div className="json-markup"> | ||
<span className="json-markup-string">{value}</span> | ||
</div> | ||
); | ||
|
||
function _jsonMarkup(value: any) { | ||
const markup = { __html: jsonMarkup(value) }; | ||
|
||
return ( | ||
// eslint-disable-next-line react/no-danger | ||
<div dangerouslySetInnerHTML={markup} /> | ||
); | ||
} | ||
|
||
function formatValue(value: any) { | ||
let content; | ||
|
||
if (typeof value === '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. Why does this logic need to change if the goal is to only remove the quotes? Attempting to parse any value as JSON led to bugs previously, where, for example, a string that looks like a number would get converted to number and change representation / precision. 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. but it has not changed - see a few lines below. 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. ok, thanks. Could you please add unit tests that cover the new conditions? There are only two lines that are not covered: https://codecov.io/gh/jaegertracing/jaeger-ui/compare/3335512110c60ace5b089e1b61d187a6d6ec4000...6c76e930f59843f41817effbf3d57c3f3a999367/diff |
||
const parsed = tryParseJson(value); | ||
content = typeof parsed === 'string' ? stringMarkup(parsed) : _jsonMarkup(parsed); | ||
} else { | ||
content = _jsonMarkup(value); | ||
} | ||
|
||
return <div className="ub-inline-block">{content}</div>; | ||
} | ||
|
||
export const LinkValue = (props: { href: string; title?: string; children: React.ReactNode }) => ( | ||
|
@@ -71,11 +97,7 @@ export default function KeyValuesTable(props: KeyValuesTableProps) { | |
<table className="u-width-100"> | ||
<tbody className="KeyValueTable--body"> | ||
{data.map((row, i) => { | ||
const markup = { | ||
__html: jsonMarkup(parseIfComplexJson(row.value)), | ||
}; | ||
// eslint-disable-next-line react/no-danger | ||
const jsonTable = <div className="ub-inline-block" dangerouslySetInnerHTML={markup} />; | ||
const jsonTable = formatValue(row.value); | ||
const links = linksGetter ? linksGetter(data, i) : null; | ||
let valueMarkup; | ||
if (links && links.length === 1) { | ||
|
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.
please keep this, it's a real use case that cased problems in the past.