IBX-9617: Fixed DB schema reinstallation - #485
Conversation
|
adriendupuis
changed the title
| $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()); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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_reversetrick 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:
- Unit test coverage for CoreInstaller. I can help with that if it's too complicated.
- 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.
There was a problem hiding this comment.
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?
You still risk to try dropping a table depending on a foreign key not dropped yet.
But, we could in your unique loop build $dropForeignKeyStatements and $dropTableStatements then return array_merge($dropForeignKeyStatements, $dropTableStatements);
alongosz
changed the title
f1038e9 to
da2fe07
Compare
|
|
Hey @adriendupuis. Is this ready for another round of review? |
0984562 to
332c126
Compare
|
1b0b1d2 to
1edac6e
Compare
|
alongosz
left a comment
alongosz
left a comment
There was a problem hiding this comment.
Core installer changes are ok, but looks like that code path with existing schema having a table was never covered by neither unit nor integration test case. Let's try adding another test case to \Ibexa\Tests\Integration\Installer\RepositoryInstaller\CoreInstallerTest:
diff --git a/tests/integration/RepositoryInstaller/Installer/CoreInstallerTest.php b/tests/integration/RepositoryInstaller/Installer/CoreInstallerTest.php
index 28f8750700..8b5a48f151 100644
--- a/tests/integration/RepositoryInstaller/Installer/CoreInstallerTest.php
+++ b/tests/integration/RepositoryInstaller/Installer/CoreInstallerTest.php
@@ -22,9 +22,23 @@ final class CoreInstallerTest extends TestCase
$this->installer = self::getServiceByClassName(CoreInstaller::class);
}
+ /**
+ * @throws \Doctrine\DBAL\DBALException
+ */
public function testImportSchema(): void
{
$this->installer->setOutput(new NullOutput());
$this->installer->importSchema();
}
+
+ /**
+ * @throws \Doctrine\DBAL\DBALException
+ *
+ * @depends testImportSchema
+ */
+ public function testRecreateSchemaIfExists(): void
+ {
+ $this->installer->setOutput(new NullOutput());
+ $this->installer->importSchema();
+ }
}Another question:
| if (!empty($schemaManager->listTables())) { | ||
| $io = new SymfonyStyle($input, $output); | ||
| if (!$io->confirm('Running this command will delete data in all Ibexa generated tables. Continue?', )) { | ||
| if (!$io->confirm('Running this command will delete data in all Ibexa generated tables. Continue?', false)) { |
There was a problem hiding this comment.
How does this behave in a non-interactive environment? E.g.: Behat regression tests.
There was a problem hiding this comment.
There could be a --yes or --force option.
There was a problem hiding this comment.
Non-interactive environments defaults to whatever was set as default - in this case, false. So we basically stop, which is what we'd expect.
There was a problem hiding this comment.
In e75f255 I added a --force option.
If a non-interactive env stop on this question because it's default is now false while it was genuinely squishing an existing database, update the command call with adding -f.
What do you think?
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
More cross-platform solution.
If written only for FKs, it didn't work.
e75f255 to
2525595
Compare
|
6 participants
Description:
Before:
Issues:
Fixes:
After:
Remaining issue I'm not able to fix by myself:
ibexa_payment_token,ibexa_taxonomy_assignments, andibexa_taxonomy_entries.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: