commons-lang icon indicating copy to clipboard operation
commons-lang copied to clipboard

Fixed a problem when parsing Chinese using the ToStringStyle.JSON_STYLE

Open ch0ice opened this issue 5 years ago • 2 comments

When I write the following code

//org.apache.commons.lang3.builder.ReflectionToStringBuilderTest
public static class TestJsonStyle{
        private String a;
        public void setA(String a){
            this.a = a;
        }
        @Override
        public String toString() {
            return ReflectionToStringBuilder.toString(this, ToStringStyle.JSON_STYLE);
        }
    }

    @Test
    public void testJsonStyle() {
        TestJsonStyle jsonStyle = new TestJsonStyle();
        jsonStyle.setA("测试");
        Assertions.assertEquals("{\"a\":\"测试\"}",jsonStyle.toString());
    }

You get the following result,I don't think this is correct

{"a":"\u6D4B\u8BD5"}

When I change the following code, the result works fine

//org.apache.commons.lang3.builder.ToStringStyle
private void appendValueAsString(final StringBuffer buffer, final String value) {
//            buffer.append('"').append(StringEscapeUtils.escapeJson(value)).append('"');
            buffer.append('"').append(value).append('"');
}
//{"a":"测试"}

But when I did the unit test, I found out An error occurred on this unit test When I looked at the test case, I thought there was something wrong with the test case itself

//org.apache.commons.lang3.builder.JsonToStringStyleTest
@Test
    public void testLANG1395() {

//        assertEquals("{\"name\":\"value\"}", new ToStringBuilder(base).append("name", "value").toString());
//        assertEquals("{\"name\":\"\"}", new ToStringBuilder(base).append("name", "").toString());
//        assertEquals("{\"name\":\"\\\"\"}", new ToStringBuilder(base).append("name", '"').toString());
//        assertEquals("{\"name\":\"\\\\\"}", new ToStringBuilder(base).append("name", '\\').toString());
//        assertEquals("{\"name\":\"Let's \\\"quote\\\" this\"}", new ToStringBuilder(base).append("name", "Let's \"quote\" this").toString());

        assertEquals("{\"name\":\"value\"}", new ToStringBuilder(base).append("name", "value").toString());
        assertEquals("{\"name\":\"\"}", new ToStringBuilder(base).append("name", "").toString());
        assertEquals("{\"name\":\"\"\"}", new ToStringBuilder(base).append("name", '"').toString());
        assertEquals("{\"name\":\"\\\"}", new ToStringBuilder(base).append("name", '\\').toString());
        assertEquals("{\"name\":\"Let's \"quote\" this\"}", new ToStringBuilder(base).append("name", "Let's \"quote\" this").toString());

    }

Or when I write it this way

System.out.println(new ToStringBuilder(base).append("name", '"').toString());
//r1(Before fix)      {"name":"\""}
//r2(After fix)      {"name":"""} 

I think the right result is r2 instead of r1

ch0ice avatar Jul 05 '19 10:07 ch0ice

Coverage Status

Coverage remained the same at 95.339% when pulling 8efff432a66d0e2d9611254244703640e0b42e09 on ch0ice:master into d21a59baa559f57ead53a5bab22d60333dc46041 on apache:master.

coveralls avatar Jul 06 '19 12:07 coveralls

I don't feel confident enough to vote +1 on this change. It seems to fix the bug described here, and the test is passing on Travis. But I am concerned about the change of removing the code that seems to escape the JSON value. It probably had a reason for being used I guess.

But will take a better look later when I have more time to try the code and read code and commit history. Thanks for the pull request!

Bruno

kinow avatar Aug 20 '19 09:08 kinow