You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2021/10/28 21:25:22 UTC
[GitHub] [thrift] thealmightygrant opened a new pull request #2474: THRIFT-5474: TSimpleJsonProtocol with inner quotes does not produce valid JSON.
thealmightygrant opened a new pull request #2474:
URL: https://github.com/apache/thrift/pull/2474
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.
- [x] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? (not required for trivial changes)
- [x] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [x] Did you squash your changes to a single commit? (not required, but preferred)
- [x] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [ ] If your change does not involve any code, include `[skip ci]` anywhere in the commit message to free up build resources.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] thealmightygrant closed pull request #2474: THRIFT-5474: TSimpleJsonProtocol with inner quotes does not produce valid JSON.
Posted by GitBox <gi...@apache.org>.
thealmightygrant closed pull request #2474:
URL: https://github.com/apache/thrift/pull/2474
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] emmenlau commented on pull request #2474: THRIFT-5474: TSimpleJsonProtocol with inner quotes does not produce valid JSON.
Posted by GitBox <gi...@apache.org>.
emmenlau commented on pull request #2474:
URL: https://github.com/apache/thrift/pull/2474#issuecomment-955163478
Thanks for the effort, @thealmightygrant ! Its always nice to see people going to the effort of making a pull request.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] thealmightygrant commented on a change in pull request #2474: THRIFT-5474: TSimpleJsonProtocol with inner quotes does not produce valid JSON.
Posted by GitBox <gi...@apache.org>.
thealmightygrant commented on a change in pull request #2474:
URL: https://github.com/apache/thrift/pull/2474#discussion_r739298670
##########
File path: lib/java/src/org/apache/thrift/protocol/TSimpleJSONProtocol.java
##########
@@ -314,6 +314,7 @@ public void writeString(String str) throws TException {
char c = str.charAt(i);
switch (c) {
case '"':
+ escape.append('\\');
case '\\':
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] thealmightygrant commented on pull request #2474: THRIFT-5474: TSimpleJsonProtocol with inner quotes does not produce valid JSON.
Posted by GitBox <gi...@apache.org>.
thealmightygrant commented on pull request #2474:
URL: https://github.com/apache/thrift/pull/2474#issuecomment-954881609
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on a change in pull request #2474: THRIFT-5474: TSimpleJsonProtocol with inner quotes does not produce valid JSON.
Posted by GitBox <gi...@apache.org>.
Jens-G commented on a change in pull request #2474:
URL: https://github.com/apache/thrift/pull/2474#discussion_r738828325
##########
File path: lib/java/src/org/apache/thrift/protocol/TSimpleJSONProtocol.java
##########
@@ -314,6 +314,7 @@ public void writeString(String str) throws TException {
char c = str.charAt(i);
switch (c) {
case '"':
+ escape.append('\\');
case '\\':
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org