Skip to content

Conversation

adriendupuis
Copy link
Contributor

@adriendupuis adriendupuis commented Feb 20, 2025

🎫 Issue IBX-9617

Description:

Before:

% ddev php bin/console ibexa:install
Creating database db if it does not exist, using doctrine:database:create --if-not-exists
Database `db` for connection named default already exists. Skipped.


 Running this command will delete data in all Ibexa generated tables. Continue? (yes/no) [yes]:
 > 

Executing 374 queries on database db (mysql)
   0/374 [>---------------------------]   0%
In AbstractMySQLDriver.php line 68:
                                                                                                                              
  An exception occurred while executing 'DROP TABLE ibexa_order_item_product':                                                
                                                                                                                              
  SQLSTATE[23000]: Integrity constraint violation: 1451 Cannot delete or update a parent row: a foreign key constraint fails  
                                                                                                                              

Issues:

  • If developer risk to lose all data, default confirmation answer should be 'no', not 'yes'.
  • Tables are not dropped in an order respecting foreign keys dependencies.

Fixes:

  • Drop all the foreign keys before dropping the tables.
  • Set 'no' as the default answer to the "Continue?" question.

After:

% ddev php bin/console ibexa:install
Creating database db if it does not exist, using doctrine:database:create --if-not-exists
Database `db` for connection named default already exists. Skipped.


 Running this command will delete data in all Ibexa generated tables. Continue? (yes/no) [no]:
 > 

% ddev php bin/console ibexa:install
Creating database db if it does not exist, using doctrine:database:create --if-not-exists
Database `db` for connection named default already exists. Skipped.


 Running this command will delete data in all Ibexa generated tables. Continue? (yes/no) [no]:
 > y

Executing 375 queries on database db (mysql)
 375/375 [============================] 100%

Executing 30 queries from /var/www/html/vendor/ibexa/core/data/mysql/cleandata.sql on database db
Executing 1 queries from /var/www/html/vendor/ibexa/installer/src/bundle/Resources/install/sql/mysql/user_settings.sql on database db
Executing 18 queries from /var/www/html/vendor/ibexa/installer/src/bundle/Resources/install/sql/mysql/content_data.sql on database db
Executing 3 queries from /var/www/html/vendor/ibexa/installer/src/bundle/Resources/install/sql/mysql/landing_pages.sql on database db
Executing 1 queries from /var/www/html/vendor/ibexa/installer/src/bundle/Resources/install/sql/mysql/permissions.sql on database db

                                                                                                                        
 [WARNING] Not passing the "--complete" option to "doctrine:schema:update" is deprecated and will not be supported when 
           using doctrine/dbal 4                                                                                        
                                                                                                                        

                                                                                                                        
 [OK] Nothing to update - your database is already in sync with the current entity metadata.                            
                                                                                                                        


In MigrationExecutor.php line 68:
                                                                                                                                                                                                          
  Executing migration "002_taxonomy_content.yml" failed. Exception: An exception occurred while executing 'INSERT INTO ibexa_taxonomy_entries (identifier, name, names, mainLanguageCode, content_id, `l  
  eft`, `right`, root, lvl, taxonomy, parent_id) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)' with params ["root", "Root", "a:1:{s:6:\"eng-GB\";s:4:\"Root\";}", "eng-GB", 58, 0, 0, 0, 0, "tags", null]:    
                                                                                                                                                                                                          
  SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'tags-root' for key 'ibexa_taxonomy_entries_identifier_idx'.                                                                                                                                                                                                                                                                                

Remaining issue I'm not able to fix by myself:

  • 3 tables aren't dropped (Commerce 4.6.x-dev with ibexa/core 1efa3ef): ibexa_payment_token, ibexa_taxonomy_assignments, and ibexa_taxonomy_entries.
  • As those tables weren't dropped, the error SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'tags-root' for key 'ibexa_taxonomy_entries_identifier_idx' is thrown, and probably other errors of this kind are waiting behind it.

For QA:

Documentation:

@adriendupuis adriendupuis marked this pull request as ready for review February 20, 2025 16:18
Copy link

@adriendupuis adriendupuis changed the title Enhance reinstall DB schema Fix DB schema reinstallation Feb 20, 2025
@adamwojs adamwojs requested a review from a team February 23, 2025 14:00
Comment on lines +102 to +112
$tables = $newSchema->getTables();
if ($databasePlatform->supportsForeignKeyConstraints()) {
// cleanup pre-existing database: drop foreign keys
foreach ($tables as $table) {
if ($existingSchema->hasTable($table->getName())) {
foreach ($this->db->getSchemaManager()->listTableForeignKeys($table->getName()) as $foreignKeyConstraint) {
$statements[] = $databasePlatform->getDropForeignKeySQL($foreignKeyConstraint->getName(), $table->getName());
}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels to me like a workaround. Tables should be dropped in a correct order. Have you tried to see if we can reorganize tables order in Yaml for doctrine schema instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alongosz I didn't. I replaced the not-working array_reverse trick with another trick.
Studying how the table list is loaded/built from YAML schema is probably also necessary to understand why 3 tables are missing. But my knowledge of Doctrine is limited and I don't manage to explore this schema declaration.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels to me like a workaround. Tables should be dropped in a correct order. Have you tried to see if we can reorganize tables order in Yaml for doctrine schema instead?

I didn't. I replaced the not-working array_reverse trick with another trick. Studying how the table list is loaded/built from YAML schema is probably also necessary to understand why 3 tables are missing. But my knowledge of Doctrine is limited and I don't manage to explore this schema declaration.

Ok. Looks like that this could go in. I'm a bit worried about performance of listing foreign keys for every table, but on the other hand it's just for re-installation. Maybe QA can focus on this when testing?

Given the method became more complicated, I'd need to see here 2 things:

  1. Unit test coverage for CoreInstaller. I can help with that if it's too complicated.
  2. Refactoring. Correct me if I'm wrong, but instead of processing the tables list 2 times, we can in one single loop for each table provide both drop foreign key statements and drop table statement? Or do we really need to drop all foreign keys first and then drop tables?

Something I cooked up on the fly would look like this:

    /**
     * @param \Doctrine\DBAL\Schema\Schema $newSchema
     * @param \Doctrine\DBAL\Platforms\AbstractPlatform $databasePlatform
     *
     * @return string[]
     */
    protected function getDropSqlStatementsForExistingSchema(
        Schema $newSchema,
        AbstractPlatform $databasePlatform
    ): array {
        $schemaManager = $this->db->getSchemaManager();
        if ($schemaManager === null) {
            throw new \LogicException('CoreInstaller: Unable to get Schema manager');
        }

        $existingSchema = $schemaManager->createSchema();
        $statements = [];
        $tables = $newSchema->getTables();
        $supportsForeignKeyConstraints = $databasePlatform->supportsForeignKeyConstraints();

        foreach ($tables as $table) {
            if (!$existingSchema->hasTable($table->getName())) {
                continue;
            }
            if ($supportsForeignKeyConstraints) {
                // cleanup pre-existing database: drop foreign keys
                foreach ($schemaManager->listTableForeignKeys($table->getName()) as $foreignKeyConstraint) {
                    $statements[] = $databasePlatform->getDropForeignKeySQL($foreignKeyConstraint->getName(), $table->getName());
                }
            }

            // cleanup pre-existing database: drop tables
            $statements[] = $databasePlatform->getDropTableSQL($table);
        }

        return $statements;
    }

Haven't tested it however.

@alongosz alongosz changed the title Fix DB schema reinstallation IBX-9617: Fixed DB schema reinstallation Feb 23, 2025
Answering "yes" to "Continue?" just throw errors and fail.

  SQLSTATE[23000]: Integrity constraint violation: 1451 Cannot delete or update a parent row: a foreign key constraint fails
If written only for FKs, it didn't work.
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants