sql: anonymize schema names, owner names, and enum values #55473
Conversation
|
This change is |
|
On further inspection I think ENUMs are not properly anonymized either. I'll circle back to fix that. |
|
I see that #55398 deals with owner names by using the newly introduced |
There was a problem hiding this comment.
For enum values unfortunately we can't use tree.Name, because it doesn't quote properly upon pretty-printing.
However we can define a custom AST node type and add a Format() method that does the anonymization for them.
Reviewed 20 of 20 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @lucy-zhang)
solongordon
force-pushed
the
string-to-name
branch
from
fc8b37a to
5d9fcac
Compare
solongordon
changed the title
|
@knz I added a second commit to handle enum values. Would you please take another look? @lucy-zhang I wasn't sure whether #55398 would be backported so I wanted to put in a simple fix for owner names. When that is merged it can replace my owner changes here. |
There was a problem hiding this comment.
Changes LGTM. Should this have formatting tests?
Reviewed 19 of 20 files at r1, 1 of 1 files at r2, 8 of 8 files at r3.
Reviewable status:
|
Simple formatting has pre-existing tests in parse_test.go, but let me augment with a few tests for anonymized formatting. |
solongordon
force-pushed
the
string-to-name
branch
from
5d9fcac to
4a1a6f7
Compare
|
Added anonymization tests. |
There was a problem hiding this comment.
Reviewed 9 of 9 files at r4, 9 of 9 files at r5.
Reviewable status:
Schema and owner names were improperly being represented by strings rather than `tree.Name`s in the AST. This meant they would not be properly anonymized in telemetry. Fixes cockroachdb#55385 Release note: None
solongordon
force-pushed
the
string-to-name
branch
from
4a1a6f7 to
399ec74
Compare
ENUM values were improperly being represented as strings in the AST, which meant that they were not anonymized in telemetry. I created a new AST node type for them which respects anonymization. Fixes cockroachdb#55385 Release note: None
solongordon
force-pushed
the
string-to-name
branch
from
399ec74 to
bad7df0
Compare
|
bors r+ |
|
Build succeeded: |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, 9 of 9 files at r7.
Reviewable status:
Schema and owner names were improperly being represented by strings
rather than
tree.Names in the AST. This meant they would not beproperly anonymized in telemetry.
ENUM values had the same problem, but we cannot use
tree.Nameinthis case because we need the values to be quoted. I created a custom AST
node for this case.
Fixes #55385
Release note: None