cli: fix double-quote escaping for JSON values with format=sql #131881
Conversation
|
This change is |
A previous commit (98f9c8a) made an attempt to fix how JSON values are escaped when they contain invalid UTF8 codes and are displayed in the CLI using the --format=sql flag (see cockroachdb#107518). That commit ended up breaking how JSON values are escaped when they contain double quotes. Luckily it turns out that both problems were actually caused by a long-lived mistake in the `clisqlexec.FormatVal` function. It shouldn't use `fmt.Sprintf("%+q", s)` to escape a string that has invalid characters, as that conflicts with how SQL strings are normally escaped. The proper way is to use `lexbase.EscapeSQLString(s)`. Release note (bug fix): Fixed a bug where the CLI would not correctly escape JSON values that had double-quotes inside of a string when using the --format=sql flag.
rafiss
force-pushed
the
fix-json-format-sql
branch
from
63d8d28
to
fcc3621
Compare
rafiss
added
backport-24.1.x
There was a problem hiding this comment.
Looks good one suggestion
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @rafiss and @rharding6373)
pkg/cli/clisqlexec/format_sql_test.go line 14 at r1 (raw file):
defer c.Cleanup() testData := []string{
Nit: I'm guessing we can't have an automated comaparison in this family of tests?
There was a problem hiding this comment.
Reviewable status:
pkg/cli/clisqlexec/format_sql_test.go line 14 at r1 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
Nit: I'm guessing we can't have an automated comaparison in this family of tests?
what did you mean by automated comparison?
this test does assert that it has the correct output. for example: https://github.com/cockroachdb/cockroach/actions/runs/11174532794/job/31064361460?pr=131881
There was a problem hiding this comment.
Reviewable status:
pkg/cli/clisqlexec/format_sql_test.go line 14 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
what did you mean by automated comparison?
this test does assert that it has the correct output. for example: https://github.com/cockroachdb/cockroach/actions/runs/11174532794/job/31064361460?pr=131881
further reading: https://go.dev/blog/examples
There was a problem hiding this comment.
Reviewable status:
pkg/cli/clisqlexec/format_sql_test.go line 14 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
further reading: https://go.dev/blog/examples
TIL, thank you.
|
tftr! bors r+ |
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches. Issue #131257: branch-release-24.2. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
A previous commit (98f9c8a) made an attempt to fix how JSON values are escaped when they contain invalid UTF8 codes and are displayed in the CLI using the --format=sql flag (see #107518).
That commit ended up breaking how JSON values are escaped when they contain double quotes.
Luckily it turns out that both problems were actually caused by a long-lived mistake in the
clisqlexec.FormatValfunction. It shouldn't usefmt.Sprintf("%+q", s)to escape a string that has invalid characters, as that conflicts with how SQL strings are normally escaped. The proper way is to uselexbase.EscapeSQLString(s).fixes #131257
Release note (bug fix): Fixed a bug where the CLI would not correctly escape JSON values that had double-quotes inside of a string when using the --format=sql flag.