-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Generate alter add/change column with explicit position for MySQL #4276
Changes from all commits
ed88b44
7a68d23
51aed61
a460898
962efa9
e6408e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,9 @@ | |
use Doctrine\DBAL\Types\Type; | ||
use Doctrine\DBAL\Types\Types; | ||
use Doctrine\Tests\Types\MySqlPointType; | ||
use InvalidArgumentException; | ||
|
||
use function array_keys; | ||
|
||
class MySqlSchemaManagerTest extends SchemaManagerFunctionalTestCase | ||
{ | ||
|
@@ -216,6 +219,55 @@ public function testDoesNotPropagateDefaultValuesForUnsupportedColumnTypes(): vo | |
self::assertFalse($onlineTable->getColumn('def_blob_null')->getNotnull()); | ||
} | ||
|
||
public function testAlterTableExplicitColumnPosition(): void | ||
{ | ||
$tableName = 'test_column_position'; | ||
$table = new Table($tableName); | ||
$table->addColumn('id', 'integer'); | ||
$table->addColumn('a', 'integer'); | ||
$table->addColumn('b', 'integer'); | ||
|
||
$this->schemaManager->createTable($table); | ||
|
||
self::assertSame(['id', 'a', 'b'], array_keys($this->schemaManager->listTableColumns($tableName))); | ||
|
||
$table2 = new Table($tableName); | ||
$table2->addColumn('id', 'float'); | ||
$table2->addColumn('new', 'float'); | ||
$table2->addColumn('b', 'float'); | ||
$table2->addColumn('a', 'float'); | ||
|
||
$comparator = new Comparator(); | ||
$diff = $comparator->diffTable($table, $table2); | ||
self::assertNotFalse($diff); | ||
$this->schemaManager->alterTable($diff); | ||
|
||
self::assertSame(['id', 'new', 'b', 'a'], array_keys($this->schemaManager->listTableColumns($tableName))); | ||
} | ||
|
||
public function testAlterTableExplicitColumnPositionColumnNotFoundException(): void | ||
{ | ||
$tableName = 'test_column_position_exception'; | ||
$table = new Table($tableName); | ||
$table->addColumn('id', 'integer'); | ||
|
||
$this->schemaManager->createTable($table); | ||
|
||
$table2 = new Table($tableName); | ||
$table2->addColumn('id', 'float'); | ||
$table2WithoutAColumn = clone $table2; | ||
$table2->addColumn('a', 'float'); | ||
|
||
$comparator = new Comparator(); | ||
$diff = $comparator->diffTable($table, $table2); | ||
self::assertNotFalse($diff); | ||
$diff->toTable = $table2WithoutAColumn; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Once the diff is generated from the two tables, why should it be possible to change the referenced table via the API? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is test - to test exception https://github.com/doctrine/dbal/pull/4276/files#diff-548b552a74ea52eab68318520c213a142ba77527a4b965c4d3bd50eeba1afa9aR675 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test was meant to be added to document the case when this exception can happen because IMO it shouldn't be possible in the first place (https://github.com/doctrine/dbal/pull/4276/files#r493065750). The test doesn't show a valid case. Which means that there is no justification for this API to throw an exception. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When the diff object is built using Comparator::diff which will add the input, then this exception can not happen. However, the |
||
|
||
$this->expectException(InvalidArgumentException::class); | ||
$this->expectExceptionMessage('Column name "a" not found'); | ||
$this->schemaManager->alterTable($diff); | ||
} | ||
|
||
public function testColumnCharset(): void | ||
{ | ||
$table = new Table('test_column_charset'); | ||
|
@@ -272,7 +324,7 @@ public function testColumnCharsetChange(): void | |
$diff = $fromSchema->getMigrateToSql($toSchema, $this->connection->getDatabasePlatform()); | ||
self::assertContains( | ||
'ALTER TABLE test_column_charset_change CHANGE col_string' | ||
. ' col_string VARCHAR(100) CHARACTER SET ascii NOT NULL', | ||
. ' col_string VARCHAR(100) CHARACTER SET ascii NOT NULL FIRST', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we expect the SQL to contain the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we generate this for ALTER TABLE only, for TABLE CREATE, position is not generated if position is the same, we regenerate whole column definition There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we generate the column position only if it's changed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can add a condition, if the previous column is the same for fromTable and toTable, then to not add the position. Tests may however need more changes as currently, we expect that SQL column definition is always fully regenerated. Let solve this as the last step before merge, firstly, I want to make sure There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I understand we want to generate minimalistic diff, but I think we do not want it. ALTER TABLE ADD/MODIFY column is always generated fully even if there is a minor column definition change like column comment. Adding position based on the original table will violate this definition. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If it's always generated fully indeed then it's ok with me.
In theory yes, in practice the tests fail There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CI fixed |
||
$diff | ||
); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to avoid having this exception by using a more suitable method signature? Each column is either first or goes after some other column. See #4145 for example. Otherwise, this will need a functional test that demonstrates the exception being thrown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can call
$toTable->getColumn()
before the foreach loop which should check if the column exists or notplease sudgest ideal approach to this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might work if instead of having the
$toTable
in theSchemaDiff
, theColumnDiff
had a property like$afterColumn
that would contain the column name and be non-empty only if the column position has changed. This assumes that a similar SQL syntax could be used on other supported platforms. This would also solve the problem of having theFIRST
andAFTER
fragments unnecessarily generated.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ColumnDiff
can not be used as we need position also forColumn
. @morozov I would like to finish this PR asap, please advise, if the$toTable
is ok - used like$fromTable
already used - or if you have a better alternative. Thanks.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand what this means. Can you please elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if column already exists, then
ColumnDiff
instance is returned from the comparatorif the column is new (not renamed/modified), then there is no
ColumnDiff
for itThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exception is covered by added test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mvorisek please make sure you don't resolve the conversations that you didn't start unless you're resolving one on the starter's behalf.