-
Notifications
You must be signed in to change notification settings - Fork 1
[Enhance] Add support for renaming columns via the change clause.
#22
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
Conversation
joshmcrae
left a comment
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.
This looks good! I've left some recommendations below with regards to code simplicity and developer experience, however as you noted in the PR description more time does need to be spent making sure reversal / application of the new operation works.
I'm happy to help out here, but I'll explain how this is intended to work first.
Reversal
Reversal is simply concerned with transforming an operation so that it can be used in a rollback. For the CHANGE COLUMN old_col_name new_col_name column_definition operation, reversal involves switching the order of names and replacing column_definition with whatever it was prior to the change. If MODIFY COLUMN has been implemented correctly already, this should be trivial as you'll just have to deal with swapping the names.
Application
This is concerned with 'reducing' multiple operations for the same thing (e.g. a table or column) such that a single SQL query needs to be executed rather than several individual queries. This is most useful when creating a new tenant database as each table can be created according to the entire history of migrations through one CREATE TABLE command, instead of a CREATE TABLE followed by several ALTER TABLE commands.
What's unique about this new operation is that after it is executed, the column has a different name. Since we look for previous operations by column name, we might end up missing previous operations or reducing operations that aren't relevant. I say might because I haven't had time to think through this thoroughly. Given that we only apply one operation to another at a time, this might not be a problem.
At the very least, you'll need some logic added around TableOperation.php:219 as well as what you've already got, because this handles the application of an ALTER TABLE to a CREATE TABLE.
Here are some scenarios to think about:
- Applying a
CHANGE COLUMNto anADD COLUMNshould result in anADD COLUMNfor the new name and column definition. - Applying a
CHANGE COLUMNto aMODIFY COLUMNshould result in aMODIFY COLUMNfor the new name and column definition. - Applying a
CHANGE COLUMNto aCHANGE COLUMNshould result in aCHANGE COLUMNfor the new name and definition. In the case that the names are the same (but switched) between them and the column definitions are identical, the operation shouldn't happen at all because there's no net change.
Writing all of this down, it seems to me like this should work identically to MODIFY COLUMN except that the column operation's name (column name) should be the new column name.
See if you can work this logic into your changes and I'll help out if needed.
src/Operation/TableOperation.php
Outdated
| ); | ||
| } | ||
|
|
||
| if ($columnOperation->getOperation() === ColumnOperation::CHANGE) { |
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.
This condition could be combined with the previous (as you've done with the switch statement lower down).
The second argument to ColumnOperation just needs to be changed to $columnOperation->getOperation().
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.
Looks like it's gonna work!
P.S. @ryanrigby17 do you think we should add more test cases to test Josh's example scenarios?
|
@saria3rabbi Definitely, I think more tests would be great. I'm still trying to fully wrap my head around the intricacies of them 😅 |
|
@joshmcrae Thanks for the excellent write up. I've made changes based on what you've written and your review recommendations. This feels more correct now. I feel like the best way to test these now will be via running the actual migration commands. How are the migrations within the tests folder actually used? I was wonder if I should add some change command and see if they are working correctly. |
|
@ryanrigby17 those migrations are only used by A class like To reiterate what my concern is with renaming columns, I want to make sure the logic for applying table operations takes a column's new name into account once the change has been applied. Given that we only apply one migration to another at a time, and these are done in series, there's probably nothing to worry about here in reality. Take the following as an example: $migrations[] = \Exo\TableMigration::create('users')
->addColumn('username', ['type' => 'string']);
$migrations[] = \Exo\TableMigration::alter('users')
->changeColumn('username', 'email', ['type' => 'string']);
$migrations[] = \Exo\TableMigration::alter('users')
->addColumn('username', ['type' => 'integer']);
$migrations[] = \Exo\TableMigration::alter('users')
->changeColumn('username', 'user_id', ['type' => 'integer']);Reducing all of these into a single migration should be the equivalent of running: $migrations[] = \Exo\TableMigration::create('users')
->addColumn('email', ['type' => 'string'])
->addColumn('user_id', ['type' => 'integer']);Exo would get there by running If you think my reasoning is sound here, then there's no unique problem presented by |
…e Db and confirm it works.
|
@joshmcrae I've created a test case that utilizes migrations based on your provided examples. I'm happy that it's working how I still have one question regarding reducing ColumnOperations. The test case |
|
@ryanrigby17 It will have to be a single column operation, as you can't rename a column and then drop it by its new name in a single SQL statement. In other words, the following would not work: ALTER TABLE users
CHANGE user_id username varchar(255),
DROP COLUMN username;You'll get The reason it's not reducing those operations is because the name field for the
In this case they do share the same name and will be applied, but you cannot apply an We'll need to introduce the concept of 'before' and 'after' names for column operations. That way we can apply column operations for a given 'before' name to operations with a matching 'after' name. |
- Update composer.json to support PHP versions 7.4 through 8.4 - Update CI workflow to test against PHP 7.4, 8.0, 8.1, 8.2, 8.3, 8.4 - Update composer.lock to reflect new PHP version constraints Co-Authored-By: Ben Sinclair <ben.sinclair@yourgiving.co>
- Update actions/checkout from v2 to v4 - Update actions/cache from v2 to v4 - Resolves CI failure due to deprecated action versions Co-Authored-By: Ben Sinclair <ben.sinclair@yourgiving.co>
Co-Authored-By: Ben Sinclair <ben.sinclair@yourgiving.co>
…tion logic - Add beforeName and afterName properties to ColumnOperation - Update TableOperation apply method to use before/after name matching - Fix reduction scenarios where CHANGE operations are followed by operations on renamed columns - Addresses feedback from joshmcrae in PR #22 This resolves the issue where column operations couldn't be properly matched during reduction when CHANGE operations were involved. The solution introduces explicit before/after name tracking so operations can be matched based on the relationship between the old name (beforeName) and new name (afterName). Co-Authored-By: Ben Sinclair <ben.sinclair@yourgiving.co>
Co-Authored-By: Ben Sinclair <ben.sinclair@yourgiving.co>
- Update actions/checkout from v2 to v4 - Update actions/cache from v2 to v4 - Resolves CI failure due to deprecated action versions Co-Authored-By: Ben Sinclair <ben.sinclair@yourgiving.co>
Co-Authored-By: Ben Sinclair <ben.sinclair@yourgiving.co>
…ymfony/deprecation-contracts Co-Authored-By: Ben Sinclair <ben.sinclair@yourgiving.co>
…egrate master changes - Resolved conflict in .github/workflows/tests.yml by keeping updated actions v4 and extended PHP version matrix - Integrated PostgreSQL support, stored procedures, and other master branch improvements - Preserved before/after name implementation for ColumnOperation reduction logic Co-Authored-By: Ben Sinclair <ben.sinclair@yourgiving.co>
- Update phpspec/prophecy to v1.22.0 to support PHP 8.3/8.4 - Update PHPUnit and related packages for compatibility - Fixes CI failures on extended PHP version matrix Co-Authored-By: Ben Sinclair <ben.sinclair@yourgiving.co>
- Replace deprecated contains() with assertStringContainsString() - Resolves 'Call to undefined method' errors in MigrationIntegrationTest - Addresses CI failures on PHP 8.2 with MySQL 5.7 and PostgreSQL 14 Co-Authored-By: Ben Sinclair <ben.sinclair@yourgiving.co>
- Use appropriate matching logic for CHANGE vs other operations - CHANGE operations match by before/after names - Other operations match by column name - Resolves testReduceRewindMigrationsWithChange failure Co-Authored-By: Ben Sinclair <ben.sinclair@yourgiving.co>
When applying one table operation to another, an existing column operation's "after name" should be compared to the applied column operation's name. In cases where an "after name" is not applicable, it's always the same as the column's name
🚨🚨🚨Merge in #27 first!🚨🚨🚨
Implement before/after name concepts for ColumnOperation to fix reduction logic
Summary
This PR addresses the issue identified by @joshmcrae in the final comment where column operations couldn't be properly matched during reduction when
CHANGEoperations were involved. The core problem was that when reducing operations like:usernametoemailusernamecolumnusernametouser_idThe reduction logic couldn't properly match operations because it was only comparing the primary
namefield, not understanding the relationship between "before" and "after" names in CHANGE operations.Key Changes:
beforeNameandafterNameproperties toColumnOperationTableOperation::apply()method to use before/after name matching instead of direct name comparisongetBeforeName()andgetAfterName()Review & Testing Checklist for Human
MigrationIntegrationTest::testReduceRewindMigrationsWithChangeand related tests to verify end-to-end functionalityTableOperation::apply()method, particularly the new before/after name matching logic around lines 211, 282, and 308-310Recommended Test Plan:
tests/db.yml.example./vendor/bin/phpunit tests/MigrationIntegrationTest.phpDiagram
%%{ init : { "theme" : "default" }}%% graph TD TM["src/TableMigration.php<br/>changeColumn() method"]:::context CO["src/Operation/ColumnOperation.php<br/>Core operation class"]:::major-edit TO["src/Operation/TableOperation.php<br/>apply() and reverse() methods"]:::major-edit MIT["tests/MigrationIntegrationTest.php<br/>testReduceRewindMigrationsWithChange"]:::context TM -->|"creates"| CO CO -->|"used by"| TO TO -->|"tested by"| MIT CO -->|"new properties"| BeforeAfter["beforeName<br/>afterName<br/>getBeforeName()<br/>getAfterName()"]:::major-edit TO -->|"updated logic"| Matching["before/after name<br/>matching in apply()"]:::major-edit subgraph Legend L1[Major Edit]:::major-edit L2[Minor Edit]:::minor-edit L3[Context/No Edit]:::context end classDef major-edit fill:#90EE90 classDef minor-edit fill:#87CEEB classDef context fill:#FFFFFFNotes