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

Add 'truncate' option to sequelize.sync. #2671

Closed
wants to merge 2 commits into from

Conversation

dkushner
Copy link

@dkushner dkushner commented Dec 4, 2014

Fixes #2670.

@mickhansen
Copy link
Contributor

@dkushner how is this exactly related to sync? Wouldn't this simply be a standalone truncate method?

@mickhansen
Copy link
Contributor

(and i actually believe we have truncate support via truncate: true for deletes)

* @param {Boolean} [options.truncate=false] Similar to force, if truncate is
* true, each DAO will do TRUNCATE TABLE, eliminating all rows within the
* table while maintaining the table structure. Note that force will always
* override this option.
Copy link
Contributor

Choose a reason for hiding this comment

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

@janmeier will this doc syntax work?

Copy link
Member

Choose a reason for hiding this comment

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

Nope

Copy link
Author

Choose a reason for hiding this comment

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

@janmeier Adding some more commits to move the functionality out of sync(). What about the syntax breaks the docs? I'll make sure not to do it again.

@dkushner
Copy link
Author

dkushner commented Dec 5, 2014

@mickhansen, it definitely can be a stand-alone truncate method. Would that be preferable? I was trying to avoid disrupting the existing API as much as possible. I also have a number of conceptual objections to the use of sync({ force: true }) given that Sequelize models do not represent an authoritative source for a database schema (and should not).

@mickhansen
Copy link
Contributor

@dkushner well in my opinion it doesn't just make a lot of sense in sync since that carries a different meaning.

I neither fully disagree or agree with your objections to sync({force: true}), it's great for its few usecases - but no, it's obviously not the solution you want after you go out of the experimental phase.

@dkushner
Copy link
Author

@mickhansen, I just saw your comment regarding the Model.delete({ truncate: true }) implementation. Since sequelize.sync() essentially just goes through the list of DAOs and calls sync() on each one, would this be an appropriate behaviour for sequelize.truncate()?

For example, it would just be something like:

return Promise.reduce(this.modelManager.daos, function(ag, dao) {
    return dao.delete({}, { truncate: true });
}, null);

Perhaps this is contrived, but a way to simply clear all rows of the database without affecting the schema seems useful to me. Thoughts?

@mickhansen
Copy link
Contributor

@dkushner Utility methods are fine to have. sequelize.truncate() would be fine :)
Your proposed solution for implementing a top level truncate method could work, although i'd probably just use Promise.map.

@mickhansen
Copy link
Contributor

Needs a rebase @dkushner, didn't see that you moved it to it's own method.

@mickhansen mickhansen self-assigned this Dec 22, 2014
@dkushner
Copy link
Author

@mickhansen, gotcha. My bad on that.

@mickhansen
Copy link
Contributor

Ping @dkushner

@janmeier
Copy link
Member

No response, closing

@janmeier janmeier closed this May 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sequelize.sync({ force: true }) is too dangerous to live.
3 participants