mdconaway/sails-ember-rest

Validations error returns 500 code

Closed this issue · 6 comments

templates/actions/create.js

Model.create(data).exec((err, newInstance) => {
            // Differentiate between waterline-originated validation errors
            // and serious underlying issues. Respond with badRequest if a
            // validation error is encountered, w/ validation info.
            if (err) return res.serverError(err, actionUtil.parseLocals(req));

It seems the comment discusses handling this but it is not handled.

Yes, this is indeed an issue!

So here is the long version of how this came to be:

In the older versions of sails.js (0.11.x, 0.12.x) there was a nice response method called res.negotiate, and it was used in the old create action:

if(err) return res.negotiate(err, actionUtil.parseLocals(req));

This method would handle exactly what is mentioned in the comments.

In sails 1.0.x+ The res.negotiate response method no longer exists, so this library took a shortcut to make things functional by simply replacing negotiate() with serverError()

Going forward, I have been debating with myself and the rest of my team whether or not this library should automatically re-insert the negotiate() response back into a sails 1.0 project, or whether we should abstract the negotiate() logic into its own utility function that can be used throughout sails-ember-rest without adding a new response method to a dependent application's api/response folder.

Seeing as you are the first person to comment on this particular issue, I would say that means it would prudent to gather your opinion on how you as a consumer of this library would like the problem solved.

Would you like to see the negotiate response added back into a sails 1.0 app running this library or do you think a utility method as part of actionUtil.js makes more sense?

(Also, I am very open to pull requests to solve this one)

Thanks for the fast response. Great work on this library! I am working on my first Sails 1.x.x app, I used sails-generate-ember-blueprints in previous apps for Sails 0.x.x, this library is crucial to the upgrade.

I see what you are saying. I kind of think the best solution might just be to copy what Sails 1.x.x does. Meaning change this:

if (err) return res.serverError(err, actionUtil.parseLocals(req));

To this:

if (err) {
  switch (err.name) {
    case 'AdapterError':
      switch (err.code) {
        case 'E_UNIQUE': return res.badRequest(err, actionUtil.parseLocals(req));
        default: return res.serverError(err, actionUtil.parseLocals(req));
      } return;
    case 'UsageError': return res.badRequest(err, actionUtil.parseLocals(req));
    default: return res.serverError(err, actionUtil.parseLocals(req));
  }
}

This would be done in every action so there is some code duplication but it isn't too much. I think keeping it as similar to the Sails approach as possible helps make maintenance easier. I copied this from here (https://github.com/balderdashy/sails/blob/master/lib/hooks/blueprints/actions/create.js) and just added actionUtil.parseLocals(req) to the response calls. Personally I would have used an if block instead of switch but I digress.

What do you think of this solution?

We (@TransNexus) use Sails/Ember a lot. I am happy to help contribute to this library.

What do you think of this solution:
f4b53e6

@mdconaway I like it!

Great! Well 1.0.8 has been pushed to NPM, so I will close this issue. Let me know if you guys have any good ideas you would like to see implemented in this library, as I always enjoy making it better. Also, you may be interested in the new README section added earlier today:

https://github.com/mdconaway/sails-ember-rest#controller

It talks a bit about a whole new lifecycle pattern that only exists in this library.

@fenichelar

@mdconaway The readme looks good! I read through the code last week to figure it out. I really like the interrupts!