Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Foreign key name inconsistency may break migrations #6518

Open
uncaught opened this issue Sep 5, 2024 · 3 comments
Open

Foreign key name inconsistency may break migrations #6518

uncaught opened this issue Sep 5, 2024 · 3 comments

Comments

@uncaught
Copy link

uncaught commented Sep 5, 2024

Bug Report

Q A
Version 4.1.0, but introduced earlier

Summary

  • Doctrine schema tools determine the names of foreign keys
  • At some point in the past they stopped tracking name changes (I guess somewhere in 3.x) in migration diffs
  • This leads to broken migrations when the FK names are different, but need to be dropped

Current behaviour

  • doctrine:migrations:diff does not rename FKs if they are out of sync with the naming strategy

How to reproduce

  • Database A was created some time ago and is used in development to create migration diffs
  • At some point table names or columns were renamed
  • Now the FK names are out of sync with what Doctrine would name them if they were freshly created
  • Now create database B with doctrine:schema:create
  • Database B will have the new FK names
  • We rename tables/columns, create a diff based on database A, which includes DROP FOREIGN KEYs
  • This diff is incompatible with database B because the named FKs don't exist there
  • The migration crashes on database B

Expected behaviour

  • If Doctrine names FKs itself, then it must track the names in all diffs itself, too.
  • There is no other way in keeping schemas in sync and not create a scenario where migrations fail.

This problem was solved with #6390, but then reverted again due to #6437.

I understand the problems in #6437, but legacy FKs shouldn't be the reason to prevent the migration system from working properly. I'm ok with having to opt-in into this, although I guess it would make more sense to offer a way to exclude certain custom/legacy FKs from the diff.


Right now I'm stuck with production servers not being able to update. And an update-path might be tricky. I might have to drop all FKs and re-add them with known names.

@uncaught
Copy link
Author

uncaught commented Sep 5, 2024

I had to drop all FKs before the breaking version
  // Version20240814000000.php
  
  public function up(Schema $schema): void {
    $sql = "SELECT TABLE_NAME, CONSTRAINT_NAME FROM information_schema.TABLE_CONSTRAINTS where table_schema = '$db' and constraint_type = 'FOREIGN KEY';";
    $rows = $this->connection->iterateNumeric($sql);
    foreach ($rows as [$table, $constraint]) {
      $this->connection->executeStatement("ALTER TABLE $table DROP CONSTRAINT $constraint");
    }
  }

Remove all FK statements from the breaking version

Re-add all FKs with their proper names after the breaking version

And added a postUp check to detect FK changes manually
  public function postUp(Schema $schema): void {
    if ($this->debug && $this->version > 'Version20240814000000') {
      $db = $this->connection->getDatabase();
      $sql = "SELECT TABLE_NAME, CONSTRAINT_NAME FROM information_schema.TABLE_CONSTRAINTS where table_schema = '$db' and constraint_type = 'FOREIGN KEY';";
      $rows = $this->connection->iterateNumeric($sql);
      $actualFks = [];
      foreach ($rows as [$table, $constraint]) {
        $actualFks[$table][] = $constraint;
      }

      $tableReflection = new \ReflectionClass(Table::class);
      $getMax = $tableReflection->getMethod('_getMaxIdentifierLength');
      $fkReflection = new \ReflectionClass(ForeignKeyConstraint::class);
      $getIdentifier = $fkReflection->getMethod('_generateIdentifierName');

      $mismatch = false;
      foreach ($schema->getTables() as $table) {
        $max = $getMax->invoke($table);
        $wanted = [];
        foreach ($table->getForeignKeys() as $fk) {
          $name = $getIdentifier->invoke($fk, array_merge([$table->getName()], $fk->getLocalColumns()), 'fk', $max);
          $wanted[] = strtoupper($name);
        }
        sort($wanted);
        $actual = $actualFks[$table->getName()] ?? [];
        sort($actual);
        $missing = array_diff($wanted, $actual);
        $superfluous = array_diff($actual, $wanted);
        if ($missing || $superfluous) {
          $missing = implode(', ', $missing);
          $superfluous = implode(', ', $superfluous);
          $this->logger->error("Table {$table->getName()} has mismatching FK names! missing: [$missing], superfluous: [$superfluous]");
          $mismatch = true;
        }
      }

      if ($mismatch) {
        throw new \RuntimeException('FK mismatch');
      }
    }
  }

While debugging I saw that 75 FKs in total had gone out of sync 🥹

@uncaught
Copy link
Author

uncaught commented Sep 6, 2024

#6520 is a pain to merge into 4.1... it's totally unclear to me, how to get the configuration into the comparator using the platform in between now.

@uncaught
Copy link
Author

uncaught commented Sep 6, 2024

I'm targeting 4.2 now directly: #6521

uncaught added a commit to uncaught/doctrine-dbal that referenced this issue Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant