Calling populateAll() on a collection

[sbohan]

Hi Team,

First off would like to say thanks for creating a great library, it's been a real pleasure playing with it over the past day or two and has greatly sped up our development cycle for a small project.

We hit a problem yesterday and I would appreciate the community's comments on where we went wrong or if it is indeed a waterline bug that was uncovered.

person.js:

var Person = Waterline.Collection.extend({
  identity: 'person',
  tableName: 'person',
  schema: true,
  connection: 'postgresConn',
  attributes: {

    firstName: {
      type: 'string',
      required: true
    },

    lastName: {
      type: 'string',
      required: true
    },

    languages: {
      collection: 'language',
      via: 'speakers',
      dominant: true
    }
}
});

language.js

var Language = Waterline.Collection.extend({

  identity: 'language',
  tableName: 'language',
  schema: true,
  connection: 'postgresConn',
  attributes: {

    name: {
      type: 'string',
      required: true
    },

    isoCode: {
      type: 'string',
      required: true
    },

    speakers: {
      collection: 'person',
      via: 'languages'
    }

  }
});

index.js

  server.route({
      method: 'GET',
      path: '/people',
      handler: function (req, res) {
        server.models.person.find().populateAll().exec(function(err, result) {
          if (err) return res({ err: err }, 500);
          res(result);
        });

      }
  });

The above code gives the following error:

Debug: internal, implementation, error 
    TypeError: Uncaught error: Cannot call method 'forEach' of undefined
    at Deferred.populateAll (REDACTED/node_modules/waterline/lib/waterline/query/deferred.js:76:30)
    at server.route.handler (REDACTED/src/routes/crud.js:10:22)
    at Object.internals.handler (REDACTED/node_modules/hapi/lib/handler.js:94:36)
    at REDACTED/node_modules/hapi/lib/handler.js:28:23
    at internals.Protect.run (REDACTED/node_modules/hapi/lib/protect.js:56:5)
    at exports.execute (REDACTED/node_modules/hapi/lib/handler.js:22:22)
    at REDACTED/node_modules/hapi/lib/request.js:321:13
    at iterate (REDACTED/node_modules/hapi/node_modules/items/lib/index.js:35:13)
    at done (REDACTED/node_modules/hapi/node_modules/items/lib/index.js:27:25)
    at REDACTED/node_modules/hapi/node_modules/hoek/lib/index.js:781:22

However when we swap the find models line to the below, the languages relation is fully populated with the correct data.

server.models.person.find().populate('languages').exec(function(err, result) {

Digging a little deeper into the error we realised the populateAll() method was trying to access an associations property that didn't exist on context and so we replaced the code with this and all seems to be fine:

Deferred.prototype.populateAll = function(criteria) {
  var self = this;
  var associations = Object.keys(new this._context._model().associations);
  associations.forEach(function(association) {
    self.populate(association, criteria);
  });
  return this;
};

I would like to know if when given the collection definitions above the populateAll()method should work out of the box and if we have missed something at definition time that is stopping it working. If the definition is fine has anybody else encountered populateAll() not working as expected and if so, shall we submit a pull request? Not sure if we have broken another usage of populateAll() that we were not aware of.

[jirojo2]

Check line 349 of sails file: /lib/hooks/orm/index.js

Ill paste the block here:

    /**
     * prepareModels
     *
     * @param {Function}  cb
     *              -> err  // Error, if one occurred, or null
     *
     * @param {Object}    stack
     *            stack.instantiatedCollections {}
     */
    prepareModels: function (cb, stack) {
      var collections = stack.instantiatedCollections.collections || [];

      Object.keys(collections).forEach(function eachInstantiatedCollection (modelID) {

        // Bind context for models
        // (this (breaks?)allows usage with tools like `async`)
        _.bindAll(collections[modelID]);

        // Derive information about this model's associations from its schema
        var associatedWith = [];
        _(collections[modelID].attributes).forEach(function buildSubsetOfAssociations(attrDef, attrName) {
          if (typeof attrDef === 'object' && (attrDef.model || attrDef.collection)) {
            var assoc = {
              alias: attrName,
              type: attrDef.model ? 'model' : 'collection'
            };
            if (attrDef.model) {
              assoc.model = attrDef.model;
            }
            if (attrDef.collection) {
              assoc.collection = attrDef.collection;
            }
            if (attrDef.via) {
              assoc.via = attrDef.via;
            }

            associatedWith.push(assoc);
          }
        });

        // Expose `Model.associations` (an array)
        collections[modelID].associations = associatedWith;


        // Set `sails.models.*` reference to instantiated Collection
        // Exposed as `sails.models[modelID]`
        sails.models[modelID] = collections[modelID];

        // Create global variable for this model
        // (if enabled in `sails.config.globals`)
        // Exposed as `[globalId]`
        if (sails.config.globals && sails.config.globals.models) {
          var globalName = sails.models[modelID].globalId || sails.models[modelID].identity;
          global[globalName] = collections[modelID];
        }
      });

      cb();
    }
  };

Looks like you have to expose the associations yourself, but it would be nice if waterline handles this

[bengillies]

So it looks like this can be fixed by explicitly stating the associations in the model above as follows:

var Person = Waterline.Collection.extend({
  identity: 'person',
  tableName: 'person',
  schema: true,
  connection: 'postgresConn',
  attributes: {
    firstName: {
      type: 'string',
      required: true
    },
    lastName: {
      type: 'string',
      required: true
    },
    languages: {
      collection: 'language',
      via: 'speakers',
      dominant: true
    }
  },
  associations: [{
    alias: 'languages'
  }]
});

This seems to work, but also seems kind of janky. I suppose either the docs should be updated or waterline should handle this automatically.

[sailsbot]

Thanks for posting, @sbohan. I'm a repo bot-- nice to meet you!

It has been 60 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message. On the other hand, if you are still waiting on a patch, please:

• review our contribution guide to make sure this submission meets our criteria (only verified bugs with documented features, please; no questions, commentary, or bug reports about undocumented features or unofficial plugins)
• create a new issue with the latest information, including updated version details with error messages, failing tests, and a link back to the original issue. This allows GitHub to automatically create a back-reference for future visitors arriving from search engines.

Thanks so much for your help!

[kamek-pf]

I'm experiencing this too.
Any particular reason @sbohan's fix was never merged ? It definitely looks like a better option than tweaking the schemas.
I don't mind sending a PR if his solution looks acceptable.

[davemackintosh]

Could this be re-opened please? It's still an issue that requires a manual fix in Sails apps and consumers of Waterline. @sailsbot