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
Add support for postgres citext data-type #10024
Add support for postgres citext data-type #10024
Conversation
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.
Looks good. I have updated docker images for CI
lib/dialects/abstract/index.js
Outdated
@@ -11,6 +11,7 @@ AbstractDialect.prototype.supports = { | |||
'ORDER NULLS': false, | |||
'UNION': true, | |||
'UNION ALL': true, | |||
'CITEXT': false, |
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.
No need for this in supports
, these flags are meant to switch code segments when generating queries or declaring support information that is used anywhere in lib/
, for tests you can just compare dialect === 'postgres'
lib/dialects/postgres/data-types.js
Outdated
@@ -168,6 +168,20 @@ module.exports = BaseTypes => { | |||
array_oids: [1009] | |||
}; | |||
|
|||
function CITEXT() { |
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.
As this implementation is not changed, defining function CITEXT
is not required. You just need
BaseTypes.CITEXT.types.postgres = {
oids: [146932],
array_oids: [146937]
};
There is some issue with postgres-9.5 image, I'll fix that in evening (approx 7-8 hours). |
Codecov Report
@@ Coverage Diff @@
## master #10024 +/- ##
==========================================
- Coverage 96.38% 96.36% -0.03%
==========================================
Files 63 63
Lines 9386 9399 +13
==========================================
+ Hits 9047 9057 +10
- Misses 339 342 +3
Continue to review full report at Codecov.
|
Now all images has been fixed, please address my code related comments and this can be merged |
Removed uneeded CITEXT in supports and changed tests to use `dialect` instead.
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 a standard datatypes test here
docs/models-definition.md
Outdated
@@ -120,6 +120,7 @@ Sequelize.STRING(1234) // VARCHAR(1234) | |||
Sequelize.STRING.BINARY // VARCHAR BINARY | |||
Sequelize.TEXT // TEXT | |||
Sequelize.TEXT('tiny') // TINYTEXT | |||
Sequelize.CITEXT() // CITEXT PostgreSQL only. |
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.
Sequelize.CITEXT
not Sequelize.CITEXT()
Discovered that CITEXT has dynamic OIDs and changed to handle that. |
@@ -221,7 +222,8 @@ class ConnectionManager extends AbstractConnectionManager { | |||
dataTypes.postgres.GEOMETRY, | |||
dataTypes.postgres.HSTORE, | |||
dataTypes.postgres.GEOGRAPHY, | |||
dataTypes.postgres.ENUM | |||
dataTypes.postgres.ENUM, | |||
dataTypes.CITEXT |
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.
dataTypes.postgres.CITEXT
is not available, right?
As we need to dynamically update OIDs it is better to define this type in postgres/data-types.js
function CITEXT(options) {
if (!(this instanceof CITEXT)) return new CITEXT(options);
BaseTypes.CITEXT.apply(this, arguments);
}
inherits(CITEXT, BaseTypes.CITEXT);
BaseTypes.CITEXT.types.postgres = {
oids: [],
array_oids: []
};
and have it exported. So you can use dataTypes.postgres.CITEXT
here and in code just below
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.
Perhaps it is bit much to redeclare in the Postgres data-types? If you are concerned about how it looks, this would be cleaner:
// Reset OID mapping for dynamic type
[
dataTypes.GEOMETRY,
dataTypes.HSTORE,
dataTypes.GEOGRAPHY,
dataTypes.ENUM,
dataTypes.CITEXT
].forEach(type => {
type.types.postgres.oids = [];
type.types.postgres.array_oids = [];
});
for (const row of result.rows) {
let type;
if (row.typname === 'geometry') {
type = dataTypes.GEOMETRY;
} else if (row.typname === 'hstore') {
type = dataTypes.HSTORE;
} else if (row.typname === 'geography') {
type = dataTypes.GEOGRAPHY;
} else if (row.typname === 'citext') {
type = dataTypes.CITEXT;
} else if (row.typtype === 'e') {
type = dataTypes.ENUM;
}
Thanks @justinkalland |
Pull Request check-list
Please make sure to review and check all of these items:
npm run test
ornpm run test-DIALECT
pass with this change (including linting)?Description of change
Adds support for CITEXT data-type. This is a Postgres only data-type. This also adds support for ARRAY(CITEXT).
To use CITEXT it must first be enabled in Postgres:
The new tests will fail unless the module is enabled in Postgres. I have created as change to enable the module in the custom Docker image: sushantdhiman/sequelize-postgres#1
Closes #9116
Closes #6572