feat(postgres): add name option for enum type

[sturdynut]

Pull Request check-list

Please make sure to review and check all of these items:

• Does npm run test or npm run test-DIALECT pass with this change (including linting)?
• Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
• Have you added new tests to prevent regressions?
• Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
• Did you update the typescript typings accordingly (if applicable)?
• Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Closes #2577

These changes allow for an optional name when defining enums.

Notes

• I made a change in the test/integration/model/count.test.js file. This was outside of the scope of the feature in I was working on, it was the only linter error and thought the change was pretty minimal enough to slide in.
• In one of the new enum name tests in .../postgres/query-interface.test.js, I ended up using the done() callback (which I know is called out as something not to do.) , in order to get an accurate failure message. Otherwise the test just times out and you don't see the error from the failed expect(...)...;
• In .../postgres/data-types.js I wasn't a huge fan of the type.type || type stuff on line 480, but I just don't understand the code base enough to make an informed refactoring here. Maybe it's fine, but just something I noticed as I was making changes.
• In .../postgres/query-generator.js I updated the signature for the pgEnumDrop(...) method and reluctantly settled on renaming the enumName parameter to dataTypeOrEnumName. I did this in order to not break existing calls that provided the name where the dataType was not available. Otherwise I'd have just renamed it to dataType and passed that in for all cases.
• When running the integration tests, I received 3 failures prior to making any changes. Just wanted to make sure this was known and that I didn't in fact cause it, which seems unlikely.

These are the existing broken integration tests:

  1) [POSTGRES Specific] Data Types
    DATEONLY SQL
      should be able to create and update records with Infinity/-Infinity:
  AssertionError: expected Sat Oct 05 2019 to be within Sun Oct 06 2019 and Sun Oct 06 2019

  2) [POSTGRES Specific] ExclusionConstraintError
    "before each" hook for "should contain error specific properties":
  SequelizeUnknownConstraintError: Unknown constraint error

  3) [POSTGRES Specific] QueryInterface
    createFunction
      uses declared variables:
  SequelizeDatabaseError: function "add_one" already exists with same argument types

[codecov]

Codecov Report

Attention: Patch coverage is 95.96977% with 48 lines in your changes are missing coverage. Please review.

Project coverage is 96.33%. Comparing base (2de3377) to head (7ce775f).
Report is 2104 commits behind head on main.

Files	Patch %	Lines
src/model.js	94.16%	14 Missing ⚠️
src/sql-string.ts	85.93%	6 Missing and 3 partials ⚠️
src/dialects/abstract/query.js	80.00%	4 Missing ⚠️
src/dialects/mariadb/query.js	76.92%	3 Missing ⚠️
src/dialects/abstract/connection-manager.js	0.00%	2 Missing ⚠️
src/dialects/abstract/query-generator.js	98.70%	2 Missing ⚠️
src/dialects/postgres/query-generator.js	96.29%	2 Missing ⚠️
src/sequelize.js	87.50%	2 Missing ⚠️
src/utils/validator-extras.ts	94.44%	1 Missing and 1 partial ⚠️
src/dialects/abstract/query-interface.js	97.29%	1 Missing ⚠️
... and 7 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11514      +/-   ##
==========================================
- Coverage   96.44%   96.33%   -0.12%     
==========================================
  Files          95       95              
  Lines        9154     9274     +120     
  Branches        0       90      +90     
==========================================
+ Hits         8829     8934     +105     
+ Misses        325      323       -2     
- Partials        0       17      +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kwakwaversal]

Should there be a test case for an ARRAY of named ENUMs?

[sturdynut]

Happy to add a test, but I don't quite follow. Can you provide an example of exactly what you mean?

[kwakwaversal]

Sorry for the late reply. Something like:

const NamedEnumUser = current.define('user', {
  mood: {
    type: DataTypes.ARRAY(DataTypes.ENUM({
      name: 'mood',
      values: ['happy', 'sad']
    }))
  }
});

To ensure an ARRAYs of named enums works.

[sturdynut]

Thank you, I will test for this.

[sturdynut]

