🐛 Oracle: fix comment retrieval on table with reserved keyword name

[homersimpsons]

Q	A
Type	bug
Fixed issues	#6764

⚠️ This targets the 3.9.x branch as requested in #6765 (comment)

Summary

Use \Doctrine\DBAL\Schema\OracleSchemaManager::_getPortableTableDefinition in \Doctrine\DBAL\Schema\OracleSchemaManager::fetchTableOptionsByTable to be consistent with \Doctrine\DBAL\Schema\AbstractSchemaManager::fetchAllAssociativeGrouped which calls \Doctrine\DBAL\Schema\OracleSchemaManager::_getPortableTableDefinition

This also fixes an issue in SQLite due to the fact that the name passed to _getPortableTableColumnList was not always normalized.

[morozov]

This is likely going to be a breaking change since all identifiers will be now introspected as quoted. This is correct but may have an unexpected ripple effect on the downstream consumers. This is going to be addressed in 5.0. See #6732 for an example of introspecting foreign key constraints. In the coming few days, I'll make a similar change for table names. But this needs to apply to all object names (indexes, indexed column names, table column names and so on).

UPD: no, this looks safer than I thought. I'll leave a comment inline.

[derrabus]

Please target 3.9.x though.

[morozov]

It probably should be done here:

dbal/src/Schema/AbstractSchemaManager.php

Lines 252 to 257 in 8ce05ed

	return $this->_getPortableTableColumnList(
	$table,
	$database,
	$this->selectTableColumns($database, $this->normalizeName($table))
	->fetchAllAssociative(),
	);

The name passed to selectTableColumns() is "normalized" but the one passed to _getPortableTableColumnList() is not.

[homersimpsons]

Thank you for the guidance, I updated the logic.

Is there any rule about where the name should be normalized and where it should be escaped?

[morozov]

Is there any rule about where the name should be normalized and where it should be escaped?

The quoted name is what we receive from the user to allow them to declare that the original case of the name should be preserved (e.g. the lower-case users table should be introspected even though Oracle by default upper-cases all names). The normalized name is the result of "parsing" the user-supplied input and deriving the actual value from it.

Ideally, we want to normalize the name as soon as possible and use the normalized one downstream.

[homersimpsons]

@morozov This change works for oracle but breaks for SQLite... I'm not sure of what approach should I take, but it looks like the mix between normalized / quoted names is the main issue here (and the various databases may not handle them the same).

[morozov]

If you want, you can scope your pull request only to fixing the table comment, which is necessary only for Oracle (in my understanding).

If you want to fix it for SQLite / column comments as well, you can try referring to the 4.3.x branch. This code has been slightly cleaned up there, and I believe the normalized/quoted names are no longer mixed there. If that's the case, you can try to reconstruct the logic from 4.3.x here.

[morozov]

This change works for oracle

I think options (and all other table objects) should be indexed by the normalized name. The quoted names are just form or user input, while the normalized ones are the actual values. E.g. on Oracle, the inputs 'users' and '"USERS"' will both be normalized to 'USERS'.

[homersimpsons]

I believe the normalized/quoted names are no longer mixed there.

I did not check whether there were mixed or no, but I confirm it works for both Oracle and SQLite there.

[morozov]

After a few attempts to fix this myself on 3.9.x, I gave up (#6891). See more details in #6764 (comment).

[homersimpsons]

After a few attempts to fix this myself on 3.9.x, I gave up (#6891). See more details in #6764 (comment).

Thanks for looking more deeply of this @morozov, I believe we will go directly to v4 instead of going through v3 then. I opened #6897 to still port the tests improvements.

I close this merge request.