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

Initial modifications to get enums working with postgres schemas #4796

Merged
merged 3 commits into from Nov 13, 2015

Conversation

3 participants
@faceleg
Contributor

faceleg commented Nov 3, 2015

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 status fields, 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.

var schema = tableName.schema || options.schema || 'public';
var delimiter = tableName.delimiter || options.delimiter || '.';

var sql = 'CREATE TYPE ' + this.quoteIdentifier(schema) + delimiter + enumName + ' AS ' + values + ';';

This comment has been minimized.

@janmeier

janmeier Nov 3, 2015

Member

Why this change - the name creation is intentionally moved to a function, which handles schema? Isn't the end output the same?

This comment has been minimized.

@faceleg

faceleg Nov 3, 2015

Contributor

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;

This comment has been minimized.

@janmeier

janmeier Nov 3, 2015

Member

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

This comment has been minimized.

@faceleg

faceleg Nov 3, 2015

Contributor

I was just typing the same thing 💃

Too many late late nights in a row

@faceleg faceleg closed this Nov 3, 2015

@faceleg faceleg reopened this Nov 3, 2015

@faceleg

This comment has been minimized.

Contributor

faceleg commented Nov 3, 2015

@janmeier another go

@faceleg

This comment has been minimized.

Contributor

faceleg commented Nov 3, 2015

@janmeier ready for your scrutiny

@faceleg

This comment has been minimized.

Contributor

faceleg commented Nov 5, 2015

@janmeier bump

@janmeier

This comment has been minimized.

Member

janmeier commented Nov 5, 2015

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

@janmeier janmeier self-assigned this Nov 5, 2015

@faceleg

This comment has been minimized.

Contributor

faceleg commented Nov 5, 2015

Is it expected that pgListEnums would get called with no tableName?

@janmeier

This comment has been minimized.

Member

janmeier commented Nov 5, 2015

Yes, it can also be used to list all existing enums, not just those in a single table

@faceleg

This comment has been minimized.

Contributor

faceleg commented Nov 5, 2015

Please let me know if this is suitable

@faceleg

This comment has been minimized.

Contributor

faceleg commented Nov 6, 2015

Bump

@faceleg

This comment has been minimized.

Contributor

faceleg commented Nov 8, 2015

Bumping again 💃

@faceleg

This comment has been minimized.

Contributor

faceleg commented Nov 8, 2015

@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 || '.';

This comment has been minimized.

@mickhansen

mickhansen Nov 10, 2015

Contributor

Please use pre-comma per the general style,

var schema = ...
  , delimiter = ...

Goes for everywhere else aswell :)

This comment has been minimized.

@mickhansen

mickhansen Nov 10, 2015

Contributor

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.

expect(sql.pgEnumName(PublicUser.getTableName(), 'mood', { schema: false }))
.to.equal('"enum_users_mood"');
expect(sql.pgEnumName(FooUser.getTableName(), 'theirMood', { schema: false }))
.to.equal('"enum_users_theirMood"');

This comment has been minimized.

@mickhansen

mickhansen Nov 10, 2015

Contributor

When would a user specifically use schema: false? Would it not normally just be unset?

expectsql(sql.pgListEnums(), {
postgres: '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 = \'public\' GROUP BY 1'
postgres: '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 = \'public\' GROUP BY 1'
});

This comment has been minimized.

@mickhansen

mickhansen Nov 10, 2015

Contributor

I see you fixed the indentation here, looks like it was expectsql that needed to be moved back tho :)

This comment has been minimized.

@faceleg

faceleg Nov 10, 2015

Contributor

Fixed

@@ -119,6 +119,7 @@ ConnectionManager.prototype.disconnect = function(connection) {
return new Promise(function (resolve, reject) {
connection.end();
resolve();
return null;

This comment has been minimized.

@mickhansen

mickhansen Nov 10, 2015

Contributor

Any specific reason for this?

This comment has been minimized.

@mickhansen

mickhansen Nov 10, 2015

Contributor

Ah, makes sense for then, sure it's required for new Promise?

This comment has been minimized.

@faceleg

faceleg Nov 10, 2015

Contributor

I have no idea - I'm getting this warning for this line in my CI tests, this was my attempt to remove the warning (didn't work).

@@ -2453,6 +2453,7 @@ Model.prototype.update = function(values, options) {
}
});
}
return null;

This comment has been minimized.

@mickhansen

mickhansen Nov 10, 2015

Contributor

same here, is it just for specificity?

var schema = 'public';
if (tableName) {
schema = tableName.schema || options.schema || 'public';
}

This comment has been minimized.

@mickhansen

mickhansen Nov 10, 2015

Contributor

Would tableName not always be set? But it might not always be an object.

};

return tableDetails;
},

This comment has been minimized.

@mickhansen

mickhansen Nov 10, 2015

Contributor

Should work for mssql aswell, might want to move the method to abstractQueryGenerator

This comment has been minimized.

@mickhansen

mickhansen Nov 10, 2015

Contributor

No real need for the intermediary variable either (just return the object).

This comment has been minimized.

@faceleg

faceleg Nov 10, 2015

Contributor

Done. When you've approved this PR in principle I will write some tests for extractTableDetails

@@ -722,34 +722,36 @@ var QueryGenerator = {

pgEnumName: function (tableName, attr, options) {
options = options || {};
var tableDetails = this.extractTableDetails(tableName, options || {})

This comment has been minimized.

@mickhansen

mickhansen Nov 10, 2015

Contributor

options has already been defaulted, no need to do it again

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Nov 11, 2015

Linter errors!

@faceleg

This comment has been minimized.

Contributor

faceleg commented Nov 11, 2015

Never ending pull request

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Nov 11, 2015

Well a make jshint woulda helped you on that one ;)

@faceleg

This comment has been minimized.

Contributor

faceleg commented Nov 11, 2015

Yep my bad :D ill have to deal with it in 12 hours it's sleep time

@faceleg

This comment has been minimized.

Contributor

faceleg commented Nov 11, 2015

@mickhansen updated

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Nov 12, 2015

@faceleg

This comment has been minimized.

Contributor

faceleg commented Nov 12, 2015

😄 🌴

@faceleg

This comment has been minimized.

Contributor

faceleg commented Nov 12, 2015

Huh, they pass on my machine. Looking into it

@faceleg

This comment has been minimized.

Contributor

faceleg commented Nov 12, 2015

Turns out I was running dramatic pause the wrong tests

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Nov 12, 2015

https://travis-ci.org/sequelize/sequelize/jobs/90688045
Just a single expectation that's off.

@faceleg

This comment has been minimized.

Contributor

faceleg commented Nov 12, 2015

Oh look, I did fix it! (went to bed after last commit)

@faceleg

This comment has been minimized.

Contributor

faceleg commented Nov 12, 2015

Ahem, I mean, it's ready to merge.

mickhansen added a commit that referenced this pull request Nov 13, 2015

Merge pull request #4796 from faceleg/feature/postgres-schema
Initial modifications to get enums working with postgres schemas

@mickhansen mickhansen merged commit 9775f36 into sequelize:master Nov 13, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Nov 13, 2015

Thank you for the good work and the patience @faceleg :)

@faceleg

This comment has been minimized.

Contributor

faceleg commented Nov 13, 2015

Pretty sure you were the patient one haha.

Thanks for bearing with me.

@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