Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

expression: fix Invalid JSONs #10510

Merged
merged 8 commits into from
Jun 6, 2019
Merged

Conversation

b41sh
Copy link
Member

@b41sh b41sh commented May 16, 2019

What problem does this PR solve?

fix Invalid JSONs #10468

What is changed and how it works?

check json is valid before UnmarshalJSON

Check List

Tests

  • Unit test
  • Integration test

@codecov
Copy link

codecov bot commented May 16, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@7bf3d69). Click here to learn what that means.
The diff coverage is 80%.

@@             Coverage Diff             @@
##             master     #10510   +/-   ##
===========================================
  Coverage          ?   79.5716%           
===========================================
  Files             ?        415           
  Lines             ?      88103           
  Branches          ?          0           
===========================================
  Hits              ?      70105           
  Misses            ?      12805           
  Partials          ?       5193

@bb7133
Copy link
Member

bb7133 commented May 20, 2019

Thanks for contributing!

Copy link
Member

@bb7133 bb7133 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bb7133 bb7133 added component/expression status/LGT1 Indicates that a PR has LGTM 1. labels May 20, 2019
Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to make the error message be more similar to MySQL?

@b41sh
Copy link
Member Author

b41sh commented May 20, 2019

Is it possible to make the error message be more similar to MySQL?

ok, Let me try

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 5, 2019
@zz-jason
Copy link
Member

zz-jason commented Jun 5, 2019

/run-all-tests

@bb7133
Copy link
Member

bb7133 commented Jun 5, 2019

Hi @b41sh Unfortunately a corner case is triggered in some TiDB internal tests:

MySQL 5.7:

mysql> select CAST(CAST('1234abcd' AS BINARY) AS JSON);
+------------------------------------------+
| CAST(CAST('1234abcd' AS BINARY) AS JSON) |
+------------------------------------------+
| "base64:type253:MTIzNGFiY2Q="            |
+------------------------------------------+
1 row in set (0.01 sec)

TiDB in this PR:

tidb> select CAST(CAST('1234abcd' AS BINARY) AS JSON);
ERROR 3140 (22032): Invalid JSON text: The document root must not be followed by other values.

TiDB before this PR:

tidb> select CAST(CAST('1234abcd' AS BINARY) AS JSON);
+------------------------------------------+
| CAST(CAST('1234abcd' AS BINARY) AS JSON) |
+------------------------------------------+
| 1234                                     |
+------------------------------------------+
1 row in set (0.00 sec)

As is shown, there is another compatibility issue about JSON(possibly related to #9996) in TiDB. But it was hidden in previous tests because although the result was WRONG, no error was raised.

I think we may: comment related test cases to make this PR merged firstly, or wait until the binary(or say, base64) related issue is solved.

PTAL @zz-jason @XuHuaiyu

@bb7133
Copy link
Member

bb7133 commented Jun 5, 2019

I marked 'DNM' temporarily before we make the decision.

@bb7133
Copy link
Member

bb7133 commented Jun 6, 2019

/run-all-tests

@bb7133 bb7133 removed the status/DNM label Jun 6, 2019
@bb7133 bb7133 merged commit bb01a5f into pingcap:master Jun 6, 2019
@bb7133
Copy link
Member

bb7133 commented Jun 6, 2019

Thanks for contributing! @b41sh

@sre-bot sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression contribution This PR is from a community contributor. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants