[bug] Fix some edge cases in query sanitization for EFCore and SqlClient

[martincostello]

Component

OpenTelemetry.Instrumentation.SqlClient

Package Version

Package Name	Version
OpenTelemetry.Instrumentation.EntityFrameworkCore	1.14.0-beta.2
OpenTelemetry.Instrumentation.SqlClient	1.14.0-beta.1

Runtime Version

Any

Description

From discussion here dotnet/efcore#29281 (comment):

I can see various issues in a casual look: for example, table and column names may include a single-quote, and that look like it would be identified as a literal to be sanitized in the current code. To compound the problem, different databases differ here: SQL Server allows quoting table/column names via brackets (so [Table'WithSingleQuote]), but MySQL uses backticks, etc. And that's just one problem.

Steps to Reproduce

Use SQL syntax as described above.

Expected Result

Queries are correctly parsed.

Actual Result

Queries may be incorrectly parsed.

Additional Context

No response

Tip

_{React with 👍 to help prioritize this issue. Please use comments to provide useful context, avoiding +1 or me too, to help us triage it. Learn more here.}

[martincostello]

/cc @alanwest @stevejgordon

[alanwest]

Yes there are definitely going to be new cases we'll need to support for other database systems.

Are we at least good with SQL server? My original implementation was ok with SQL server's bracketed identifiers. We should validate that it is still ok.

For MySQL the backticked identifiers is definitely a thing. Postgres also has some different syntax for guids which will need to be handled. Oracle likely has unique needs as well.

[martincostello]

This is probably the bit that needs checking we have coverage for and that it's working:

table and column names may include a single-quote, and that look like it would be identified as a literal to be sanitized in the current code

[stevejgordon]

@martincostello I can add some test cases for that specific example and see whether our code handles them. I'm unlikely to have cycles until W/C 15th though. Feel free to assign me and I'll try to take a proper look once some other priorities are completed.

[stevejgordon]

I've added some test cases today which cover the escaped single quote scenario + other allowed special characters. We do indeed have failures for most of those resulting in incomplete sanitised query text and/or summary. The good news is that we don't hang in such cases. I'm still hoping to carve out time next week to work on improving the handling of these scenarios.

[alanwest]

Added a simple test illustrating status quo for MSSQL bracketed identifiers #3616. Also shared my thoughts on these edge cases.

@stevejgordon if my PR is redundant based on the work you're doing, we can just close it. I just wanted to share my thoughts.

[stevejgordon]

@alanwest, thanks. I have about 10 tests for various escaped identifiers, so I think we can close yours. I'm trying ot carve some time this week to implement a solution where we allow the special characters in identifiers, when we're in escaped (square brackets). I think we'll need to track a bit more on the state to know when we're in escaped mode and then extend our IsLetter check with the range of possible characters. I hope it's not too complex a change.