Fix identifier escaping when used as string literal for T-SQL

[mvorisek]

Q	A
Type	bug
BC Break	no
Fixed issues	#4283

Summary

Alternative to closes #4284 to fix it correctly and with proper functional tests.

The issue was that MSSQL expects string literal (quoted by ') for EXEC sp_addextendedproperty instead of identifier name (quoted by []).

[greg0ire]

TL;DR of the issue, the blog post and your diff please?
Also, there are no tests, why is that?

[mvorisek]

@greg0ire https://docs.microsoft.com/en-us/sql/relational-databases/system-stored-procedures/sp-dropextendedproperty-transact-sql?view=sql-server-ver15 shows the sp_addextendedproperty accepts sysname datatype which is nothing else than a string datatype - therefore the link

everything is covered by static tests - you want one functional test for getCreateColumnCommentSQL?

[mvorisek]

@metaturso can you please submit your review on this PR?

[mvorisek]

Yes:

You will have to find another maintainer.

What else can I do to get this PR merged? Btw. it is a bug fix incl. full functional tests prooving the fix.

ping @greg0ire @morozov @beberlei

[morozov]

2 assumes that the schema name cannot contain dots but the table can. Why?

[mvorisek]

@morozov the feedback has been addressed, it there anything left?

[morozov]

In my opinion, this whole magic about smashing the schema name and the table together and then exploding/parsing with regular expressions is absolutely unreliable.

What if the schema name is "example.com" and the table name is "inventory"?

A proper API must operate with object (schema, table) names and quote, and concatenate them while building SQL. The API works that worse the other way around may pass the tests that cover the happy but won't work in real cases.

[mvorisek]

@morozov Please tell me where introduced newly, this PR fixes a bug related with wrong column names escaping for EXEC sp_addextendedproperty prodedure. Exploding name by . is the original behaviour.

[morozov]

I see that this PR relies on the reworked implementation using explode() which was wrong and is still wrong. I can't tell if this is relevant to the fix because this PR:

1. Did contain the irrelevant previously (argument type changes in getAddExtendedPropertySQL()).
2. Does contain the corresponding type casting which is no longer necessary.

It's challenging to review such changes since it takes extra effort to get to their essence.

Apart from that, I find the way you behave in the community hard to deal with:

1. You propose changes that have questionable value and/or take unnecessarily much time and energy to discuss (ref).
2. Given the contributions above, I don't believe you're credible to work on such complex code changes.
3. You create an unnecessary urgency to review your changes (Generate alter add/change column with explicit position for MySQL #4276 (comment)) and tag people that has no relation to the work you're doing (Fix identifier escaping when used as string literal for T-SQL #4360 (comment)).

With that said, I'm not personally interested in reviewing this. You can continue working with other DBAL maintainers on getting your changes approved.

[mvorisek]

@morozov I have added functional tests covering reserved keywords in column names for all database platforms (first commit).

I belive I fixed the issue cleanly and in a minimalistic way. If you think the issue can be fixed better (and still fully passing added tests), please advise.

To my crediatability. I understand you may "rate me" by several closed PRs into this repo, but I am proud to say this is pure coincidence. I am strong contributor to another backend repos and database abstraction libs like atk4/dsql, atk4/data in which I actually spotted the issue.

[morozov]

The DBAL currently has very limited support for quoting identifiers (#4357). Most importantly, there's no distinction at the API level between object names (e.g. foo-bar) and the SQL representing them: [foo-bar], "foo-bar", `foo-bar`etc. Even when an identifier is passed as `foo-bar`, it's not clear whether it's foo-bar represented as SQL or the backticks are part of the name (which is also valid).

I don't see much point in complicating the implementation of the API that was not supposed to be used for this purpose in the first place.

[mvorisek]

@morozov Your reaction seems brutal to me. With a lot of efford (several iterations of feedback, 10+ hours spent) I was able to fix the issue with very good functional testing FOR EVERY PLATFORM.

I don't see much point in complicating the implementation of the API

What is wrong with fixed ~80 lines (+ added tests, + normalized quotes in existing tests)?

Here is example of the current issue: https://github.com/atk4/data/runs/2261267667#step:12:24

[beberlei]

These kind of changes to quoting fundamentals are very high risk for Doctrine. In conjunction with our limited understanding of SQLServer platform, this PR is something we all hesitate to merge, defensive maintenance is critical for a database library.

Sorry for getting to the conclusion so late, i certainly have been there myself after having to revert a patch on sqlite foreign keys that took weeks to finish last year for example.

As a point in feedback, this kind of deep change to a driver needs careful research and your PR description leaves a lot open wirh respect to quoting original documentation, resources explaining the issue with examples and your own evaluation of them w.r.t Doctrine. We have asked for this several times and we habe also invested hours reviewing this change but still far away from 100% understanding.

[mvorisek]

I am still convinced this is a bug and it should be fixed.

Guys, @greg0ire , @morozov, @beberlei , let me discuss the possible impacts:

Pros:

• issue Doctrine fails to declare Extended Property on SQL-Server array column (when table name is quoted)  #4283 is fixed (ie. names with unlisted keywords, already quoted names)
• all existing usage continues to work (no single changed test usage)
• new test cases work with all platforms and full table/column lifecycle

To consider:

• expected sql queries are changed to a different quoting (see https://github.com/doctrine/dbal/pull/4360/files#diff-037ab13274497cf54e95b2386c69077b5a7838858d83085cb349a0b8a5b5e6b0R797 , all arguments except the last one are already using the proposed escaping ')
• getCreateColumnCommentSQL (and getAlter... variants) methods are now stricter:
  • explode now does not ignore name with more dot symbols (I belive error is better than silent ignore, if not, this change is definitely not required, I can remove the 2 explode limit)
  • getAddExtendedPropertySQL previously accepted splitted but already SQL escaped name parts (but comment was/is accepted unescaped, now normalized to accept all parts unescaped and escaping is done inside - always, thus stronger)
  • no other behaviour changed

Cons:

• except discussed cases above I do not see any real BC break

Can we agree on these points and is there anything else to discuss/address?