sequelize / umzug

Framework agnostic migration tool for Node.js
MIT License
2.01k stars 158 forks source link

Do not set charset and collate options on postgresql #623

Closed QuentinFarizon closed 11 months ago

QuentinFarizon commented 11 months ago

Hi! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch umzug@2.3.0 for the project I'm working on.

Sequelize v7.0.0-alpha.33 complains (rightly so), that collate and charset optinos are not supposed to be passed for postgres dialect. To my knowledge, this is only used for mysql

Here is the diff that solved my problem:

diff --git a/node_modules/umzug/lib/storages/SequelizeStorage.js b/node_modules/umzug/lib/storages/SequelizeStorage.js
index 1277063..469a6cc 100644
--- a/node_modules/umzug/lib/storages/SequelizeStorage.js
+++ b/node_modules/umzug/lib/storages/SequelizeStorage.js
@@ -90,8 +90,8 @@ class SequelizeStorage extends _Storage.default {
       tableName: this.tableName,
       schema: this.schema,
       timestamps: this.timestamps,
-      charset: 'utf8',
-      collate: 'utf8_unicode_ci'
+      charset: this.sequelize.dialect.name === 'mysql' ? 'utf8' : null,
+      collate: this.sequelize.dialect.name === 'mysql' ? 'utf8_unicode_ci' : null
     });
   }
   /**

This issue body was partially generated by patch-package.

WikiRik commented 11 months ago

See #612 for earlier discussion about this

QuentinFarizon commented 11 months ago

@WikiRik What do you think of my solution ?

With something like :

const charsetableDialects = ['mysql', 'mariadb', 'snowflake']
(...)
charset: charsetableDialects.includes(this.sequelize.dialect.name) ? 'utf8' : null,
collate: charsetableDialects.includes(this.sequelize.dialect.name) ? 'utf8_unicode_ci' : null
WikiRik commented 11 months ago

That looks good

QuentinFarizon commented 11 months ago

@WikiRik I've pushed a PR : https://github.com/sequelize/umzug/pull/624

github-actions[bot] commented 11 months ago

Released in v3.4.0.

github-actions[bot] commented 11 months ago

Released in v3.4.0.

qfarizon-qftech commented 5 months ago

@WikiRik It would be great to update sequelize-cli to use this new version !

WikiRik commented 5 months ago

@qfarizon-qftech sequelize-cli is not meant tested with or updated for the v7 alphas and still uses umzug v2. The new CLI, which we started work on in the sequelize monorepo (and will be published as @sequelize/cli), will be using umzug v3 with this update.

qfarizon-qftech commented 5 months ago

@WikiRik Can I test @sequelize/cli as of today ?

WikiRik commented 5 months ago

@WikiRik Can I test @sequelize/cli as of today ?

It only has the seed generate and migration generate commands so it's not usable yet. (We're doing a full rewrite)

If @mmkal agrees you might be able to backport your change to umzug v2 and see how much of the old CLI works with the v7 alphas.

qfarizon-qftech commented 5 months ago

For now I have patched umzug@2 using patch-package.

mmkal commented 5 months ago

If anyone would like to backport anything to v2.x I'm fine to review a pull request and publish, just open it against v2.x branch. I won't check it beyond just building and publishing though.