Fixes invalid T-SQL column comment for quoted table names.#4284
Fixes invalid T-SQL column comment for quoted table names.#4284metaturso wants to merge 1 commit intodoctrine:2.12.xfrom metaturso:fix/tsql-comment-column
Conversation
|
I think you should target 2.11.x |
|
I rebased my branch onto I'm happy to open a fresh MR targeting the desired branch if you prefer. There's a few places left in How would you feel about a method which initialises both variables at once, e.g. |
Please don't, it's great the way it is 👍 I will close and reopen, it might help with the checks. |
| $createTableSQL = $this->getCreateTableSQL('table_name'); | ||
|
|
||
| self::assertEquals( | ||
| "EXEC sp_addextendedproperty N'MS_Description', N'(DC2Type:array)', " | ||
| . "N'SCHEMA', 'dbo', N'TABLE', 'table_name', N'COLUMN', column_name", | ||
| $createTableSQL | ||
| ); |
There was a problem hiding this comment.
| $createTableSQL = $this->getCreateTableSQL('table_name'); | |
| self::assertEquals( | |
| "EXEC sp_addextendedproperty N'MS_Description', N'(DC2Type:array)', " | |
| . "N'SCHEMA', 'dbo', N'TABLE', 'table_name', N'COLUMN', column_name", | |
| $createTableSQL | |
| ); | |
| self::assertEquals( | |
| "EXEC sp_addextendedproperty N'MS_Description', N'(DC2Type:array)', " | |
| . "N'SCHEMA', 'dbo', N'TABLE', 'table_name', N'COLUMN', column_name", | |
| $this->getCreateTableSQL('table_name') | |
| ); |
| $createTableSQL = $this->getCreateTableSQL('`quoted-table-name`'); | ||
|
|
||
| self::assertEquals( | ||
| "EXEC sp_addextendedproperty N'MS_Description', N'(DC2Type:array)', " | ||
| . "N'SCHEMA', 'dbo', N'TABLE', [quoted-table-name], N'COLUMN', column_name", | ||
| $createTableSQL | ||
| ); | ||
| } |
There was a problem hiding this comment.
| $createTableSQL = $this->getCreateTableSQL('`quoted-table-name`'); | |
| self::assertEquals( | |
| "EXEC sp_addextendedproperty N'MS_Description', N'(DC2Type:array)', " | |
| . "N'SCHEMA', 'dbo', N'TABLE', [quoted-table-name], N'COLUMN', column_name", | |
| $createTableSQL | |
| ); | |
| } | |
| self::assertEquals( | |
| "EXEC sp_addextendedproperty N'MS_Description', N'(DC2Type:array)', " | |
| . "N'SCHEMA', 'dbo', N'TABLE', [quoted-table-name], N'COLUMN', column_name", | |
| $this->getCreateTableSQL('`quoted-table-name`') | |
| ); | |
| } |
| $createTableSQL = $this->getCreateTableSQL('`custom.quoted-table-name`'); | ||
|
|
||
| self::assertEquals( | ||
| "EXEC sp_addextendedproperty N'MS_Description', N'(DC2Type:array)', " | ||
| . "N'SCHEMA', [custom], N'TABLE', [quoted-table-name], N'COLUMN', column_name", | ||
| $createTableSQL | ||
| ); |
There was a problem hiding this comment.
| $createTableSQL = $this->getCreateTableSQL('`custom.quoted-table-name`'); | |
| self::assertEquals( | |
| "EXEC sp_addextendedproperty N'MS_Description', N'(DC2Type:array)', " | |
| . "N'SCHEMA', [custom], N'TABLE', [quoted-table-name], N'COLUMN', column_name", | |
| $createTableSQL | |
| ); | |
| self::assertEquals( | |
| "EXEC sp_addextendedproperty N'MS_Description', N'(DC2Type:array)', " | |
| . "N'SCHEMA', [custom], N'TABLE', [quoted-table-name], N'COLUMN', column_name", | |
| $this->getCreateTableSQL('`custom.quoted-table-name`') | |
| ); |
| . "N'SCHEMA', [custom], N'TABLE', [quoted-table-name], N'COLUMN', column_name", | ||
| $createTableSQL | ||
| ); | ||
| } |
There was a problem hiding this comment.
Would it make sense to use a data provider instead? All 3 tests have the same logic, it would be great to be able to see just what changes in one place.
|
|
||
| public function getCreateTableSQL(string $quotedName): string | ||
| { | ||
| $driver = $this->createMock(Driver::class); |
There was a problem hiding this comment.
Since you perform no expectations, this is a stub:
| $driver = $this->createMock(Driver::class); | |
| $driver = $this->createStub(Driver::class); |
|
This change requires a functional test that runs the updated SQL queries by a real database, not making assertions against the generated SQL and using mocks. |
|
Couldn't agree more! The proposed unit test is good enough to show the problem exists not prove that it's fixed. In that sense, it helped expose the fact that the ALTER and DROP column/table cases are also bugged. I won't get another chance to work on this probably until next weekend, when I plan to begin writing the new integration test against a real SQL Server instance before attempting a fix. I'll try to push the new version of the unit failing unit test sooner than that to arouse discussion about a clean fix. In the meantime, could you let me know your requirements or preferences for this new test? For example, could you point me to an existing test that I can or should use as reference for level of abstraction, structure, assertion style, etc., just to get an idea? On a related note, is the current Travis CI pipeline already configured to run SQL Server tests? Thanks in advance for your time. |
As far as I understand the problem, you'll need to create a table with a name that has to be quoted. Look for the tests that call Please implement it in a platform-agnostic way (e.g. see
|
| $tableSQL = $this->quoteStringLiteral($tableName); | ||
| } | ||
|
|
||
| if (strpos($tableName, '[') === false) { |
There was a problem hiding this comment.
I think we should preg_replace('~^\[|\]$~s', '', $tableName) instead and quote as literal as we would normally do.
String literal seeems to be the official way to escape: https://docs.microsoft.com/en-us/sql/relational-databases/system-stored-procedures/sp-addextendedproperty-transact-sql?view=sql-server-ver15#b-adding-an-extended-property-to-a-column-in-a-table
There was a problem hiding this comment.
@metaturso please see #4360 , I think we need to fix in a little more abstract way - please provide feedback, if you want to change anything or help with the tests, please send a PR directly to my branch. Michael
@mvorisek thanks for stepping in with a patch for this, chap. Your fix in PR #4360 is reasonably self-contained and I'm happy to ditch my changes in favour of yours.
There's still something about the amount of (un)escaping that's done at multiple levels that doesn't completely sit right with me, but fixing the data-flow is a much more pervasive change that's likely to break B/C or third-party code so I can't offer a better alternative.
I didn't get a chance to sit down and write the promised integration test. I'll commit and push the fixed version of the unit test, which runs against 2.11.x version of the codebase, during my lunch break. Feel free to steal this from my PR if that's of any use to you.
Should you decide not to use my test, please don't forget to cover for #4283 by adding test cases or fixture using i) table names with a custom schema name and ii) containing special characters or reserved keywords to your pull request.
|
@metaturso please see #4360 , I think we need to fix in a little more abstract way - please provide feedback, if you want to change anything or help with the tests, please send a PR directly to my branch. Michael |
…p|update}extendedproperty.
|
I've applied all suggestions to the unit test and updated it to cover the add, update and drop Extended Property cases, @morozov and @greg0ire thanks for your feedback and suggestions. All changes to the SQL Server platform class have been removed from this branch because @mvorisek has already raised a comprehensive fix to the original issue (thanks for that.) The failing unit test has been left here for his convenience. I reckon this PR can now be closed in favour of #4360. |
|
@metaturso please submit the tests as PR into https://github.com/mvorisek/dbal/tree/fix_mssql_comment_with_escaped_column branch, so your work can be integrated into #4360 |
|
Will do shortly. Close this PR at your discretion. |
Summary
This merge request fixes the issue whereby a quoted table or schema name was getting converted into a string literal, resulting in a T-SQL statement referencing an invalid table name like
N'[table_name]'.The issues happened whenever the entity table name contained Quoted Reserved Words, for example:
The issue is fixed within the
SQLServer2012Platformbase class by inserting the schema or table names as T-SQL identifiers when generating the call tosp_addextendedpropertyfor quoted names.In other words, because Doctrine converts a quoted name like
`table-name`into a valid T-SQL identifier like[table-name]we needn't wrap the identifier into a string literal before passing it togetAddExtendedPropertySQL.