balderdashy / waterline-sequel

A SQL generator for use in Waterline Adapters
MIT License
16 stars 61 forks source link

broken atribute columnName #88

Open masitko opened 8 years ago

masitko commented 8 years ago

After updating to version 0.6.2 (from 0.5.x, using sails-postgresql adapter) there is an error when lifting sails:

error: error: A hook (`orm`) failed to load!
error: error: Error (E_UNKNOWN) :: Encountered an unexpected error
error: column author.createdUser does not exist
    at Connection.parseE (/var/www/html/ccentre/backend/node_modules/sails-postgresql/node_modules/pg/lib/connection.js:539:11)
    at Connection.parseMessage (/var/www/html/ccentre/backend/node_modules/sails-postgresql/node_modules/pg/lib/connection.js:366:17)
    at Socket.<anonymous> (/var/www/html/ccentre/backend/node_modules/sails-postgresql/node_modules/pg/lib/connection.js:105:22)
    at emitOne (events.js:90:13)
    at Socket.emit (events.js:182:7)
    at readableAddChunk (_stream_readable.js:147:16)
    at Socket.Readable.push (_stream_readable.js:111:10)
    at TCP.onread (net.js:523:20)

the error seems to be caused by problem in select.js around line 90:

attributes.forEach(function(key) {
    // Default schema to {} in case a raw DB column name is sent.  This shouldn't happen
    // after https://github.com/balderdashy/waterline/commit/687c869ad54f499018ab0b038d3de4435c96d1dd
    // but leaving here as a failsafe.
    var schema = self.schema[self.currentTable].definition[key] || {};
//    console.log(schema);
    if(!schema) return;
    if(hop(schema, 'collection')) return;
    selectKeys.push({ table: self.currentTable, key: schema.columnName || key });
  });
sailsbot commented 8 years ago

@masitko Thanks for posting, we'll take a look as soon as possible. In the meantime, if you haven’t already, please carefully read the issue contribution guidelines and double-check for any missing information above. In particular, please ensure that this issue is about a stability or performance bug with a documented feature; and make sure you’ve included detailed instructions on how to reproduce the bug from a clean install. Finally, don’t forget to include the version of Node.js you tested with, as well as your version of Sails or Waterline, and of any relevant standalone adapters/generators/hooks.

Thank you!

masitko commented 8 years ago

To fix the error line 92 of file https://github.com/balderdashy/waterline-sequel/blob/master/sequel/select.js needs to be changed from

    var schema = self.schema[self.currentTable].definition[key] || {};

to

    var schema = self.schema[self.currentTable].attributes[key] || {};
masitko commented 8 years ago

Pull request: https://github.com/balderdashy/waterline-sequel/pull/75/commits/9b6d19958a7905f5b4a44e606f72f66280d9f772

particlebanana commented 8 years ago

@masitko it looks like it's actually a combination of a few things. I submitted 2 PR's that should solve this problem I'd like you to test before I go ahead and publish them.

First was an issue in Waterline where the auto-migrations were not normalizing the select values to use column names when lifting. The other issue was my completely horrible code in this repo.

Both of those PR's are merged so let me know what you see. I ran through some manual tests and they pass all the adapter tests.

masitko commented 8 years ago

@particlebanana it wasn't fixed I'm afraid. Still

error: error: A hook (`orm`) failed to load!
error: error: Error (E_UNKNOWN) :: Encountered an unexpected error
error: column userlogin.user does not exist
    at Connection.parseE (/var/www/html/ccentre/backend/node_modules/pg/lib/connection.js:539:11)
    at Connection.parseMessage (/var/www/html/ccentre/backend/node_modules/pg/lib/connection.js:366:17)
    at Socket.<anonymous> (/var/www/html/ccentre/backend/node_modules/pg/lib/connection.js:105:22)
    at emitOne (events.js:90:13)
    at Socket.emit (events.js:182:7)
    at readableAddChunk (_stream_readable.js:147:16)
    at Socket.Readable.push (_stream_readable.js:111:10)
    at TCP.onread (net.js:523:20)

when trying to lift Sails.

Please have a look at file select.js around line 90:

 var attributes = queryObject.select || Object.keys(this.schema[this.currentTable].definition);
  delete queryObject.select;

attributes.forEach(function(key) {
    // Default schema to {} in case a raw DB column name is sent.  This shouldn't happen
    // after https://github.com/balderdashy/waterline/commit/687c869ad54f499018ab0b038d3de4435c96d1dd
    // but leaving here as a failsafe.
    var schema = self.schema[self.currentTable].definition[key] || {};
    if(!schema) return;
    if(hop(schema, 'collection')) return;
    selectKeys.push({ table: self.currentTable, key: schema.columnName || key });
  });

