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 sanitizer: handle double quoted table names #5699

Merged
merged 3 commits into from
Mar 29, 2022

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Mar 28, 2022

Resolves #5691

@laurit laurit requested a review from a team March 28, 2022 08:36
Comment on lines 378 to 380
if (operation.mainTable != null && operation.mainTable.startsWith("\"") && operation.mainTable.endsWith("\"")) {
operation.mainTable = operation.mainTable.substring(1, operation.mainTable.length() - 1);
}
Copy link
Member

Choose a reason for hiding this comment

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

Nit: WDYT about moving this if into the handleIdentifier() method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a method that reads text matched by current token and strips double quotes and backticks.

@@ -105,6 +105,8 @@ class SqlStatementSanitizerTest extends Specification {
sql | expected
// Select
'SELECT x, y, z FROM schema.table' | SqlStatementInfo.create(sql, 'SELECT', 'schema.table')
'SELECT x, y, z FROM `schema.table`' | SqlStatementInfo.create(sql, 'SELECT', 'schema.table')
Copy link
Member

Choose a reason for hiding this comment

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

Wait, did we have backtick ` parsing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this already worked just didn't have a test.

Copy link
Member

Choose a reason for hiding this comment

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

Nice 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it only works fine when you don't have a space inside backticks. If there is a space it returns the first word as the table name.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, we don't have any special rules for backticks, I guess they're just treated as "other characters" - that maybe that behavior is caused by this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, hopefully fixed now.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

nice 👍

@laurit laurit merged commit b85696b into open-telemetry:main Mar 29, 2022
@laurit laurit deleted the sql-sanitizer-double-quote-table branch March 29, 2022 07:48
RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this pull request May 23, 2022
* Sql sanitizer: handle double quoted table names

* handle backtick

* strip double quotes and backtick from table name in a separate method
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.

SQL sanitizer produces inaccurate table names with some queries
3 participants