Validation Error in doctrine:schema:validate After Merging PR #6418

[berkut1]

Bug Report

Q	A
Version	3.8.5 and 4.0.3

Summary

The PR #6418 breaks doctrine:schema:validate and prevents validation, suggesting replacing all CONSTRAINT definitions, especially those with the FOREIGN KEY.

Removing the problematic line resolves the issue:

dbal/src/Schema/Comparator.php

Lines 374 to 376 in 8edbce7

	if (strtolower($key1->getName()) !== strtolower($key2->getName())) {
	return true;
	}

Current behaviour

doctrine:schema:validate fails validation and suggests replacing all foreign key CONSTRAINT definitions that include the FOREIGN KEY.

How to reproduce

Run doctrine:schema:validate
Observe the validation errors related to foreign key CONSTRAINT definitions with FOREIGN KEY

Example configuration causing the issue:

CREATE TABLE "cp_packages" (
            "id_package"    UUID NOT NULL, 
            "name"          VARCHAR(128) NOT NULL,
            "package_type"  VARCHAR(512) NOT NULL,
            CONSTRAINT "pk_cp_packages" PRIMARY KEY ("id_package")                         
            )

CREATE TABLE "cp_package_virtual_machines" (
            "id_package"    UUID NOT NULL,
            -- some attributes 
            CONSTRAINT "pk_cp_package_virtual_machines" PRIMARY KEY ("id_package"),
            CONSTRAINT "id_package_cp_package_virtual_machines" FOREIGN KEY ("id_package")
                     REFERENCES "cp_packages"("id_package") ON DELETE CASCADE                         
            )

validation schema errors

Mapping
-------                                                                                                                        
 [OK] The mapping files are correct.     

Database
--------
                                                                                                                        
 [ERROR] The database schema is not in sync with the current mapping file.                                              

ALTER TABLE cp_package_virtual_machines DROP CONSTRAINT id_package_cp_package_virtual_machines;
ALTER TABLE cp_package_virtual_machines ADD CONSTRAINT FK_D8247456F5CEE5 FOREIGN KEY (id_package) REFERENCES cp_packages (id_package) ON DELETE CASCADE NOT DEFERRABLE INITIALLY IMMEDIATE;
                                                                                                                        

It also completely ignores the ON DELETE RESTRICT rule, completely removing it. Re-tested that is not true.

Expected behaviour

doctrine:schema:validate should successfully validate the schema without suggesting changes to CONSTRAINT definitions, including those with the FOREIGN KEY

[berkut1]

According to this #6390, I understand that there is a need to somehow track changes in the name of a foreign key (but I really don't know why). However, this breaks the ability to create named foreign keys, which has been in existence for near 10 years, and no alternative has been proposed for creating named foreign keys.
Additionally, it currently override all ON DELETE rules

[greg0ire]

Cc @achterin

[achterin]

@berkut1 thanks for your bug report. i'll have a look at this asap.
to explain why i proposed this change: we are using a symfony application that uses the doctrine bundle to handle the database side of things. while we developing a new feature we discovered that doctrine does not keep track of the auto-generated foreign-key names. if, for whatever reason, the autogenerated name changes, doctrine does not pick up the change and will leave the existing foreign key as is. this is not a huge problem per se, but once you use the functionality to destroy the current schema and recreate it (for example if you execute tests and want a clean database for each test) programatically you will run into a problem because doctrine will build the current schema based on your entity definitions and tries to execute the drop commands which will fail because the generated foreign key name doesnt match the one existing in the database. this then leads to tables being unable to be deleted because of the still existing foreign key.

in my opinion doctrine should always keep track of the foreign key names because it already does so for indexes. but i also agree that having support for named foreign keys is a must.

[berkut1]

@achterin
I don't understand how your tests affect this "bug." If you deploy a migration each time, then after completing the tests, you should simply roll back the migration, where all tables and foreign keys will be removed.
And wouldn't using transactions be suitable in your case? You perform the tests within a transaction and then roll it back after execution. For example https://github.com/dmaicher/doctrine-test-bundle
As far as I know, Doctrine has not tracked foreign key (and primary keys too) names since it was created, and now this change looks like a very significant change in Doctrine's schema validating logic. In my case, it triggers the renaming of over 200 keys and rewriting all ON DELETE rules.

Thanks.

[achterin]

But then i suspect your definitions to be wrong as doctrine is creating an empty table diff if i specifically name the foreign keys in a test:

public function testKeepNamedForeignKey(): void
{
    $tableA = new Table('foo');
    $tableA->addColumn('id', Types::INTEGER);
    $tableA->addForeignKeyConstraint('bar', ['id'], ['id'], [], 'foo_constraint');

    $tableB = new Table('foo');
    $tableB->addColumn('ID', Types::INTEGER);
    $tableB->addForeignKeyConstraint('bar', ['id'], ['id'], [], 'foo_constraint');

    $tableDiff = $this->comparator->compareTables($tableA, $tableB);

    self::assertTrue($tableDiff->isEmpty());
}

