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(sync): allow to bypass drop statements when sync with alter enabled #11708

Merged

Conversation

@Floppy012
Copy link
Contributor

Floppy012 commented Nov 24, 2019

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

This is a less impactful version of a PR from last year (#9732 #9731). This feature implements the ability for the alter option to take an object for further configuration of the alter process during sequelize.sync(). Inside of the object the developer then can decide whether to allow drop statements or not using the drop option.

To give a small example why this feature is useful: I'm currently developing an app that can be extended by plugins. These plugins should have the ability to extend existing database models. I found out about the alter option in sync but it is currently not suitable for me since the end-user would instantly lose data if a plugin would not load correctly or would get removed by the user.

Usage of this feature would look like this:

// Alter table but don't generate drop statements
sequelize.sync({ alter: { drop : false } });

// Normal alter behavior as expected
sequelize.sync({ alter: true });

// Same behavior as { alter: true } (for safety reasons)
sequelize.sync({ alter: {} });

// Normal behavior as expected
sequelize.sync();
@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 24, 2019

Codecov Report

Merging #11708 into master will decrease coverage by 5.95%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11708      +/-   ##
==========================================
- Coverage   95.83%   89.88%   -5.96%     
==========================================
  Files          91       92       +1     
  Lines        8987     9095     +108     
==========================================
- Hits         8613     8175     -438     
- Misses        374      920     +546
Impacted Files Coverage Δ
lib/sequelize.js 95.31% <ø> (ø) ⬆️
lib/model.js 96.48% <100%> (-0.07%) ⬇️
lib/dialects/mssql/query-generator.js 2.62% <0%> (-93.3%) ⬇️
lib/dialects/mssql/query-interface.js 8.69% <0%> (-91.31%) ⬇️
lib/dialects/mssql/query.js 5.58% <0%> (-89.95%) ⬇️
lib/dialects/mssql/resource-lock.js 22.22% <0%> (-77.78%) ⬇️
lib/dialects/mssql/data-types.js 30.12% <0%> (-69.88%) ⬇️
...dialects/abstract/query-generator/helpers/quote.js 93.33% <0%> (-6.67%) ⬇️
lib/dialects/abstract/query-generator.js 95.81% <0%> (-1.2%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update beeff96...cfe80df. Read the comment docs.

@Floppy012 Floppy012 force-pushed the Floppy012:feature/bypass-drop-on-alter-sync branch from 9eb8e3c to cfe80df Nov 24, 2019
@sushantdhiman sushantdhiman merged commit bc87076 into sequelize:master Dec 12, 2019
4 checks passed
4 checks passed
codecov/patch 100% of diff hit (target 95.83%)
Details
codecov/project Absolute coverage decreased by -5.95% but relative coverage increased by +4.16% compared to beeff96
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sushantdhiman

This comment has been minimized.

Copy link
Member

sushantdhiman commented Dec 12, 2019

Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.