Fixes #10176 Updating struct.to_json to escape the key string. #10577
Conversation
|
@alandonovan You've touched this code most in the last year. Can you take a look. |
| @@ -513,6 +513,7 @@ java_library( | |||
| "//third_party:guava", | |||
| "//third_party:jsr305", | |||
| "//third_party/protobuf:protobuf_java", | |||
| "//third_party:apache_commons_lang" | |||
There was a problem hiding this comment.
This is a new dependency, and it's totally unnecessary. There is already code in StructImpl.java to escape Java strings into JSON notation (see L319); please re-use it.
| @@ -299,7 +300,7 @@ private void printJson(Object value, StringBuilder sb, Location loc, String cont | |||
| throw new EvalException(loc, errorMessage); | |||
| } | |||
| sb.append("\""); | |||
| sb.append(entry.getKey()); | |||
| sb.append(StringEscapeUtils.escapeJava(String.valueOf(entry.getKey()))); | |||
There was a problem hiding this comment.
Don't use String.valueOf(x) here; it's equivalent to x.toString() except when x is null, which can't happen because this statement is dominated by the check on L291 (and even if it could, I would prefer to see a crash in that case than to silently emit incorrect JSON).
Use: (String) entry.getKey()
There was a problem hiding this comment.
Could you also apply escaping to the field names on L280? Field names should be valid identifiers, but this is not guaranteed, and we don't want to emit corrupt JSON in that case.
At that point, there are three places that emit quote escape(s) quote; could you refactor jsonEscapeString to accept a StringBuilder and emit the quotes too? Thanks.
| .replace("\r", "\\r") | ||
| .replace("\t", "\\t"); | ||
| .replace("\t", "\\t")); | ||
| if (appendColon) { |
There was a problem hiding this comment.
The appendColon logic has no business being in here, as it can be composed separately an no extra cost, resulting in a simpler function.
| private String jsonEscapeString(String string) { | ||
| return escapeDoubleQuotesAndBackslashesAndNewlines(string) | ||
| private void jsonEscapeString(StringBuilder stringBuilder, String string, boolean appendColon) { | ||
| stringBuilder.append("\""); |
There was a problem hiding this comment.
If you like, use append('"') to avoid the internal loop.
There was a problem hiding this comment.
Not sure I completely understand your suggestion. Was my change what you had in mind?
| @@ -333,10 +327,16 @@ private void printJson(Object value, StringBuilder sb, Location loc, String cont | |||
| } | |||
| } | |||
|
|
|||
| private String jsonEscapeString(String string) { | |||
| return escapeDoubleQuotesAndBackslashesAndNewlines(string) | |||
| private void jsonEscapeString(StringBuilder stringBuilder, String string, boolean appendColon) { | |||
There was a problem hiding this comment.
Let's call this function appendJSONStringLiteral, the StringBuilder 'out', and the String s.
dwhu
changed the title
|
Can someone please resolve the merge conflicts so we can merge this. |
Please let me know where to place an appopriate test. No existing test class appeared appropriate as far as I could tell.
- Adding another usage of jsonEscapeString - Removing dependency on StringEscapeUtils
- Updating method and paremeter naming of jsonEscapeString method
functions in StructImpl.java
Please let me know where to place an appopriate test. No existing test
class appeared appropriate as far as I could tell.