types/json: fix an over-quoted bug in BinaryJSON.Unquote function

[bb7133]

What problem does this PR solve?

This PR tries to fix the following issue:

create table a(a json);
insert into a values ("{\"low_keywords\":\"[{\\\"x\\\":\\\"{\\\\\\\"y\\\\\\\":12}\\\"}]\"}");

In current TiDB(master/3.0/2.1):

tidb> select a->>"$.low_keywords" from a;
+----------------------+
| a->>"$.low_keywords" |
+----------------------+
| [{"x":"{"y":12}"}] |
+----------------------+
1 rows in set (0.02 sec)

In MySQL 5.7/8.0:

tidb> select a->>"$.low_keywords" from a;
+----------------------+
| a->>"$.low_keywords" |
+----------------------+
| [{"x":"{\"y\":12}"}] |
+----------------------+
1 rows in set (0.02 sec)

What is changed and how it works?

The issue is caused by the over-unquote behaviors of BinaryJSON.Unquote: for a string literal of JSON, it should be unquoted only when enclosed by double quotes(")

Check List

Tests

• Unit test
• Integration test
• Manual test (add detailed scripts or steps below)

tidb> select json_unquote("\"[{\\\"x\\\":\\\"{\\\\\\\"y\\\\\\\":12}\\\"}]\"");
+------------------------------------------------------------------+
| json_unquote("\"[{\\\"x\\\":\\\"{\\\\\\\"y\\\\\\\":12}\\\"}]\"") |
+------------------------------------------------------------------+
| [{"x":"{\"y\":12}"}]                                             |
+------------------------------------------------------------------+
1 row in set (0.00 sec)

tidb> select json_unquote("\\\\");
+----------------------+
| json_unquote("\\\\") |
+----------------------+
| \\                   |
+----------------------+
1 row in set (0.00 sec)

• No code

Code changes

• Has exported function/method change

Side effects

N/A

Related changes

• Need to cherry-pick to the release branch

Release note

Fix an over-quoted bug for JSON_UNQUOTE function: only values enclosed by the double quote marks(") should be unquoted. For example, result of SELECT JSON_UNQUOTE("\\\\") should be \\(not changed).

[codecov]

Codecov Report

Merging #11934 into master will decrease coverage by 0.0055%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##             master     #11934        +/-   ##
================================================
- Coverage   81.2155%   81.2099%   -0.0056%     
================================================
  Files           443        443                
  Lines         95004      95013         +9     
================================================
+ Hits          77158      77160         +2     
- Misses        12351      12356         +5     
- Partials       5495       5497         +2

[bb7133]

/run-all-tests

[winkyao]

LGTM

[lonng]

I think we can remove the Trace.

[bb7133]

The error is returned by unquoteString, so I think it is better to use Trace? @lonng

[lonng]

@bb7133 All errors returned by unquoteString have been wrapt withTrace. (anyway, not a strong opinion)

[bb7133]

I see, thanks.

[bb7133]

Addressed

[lonng]

Rest LGTM

[bb7133]

/run-all-tests

[sre-bot]

/run-all-tests

[lonng]

@bb7133 Do we need to cherry-pick to release branches?

[sre-bot]

cherry pick to release-2.1 failed

[sre-bot]

cherry pick to release-3.0 in PR #11955

[cyliu0]

@bb7133 Do we need to cherry pick this one to release-3.1 ?

[sre-bot]

It seems that, not for sure, we failed to cherry-pick this commit to release-2.1. Please comment '/run-cherry-picker' to try to trigger the cherry-picker if we did fail to cherry-pick this commit before. @bb7133 PTAL.

[bb7133]

/run-cherry-picker

[sre-bot]

cherry pick to release-2.1 failed

[you06]

Already cherry picked in #12096.