Incorrect handling changed primary key

[SCIF]

I've received next error message:

SQLSTATE[42704]: Undefined object: 7 ERROR:  index "primary" does not exist

Searching got me next ticket: doctrine/orm#6271

Here is a dump of diff:

Doctrine\DBAL\Schema\TableDiff {#2273
  +name: "sellers"
  +newName: false
  +addedColumns: array:1 [
    "id" => Doctrine\DBAL\Schema\Column {#1972
      #_type: Doctrine\DBAL\Types\IntegerType {#183}
      #_length: null
      #_precision: 0
      #_scale: 0
      #_unsigned: true
      #_fixed: false
      #_notnull: true
      #_default: null
      #_autoincrement: false
      #_platformOptions: array:1 [
        "version" => false
      ]
      #_columnDefinition: null
      #_comment: null
      #_customSchemaOptions: []
      #_name: "id"
      #_namespace: null
      #_quoted: false
    }
  ]
  +changedColumns: []
  +removedColumns: array:1 [
    "user_id" => Doctrine\DBAL\Schema\Column {#1410
      #_type: Doctrine\DBAL\Types\SmallIntType {#171}
      #_length: null
      #_precision: 10
      #_scale: 0
      #_unsigned: false
      #_fixed: false
      #_notnull: true
      #_default: null
      #_autoincrement: false
      #_platformOptions: []
      #_columnDefinition: null
      #_comment: null
      #_customSchemaOptions: []
      #_name: "user_id"
      #_namespace: null
      #_quoted: false
    }
  ]
  +renamedColumns: []
  +addedIndexes: []
  +changedIndexes: array:1 [
    "sellers_pkey" => Doctrine\DBAL\Schema\Index {#1993
      #_columns: array:1 [
        "id" => Doctrine\DBAL\Schema\Identifier {#1994
          #_name: "id"
          #_namespace: null
          #_quoted: false
        }
      ]
      #_isUnique: true
      #_isPrimary: true
      #_flags: []
      -options: []
      #_name: "primary"
      #_namespace: null
      #_quoted: false
    }
  ]
  +removedIndexes: []
  +renamedIndexes: []
  +addedForeignKeys: []
  +changedForeignKeys: []
  +removedForeignKeys: []
  +fromTable: Doctrine\DBAL\Schema\Table {#1426
    #_name: "sellers"
    #_columns: array:3 [
      "user_id" => Doctrine\DBAL\Schema\Column {#1410}
…
    ]
    -implicitIndexes: []
    #_indexes: array:1 [
      "sellers_pkey" => Doctrine\DBAL\Schema\Index {#1364
        #_columns: array:1 [
          "user_id" => Doctrine\DBAL\Schema\Identifier {#1395
            #_name: "user_id"
            #_namespace: null
            #_quoted: false
          }
        ]
        #_isUnique: true
        #_isPrimary: true
        #_flags: []
        -options: []
        #_name: "sellers_pkey"
        #_namespace: null
        #_quoted: false
      }
    ]
    #_primaryKeyName: "sellers_pkey"
    #_fkConstraints: []
    #_options: []
    #_schemaConfig: Doctrine\DBAL\Schema\SchemaConfig {#1889
      #hasExplicitForeignKeyIndexes: false
      #maxIdentifierLength: 63
      #name: "dachnik"
      #defaultTableOptions: []
    }
    #_namespace: null
    #_quoted: false
  }
}

Diff looks correct except one notice: name for primary keys is primary for newely generated index but typical postgres schema looks like {$tableName}_pkey. That difference causes generation incorrect SQL here. That line produces:

DROP INDEX "primary";

Changing code to:

        foreach ($diff->changedIndexes as $indexName => $index) {
            $sql[] = $this->getDropIndexSQL($indexName, $tableName);
        }

probably is a bit better but not fully correct, because changed PK could hould raise much more difference than just drop index (postgres requires to drop constraint {$table}_pkey before dropping index).

I'm ready to prepare PR and add proper tests but require help of gurus to advice me a correct direction. :)

Thanks.

[lcobucci]

@SCIF thanks for being ready to help us! Writing functional tests for schema comparison is quite simple, you can use these tests as example:

dbal/tests/Doctrine/Tests/DBAL/Functional/Schema/PostgreSqlSchemaManagerTest.php

Lines 470 to 505 in 954ce2e

	/**
	* @group 2916
	*
	* @dataProvider autoIncrementTypeMigrations
	*/
	public function testAlterTableAutoIncrementIntToBigInt(string $from, string $to, string $expected) : void
	{
	$tableFrom = new Table('autoinc_type_modification');
	$column = $tableFrom->addColumn('id', $from);
	$column->setAutoincrement(true);
	$this->_sm->dropAndCreateTable($tableFrom);
	$tableFrom = $this->_sm->listTableDetails('autoinc_type_modification');
	self::assertTrue($tableFrom->getColumn('id')->getAutoincrement());
	$tableTo = new Table('autoinc_type_modification');
	$column = $tableTo->addColumn('id', $to);
	$column->setAutoincrement(true);
	$c = new Comparator();
	$diff = $c->diffTable($tableFrom, $tableTo);
	self::assertInstanceOf(TableDiff::class, $diff, "There should be a difference and not false being returned from the table comparison");
	self::assertSame(['ALTER TABLE autoinc_type_modification ALTER id TYPE ' . $expected], $this->_conn->getDatabasePlatform()->getAlterTableSQL($diff));
	$this->_sm->alterTable($diff);
	$tableFinal = $this->_sm->listTableDetails('autoinc_type_modification');
	self::assertTrue($tableFinal->getColumn('id')->getAutoincrement());
	}
	public function autoIncrementTypeMigrations() : array
	{
	return [
	'int->bigint' => ['integer', 'bigint', 'BIGINT'],
	'bigint->int' => ['bigint', 'integer', 'INT'],
	];
	}
	}

[SCIF]

@lcobucci , please take a look on test introduced: SCIF@c7ca968

[lcobucci]

Can you send us a PR? It's way easier for us

[SCIF]

@lcobucci , I thought to send PR since I will fix it. Right now it's just test case reproducing issue.

[lcobucci]

@SCIF you can send the PR with the test only and add new commits with the fix later 👍

[SCIF]

@lcobucci , PR and error.

[SCIF]

PK'es are treated as index. Let's assume it's correct idea and should not be changed.
So, these lines looks correct and next step is take a look here. We can't distinct PK'es and general indexes if supplied $index is just a string. I can introduce check for PSQL platfor if $index is object and isPrimaryKey() and then run drop constraint method. But that will introduce a bit of inconsistency of API because string indexes will not be checked and handled by incorrect way. I worry that BC brake policy will not allow me to refactor signature in favour of: public function getDropIndexSQL(Index $index, string $table = null)

[SCIF]

Hi @lcobucci , I changed my mind and implemented a fix in another way. Could you check it? All green except scrutinizer's check detected previously introduced bugs. Please take a look on my implementation.

[diimpp]

Hi, same issue with Postgresql and changing index.

Is it possible to tag this issue, so it would be clear from first glance if it's valid, confirmed, bug or worked on.

Thanks.

[Ocramius]

worked on

See related PRs