Do not synchronize by default?

[Thooto]

I'm submitting a...

[ ] Regression 
[ ] Bug report
[x] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

Not explicitely providing the synchronize: false option to the module calls the sequelize.sync() method which can alter a database in unexpected ways.

Expected behavior

Not explicitely providing the synchronize option shouldn't call the sequelize.sync() method.

What is the motivation / use case for changing the behavior?

I believe having the synchronize option enabled by default can be quite unforgiving if forgotten or omitted as this isn't a sequelize default behaviour (you have to explicitely call sequelize.sync()) and would be safer for projects relying on another database syncing mechanism (migrations, etc.).

Also the documentation doesn't appear really clear to me (keeping both original sync option and the new synchronize option).

Thanks for the consideration!

[cojack]

It's still related?

[aroncal]

I think there's something wrong in the following code:

https://github.com/nestjs/sequelize/blob/f93e517458f462956e0070bcea9cde4f2532bca9/lib/sequelize-core.module.ts#L155

Here:

        if (typeof options.synchronize === 'undefined' || options.synchronize) {
          await sequelize.sync(options.sync);
        }

I think it should be:

        if (typeof options.synchronize === 'undefined' || options.synchronize) {
          await sequelize.sync(options.synchronize);
        }

Following Sequelize's manual here: https://sequelize.org/docs/v6/core-concepts/model-basics/#model-synchronization

User.sync() - This creates the table if it doesn't exist (and does nothing if it already exists)

So that, if in nestjs/sequelize we find options.synchronize === 'undefined', i.e. missing synchronizeoption, it would be transmitted to Sequelize as a non-destructive syncronization (no DROPS or ALTERS, just CREATE TABLE if it doesn't exist).

I would amend it myself, but it'd feel safer if a contributor checks this supposition of mine.

[aroncal]

Hi @kamilmysliwiec, can we see where the commit for this fix is? Thanks!

[kamilmysliwiec]

I think it should be: options.synchronize

The options.sync property is specifically designed to configure the synchronization process. We intentionally pass it as an argument to the sync() method; there's nothing wrong here.

As of now, the code is functioning as intended.