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.

Sign up for GitHub

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

Jump to bottom

feat: add support to string array on dropColumns #7654

Merged
merged 3 commits into from Jul 31, 2021
Merged

feat: add support to string array on dropColumns #7654

merged 3 commits into from Jul 31, 2021

Conversation

mateusppereira
Copy link
Contributor

@mateusppereira mateusppereira commented May 13, 2021 •
edited by AlexMesser

Description of change

This PR intends to add support to an array of strings when droping a list of columns.
After this PR, this:

await queryRunner.dropColumns(
  'table_x',
  [
    new TableColumn({ name: 'id', type: 'int' }),
    new TableColumn({ name: 'name', type: 'varchar' }),
  ],
)

will turn into this:

await queryRunner.dropColumns('table_x', ['id', 'name'])

Less verbosity and keep consistent with dropColumn that already accept a string.

Fixes #4807
Fixes #7655

Pull-Request Checklist

  • Code is up-to-date with the master branch
  • npm run lint passes with this change
  • npm run test passes with this change
  • This pull request links relevant issues as Fixes #0000
  • There are new or updated unit tests validating the change
  • Documentation has been updated to reflect this change
  • The new commits follow conventions explained in CONTRIBUTING.md

@mateusppereira mateusppereira marked this pull request as ready for review May 13, 2021 23:24
@mateusppereira mateusppereira changed the title feat: add support to string array on dropColumns (#1) feat: add support to string array on dropColumns May 14, 2021
@mateusppereira mateusppereira mentioned this pull request May 14, 2021
Closed
21 tasks
@mateusppereira
Copy link
Contributor Author

#7655

AlexMesser
AlexMesser reviewed May 15, 2021
View reviewed changes
src/driver/sqlite-abstract/AbstractSqliteQueryRunner.ts Outdated
changedTable.findColumnForeignKeys(column).forEach(fk => changedTable.removeForeignKey(fk));
columns.forEach((column: TableColumn|string) => {
const columnInstance = column instanceof TableColumn ? column : table.findColumnByName(column);
if (!columnInstance) return;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We must throw an error here instead of silently break the operation:

if (!columnInstance)
   throw new Error(`Column "${column}" was not found in table "${table.name}"`);

AlexMesser
AlexMesser reviewed May 15, 2021
View reviewed changes
src/driver/sqlite-abstract/AbstractSqliteQueryRunner.ts Outdated
table.findColumnForeignKeys(column).forEach(fk => table.removeForeignKey(fk));
columns.forEach((column: TableColumn|string) => {
const columnInstance = column instanceof TableColumn ? column : table.findColumnByName(column);
if (!columnInstance) return;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We must throw an error here instead of silently break the operation:

if (!columnInstance)
   throw new Error(`Column "${column}" was not found in table "${table.name}"`);

@mateusppereira mateusppereira marked this pull request as draft May 18, 2021 11:16
@mateusppereira
Copy link
Contributor Author

@AlexMesser i've applied your suggestions and some tests are failing now... as soon as i fixes then I let you know, thanks!

@AlexMesser
Copy link
Collaborator

#4807 related

AlexMesser
AlexMesser reviewed May 22, 2021
View reviewed changes
src/driver/sqlite-abstract/AbstractSqliteQueryRunner.ts Outdated
table.removeColumn(columnInstance);
table.findColumnUniques(columnInstance).forEach(unique => table.removeUniqueConstraint(unique));
table.findColumnIndices(columnInstance).forEach(index => table.removeIndex(index));
table.findColumnForeignKeys(columnInstance).forEach(fk => table.removeForeignKey(fk));
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this block after await this.recreateTable(changedTable, table); since table is replaced with changedTable in recreateTable() method. This is why your test is failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @AlexMesser, now all tests passed!

@mateusppereira mateusppereira marked this pull request as ready for review June 15, 2021 02:21
@AlexMesser
Update drop-column.ts
5becda2 
removed `skip`
imnotjames
imnotjames requested changes Jun 18, 2021
View reviewed changes
test/functional/query-runner/drop-column.ts

await queryRunner.executeMemoryDownSql();
describe("when columns are strings", () => {
it("should correctly drop column and revert drop", () => Promise.all(connections.map(async connection => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add test to affirm that trying to drop a non-existent column emits an error

@AlexMesser AlexMesser merged commit 91d5b2f into typeorm:master Jul 31, 2021
@AlexMesser
Copy link
Collaborator

thank you for contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imnotjames imnotjames imnotjames requested changes

@AlexMesser AlexMesser Awaiting requested review from AlexMesser

Assignees
No one assigned
Labels
needs tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

QueryRunner.dropColumns doesnt accept list of column names as strings dropColumns still have an old type
3 participants
@mateusppereira @AlexMesser @imnotjames