Populate should throw upon detecting an ORM result.

[eddyystop]

A substantial percentage of populate issues come down to the DB not being confirmed to return plain JS objects. Mongoose and Serialize return ORM's particular to their DB.

The problem is not solved by having populate automatically convert these ORM's to JS objects because all hooks except JS objects. We could change get-items.js to convert the ORM's but Feathers in general expects JS objects.

We have decided its more generic to change the defaults for Mongoose and Sequelize to return JS objects, so a user has to explicitly op-out to return an ORM. I'm not sure if this change has been pushed yet but its not prevelant in the field.

To save developers' and core-members' time, populate should check for ORM results and throw an explanatory message.

[DesignByOnyx]

Hey, I have figured out the issue, and I would like to voice my findings (and some opinions) on this:

Most importantly (strong opinion) - I do not think we should force users to use raw: true for their ORM. This pretty much defeats the purpose of the ORM and I'd rather feathers drop support for ORMs before forcing users to use the raw option. I say this as a user of Sequelize where my before and after hooks use the ORM and all its goodness.

Second most importantly - the raw option doesn't even work for feathers-sequelize. The data still comes back as an ORM object. While this is likely a bug with feathers-sequelize, I don't think feathers-sequelize users should ever have to set raw:true at a global level (same reasons as above). This should be a service-level decision, and it's a very simple hook users can implement manually.

// sequelize-raw-hook.js
function (hook) {
    hook.params.sequelize = Object.assign({}, hook.params.sequelize, { raw: true });
    return Promise.resolve(hook);
}

The solution - the issue the ORM folks have been experiencing is solved by checking if an object implements toJSON - and if so, calling it. The toJSON method, while not part of any official ECMA spec, is well documented on MDN and fairly widely used (Sequelize, Mongoose, Backbone, custom code I've written in the past, I'm sure many others). A quick smoke test in Chrome, Firefox, Safari, and Node 0.12+ reveals that JSON.stringify will call toJSON if implemented, so I think it's very safe to use and is not ORM-specific.

var obj = {};
obj.toJSON = function () { return "hello"; };
JSON.stringify(obj); //-> "hello"

I will be submitting a PR to fix this issue. Hopefully it makes it :)

[eddyystop]

The problem with ORMs is not limited to populate. No hook (the legacy populate being an exception) can handle ORMs. No other part of Feathers can handle ORMs. So solving the problem in populate does not solve the problem in general.

We could change the hook utility get-items.js if we wanted to convert ORMs to plain JS objects throughout the hooks. But that would not solve the problem generally for Feathers.

daffl and I have talked a bunch about this issue and the conclusion was that feathers-sequelize and feathers-mongoose should default to raw:true and 'lean:true`. That way hooks work for people who are not experts, and others can override the default and know what to do.

I'm not sure if this has made it into Auk yet.

I would talk this over with @daffl before submitting a PR.

[DesignByOnyx]

Ah, this makes sense - thanks for pointing out what I was not seeing. Then maybe I would flip my stance on this. The thing that doesn't really sit well with me is asking users to use raw: true in their config. Failure to do so causes errors and lots of other unforeseen issues. So with this I totally agree that "raw" queries must be made. I would like to make a case for feathers doing that for users inside the adapter so that all queries to the DB are always "raw" (eg. it's not even configurable). This has several benefits:

• Users don't have to think about it or know about
• Most performant query mechanism comes "on" by default (no overhead of ORM instantiation)
• Ensures that all feathers hooks and 3rd party hooks work out of the box

Then users can opt into ORM instantiation. The middleware is quite simple and could even ship with the adapters for ease of use:

// feathers-sequelize/instantiate-hook.js
function (hook) {
    if (!hook.type !== 'after') throw new Error('Instantiate hook must be used in "after" phase');
    if (hook.result.data) {
        hook.result.data = hook.result.data.map(item => this.Model.build(item));
    } else {
        hook.result = this.Model(hook.result);
    }
    return hook;
}

[daffl]

That's exactly what we did already for Mongoose so it would only be consistent to also do it for Sequelize. Feathers expects plain objects and almost every issue that comes up in hooks is either a misuse of Promises or an ORM model acting strangely.

[daffl]

Related #150

[eddyystop]

Implemented in #169 now that @DesignByOnyx 's PRs have been merged which result in feathers-sequelize return POJO by default.

[ricardopolo]

This means the best practice is not set raw?
Even more, raw is false by default in Sequalize or how can implement it for all of our solution and not hook by hook?

[eddyystop]

Configure raw mode when you configure the service. The sequelize adapter has some helper funcs to switch back to ORM only when you need to.