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

sql: don't include quotes in parsed strings #17799

Merged
merged 1 commit into from
Aug 22, 2017
Merged

Conversation

dt
Copy link
Member

@dt dt commented Aug 22, 2017

The planner.TypeAsString* helpers, currently only really used in backup/restore/import, were using
parser.AsStringWithFlags to get the string value of the expr they retured. However even in 'bareStrings'
mode, this still prints non-trivial strings in quotes. If calling code is planning to use the value of
the string, for instance as a URI or parameter value, mangling it with added quotes is almost certainly
not desired. These helpers are used to read params from a query for processing, not print parsable SQL.

The planner.TypeAsString* helpers, currently only really used in backup/restore/import, were using
parser.AsStringWithFlags to get the string value of the expr they retured. However even in 'bareStrings'
mode, this still prints non-trivial strings *in quotes*. If calling code is planning to use the value of
the string, for instance as a URI or parameter value, mangling it with added quotes is almost certainly
not desired. These helpers are used to read params from a query for processing, not print parsable SQL.
@dt dt requested review from danhhz, knz, benesch and a team August 22, 2017 02:04
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@dt
Copy link
Member Author

dt commented Aug 22, 2017

I understand how the backup unit tests didn't catch this with their tidy file path params that didn't trigger quoting, but i'm not sure how we didn't catch this trying to use cloud URIs? but out cloud store URIs were working too since they didn't include comma or spaces so far.

@knz
Copy link
Contributor

knz commented Aug 22, 2017

:lgtm:


Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@dt dt merged commit 6dd3507 into cockroachdb:master Aug 22, 2017
@dt dt deleted the quotes branch August 22, 2017 12:41
@dt
Copy link
Member Author

dt commented Aug 22, 2017

TFTR!

@benesch
Copy link
Contributor

benesch commented Aug 22, 2017

Heh, that call to parser.AsStringWithFlags always struck me as suspicious. Nice catch! If you find yourself in the area again, would be nice to have a comment about why the space is important so it doesn't get refactored away.

@dt
Copy link
Member Author

dt commented Aug 22, 2017

Yeah, a unit test on planner.TypeAsString* would be ideal -- I was just wanting to get a quick fix though so I could try another prod test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants