-
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
fixing mangling of floating point numbers by console #23685
fixing mangling of floating point numbers by console #23685
Conversation
💔 Build Failed |
retest |
💔 Build Failed |
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.
Can you jog my memory, why does this change require the unicodeBytesToString
stuff?
|
||
function resolveUri(base, path) { | ||
return `${trimRight(base, '/')}/${trimLeft(path, '/')}`; | ||
let pathToUse = `${trimRight(base, '/')}/${trimLeft(path, '/')}`; | ||
const questionMarkIndex = pathToUse.indexOf('?'); |
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.
nit: You can use String.prototype.includes
now, so no need for indexOf checks and extra variables.
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.
Doesn't work in IE11 though (we've had bugs related to that).
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.
That's odd... it should be in the babel polyfill we use. Anyway, something I can sort out separately from this PR.
we have to decode the bytes raw on the server side because the wreck library does JSON parse on the response otherwise, and the 10.0 gets mangled to 10 there. So basically there were two places we had to not treat the ES response as JSON. |
💔 Build Failed |
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.
When loading console in the browser, I'm getting a RangeError
exception on the server resulting in a 500 response on the request to /api/console/proxy?path=_template&method=GET
:
RangeError: Uncaught error: Maximum call stack size exceeded
at unicodeBytesToString (/p/kibana/src/core_plugins/console/server/lib/decode.js:24:39)
at _wreck2.default.read (/p/kibana/src/core_plugins/console/server/proxy_route.js:129:36)
at finish (/p/kibana/node_modules/wreck/lib/index.js:369:20)
at wrapped (/p/kibana/node_modules/hoek/lib/index.js:879:20)
at module.exports.internals.Recorder.onReaderFinish (/p/kibana/node_modules/wreck/lib/index.js:443:16)
at Object.onceWrapper (events.js:313:30)
at emitNone (events.js:111:20)
at module.exports.internals.Recorder.emit (events.js:208:7)
at finishMaybe (_stream_writable.js:614:14)
at endWritable (_stream_writable.js:622:3)
at module.exports.internals.Recorder.Writable.end (_stream_writable.js:573:5)
at IncomingMessage.onend (_stream_readable.js:595:10)
at Object.onceWrapper (events.js:313:30)
at emitNone (events.js:111:20)
at IncomingMessage.emit (events.js:208:7)
at endReadableNT (_stream_readable.js:1064:12)
at _combinedTickCallback (internal/process/next_tick.js:138:11)
at process._tickDomainCallback (internal/process/next_tick.js:218:9)
Since it's in unicodeBytesToString
it seemed clear it was from this PR, but I verified in master just in case and it isn't happening there.
💔 Build Failed |
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.
LGTM
I tested multiple requests through console, including some that had query parameters, and everything seemed to work as expected.
💔 Build Failed |
💔 Build Failed |
retest |
💔 Build Failed |
💚 Build Succeeded |
* fixing mangling of floating point numbers by console * fixing tests * fixing issue with large requests * restoring old code for server side as it handles large responses better
This change preserves floating point numbers as floats when displayed in console results. For example, previously a float
10.0
was getting mangled into10
when console displayed it.Closes #23530