and what self.schema[self.currentTable] contains:

{ attributes: 
   { ip: { type: 'string', required: true },
     host: { type: 'string', required: true },
     agent: { type: 'text', required: true },
     browser: { type: 'string', defaultsTo: 'Unknown' },
     browserVersion: { type: 'string', defaultsTo: 'Unknown' },
     browserFamily: { type: 'string', defaultsTo: 'Unknown' },
     os: { type: 'string', defaultsTo: 'Unknown' },
     osVersion: { type: 'string', defaultsTo: 'Unknown' },
     osFamily: { type: 'string', defaultsTo: 'Unknown' },
     count: { type: 'integer', defaultsTo: 1 },
     id: 
      { type: 'integer',
        autoIncrement: true,
        primaryKey: true,
        unique: true },
     createdAt: { type: 'datetime', default: 'NOW' },
     updatedAt: { type: 'datetime', default: 'NOW' },
     user: 
      { columnName: 'userId',
        type: 'integer',
        foreignKey: true,
        references: 'user',
        on: 'id',
        onKey: 'id' } },
  definition: 
   { ip: { type: 'string' },
     host: { type: 'string' },
     agent: { type: 'text' },
     browser: { type: 'string', defaultsTo: 'Unknown' },
     browserVersion: { type: 'string', defaultsTo: 'Unknown' },
     browserFamily: { type: 'string', defaultsTo: 'Unknown' },
     os: { type: 'string', defaultsTo: 'Unknown' },
     osVersion: { type: 'string', defaultsTo: 'Unknown' },
     osFamily: { type: 'string', defaultsTo: 'Unknown' },
     count: { type: 'integer', defaultsTo: 1 },
     id: 
      { type: 'integer',
        autoIncrement: true,
        primaryKey: true,
        unique: true },
     createdAt: { type: 'datetime' },
     updatedAt: { type: 'datetime' },
     userId: 
      { type: 'integer',
        model: 'user',
        foreignKey: true,
        alias: 'user' } },
  meta: { junctionTable: false },
  tableName: 'userlogin',
  connection: [ 'postgresql' ] }

As we can see definition key for column user is already updated to userId. Code mentioned above iterates through schema keys and extracts values from definition using the same (wrong in this case) keys

var schema = self.schema[self.currentTable].definition[key] || {};

When columnName is set schema = {}

masitko commented 8 years ago

@particlebanana it does lift after updating waterline to the latest master. There is lots of errors during migration but it works with a clean database. Will have a look at it tomorrow.

masitko commented 8 years ago

@particlebanana it lifted once with clean database but after loading fixtures I got some validation errors:

error: error: A hook (`orm`) failed to load!
error: error: Error (E_VALIDATION) :: 1 attribute is invalid
    at WLValidationError.WLError (/var/www/html/ccentre/backend/node_modules/sails-hook-orm/node_modules/waterline/lib/waterline/error/WLError.js:25:15)
    at new WLValidationError (/var/www/html/ccentre/backend/node_modules/sails-hook-orm/node_modules/waterline/lib/waterline/error/WLValidationError.js:19:28)
    at _afterValidating (/var/www/html/ccentre/backend/node_modules/sails-hook-orm/node_modules/waterline/lib/waterline/query/validate.js:53:23)
    at allValidationsChecked (/var/www/html/ccentre/backend/node_modules/sails-hook-orm/node_modules/waterline/lib/waterline/core/validations.js:288:12)
    at /var/www/html/ccentre/backend/node_modules/async/lib/async.js:52:16
    at Object.async.forEachOf.async.eachOf (/var/www/html/ccentre/backend/node_modules/async/lib/async.js:236:30)
    at Object.async.forEach.async.each (/var/www/html/ccentre/backend/node_modules/async/lib/async.js:209:22)
    at Validator.validate (/var/www/html/ccentre/backend/node_modules/sails-hook-orm/node_modules/waterline/lib/waterline/core/validations.js:167:9)
    at /var/www/html/ccentre/backend/node_modules/sails-hook-orm/node_modules/waterline/lib/waterline/query/validate.js:42:25
    at /var/www/html/ccentre/backend/node_modules/async/lib/async.js:718:13
    at Immediate.iterate [as _onImmediate] (/var/www/html/ccentre/backend/node_modules/async/lib/async.js:262:13)
    at processImmediate [as _immediateCallback] (timers.js:383:17)

