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

Closed
wants to merge 1 commit into
base: master
from

Conversation

4 participants
@aweary
Contributor

aweary commented Sep 9, 2015

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:

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.

@sorin

This comment has been minimized.

Contributor

sorin commented Sep 9, 2015

This only works in Postgres 9.3+ not sure if sequelize officially supports older versions.

@aweary

This comment has been minimized.

Contributor

aweary commented Sep 9, 2015

Hmm, if it does is there a way in sequelize we can use this only if 9.3+ is being used?

@janmeier

This comment has been minimized.

Member

janmeier commented Sep 10, 2015

We definitely support older versions. But you can use this.sequelize.options.databaseVersion like here https://github.com/sequelize/sequelize/blob/master/lib/dialects/abstract/query-generator.js#L253

@aweary

This comment has been minimized.

Contributor

aweary commented Sep 10, 2015

Oh sweet, I added a check for postgres 9.3.0+

@aweary aweary force-pushed the aweary:master branch from a1ef471 to 20812e2 Sep 10, 2015

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);

This comment has been minimized.

@janmeier

janmeier Sep 14, 2015

Member
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 :)

This comment has been minimized.

@aweary

aweary Sep 14, 2015

Contributor

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.

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'"

This comment has been minimized.

@janmeier

janmeier Sep 14, 2015

Member

extra " here

@janmeier

This comment has been minimized.

Member

janmeier commented Sep 14, 2015

LGTM - just needs a couple of SQL unit tests

@janmeier janmeier self-assigned this Sep 14, 2015

Check if ENUM value exists before ALTER TYPE
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

@aweary aweary force-pushed the aweary:master branch from 54d7d51 to 9b77a82 Sep 14, 2015

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);

This comment has been minimized.

@mickhansen

mickhansen Sep 15, 2015

Contributor

Please add sql unit tests, don't ammend the integration tests that test sql (we're in the procress of removing them).

@janmeier

This comment has been minimized.

Member

janmeier commented Oct 8, 2015

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

@aweary

This comment has been minimized.

Contributor

aweary commented Oct 8, 2015

👍 I'll get on that as soon as I can

janmeier added a commit that referenced this pull request Jan 20, 2016

janmeier added a commit that referenced this pull request Jan 20, 2016

janmeier added a commit that referenced this pull request Jan 20, 2016

@janmeier

This comment has been minimized.

Member

janmeier commented Jan 20, 2016

Added in #5272, thanks @aweary

@janmeier janmeier closed this Jan 20, 2016

@jifeon jifeon referenced this pull request Feb 24, 2016

Merged

Postgres version and base image updated #4

1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment