balderdashy / waterline-sequel

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

Population is broken for models which contains a many-to-* relationship in version 0.6.0 #83

Closed Bazze closed 8 years ago

Bazze commented 8 years ago

When upgrading to use version 0.6.0 (from 0.5.7) some of my populations stopped working. I started digging and it seems to me that instead of doing SELECT * FROM in populate queries, all fields are now explicitly stated instead of the * (https://github.com/balderdashy/waterline-sequel/commit/35c23bfbc54ce8509e2ad175fd4fbe4d51c85336).

This breaks stuff since my model can have an attribute that is a collection of another model. An attribute that is a collection of another model doesn't have a column in the table with that name causing the query to include column names in the select query that doesn't exist. Maybe this is also somehow related to changing from using attributes to definition?

My model setup:

// MainModel
module.exports = {
  tableName: 'custom_tbl_name',
  attributes: {
    subject: {
      model: 'Subject',
    },
    childModelOne: {
      collection: 'ChildModelOne',
      via: 'mainModel',
    },
    childModelTwo: {
      collection: 'ChildModelTwo',
      via: 'mainModel',
    },
  },
}

// ChildModelOne
module.exports = {
  tableName: 'custom_tbl_name2',
  attributes: {
    mainModel: {
      model: 'MainModel',
      via: 'childModelOne',
    },
    childModelThree: {
      model: 'ChildModelThree',
    },
  },
}

// ChildModelTwo
module.exports = {
  tableName: 'custom_tbl_name3',
  attributes: {
    mainModel: {
      model: 'MainModel',
      via: 'childModelTwo',
    },
    childModelFour: {
      model: 'ChildModelFour',
    },
    childModelFive: {
      collection: 'ChildModelFive',
      via: 'childModelTwo',
    }
  },
}

When doing...

MainModel
  .find({where: {subject: subject.id}})
  .populate('childModelOne')
  .populate('childModelTwo')

...childModelOne populates fine while childModelTwo is just returned as an empty array (which it didn't in v0.5.7).

sailsbot commented 8 years ago

@Bazze 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!

particlebanana commented 8 years ago

@Bazze looking into this as it's been reported a few places. The latest versions of the adapters should have been backwards compatible with Waterline 0.11 but it looks like we need to add some additional checks.

The removal of the * was part of a feature addition to Waterline 0.12 to support projections using the select key.

particlebanana commented 8 years ago

Also @Bazze maybe I'm a bit lost but what in your definition is different between populating childModelOne and childModelTwo? Can you create an example project I can clone that represents the issue?

Also just as as side note using via on a model attribute isn't supported or valid.

Bazze commented 8 years ago

@particlebanana: The difference between populating childModelOne and childModelTwo is that childModelTwo includes an attribute that is referencing a collection of ChildModelFive. This reference is not an actual column existing in the childModelTwo table but is included in the select query as it was. I'll try to get an example project up during the day.

Thanks for the info on via, I'll clean it out!

Bazze commented 8 years ago

I tried creating a demo app but it doesn't even lift with the latest 0.6.0 version, probably related to #84.

$ sails lift

info: Starting app...

/usr/local/lib/node_modules/sails/node_modules/waterline/node_modules/waterline-schema/lib/waterline-schema/utils.js:47
  return hop.call(obj, prop);
             ^

TypeError: Cannot convert undefined or null to object
    at hasOwnProperty (native)
    at exports.object.hasOwnProperty (/usr/local/lib/node_modules/sails/node_modules/waterline/node_modules/waterline-schema/lib/waterline-schema/utils.js:47:14)
    at JoinTables.parseAttribute (/usr/local/lib/node_modules/sails/node_modules/waterline/node_modules/waterline-schema/lib/waterline-schema/joinTables.js:148:26)
    at /usr/local/lib/node_modules/sails/node_modules/waterline/node_modules/waterline-schema/lib/waterline-schema/joinTables.js:83:22
    at Array.forEach (native)
    at JoinTables.buildJoins (/usr/local/lib/node_modules/sails/node_modules/waterline/node_modules/waterline-schema/lib/waterline-schema/joinTables.js:82:24)
    at new JoinTables (/usr/local/lib/node_modules/sails/node_modules/waterline/node_modules/waterline-schema/lib/waterline-schema/joinTables.js:40:23)
    at new module.exports (/usr/local/lib/node_modules/sails/node_modules/waterline/node_modules/waterline-schema/lib/waterline-schema.js:33:17)
    at Waterline.initialize (/usr/local/lib/node_modules/sails/node_modules/waterline/lib/waterline.js:107:17)
    at buildORM (/usr/local/lib/node_modules/sails/lib/hooks/orm/build-orm.js:52:15)
    at Array.async.auto.instantiatedCollections (/usr/local/lib/node_modules/sails/lib/hooks/orm/index.js:203:11)
    at listener (/usr/local/lib/node_modules/sails/node_modules/async/lib/async.js:493:46)
    at /usr/local/lib/node_modules/sails/node_modules/async/lib/async.js:444:17
    at Array.forEach (native)
    at _each (/usr/local/lib/node_modules/sails/node_modules/async/lib/async.js:46:24)
    at Immediate.taskComplete (/usr/local/lib/node_modules/sails/node_modules/async/lib/async.js:443:13)
Bazze commented 8 years ago

Nevermind my previous comment. I think I screwed something up. I'll get back when I've got it sorted.

Bazze commented 8 years ago

@particlebanana: Thanks for your quick action. Unfortunately though, #84 doesn't solve my issue. I've created a demo project to demonstrate it, have a look here: https://github.com/Bazze/waterline-sequel-issue-83. Reproduce steps are described in the README.

particlebanana commented 8 years ago

@Bazze thanks for the repo. I went ahead and set the latest tag on npm for sails-mysql back to 0.11.5 while we work this out.

particlebanana commented 8 years ago

Ok @Bazze I submitted #85 which hopefully fixes this up once and for all. I tested it on your example repo and it worked. If you want to give it a go that would be great.

Bazze commented 8 years ago

Thanks @particlebanana. This fix works on the demo repo and on my own project, so I would consider this solved. Good job! :smiley:

particlebanana commented 8 years ago

Published 0.6.2 and will be rolling out updates to the sql adapters shortly. Thanks everyone!