Invalid attributes sent to Passport:
 • protocol
   • `undefined` should be a alphanumeric (instead of "null", which is a object)
   • "required" validation rule failed for input: null
Specifically, it threw an error.  Details:
 undefined

This seems to be an old problem, I'm not sure if this i's related to my models definition/fixtures or changes in waterline 0.11.x -> 0.12.x latest master.

particlebanana commented 8 years ago

var schema = self.schema[self.currentTable].definition[key] || {};

That is the expected behavior. By the time it gets to there the select statement should contain the normalized column name values. The patch in Waterline fixes this where the auto migrations were not running the same code as normal .find/.create. The last validation issue you posted may have something to do with this, now the migrations run through the .create(). I'm wondering if this is the correct behavior or not.

masitko commented 8 years ago

I'm not sure if this works correctly, it may be related to my broken/out of date dependencies but:

I can lift sails only first time with clean database and fixtures are being loaded OK. There is lots of errors

error: error: Must specify an `id` when calling `Model.room(id)`

which I never seen before and relations between models seem to be broken.

After that every attempt to lift ends with an error and waterline tries to migrate my database despite I didn't change anything:

Waterline encountered a fatal error when trying to perform the `alter` auto-migration strategy.
In a couple of seconds, the data (cached in memory) will be logged to stdout.
(a failsafe put in place to preserve development data)

In the mean time, here's the error:

Error (E_VALIDATION) :: 2 attributes are invalid
    at WLValidationError.WLError (/var/www/html/ccentre/backend/node_modules/sails-hook-orm/node_modules/waterline/lib/waterline/error/WLError.js:25:15)
    at new WLValidationError (/var/www/html/ccentre/backend/node_modules/sails-hook-orm/node_modules/waterline/lib/waterline/error/WLValidationError.js:19:28)
    at _afterValidating (/var/www/html/ccentre/backend/node_modules/sails-hook-orm/node_modules/waterline/lib/waterline/query/validate.js:53:23)
    at allValidationsChecked (/var/www/html/ccentre/backend/node_modules/sails-hook-orm/node_modules/waterline/lib/waterline/core/validations.js:288:12)
    at /var/www/html/ccentre/backend/node_modules/async/lib/async.js:52:16
    at Object.async.forEachOf.async.eachOf (/var/www/html/ccentre/backend/node_modules/async/lib/async.js:236:30)
    at Object.async.forEach.async.each (/var/www/html/ccentre/backend/node_modules/async/lib/async.js:209:22)
    at Validator.validate (/var/www/html/ccentre/backend/node_modules/sails-hook-orm/node_modules/waterline/lib/waterline/core/validations.js:167:9)
    at /var/www/html/ccentre/backend/node_modules/sails-hook-orm/node_modules/waterline/lib/waterline/query/validate.js:42:25
    at /var/www/html/ccentre/backend/node_modules/async/lib/async.js:718:13
    at Immediate.iterate [as _onImmediate] (/var/www/html/ccentre/backend/node_modules/async/lib/async.js:262:13)
    at processImmediate [as _immediateCallback] (timers.js:383:17)

Invalid attributes sent to Book:
 • title
   • `undefined` should be a string (instead of "null", which is a object)
   • "required" validation rule failed for input: null
Specifically, it threw an error.  Details:
 undefined
 • releaseDate
   • `undefined` should be a date (instead of "null", which is a object)
   • "required" validation rule failed for input: null
Specifically, it threw an error.  Details:
 undefined

================================
Data backup:
================================

[ { id: 1 },
  { id: 2 },
  { id: 3 },
  { id: 4 },
  { id: 5 },
  { id: 6 },
  { id: 7 },
  { id: 8 },
  { id: 9 },
  { id: 10 },
  { id: 11 },
  { id: 12 },
  { id: 13 },
  { id: 14 },
  { id: 15 },
  { id: 17 },
  { id: 16 },
  { id: 26 },
  { id: 36 },
  { id: 46 },
  { id: 18 },
  { id: 28 },
  { id: 38 },
  { id: 48 },
  { id: 19 },
  { id: 29 },
  { id: 42 },
  { id: 20 },
  { id: 30 },
  { id: 43 },
  { id: 21 },
  { id: 31 },
  { id: 39 },
  { id: 22 },
  { id: 32 },
  { id: 40 },
  { id: 23 },
  { id: 33 },
  { id: 41 },
  { id: 24 },
  { id: 34 },
  { id: 44 },
  { id: 25 },
  { id: 35 },
  { id: 45 },
  { id: 27 },
  { id: 37 },
  { id: 47 } ]
