[console] Replace text/plain fallback with application/json #12294
Conversation
|
jenkins, test it |
There was a problem hiding this comment.
This all seems really hacky to me, and it is brittle enough that I worry about us introducing regressions in the future.
Line 20 will split valid JSON if a string in the JSON includes a valid newline character. The only reason this works out OK is that a future line (line 22) will throw an exception when it tries to parse each individual "line", which happens to now be broken, and the error handling for line 22 happens to fallback to application/json.
I'm trying not to get too hung up on it since the existing implementation isn't immune to this flakiness either, but I'd feel more comfortable if there were some unit tests around this behavior.
|
Understandable, I will say that case was intentional but is hidden. In the case of multiline json, each line must be a valid json object. I'll get some tests together and see if I can make it more clear. |
|
Added tests, reminder note that console tests are available at http://localhost:5601/app/sense-tests |
jbudz
changed the title
|
I updated this, it only addresses #10504 now. |
| foo: 'bar' | ||
| }, null, 2)); | ||
|
|
||
| equal(contentType, APPLICATION_JSON); |
There was a problem hiding this comment.
I don't get it. This case adds newlines and spaces and yet the contenttype should be the one without _NEWLINE_?
There was a problem hiding this comment.
x-ndjson requires each line to be a valid json object. if there's lines that aren't parseable then we use application/json
| contentType = 'application/x-ndjson'; | ||
| } | ||
| catch (e) { | ||
| contentType = 'application/json'; |
There was a problem hiding this comment.
Are we absolutely 110% sure that it should always be this contentType when we fail for any reason?
There was a problem hiding this comment.
yes, elasticsearch won't accept content types outside of x-ndjson and json. it was text/plain before to prevent proxies and load balancers from parsing it, but in practice this also prevents us from getting detailed error messages back from es
There was a problem hiding this comment.
this also prevents us from getting detailed error messages back from es
Is this desired?
There was a problem hiding this comment.
imo, #13423 has an example of this case
| try { | ||
| // there is more than one line | ||
| const lines = body.split('\n').filter(Boolean); | ||
| if (!lines.length) throw new Error('not multiline json') |
There was a problem hiding this comment.
Calling split with a substring that doesn't have any matches results in an array of length 1, so this multiline error check will never fire. As a result, even a simple:
POST logstash-*/_search
{}
results in an error
|
This is good for review again. |
|
@jbudz You mentioned wanting to just send application/json always. Do you still want to do that? If so, then we probably shouldn't bother moving forward with this PR |
|
@epixa, mind taking another pass at this? |
|
LGTM |
* [console] Always set content-type for requests to es * [console] always set content type to application/json if there is a body * remove extra space
* [console] Always set content-type for requests to es * [console] always set content type to application/json if there is a body * remove extra space
This ensures that all elasticsearch queries sent from console that have a content-type set it to either application/json or application/x-ndjson.
Closes #10504