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
Check if ENUM value exists before ALTER TYPE #4464
Conversation
This only works in Postgres 9.3+ not sure if sequelize officially supports older versions. |
Hmm, if it does is there a way in sequelize we can use this only if 9.3+ is being used? |
We definitely support older versions. But you can use |
Oh sweet, I added a check for postgres |
var enumName = this.pgEnumName(tableName, attr); | ||
var sql = semver.gte(this.sequelize.options.databaseVersion, '9.3.0') | ||
? 'ALTER TYPE ' + enumName + ' ADD VALUE IF NOT EXISTS ' + this.escape(value) | ||
: 'ALTER TYPE ' + enumName + ' ADD VALUE ' + this.escape(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sql = 'ALTER TYPE ' + enumName + ' ADD VALUE';
if (semver.gte(this.sequelize.options.databaseVersion, '9.3.0')) {
sql += 'IF NOT EXISTS';
}
sql += this.escape(value)
Nitpicking now, but this is slightly more DRY, and less prone to errors where you refactor one case but not the other :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree! I was thinking of doing something like this but thought it might have hurt readability a bit, so decided against it. Let me fix it and squash these commits real quick.
LGTM - just needs a couple of SQL unit tests |
Currently sequelize will not check if an ENUM value exists before attempting to alter the type. If it does it can throw an error such as: ``` SequelizeDatabaseError: enum label "[labelName]" already exists at [object Object].Query.formatError (node_modules/sequelize/lib/dialects/postgres/query.js:433:14) at [object Object].<anonymous> (node_modules/sequelize/lib/dialects/postgres/query.js:108:19) at [object Object].Query.handleError (node_modules/pg/lib/query.js:108:8) at [object Object].<anonymous> (node_modules/pg/lib/client.js:171:26) at Socket.<anonymous> (node_modules/pg/lib/connection.js:109:12) at readableAddChunk (_stream_readable.js:146:16) at Socket.Readable.push (_stream_readable.js:110:10) at TCP.onread (net.js:523:20) ``` By adding the `IF NOT EXISTS` qualifier it will only throw a `NOTICE` that the enum label exists and skip it. Check for Postgres 9.3.0 and greater require in semver Add check for 9.3.0 ENUM feature Remove extra quotation mark Break out conditional SQL statement into IF block
count++; | ||
} | ||
else if (sql.indexOf('joyful') > -1) { | ||
var cmd = semver.gte(this.sequelize.options.databaseVersion, '9.3.0') | ||
? "ALTER TYPE \"enum_UserEnums_mood\" ADD VALUE IF NOT EXISTS 'joyful' AFTER 'meh'" | ||
: "ALTER TYPE \"enum_UserEnums_mood\" ADD VALUE 'joyful' AFTER 'meh'" | ||
expect(sql.indexOf("ALTER TYPE \"enum_UserEnums_mood\" ADD VALUE 'joyful' AFTER 'meh'")).to.not.be.equal(-1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add sql unit tests, don't ammend the integration tests that test sql (we're in the procress of removing them).
Ping @aweary . Please move the test to https://github.com/sequelize/sequelize/blob/master/test/unit/sql/enum.test.js then this should be good to go |
👍 I'll get on that as soon as I can |
Currently sequelize will not check if an
ENUM
value exists before attempting to alter the type. As it stands it can throw an error such as:By adding the
IF NOT EXISTS
qualifier it will only throw aNOTICE
that the enum label exists and skip it.