I wrote the test for this and realized how this still does not seem right. I'm not sure of a case where you'd want an array of enums for the type. Apologies, maybe this is over my head but as far as I know, when you have a column in PG that is of type enum...there is only one enum type that it is tied to. Can you help me understand a bit more?

[nmchaves]

I think they're talking about a scenario like this:

CREATE TABLE person (
    name text,
    current_moods mood[]
);

...where the mood[] type allows the person to have multiple moods simultaneously.

[sturdynut]

Ahh, right. That makes sense. Thank you.

[nmchaves]

Thank you for working on this PR! 👏

[kwakwaversal]

As @nmchaves said.

An enumerated type in PostgreSQL is typically a text type. So think of it as me suggesting an array of text which is perfectly valid. The only difference by using an enum instead of text is obviously that the array values can only be one of the defined constants.

[papb]

• In .../postgres/data-types.js I wasn't a huge fan of the type.type || type stuff on line 480, but I just don't understand the code base enough to make an informed refactoring here. Maybe it's fine, but just something I noticed as I was making changes.

Weird indeed, but are you sure this is necessary? What happens when using the code below instead?

Suggested change

	const enumType = this.type.type || this.type;
	const enumType = this.type;

[sturdynut]

Made the change, re-ran tests and still passed, so should be good!

Here are the other places where I've added this same check. Tests failed when I removed either of these, so assume they are ok.

File: /lib/dialects/postgres/query-generator.js
Line 466: const enumType = attribute.type.type || attribute.type;
Line 764: const type = dataType.type || dataType;

Not sure if there is any refactoring that should be done here, but just wanted to make note that the reason I originally added these checks were because I saw the code below:

File: /lib/dialects/postgres/query-interface.js
Line 84: const enumType = type.type || type;

[papb]

Hello! I see you are a first-time contributor, thank you for taking the time to help Sequelize! I hope to see more PRs from you in the future!

I have made a first review to your PR above. Thanks for your work :)

• In .../postgres/query-generator.js I updated the signature for the pgEnumDrop(...) method and reluctantly settled on renaming the enumName parameter to dataTypeOrEnumName. I did this in order to not break existing calls that provided the name where the dataType was not available. Otherwise I'd have just renamed it to dataType and passed that in for all cases.

This part I haven't looked yet, will do later.

• When running the integration tests, I received 3 failures prior to making any changes. Just wanted to make sure this was known and that I didn't in fact cause it, which seems unlikely.

These are the existing broken integration tests:

  1) [POSTGRES Specific] Data Types
    DATEONLY SQL
      should be able to create and update records with Infinity/-Infinity:
  AssertionError: expected Sat Oct 05 2019 to be within Sun Oct 06 2019 and Sun Oct 06 2019

  2) [POSTGRES Specific] ExclusionConstraintError
    "before each" hook for "should contain error specific properties":
  SequelizeUnknownConstraintError: Unknown constraint error

  3) [POSTGRES Specific] QueryInterface
    createFunction
      uses declared variables:
  SequelizeDatabaseError: function "add_one" already exists with same argument types

Looks like this only happened locally for you, since all checks have passed in the PR, so don't worry about it. It must be something about your local postgres setup.

[sturdynut]

@papb - Thank you! I've used sequelize on many projects and it is a pleasure to help out. 😃

I've pushed my latest updates based on your feedback.

• Returned promise and removed done() callback
• Removed unnecessary .type check

Dismissing my outdated review

[papb]

Sorry I don't have time to look into it right now... Will do later, hopefully, or maybe another maintainer will show up :)

[sturdynut]

@papb - All good! I just wanted to make sure you knew that it was ready for re-review. Thank you!

[sushantdhiman]

Please remove unrelated name changes etc from this branch

[sturdynut]

@sushantdhiman -Are you referring to where I've renamed a parameter because I added another?
An example is where I've renamed "attributes" to "sqlAttributes". I'm happy to make the change I just want to be sure I fully understand.

[papb]

Hello @sturdynut, thanks for the patience so far. I am not sure what @sushantdhiman was referring to... Perhaps the change from dataType to dbDataType? That's fine by me though...

[sturdynut]

@papb - No problem. I'm happy to make whatever naming changes necessary. Once @sushantdhiman confirms direction I will proceed.

