Minor fixes

[fabien]

Allow data to be empty - otherwise MongoDB (2.6+) will throw; handle 'noop' updateAttributes gracefully.

Example use-case:

Model.observe('before save', function(ctx, next) {
  if (ctx.data) ctx.data = _.omit(ctx.data, 'reserved', 'property', 'example');
  // ctx.data might end up empty now - and Mongo will blow up
  next();
});

Fix: return proper error object, not string.

/cc @raymondfeng @bajtos

[bajtos]

@fabien Please add a unit-test to verify the fix and prevent regressions in the future.

[bajtos]

I'll let @raymondfeng to give a final approval, he is more familiar with this module than I am.

[bajtos]

Please add a unit-test to verify the fix and prevent regressions in the future.

In fact, it would be best to add the test to juggler, so that all connectors are fulfilling this contract. That makes me wonder, why are you fixing the problem here and not in juggler's lib/dao.js instead?

[fabien]

I'm fixing it here because AFAIK only MongoDB will complain when there's nothing to actually update. And DAO should not actually treat this as a failure, meaning: the after save hook should still run.

[fabien]

@bajtos please don't drag this tiny fix out into an extended discussion, there's no harm in creating a separate issue regarding any other connectors and such. We (and I) can't cover everything at once, and this is currently something that affects MongoDB in particular. Let's take this into consideration, land this PR and check with the other connectors and Juggler tests later.

[bajtos]

I'll leave the decision to @raymondfeng. As long as there is a unit-test for the change, I can live with what's proposed here.

We (and I) can't cover everything at once, and this is currently something that affects MongoDB in particular. Let's take this into consideration, land this PR and check with the other connectors and Juggler tests later.

The problem with your proposed approach is that you end up with very inconsistent behaviour across connectors. LoopBack's basic promise is that you can swap connectors/databases and the application should keep working as long as it's using the standard API only. When we land changes to individual connectors which should have been implemented globally in juggler for all connectors, then we slowly build up a technical debt that will cost us dearly to repay in the future. It's questionable whether benefits of such contribution outweigh the cost of future cleanup.

That's the problem in general, I am not saying that it must necessarily apply to your patch too. As I said, I'll leave the final decision to @raymondfeng.

[bajtos]

Also in my experience, later = never, because things that are not important enough at the implementation time will very rarely become important in the future.

[ritch]

land this PR and check with the other connectors and Juggler tests later

IMO this is fine if you create an issue that captures the remaining work (like what is quoted above) and assign that issue to someone (most likely yourself).

I think the point is a PR like this is part of a larger task... it shouldn't spawn additional work. Especially if is a one line change. Thats how PRs get to PR-pergatory where they never get completed, never merged, never closed, etc.

I guess what I'm advocating for is a clear distinction between PRs and issues... PRs shouldn't be the breading ground for additional TODOs outside the scope of the original PR. Just create an issue instead and link it to the PR.

[fabien]

@raymondfeng thoughts? I bumped into this again today.

[raymondfeng]

Please wrap the return with process.nextTick.

[raymondfeng]

Sorry for the delay. I'm good to merge it after fixing the issue I commented.

[fabien]

Closing with #254 as a resolution.