Handle unicode characters in filters #707
Conversation
- Updated ANTLR4 grammar to support unicode chars - Added test - Removed logic that escapes/unescapes JSON values, should is handled by at the JSON parser (jackson) Fixes: #564
|
View details about the 'Apache Rat - Check' build 151 in the Build Scan 📊 |
|
View details about the 'Maven Verify (with Java 21-zulu)' build 1419 in the Build Scan 📊 |
|
View details about the 'Maven Verify (with Java 17-zulu)' build 1419 in the Build Scan 📊 |
|
@joshuapsteele, took a little longer to track the issue down, but when you get a chance can you take a look at this. @chrisharm if you are still watching can you take a look at this too? I'm guessing this logic was here for a reason and I'm guessing I shouldn't be ripping it out... Do you have ideas for test cases for the original escape logic? |
| // TODO change this to escapeJson() when dependencies get upgraded | ||
| String escaped = StringEscapeUtils.escapeEcmaScript((String) this.compareValue) | ||
| // StringEscapeUtils follows the outdated JSON spec requiring "/" to be escaped, this could subtly break things | ||
| .replaceAll("\\\\/", "/") | ||
| // We don't want single-quotes escaped, this will be unnecessary with escapeJson() | ||
| .replaceAll("\\\\'", "'"); |
There was a problem hiding this comment.
Sorry, just now giving this a quick look, @bdemers. Can we note for posterity, even if just in this PR convo, where escapeJson() is now being used?
| String doubleEscaped = jsonValue.substring(1, jsonValue.length() - 1) | ||
| // StringEscapeUtils follows the outdated JSON spec requiring "/" to be escaped, this could subtly break things | ||
| .replaceAll("\\\\/", "\\\\\\\\/") | ||
| // Just in case someone needs a single-quote with a backslash in front of it, this will be unnecessary with escapeJson() | ||
| .replaceAll("\\\\'", "\\\\\\\\'"); | ||
|
|
||
| // TODO change this to escapeJson() when dependencies get upgraded | ||
| return StringEscapeUtils.unescapeEcmaScript(doubleEscaped); |
There was a problem hiding this comment.
@bdemers Can we note for posterity, even if just in this PR convo, where escapeJson() is now being used?
Fixes: #564