Rework handling quoted object names and reserved SQL keywords #6589
Conversation
morozov
force-pushed
the
name-parser
branch
4 times, most recently
from
5a3fa90 to
5723efa
Compare
morozov
force-pushed
the
name-parser
branch
2 times, most recently
from
b108afa to
7784118
Compare
morozov
force-pushed
the
name-parser
branch
2 times, most recently
from
b7f3c91 to
0ae3d7a
Compare
The SQL being used by a component is an implementation detail. It should be tested in unit tests.
| @@ -671,8 +671,7 @@ protected function _getPortableTableColumnList(string $table, string $database, | |||
| foreach ($tableColumns as $tableColumn) { | |||
| $column = $this->_getPortableTableColumnDefinition($tableColumn); | |||
|
|
|||
| $name = strtolower($column->getQuotedName($this->platform)); | |||
| $list[$name] = $column; | |||
| $list[strtolower($column->getName())] = $column; | |||
There was a problem hiding this comment.
Technically, it may be a breaking change but practically nobody will care.
The only reason why this might be useful is for Oracle to avoid conflicts between fully a upper-cased column name and non-fully upper-cased (e.g. ID and id). In this case, the latter will be enclosed in quotes, so even after lower-casing they will be different (id and "id") respectively.
However, this is a half-measure because:
- The behavior of quoting of introspected names is implemented only for Oracle but not for IBM DB2 or PostgreSQL which are also SQL-92 compliant and are prone to the same problem.
- Even on Oracle, it won't work for columns like
IdandiD– they both will be converted to"id"and collide.
| 'CREATE INDEX main."i" ON mytable ("a", "b")', | ||
| $this->platform->getCreateIndexSQL($indexDef, 'main.mytable'), |
There was a problem hiding this comment.
The index name is quoted because it comes from Index, and we know that it's an unqualified name (i.e., the literal value). The table name comes from 'main.table', which is SQL. It's unquoted in the method argument, so it's unquoted in the resulting SQL as well. This isn't entirely correct but is out of scope.
| @@ -95,31 +94,4 @@ public static function columnNameProvider(): iterable | |||
| yield ['C1', 'c1_x']; | |||
| yield ['importantColumn', 'veryImportantColumn']; | |||
| } | |||
|
|
|||
| /** @throws Exception */ | |||
| public function testRenameColumToQuoted(): void | |||
There was a problem hiding this comment.
This test was introduced in #6602 and is no longer necessary. All new names are quoted by default, so this case is covered by existing tests.
| } | ||
|
|
||
| protected function getGenerateForeignKeySql(): string | ||
| { | ||
| return 'ALTER TABLE test ADD FOREIGN KEY (fk_name_id) REFERENCES other_table (id)'; | ||
| return 'ALTER TABLE test ADD FOREIGN KEY (`fk_name_id`) REFERENCES `other_table` (`id`)'; |
There was a problem hiding this comment.
The table name here is still unquoted because AbstractPlatform::getCreateForeignKeySQL() accepts the table name as SQL, not as a literal. Consistent quoting can be achieved by introducing values objects to represent object names (#4772).
| // Foreign column with reserved keyword as name | ||
| $foreignTable->addColumn('create', Types::STRING); |
There was a problem hiding this comment.
Technically, this part of the test is now redundant since all identifiers are rendered in SQL consistently regardless of whether they are keywords or not. But redesigning the test to correspond to the new design is a bit too much work for now.
Technical details
With this change, DBAL will consistently quote all identifiers, regardless of whether they are keywords or not. It will also normalize the case of unquoted identifiers to match the behavior of the target database platform:
To preserve the lower case of identifiers on Oracle and IBM DB2 or the upper case on PostgreSQL, the identifiers
should be quoted explicitly.
This change will address some of the issues documented in #4357.
End user impact
The impact on the users will depend on the set of target platforms they support in their applications