Get field mapping api should honour pretty flag #8806
Conversation
| responseBuilder.endObject(); | ||
| String responseStrings = responseBuilder.string(); | ||
|
|
||
| String expectedStrings ="{\n"+ |
There was a problem hiding this comment.
This seems fragile, since any slight change in the format/structure would cause the test to break. Could we just check the output contains some newlines (which non pretty results would not)?
Change assertion more simple Use writeRawField if pretty off Closes elastic#6552
|
|
||
| assertTrue(responseStrings.contains("\"mapping\" : {\n")); | ||
|
|
||
| // pretty=false |
There was a problem hiding this comment.
This comment doesn't add any value
|
LGTM, just a couple suggestions. |
| @@ -81,7 +82,8 @@ public RestResponse buildResponse(GetFieldMappingsResponse response, XContentBui | |||
| status = NOT_FOUND; | |||
| } | |||
| builder.startObject(); | |||
| response.toXContent(builder, ToXContent.EMPTY_PARAMS); | |||
| Map<String, String> param = Maps.newHashMap(); | |||
There was a problem hiding this comment.
this can be simplified to response.toXContent(builder,request);
|
Thx @johtani . Left one comment about the implementation. |
Fix two review comments Closes elastic#6552
Fix assertion only check ":{"
Closes elastic#6552
|
Fix all comments |
| responseBuilder.endObject(); | ||
| String responseStrings = responseBuilder.string(); | ||
|
|
||
| assertFalse(responseStrings.contains(":{")); |
There was a problem hiding this comment.
I think checking for newline is better than relying on pretty printing having space between key/object...
There was a problem hiding this comment.
@rjernst I see...
I have no idea for good assertion.
Do you have any other idea?
Output without pretty
{
"index" : {
"mappings" : {
"type" : {
"obj.subfield" : {
"full_name" : "obj.subfield",
"mapping":{"subfield":{"type":"string","index":"not_analyzed"}}
},
"field1" : {
"full_name" : "field1",
"mapping":{"field1":{"type":"string"}}
}
}
}
}
}
Output with pretty
{
"index" : {
"mappings" : {
"type" : {
"obj.subfield" : {
"full_name" : "obj.subfield",
"mapping" : {
"subfield" : {
"type" : "string",
"index" : "not_analyzed"
}
}
},
"field1" : {
"full_name" : "field1",
"mapping" : {
"field1" : {
"type" : "string"
}
}
}
}
}
}
}
There was a problem hiding this comment.
maybe just take the output you get from the API and re-feed it to a builder you know is pretty printing and make sure the output is identical to the input?
There was a problem hiding this comment.
Thx @bleskes ! I changed the assertion and pushed. Does it make sense?
|
LGTM (note that @rjernst had a comment) |
Change an assertion using re-parse pretty printing Closes elastic#6552
|
|
||
| XContentBuilder prettyJsonBuilder = XContentFactory.jsonBuilder().prettyPrint(); | ||
| prettyJsonBuilder.copyCurrentStructure(XContentFactory.xContent(responseStrings).createParser(responseStrings)); | ||
| assertThat(responseStrings, equalTo(prettyJsonBuilder.string())); |
|
LGTM :) |
|
|
||
| prettyJsonBuilder = XContentFactory.jsonBuilder().prettyPrint(); | ||
| prettyJsonBuilder.copyCurrentStructure(XContentFactory.xContent(responseStrings).createParser(responseStrings)); | ||
| assertThat(responseStrings,not(equalTo(prettyJsonBuilder.string()))); |
There was a problem hiding this comment.
Shouldn't this be equal to the jsonBuilder().string() above, without adding .prettyPrint()? And a nitpick: please add a space after the comma..
|
@johtani Thanks, LGTM. I left one minor comment. No need for another review regardless of if/how that is addressed. |
|
Merged 80bd698 |
clintongormley
added
:Core/Infra/Transport API
clintongormley
changed the title
Use builder.field method instead of XContentHelper#writeRawField
Closes #6552