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

feat(postgres): support for ADD COLUMN IF NOT EXISTS and DROP COLUMN IF EXISTS #15119

Merged

Conversation

frostzt
Copy link
Contributor

@frostzt frostzt commented Oct 12, 2022

Pull Request Checklist

  • Have you added new tests to prevent regressions?
  • If a documentation update is necessary, have you opened a PR to the documentation repository?
  • Did you update the typescript typings accordingly (if applicable)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Does the name of your PR follow our conventions?

Description Of Change

Its a feature request opened by #14928
Postgres supports IF NOT EXISTS clause on adding columns. Adds support for Postgres dialect!

Todos

  • Add tests for IF NOT EXISTS both Unit and Integration
  • Add dialect support check for all the other dialects

@frostzt
Copy link
Contributor Author

frostzt commented Oct 12, 2022

@ephys I have few questions on this as you mentioned that dialects that don't support this should throw, I do think there are some workarounds for few of them should I try and implement them if possible. Like for ones that don't support it as you mentioned they can first query it and then try and add it if not exists.

Second question I have is passing that option in the attribute a good idea because I don't think that is what I am supposed to do. Is there a better way to implement it?

Also another question is, as you said should all the dialects that don't support this feature should throw right?

@frostzt frostzt marked this pull request as draft October 12, 2022 14:56
Copy link

@Betree Betree left a comment

Choose a reason for hiding this comment

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

Nice work @frostzt, it's looking good so far! Thanks for getting the ball rolling on this one.

I'm not the best person to provide a comprehensive review, but I think we should keep #11774 in mind while implementing this.

@frostzt
Copy link
Contributor Author

frostzt commented Oct 13, 2022

@ephys I think if the above is correct I can push the changes for #11774 in this PR too?

@WikiRik
Copy link
Member

WikiRik commented Oct 13, 2022

I think it would be good to tackle that in this PR as well

@fzn0x
Copy link
Member

fzn0x commented Oct 13, 2022

Maybe you can rename this PR to includes DROP COLUMN IF EXIST if you are sure want to tackle it in this PR, it would be great :) @frostzt

@frostzt frostzt changed the title feat: IF NOT EXISTS support for Postgres feat(postgres): support for IF NOT EXISTS and DROP IF NOT EXISTS Oct 13, 2022
@fzn0x fzn0x changed the title feat(postgres): support for IF NOT EXISTS and DROP IF NOT EXISTS feat(postgres): support for IF NOT EXISTS and DROP IF EXISTS Oct 13, 2022
Copy link
Member

@ephys ephys left a comment

Choose a reason for hiding this comment

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

Nice work :)

Yes feel free to add DROP COLUMN it in this PR if you prefer, splitting it is fine too


Could you add a unit test in unit/query-generator/add-column-query.test.ts?

All dialects should be tested, but the ones that don't support the option should throw (like in list-databases-query.test.ts)

I don't think you need an integration test for this option


I do think there are some workarounds for few of them should I try and implement them if possible

I think we can look into that if there is interest but I'd propose to keep it for a follow-up PR

src/dialects/abstract/index.ts Outdated Show resolved Hide resolved
src/dialects/postgres/query-generator.js Outdated Show resolved Hide resolved
@ephys ephys changed the title feat(postgres): support for IF NOT EXISTS and DROP IF EXISTS feat(postgres): support for CREATE COLUMN IF NOT EXISTS and DROP COLUMN IF EXISTS Oct 13, 2022
@ephys ephys changed the title feat(postgres): support for CREATE COLUMN IF NOT EXISTS and DROP COLUMN IF EXISTS feat(postgres): support for ADD COLUMN IF NOT EXISTS and DROP COLUMN IF EXISTS Oct 13, 2022
@fzn0x
Copy link
Member

fzn0x commented Oct 13, 2022

Better PR name :)

@ephys
Copy link
Member

ephys commented Oct 13, 2022

Gotta be super explicit for the changelog :p

@frostzt frostzt requested a review from ephys October 15, 2022 12:01
@fzn0x fzn0x added the type: feature For issues and PRs. For new features. Never breaking changes. label Oct 16, 2022
@frostzt frostzt requested a review from ephys October 16, 2022 12:41
Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

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

Few comments about your tests, haven't looked into the rest yet

test/unit/query-generator/add-column-query.test.ts Outdated Show resolved Hide resolved
test/unit/query-generator/add-column-query.test.ts Outdated Show resolved Hide resolved
test/unit/query-generator/add-column-query.test.ts Outdated Show resolved Hide resolved
test/unit/query-generator/add-column-query.test.ts Outdated Show resolved Hide resolved
@frostzt frostzt requested review from WikiRik and ephys and removed request for ephys and WikiRik October 18, 2022 18:41
@frostzt frostzt marked this pull request as ready for review October 18, 2022 18:59
Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

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

It seems that this is not a postgres exclusive feature

@ephys I'll let you look at the type definitions for {add,remove}ColumnQuery

test/unit/query-generator/remove-column-query.test.ts Outdated Show resolved Hide resolved
src/dialects/postgres/query-generator.js Show resolved Hide resolved
src/dialects/postgres/index.js Show resolved Hide resolved
src/dialects/abstract/query-interface.js Show resolved Hide resolved
src/dialects/abstract/query-generator.d.ts Show resolved Hide resolved
src/dialects/mariadb/index.js Show resolved Hide resolved
src/dialects/mssql/index.js Show resolved Hide resolved
Copy link
Member

@ephys ephys left a comment

Choose a reason for hiding this comment

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

This looks good to me. I'm OK with adding support for other dialects in follow-up PRs. :)

Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

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

Yes, let's add support for additional dialects in future PRs and merge this one now. Would be nice if you could still work on those @frostzt :)

@WikiRik WikiRik merged commit 76cc97f into sequelize:main Nov 5, 2022
@fzn0x
Copy link
Member

fzn0x commented Nov 5, 2022

Happy to see this is landed!

@frostzt
Copy link
Contributor Author

frostzt commented Nov 6, 2022

@WikiRik thanks I'll start working on those!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature For issues and PRs. For new features. Never breaking changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants