Initial modifications to get enums working with postgres schemas #4796
Conversation
| var schema = tableName.schema || options.schema || 'public'; | ||
| var delimiter = tableName.delimiter || options.delimiter || '.'; | ||
|
|
||
| var sql = 'CREATE TYPE ' + this.quoteIdentifier(schema) + delimiter + enumName + ' AS ' + values + ';'; |
There was a problem hiding this comment.
Why this change - the name creation is intentionally moved to a function, which handles schema? Isn't the end output the same?
There was a problem hiding this comment.
It was causing this portion (L746) to fail:
var query = 'SELECT t.typname enum_name, array_agg(e.enumlabel) enum_value FROM pg_type t ' +
'JOIN pg_enum e ON t.oid = e.enumtypid ' +
'JOIN pg_catalog.pg_namespace n ON n.oid = t.typnamespace ' +
"WHERE n.nspname = '" + schema + "'" + enumName + ' GROUP BY 1';Example:
WHERE n.nspname = "public" "public"."enumNameHere" GROUP BY 1;There was a problem hiding this comment.
Then the change should be the other way I think - so that enumName is generated manually here. pgEnumName is used name places, otherwise we'd have to duplicate the logic for adding schema in front of enum name to all those functiond
There was a problem hiding this comment.
I was just typing the same thing 💃
Too many late late nights in a row
|
@janmeier another go |
|
@janmeier ready for your scrutiny |
|
@janmeier bump |
|
One failing test, otherwise LGTM. If you could fix the test (just add a check for options), add a changelog entry and squash to a single descriptive commit this is good to go |
|
Is it expected that pgListEnums would get called with no tableName? |
|
Yes, it can also be used to list all existing enums, not just those in a single table |
|
Please let me know if this is suitable |
|
Bump |
|
Bumping again 💃 |
|
@mickhansen if @janmeier is super busy, would you please be able to approve this? |
| enumName = '"' + tableName.schema + '"' + tableName.delimiter + enumName; | ||
| if (options.schema !== false && (tableName.schema || options.schema)) { | ||
| var schema = tableName.schema || options.schema || 'public'; | ||
| var delimiter = tableName.delimiter || options.delimiter || '.'; |
There was a problem hiding this comment.
Please use pre-comma per the general style,
var schema = ...
, delimiter = ...Goes for everywhere else aswell :)
There was a problem hiding this comment.
This code handles having schema/delimter be part of the table name and be part of the options.
I imagine we'll need this multiple places since a lot of queryInterface (migration) calls should support both, ideally we could provide some helper that takes a tableName and an options and returns schema, delimiter and tableName.
|
Linter errors! |
|
Never ending pull request |
|
Well a |
|
Yep my bad :D ill have to deal with it in 12 hours it's sleep time |
|
@mickhansen updated |
|
postgres breaks: https://travis-ci.org/sequelize/sequelize/jobs/90589723 |
|
😄 🌴 |
|
Huh, they pass on my machine. Looking into it |
|
Turns out I was running dramatic pause the wrong tests |
|
https://travis-ci.org/sequelize/sequelize/jobs/90688045 |
|
Oh look, I did fix it! (went to bed after last commit) |
|
Ahem, I mean, it's ready to merge. |
Initial modifications to get enums working with postgres schemas
|
Thank you for the good work and the patience @faceleg :) |
|
Pretty sure you were the patient one haha. Thanks for bearing with me. |
First off, thanks for all the hard work, I'm loving Sequelize.
I am using Postgres and want to use a non-standard schema for some models. I use ENUM for my model
statusfields, which has lead to problems for me when combined with the non-standard schema.This PR aims to make Sequelize work perfectly with non standard Postgres schemas.
Please can you glance through the commit and let me know if you're happy with the direction I'm heading. Once I have it working I'll look at extracting the duplication in to a helper function or two.