[papb]

How does this PR relate to sync({ alter: true }) (#7649)?

I think it would be great if this PR could handle both issues at the same time (and if luckily it already fixes alter: true as is, please add a test for that).

[sturdynut]

@papb - This certainly is in the same code. I can add the check for existing enum mentioned in #7649

Still waiting on confirmation as to how to proceed on this PR. I can revert any name changes I made, but I just want to confirm before I slate more time to work on this.

Is it just the name change from dataType to dbDataType? @sushantdhiman

[papb]

Thanks @sturdynut, let's wait for @sushantdhiman then :)

[ShahoG]

Any news on this? Much appreciated feature

[sturdynut]

@ShahoG - I was waiting to hear back from @sushantdhiman before moving forward. If I don't hear anything by this weekend, i'm going to make the changes I believe were requested along with the fix from #7649

I don't like having this sitting too long. Cringing thinking about potential merge conflicts, but will make traction on this over the weekend regardless.

[laurelgr]

This feature would be easier than having to use litteral queries in migrations or extend DataTypes.

How would you use it beyond adding the name? Does it create the enum if it doesn't exist, or should the enum already exists in postgres?
What if the values provided do not match the existing type, or each other across multiple models?

[sturdynut]

This feature would be easier than having to use litteral queries in migrations or extend DataTypes.

How would you use it beyond adding the name? Does it create the enum if it doesn't exist, or should the enum already exists in postgres?
What if the values provided do not match the existing type, or each other across multiple models?

This feature simply allows you to provide a name for an ENUM type vs having sequelize create it for you using the table name and field name. For existing enums, it already is named, so it wouldn't really help with changing the name, you'd likely need to drop your enum and re-create it.

Example:

DataTypes.ENUM({
    name: 'my_enum',
    values: ['value', 'another value']
 })

[ghost]

I'm sorry to bother everyone, I would love to see this merged, is there anything specific that is holding this PR back?

[angi-]

@sturdynut I'm looking forward to this PR!

How would I reuse the enum, something like this?

const statusEnum = DataTypes.ENUM({
    name: 'statuses',
    values: ['a', 'b', 'c']
});

FilterData.init({
    baseStatus: statusEnum,
    containsStatuses: DataTypes.ARRAY(statusEnum),
    notContainsStatuses: DataTypes.ARRAY(statusEnum)
});

How would I share this enum across multiple model files and migrations?

[fzn0x]

Can you continue with this PR? @sdepold
This is nice to being merged

Possibly fix :
#2577 (the early one)
#7649

Possibly close this related PR :
#12019

[ephys]

This is something I've wanted for a while so I'll likely review it as some point. Can't promise when, work is keeping me busy again :/

[sturdynut]

@ephys - It's been awhile since I looked at this myself, but will keep an eye out for any feedback you provide. 🙌🏼

[ephys]

Just noticed this PR is targeting master. We renamed the branch to main a little while ago. It's likely there are going to be a few merge conflicts

I can handle it if you don't have time but it will be after the PRs I'm currently working on

[sturdynut]

Just noticed this PR is targeting master. We renamed the branch to main a little while ago. It's likely there are going to be a few merge conflicts

I can handle it if you don't have time but it will be after the PRs I'm currently working on

Just updated to main. Yup, definitely some conflicts! I don't have time at the moment to resolve, I will check back in here if I free up and can start to tackle them myself so we don't duplicate efforts.

[ephys]

We are refactoring DataTypes in #14505 and, to reduce special-casing, the ENUM DataType is now responsible for generating its SQL name.
This should make adding this option much easier.

[iwaduarte]

I know you have a lot on your plate right now. What would we need to have that merged in v5 and v6? This is a 3-year-old pull request and although refactoring DataTypes would help, shouldn't we resolve this first?

[ephys]

Sequelize 5 has been EOL for almost a year now.
We're not opposed to merging this in v6 but as you said, this PR is 3 years old. We have more active maintainers now but there are so many conflicts in this PR we may as well start over.

I personally focus more on fixing the foundations, but I'll review an updated PR that does this in v6 (but not v7 until #14505 has been reviewed and merged)