field names in maps are not properly escaped in toJson()

[sfdcsergei]

CharBuf.addJsonFieldName() does not check for chars that need to be escaped in valid JSON. Here's the sample:

    @Test
    public void testEscapesInFieldName(){
        String json = "{\"\\\"a\":5}";
        System.out.println(json); // {"\"a":5}

        Object map = JsonFactory.fromJson(json);
        System.out.println(map); // {"a=5}

        String json1 = JsonFactory.toJson(map);
        System.out.println(json1); // {""a":5}

        assertEquals(json, json1);
    }

[RichardHightower]

I will take a look. We just did a boon release. And I am working on QBit at a break neck speed. But I will circle back to this...

So this test fails correct?

[sfdcsergei]

It does fail unfortunately.

[darxriggs]

I think it's the same issue as in Groovy. I already fixed it there.

[RichardHightower]

As I mentioned in the comment, that does fix the issue but where is the issue coming from.

Java field names are safe for JSON with no escaping no?

Is the issue just for maps, in that case the fix should be only for map serialization.

Checking for escape carriers for each field will add a lot of processing time for serialization.

Did you run a perf test before/after the change? I bet there was a significant drop in performance.

Which is why I am asking... when does this happen?

Is is just for serializing maps?

I'd rather add it just for map serialization if that is the biggest concern, and may even want to make it optional.

My goal is not to penalize the 99% use case perf for the 1% use case fix. My goal is not to penalize the 80% use case perf for the 20% use case fix.

I can't accept the pull request as is. I want to brainstorm about it more.

On Wed, Mar 18, 2015 at 3:46 PM, René Scheibe notifications@github.com wrote:

I think it's the same issue as in Groovy. I already fixed it there https://github.com/darxriggs/groovy-core/commit/95855de54335b772f9866bceae5cfba127e666e5 .

— Reply to this email directly or view it on GitHub https://github.com/boonproject/boon/issues/298#issuecomment-83216195.

Rick Hightower (415) 968-9037 Profile http://www.google.com/profiles/RichardHightower

[darxriggs]

I was expecting you rejecting the pull request due to performance reasons. :-)

I did a performance test for my change in Groovy. It showed a degradation of about 15% for a map entry with key size 50.

[RichardHightower]

I saw perf test in the groovy JSON issue. Thanks btw for the pull request. I do appreciate it.

The most common case is serializing Java objects which should not have this issue.

I think I will add your patch as the defaults for serializing maps with the ability to turn it off.

Thanks again.

On Thursday, March 19, 2015, René Scheibe notifications@github.com wrote:

I was expecting you rejecting the pull request due to performance reasons. :-)

I did a performance test for my change in Groovy. It showed a degradation of about 15% for a map entry with key size 50.

— Reply to this email directly or view it on GitHub https://github.com/boonproject/boon/issues/298#issuecomment-83760475.

Rick Hightower (415) 968-9037 Profile http://www.google.com/profiles/RichardHightower