as to your comment about my issue: i agree with you that migrating up and down would also solve the issue. but thats not the point. the whole point of doctrine is to keep the database schema in sync with the definition in code. and if a database has a foreign key that has a different name compared to the definition, the comperator should pick this up. otherwise two databases can have two different foreign key names which means they are out of sync and doctrine is happy with both schemas. thats a bug in my book.

[berkut1]

But if this has been acceptable for 10 years and is used by programmers, can it really be considered a bug?
In particular, I actively use named foreign keys, which help identify where the problem occurred by their names, much more effectively than a random set of characters from Doctrine.

[berkut1]

I just read the name of the test that used to be testCompareForeignKeyBasedOnPropertiesNotName meaning this was not considered a bug.

UPD:
In that case, I think it's appropriate to add your bug fix with an option to disable it until the ability to set key names through code is added.

[achterin]

I just read the name of the test that used to be testCompareForeignKeyBasedOnPropertiesNotName meaning this was not considered a bug.

and as i said in the pr, this was also the case for indexes and yet this behavior was changed. so first of all we should keep the behavior as it is now, as it is the right thing to do but also find a way to support named foreign keys. but i guess this is not the task of the dbal layer but the orm because the dbal layer supports named foreign keys just fine. the only thing is that the orm layer doesnt support this to my knowledge and it looks like it is not going to which worries me: doctrine/orm#3753 (comment)

[achterin]

UPD: In that case, I think it's appropriate to add your bug fix with an option to disable it until the ability to set key names through code is added.

as i said and already have proven with the posted test: the dbal layer already supports named foreign keys just fine. the only issue is that the orm layer does not. but i guess we should align this with the project managers as i agree with you that the current implementation will break custom named foreign keys of users that have been told to do just that. and yes, this will be a problem but can be sorted out.

[achterin]

After digging a bit more into this topic I'm even more convinced that the orm preject needs to decide if they want to support named foreign keys (I'm willing to develop a pr for that if necessary) or not - letting users to custom changes to a database leads to issues as this one.
Furthermore, the orm project provides a SchemaTool that lets you drop (dropSchema()) and create (createSchema()) a given schema. As it is pretty hard to get the current schema of the database most users will get the schema from the metadata collected by the entity manager. This will then lead to the problem that it wont remove the named foreign keys because it doesnt know they even exist.

[berkut1]

Hmm, I see it might be important if you dynamically create and modify schemas, for example, for a CMS. However, the conditions for generating a foreign key

dbal/src/Schema/AbstractAsset.php

Lines 148 to 155 in 8edbce7

	protected function _generateIdentifierName(array $columnNames, string $prefix = '', int $maxSize = 30): string
	{
	$hash = implode('', array_map(static function ($column): string {
	return dechex(crc32($column));
	}, $columnNames));
	return strtoupper(substr($prefix . '_' . $hash, 0, $maxSize));
	}

apply when the table name or attribute name changes. But this will work even without your fix because Doctrine will notice that the attributes or table name have changed and will recreate the foreign keys.
In that case, in what situations could the generated key name become outdated after the first migration? If the algorithm for crc32() changes after updating PHP or libraries in the OS? But wouldn't that still lead to the same situation where we get different foreign key names during dropSchema() if we haven't specifically synchronized the schemas before?

[acoulton]

This change has also made 3.8.5 a breaking release across all our projects, which is unfortunate.

We run a database schema validation as part of our CI flow, after applying database migrations, to ensure that the migrated database is in sync with the doctrine schema tagging. These are all now failing because foreign key names don't match.

The mismatches are for one of two reasons:

• It's a legacy project that we migrated to doctrine and there are pre-existing human-readable FK names
• It's an FK that was presumably created under a different version of doctrine that used a different strategy for automatic key naming (e.g. ALTER TABLE widgets DROP FOREIGN KEY FK_9D58E4C1EAD0F67C; ALTER TABLE widgets ADD CONSTRAINT FK_9D58E4C1FF9FC001 FOREIGN KEY (access_key_id) REFERENCES access_keys (id);)

I can see there might be a purist argument for tracking the names as well as the definitions, and I don't object to that in principle (especially since index names are validated).

However, I don't think that can be made mandatory in a point release unless there's a way for users to solve the validation failure by customising the doctrine schema so the expected name matches the existing database.

Certainly for mysql, a Foreign Key can only be renamed by dropping it and recreating it, which is potentially both an expensive and risky migration to run and I'm really not keen to do that across all our tables and projects. Particularly since evidently doctrine's auto-naming strategy has already changed at some point in the past, and could change again in future, and it feels wrong if users are forced to change existing database schemas if & when that happens.

In my opinion, assuming ORM don't have immediate plans to support customising FK names, this behaviour should either be reverted or made opt-in in the next 3.8.x point release.

[greg0ire]

Agreed, please send a revert PR then it can be reintroduced with an opt in mechanism in 3.9

[berkut1]

@greg0ire But these changes also affect the 4.0.x version, or in this case, will everyone affected on 4.0 have to downgrade to version 3.8.x?

[greg0ire]

No, we'll just merge up the revert

[dmaicher]

@greg0ire I reverted it for 3.8.x here

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.