THRIFT-5474: TSimpleJsonProtocol with inner quotes does not produce valid JSON. #2474
Conversation
thealmightygrant
force-pushed
the
fix-inner-quotes-simple-json
branch
from
3d43b82
to
5989296
Compare
…alid JSON. There is an issue in the generation of JSON from the TSimpleJSONProtocol, where if there are quotes inside of a string, then those quotes will not be serialized correctly. These quotes one more layer of escaping to produce valid JSON. I have included JSON parsing into the test for TSimpleJSONProtocol to ensure that this does not happen if there are changes to this class in the future.
thealmightygrant
force-pushed
the
fix-inner-quotes-simple-json
branch
from
5989296
to
ce40f97
Compare
| @@ -314,6 +314,7 @@ public void writeString(String str) throws TException { | |||
| char c = str.charAt(i); | |||
| switch (c) { | |||
| case '"': | |||
| escape.append('\\'); | |||
| case '\\': | |||
There was a problem hiding this comment.
So this is the whole net patch content, buried under a pile of unrelated other changes.
Plus I don't think this change is even remotely correct
There was a problem hiding this comment.
I'll work up a more complete test example for you today. I can see how you might not think this would make a difference. The other code is intended to try to parse the JSON strings produced from these tests, it will throw an exception otherwise.
|
I was unable to reproduce this in the latest version of thrift, so it must be some artifact of the version that I am using that is no longer relevant. For posterity, this occurs in thrift 0.9.3. See the latest commit for an explicit example of how this would fail in that version of the thrift library. |
|
Thanks for the effort, @thealmightygrant ! Its always nice to see people going to the effort of making a pull request. |
There is an issue in the generation of JSON from the TSimpleJSONProtocol, where if there are quotes inside of a string, then those quotes will not be serialized correctly. These quotes need one more layer of escaping to produce valid JSON.
I have also added the Jackson library to these tests to ensure that any future changes to this library produce valid JSON.
[skip ci]anywhere in the commit message to free up build resources.