From d58b456d15f78b1a3723eb62ab1e49d303b0ab55 Mon Sep 17 00:00:00 2001 From: Scott Gress Date: Mon, 22 May 2017 15:29:47 -0500 Subject: [PATCH 1/4] Log a warning when `type: 'number'` is used with certain large numeric column types. Refs https://github.com/balderdashy/waterline/issues/1487 --- helpers/register-data-store.js | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/helpers/register-data-store.js b/helpers/register-data-store.js index 4ae8b0f..c983f56 100644 --- a/helpers/register-data-store.js +++ b/helpers/register-data-store.js @@ -104,16 +104,29 @@ module.exports = require('machine').build({ return exits.badConfiguration(new Error('Connection config is missing a database value.')); } - // Loop through every model assigned to the datastore we're registering, - // and ensure that each one's primary key is either required or auto-incrementing. + // Loop through every model assigned to the datastore we're registering. try { - _.each(inputs.models, function checkPrimaryKey(modelDef, modelIdentity) { - var primaryKeyAttr = modelDef.definition[modelDef.primaryKey]; + _.each(inputs.models, function checkModel(modelDef, modelIdentity) { - // Ensure that the model's primary key has either `autoIncrement` or `required` + // Ensure that each the primary key is either required or auto-incrementing. + var primaryKeyAttr = modelDef.definition[modelDef.primaryKey]; if (primaryKeyAttr.required !== true && (!primaryKeyAttr.autoMigrations || primaryKeyAttr.autoMigrations.autoIncrement !== true)) { throw new Error('In model `' + modelIdentity + '`, primary key `' + modelDef.primaryKey + '` must have either `required` or `autoIncrement` set.'); } + + // Loop through each attribute in the model. + _.each(modelDef.definition, function(attribute, attributeName) { + // If the column type is 'bigint', 'decimal', 'numeric' or 'bigserial', + // and the attribute type is not 'string', log a warning. The Postgresql + // driver will always return data from these columns as strings because + // they may be too big to fit in a JavaScript integer. + if (_.contains(['bigint', 'decimal', 'numeric', 'bigserial'], attribute.autoMigrations.columnType) && attribute.type !== 'string') { + console.log('In attribute `' + attributeName + '` of model `' + modelIdentity + '`:'); + console.log('When `columnType` is set to `' + attribute.autoMigrations.columnType + '`, `type` should be set to `string` in order to avoid a loss in precision.'); + console.log(); + } + }); + }); } catch (e) { return exits.badConfiguration(e); From 128f36534b183ca2b4ef74b8ce19b6d98d52cb75 Mon Sep 17 00:00:00 2001 From: Scott Gress Date: Tue, 23 May 2017 17:22:34 -0500 Subject: [PATCH 2/4] Skip warning about BIGINT for timestamps and attributes w/out autoMigrations set --- helpers/register-data-store.js | 1 + 1 file changed, 1 insertion(+) diff --git a/helpers/register-data-store.js b/helpers/register-data-store.js index c983f56..4876ce6 100644 --- a/helpers/register-data-store.js +++ b/helpers/register-data-store.js @@ -120,6 +120,7 @@ module.exports = require('machine').build({ // and the attribute type is not 'string', log a warning. The Postgresql // driver will always return data from these columns as strings because // they may be too big to fit in a JavaScript integer. + if (!attribute.autoMigrations || attribute.autoCreatedAt || attribute.autoUpdatedAt) { return; } if (_.contains(['bigint', 'decimal', 'numeric', 'bigserial'], attribute.autoMigrations.columnType) && attribute.type !== 'string') { console.log('In attribute `' + attributeName + '` of model `' + modelIdentity + '`:'); console.log('When `columnType` is set to `' + attribute.autoMigrations.columnType + '`, `type` should be set to `string` in order to avoid a loss in precision.'); From 4fb607c36dd4726f1f85f47dfa5da06f22743376 Mon Sep 17 00:00:00 2001 From: Scott Gress Date: Tue, 23 May 2017 17:22:56 -0500 Subject: [PATCH 3/4] Update comment about transforming timestamps to numbers --- helpers/private/query/process-each-record.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/helpers/private/query/process-each-record.js b/helpers/private/query/process-each-record.js index e55496c..4a77867 100644 --- a/helpers/private/query/process-each-record.js +++ b/helpers/private/query/process-each-record.js @@ -47,9 +47,10 @@ module.exports = function processEachRecord(options) { // Run all the records through the iterator so that they can be normalized. eachRecordDeep(options.records, function iterator(record, WLModel) { - // Check if the record and the model contain auto timestamps and make - // sure that if they are type number that they are actually numbers and - // not strings. + // We're forced to store JS timestamps in BIGINT columns b/c they are too big for + // regular INT columns, but BIGINT can potentially be larger than the JS + // max int so Postgresql automatically transforms it to a string. If the user + // declared them with `type: 'number'`, we'll transform them back to numbers here. _.each(WLModel.definition, function checkAttributes(attrVal) { var columnName = attrVal.columnName; From 4bdb5a32cbe090551a571bcb09c3f1f706e6b0ca Mon Sep 17 00:00:00 2001 From: Scott Gress Date: Tue, 23 May 2017 23:09:01 -0500 Subject: [PATCH 4/4] Update comments and re-order logic. --- helpers/register-data-store.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/helpers/register-data-store.js b/helpers/register-data-store.js index 4876ce6..b576aff 100644 --- a/helpers/register-data-store.js +++ b/helpers/register-data-store.js @@ -108,7 +108,7 @@ module.exports = require('machine').build({ try { _.each(inputs.models, function checkModel(modelDef, modelIdentity) { - // Ensure that each the primary key is either required or auto-incrementing. + // Ensure that the primary key is either required or auto-incrementing. var primaryKeyAttr = modelDef.definition[modelDef.primaryKey]; if (primaryKeyAttr.required !== true && (!primaryKeyAttr.autoMigrations || primaryKeyAttr.autoMigrations.autoIncrement !== true)) { throw new Error('In model `' + modelIdentity + '`, primary key `' + modelDef.primaryKey + '` must have either `required` or `autoIncrement` set.'); @@ -120,8 +120,11 @@ module.exports = require('machine').build({ // and the attribute type is not 'string', log a warning. The Postgresql // driver will always return data from these columns as strings because // they may be too big to fit in a JavaScript integer. - if (!attribute.autoMigrations || attribute.autoCreatedAt || attribute.autoUpdatedAt) { return; } - if (_.contains(['bigint', 'decimal', 'numeric', 'bigserial'], attribute.autoMigrations.columnType) && attribute.type !== 'string') { + // + // Skip this check for `autoCreatedAt` and `autoUpdatedAt` attributes, which are transformed + // into numbers internally if the `type` is 'number'. + if (attribute.autoCreatedAt || attribute.autoUpdatedAt) { return; } + if (attribute.autoMigrations && _.contains(['bigint', 'decimal', 'numeric', 'bigserial'], attribute.autoMigrations.columnType) && attribute.type !== 'string') { console.log('In attribute `' + attributeName + '` of model `' + modelIdentity + '`:'); console.log('When `columnType` is set to `' + attribute.autoMigrations.columnType + '`, `type` should be set to `string` in order to avoid a loss in precision.'); console.log();