error: error: A hook (`orm`) failed to load!
error: Error (E_VALIDATION) :: 2 attributes are invalid
    at WLValidationError.WLError (/var/www/html/ccentre/backend/node_modules/sails-hook-orm/node_modules/waterline/lib/waterline/error/WLError.js:25:15)
    at new WLValidationError (/var/www/html/ccentre/backend/node_modules/sails-hook-orm/node_modules/waterline/lib/waterline/error/WLValidationError.js:19:28)
    at _afterValidating (/var/www/html/ccentre/backend/node_modules/sails-hook-orm/node_modules/waterline/lib/waterline/query/validate.js:53:23)
    at allValidationsChecked (/var/www/html/ccentre/backend/node_modules/sails-hook-orm/node_modules/waterline/lib/waterline/core/validations.js:288:12)
    at /var/www/html/ccentre/backend/node_modules/async/lib/async.js:52:16
    at Object.async.forEachOf.async.eachOf (/var/www/html/ccentre/backend/node_modules/async/lib/async.js:236:30)
    at Object.async.forEach.async.each (/var/www/html/ccentre/backend/node_modules/async/lib/async.js:209:22)
    at Validator.validate (/var/www/html/ccentre/backend/node_modules/sails-hook-orm/node_modules/waterline/lib/waterline/core/validations.js:167:9)
    at /var/www/html/ccentre/backend/node_modules/sails-hook-orm/node_modules/waterline/lib/waterline/query/validate.js:42:25
    at /var/www/html/ccentre/backend/node_modules/async/lib/async.js:718:13
    at Immediate.iterate [as _onImmediate] (/var/www/html/ccentre/backend/node_modules/async/lib/async.js:262:13)
    at processImmediate [as _immediateCallback] (timers.js:383:17)

Let me know if I can help in any way.

mr47 commented 8 years ago

@masitko @particlebanana Guys after hours of debug, i found that magic wrong goes here with 99% not in waterline-shema or waterline itself. First of all queryObject.select always undefined, but it seems its should pass to queryObject.select. So when using columnName it don't selects field and when comes time to processChildren we gen someting like this

^^^^^^^^^^^^^^^^^^^^MySQL.processChildren.groupedRecords^^^^^^^^^^^^^^
{ undefined:
   [ RowDataPacket {
       id: 1,
       comment: '2',
       createdAt: Mon Apr 18 2016 23:41:20 GMT+0300 (FLE Daylight Time),
       updatedAt: Mon Apr 18 2016 23:41:20 GMT+0300 (FLE Daylight Time) } ] }
^^^^^^^^^^^^^^^^^^^^groupedRecords^^^^^^^^^^^^^^

So we see that it should get an field place_id but when build sql its don't select one more field that used to map result to an collection. And more raw logs with some internal names:

MySQL.populateBuffers:
  SELECT `places`.`id`, `places`.`rawData`, `places`.`createdAt`, `places`.`updatedAt` FROM `places` AS `places`   LIMIT 30 OFFSET 0

MySQL.processChildren:
 (SELECT `placecomments`.`id`,`placecomments`.`comment`,`placecomments`.`createdAt`,`placecomments`.`updatedAt` FROM `placecomments` AS `placecomments` WHERE `place_id` = 148864955158827  ORDER BY `placecomments`.`id` ASC LIMIT 30) UNION ALL (SELECT `placecomments`.`id`,`placecomments`.`comment`,`placecomments`.`createdAt`,`placecomments`.`updatedAt` FROM `placecomments` AS `placecomments` WHERE `place_id` = 148864955158828  ORDER BY `placecomments`.`id` ASC LIMIT 30)

MySQL.processChildren.instructions: 

{ 
  parent: 'places',
  parentKey: 'id',
  child: 'placecomments',
  childKey: 'place_id',
  select: [ 'id', 'comment', 'createdAt', 'updatedAt', 'place_id' ],
  alias: 'comments',
  removeParentKey: false,
  model: false,
  collection: true,
  criteria: { where: {}, limit: 30, sort: { id: 1 } } 
}
^^^^^^^^^^^^^^^^^^^^MySQL.processChildren.result^^^^^^^^^^^^^^

I dont know if it helps but additional info always need. PS: for this solution is don't using a columnNames, and it should works if get error do few restarts and it will remove all data from tables. To fix problem with migrate: "alter" refer here

atiertant commented 8 years ago

i don't think the real problem is in sequel, i think adapter should not receive other thing than columnName by waterline...the bug should be report in waterline repo because all adapter should have the same problem if they don't play with attributes ...